Repository navigation
Wipe the KEX shared secret once keys are derived - #1275
ejohnstown wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Cleanup misses some exit paths and occurs too late before packet sending.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
Wipes KEX shared secrets and failed handshake key material earlier, with regression coverage.
Changes:
- Clears
ssh->kand resetskSz. - Clears handshake key sets when derivation fails.
- Adds KEX and failure-path tests.
| File | Summary |
|---|---|
tests/regress.c |
Verifies wiping behavior across handshakes and failures. |
src/internal.c |
Implements secret and key-material cleanup. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@wolfSSL-Fenrir-bot review |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1275
Scan targets checked: wolfssh-src, wolfssh-bugs
Coverage: 2 of 2 in-scope changed file(s) opened by the reviewer
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Lite
DoKexDhReply() and SendKexDhReply() zero all of ssh->k and reset kSz on every exit, so K does not outlive the key exchange. GenerateKeys() zeroes both handshake key sets when a derivation fails. - the full-buffer wipe clears CreateMpint and ML-KEM hybrid leftovers - tests/regress.c checks K after each KEX, the hybrids included - tests/regress.c checks a failed derivation leaves no keys Issue: F-8845, F-11680, F-11681, F-8843, F-11682, F-14001
GenerateKeys() now wipes only the iv, encKey and macKey bytes when a derivation fails, so the negotiated sizes stay put and a second call cannot report success with no keys. - tests/regress.c checks a failed server KEXDH_REPLY send wipes K - AssertHandshakeSucceeds() takes a KEX algorithm for the per-KEX check - KEX wipe failures name the algorithm and the endpoint Issue: F-8845, F-11680, F-11681, F-8843, F-11682, F-14001
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1275
Scan targets checked: wolfssh-src, wolfssh-bugs
Coverage: 2 of 2 in-scope changed file(s) opened by the reviewer
Fenrir result: Approved ✅
No new issues found in the changed files.
Advisory only — this automated result does not count as a GitHub approval.
Review tier: Lite
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1275
Fenrir already completed a review of this PR at commit 48243dee851b (run 3031); its findings are the review threads on the PR. Push a new commit to request another review.
The KEX shared secret K stayed in ssh->k until the session was freed. It is now wiped as soon as the session keys are derived, whether the exchange succeeds or fails.