Skip to content

fix: harden bounds/underflow paths in wolfSSH#1096

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

fix: harden bounds/underflow paths in wolfSSH#1096
MarkAtwood wants to merge 1 commit into
wolfSSL:masterfrom
MarkAtwood:fix/wolfssh-bounds_underflow

Conversation

@MarkAtwood

Copy link
Copy Markdown

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.

Address a batch of integer-underflow and bounds findings from Fenrir
static analysis. Build-verified together (--enable-all, wolfSSL
--enable-all) on Ubuntu 24.04.

- #2480 src/internal.c ChannelNew: skip channel ids already in use 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 (>= -> >).
- #3881 src/ssh.c wolfSSH_RealPath: guard segSz==0/segSz>=outSz and
  reserve separator+NUL bytes to stop the outSz-segSz underflow.
- #4586 src/internal.c SendChannelData/SendChannelExtendedData: report
  WS_WINDOW_FULL when the computed bound is zero instead of a silent
  zero-length send.
- #4587 src/internal.c BuildNameList: return 0 for srcSz==0 before any
  *src deref or srcSz-- underflow.
- #4592 src/agent.c FindKeyId: compare keyBlob only when sizes match, so
  WMEMCMP never reads past id->keyBlob.
- #4795 apps/wolfssh/common.c ClientPublicKeyCheck: require a 4-byte
  length prefix before ato32 to stop the pubKeySz-sizeof(word32)
  underflow and 4-byte over-read.
- #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 cannot
  wrap to 0xFFFFFFFF.
- #6697 src/wolfsftp.c wolfSSH_SFTP_DoStatus: clamp out-of-range status
  to WOLFSSH_FTP_FAILURE so it cannot alias a negative WS_* error code.
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
Comment on lines +12372 to +12374
if (srcSz == 0) {
return 0;
}
@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