fix: harden bounds/underflow paths in wolfSSH#1096
Open
MarkAtwood wants to merge 1 commit into
Open
Conversation
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.
Contributor
There was a problem hiding this comment.
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 on lines
+12372
to
+12374
| if (srcSz == 0) { | ||
| return 0; | ||
| } |
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.
Batch of integer-underflow / bounds-check hardening fixes reported by Fenrir static analysis. Build-verified together on Ubuntu 24.04 with
--enable-allagainst a--enable-allwolfSSL (make -j8, exit 0).src/internal.cChannelNew— skip channel ids already in use before assigning, so a wrappedword32nextChannelcannot collide with a live channel.src/internal.cGetNameList— off-by-one: accept a length prefix occupying exactly the final 4 bytes (>=→>); downstreamGetStringRefstill fully validates bounds.src/ssh.cwolfSSH_RealPath— guardsegSz==0/segSz>=outSzand reserve the separator+NUL bytes to stop theoutSz - segSzunsigned underflow.src/internal.cSendChannelData/SendChannelExtendedData— returnWS_WINDOW_FULLwhen the computedboundis zero rather than performing a silent zero-length send; mirrors the existingpeerWindowSz == 0guard.src/internal.cBuildNameList— return0forsrcSz==0before any*srcderef orsrcSz--underflow.src/agent.cFindKeyId— only comparekeyBlobwhenkeyBlobSz == id->keyBlobSz, soWMEMCMPnever reads pastid->keyBlob.apps/wolfssh/common.cClientPublicKeyCheck— require a 4-byte length prefix beforeato32, eliminating thepubKeySz - sizeof(word32)underflow and 4-byte over-read whenpubKeySz < 4.src/internal.cDoKexDhReply— rejectsigSzin[4,7]before thesigSz - begin - LENGTH_SZsubtraction can underflow.src/agent.cDoMessage— rejectpayloadSz==0sopayloadSz - 1(word32) cannot wrap to0xFFFFFFFF.src/wolfsftp.cwolfSSH_SFTP_DoStatus— clamp an out-of-range status toWOLFSSH_FTP_FAILUREso it cannot alias a negativeWS_*error code and bypass caller classification.Reported by Fenrir static analysis.