ML-KEM encapsulation key modulus check #1868
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1868 +/- ##
==========================================
+ Coverage 78.46% 78.47% +0.01%
==========================================
Files 585 585
Lines 99457 99511 +54
Branches 14236 14246 +10
==========================================
+ Hits 78038 78092 +54
Misses 20784 20784
Partials 635 635 ☔ View full report in Codecov by Sentry. |
dkostic
changed the title
| **************************************************/ | ||
| int crypto_kem_enc(ml_kem_params *params, | ||
| uint8_t *ct, | ||
| uint8_t *ss, | ||
| const uint8_t *pk) | ||
| { | ||
| if (encapsulation_key_modulus_check(params, pk) != 0) { | ||
| return 1; |
There was a problem hiding this comment.
now that this function has a failure path, we might need to adjust our libssl hybrid keyshare test that expects encaps to always succeed:
// |client_public_key| is initialized with key material that is the correct
// length, but is not a valid key. In this case, the basic sanity checks
// will not reject the key because it has been initialized properly with
// the correct amount of data. **The KEM encapsulate function is written
// so that it will return success if given an invalid key of the correct
// length.** Therefore, the call to server_key_share->Accept() will succeed,
// but ultimately, the ciphertext (server's public key) will be garbage,
// the server and client will end up with different secrets, and the
// overall handshake will eventually fail.
I don't think this is causing the current CI failure, as that failure's logs don't show the expected error trace that would result from failure in key share's call to encaps, but you'd likely hit the modulus check next.
There was a problem hiding this comment.
I looked into this -- the wording in that comment is a bit weird. It says an "invalid public key" but what it means is a public key that doesn't match the corresponding secret key. So I updated the comment. There was another issue with the test code that I fixed as well. The code was "invalidating" the key by negating its first byte. This can make the key actually invalid in the sense of FIPS 203 and with the change in this PR such key would be rejected, while the intention of the test is not to test real invalid keys but valid public keys that are not matching the secret key and thus would fail the final handshake.
| #define BYTE_DECODE_12_IN_SIZE (32 * 12) | ||
| #define BYTE_DECODE_12_NUM_BITS (256 * 12) | ||
|
|
||
| // FIPS 203. Algorithm 5 ByteDecode_12 |
|
|
||
| int16_t ek_decoded[ENCAPS_KEY_DECODED_MAX_SIZE]; | ||
| uint8_t ek_recoded[ENCAPS_KEY_ENCODED_MAX_SIZE]; | ||
|
|
There was a problem hiding this comment.
Why is the size of ek not verified here?
There was a problem hiding this comment.
PR #1868 implemented the encapsulation key modulus check naively, as specified in FIPS 203 which introduced a significant performance hit to encapsulation. In this change we optimize encoding/decoding to/from bytes functions to reduce the performance regression. For example, encapsulation performance (ops/s) measured on M1 macbook: ``` | Variant | Before #1868 | After #1868 | After this change | | 512 | 94738 | 68293 | 92494 | | 768 | 58672 | 42083 | 55572 | | 1024 | 38893 | 28536 | 36820 | ```
Issues:
CryptoAlg-2620
Description of changes:
ML-KEM encapsulation key modulus check as specified in FIPS 203 Section 7.2.
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.