Skip to content

fix: harden bounds/underflow paths in wolfSSH - #1096

Merged
JacobBarthelmeh merged 4 commits into
wolfSSL:masterfrom
MarkAtwood:fix/wolfssh-bounds_underflow
Jul 28, 2026
Merged

fix: harden bounds/underflow paths in wolfSSH#1096
JacobBarthelmeh merged 4 commits into
wolfSSL:masterfrom
MarkAtwood:fix/wolfssh-bounds_underflow

Conversation

@MarkAtwood

Copy link
Copy Markdown
Contributor

Batch of integer-underflow / bounds-check hardening fixes reported by Fenrir static analysis. Build-verified together on Ubuntu 24.04 with --enable-all against a --enable-all wolfSSL (make -j8, exit 0).

  • #2480 src/internal.c ChannelNew — skip channel ids already in use before assigning, so a wrapped word32 nextChannel cannot collide with a live channel.
  • #3449 src/internal.c GetNameList — off-by-one: accept a length prefix occupying exactly the final 4 bytes (>=>); downstream GetStringRef still fully validates bounds.
  • #3881 src/ssh.c wolfSSH_RealPath — guard segSz==0 / segSz>=outSz and reserve the separator+NUL bytes to stop the outSz - segSz unsigned underflow.
  • #4586 src/internal.c SendChannelData / SendChannelExtendedData — return WS_WINDOW_FULL when the computed bound is zero rather than performing a silent zero-length send; mirrors the existing peerWindowSz == 0 guard.
  • #4587 src/internal.c BuildNameList — return 0 for srcSz==0 before any *src deref or srcSz-- underflow.
  • #4592 src/agent.c FindKeyId — only compare keyBlob when keyBlobSz == id->keyBlobSz, so WMEMCMP never reads past id->keyBlob.
  • #4795 apps/wolfssh/common.c ClientPublicKeyCheck — require a 4-byte length prefix before ato32, eliminating the pubKeySz - sizeof(word32) underflow and 4-byte over-read when pubKeySz < 4.
  • #6525 src/internal.c DoKexDhReply — reject sigSz in [4,7] before the sigSz - begin - LENGTH_SZ subtraction can underflow.
  • #6693 src/agent.c DoMessage — reject payloadSz==0 so payloadSz - 1 (word32) cannot wrap to 0xFFFFFFFF.
  • #6697 src/wolfsftp.c wolfSSH_SFTP_DoStatus — clamp an out-of-range status to WOLFSSH_FTP_FAILURE so it cannot alias a negative WS_* error code and bypass caller classification.

Reported by Fenrir static analysis.

Copilot AI review requested due to automatic review settings July 10, 2026 00:28

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 applies a set of integer-underflow and bounds-check hardening fixes across wolfSSH core parsing, SFTP handling, agent message processing, and client-side public-key parsing, based on static analysis findings.

Changes:

  • Hardened path canonicalization, name-list parsing, channel allocation, KEX signature parsing, and channel window-bound handling in core SSH code.
  • Strengthened agent-side key matching and message payload length validation.
  • Added additional bounds validation/clamping in SFTP status handling and client public-key parsing.

Reviewed changes

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

Show a summary per file
File Description
src/wolfsftp.c Clamps out-of-range SFTP status codes to avoid aliasing internal negative error codes.
src/ssh.c Tightens RealPath bounds checks to prevent unsigned underflow in remaining-space calculations.
src/internal.c Adds channel-id collision avoidance on wrap, fixes name-list bounds edge, hardens KEX sig parsing, guards empty BuildNameList input, and handles zero computed send bounds.
src/agent.c Prevents out-of-bounds memcmp in key lookup and rejects zero-length agent payloads.
apps/wolfssh/common.c Requires a 4-byte length prefix before parsing key type length to avoid underflow/over-read.
Comments suppressed due to low confidence (1)

