From f2e95c87c623aeafb18a0eb5fc6e5c8b30bf0b61 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Mon, 17 Aug 2026 13:36:46 -0700 Subject: [PATCH 1/7] Bound channel maxPacketSz below the wire limit 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 --- src/ssh.c | 2 +- tests/api.c | 14 +++++++++----- wolfssh/internal.h | 29 +++++++++++++++++++++++++++++ 3 files changed, 39 insertions(+), 6 deletions(-) diff --git a/src/ssh.c b/src/ssh.c index 1f3f7cc49..ed78d9939 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -3314,7 +3314,7 @@ int wolfSSH_CTX_SetWindowPacketSize(WOLFSSH_CTX* ctx, return WS_BAD_ARGUMENT; if (windowSz == 0) windowSz = DEFAULT_WINDOW_SZ; - if (maxPacketSz != 0 && maxPacketSz > MAX_PACKET_SZ) + if (maxPacketSz != 0 && maxPacketSz > MAX_CHANNEL_PACKET_SZ) return WS_BAD_ARGUMENT; if (maxPacketSz == 0) maxPacketSz = DEFAULT_MAX_PACKET_SZ; diff --git a/tests/api.c b/tests/api.c index e432fdc2a..2abb0a926 100644 --- a/tests/api.c +++ b/tests/api.c @@ -1308,14 +1308,18 @@ static void test_wolfSSH_CTX_SetWindowPacketSize(void) wolfSSH_CTX_SetWindowPacketSize(ctx, WINDOW_SZ_UPPER_BOUND + 1, 0)); - /* maxPacketSz exactly at transport limit: must succeed and be stored. */ + /* maxPacketSz exactly at the channel limit: must succeed and be stored. */ AssertIntEQ(WS_SUCCESS, - wolfSSH_CTX_SetWindowPacketSize(ctx, 0, MAX_PACKET_SZ)); - AssertIntEQ(MAX_PACKET_SZ, (int)ctx->maxPacketSz); + wolfSSH_CTX_SetWindowPacketSize(ctx, 0, MAX_CHANNEL_PACKET_SZ)); + AssertIntEQ(MAX_CHANNEL_PACKET_SZ, (int)ctx->maxPacketSz); + + /* maxPacketSz one above the channel limit: must fail. */ + AssertIntEQ(WS_BAD_ARGUMENT, + wolfSSH_CTX_SetWindowPacketSize(ctx, 0, MAX_CHANNEL_PACKET_SZ + 1)); - /* maxPacketSz one above transport limit: must fail. */ + /* The transport limit itself does not fit once framing is added. */ AssertIntEQ(WS_BAD_ARGUMENT, - wolfSSH_CTX_SetWindowPacketSize(ctx, 0, MAX_PACKET_SZ + 1)); + wolfSSH_CTX_SetWindowPacketSize(ctx, 0, MAX_PACKET_SZ)); /* Both valid non-zero values: must succeed and be stored. */ AssertIntEQ(WS_SUCCESS, diff --git a/wolfssh/internal.h b/wolfssh/internal.h index c40d78edc..1d5545a65 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -735,6 +735,35 @@ WOLFSSH_LOCAL int CheckAlgoList(const char* list, byte type); * block size first to decrypt to find the size of * the rest of the data. */ +/* 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. */ +#define MAX_CHANNEL_PACKET_SZ (MAX_PACKET_SZ - CHANNEL_PACKET_OVERHEAD_SZ) + +/* wolfSSH_CTX_SetWindowPacketSize() bounds an explicit size by + * MAX_CHANNEL_PACKET_SZ, but a zero there and CtxInit() both take + * DEFAULT_MAX_PACKET_SZ unchecked, so assert the default holds too. Both + * MAX_PACKET_SZ and DEFAULT_MAX_PACKET_SZ are overridable and the default + * path is the common one. */ +#if DEFAULT_MAX_PACKET_SZ > (MAX_PACKET_SZ - CHANNEL_PACKET_OVERHEAD_MAX) + #error "DEFAULT_MAX_PACKET_SZ too large to frame inside MAX_PACKET_SZ" +#endif + typedef struct WOLFSSH_BUFFER { void* heap; /* Heap for allocations */ From 47738c8b20f223b8afd75badb9efc790142a8946 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Mon, 17 Aug 2026 13:37:12 -0700 Subject: [PATCH 2/7] Report a channel name the peer cannot use 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. --- apps/wolfssh/wolfssh.c | 4 ++-- src/ssh.c | 26 +++++++++++++++++++++++--- tests/api.c | 28 +++++++++++++++++++++++++--- 3 files changed, 50 insertions(+), 8 deletions(-) diff --git a/apps/wolfssh/wolfssh.c b/apps/wolfssh/wolfssh.c index 9702122e5..14a63ffe6 100644 --- a/apps/wolfssh/wolfssh.c +++ b/apps/wolfssh/wolfssh.c @@ -728,8 +728,8 @@ static int wolfSSH_AGENT_IO_Cb(WS_AgentIoCbAction action, /* Mirrors the library's channel name limit. wolfSSH_SetChannelType() - * discards a longer command and still returns WS_SUCCESS, which would - * send an exec request with no command string. */ + * rejects a longer command with WS_BAD_ARGUMENT; checking it here reports + * it before a connection is attempted. */ #ifndef WOLFSSH_MAX_CHN_NAMESZ #define WOLFSSH_MAX_CHN_NAMESZ 4096 #endif diff --git a/src/ssh.c b/src/ssh.c index ed78d9939..675f7c86d 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -1674,7 +1674,18 @@ int wolfSSH_SetChannelType(WOLFSSH* ssh, byte type, byte* name, word32 nameSz) case WOLFSSH_SESSION_SUBSYSTEM: { byte* newName; - if (name != NULL && nameSz > 0 && nameSz < WOLFSSH_MAX_CHN_NAMESZ) { + if (name == NULL && nameSz > 0) { + WLOG(WS_LOG_DEBUG, "Channel name size without a name"); + return WS_BAD_ARGUMENT; + } + if (name != NULL && nameSz >= WOLFSSH_MAX_CHN_NAMESZ) { + /* Report it. Dropping the name sends a request with no name + * string, which the peer rejects as malformed. */ + WLOG(WS_LOG_DEBUG, "Channel name too large"); + return WS_BAD_ARGUMENT; + } + + if (name != NULL && nameSz > 0) { /* only (re)allocate when the name changed; SFTP/SCP retry * loops re-set the same name on every poll */ if (ssh->channelName == NULL || ssh->channelNameSz != nameSz || @@ -1693,9 +1704,18 @@ int wolfSSH_SetChannelType(WOLFSSH* ssh, byte type, byte* name, word32 nameSz) ssh->channelNameSz = nameSz; } } + else if (ssh->channelName == NULL) { + /* No name now and none from an earlier call. Same reason as + * the oversize case: exec and subsystem both carry a + * required name string, and SendChannelRequest() leaves the + * field out entirely when it has nothing to put there. */ + WLOG(WS_LOG_DEBUG, "No channel name to send"); + return WS_BAD_ARGUMENT; + } else { - /* invalid name ignored; type set but WS_SUCCESS returned */ - WLOG(WS_LOG_DEBUG, "No subsystem name or name was too large"); + /* keep the name an earlier call stored; SFTP/SCP retry + * loops re-enter with nothing to say */ + WLOG(WS_LOG_DEBUG, "Keeping the stored channel name"); } ssh->connectChannelId = type; break; diff --git a/tests/api.c b/tests/api.c index 2abb0a926..cd95d6799 100644 --- a/tests/api.c +++ b/tests/api.c @@ -299,6 +299,7 @@ static void test_wolfSSH_SetChannelType(void) const byte sub1[] = "sftp"; const byte sub2[] = "a-longer-subsystem-name"; byte* prevName; + byte* maxName; AssertIntNE(WS_SUCCESS, wolfSSH_SetChannelType(NULL, WOLFSSH_SESSION_SHELL, NULL, 0)); @@ -311,11 +312,19 @@ static void test_wolfSSH_SetChannelType(void) AssertNull(ssh->channelName); AssertIntEQ(0, ssh->channelNameSz); - AssertIntEQ(WS_SUCCESS, wolfSSH_SetChannelType(ssh, + /* subsystem carries a required name string, so with none stored and + * none given the request would go out without one */ + AssertIntEQ(WS_BAD_ARGUMENT, wolfSSH_SetChannelType(ssh, WOLFSSH_SESSION_SUBSYSTEM, NULL, 0)); AssertNull(ssh->channelName); - AssertIntEQ(WS_SUCCESS, wolfSSH_SetChannelType(ssh, + /* likewise for a size with no name behind it */ + AssertIntEQ(WS_BAD_ARGUMENT, wolfSSH_SetChannelType(ssh, + WOLFSSH_SESSION_SUBSYSTEM, NULL, 4)); + AssertNull(ssh->channelName); + + /* an oversized name is reported, not silently dropped */ + AssertIntEQ(WS_BAD_ARGUMENT, wolfSSH_SetChannelType(ssh, WOLFSSH_SESSION_SUBSYSTEM, (byte*)sub1, WOLFSSH_MAX_CHN_NAMESZ)); AssertNull(ssh->channelName); @@ -335,7 +344,7 @@ static void test_wolfSSH_SetChannelType(void) AssertIntEQ(1, ssh->channelName == prevName); /* a rejected (oversize) name must leave the previous buffer intact */ - AssertIntEQ(WS_SUCCESS, wolfSSH_SetChannelType(ssh, + AssertIntEQ(WS_BAD_ARGUMENT, wolfSSH_SetChannelType(ssh, WOLFSSH_SESSION_SUBSYSTEM, (byte*)sub1, WOLFSSH_MAX_CHN_NAMESZ)); AssertIntEQ(1, ssh->channelName == prevName); AssertIntEQ((int)(sizeof(sub1) - 1), (int)ssh->channelNameSz); @@ -347,6 +356,19 @@ static void test_wolfSSH_SetChannelType(void) AssertIntEQ(1, ssh->channelName == prevName); AssertIntEQ((int)(sizeof(sub1) - 1), (int)ssh->channelNameSz); + /* the largest name the limit still admits is stored, pinning the + * other side of the boundary the oversize checks above cover */ + AssertNotNull(maxName = (byte*)malloc(WOLFSSH_MAX_CHN_NAMESZ - 1)); + memset(maxName, 'a', WOLFSSH_MAX_CHN_NAMESZ - 1); + AssertIntEQ(WS_SUCCESS, wolfSSH_SetChannelType(ssh, + WOLFSSH_SESSION_SUBSYSTEM, maxName, + WOLFSSH_MAX_CHN_NAMESZ - 1)); + AssertIntEQ(WOLFSSH_MAX_CHN_NAMESZ - 1, (int)ssh->channelNameSz); + AssertIntEQ(0, memcmp(ssh->channelName, maxName, + WOLFSSH_MAX_CHN_NAMESZ - 1)); + AssertIntEQ(0, ssh->channelName[ssh->channelNameSz]); /* NUL terminated */ + free(maxName); + /* repeated set frees the previous buffer before replacing it */ AssertIntEQ(WS_SUCCESS, wolfSSH_SetChannelType(ssh, WOLFSSH_SESSION_SUBSYSTEM, (byte*)sub2, From 2131a5e90db6057b30a4cd73bd124429dd686aea Mon Sep 17 00:00:00 2001 From: John Safranek Date: Mon, 17 Aug 2026 13:43:12 -0700 Subject: [PATCH 3/7] Frame DoPacket from the validated packet length 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 cursor across an unimplemented message, driving DoPacket through a new wolfSSH_TestDoPacket() hook. The ShrinkBuffer() noted below zeroes the cursor, so DoReceive() cannot be in the path. 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 --- src/internal.c | 31 ++++++---- tests/unit.c | 141 +++++++++++++++++++++++++++++++++++++++++++++ wolfssh/internal.h | 2 + 3 files changed, 163 insertions(+), 11 deletions(-) diff --git a/src/internal.c b/src/internal.c index 316397e69..07e85bf61 100644 --- a/src/internal.c +++ b/src/internal.c @@ -12109,7 +12109,9 @@ static int DoChannelExtendedData(WOLFSSH* ssh, static int DoPacket(WOLFSSH* ssh, byte* bufferConsumed) { byte* buf = (byte*)ssh->inputBuffer.buffer; - word32 idx = ssh->inputBuffer.idx; + word32 pktStart = ssh->inputBuffer.idx; + word32 pktSz = ssh->curSz; + word32 idx = pktStart; word32 len = ssh->inputBuffer.length; word32 payloadSz; byte padSz; @@ -12132,11 +12134,11 @@ static int DoPacket(WOLFSSH* ssh, byte* bufferConsumed) } /* check for underflow */ - if ((word32)(PAD_LENGTH_SZ + padSz + MSG_ID_SZ) > ssh->curSz) { + if ((word32)(PAD_LENGTH_SZ + padSz + MSG_ID_SZ) > pktSz) { return WS_OVERFLOW_E; } - payloadSz = ssh->curSz - PAD_LENGTH_SZ - padSz - MSG_ID_SZ; + payloadSz = pktSz - PAD_LENGTH_SZ - padSz - MSG_ID_SZ; msg = buf[idx++]; /* At this point, payload starts at "buf + idx". */ @@ -12365,15 +12367,17 @@ static int DoPacket(WOLFSSH* ssh, byte* bufferConsumed) /* if the auth is still pending, don't discard the packet data */ if (ret != WS_AUTH_PENDING) { - if (payloadSz > 0) { - idx += payloadIdx; - if (idx + padSz > len) { - WLOG(WS_LOG_DEBUG, "Not enough data in buffer for pad."); - ret = WS_BUFFER_E; - } + /* Step over the packet using the length DoReceive already validated, + * not payloadIdx. A handler is free to read less than the payload it + * was handed -- the default case above reads none of it -- and that + * must not decide where the next packet begins. pktStart and pktSz + * are both from entry, so a handler cannot move the frame either. */ + idx = pktStart + UINT32_SZ + pktSz; + if (idx > len) { + WLOG(WS_LOG_DEBUG, "Not enough data in buffer for packet."); + ret = WS_BUFFER_E; + idx = len; } - - idx += padSz; ssh->inputBuffer.idx = idx; ssh->peerSeq++; ssh->rxMsgCount++; @@ -22891,6 +22895,11 @@ int wolfSSH_TestDoReceive(WOLFSSH* ssh) return DoReceive(ssh); } +int wolfSSH_TestDoPacket(WOLFSSH* ssh, byte* bufferConsumed) +{ + return DoPacket(ssh, bufferConsumed); +} + int wolfSSH_TestDoUserAuthBanner(WOLFSSH* ssh, byte* buf, word32 len, word32* idx) { diff --git a/tests/unit.c b/tests/unit.c index e1eff261c..7e90a4cad 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -1267,6 +1267,140 @@ static word32 BuildMacTestPacketPrefix(byte msgId, byte* out, word32 outSz) #endif +/* The test below drives a server-side session. With NO_WOLFSSH_SERVER the + * message filter has no server branch, so every message on such a session is + * refused and the test cannot run. */ +#if defined(WOLFSSH_TEST_INTERNAL) && !defined(NO_WOLFSSH_SERVER) + +/* Swallow the SSH_MSG_UNIMPLEMENTED that DoPacket sends back, so its send + * does not decide the result of the test. */ +static int SinkIoSendUnimplemented(WOLFSSH* ssh, void* buf, word32 sz, + void* ctx) +{ + (void)ssh; (void)buf; (void)ctx; + return (int)sz; +} + +/* Two back-to-back packets in one buffer. DoPacket() is driven directly: + * DoReceive() force-frees the buffer after every packet, zeroing the cursor + * under test. */ +static const byte s_unimplStream[] = { + /* unimplemented message, 8 bytes of payload that no handler reads */ + 0x00, 0x00, 0x00, 0x10, /* packetSz = 16 */ + 0x06, /* padSz = 6 */ + 0x0A, /* msgId = 10 */ + 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, 0xAA, /* payload */ + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, /* padding */ + /* MSGID_IGNORE carrying a zero-length string */ + 0x00, 0x00, 0x00, 0x0C, /* packetSz = 12 */ + 0x06, /* padSz = 6 */ + 0x02, /* msgId = IGNORE */ + 0x00, 0x00, 0x00, 0x00, /* string, len 0 */ + 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 /* padding */ +}; + +/* Message ID 10 is unassigned and always allowed, so it reaches DoPacket's + * default case, which reads none of its 8 payload bytes. DoPacket must step + * over them itself. Checked by the cursor it leaves, and by the length peeked + * there being the next packet's. Framing from payloadIdx lands 8 short. */ +static int test_DoPacket_UnimplementedConsumesPayload(void) +{ + WOLFSSH_CTX* ctx = NULL; + WOLFSSH* ssh = NULL; + int result = 0; + int ret; + int i; + /* packetSz of each packet in s_unimplStream, in order */ + static const word32 pktSizes[] = { 16, 12 }; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + if (ctx == NULL) + return -220; + wolfSSH_SetIOSend(ctx, SinkIoSendUnimplemented); + + ssh = wolfSSH_new(ctx); + if (ssh == NULL) { result = -221; goto done; } + + /* Past user auth, so the connection-layer message IDs are allowed. */ + ssh->acceptState = ACCEPT_SERVER_USERAUTH_SENT; + ssh->peerEncryptId = ID_NONE; + ssh->peerMacId = ID_NONE; + ssh->peerAeadMode = 0; + ssh->peerBlockSz = MIN_BLOCK_SZ; + ssh->peerMacSz = 0; + ssh->peerSeq = 0; + ssh->error = 0; + + /* Both packets in the buffer at once. */ + ShrinkBuffer(&ssh->inputBuffer, 1); + if (GrowBuffer(&ssh->inputBuffer, (word32)sizeof(s_unimplStream)) + != WS_SUCCESS) { + result = -222; + goto done; + } + WMEMCPY(ssh->inputBuffer.buffer, s_unimplStream, sizeof(s_unimplStream)); + ssh->inputBuffer.length = (word32)sizeof(s_unimplStream); + ssh->inputBuffer.idx = 0; + + for (i = 0; i < (int)(sizeof(pktSizes) / sizeof(pktSizes[0])); i++) { + word32 pktStart = ssh->inputBuffer.idx; + const byte* lenField = ssh->inputBuffer.buffer + pktStart; + word32 curSz; + byte bufferConsumed = 0; + + /* The peek DoReceive does; a short cursor reads leftover payload. */ + curSz = ((word32)lenField[0] << 24) | ((word32)lenField[1] << 16) + | ((word32)lenField[2] << 8) | (word32)lenField[3]; + if (curSz != pktSizes[i]) { + printf("DoPacket[%d]: packetSz=%u at cursor %u, expected %u\n", + i, curSz, pktStart, pktSizes[i]); + result = -223; + goto done; + } + ssh->curSz = curSz; + + ret = wolfSSH_TestDoPacket(ssh, &bufferConsumed); + if (ret != WS_SUCCESS) { + printf("DoPacket[%d]: ret=%d, error=%d\n", i, ret, ssh->error); + result = -224; + goto done; + } + if (!bufferConsumed) { + printf("DoPacket[%d]: packet not consumed\n", i); + result = -225; + goto done; + } + /* The whole packet, no more and no less. */ + if (ssh->inputBuffer.idx != pktStart + UINT32_SZ + curSz) { + printf("DoPacket[%d]: cursor at %u, expected %u\n", i, + ssh->inputBuffer.idx, pktStart + UINT32_SZ + curSz); + result = -226; + goto done; + } + if (ssh->peerSeq != (word32)(i + 1)) { + printf("DoPacket[%d]: peerSeq=%u, expected %d\n", i, + ssh->peerSeq, i + 1); + result = -227; + goto done; + } + } + + /* Buffer exactly spent. */ + if (ssh->inputBuffer.idx != (word32)sizeof(s_unimplStream)) { + printf("DoPacket: %u of %u bytes consumed\n", ssh->inputBuffer.idx, + (word32)sizeof(s_unimplStream)); + result = -228; + goto done; + } + +done: + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); + return result; +} +#endif /* WOLFSSH_TEST_INTERNAL && !NO_WOLFSSH_SERVER */ + + #if defined(WOLFSSH_TEST_INTERNAL) && \ (!defined(WOLFSSH_NO_HMAC_SHA1) || \ !defined(WOLFSSH_NO_HMAC_SHA1_96) || \ @@ -16127,6 +16261,13 @@ int wolfSSH_UnitTest(int argc, char** argv) testResult = testResult || unitResult; #endif +#if defined(WOLFSSH_TEST_INTERNAL) && !defined(NO_WOLFSSH_SERVER) + unitResult = test_DoPacket_UnimplementedConsumesPayload(); + printf("DoPacketUnimplemented: %s\n", + (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; +#endif + #if defined(WOLFSSH_TEST_INTERNAL) && \ (!defined(WOLFSSH_NO_HMAC_SHA1) || \ !defined(WOLFSSH_NO_HMAC_SHA1_96) || \ diff --git a/wolfssh/internal.h b/wolfssh/internal.h index 1d5545a65..7545c582b 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1805,6 +1805,8 @@ enum WS_MessageIdLimits { WOLFSSH_API int wolfSSH_TestIsMessageAllowed(WOLFSSH* ssh, byte msg, byte state); WOLFSSH_API int wolfSSH_TestDoReceive(WOLFSSH* ssh); + WOLFSSH_API int wolfSSH_TestDoPacket(WOLFSSH* ssh, + byte* bufferConsumed); WOLFSSH_API int wolfSSH_TestDoUserAuthBanner(WOLFSSH* ssh, byte* buf, word32 len, word32* idx); WOLFSSH_API int wolfSSH_TestPrepareUserAuthRequestPassword(WOLFSSH* ssh, From bd3739e69fecb75072076eac99f3b5a1ec047d2a Mon Sep 17 00:00:00 2001 From: John Safranek Date: Mon, 17 Aug 2026 13:45:34 -0700 Subject: [PATCH 4/7] Clamp terminal dimensions to what a winsize can hold 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 --- src/internal.c | 47 +++++++++++++++++++--------- tests/unit.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++ wolfssh/internal.h | 1 + 3 files changed, 110 insertions(+), 14 deletions(-) diff --git a/src/internal.c b/src/internal.c index 07e85bf61..5920e7b7c 100644 --- a/src/internal.c +++ b/src/internal.c @@ -11611,6 +11611,23 @@ static int ChannelRequestIs(const char* type, word32 typeSz, const char* name) } +#ifdef WOLFSSH_TERM +/* Store the terminal size a pty-req or window-change carried. All four + * are taken as sent, zero included, as others do; a zero is how a peer + * reports a dimension it has no value for. The clamp is for the + * consumers, which copy these into the unsigned short fields of a + * struct winsize, and for termResizeCb, which sees them first. */ +static void SetTerminalSize(WOLFSSH* ssh, word32 widthChar, word32 heightRows, + word32 widthPixels, word32 heightPixels) +{ + ssh->widthChar = min(widthChar, TERMINAL_DIMENSION_MAX); + ssh->heightRows = min(heightRows, TERMINAL_DIMENSION_MAX); + ssh->widthPixels = min(widthPixels, TERMINAL_DIMENSION_MAX); + ssh->heightPixels = min(heightPixels, TERMINAL_DIMENSION_MAX); +} +#endif /* WOLFSSH_TERM */ + + static int DoChannelRequest(WOLFSSH* ssh, byte* buf, word32 len, word32* idx) { @@ -11698,23 +11715,26 @@ static int DoChannelRequest(WOLFSSH* ssh, else if (ChannelRequestIs(type, typeSz, "pty-req")) { char term[32]; word32 termSz; + word32 widthChar, heightRows, widthPixels, heightPixels; 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); if (ret == WS_SUCCESS) - ret = GetUint32(&ssh->heightRows, buf, len, &begin); + ret = GetUint32(&heightRows, buf, len, &begin); if (ret == WS_SUCCESS) - ret = GetUint32(&ssh->widthPixels, buf, len, &begin); + ret = GetUint32(&widthPixels, buf, len, &begin); if (ret == WS_SUCCESS) - ret = GetUint32(&ssh->heightPixels, buf, len, &begin); + ret = GetUint32(&heightPixels, buf, len, &begin); if (ret == WS_SUCCESS) ret = GetStringAlloc(ssh->ctx->heap, (char**)&ssh->modes, &ssh->modesSz, buf, len, &begin); if (ret == WS_SUCCESS) { + SetTerminalSize(ssh, widthChar, heightRows, + widthPixels, heightPixels); WLOG(WS_LOG_DEBUG, " term = %s", term); WLOG(WS_LOG_DEBUG, " widthChar = %u", ssh->widthChar); WLOG(WS_LOG_DEBUG, " heightRows = %u", ssh->heightRows); @@ -11746,17 +11766,16 @@ static int DoChannelRequest(WOLFSSH* ssh, ret = GetUint32(&heightPixels, buf, len, &begin); if (ret == WS_SUCCESS) { - WLOG(WS_LOG_DEBUG, " widthChar = %u", widthChar); - WLOG(WS_LOG_DEBUG, " heightRows = %u", heightRows); - WLOG(WS_LOG_DEBUG, " widthPixels = %u", widthPixels); - WLOG(WS_LOG_DEBUG, " heightPixels = %u", heightPixels); - ssh->widthChar = widthChar; - ssh->heightRows = heightRows; - ssh->widthPixels = widthPixels; - ssh->heightPixels = heightPixels; + SetTerminalSize(ssh, widthChar, heightRows, + widthPixels, heightPixels); + WLOG(WS_LOG_DEBUG, " widthChar = %u", ssh->widthChar); + WLOG(WS_LOG_DEBUG, " heightRows = %u", ssh->heightRows); + WLOG(WS_LOG_DEBUG, " widthPixels = %u", ssh->widthPixels); + WLOG(WS_LOG_DEBUG, " heightPixels = %u", ssh->heightPixels); if (ssh->termResizeCb) { - if (ssh->termResizeCb(ssh, widthChar, heightRows, - widthPixels, heightPixels, + if (ssh->termResizeCb(ssh, + ssh->widthChar, ssh->heightRows, + ssh->widthPixels, ssh->heightPixels, ssh->termCtx) != WS_SUCCESS) { ret = WS_FATAL_ERROR; } diff --git a/tests/unit.c b/tests/unit.c index 7e90a4cad..74ada6bf8 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -6765,6 +6765,82 @@ static int test_DoChannelRequest(void) goto done; } } + + /* Dimensions are stored as sent, zero included, as others do. + * Oversized ones are clamped to what a struct winsize holds. */ + { + static byte payWindowChange[] = { + 0x00,0x00,0x00,0x00, /* channelId = 0 */ + 0x00,0x00,0x00,0x0D, /* typeSz = 13 */ + 0x77,0x69,0x6E,0x64,0x6F,0x77,0x2D, /* "window-" */ + 0x63,0x68,0x61,0x6E,0x67,0x65, /* "change" */ + 0x00, /* wantReply = 0 */ + 0x00,0x00,0x00,0x00, /* widthChar */ + 0x00,0x00,0x00,0x00, /* heightRows */ + 0x00,0x00,0x00,0x00, /* widthPixels */ + 0x00,0x00,0x00,0x00 /* heightPixels */ + }; + /* offsets of the four dimensions within the payload above */ + const word32 wcOff = 22, hrOff = 26, wpOff = 30, hpOff = 34; + word32 idx2; + int ret2; + int d; + struct { + const char* label; + word32 widthChar; + word32 heightRows; + word32 widthPixels; + word32 heightPixels; + word32 expectWidth; + word32 expectRows; + word32 expectPixWidth; + word32 expectPixHeight; + int errBase; + } dimCases[] = { + /* establish a known size first */ + { "baseline", 120, 40, 960, 640, 120, 40, 960, 640, -1600 }, + /* zeros are stored as sent, not merged with the previous size */ + { "zeroes", 0, 0, 0, 0, 0, 0, 0, 0, -1602 }, + /* including a zero in one dimension only */ + { "zeroWidth", 0, 50, 0, 640, 0, 50, 0, 640, -1604 }, + /* out of range is clamped, not wrapped to zero */ + { "wrapping", 0x10000, 0x10000, 0x10000, 0x10000, + 65535, 65535, 65535, 65535, -1606 }, + { "huge", 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, + 65535, 65535, 65535, 65535, -1608 } + }; + + for (d = 0; d < (int)(sizeof(dimCases) / sizeof(dimCases[0])); d++) { + PutU32BE(payWindowChange + wcOff, dimCases[d].widthChar); + PutU32BE(payWindowChange + hrOff, dimCases[d].heightRows); + PutU32BE(payWindowChange + wpOff, dimCases[d].widthPixels); + PutU32BE(payWindowChange + hpOff, dimCases[d].heightPixels); + + idx2 = 0; + ret2 = wolfSSH_TestDoChannelRequest(ssh, payWindowChange, + (word32)sizeof(payWindowChange), &idx2); + if (ret2 != WS_SUCCESS) { + printf("DoChannelRequest[%s]: ret=%d, expected=%d\n", + dimCases[d].label, ret2, WS_SUCCESS); + result = dimCases[d].errBase; + goto done; + } + if (ssh->widthChar != dimCases[d].expectWidth || + ssh->heightRows != dimCases[d].expectRows || + ssh->widthPixels != dimCases[d].expectPixWidth || + ssh->heightPixels != dimCases[d].expectPixHeight) { + printf("DoChannelRequest[%s]: got %ux%u %ux%u, " + "expected %ux%u %ux%u\n", dimCases[d].label, + ssh->widthChar, ssh->heightRows, + ssh->widthPixels, ssh->heightPixels, + dimCases[d].expectWidth, dimCases[d].expectRows, + dimCases[d].expectPixWidth, + dimCases[d].expectPixHeight); + result = dimCases[d].errBase - 1; + goto done; + } + } + } #endif /* WOLFSSH_SHELL && WOLFSSH_TERM */ done: diff --git a/wolfssh/internal.h b/wolfssh/internal.h index 7545c582b..387d04d2f 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -567,6 +567,7 @@ enum NameIdType { #define TERMINAL_MODES_MAX_SZ 4096 #define TERMINAL_WIDTH_DEFAULT 80 /* used when there is no terminal */ #define TERMINAL_HEIGHT_DEFAULT 24 +#define TERMINAL_DIMENSION_MAX 65535 /* what struct winsize can hold */ #define AEAD_IMP_IV_SZ 4 #define AEAD_EXP_IV_SZ 8 #define AEAD_NONCE_SZ (AEAD_IMP_IV_SZ+AEAD_EXP_IV_SZ) From ea73cad7b46fe4909f1c9081f9e5d85815ae9498 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Mon, 17 Aug 2026 18:38:32 -0700 Subject: [PATCH 5/7] Reject a window-change on a channel with no pty 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. --- src/internal.c | 10 +++++-- tests/unit.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 79 insertions(+), 3 deletions(-) diff --git a/src/internal.c b/src/internal.c index 5920e7b7c..e66bc3407 100644 --- a/src/internal.c +++ b/src/internal.c @@ -11717,7 +11717,6 @@ static int DoChannelRequest(WOLFSSH* ssh, word32 termSz; word32 widthChar, heightRows, widthPixels, heightPixels; - channel->ptyReq = 1; /* received a pty request */ termSz = (word32)sizeof(term); ret = GetString(term, &termSz, buf, len, &begin); if (ret == WS_SUCCESS) @@ -11733,6 +11732,7 @@ static int DoChannelRequest(WOLFSSH* ssh, (char**)&ssh->modes, &ssh->modesSz, buf, len, &begin); if (ret == WS_SUCCESS) { + channel->ptyReq = 1; /* only on a fully parsed request */ SetTerminalSize(ssh, widthChar, heightRows, widthPixels, heightPixels); WLOG(WS_LOG_DEBUG, " term = %s", term); @@ -11765,7 +11765,13 @@ static int DoChannelRequest(WOLFSSH* ssh, if (ret == WS_SUCCESS) ret = GetUint32(&heightPixels, buf, len, &begin); - if (ret == WS_SUCCESS) { + if (ret == WS_SUCCESS && !channel->ptyReq) { + /* Nothing to resize without a pty on this channel. Dropbear + * refuses the same request for the same reason. */ + WLOG(WS_LOG_DEBUG, " no pty on this channel, rejecting."); + rej = 1; + } + else if (ret == WS_SUCCESS) { SetTerminalSize(ssh, widthChar, heightRows, widthPixels, heightPixels); WLOG(WS_LOG_DEBUG, " widthChar = %u", ssh->widthChar); diff --git a/tests/unit.c b/tests/unit.c index 74ada6bf8..269a8d75f 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -6730,7 +6730,8 @@ static int test_DoChannelRequest(void) #endif /* WOLFSSH_TERM || WOLFSSH_SHELL */ /* RFC 4254 sec 6.7: window-change must not send a reply even if the - * wire wantReply byte is 1. */ + * wire wantReply byte is 1. No pty-req has been seen on this channel + * yet, so the request is also rejected and the size stays put. */ #if defined(WOLFSSH_SHELL) && defined(WOLFSSH_TERM) { static const byte payWindowChange[] = { @@ -6764,6 +6765,75 @@ static int test_DoChannelRequest(void) result = -451; goto done; } + if (ssh->widthChar != 0 || ssh->heightRows != 0) { + printf("DoChannelRequest[window-change]: applied %ux%u without " + "a pty\n", ssh->widthChar, ssh->heightRows); + result = -452; + goto done; + } + } + + /* A pty-req establishes the pty and the size, and runs the same + * clamping the window-change branch does. */ + { + static byte payPtyReq[] = { + 0x00,0x00,0x00,0x00, /* channelId = 0 */ + 0x00,0x00,0x00,0x07, /* typeSz = 7 */ + 0x70,0x74,0x79,0x2D,0x72,0x65,0x71, /* "pty-req" */ + 0x00, /* wantReply = 0 */ + 0x00,0x00,0x00,0x05, /* termSz = 5 */ + 0x76,0x74,0x31,0x30,0x30, /* "vt100" */ + 0x00,0x00,0x00,0x50, /* widthChar = 80 */ + 0x00,0x00,0x00,0x18, /* heightRows = 24 */ + 0x00,0x00,0x02,0x80, /* widthPixels = 640 */ + 0x00,0x00,0x01,0xE0, /* heightPixels = 480 */ + 0x00,0x00,0x00,0x01, /* modesSz = 1 */ + 0x00 /* TTY_OP_END */ + }; + /* offsets of the four dimensions within the payload above */ + const word32 pwcOff = 25, phrOff = 29, pwpOff = 33, phpOff = 37; + word32 idx2 = 0; + int ret2; + + ret2 = wolfSSH_TestDoChannelRequest(ssh, payPtyReq, + (word32)sizeof(payPtyReq), &idx2); + if (ret2 != WS_SUCCESS) { + printf("DoChannelRequest[pty-req]: ret=%d, expected=%d\n", + ret2, WS_SUCCESS); + result = -453; + goto done; + } + if (ssh->widthChar != 80 || ssh->heightRows != 24 || + ssh->widthPixels != 640 || ssh->heightPixels != 480) { + printf("DoChannelRequest[pty-req]: got %ux%u %ux%u, " + "expected 80x24 640x480\n", ssh->widthChar, + ssh->heightRows, ssh->widthPixels, ssh->heightPixels); + result = -454; + goto done; + } + + /* the clamp is on the pty-req path too, pixels included */ + PutU32BE(payPtyReq + pwcOff, 0x10000); + PutU32BE(payPtyReq + phrOff, 0x10000); + PutU32BE(payPtyReq + pwpOff, 0xFFFFFFFF); + PutU32BE(payPtyReq + phpOff, 0xFFFFFFFF); + idx2 = 0; + ret2 = wolfSSH_TestDoChannelRequest(ssh, payPtyReq, + (word32)sizeof(payPtyReq), &idx2); + if (ret2 != WS_SUCCESS) { + printf("DoChannelRequest[pty-req clamp]: ret=%d, expected=%d\n", + ret2, WS_SUCCESS); + result = -455; + goto done; + } + if (ssh->widthChar != 65535 || ssh->heightRows != 65535 || + ssh->widthPixels != 65535 || ssh->heightPixels != 65535) { + printf("DoChannelRequest[pty-req clamp]: got %ux%u %ux%u, " + "expected 65535 throughout\n", ssh->widthChar, + ssh->heightRows, ssh->widthPixels, ssh->heightPixels); + result = -456; + goto done; + } } /* Dimensions are stored as sent, zero included, as others do. From 14ef9c4292105c8af0b5846002dee76fa5fa1b16 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Tue, 18 Aug 2026 08:58:52 -0700 Subject: [PATCH 6/7] Document the wolfSSH_SetChannelType contract 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. --- src/ssh.c | 11 +++++++++++ tests/api.c | 5 +++++ 2 files changed, 16 insertions(+) diff --git a/src/ssh.c b/src/ssh.c index 675f7c86d..1ad18acca 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -1645,7 +1645,18 @@ int wolfSSH_SetExitStatus(WOLFSSH* ssh, word32 exitStatus) * name name or command in the case of subsystem and exec channel types * nameSz size of name buffer * + * Exec and subsystem carry a name string the peer requires, so one must be + * available. Passing none keeps the name an earlier call stored; with + * nothing stored the call is refused rather than sending a request the peer + * reads as malformed. Shell and terminal take no name and drop any stored + * one. A refused call changes nothing, the selected type included. + * * returns WS_SUCCESS on success + * returns WS_BAD_ARGUMENT for a NULL ssh or an unknown type, for exec on + * the server side, for a name at or above WOLFSSH_MAX_CHN_NAMESZ, for a + * nameSz with no name behind it, and for exec or subsystem with no name + * given and none stored + * returns WS_MEMORY_E if the name cannot be allocated */ int wolfSSH_SetChannelType(WOLFSSH* ssh, byte type, byte* name, word32 nameSz) { diff --git a/tests/api.c b/tests/api.c index cd95d6799..2ac3fec21 100644 --- a/tests/api.c +++ b/tests/api.c @@ -317,16 +317,20 @@ static void test_wolfSSH_SetChannelType(void) AssertIntEQ(WS_BAD_ARGUMENT, wolfSSH_SetChannelType(ssh, WOLFSSH_SESSION_SUBSYSTEM, NULL, 0)); AssertNull(ssh->channelName); + /* a refused call leaves the selected type alone, not just the name */ + AssertIntEQ(WOLFSSH_SESSION_SHELL, ssh->connectChannelId); /* likewise for a size with no name behind it */ AssertIntEQ(WS_BAD_ARGUMENT, wolfSSH_SetChannelType(ssh, WOLFSSH_SESSION_SUBSYSTEM, NULL, 4)); AssertNull(ssh->channelName); + AssertIntEQ(WOLFSSH_SESSION_SHELL, ssh->connectChannelId); /* an oversized name is reported, not silently dropped */ AssertIntEQ(WS_BAD_ARGUMENT, wolfSSH_SetChannelType(ssh, WOLFSSH_SESSION_SUBSYSTEM, (byte*)sub1, WOLFSSH_MAX_CHN_NAMESZ)); AssertNull(ssh->channelName); + AssertIntEQ(WOLFSSH_SESSION_SHELL, ssh->connectChannelId); AssertIntEQ(WS_SUCCESS, wolfSSH_SetChannelType(ssh, WOLFSSH_SESSION_SUBSYSTEM, (byte*)sub1, @@ -347,6 +351,7 @@ static void test_wolfSSH_SetChannelType(void) AssertIntEQ(WS_BAD_ARGUMENT, wolfSSH_SetChannelType(ssh, WOLFSSH_SESSION_SUBSYSTEM, (byte*)sub1, WOLFSSH_MAX_CHN_NAMESZ)); AssertIntEQ(1, ssh->channelName == prevName); + AssertIntEQ(WOLFSSH_SESSION_SUBSYSTEM, ssh->connectChannelId); AssertIntEQ((int)(sizeof(sub1) - 1), (int)ssh->channelNameSz); AssertIntEQ(0, strcmp((const char*)ssh->channelName, (const char*)sub1)); From 1bc89512843faa427caeb176d044277d9add72e1 Mon Sep 17 00:00:00 2001 From: John Safranek Date: Tue, 18 Aug 2026 08:58:52 -0700 Subject: [PATCH 7/7] Check the channel packet overhead literal against its expression 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. --- src/internal.c | 8 ++++++++ wolfssh/internal.h | 24 +++++++++--------------- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/src/internal.c b/src/internal.c index e66bc3407..fa72a691f 100644 --- a/src/internal.c +++ b/src/internal.c @@ -98,6 +98,14 @@ #include + +/* The #error in internal.h can't compare the two: the expression's terms are + * enum constants that #if reads as zero. Here both are ordinary constant + * expressions, so a term added without bumping the literal fails the build. */ +typedef char wolfSSH_channel_overhead_check[ + (CHANNEL_PACKET_OVERHEAD_SZ <= CHANNEL_PACKET_OVERHEAD_MAX) ? 1 : -1]; + + /* Flags: HAVE_WC_ECC_SET_RNG diff --git a/wolfssh/internal.h b/wolfssh/internal.h index 387d04d2f..32714c14f 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -736,14 +736,11 @@ WOLFSSH_LOCAL int CheckAlgoList(const char* list, byte type); * block size first to decrypt to find the size of * the rest of the data. */ -/* 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. */ +/* Channel data packet overhead: transport framing, the larger channel data + * header (CHANNEL_EXTENDED_DATA), worst-case BundlePacket() padding, and the + * largest MAC. Spelled twice because AES_BLOCK_SIZE and MAX_HMAC_SZ are enum + * constants that #if reads as zero. The literal is a ceiling: 101 with a + * 64-byte MAC, less with a smaller digest. */ #define CHANNEL_PACKET_OVERHEAD_SZ \ (LENGTH_SZ + PAD_LENGTH_SZ \ + MSG_ID_SZ + (UINT32_SZ * 2) + LENGTH_SZ \ @@ -752,15 +749,12 @@ WOLFSSH_LOCAL int CheckAlgoList(const char* list, byte type); #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. */ + * bounds the whole binary packet. At most 35000 - 101 = 34899. Derived, not + * a tunable, so deliberately not overridable. */ #define MAX_CHANNEL_PACKET_SZ (MAX_PACKET_SZ - CHANNEL_PACKET_OVERHEAD_SZ) -/* wolfSSH_CTX_SetWindowPacketSize() bounds an explicit size by - * MAX_CHANNEL_PACKET_SZ, but a zero there and CtxInit() both take - * DEFAULT_MAX_PACKET_SZ unchecked, so assert the default holds too. Both - * MAX_PACKET_SZ and DEFAULT_MAX_PACKET_SZ are overridable and the default - * path is the common one. */ +/* Both sizes are overridable, and CtxInit() takes DEFAULT_MAX_PACKET_SZ + * unchecked, so assert the default frames too. */ #if DEFAULT_MAX_PACKET_SZ > (MAX_PACKET_SZ - CHANNEL_PACKET_OVERHEAD_MAX) #error "DEFAULT_MAX_PACKET_SZ too large to frame inside MAX_PACKET_SZ" #endif