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

ECDSA when the hash length is greater than the group's order #17

Closed
rgeslain-ledger opened this issue May 21, 2020 · 5 comments
Closed

Comments

@rgeslain-ledger
Copy link
Contributor

The library does not support the case where, either for ECDSA sign or ECDSA verify, the hash length is greater than the group's order.

As an example, it can be (not the optimal way) implemented like this:

  • In ecdsa.py#_do_sign, right after msg = int.from_bytes(msg, 'big'):
nbyteslen = (n.bit_length() + 7) // 8
mbyteslen = (msg.bit_length() + 7) // 8

if nbyteslen < mbyteslen:
    # msg is the 'n.bit_length()' leftmost bits of the received msg.
    msg = int(hex(msg)[2:2*nbyteslen + 2], 16)
  • In ecdsa.py#verify, right after h = int.from_bytes(msg,'big'):
nbyteslen = (n.bit_length() + 7) // 8
hbyteslen = (h.bit_length() + 7) // 8
if nbyteslen < hbyteslen:
    # h is the 'n.bit_length()' leftmost bits of the received h value.
    h = int(hex(h)[2:2*nbyteslen + 2], 16)
@cslashm
Copy link
Owner

cslashm commented May 25, 2020

Thanks for reporting. You're right.
Feel free to make a PR, else I will work on it this week.

@rgeslain-ledger
Copy link
Contributor Author

Should be fixed by #18.

@matbok
Copy link

matbok commented Oct 16, 2020

Should be fixed by #18.

No, There is a bug if hash has at least 8 zero bits leading: indeed, in the #18 fix, it's checked the real bit length, not the byte length multiplied by 8 (to obtain the "full bytes" bit length), and mbyteslen must be len(msg) (before making it integer)

@cslashm
Copy link
Owner

cslashm commented Oct 16, 2020

hmmm I'll check against RFC and internal Ledger OS code

@cslashm
Copy link
Owner

cslashm commented Oct 26, 2020

should be fixed in 1.2.4 @matbok

@cslashm cslashm closed this as completed Oct 26, 2020
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

No branches or pull requests

3 participants