Skip to content

Buffer client SFTP VERSION header across partial reads - #1138

Open
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/f_7505
Open

Buffer client SFTP VERSION header across partial reads#1138
yosuke-wolfssl wants to merge 1 commit into
wolfSSL:masterfrom
yosuke-wolfssl:fix/f_7505

Conversation

@yosuke-wolfssl

@yosuke-wolfssl yosuke-wolfssl commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Problem

SFTP_ClientRecvInit() read the 9-byte SFTP VERSION header into a stack buffer and required all 9 bytes from a single wolfSSH_stream_read():

if ((len = wolfSSH_stream_read(ssh, buf, sizeof(buf))) != sizeof(buf)) {
    /* @TODO partial read on small packet */
    return len;
}

A short-but-positive read consumed those bytes off the channel and discarded them — nothing recorded an offset. wolfSSH_SFTP_connect() collapsed the result to WS_FATAL_ERROR while leaving ssh->sftpState at SFTP_RECV, so a retry parsed the remaining header bytes as a new packet and negotiation could not recover. wolfSSH_stream_read() also leaves ssh->error at WS_SUCCESS on a short read, so the standard WS_WANT_READ retry loop treated it as fatal and gave up. Triggered by any server whose VERSION message arrives split across SSH channel-data messages, or a non-blocking read that returns partway.

Fix (src/wolfsftp.c)

The server path already solved this; the client is now the same shape.

  • SFTP_ClientRecvInit() accumulates into ssh->recvInitState->buffer via wolfSSH_SFTP_buffer_read(), which retains buffer->idx across calls and reports WS_WANT_READ until complete. The SFTP_EXT arm went the same way, replacing its per-call WMALLOC/discard logic.
  • wolfSSH_SFTP_connect() gates cleanup on NoticeError() like wolfSSH_SFTP_accept() does: the partial buffer survives a retryable status, is freed otherwise, and STATE_ID_RECV_INIT is cleared on success. Return value stays WS_FATAL_ERROR — no API change.
  • wolfssh/internal.h: sftpExtSz was the client path's only user and is now dead; removed (noinst header, not public ABI).

This closes f-7505.

Tests (tests/unit.c)

SftpClientRecvInitSplit delivers a VERSION message in two pieces, split once inside the header and once inside the trailing extension data. SftpClientRecvInitVersion covers the four single-read outcomes: bad declared size, wrong message type, version below v3 (all must fail and release the buffered state), and version above v3 (must still negotiate).

Verification

  • make check: 10 passed, 1 skipped, 0 failed.
  • GCC -Werror preflight clean across 6 configs; ASan + UBSan clean on unit.test and testsuite.test.
  • Negative controls: reverting the fix, stripping the teardown clears, and mutating the version guard (<= and !=) each fail the corresponding test.

@yosuke-wolfssl yosuke-wolfssl self-assigned this Aug 3, 2026
Copilot AI review requested due to automatic review settings August 3, 2026 02:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a client-side SFTP negotiation bug where the 9-byte SSH_FXP_VERSION header could be partially read, consumed, and then lost across retries—preventing recovery on WS_WANT_READ/partial delivery. The client negotiation path is updated to buffer the VERSION message across calls, matching the server-side buffering approach already used elsewhere in the codebase.

Changes:

  • Reworks client VERSION receive (SFTP_ClientRecvInit) to accumulate header and extension bytes using the reusable SFTP buffer/state mechanism instead of a stack buffer and single-shot reads.
  • Updates wolfSSH_SFTP_connect() to preserve buffered negotiation state on retryable conditions (via NoticeError()), and to clear the recv-init state on success.
  • Adds unit tests covering split VERSION delivery and various VERSION validation outcomes; removes the now-dead sftpExtSz field from the internal session struct.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
src/wolfsftp.c Buffers client-side VERSION header/extensions across partial reads and adjusts connect cleanup semantics to preserve retryable state.
tests/unit.c Adds unit tests for split VERSION delivery and VERSION validation/teardown behavior.
wolfssh/internal.h Removes unused internal sftpExtSz field now that client extensions are buffered via recv-init state.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/unit.c

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fenrir Automated Review — PR #1138

Scan targets checked: wolfssh-bugs, wolfssh-src

Findings: 1
1 finding(s) posted as inline comments (see file-level comments below)

This review was generated automatically by Fenrir. Findings are non-blocking.

Comment thread tests/unit.c
Comment thread tests/unit.c

@wolfSSL-Fenrir-bot wolfSSL-Fenrir-bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fenrir Automated Review — PR #1138

Scan targets checked: wolfssh-bugs, wolfssh-src

No new issues found in the changed files. ✅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants