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

Expose EC group order #10600

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

DavidBuchanan314
Copy link
Contributor

Here's a patch to expose the group order of EllipticCurve objects. As mentioned on IRC, this makes it easier to write code to enforce "Low-S" ECDSA signatures without hard-coding parameters (but does not itself implement anything of the sort).

Assuming I cleaned this up and added a test or two, is this something you'd consider merging?

@alex
Copy link
Member

alex commented Mar 18, 2024

I have not had a chance to review this in any detail, but I wanted to flag that we are (slowly) migrating away from cffi, so new PRs shouldn't be introducing any new usages of it.

@reaperhulk
Copy link
Member

reaperhulk commented Mar 18, 2024

If we want to add group order is there any reason to do it dynamically or should we just add it as a class constant on the curve classes?

@alex
Copy link
Member

alex commented Mar 18, 2024 via email

@DavidBuchanan314
Copy link
Contributor Author

DavidBuchanan314 commented Mar 21, 2024

The main reason I didn't just add them as constants is because it's kinda hard to demonstrate their correctness/legitimacy. Someone has to hard-code them somewhere (or calculate from first principles I suppose), and since openssl has already done that (in openssl/crypto/ec/ec_curve.c) I thought it'd be best to piggyback off that effort.

But, if you're happy to just have them as constants, then that works for me.

As for testing for correctness, it just occurred to me that an easy test would be to generate a signature, replace s with group_order-s, and check that the signature is still valid.

@reaperhulk
Copy link
Member

I think we're fine with constants -- we can check them against authoritative sources as part of review 😄

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

Successfully merging this pull request may close these issues.

None yet

3 participants