Verify size of mlen in ML-DSA external mu mode #2841
Conversation
| else { | ||
| // When external_mu is true, m is expected to be exactly ML_DSA_CRHBYTES | ||
| if (mlen != ML_DSA_CRHBYTES) { | ||
| return -1; |
There was a problem hiding this comment.
you'll skip the destruction of intermediate values with this.
There was a problem hiding this comment.
Thank you, great spot, added in 2185013
| else { | ||
| // When external_mu is true, m is expected to be exactly ML_DSA_CRHBYTES | ||
| if (mlen != ML_DSA_CRHBYTES) { | ||
| return -1; |
There was a problem hiding this comment.
Thank you, great spot, added in 2185013
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| SHAKE_Absorb(&state, tr, ML_DSA_TRBYTES); | ||
| SHAKE_Absorb(&state, pre, prelen); | ||
| SHAKE_Absorb(&state, m, mlen); | ||
| SHAKE_Final(mu, &state, ML_DSA_CRHBYTES); |
There was a problem hiding this comment.
warning: call to undeclared function 'SHAKE_Final'; ISO C99 and later do not support implicit function declarations [clang-diagnostic-implicit-function-declaration]
SHAKE_Final(mu, &state, ML_DSA_CRHBYTES);
^Additional context
crypto/fipsmodule/ml_dsa/ml_dsa_ref/sign.c:204: did you mean 'SHA1_Final'?
SHAKE_Final(mu, &state, ML_DSA_CRHBYTES);
^include/openssl/sha.h:84: 'SHA1_Final' declared here
OPENSSL_EXPORT int SHA1_Final(uint8_t out[SHA_DIGEST_LENGTH], SHA_CTX *sha);
^| SHAKE_Absorb(&state, tr, ML_DSA_TRBYTES); | ||
| SHAKE_Absorb(&state, pre, prelen); | ||
| SHAKE_Absorb(&state, m, mlen); | ||
| SHAKE_Final(mu, &state, ML_DSA_CRHBYTES); |
There was a problem hiding this comment.
warning: use of undeclared identifier 'state' [clang-diagnostic-error]
SHAKE_Final(mu, &state, ML_DSA_CRHBYTES);
^
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2841 +/- ##
==========================================
- Coverage 78.24% 78.24% -0.01%
==========================================
Files 683 683
Lines 117354 117358 +4
Branches 16492 16492
==========================================
- Hits 91826 91822 -4
- Misses 24640 24648 +8
Partials 888 888 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
manastasova
left a comment
manastasova
left a comment
There was a problem hiding this comment.
Why not checking for the correctness of the length at the beginning of these two functions and, thus, avoid the memory allocations and function calls?
Ahh yeah that could work too, follow the exisiting siglen check with an mlen check... I'm happy with either |
|
I can see it being at the very beginning of the functions (along with the external mu check): |
| polyvecl mat[ML_DSA_K_MAX], s1, y, z; | ||
| polyveck t0, s2, w1, w0, h; | ||
| ml_dsa_poly cp; | ||
| KECCAK1600_CTX state; |
There was a problem hiding this comment.
warning: use of undeclared identifier 'KECCAK1600_CTX' [clang-diagnostic-error]
KECCAK1600_CTX state;
^
| * - int external_mu: indicates input message m is to be processed as mu | ||
| * | ||
| * Returns 0 (success) or -1 (context string too long) | ||
| * Returns 0 (success) or -1 (context string or mlen too long) |
There was a problem hiding this comment.
Or too short? I could not see where the length of context is checked.
| } | ||
|
|
||
| if (external_mu && mlen != ML_DSA_CRHBYTES) { | ||
| return -1; |
| polyveck t1, w1, h; | ||
| KECCAK1600_CTX state; | ||
|
|
||
| if(siglen != params->bytes) { |
There was a problem hiding this comment.
we could add space here if <space> ( ....
There was a problem hiding this comment.
There is a mix of if's with and without -- for now I can live with this
4 participants
Issues:
Resolves #N/A
Description of changes:
When signing/verifying in ML-DSA the caller provides
mlenthe the length of the messagemthat is being signed/verified. Currently, we do no validation on the size ofmlenwhen in pre-hash mode (called "external mu" in ML-DSA). Since the pre-hash is the output of a SHAKE256 hash function, it is of fixed sizeML_DSA_CRHBYTES. This adds validation to check that, returning-1if not true to match the case where the contextctxhas length too large.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and the ISC license.