Zeroize SFTP file payload buffers before freeing#1053
Open
yosuke-wolfssl wants to merge 1 commit into
Open
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aims to harden SFTP data handling by wiping (zeroizing) heap buffers that may contain transferred file contents before they are freed, reducing the risk of sensitive data remaining recoverable from heap memory after WFREE().
Changes:
- Update
wolfSSH_SFTP_buffer_free()to callForceZero()before freeingbuffer->data. - Update
wolfSSH_SFTP_RecvSetSend()to callForceZero()before freeing the previousstate->buffer.dataallocation (when it’s a distinct allocation).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
5a3a6a3 to
e86ab20
Compare
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1053
Scan targets checked: wolfssh-bugs, wolfssh-src
No new issues found in the changed files. ✅
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.
Zeroize SFTP file payload buffers before freeing
Summary
The shared SFTP receive/send buffer can hold transferred file contents, but it
was released with
WFREEonly. Freed heap could retain SFTP file data untilallocator reuse, exposing sensitive file contents to local memory disclosure or
forensic tooling.
This PR zeroizes SFTP buffers that may carry file payload before they are freed
or trimmed, and adds a build gate so deployments can opt out for bulk-transfer
throughput.
Changes
Zeroization
wolfSSH_SFTP_buffer_free()—ForceZero(buffer->data, buffer->sz)beforeWFREE. Covers the read-response path and every per-state buffer release thatfunnels through this helper.
wolfSSH_SFTP_RecvSetSend()—ForceZerobefore theWFREE, kept insidethe existing
buf != state->buffer.dataaliasing guard so the outgoingresponse buffer (when it aliases the request buffer) is never wiped.
wolfSSH_SFTP_buffer_set_size()— wipe the trimmed-off region(
ForceZero(buffer->data + sz, buffer->sz - sz)) before shrinking. This helperreduces a buffer's logical length without reallocating, so without this a
shrink could strand file payload past the new size. Pairing the shrink-time
wipe with the free-time wipe gives a clean invariant: every sensitive byte is
cleared either when it is trimmed off or when the buffer is freed.
ForceZerois already declared inwolfssh/misc.h(included bywolfsftp.c);no new include is needed. All three functions are
static— no public API orsignature change.
Build gate
New
WOLFSSH_NO_SFTP_BUFFER_ZEROmacro gates the three zeroization sites,secure-by-default (zeroization enabled unless explicitly disabled). The wipe of
bulk read/write response buffers is a measurable per-chunk cost on large
transfers, so deployments that prioritize throughput over this defense-in-depth
can opt out.
configure.ac—--disable-sftp-zeroize(default enabled), emits-DWOLFSSH_NO_SFTP_BUFFER_ZEROwhen disabled, plus a config-summary line.#ifndefguards (no wrapper macro) soForceZerostays visible for security auditing.
CI
.github/workflows/os-check.yml— added--enable-sftp --disable-sftp-zeroizeto the build matrix so the disabled code path is compiled and
make check-edon ubuntu/macos across the wolfSSL version matrix. The default (zeroize-on)
path is already covered by the existing
--enable-sftp/--enable-allentries.
Scope notes
This wipes buffers that actually carry file payload. It intentionally does not
wipe never-written uninitialized slack (e.g. the tail of a short-read server
response buffer), which never held this request's file data and is outside the
finding's scope. The client read path reads file data into the caller-provided
outbuffer, not a wolfSSH-owned buffer, so there is nothing for wolfSSH towipe there.
Testing
./configure --enable-sftp && make && make check— 7/7 pass../configure --enable-sftp --disable-sftp-zeroize && make && make check—7/7 pass (disabled path builds and runs).