Skip to content

ML-KEM memory safety #2263

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

Merged
merged 49 commits into from
Jun 5, 2025
Merged

ML-KEM memory safety #2263

merged 49 commits into from
Jun 5, 2025

Conversation

m271828
Copy link
Contributor

@m271828 m271828 commented Mar 11, 2025

Description of changes:

Adds length parameters to ML KEM module to ensure allocated memory for various outputs is sufficient for generated results. Checks that already exist in higher levels for some functions were not removed.

Issues:

Resolves #CryptoAlg-2945

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.

@codecov-commenter
Copy link

codecov-commenter commented Mar 12, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.91%. Comparing base (b68e77a) to head (1d6fd03).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2263      +/-   ##
==========================================
+ Coverage   78.89%   78.91%   +0.02%     
==========================================
  Files         621      622       +1     
  Lines      108685   108918     +233     
  Branches    15420    15431      +11     
==========================================
+ Hits        85745    85958     +213     
- Misses      22267    22290      +23     
+ Partials      673      670       -3     

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

@m271828 m271828 marked this pull request as ready for review March 13, 2025 23:31
@m271828 m271828 requested a review from a team as a code owner March 13, 2025 23:31
@m271828
Copy link
Contributor Author

m271828 commented Apr 9, 2025

Updated to resolve merge conflicts from #2176 (change in how parameters are being used, new functions that were added have length checks too).

torben-hansen
torben-hansen previously approved these changes Apr 15, 2025
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

There were too many comments to post at once. Showing the first 10 out of 246. Check the log or trigger a new build to see more.

ASSERT_EQ(params.secret_len, decaps_len);
}

static const uint8_t encaps512Key[MLKEM512_PUBLIC_KEY_BYTES] = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering does this test depend on this particular key or could it pull an existing KAT or generate one? It's a small announce because it makes reading the file harder (self_check.c is out of control but requires the KAT be baked into the library).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't depend on this particular keys, just that the key be valid. When testing, I've always used a hard coded known good key in cases like this to avoid false failures (this is testing the length checks only, we don't want a test failing because of an invalid key, that should cause a different test to fail).

@torben-hansen torben-hansen merged commit 93d3ba9 into aws:main Jun 5, 2025
115 of 118 checks passed
@m271828 m271828 deleted the memory-safety-ml-kem branch June 5, 2025 21:14
@justsmth justsmth mentioned this pull request Jun 6, 2025
justsmth added a commit that referenced this pull request Jun 13, 2025
## What's Changed
* Add build with hardened flag by @m271828 in
#2396
* Openssl tool output ordered by options provided by @justsmth in
#2452
* [SCRUTINICE] Remove redundant condition check by @nhatnghiho in
#2450
* Support relro in delocator by @torben-hansen in
#2455
* Explicitly don't allow buffers aliasing in ctr-drbg implementation by
@torben-hansen in #2458
* Remove unused Windows afunix.h by @justsmth in
#2461
* Revert "Rework memory BIOs and implement BIO_seek (2nd try) (#2433)"
by @justsmth in #2466
* Use max_cert_list for TLSv1.3 NewSessionTicket by @skmcgrail in
#2453
* ML-KEM memory safety by @m271828 in
#2263
* Simplify Compiler CI jobs by @justsmth in
#2430
* Improve support for multilib-style distros in our test scripts by
@justsmth in #2467
* Fix Ruby mainline and nginx CI by @samuel40791765 in
#2460
* Add hardened build back in by @m271828 in
#2474
* Fix OCSP integration test failures by @samuel40791765 in
#2480
* Fix some theoretical missing earlyclobber markers in inline assembly
by @torben-hansen in #2477
* Simplify sshkdf and kbkdf by @torben-hansen in
#2478
* Run 3p module tests on python 3.13, add patch for 3.14 by
@WillChilds-Klein in #2476
* [UPSTREAM] Fix BIO_eof for BIO pairs by @justsmth in
#2440
* Fix service indicator in HKDF, more paranoid zeroization, and simplify
logic by @torben-hansen in #2482


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

6 participants