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 verification: Refactor a*g + b*p multiplication #1757

Merged
merged 4 commits into from
Oct 18, 2023
Merged

Conversation

briansmith
Copy link
Owner

Only the nistz256 implementation should be doing this multiplication in the naïve way. Even then, we should be using the optimized variable-time implementation from BoringSSL, so bring it in and use it.

After this PR, we can more easily replace twin_mul_inefficient with a proper twin multiplication implementation that works for any curve.

See the individual commit messages to see the changes made to the imported BoringSSL code; the second commit imports it unmodified, and the third commit makes the needed changes.

@briansmith briansmith self-assigned this Oct 17, 2023
@briansmith
Copy link
Owner Author

@vkrasnov PTAL.

@codecov
Copy link

codecov bot commented Oct 17, 2023

Codecov Report

Merging #1757 (107c046) into main (2a0e495) will increase coverage by 0.04%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1757      +/-   ##
==========================================
+ Coverage   95.93%   95.98%   +0.04%     
==========================================
  Files         138      138              
  Lines       20610    20691      +81     
  Branches      221      226       +5     
==========================================
+ Hits        19772    19860      +88     
+ Misses        800      792       -8     
- Partials       38       39       +1     
Files Coverage Δ
crypto/fipsmodule/ec/p256-nistz.c 96.73% <100.00%> (+0.84%) ⬆️
src/ec/suite_b/ecdsa/verification.rs 98.52% <100.00%> (-0.12%) ⬇️
src/ec/suite_b/ops.rs 98.08% <100.00%> (+0.03%) ⬆️
src/ec/suite_b/ops/p256.rs 100.00% <100.00%> (ø)
src/ec/suite_b/ops/p384.rs 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Allow each curve to provide its own `twin_mul` implementation. For now,
use the same implementation we've been using.
From BoringSSL commit 8d71d24.

Comment it out until it is modified to work.
@briansmith briansmith force-pushed the b/twin-mul-1 branch 3 times, most recently from 86f4976 to 3d04b83 Compare October 18, 2023 01:05
Import the optimized nistz256 verification from BoringSSL.
Move more of the logic for the nistz256 multiplication into Rust.
@briansmith briansmith merged commit 9a49f37 into main Oct 18, 2023
138 checks passed
@briansmith briansmith deleted the b/twin-mul-1 branch October 18, 2023 16:49
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