Repository navigation
Reduce the base point in ecc_point_to_mont() when the normalizer is 1 - #11913
Conversation
ecc_point_to_mont() converts the base point to Montgomery form for wc_ecc_mulmod_ex() and wc_ecc_mulmod_ex2(). When the Montgomery normalizer is 1 it copied the ordinates instead of multiplying them, so they were not reduced modulo the prime. SP int uses R = 2^bits(m), so for a prime of the form 2^n - 1 the normalizer is 1. P-521's prime is 2^521 - 1, so with SP int math a P-521 base point with an ordinate at or above the prime, or a negative ordinate, went straight into the point arithmetic, which needs values below the modulus. wc_ecc_mulmod() then returned MP_VAL for y + p for every scalar and for negative ordinates, and a wrong, unreduced point for x + p with some scalars. Other curves, and fastmath, take the mp_mulmod() branch, which reduces. Reduce the ordinates with mp_mod() when the normalizer is 1, and add test_wc_ecc_mulmod_unreduced, which checks P-521 with each ordinate increased by the prime and, where negative values are supported, decreased by it.
|
@claude review |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #11913
Scan targets checked: wolfcrypt-src, wolfcrypt-bugs, wolfssl-bugs
Not reviewed (no scan target covers them): tests/api/test_ecc.h
Coverage: 2 of 2 in-scope changed file(s) opened by the reviewer
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Lite
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The fix is small, but it changes core ECC point arithmetic in wolfcrypt, so a maintainer familiar with the math backends should still look at it.
What was reviewed:
- The
mu == 1branch ofecc_point_to_mont()inwolfcrypt/src/ecc.c: x, y and z are now reduced withmp_mod(), which matches what themp_mulmod()branch already does for other curves and fastmath. - The new
test_wc_ecc_mulmod_unreduced: its feature-macro guards, the extra negative-ordinate cases that only run when negative mp_ints are supported, and theFP_ECCcache clearing before each call. - A possible test failure on 64-bit builds that use SP P-521 code and support negative mp_ints was looked into and ruled out.
Extended reasoning...
The change replaces three mp_copy calls with mp_mod in the Montgomery-normalizer-equals-1 branch of ecc_point_to_mont() in wolfcrypt/src/ecc.c. It also adds one P-521 regression test and registers it in tests/api/test_ecc.h. The library change is small and follows the reduction the other branch already does. It does touch crypto: the ECC scalar multiplication path used for public-key operations. Because it changes core crypto behavior across several math backends (SP int, fastmath, SP asm), I'm deferring to a human rather than approving.
|
douzzer
added
the
For FIPS v7 Module
|
tests good with multi-test with |
Description
ecc_point_to_mont()converts the base point to Montgomery form forwc_ecc_mulmod_ex()andwc_ecc_mulmod_ex2(). When the Montgomery normalizer is 1, it copied the ordinates instead of multiplying them, so they were not reduced modulo the prime.SP int uses R = 2^bits(m), so for a prime of the form 2^n - 1 the normalizer is 1. P-521's prime is 2^521 - 1, so with SP int math a P-521 base point with an ordinate at or above the prime, or a negative ordinate, went straight into the point arithmetic, whose add/sub/reduce operations require values below the modulus. Other curves, and fastmath, take the
mp_mulmod()branch, which reduces.Results for
wc_ecc_mulmod()on P-521 before this change:MP_VALfor every scalar (the ordinate outgrows the temporaries during doubling).WOLFSSL_SP_INT_NEGATIVE):MP_VAL.This reduces the ordinates with
mp_mod()when the normalizer is 1, as the other branch effectively does. WithFP_ECC,build_lut()always usesmp_mulmod(), so the cached table hid this after the first use of a point.Found while adding a test for #11912, whose
test_wc_ecc_mulmod_curvescurrently checks an unreduced z only because of this; once this is in, it can test unreduced x and y too.Testing
test_wc_ecc_mulmod_unreduced: multiplies the P-521 base point with each ordinate increased by the prime (and decreased, where negative values are supported) using a fixed scalar that showed the x failure, and compares with the result for the reduced point. It fails without the fix (x: wrong point; y and negative ordinates:MP_VAL)../tests/unit.test --api --group eccandtestwolfcryptpass, on macOS arm64, with:--disable-sp --enable-ecccustcurves=all CPPFLAGS=-DWOLFSSL_PUBLIC_MP--enable-sp=ec256 --disable-sp-asm --enable-ecccustcurves=all CPPFLAGS=-DWOLFSSL_PUBLIC_MP--enable-sp=ec256,ec384,ec521 --enable-sp-asm --enable-ecccustcurves=all CPPFLAGS=-DWOLFSSL_PUBLIC_MP--enable-fastmath --enable-sp=ec256 --disable-sp-asm --enable-ecccustcurves=all CPPFLAGS=-DWOLFSSL_PUBLIC_MP--enable-all --enable-dilithiumwc_ecc_mulmod()with unreduced x, y and z on P-256/384/521, secp256k1 and the Brainpool curves (random scalars, fixed-point cache cleared each time): 0 failures in 2000 iterations each with the fix; without it, unreduced y on P-521 failed 2000/2000 and unreduced x 5/2000.Checklist