Fix hybrid ML-KEM client secret sizing - #1282
Open
ejohnstown wants to merge 3 commits into
Open
ejohnstown wants to merge 3 commits into
ejohnstown wants to merge 3 commits into
Conversation
KeyAgreeEcdhMlKem_client() writes the classical shared secret after the ML-KEM secret in ssh->k, so it now offers the classical call only the capacity left after that prefix, as the server side does. The ML-KEM private key decode result is now checked before decapsulation. - reject a ssh->k capacity that cannot hold the ML-KEM secret - add wolfSSH_TestKeyAgreeEcdhMlKem_client() and a unit test that runs each hybrid KEX at, below and above the capacity bound, and with a corrupted private key, and checks the derived secret Issue: F-14225, F-10559
wolfSSL can build ML-KEM without one of its parameter sets, or without FIPS 203 ML-KEM at all. The hybrid KEX gates now follow the set each one needs, so a missing set is neither advertised nor negotiated. - add WOLFSSH_NO_MLKEM768 and WOLFSSH_NO_MLKEM1024 from WOLFSSL_NO_ML_KEM, WOLFSSL_NO_ML_KEM_768 and WOLFSSL_NO_ML_KEM_1024 - derive the three hybrid KEX gates from them
DoKexDhReply() maps any KeyAgree_client() failure to WS_CRYPTO_FAILED and sends KEY_EXCHANGE_FAILED. Pin that for the hybrid ML-KEM KEXes, whose client agreement returns raw wolfCrypt codes. - truncate f in a hybrid KEXDH_REPLY and check the client's error and disconnect reason
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementation is correctly bounded, consistently gated, and covered by focused unit and regression tests.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes ML-KEM hybrid client key agreement sizing and decode-error handling while respecting enabled parameter sets.
Changes:
- Bounds classical secrets by remaining buffer capacity.
- Propagates ML-KEM private-key decode failures.
- Adds parameter-set gating and regression coverage.
| File | Description |
|---|---|
wolfssh/internal.h |
Adds ML-KEM parameter-set guards and test API. |
src/internal.c |
Corrects hybrid secret sizing and decode handling. |
tests/unit.c |
Tests capacity boundaries and corrupted keys. |
tests/regress.c |
Tests disconnect behavior after hybrid KEX failure. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The hybrid ML-KEM client offered the classical key agreement the full ssh->k capacity although the ML-KEM secret already sits in front of it, and did not check the ML-KEM private key decode. The hybrid KEXes are also now gated on the ML-KEM parameter set each one needs.