Repository navigation
Free only the KEX keys that were initialized - #1362
Open
ejohnstown wants to merge 1 commit into
Open
ejohnstown wants to merge 1 commit into
ejohnstown wants to merge 1 commit into
Conversation
SendKexDhReply and the ECDH and Curve25519 KeyAgree functions now track which keys were initialized and free only those. A host-key format with no loaded key, or a failed small-stack allocation, no longer hands never-initialized storage to the wolfCrypt free. - sigKeyBlockFull gains keyInit, set by SendKexGetSigningKey - per-key Inited flags in the client and server KeyAgree functions - regress: no matching host key frees nothing (WOLF_CRYPTO_CB_FREE) - unit: server KeyAgree survives an allocation failure (small stack) Issue: F-11675, F-11939
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues are recorded for the changes guarding key-exchange cleanup against freeing uninitialized keys.
No open issues 🎉
What changed in this PR
This PR hardens key-exchange cleanup so wolfSSH frees only keys that were successfully initialized, avoiding invalid cleanup when a host key is missing or a small-stack allocation fails.
Changes:
- Tracks initialization of signing keys and temporary ECDH and Curve25519 keys before freeing them.
- Adds regression coverage for a missing matching host key and allocation-failure tests for server key agreement.
| File | Description |
|---|---|
wolfssh/internal.h |
Declares the Curve25519 server key-agreement test hook. |
tests/unit.c |
Adds fault-injected allocation tests that detect cleanup of uninitialized keys. |
tests/regress.c |
Tests missing-host-key failure without freeing an uninitialized RSA key. |
src/internal.c |
Guards key cleanup with initialization flags and exposes the new test hook. |
Review effort: Balanced
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1362
Scan targets checked: wolfssh-src, wolfssh-bugs
Coverage: 2 of 4 in-scope changed file(s) opened by the reviewer; not opened: tests/unit.c, wolfssh/internal.h
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
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.
SendKexDhReply() and the ECDH and Curve25519 KeyAgree functions free
only the keys they initialized, so a missing host key or a failed
small-stack allocation no longer frees uninitialized storage.
Issue: F-11675, F-11939