Channel and connection layer input-handling fixes - #1177
Open
ejohnstown wants to merge 7 commits into
Open
Conversation
MAX_PACKET_SZ caps the whole SSH binary packet, but the channel maxPacketSz it was compared against counts only channel payload. A peer honoring the advertised 35000 overruns the receiver's own check. - Derive MAX_CHANNEL_PACKET_SZ in internal.h: MAX_PACKET_SZ less the transport framing, the CHANNEL_EXTENDED_DATA header, the worst-case padding BundlePacket() picks, and MAX_HMAC_SZ. 34899 by default. - Name that overhead twice, once for the compiler and once as a literal for the preprocessor, which reads the wolfCrypt enum constants in the first form as zero. The #error guarding DEFAULT_MAX_PACKET_SZ uses the second rather than its own copy. - MAX_CHANNEL_PACKET_SZ is derived rather than a tunable, so it is not overridable; an override defeated the bound it enforces. - wolfSSH_CTX_SetWindowPacketSize() bounds maxPacketSz against that instead of MAX_PACKET_SZ; DEFAULT_MAX_PACKET_SZ is unaffected. - api.c tests the new edge and that MAX_PACKET_SZ is now rejected. Issue: F-8835
wolfSSH_SetChannelType() discarded an exec or subsystem name it could not use and still returned WS_SUCCESS. SendChannelRequest() then omits the name field entirely, which the peer rejects as malformed, dropping the connection. Both an oversized name and an empty one reach it; the empty case is reachable from the command line as "wolfssh -c ''". - Return WS_BAD_ARGUMENT for a name at or above WOLFSSH_MAX_CHN_NAMESZ, matching how the function already reports a bad type or side. - Return WS_BAD_ARGUMENT when no name is given and none was stored by an earlier call, and when a size arrives with no name behind it. - Keep returning WS_SUCCESS when an earlier call stored a name, which is what the SFTP and SCP retry loops depend on. - Return before setting connectChannelId so a rejected call leaves no state behind, as the server-side exec rejection does. - Keep the stored name intact when a later call is refused. - api.c asserts each refusal, and the largest name still admitted.
DoPacket stepped to the next packet using payloadIdx, which handlers set to however much they read. The default case reads none of an unimplemented message's payload, leaving the cursor short by that much. - Advance inputBuffer.idx by UINT32_SZ + curSz from the packet start, the length DoReceive already bounds-checked, so a handler that ignores trailing bytes cannot move the next packet's start. - Snapshot curSz on entry beside the packet start, so the frame is computed entirely from entry-time state. Reading it back after the handler switch would describe the next packet if a handler ever re-entered the receive path. - Covers DoIgnore, DoDebug, DoUnimplemented, DoChannelSuccess and DoChannelFailure, which are all short on a padded payload. - Clamp to the buffer length on the WS_BUFFER_E path. - unit.c pins the framing across an unimplemented message. Note the short cursor is not currently observable: DoReceive calls ShrinkBuffer() with forcedFree, which drops the rest of the buffer after every packet. This is hardening, not a live desync. Issue: F-8825
The four pty-req and window-change dimensions were decoded straight into the WOLFSSH fields and handed to the resize callback unchecked. The consumers copy them into the unsigned short fields of a struct winsize for TIOCSWINSZ, so anything above 65535 wraps, and 0x10000 arrives as a 0x0 terminal. - Add SetTerminalSize() and route both the pty-req and window-change branches through it, so pty-req stops decoding straight into the WOLFSSH fields. - Clamp all four to TERMINAL_DIMENSION_MAX. Others truncate at the ioctl and accept it, but wolfSSH hands the word32 values to termResizeCb first, so an unclamped dimension escapes the library rather than being cut down on the way to the ioctl. - Take a zero dimension as sent. Others do too, and a zero is how a peer reports a dimension it has no information about. - unit.c drives all four dimensions from one table, covering the zero, single-zero and wrapping cases, in an error code range no other case in the function claims. F-8833 recommended ignoring a zero dimension. That is declined above: Others take zeros as sent, and a zero is how a peer reports a dimension it has no information about. The finding's symptom, a 0x0 terminal, is also reached by a route it did not identify, a dimension above 65535 wrapping, and the clamp closes that one. Issue: F-8833
A window-change arriving before any pty-req had nothing to resize, but the size was stored and the resize callback run anyway. Dropbear refuses the same request for the same reason. - Reject it with the existing rej path, so no reply is sent for a request RFC 4254 sec 6.7 says takes none, and the session continues. - unit.c covers the rejection, and now drives a real pty-req, which had no coverage on the receive side at all.
The refusals added for names the peer cannot use changed the return contract of a public API whose block comment still promised only WS_SUCCESS. There is no dox_comments entry, so that comment is all an embedder has. - Spell out each WS_BAD_ARGUMENT case, the keep-the-stored-name rule, and that a refused call leaves the selected type alone. - api.c asserts connectChannelId across the refusals. It is the field SendChannelRequest() switches on, so moving the checks back below the assignment would otherwise pass.
CHANNEL_PACKET_OVERHEAD_MAX is a hand-computed copy of CHANNEL_PACKET_OVERHEAD_SZ, needed because the expression bottoms out in wolfCrypt enum constants that #if reads as zero. Nothing tied the two together, so a term added to the expression would leave the #error guarding DEFAULT_MAX_PACKET_SZ silently ineffective. Assert the bound in internal.c, where both are ordinary constant expressions, with a negative-array-size typedef.
ejohnstown
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Hardens wolfSSH channel/connection input handling by correcting packet-size bounds, tightening channel-type/name validation, making packet framing robust to handlers that don’t consume payload, and clamping/rejecting problematic terminal resize requests.
Changes:
- Derive and enforce
MAX_CHANNEL_PACKET_SZ(wire-safe channel payload max) and add compile-time consistency checks. - Make
wolfSSH_SetChannelType()reject missing/oversized subsystem/exec names instead of silently sending malformed requests. - Harden
DoPacket()framing and add terminal-dimension clamping + rejectwindow-changewhen no PTY exists; expand unit/api test coverage.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfssh/internal.h | Adds terminal dimension max and derives channel payload max from wire packet limit with compile-time guards. |
| src/ssh.c | Tightens wolfSSH_SetChannelType() validation and fixes maxPacketSz bound to use channel payload limit. |
| src/internal.c | Adds terminal size clamping helper; rejects window-change without PTY; uses validated packet length for framing. |
| tests/api.c | Updates/extends API tests for wolfSSH_SetChannelType() and wolfSSH_CTX_SetWindowPacketSize() bounds. |
| tests/unit.c | Adds a DoPacket framing regression test and expands channel-request terminal sizing tests. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
11503
to
+11507
| channel->ptyReq = 1; /* received a pty request */ | ||
| termSz = (word32)sizeof(term); | ||
| ret = GetString(term, &termSz, buf, len, &begin); | ||
| if (ret == WS_SUCCESS) | ||
| ret = GetUint32(&ssh->widthChar, buf, len, &begin); | ||
| ret = GetUint32(&widthChar, buf, len, &begin); |
Comment on lines
+732
to
+745
| /* What a channel data packet carries besides its payload: the transport | ||
| * framing, the larger of the two channel data headers (CHANNEL_EXTENDED_DATA), | ||
| * the worst-case padding BundlePacket() can pick, and the largest MAC. | ||
| * The MAC term is the one that varies, so it is spelled twice: once with | ||
| * MAX_HMAC_SZ for the compiler, and once as a literal 64, the largest | ||
| * wolfCrypt digest, for the preprocessor. AES_BLOCK_SIZE and MAX_HMAC_SZ are | ||
| * wolfCrypt enum constants, which #if reads as zero, so the #error below | ||
| * cannot use the macro form. Both come to 4+1+1+8+4+19+64 = 101. */ | ||
| #define CHANNEL_PACKET_OVERHEAD_SZ \ | ||
| (LENGTH_SZ + PAD_LENGTH_SZ \ | ||
| + MSG_ID_SZ + (UINT32_SZ * 2) + LENGTH_SZ \ | ||
| + (AES_BLOCK_SIZE + MIN_PAD_LENGTH - 1) \ | ||
| + MAX_HMAC_SZ) | ||
| #define CHANNEL_PACKET_OVERHEAD_MAX 101 |
|
|
||
| /* Largest channel payload that still fits MAX_PACKET_SZ on the wire, which | ||
| * bounds the whole binary packet. Comes to 35000 - 101 = 34899. Derived, not | ||
| * a tunable, so it is deliberately not overridable. */ |
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.
Seven independent input-handling fixes in the channel and connection layer: a packet-size bound checked against the wrong limit, a public API accepting a channel name it then failed to send, packet framing taken from the handler's cursor, and two terminal-size problems. All covered by api.c or unit.c tests.
Channel maxPacketSz bound (F-8835)
wolfSSH_CTX_SetWindowPacketSize()checked the channel payload limit againstMAX_PACKET_SZ, which caps the whole binary packet. Bound it against a new derivedMAX_CHANNEL_PACKET_SZinstead.wolfSSH_SetChannelType()
WS_SUCCESS, and the request went out malformed. NowWS_BAD_ARGUMENT, beforeconnectChannelIdis set.DoPacket framing (F-8825)
DoReceive-validated length rather thanpayloadIdx, which a handler ignoring trailing bytes leaves short. Hardening, not a live desync.Terminal dimensions (F-8833)
SetTerminalSize(); above 65535 they wrap in astruct winsize. Zero is still taken as sent.