src/ssh.c:3847

  • wolfSSH_RealPath() uses WSTRNCAT(out, ..., outSz - curSz), but WSTRNCAT maps to wstrncat() where the third argument is the total destination buffer size (see src/port.c). Passing a shrinking "remaining" size can underflow the internal freeSpace calculation (size_t) and turn this into a buffer overflow. The bounds check added here should be paired with passing the full outSz (and ideally checking WSTRNCAT's return) to make the concatenation actually safe.
            if (segSz == 0 || segSz >= outSz ||
                    curSz >= outSz - segSz - (curSz != 1 ? 1 : 0)) {
                return WS_INVALID_PATH_E;
            }

            if (curSz != 1) {
                WSTRNCAT(out, "/", outSz - curSz);
                curSz++;
            }
            WSTRNCAT(out, seg, outSz - curSz);

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

Comment thread src/internal.c
MarkAtwood and others added 4 commits July 25, 2026 01:03
Address a batch of integer-underflow and bounds findings from Fenrir
static analysis.

- src/internal.c ChannelNew: skip channel ids already in use so a wrapped
  word32 nextChannel cannot collide with a live channel.
- src/internal.c GetNameList: drop the length pre-check. GetStringRef
  bounds both the length prefix and the list, and the pre-check also
  rejected a valid empty name list ending exactly at len.
- src/internal.c DoKexDhReply: parse the signature name and blob with
  GetStringRef and GetSize instead of GetUint32 plus hand-rolled
  remainder checks, which underflowed sigSz - begin - LENGTH_SZ for a
  sigSz of 4 to 7 and let a name comparison read past the packet. Retires
  the redundant sigSz + begin + tmpIdx > len check in the RSA path.
- src/internal.c BuildNameList: return 0 for srcSz == 0, before any *src
  deref or srcSz-- underflow, terminating buf for the callers that
  measure it with WSTRLEN.
- src/internal.c SendChannelData/SendChannelExtendedData: report
  WS_WINDOW_FULL when the computed bound is zero, rather than sending a
  zero-length packet.
- src/wolfsftp.c wolfSSH_SFTP_DoStatus: clamp out-of-range status to
  WOLFSSH_FTP_FAILURE so it cannot alias a negative WS_* error code.

A malformed KEXDH signature length prefix now reports WS_BUFFER_E from
the parse helper rather than WS_PARSE_E. The signature name mismatch
still reports WS_PARSE_E.

Issue: F-2480, F-3449, F-4586, F-4587, F-6525, F-6697
DoMessage hand-rolled the payload length check: a five-byte room test,
then ato32, then payloadSz > len - begin. GetSize does both bounds in one
call, and a nonzero payloadSz already covers the message id byte that the
MSG_ID_SZ term reserved. The failure still reports WS_OVERFLOW_E rather
than the helper's WS_BUFFER_E, so the agent's error codes are unchanged.

Issue: F-6693
None of the three paths this branch changed had coverage, which is why
make check passed both before and after the SendChannelData regression.

- unit: SendChannelData and SendChannelExtendedData with a peer maximum
  packet size of 0 report WS_WINDOW_FULL, queue nothing, and leave
  peerWindowSz alone. A zero-length send still succeeds.
- unit: BuildNameList terminates buf for an empty id list. The buffer is
  poisoned first, so a missing terminator shows up as a wrong length
  instead of depending on what the allocator handed back. Reaches the
  static function through a new wolfSSH_TestBuildNameList hook.
- regress: a KEXDH_REPLY whose signature blob holds nothing but a name
  length prefix is rejected with WS_BUFFER_E, pinning the bounded read
  that replaced the hand-rolled name parse.

Each test was confirmed to fail with its fix reverted.

Issue: F-4586, F-4587, F-6525
- src/internal.c DoKexDhReply: drop the stale fuzz note on the signature
  size. The size checks it pointed at stay.
- tests/unit.c: replace an em-dash with a comma, for ASCII-only sources.
@ejohnstown
ejohnstown force-pushed the fix/wolfssh-bounds_underflow branch from b998962 to 00880c6 Compare July 27, 2026 14:35
@JacobBarthelmeh
JacobBarthelmeh merged commit 51a7eca into wolfSSL:master Jul 28, 2026
142 checks passed
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.

5 participants