Skip to content

Channel and connection layer input-handling fixes #1177

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ejohnstown wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Channel and connection layer input-handling fixes #1177

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f2e95c8
Bound channel maxPacketSz below the wire limit
ejohnstown Aug 17, 2026
47738c8
Report a channel name the peer cannot use
ejohnstown Aug 17, 2026
2131a5e
Frame DoPacket from the validated packet length
ejohnstown Aug 17, 2026
bd3739e
Clamp terminal dimensions to what a winsize can hold
ejohnstown Aug 17, 2026
ea73cad
Reject a window-change on a channel with no pty
ejohnstown Aug 18, 2026
14ef9c4
Document the wolfSSH_SetChannelType contract
ejohnstown Aug 18, 2026
1bc8951
Check the channel packet overhead literal against its expression
ejohnstown Aug 18, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions apps/wolfssh/wolfssh.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
96 changes: 69 additions & 27 deletions src/internal.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,14 @@

#include <wolfssl/wolfcrypt/coding.h>


/* 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
Expand Down Expand Up @@ -11611,6 +11619,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)
{
Expand Down Expand Up @@ -11698,23 +11723,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) {
channel->ptyReq = 1; /* only on a fully parsed request */
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);
Expand Down Expand Up @@ -11745,18 +11773,23 @@ static int DoChannelRequest(WOLFSSH* ssh,
if (ret == WS_SUCCESS)
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;
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);
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;
}
Expand Down Expand Up @@ -12109,7 +12142,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;
Expand All @@ -12132,11 +12167,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". */
Expand Down Expand Up @@ -12365,15 +12400,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++;
Expand Down Expand Up @@ -22891,6 +22928,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)
{
Expand Down
39 changes: 35 additions & 4 deletions src/ssh.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -1674,7 +1685,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 ||
Expand All @@ -1693,9 +1715,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;
Expand Down Expand Up @@ -3314,7 +3345,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;
Expand Down
47 changes: 39 additions & 8 deletions tests/api.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -311,13 +312,25 @@ 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);
/* a refused call leaves the selected type alone, not just the name */
AssertIntEQ(WOLFSSH_SESSION_SHELL, ssh->connectChannelId);

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);
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,
Expand All @@ -335,9 +348,10 @@ 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(WOLFSSH_SESSION_SUBSYSTEM, ssh->connectChannelId);
AssertIntEQ((int)(sizeof(sub1) - 1), (int)ssh->channelNameSz);
AssertIntEQ(0, strcmp((const char*)ssh->channelName, (const char*)sub1));

Expand All @@ -347,6 +361,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,
Expand Down Expand Up @@ -1308,14 +1335,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 transport limit: must fail. */
/* maxPacketSz one above the channel limit: must fail. */
AssertIntEQ(WS_BAD_ARGUMENT,
wolfSSH_CTX_SetWindowPacketSize(ctx, 0, MAX_PACKET_SZ + 1));
wolfSSH_CTX_SetWindowPacketSize(ctx, 0, MAX_CHANNEL_PACKET_SZ + 1));

/* The transport limit itself does not fit once framing is added. */
AssertIntEQ(WS_BAD_ARGUMENT,
wolfSSH_CTX_SetWindowPacketSize(ctx, 0, MAX_PACKET_SZ));

/* Both valid non-zero values: must succeed and be stored. */
AssertIntEQ(WS_SUCCESS,
Expand Down
Loading
Loading