Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2001 +/- ##
==========================================
- Coverage 78.68% 78.66% -0.02%
==========================================
Files 598 598
Lines 103350 103351 +1
Branches 14692 14691 -1
==========================================
- Hits 81321 81305 -16
- Misses 21377 21393 +16
- Partials 652 653 +1 ☔ View full report in Codecov by Sentry. |
geedo0
left a comment
geedo0
left a comment
There was a problem hiding this comment.
Looks pretty straightforward. The only big thing missing here seems to be updates to the README.md file.
| #include "fips202.h" | ||
|
|
||
| void dilithium_shake128_stream_init(keccak_state *state, const uint8_t seed[SEEDBYTES], uint16_t nonce) | ||
| /************************************************* |
There was a problem hiding this comment.
I'll defer to the LC team on this point, but IMO the docstrings here should be in the header file and match LC's style conventions for comments. I can see that these were ported over from the fips202 source, so I can see the merit in retaining that style and content. It's also not part of LC's public interface so I feel less strong about belaboring it.
There was a problem hiding this comment.
Good call out. As I added these functions within the crypto/dilithium/pqcrystals_dilithium_ref_common/ directory, I wanted to be consistant with function conventions. I did the same within ML-KEM (see example).
| shake128_finalize(state); | ||
| SHAKE_Init(ctx, SHAKE128_BLOCKSIZE); | ||
| SHA3_Update(ctx, seed, SEEDBYTES); | ||
| SHA3_Update(ctx, t, 2); |
There was a problem hiding this comment.
Perhaps extract the 2 as a constant value to improve readability?
README.md updated (was waiting on merge). |
| shake256_squeezeblocks(buf, 1, &state); | ||
| SHAKE_Init(&state, SHAKE256_BLOCKSIZE); | ||
| SHA3_Update(&state, seed, params->c_tilde_bytes); | ||
| dilithium_shake256_squeeze(&state, buf, 1); |
There was a problem hiding this comment.
why can't we just use SHA_Final directly?
There was a problem hiding this comment.
actually, why can't we use SHAKE256 instead of the sequence of Init, Update, Final?
There was a problem hiding this comment.
Because we need an additional squeeze down at line 524 below (part of the rejection sampling loop). If we call SHAKE256 directly at this stage, we cannot additionally squeeze output later. This is exactly the difference between calling SHAKE256 as a single shot output, vs as an extensible output function, as required for both ML-KEM and ML-DSA.
There was a problem hiding this comment.
However, I agree that we can use SHA_Final directly, and have modified the PR to do this for both SHAKE256 and SHAKE 128 in f4cc0fe
crypto/dilithium/pqcrystals_dilithium_ref_common/symmetric-shake.c
Outdated
Show resolved
Hide resolved
|
Any further required actions here? |
| SHAKE_Init(&state, SHAKE128_BLOCKSIZE); | ||
| SHA3_Update(&state, seed, SEEDBYTES); | ||
| SHA3_Update(&state, t, 2); | ||
| SHAKE_Final(buf, &state,POLY_UNIFORM_NBLOCKS * SHAKE128_BLOCKSIZE); |
There was a problem hiding this comment.
| SHAKE_Final(buf, &state,POLY_UNIFORM_NBLOCKS * SHAKE128_BLOCKSIZE); | |
| SHAKE_Final(buf, &state, POLY_UNIFORM_NBLOCKS * SHAKE128_BLOCKSIZE); |
4 participants
Issues:
Resolves CryptoAlg-2726
Description of changes:
Following the inclusion of SHAKE as an extensible-output-function (XOF) in #1839, we are now able to fully support ML-DSA with SHA3/SHAKE usage within
crypto/fipsmodule. As such, all references to the internal implementation of SHA3 (withincrypto/dilithium/pqcrystals_dilithium_ref_common/fips202.{h|c}) have been removed.Call-outs:
keccak_stateprovided by the Dilithium reference implementation has been replaced withKECCAK1600_CTXabsorb/update/finalhave been replaced with a straight swap to AWS-LC's implementationdilithium_shake128_stream_init,dilithium_shake128_squeeze,dilithium_shake256_stream_init,dilithium_shake256_squeezeoffips202.chas been replaced with versions that call SHA3 from fipsmodule.Testing:
The testing framework is the KATs for ML-DSA, and all other ML-DSA tests completed.
#CryptoAlg-2589 design documentation provides an analysis of all
fips202.cusage from ML-DSA, to verify that all functionality that used to be provided byfips202.chas been replaced by this commit, the file has been removed from the library.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.