fix: validate ML-DSA s1/s2 coefficient bounds in wc_dilithium_check_key#10254
fix: validate ML-DSA s1/s2 coefficient bounds in wc_dilithium_check_key#10254MarkAtwood wants to merge 2 commits intowolfSSL:masterfrom
Conversation
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.
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.
| for (i = 0; i < params->k; i++) { | ||
| for (j = 0; j < DILITHIUM_N; j++) { | ||
| x |= tt[j] ^ t1[j]; | ||
| /* Calcaluate t = NTT-1(A o NTT(s1)) + s2 */ |
There was a problem hiding this comment.
Typo in comment: 'Calcaluate' should be 'Calculate'.
| /* Calcaluate t = NTT-1(A o NTT(s1)) + s2 */ | |
| /* Calculate t = NTT-1(A o NTT(s1)) + s2 */ |
| for (c = 0; c < (word32)(params->l * DILITHIUM_N); c++) { | ||
| if (s1[c] < -eta || s1[c] > eta) { | ||
| ret = PUBLIC_KEY_E; | ||
| break; | ||
| } | ||
| } | ||
| for (c = 0; (ret == 0) && (c < (word32)(params->k * DILITHIUM_N)); c++) { | ||
| if (s2[c] < -eta || s2[c] > eta) { | ||
| ret = PUBLIC_KEY_E; | ||
| break; | ||
| } | ||
| } |
There was a problem hiding this comment.
This introduces data-dependent early exits (break) while processing secret-key material (s1/s2). If wc_dilithium_check_key() is used in contexts where timing differences are observable, consider making this check branchless by accumulating a violation flag across all coefficients and setting ret = PUBLIC_KEY_E after the loops complete (same behavior, less timing leakage).
| for (c = 0; c < (word32)(params->l * DILITHIUM_N); c++) { | |
| if (s1[c] < -eta || s1[c] > eta) { | |
| ret = PUBLIC_KEY_E; | |
| break; | |
| } | |
| } | |
| for (c = 0; (ret == 0) && (c < (word32)(params->k * DILITHIUM_N)); c++) { | |
| if (s2[c] < -eta || s2[c] > eta) { | |
| ret = PUBLIC_KEY_E; | |
| break; | |
| } | |
| } | |
| word32 bad = 0; | |
| for (c = 0; c < (word32)(params->l * DILITHIUM_N); c++) { | |
| if (s1[c] < -eta || s1[c] > eta) { | |
| bad = 1; | |
| } | |
| } | |
| for (c = 0; c < (word32)(params->k * DILITHIUM_N); c++) { | |
| if (s2[c] < -eta || s2[c] > eta) { | |
| bad = 1; | |
| } | |
| } | |
| if (bad != 0) { | |
| ret = PUBLIC_KEY_E; | |
| } |
| /* Validate s1 and s2 coefficients are within [-eta, eta]. */ | ||
| { | ||
| sword32 eta = (sword32)params->eta; | ||
| word32 c; | ||
| for (c = 0; c < (word32)(params->l * DILITHIUM_N); c++) { | ||
| if (s1[c] < -eta || s1[c] > eta) { | ||
| ret = PUBLIC_KEY_E; | ||
| break; | ||
| } | ||
| } | ||
| for (c = 0; (ret == 0) && (c < (word32)(params->k * DILITHIUM_N)); c++) { | ||
| if (s2[c] < -eta || s2[c] > eta) { | ||
| ret = PUBLIC_KEY_E; | ||
| break; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The new coefficient validation is duplicated for s1 and s2 with slightly different loop bounds. Consider extracting this into a small internal helper (e.g., dilithium_vec_check_eta_bounds(const sword32* v, word32 coeffCount, sword32 eta)) to reduce duplication and make future changes (like constant-time accumulation or different bounds) less error-prone.
|
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