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

Implement NIST P521 signature verification #1631

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

Conversation

vkrasnov
Copy link
Contributor

@vkrasnov vkrasnov commented Sep 1, 2023

Need a way to work on the 32bit part, as 521 is kinda awkward in this regard, because number of limbs in 32 bit mode is not double that of 521 mode.

@briansmith
Copy link
Owner

Vlad, my understanding is that you have it working for 64-bit but not 32-bit targets. is that right? Happy to have a chat with you about how to resolve the 32-bit issues.

@vkrasnov
Copy link
Contributor Author

vkrasnov commented Oct 3, 2023

Yes, this is correct

@vkrasnov vkrasnov force-pushed the vlad/p521_verify branch 8 times, most recently from 8440c57 to 4e94c35 Compare October 6, 2023 17:48
@codecov
Copy link

codecov bot commented Oct 6, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ee5db43) 96.01% compared to head (561a613) 96.26%.

❗ Current head 561a613 differs from pull request most recent head 932beef. Consider uploading reports for the commit 932beef to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1631      +/-   ##
==========================================
+ Coverage   96.01%   96.26%   +0.24%     
==========================================
  Files         136      137       +1     
  Lines       20774    20764      -10     
  Branches      226      228       +2     
==========================================
+ Hits        19946    19988      +42     
+ Misses        793      742      -51     
+ Partials       35       34       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@briansmith
Copy link
Owner

Vlad, I just created a new branch b/generate-curve that contains the test case generator I used for P-256 and P-384 many years ago. This will help you generate the analogous tests for P-521.

I am still looking for the program I used to generate the tops of ops/p{256,384}.rs. I will share it with you when I find it.

@briansmith
Copy link
Owner

I am still looking for the program I used to generate the tops of ops/p{256,384}.rs. I will share it with you when I find it.

It seems like I do have that program, but it isn't going to help us. I will review your PR #1702 and come back to this one.

@vkrasnov
Copy link
Contributor Author

vkrasnov commented Oct 9, 2023

I copied the ec tests from boringssl, the ops tests could perhaps help understand why 32bit code fails, but my understanding that they are provided in montromery representation, which is different between 64 and 32 bits

@briansmith
Copy link
Owner

I copied the ec tests from boringssl, the ops tests could perhaps help understand why 32bit code fails, but my understanding that they are provided in montromery representation, which is different between 64 and 32 bits

I will generate the ops tests for P-521 so that they work for 32-bit and 64-bit targets and share them in a PR. This should let us identify the failure for 32-bit targets. I think we need to also do merge 1758 and rebase this on top of that, as I'm not confortable with the type punning we're doing, especially when BoringSSL is no longer doing it.

@vkrasnov vkrasnov force-pushed the vlad/p521_verify branch 2 times, most recently from 4a38b14 to 6b2b571 Compare October 24, 2023 15:46
src/ec/suite_b/ecdsa/digest_scalar.rs Outdated Show resolved Hide resolved
src/ec/suite_b/ops/p521.rs Outdated Show resolved Hide resolved
src/ec/suite_b/ops/p521.rs Outdated Show resolved Hide resolved
crypto/fipsmodule/ec/ecp_nistz.inl Outdated Show resolved Hide resolved
crypto/fipsmodule/ec/ecp_nistz.inl Show resolved Hide resolved
Copy link
Owner

@briansmith briansmith left a comment

Choose a reason for hiding this comment

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

Some more suggestions.

crypto/fipsmodule/ec/ecp_nistz.inl Show resolved Hide resolved
crypto/fipsmodule/ec/ecp_nistz.inl Show resolved Hide resolved
crypto/fipsmodule/ec/gfp_p384.c Outdated Show resolved Hide resolved
@briansmith
Copy link
Owner

Regarding the missing tests: I am working on changing the point arithmetic tests so that the test data files are NOT Montgomery-encoded. See PR #1769 and PR #1770.

Regarding generating tests for p521_elem_*, it looks like the Montgomery encoding issue only affects the multiplication tests. I guess I will do something similar to what I am doing in #1770 for those.

@briansmith
Copy link
Owner

Now that PR #1756 is merged, you can run python3 mk/generate_curves.py which will generate target/curves/p{256,384,521}.rs. When you update this PR, could you merge in the changes from what's generated into target/curves (using two-way merge) and/or change mk/generate_curves.py as needed? This will make it easier to review this as well. Simiarly, if you have code that generates gfp_521.c, it would be awesome to have that added to mk/generate_curves.py.

@yacinehmito
Copy link

yacinehmito commented Nov 21, 2023

@briansmith @vkrasnov Is there a way I can help getting this merged? (I really need this; see denoland/deno#21169).

I can take action on the review, but I am wary of doing this without asking first.

@tgross35
Copy link

tgross35 commented Nov 30, 2023

If it helps for reference, rustcrypto also very recently imlemented signing for P521 RustCrypto/elliptic-curves#953 RustCrypto/elliptic-curves#956

@vkrasnov vkrasnov marked this pull request as draft January 5, 2024 17:50
@vkrasnov
Copy link
Contributor Author

vkrasnov commented Jan 5, 2024

Converting to draft until #1881 and #1883 are merged

@briansmith
Copy link
Owner

The clippy failure is due to the newest Rust toolchain having a stricter clippy. If you rebase on top of main then you will get the fix, which was in #1888.

This change makes it easier to reuse the P384 code which is quite
generic already. No algorithmic changes are made, only some code
is shuffled around. This prepares the ground for P521 implementation.
This code almost entirely reuses the P384 code, and only implements
the dedicated inversion routines on top of the code generated by
generate_curves.py
@briansmith briansmith marked this pull request as ready for review January 25, 2024 21:07
@yacinehmito
Copy link

How can I help move this forward?

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.

4 participants