Repository navigation
scp: wipe file data in the SCP transfer buffer - #1376
ejohnstown wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
No active review issues remain for the changes that reduce SCP plaintext retention in session memory.
No open issues 🎉
What changed in this PR
This PR reduces how long SCP file plaintext remains in session memory by wiping transfer buffers after use and across their allocation lifecycle.
Changes:
- Wipes received chunks after callback delivery and sent bytes after buffer compaction.
- Clears full buffers before replacement or release, initializes receive buffers, and resets stale chunk lengths for empty files.
- Adds test-only hooks and regression checks for buffer wiping and empty-file handling.
| File | Description |
|---|---|
wolfssh/internal.h |
Documents buffer size semantics and declares test-only SCP hooks. |
tests/unit.c |
Tests empty-file callback lengths and full-buffer wiping before release or replacement. |
tests/api.c |
Checks that completed SCP transfers leave no file data in the buffer. |
src/wolfscp.c |
Adds transfer-buffer wiping, initialization, length reset, and test hooks. |
src/internal.c |
Wipes the full SCP buffer allocation during session cleanup. |
Review effort: Balanced
|
@claude review |
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. It changes how file plaintext is kept in the SCP transfer buffer, which is data-exposure hardening in an SSH library, so a maintainer should still look at it.
What was reviewed:
- Every allocation of
scpFileBuffer(ScpSourceInit,ReceiveScpFile) usesDEFAULT_SCP_BUFFER_SZ. That makes the full-sizeWS_FORCEZEROinSshResourceFreeandScpSourceInitstay in bounds. - Source path after a partial send: after the
WMEMMOVE, the zeroed range[scpBufferedSz, scpBufferedSz+ret)is exactly the vacated tail. - Sink path: zeroing each chunk after the receive callback, and resetting
scpFileBufferSzto 0 inReceiveScpFile. With the reset, an empty file getsbufSz == 0, and aWS_WANT_READreturn still leaves the state consistent.
Extended reasoning...
The change touches the SCP sink and source code in src/wolfscp.c, the buffer free in src/internal.c, field comments and two test-only wrappers in wolfssh/internal.h, and new unit and api tests. Its security-sensitive surface is data exposure: it wipes transferred file plaintext from session memory and stops stale buffer bytes from reaching the sink callback. The code changes are small, consistent with the existing WS_FORCEZERO/WFREE conventions, and covered by new tests. I deferred instead of approving because this is hardening in an SSH library. There is also an earlier Copilot review whose contents I can't see, so I can't confirm it has been addressed.
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1376
Scan targets checked: wolfssh-src, wolfssh-bugs
Coverage: 3 of 5 in-scope changed file(s) opened by the reviewer; not opened: src/internal.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
The sink wipes each received chunk once the receive callback has it, and the source wipes the bytes it has sent. Freeing the buffer wipes its full DEFAULT_SCP_BUFFER_SZ allocation, since scpFileBufferSz holds only the last chunk length on the sink side and is zero by teardown. ReceiveScpFile zeroes the buffer when it allocates it, as ScpSourceInit does, so no stale heap bytes sit past the received chunk. - DoScpSink: zero the chunk before resetting scpFileBufferSz - DoScpSource: zero the sent span after compacting the buffer - ScpSourceInit, SshResourceFree: zero the whole allocation - add wolfSSH_TestDoScpSource under WOLFSSH_TEST_INTERNAL - tests: check the buffer is clear after the SCP rekey transfers - unit: ScpFileBuffer_zeroedOnFree reads the buffer back after SshResourceFree and after ScpSourceInit replaces it Issue: F-8847, F-8844, F-10595
ReceiveScpFile clears scpFileBufferSz before it reads, so an empty file hands the receive callback a zero-length chunk. A source run leaves the buffer capacity in that field, and a later sink of an empty file on the same session passed the whole stale buffer to the callback as data. - ReceiveScpFile: zero scpFileBufferSz before the early return - add wolfSSH_TestDoScpSink under WOLFSSH_TEST_INTERNAL - unit: ScpSinkEmptyFileAfterSource checks the empty-file chunk size
278084c to
3db66a8
Compare
The SCP sink and source wipe file data from the transfer buffer once it
is handed off or sent, and the whole buffer is zeroed when it is
allocated, replaced or freed.
passes stale buffer bytes to the receive callback
Issue: F-8847, F-8844, F-10595