-
Notifications
You must be signed in to change notification settings - Fork 120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ML-KEM decapsulation key hash check #1873
Conversation
ML-KEM encapsulation key modulus check as specified in Section 7.2 of FIPS 203: https://nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.203.pdf
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1873 +/- ##
==========================================
- Coverage 78.47% 78.46% -0.01%
==========================================
Files 585 585
Lines 99511 99524 +13
Branches 14249 14247 -2
==========================================
+ Hits 78094 78096 +2
- Misses 20781 20792 +11
Partials 636 636 ☔ View full report in Codecov by Sentry. |
// This check takes |ek| out of |dk|, computes H(ek), and verifies that it is | ||
// the same as the H(ek) portion stored in |dk|. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this sounds like input check 3. from the spec, but what about the "type checks" in 1. and 2.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering particularly about check 1 on the ciphertext which is said to have to be performed for every decapsulation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, this is the Hash check (as specified in L159). The other two Decaps checks and also the first check for Encaps (Section 7.2) are all length checks on the input arrays. We can safely omit them here because those checks are done in higher level functions. The required lengths for different variants of ML-KEM are hard-coded here:
aws-lc/crypto/fipsmodule/kem/kem.c
Line 133 in 2835116
DEFINE_LOCAL_DATA(KEM, KEM_ml_kem_512) {
The ciphertext length for Decaps is checked here:aws-lc/crypto/fipsmodule/evp/p_kem.c
Line 272 in 2835116
if (ciphertext_len != kem->ciphertext_len ||
If a key is generated by aws-lc then it satisfies the length requirements. If a key is generated outside of aws-lc, it has to be imported into anEVP_PKEY
object to be used within aws-lc. We provide only these three functions to do that:EVP_PKEY_kem_new_raw_key
,EVP_PKEY_kem_new_raw_secret_key
,EVP_PKEY_kem_new_raw_public_key
. The lengths are checked for example here:aws-lc/crypto/fipsmodule/evp/p_kem.c
Line 465 in 2835116
if (kem->public_key_len != len_public || kem->secret_key_len != len_secret) {
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @dkostic. Maybe you can summarise somewhere that all checks for encapsulate and decapsulate are done as per fips 203 in various places because I think someone coming to this part of the code where it says as in Sec 7.2 or 7.3 will wonder where the other checks are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think someone coming to this part of the code where it says as in Sec 7.2 or 7.3 will wonder where the other checks are.
That's me, and I was indeed wondering that. I would appreciate it if the comments could be added.
Issues:
CryptoAlg-2620
Description of changes:
ML-KEM decapsulation key check as specified in Section 7.3 of FIPS 203: https://nvlpubs.nist.gov/nistpubs/FIPS/NIST.FIPS.203.pdf
Call-outs:
Point out areas that need special attention or support during the review process. Discuss architecture or design changes.
Testing:
How is this change tested (unit tests, fuzz tests, etc.)? Are there any testing steps to be verified by the reviewer?
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.