Skip to content
New issue

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

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

Already on GitHub? # to your account

fix: signature verification due to leading zeros #326

Merged
merged 1 commit into from
Oct 26, 2024

Conversation

indutny
Copy link
Owner

@indutny indutny commented Oct 25, 2024

According to FIPS 186-5, section 6.4.2 ECDSA Signature Verification Algorithm, the hash of the message must be adjusted based on the order n of the base point of the elliptic curve:

If log2(n) ≥ hashlen, set E = H. Otherwise, set E equal to
the leftmost log2(n) bits of H.

Unfortunately because elliptic converts messages to BN instances the reported byteLength() for the message can be incorrect if the message has 8 or more leading zero bits.

Here we fix it by:

  1. Counting leading zeroes in hex strings provided as messages
  2. Counting all array entries in Array-like (e.g. Buffer) messages
  3. Providing an msgBitLength option to both .sign/.verify to let user override the behavior

Original PR: #322
Credit: @Markus-MS

According to FIPS 186-5, section 6.4.2 ECDSA Signature
Verification Algorithm, the hash of the message must be adjusted
based on the order n of the base point of the elliptic curve:

    If log2(n) ≥ hashlen, set E = H. Otherwise, set E equal to
    the leftmost log2(n) bits of H.

Unfortunately because elliptic converts messages to BN instances the
reported `byteLength()` for the message can be incorrect if the message
has 8 or more leading zero bits.

Here we fix it by:

1. Counting leading zeroes in hex strings provided as messages
2. Counting all array entries in Array-like (e.g. Buffer)
   messages
3. Providing an `msgBitLength` option to both `.sign`/`.verify` to let
   user override the behavior

Original PR: #322
Credit: @Markus-MS
@paulmillr
Copy link

Who would use options.msgBitLength, and what for?

@indutny
Copy link
Owner Author

indutny commented Oct 25, 2024

@paulmillr thanks for the review! I just wanted to provide an escape hatch for people whom this patch broke 😭

@indutny indutny merged commit 34c8534 into master Oct 26, 2024
@indutny indutny deleted the fix/pr-322-alternative branch October 26, 2024 03:14
@indutny
Copy link
Owner Author

indutny commented Oct 26, 2024

Published in 6.6.0. Thank you!

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

Successfully merging this pull request may close these issues.

2 participants