Skip to content

Hardening fixes for ML-DSA digest mode, XTS key comparison, and urandom fd#3129

Merged
justsmth merged 2 commits intoaws:mainfrom
justsmth:mldsa-mu-hardening
Mar 27, 2026
Merged

Hardening fixes for ML-DSA digest mode, XTS key comparison, and urandom fd#3129
justsmth merged 2 commits intoaws:mainfrom
justsmth:mldsa-mu-hardening

Conversation

@justsmth
Copy link
Copy Markdown
Contributor

Issues

  • Addresses: P398529487

Description of changes:

Several small hardening fixes:

  • ML-DSA digest length validation: Added digest_len to the PQDSA struct and enforce that the mu input to ML-DSA sign/verify digest mode is exactly MLDSA_CRHBYTES (64 bytes). Previously the digest-mode paths accepted any message_len and passed it directly to the underlying implementation without validation.
  • PQDSA signature buffer check: Changed the sig_len check from != to < so callers providing a larger-than-needed buffer aren't rejected. This is consistent with standard OpenSSL buffer conventions.
  • XTS key comparison: Switched the XTS duplicate-key check from OPENSSL_memcmp to CRYPTO_memcmp to avoid timing leakage on key material.
  • urandom fd initialization: Changed urandom_fd from 0 to -1 so that any use before initialization fails with EBADF rather than silently reading from stdin.

Call-outs:

The digest_len field in pqdsa.c is set to the literal 64 rather than MLDSA_CRHBYTES because the macro is not available in that translation unit (due to bcm.c undef behavior). Comments in the code explain this.

Testing:

Existing test coverage for ML-DSA sign/verify, XTS, and RAND should exercise these code paths. The ML-DSA digest length checks are new validation — negative test cases that pass an incorrect mu length could be a useful follow-up.

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.

@justsmth justsmth requested a review from a team as a code owner March 27, 2026 15:19
@justsmth justsmth requested a review from jakemas March 27, 2026 15:20
@justsmth justsmth requested review from dkostic and nebeid March 27, 2026 15:21
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 63.63636% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.01%. Comparing base (3c2cc1e) to head (9dc81cf).

Files with missing lines Patch % Lines
crypto/fipsmodule/evp/p_pqdsa.c 42.85% 4 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3129   +/-   ##
=======================================
  Coverage   78.01%   78.01%           
=======================================
  Files         689      689           
  Lines      122406   122414    +8     
  Branches    17071    17086   +15     
=======================================
+ Hits        95493    95500    +7     
- Misses      26014    26016    +2     
+ Partials      899      898    -1     

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

out->signature_len = MLDSA44_SIGNATURE_BYTES;
out->keygen_seed_len = MLDSA44_KEYGEN_SEED_BYTES;
out->sign_seed_len = MLDSA44_SIGNATURE_SEED_BYTES;
out->digest_len = 64; // MLDSA_CRHBYTES, unavailable here due to bcm.c undef
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I see no need to statically assert MLDSA_CRHBYTES == 64 anywhere as this has to match the out put length of fixed shake256.

@justsmth justsmth enabled auto-merge (squash) March 27, 2026 16:42
@justsmth justsmth merged commit e132d63 into aws:main Mar 27, 2026
534 of 539 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants