wolfcrypt/mldsa: reduce w0/ct0 before check_low, matching the z step - #11113
wolfcrypt/mldsa: reduce w0/ct0 before check_low, matching the z step#11113SemyonAndreyev wants to merge 3 commits into
Conversation
The z path in mldsa_sign_with_seed_mu() calls mldsa_poly_red() on its NTT^-1 output before check_low(). The w0 path computes the same kind of NTT^-1 output but calls check_low() directly, without that reduction step. This aligns w0 with the existing z pattern already present in the same function, so both bound checks receive a canonically-reduced input. The WOLFSSL_MLDSA_SIGN_SMALL_MEM variant of this same computation already reduces its w0t the same way, one further point of internal consistency. No change in behavior on any input that currently passes check_low: the bound values here are well below the range mldsa_poly_red actually affects, so a reduction can only turn some current rejections into acceptances, one step earlier in the existing rejection-sampling loop (FIPS 204 Algorithm 2, step 23).
The z path in mldsa_sign_with_seed_mu() calls mldsa_poly_red() on its NTT^-1 output before check_low(). The ct0 path computes the same kind of NTT^-1 output (via mldsa_mul_invntt) but calls check_low() directly, without that reduction step. This aligns ct0 with the existing z/w0 pattern in the same function. No change in behavior on any input that currently passes check_low -- gamma2 (the bound used here) is well below the range mldsa_poly_red actually affects, so a reduction can only turn some current rejections into acceptances, one step earlier in the existing rejection-sampling loop (FIPS 204 Algorithm 2, step 27).
|
Can one of the admins verify this patch? |
…th too
The default-build fix in this PR (previous commit) covers the fused
mldsa_mul_invntt() call in mldsa_sign_with_seed_mu()'s main path. The
same function also has a second implementation, compiled instead of the
first under WOLFSSL_MLDSA_SIGN_SMALL_MEM (a real, documented,
non-default build option for low-memory signing -- see the option
reference at the top of this file), which does the same mul+invntt
manually as two separate calls:
mldsa_mul(ct0, c, t0 + r * MLDSA_N);
mldsa_invntt(ct0);
valid = mldsa_check_low(ct0, params->gamma2);
mldsa_mul_invntt(r, c, v) is itself defined as exactly
"mldsa_mul(r, c, v); mldsa_invntt(r);" -- so this is the identical
computation as the already-fixed path, missing the identical reduction
before the identical check. w0 and z already reduce before their checks
in this same small-mem path; only ct0 was missing it, same asymmetry as
the default path.
Verified on a clean GitHub Actions runner (independent of the local
machine) in the actual target configuration: `./configure --enable-mldsa`
with CFLAGS=-DWOLFSSL_MLDSA_SIGN_SMALL_MEM_PRECALC, native `make check`
gives an identical 10 TOTAL / 5 PASS / 5 SKIP / 0 FAIL both before and
after this change -- no regression, matching the pattern already
established for the default-path fix.
|
Hi @SemyonAndreev , thank you for this report. I've assigned this to @SparkiDev to review. Because this is a small change we will treat as a bug report. In order to accept contributions we require a signed contributor agreement. Can I ask how you found this? Can you tell us more about your project and region? Are you planning any further contributions? |
|
Hi @dgarske, thanks for the fast turnaround, and for putting this in front of @SparkiDev. Contributor agreement — happy to sign. Should I request it from support@wolfssl.com, or can you How we found it. We build static analysis tooling for reduction placement in ML-DSA We ran the scan against four independent ML-DSA codebases: OpenSSL 3.5, mldsa-native, TQ42 Cryptography One more site, found the same way. While preparing this, we noticed the same function has a second Project and region. We are Q² quantum ecosystem (https://www.linkedin.com/company/q2-quantum-ecosystem/), Further contributions. Yes. We are continuing on ML-DSA and expect to cover more of the standard, If the fix lands as your own commit rather than through this PR, that is fine. A mention of where the Semyon |
4 participants
Description
The
zpath inmldsa_sign_with_seed_mu()(wolfcrypt/src/wc_mldsa.c) callsmldsa_poly_red()on its NTT⁻¹ output beforemldsa_vec_check_low(). Thew0andct0paths in the same function compute the same kind of NTT⁻¹ output but callcheck_low()directly, without that reduction step. This PR aligns both with the existingzpattern already present in the same function, so all three bound checks receive a canonically-reduced input.mldsa_sign_with_seed_mu()has a second implementation of this same logic, compiled instead of the default one underWOLFSSL_MLDSA_SIGN_SMALL_MEM(a real, documented, non-default build option for low-memory signing). That variant already reducesw0t/zthe same way before their checks, but had the identical missing reduction onct0— the third commit here fixes that too, soct0is now handled consistently withw0/zin both variants, not just the default one.No change in behavior on any input that currently passes
check_low— the bound values here (hi) are well below the rangemldsa_poly_redactually affects, so a reduction can only turn some current rejections into acceptances, one step earlier in the existing rejection-sampling loop (FIPS 204 Algorithm 2, steps 23/27).Three commits:
w0(default path),ct0(default path),ct0(small-mem path) — same one-line change pattern each, independently revertible/bisectable if useful during review.Testing
Verified against the existing native test suite (
./configure --enable-mldsa && make && make check) in a clean environment (fresh GitHub Actionsubuntu-latestcheckout, no local state): identical result before and after this change, and with each commit applied individually —10 TOTAL / 5 PASS / 5 SKIP / 0 FAILin all cases, byte-for-byte identicalmake checkoutput.The small-mem variant isn't exercised by that default build (it's behind a build flag with no
./configureswitch, only settable viaCFLAGS/user_settings.h), so it was verified separately: built withCFLAGS=-DWOLFSSL_MLDSA_SIGN_SMALL_MEM_PRECALCon a fresh runner,make checkgives the identical10/5/5/0result both with and without the third commit — no regression from enabling that variant, and none from this fix within it.Limitation, stated directly rather than left implicit: I attempted to write a targeted unit test exercising
mldsa_poly_red()/mldsa_vec_check_low()directly at the specific boundary value where the missing reduction changes the outcome (a witness value from static bounds analysis of this code path). Both functions arestaticinwc_mldsa.c, andtests/unit-mcdc/(the framework wolfSSL uses for whitebox coverage ofstaticinternals) is documented as external tooling with its own build driver, separate from the normal./configure && makebuild — I wasn't able to wire a standalone test into that framework in reasonable time. So this PR's correctness argument rests on (a) the round-trip sign/verify test suite showing no regression in either build variant, and (b) the bound-value reasoning above (the change can only affect the rejection-sampling loop's iteration count, never signature validity), not on a dedicated unit test isolating the exact value this reduction protects.Checklist