ML-DSA: Missing Private Key Validation Checks#2874
Conversation
|
CI seems to not love that I am referencing internal functions: https://github.com/aws/aws-lc/actions/runs/19872727434/job/56952394758?pr=2874 So lemme think on it, maybe I should hardcode the vectors instead of generating them inside the test. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2874 +/- ##
==========================================
+ Coverage 78.16% 78.36% +0.20%
==========================================
Files 689 690 +1
Lines 118653 118669 +16
Branches 16681 16685 +4
==========================================
+ Hits 92740 92997 +257
+ Misses 25023 24779 -244
- Partials 890 893 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jakemas
left a comment
There was a problem hiding this comment.
This PR will need to made in https://github.com/pq-code-package/mldsa-native not here in aws-lc. The process for working on ml-dsa code is to develop in mldsa-native and use the importer. This can be added upstream now, and once #2849 is merged, we can run the "importer" to update the code -- as with ml-kem.
|
@jakemas oops, good catch, I forgot about that. I'll make an upstream PR for the mldsa changes, and hold off on adding the test till it we merge those changes into LC. |
|
the mldsa-native code is not imported to aws-lc yet afaik, so we do need to make this change in aws-lc. |
|
@dkostic the argument is that since the mldsa-native import PR is in-flight as #2849, we can land this on top of that, instead of creating unnecessary merge conflicts and test failures for that PR. Since the severity is low and fix upstream is not going to be too bad (see pq-code-package/mldsa-native#714 which describes the function that we are going to add the bounds check to), I agree with @jakemas. Re proofs: these are the |
I cherrypicked the new tests onto @jakemas's #2849 and the tests all pass locally without any changes. 🎉 Likely because of the above So, I will continue to hold this PR till that lands. Then, we can add these tests on top of it and run it on CI to see if we need to make any changes to upstream. |
These tests are expected to fail till aws#2874 is resolved
### Description of changes: Integrates 9 Wycheproof ML-DSA test vector files, three for each of the three parameter sets (ML-DSA-44, ML-DSA-65, ML-DSA-87). Each integration adds upstream JSON vectors and converted txt files to `third_party/vectors/`, and adds test code with duvet annotations for traceability. ### Call-outs: - This is a follow-up to #2874 - All tests pass - Wycheproof ECDSA vectors are outdated, and that'll be addressed in #2972 ### Testing: All new tests pass and duvet verification succeeds: ```bash cd build && ./crypto/crypto_test --gtest_filter="*Wycheproof*" cd third_party/vectors && duvet report --ci ``` 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.
Issue:
EVP_PKEY_pqdsa_new_raw_private_key()accepts malformed keys with secret vectorss1ands2containing coefficients outside the valid range[-η, η]. These keys lead to undefined behavior, like producing signatures that fail verification.Description of changes:
Adds the missing validation checks to
ml_dsa_pack_pk_from_sk()incrypto/fipsmodule/ml_dsa/ml_dsa_ref/packing.c. It now rejects keys ifs1ors2have coefficients exceeding[-η, η].Call-outs:
Testing:
crypto/fipsmodule/ml_dsa/make_corrupted_key_tests.cccrypto/evp_extra/mldsa_corrupted_key_tests.txtcrypto/evp_extra/mldsa_test.ccthat uses these test vectorsTo run the test:
To (re-)generate the test vectors:
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.