ML-DSA: Missing Private Key Validation Checks

[sgmenda]

Issue:

EVP_PKEY_pqdsa_new_raw_private_key() accepts malformed keys with secret vectors s1 and s2 containing 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() in crypto/fipsmodule/ml_dsa/ml_dsa_ref/packing.c. It now rejects keys if s1 or s2 have coefficients exceeding [-η, η].

Call-outs:

• With the addition of these validation checks, we should reject all invalid private keys.
• Discovered via Wycheproof test vector: https://github.com/C2SP/wycheproof/blob/e3c37e9db0f85a762dfcef1642b046bd31090ca4/testvectors_v1/mldsa_44_sign_noseed_test.json#L626-L646

Testing:

• Adds test vector generation script crypto/fipsmodule/ml_dsa/make_corrupted_key_tests.cc
• Adds the generated test vectors crypto/evp_extra/mldsa_corrupted_key_tests.txt
• Adds a test crypto/evp_extra/mldsa_test.cc that uses these test vectors

To run the test:

$ cd build
$ ./crypto/crypto_test --gtest_filter="*MLDSATest.ExpandedKeyValidation*"

To (re-)generate the test vectors:

$ cd crypto/fipsmodule/ml_dsa
$ make generate

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.

[sgmenda]

CI seems to not love that I am referencing internal functions:

/usr/bin/ld: crypto/CMakeFiles/crypto_test.dir/evp_extra/mldsa_test.cc.o: in function `MLDSATest::TestCorruptedKey(MLDSAParamSet const&, ml_dsa_params*, unsigned char const*, std::function<void (polyvecl*, polyveck*, polyveck*)>, char const*)':
mldsa_test.cc:(.text._ZN9MLDSATest16TestCorruptedKeyERK13MLDSAParamSetP13ml_dsa_paramsPKhSt8functionIFvP8polyveclP8polyveckSB_EEPKc[_ZN9MLDSATest16TestCorruptedKeyERK13MLDSAParamSetP13ml_dsa_paramsPKhSt8functionIFvP8polyveclP8polyveckSB_EEPKc]+0xad): undefined reference to `ml_dsa_unpack_sk'
/usr/bin/ld: mldsa_test.cc:(.text._ZN9MLDSATest16TestCorruptedKeyERK13MLDSAParamSetP13ml_dsa_paramsPKhSt8functionIFvP8polyveclP8polyveckSB_EEPKc[_ZN9MLDSATest16TestCorruptedKeyERK13MLDSAParamSetP13ml_dsa_paramsPKhSt8functionIFvP8polyveclP8polyveckSB_EEPKc]+0x173): undefined reference to `ml_dsa_pack_sk'
/usr/bin/ld: mldsa_test.cc:(.text._ZN9MLDSATest16TestCorruptedKeyERK13MLDSAParamSetP13ml_dsa_paramsPKhSt8functionIFvP8polyveclP8polyveckSB_EEPKc[_ZN9MLDSATest16TestCorruptedKeyERK13MLDSAParamSetP13ml_dsa_paramsPKhSt8functionIFvP8polyveclP8polyveckSB_EEPKc]+0x3f4): undefined reference to `ml_dsa_pack_pk_from_sk'
/usr/bin/ld: mldsa_test.cc:(.text._ZN9MLDSATest16TestCorruptedKeyERK13MLDSAParamSetP13ml_dsa_paramsPKhSt8functionIFvP8polyveclP8polyveckSB_EEPKc[_ZN9MLDSATest16TestCorruptedKeyERK13MLDSAParamSetP13ml_dsa_paramsPKhSt8functionIFvP8polyveclP8polyveckSB_EEPKc]+0x4cc): undefined reference to `ml_dsa_pack_sk'
collect2: error: ld returned 1 exit status
[671/676] Linking C executable util/fipstools/test_fips
[672/676] Linking CXX executable ssl/test/handshaker
[673/676] Building CXX object tool-openssl/CMakeFiles/tool_openssl_test.dir/req_test.cc.o
[674/676] Building CXX object tool-openssl/CMakeFiles/tool_openssl_test.dir/rsa_test.cc.o
[675/676] Building CXX object tool-openssl/CMakeFiles/tool_openssl_test.dir/x509_test.cc.o
ninja: build stopped: subcommand failed.

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-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.22%. Comparing base (b5e2f86) to head (362f051).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2874      +/-   ##
==========================================
- Coverage   78.24%   78.22%   -0.02%     
==========================================
  Files         683      684       +1     
  Lines      117388   117428      +40     
  Branches    16497    16501       +4     
==========================================
+ Hits        91853    91862       +9     
- Misses      24649    24680      +31     
  Partials      886      886              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[jakemas]

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.

[sgmenda]

@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.

[dkostic]

the mldsa-native code is not imported to aws-lc yet afaik, so we do need to make this change in aws-lc.

[sgmenda]

@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 requires clauses that the malformed keys violate: https://github.com/pq-code-package/mldsa-native/blob/main/mldsa/src/sign.c#L208-L209

[sgmenda]

Re proofs: these are the requires clauses that the malformed keys violate: https://github.com/pq-code-package/mldsa-native/blob/main/mldsa/src/sign.c#L208-L209

I cherrypicked the new tests onto @jakemas's #2849 and the tests all pass locally without any changes. 🎉 Likely because of the above requires clauses---yet another win for formal verification. (me 0-1 formal verification :)

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.