Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clarify cryptographic magic numbers #4

Open
wants to merge 3 commits into
base: audit_findings
Choose a base branch
from

Conversation

tarakby
Copy link

@tarakby tarakby commented Jun 6, 2024

  • declare new cryptographic constants related to sha3 and the supported elliptic curves.
  • use the cryptographic constants to replace magic numbers

The purpose is:

  • clarify the intention of using magic numbers (especially when the same constant 32 is used multiple times, but it has a different meaning depending on the context)
  • make the code base more robust if more hashing algorithms or elliptic curves are used in the future

@tarakby tarakby mentioned this pull request Jun 6, 2024
@relatko relatko force-pushed the audit_findings branch 2 times, most recently from 4dff0a0 to e10d732 Compare June 6, 2024 17:54
src/crypto.c Outdated
@@ -185,7 +182,7 @@ zxerr_t crypto_sign(const hd_path_t path,
sizeof(messageDigest),
&messageDigestSize));

if (messageDigestSize != CX_SHA256_SIZE) {
if (messageDigestSize != CURVE_ORDER_SIZE) {
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. explicitly check that the hashing worked as expected, as in the digest size is equal to the expected hash output constant
  2. 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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

check added 1e5abf8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant