fix: validate ML-DSA s1/s2 coefficient bounds in wc_dilithium_check_key#10254
Open
MarkAtwood wants to merge 3 commits into
Open
fix: validate ML-DSA s1/s2 coefficient bounds in wc_dilithium_check_key#10254MarkAtwood wants to merge 3 commits into
MarkAtwood wants to merge 3 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds explicit coefficient-bound validation for ML-DSA (Dilithium) private key components so malformed keys with out-of-range s1/s2 coefficients are rejected before running NTT/matrix checks.
Changes:
- Validate decoded
s1/s2coefficients are within[-eta, +eta]inwc_dilithium_check_key. - Skip the NTT/matrix-multiply consistency pipeline when coefficient validation fails.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Member
|
@MarkAtwood see copilot feedback. If you have fixed it or won't just mark it resolved and assign wolfssl-bot only indicating its ready for merge. |
Member
|
Jenkins retest this please |
After decoding s1 and s2 from the private key, check that every coefficient lies within [-eta, +eta]. 3-bit values 5/6/7 (eta=2) and 4-bit nibbles 9-15 (eta=4) are out of spec per FIPS 204 and must be rejected. Without this check, internally-consistent but spec-invalid keys pass the mathematical consistency test. Catches the InvalidPrivateKey case in Wycheproof ML-DSA mldsa_*_sign_noseed_test.json vectors.
After the coefficient bounds check sets ret=PUBLIC_KEY_E, wrap the subsequent NTT and matrix-multiply block in if (ret == 0) so that invalid coefficient data is never passed to NTT functions.
MarkAtwood
force-pushed
the
fix/mldsa-privkey-bounds
branch
from
3d992cc to
49a7603
Compare
MarkAtwood
force-pushed
the
fix/mldsa-privkey-bounds
branch
from
7c45d46 to
e6d1b6d
Compare
MarkAtwood
force-pushed
the
fix/mldsa-privkey-bounds
branch
from
e6d1b6d to
e4559ae
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.
Summary
dilithium_vec_decode_eta_bitsperforms no bounds checking — it decodes raw bit fields and applies the formula without rejecting out-of-range values. For eta=2, 3-bit fields 5/6/7 (valid range 0–4) produce coefficients outside [-2, +2]. For eta=4, 4-bit nibbles 9–15 (valid range 0–8) produce coefficients outside [-4, +4]. Both are out of spec per FIPS 204 §7.2.wc_dilithium_check_keyhad no coefficient range validation after decode. The mathematical consistency check (As1 + s2 = tmod q) validates algebraic structure, not coefficient range — a malformed key with out-of-range s1/s2 can still pass it.Commit 1: After decoding s1/s2, validate every coefficient is in
[-eta, +eta]. Setsret = PUBLIC_KEY_Eon first violation.Commit 2: Wrap the NTT/matrix-multiply pipeline in
if (ret == 0)so invalid coefficients are never fed to the NTT after the bounds check has already failed.Found via Wycheproof ML-DSA
mldsa_*_sign_noseed_test.jsonInvalidPrivateKeyvectors.Test plan
/cc @wolfSSL-Fenrir-bot please review