fix: correct sp_384/sp_521 div quotient when borrow hides in t1[0]#10257
Open
MarkAtwood wants to merge 1 commit intowolfSSL:masterfrom
Open
fix: correct sp_384/sp_521 div quotient when borrow hides in t1[0]#10257MarkAtwood wants to merge 1 commit intowolfSSL:masterfrom
MarkAtwood wants to merge 1 commit intowolfSSL:masterfrom
Conversation
sp_521_div_9 (sp_c64.c), sp_521_div_21 (sp_c32.c), sp_384_div_7 (sp_c64.c), and sp_384_div_15 (sp_c32.c) all use a quotient-correction loop that at i=0 calls sp_N_norm_N(&t1[1]), skipping t1[0]. A borrow hiding in t1[0] is invisible to the correction mask, resurfaces in the top limb after the outer norm, and yields a quotient one too small and a remainder p too large. Fix: after the outer norm, test the top limb sign and add sd back once more when negative, then renormalise. sp_256_div uses a two-pass subtraction strategy and is unaffected. Reproducer: Wycheproof secp521r1 ECDH tcId 55 returns the wrong shared secret without this fix.
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes an off-by-one quotient correction bug in the special-prime division routines for P-384 and P-521 when a borrow is “hidden” in t1[0] during the correction loop.
Changes:
- Adds a post-normalization correction pass in
sp_384_div_7andsp_521_div_9(64-bit backend). - Adds the same post-normalization correction pass in
sp_384_div_15andsp_521_div_21(32-bit backend).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| wolfcrypt/src/sp_c64.c | Adds an extra sign-based correction step after the final normalization in P-384/P-521 div routines (64-bit). |
| wolfcrypt/src/sp_c32.c | Adds the same extra correction step after the final normalization in P-384/P-521 div routines (32-bit). |
Comments suppressed due to low confidence (5)
wolfcrypt/src/sp_c64.c:1
maskas computed here will be0or1whensp_digitis an unsigned type, sosd[i] & maskonly preserves the LSB instead of conditionally adding the whole limb. To keep this constant-time and correct, build a full-width mask (all-zeros or all-ones) from the MSB (e.g., by negating the extracted bit) before ANDing withsd[i].
wolfcrypt/src/sp_c64.c:1- Same issue as in
sp_384_div_7: ifsp_digitis unsigned, this produces a 0/1 mask and will only add the LSB of eachsd[i]. Use a full-width all-ones/all-zeros mask derived from the MSB to make the conditional add correct and constant-time.
wolfcrypt/src/sp_c32.c:1 - Here
maskwill evaluate to0or1under an unsignedsp_digit, sosd[i] & maskdoes not conditionally add the full limb. Convert the extracted sign bit into a full-width mask (0 or ~0) before applying it tosd[i].
wolfcrypt/src/sp_c32.c:1 - Same masking issue as above: as written, this produces a 0/1 mask for unsigned
sp_digitand will only add 1-bit fromsd[i]. Use a full-width mask derived from the sign bit sosd[i]is either fully added or not added.
wolfcrypt/src/sp_c64.c:1 - The comment refers to a “correction mask that checks
t1[7]” but the added correction usest1[6]as the sign/borrow indicator. This mismatch makes the rationale harder to follow; consider rewording to consistently describe which limb is checked in the main loop vs. which limb becomes negative after the outer normalization (or refer to “top/extra limb” instead of hard-coded indices).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
sp_521_div_9(sp_c64.c),sp_521_div_21(sp_c32.c),sp_384_div_7(sp_c64.c), andsp_384_div_15(sp_c32.c) all share the same quotient-correction loop structure. At iterationi=0, the loop callssp_N_norm_N(&t1[1]), which normalises limbs[1..N]but skipst1[0]. A borrow that exists only int1[0]after the subtraction is therefore invisible to the correction mask (which tests the sign oft1[N]). After the outersp_N_norm_N(t1)at the end of the function, the hidden borrow resurfaces in the top limb, yielding a quotient one too small and a remainder that is one multiple of the divisor too large.Fix: after the outer norm, test the sign of the top limb and add
sdback once more when negative, then renormalise.sp_256_divuses a two-pass subtraction strategy and is unaffected.Reproducer: Wycheproof secp521r1 ECDH
tcId 55(EdgeCaseEphemeralKey,x² ≡ -3 mod p) returns the wrong shared secret without this fix.Test plan
/cc @wolfSSL-Fenrir-bot please review