Clarify cryptographic magic numbers#4
Clarify cryptographic magic numbers#4tarakby wants to merge 3 commits intovacuumlabs:audit_findingsfrom
Conversation
4dff0a0 to
e10d732
Compare
src/crypto.c
Outdated
There was a problem hiding this comment.
In theory, it's enough to only check messageDigestSize < CURVE_ORDER_SIZE to error. messageDigestSize >= CURVE_ORDER_SIZE is the allowed ECDSA requirement, but that also depends on how bip32_derive_ecdsa_sign_hash_256 is implemented internally. In Flow, the supported hashing output was forced to match the curve order size, hence the check for equality here.
On a slightly different note, if bip32_derive_ecdsa_sign_hash_256 checks for the curve/digest compatibility internally, then this check can be reduced to checking that hashing has worked correctly, as in messageDigestSize is equal to CX_SHA256_SIZE or CX_SHA3_256_SIZE.
There was a problem hiding this comment.
After checking the doc of bip32_derive_ecdsa_sign_hash_256, I see that there is no check of compatibility of the group order/hash length!
The caller of bip32_derive_ecdsa_sign_hash_256 needs to make the check in this case. My preference in this case is to:
- explicitly check that the hashing worked as expected, as in the digest size is equal to the expected hash output constant
- explicitly check that the digest size is at least the curve group order (not checked by the Ledger SDK).
If we skip (2), the code may integrate for instance sha224 and use it to sign without an error. The security of the curve (128 bits) is silently reduced in this case to the security of the hash (112 bits), which is not good. Let me know if you have questions to clarify
|
replace by onflow#115 |
The purpose is:
32is used multiple times, but it has a different meaning depending on the context)