Skip to content

fix: buffer write hardening across wolfSSH#1094

Open
MarkAtwood wants to merge 1 commit into
wolfSSL:masterfrom
MarkAtwood:fix/wolfssh-buffer_write
Open

fix: buffer write hardening across wolfSSH#1094
MarkAtwood wants to merge 1 commit into
wolfSSL:masterfrom
MarkAtwood:fix/wolfssh-buffer_write

Conversation

@MarkAtwood

Copy link
Copy Markdown

Buffer-write hardening across wolfSSH. Three independent fixes reported by Fenrir static analysis, batched into one PR. Build-verified on Ubuntu 24.04 against wolfSSL --enable-all --enable-ssh (./configure --enable-all && make -j8, exit 0).

  • #3446examples/portfwd/portfwd.c and examples/client/common.c: clamp passwordSz to sizeof(userPassword) (256) before the memcpy. Without the clamp, a defaultPassword longer than 256 bytes overflows the fixed userPassword buffer. userPassword is a raw password buffer whose length travels in password.passwordSz, so clamping without a NUL terminator is correct.
  • #4105examples/client/client.c and apps/wolfssh/wolfssh.c: in readPeer()'s Windows branch, WriteFile now writes (DWORD)ret (bytes actually read) instead of the full bufSz, matching the POSIX write(STDOUT_FILENO, buf, ret) sibling. Prevents emitting stale buffer tail on short reads.
  • #4106src/wolfterm.c cases 'D' and 'E': WS_WRITECONSOLE now writes sizeof("\n") - 1 (1 byte) instead of sizeof("\n") (2 bytes), dropping the stray trailing NUL from the console output.

The #4105 and #4106 sites are under USE_WINDOWS_API (not compiled on Linux); they are source-verified against the working POSIX path. The #3446 sites are compiled and covered by the successful Linux build.

Reported by Fenrir static analysis.

- #3446: clamp passwordSz to sizeof(userPassword) before memcpy in
  examples/portfwd/portfwd.c and examples/client/common.c, preventing
  an over-read/overflow when the default password exceeds 256 bytes.
- #4105: in readPeer() Windows branch, write (DWORD)ret bytes (the
  actual bytes read) instead of the full bufSz in examples/client/client.c
  and apps/wolfssh/wolfssh.c, matching the POSIX sibling.
- #4106: src/wolfterm.c cases 'D' and 'E' write sizeof("\n") - 1 (1 byte)
  instead of sizeof("\n") (2 bytes), dropping the stray trailing NUL.
Copilot AI review requested due to automatic review settings July 10, 2026 00:27

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 hardens several buffer-write sites across wolfSSH (mostly in examples and Windows-only console output paths) to prevent buffer overflows and unintended output of stale/NUL bytes.

Changes:

  • Clamp password copy length to the fixed userPassword[256] buffer size before memcpy in example authentication callbacks.
  • On Windows, write only the number of bytes actually read (ret) to stdout in readPeer() instead of the full buffer size.
  • In Windows console escape-sequence handling, write only the newline character (not the trailing NUL) via WriteConsole.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file
File Description
src/wolfterm.c Avoids writing the trailing NUL when emitting "\n" via WriteConsole in escape-sequence cases.
examples/portfwd/portfwd.c Clamps passwordSz to prevent overflow when copying defaultPassword into userPassword[256].
examples/client/common.c Same password-length clamp as port forwarding example to prevent overflow into userPassword[256].
examples/client/client.c Windows readPeer() now writes only ret bytes to stdout to avoid emitting stale buffer tail.
apps/wolfssh/wolfssh.c Same Windows readPeer() stdout write-length fix as the client example.

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

@MarkAtwood MarkAtwood requested a review from ejohnstown July 10, 2026 00:59
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.

3 participants