Repository navigation
agent: wipe and free agent buffers - #1370
ejohnstown wants to merge 5 commits into
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The allocator-hook test checks only allocation counts and would pass without message wiping, leaving the security behavior unverified for sign-request cleanup and destruction with an outstanding message.
1 open issue
What changed in this PR
This PR hardens SSH agent buffer cleanup by wiping message data before release and fixing a failure-reply leak.
Changes:
- Centralizes message cleanup and securely clears agent state.
- Rejects overflowing key-type lengths and returns an explicit unimplemented error from the agent worker.
- Adds agent regression tests and guards allocator-hook tests with
USE_WOLFSSL_MEMORY.
| File | Description |
|---|---|
| tests/unit.c | Requires allocator-hook support for capturing-allocator tests. |
| tests/regress.c | Guards SFTP allocator-failure tests and their calls. |
| tests/api.c | Tests unsupported replies, reply cleanup, and key-type length overflow. |
| src/agent.c | Adds secure cleanup, fixes the reply leak, and corrects bounds checking and worker status. |
Review effort: Balanced
AgentMsgFree() force-zeroes and releases agent->msg, which can hold key blobs, comments, data to sign and signatures. Every site that freed agent->msg now goes through it, and the context teardown wipes with WS_FORCEZERO, which the compiler cannot drop before the free. - AgentInit() failure path and wolfSSH_AGENT_free() use WS_FORCEZERO instead of WMEMSET - api.c: a free hook checks the sign-request message and one left outstanding at wolfSSH_free() are wiped before release Issue: F-13999, F-14000
SendFailure() wipes and frees the reply once it is bundled, as the other senders do. wolfSSH_AGENT_SignRequest() gets one through DoUnimplemented() for a reply id it does not handle, and dropped the pointer at cleanup. - api.c drives SignRequest with an EXTENSION_FAILURE reply and, under USE_WOLFSSL_MEMORY, counts allocations across the second exchange Issue: F-16054
wolfSSH_AGENT_worker() returns WS_UNIMPLEMENTED_E instead of calling SendSuccess() and DoMessage() with NULL arguments and claiming success. It has no callers. SendSuccess() is unused without it, so it joins SendRequestIdentities() under #if 0. Issue: F-10562
DoAddIdentity() compares the key-type length against the bytes left rather than adding it to the offset, so a length near UINT32_MAX can no longer wrap past the check. - api.c sends an ADD_IDENTITY reply with a 0xFFFFFFFF key-type length Issue: F-11937
wolfSSL defines wolfSSL_SetAllocators() and wolfSSL_GetAllocators() only under USE_WOLFSSL_MEMORY, so a wolfSSL built with NO_WOLFSSL_MEMORY or XMALLOC_USER left unit.test and regress.test unable to link. The capturing-allocator tests in unit.c and the SFTP allocation-failure tests in regress.c now also require USE_WOLFSSL_MEMORY.
e08f0eb to
1b29f29
Compare
|
@claude review |
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1370
Scan targets checked: wolfssh-src, wolfssh-bugs
Coverage: 1 of 4 in-scope changed file(s) opened by the reviewer; not opened: tests/api.c, tests/regress.c, tests/unit.c
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
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It changes how SSH agent buffers holding key and signature material are wiped, and it changes the DoAddIdentity parser bounds check, so a human look is still worthwhile. Also, the Copilot inline thread at src/agent.c:84 was resolved by the PR author, not by an independent reviewer.
What was reviewed:
- AgentMsgFree() wipes msgSz bytes. After BundleMessage that is the number of bytes actually written, so the unwiped tail of the allocation never held message data.
- The DoAddIdentity check
sz > len - begincan't underflow, because the earlierbegin + LENGTH_SZ > lencheck already guarantees begin <= len. - wolfSSH_AGENT_worker is WOLFSSH_LOCAL and nothing calls it, so returning WS_UNIMPLEMENTED_E affects no callers.
- SendFailure's caller (DoMessage) doesn't use agent->msg after SendFailure returns, so freeing it there is safe.
Extended reasoning...
The change touches the wolfSSH agent in src/agent.c (lifecycle of message buffers that hold keys and signatures, the add-identity parse bounds check, and context wipes) and adds allocator-hook tests in tests/api.c, regress.c and unit.c. The security-sensitive surface is key material handling and parsing of untrusted agent input. No defects were found. I deferred because the change is in crypto- and parsing-sensitive code, and because the only other reviewer's inline thread was resolved by the PR author with no independent sign-off.
Agent message buffers, which can hold keys and signatures, are wiped
with WS_FORCEZERO before they are freed, and a reply SendFailure()
builds is no longer leaked.
success
regress link against a wolfSSL without it
Issue: F-13999, F-14000, F-16054, F-10562, F-11937