Repository navigation
Conversation
|
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The new test’s curve-selection guards omit ECC_MIN_KEY_SZ, causing it to select excluded curves and fail in supported minimum-key-size configurations.
1 open issue
What changed in this PR
This PR prevents incorrect ECC multiplication caused by SP point-cache collisions by rejecting non-affine base points in SP-math-only builds with FP_ECC.
Changes:
- Return
ECC_BAD_ARG_Efor base points withz != 1in both multiplication entry points. - Add regression coverage for P-256, P-384, and P-521, including checks that subsequent affine multiplication remains correct.
- Register the new test in the ECC API test group.
| File | Description |
|---|---|
wolfcrypt/src/ecc.c |
Reject non-affine inputs in the affected multiplication paths. |
tests/api/test_ecc.h |
Declare and register the projective-point regression test. |
tests/api/test_ecc.c |
Test projective-point handling and subsequent affine results across supported curves. |
Review effort: Balanced
LinuxJedi
force-pushed
the
ecc-mulmod-sp-math-fpecc
branch
from
November 5, 2026 10:51
e2615b2 to
56387a1
Compare
SparkiDev
requested changes
Nov 6, 2026
| /* The SP code caches tables for points keyed on x and y only, so a point | ||
| * with z other than 1 could be multiplied using the table of another | ||
| * point. Only affine points are supported. */ | ||
| if (!mp_isone(G->z)) { |
Contributor
There was a problem hiding this comment.
Counting bits of G->z above.
Member
Author
There was a problem hiding this comment.
Removed the G->z bit count, as z != 1 is rejected right after it (bf24470).
| return IS_POINT_E; | ||
| } | ||
| /* Only affine points are supported - see wc_ecc_mulmod_ex(). */ | ||
| if (!mp_isone(G->z)) { |
Contributor
There was a problem hiding this comment.
Counting bits of G->z above.
Member
Author
There was a problem hiding this comment.
Removed the G->z bit count, as z != 1 is rejected right after it (bf24470).
Member
Author
|
PRB failure is fixed with #12032 |
With WOLFSSL_SP_MATH and FP_ECC, wc_ecc_mulmod_ex() and wc_ecc_mulmod_ex2() pass the base point unchanged to sp_ecc_mulmod_256/384/521(). The SP code caches a stripe table for each point it multiplies, keyed on the x and y ordinates only. A point with z other than 1 could therefore be multiplied using the table built from another point with the same x and y, giving a wrong result. For example, (Gx, Gy, p - 1) is -G. Multiplying G, then -G as (Gx, Gy, p - 1), then G again returns -kG for the last call, because the table was built from -G. P-256, P-384 and P-521 are all affected. This build has no generic point arithmetic to convert the point to affine or to fall back to, so return ECC_BAD_ARG_E when z is not 1. Callers in the library only pass affine points. Add test_wc_ecc_mulmod_projective, which multiplies G, -G as (Gx, Gy, p - 1), and G again on each SP-supported curve, and checks the results (or ECC_BAD_ARG_E for -G with SP math only and FP_ECC).
ecc_sets leaves out curves smaller than ECC_MIN_KEY_SZ, so with --with-eccminsz=384 the test looked up P-256 and failed to get its parameters. Gate each curve on the minimum key size as well.
wc_ecc_mulmod_ex() and wc_ecc_mulmod_ex2() now reject any z other than 1, so counting the bits of z first is redundant. A z wider than the modulus now gives ECC_BAD_ARG_E like any other z that is not 1.
LinuxJedi
force-pushed
the
ecc-mulmod-sp-math-fpecc
branch
from
November 6, 2026 11:43
bf24470 to
becf581
Compare
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.
Description
With
WOLFSSL_SP_MATHandFP_ECC,wc_ecc_mulmod_ex()andwc_ecc_mulmod_ex2()pass the base point unchanged tosp_ecc_mulmod_256/384/521(). WithFP_ECCthe SP code caches a stripe table for each point it multiplies (sp_ecc_get_cache_256()and the 384/521 versions), keyed on the x and y ordinates only. A point with z other than 1 could therefore be multiplied using the table built from another point with the same x and y.For example,
(Gx, Gy, p - 1)is -G. Multiplying G, then -G as(Gx, Gy, p - 1), then G again returns -kG for the last call. Once a table exists for x = Gx and y = Gy,(Gx, Gy, 2)also gets -G's result. P-256, P-384 and P-521 are all affected. WithoutFP_ECCthe SP code handles z != 1 correctly.This build has no generic point arithmetic to convert the point to affine or to fall back to, so return
ECC_BAD_ARG_Ewhen z is not 1. Callers in the library only pass affine points. Other math configurations, and SP math withoutFP_ECC, are unchanged.Testing
Added
test_wc_ecc_mulmod_projective: on each SP-supported curve (P-256, P-384, P-521), multiplies G, then -G as(Gx, Gy, p - 1)withwc_ecc_mulmod()(andwc_ecc_mulmod_ex2()whenWOLFSSL_PUBLIC_ECC_ADD_DBLis defined), then G again. Checks for kG, -kG (orECC_BAD_ARG_Ewith SP math only andFP_ECC) and kG. Fails without the fix../tests/unit.test --api --group eccand./wolfcrypt/test/testwolfcryptpass with:--enable-sp-math --enable-sp=yes,384,521 --disable-sp-asm --enable-fpecc CPPFLAGS=-DWOLFSSL_PUBLIC_MP, also with-DWOLFSSL_PUBLIC_ECC_ADD_DBL--enable-sp-math --enable-sp=yes,384,521 --disable-sp-asm, also withCPPFLAGS=-DWOLFSSL_PUBLIC_MP--enable-sp=ec256 --enable-fpecc --enable-smallstack, also withCPPFLAGS=-DWOLFSSL_PUBLIC_MP--enable-allChecklist