Skip to content

Commit 78810b4

Browse files
ejohnstownejohnstown
committed
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
1 parent 12a53f5 commit 78810b4

3 files changed

Lines changed: 110 additions & 14 deletions

File tree

‎src/internal.c‎

Lines changed: 33 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -11383,6 +11383,23 @@ static int ChannelRequestIs(const char* type, word32 typeSz, const char* name)
1138311383
}
1138411384

1138511385

11386+
#ifdef WOLFSSH_TERM
11387+
/* Store the terminal size a pty-req or window-change carried. All four
11388+
* are taken as sent, zero included, as others do; a zero is how a peer
11389+
* reports a dimension it has no value for. The clamp is for the
11390+
* consumers, which copy these into the unsigned short fields of a
11391+
* struct winsize, and for termResizeCb, which sees them first. */
11392+
static void SetTerminalSize(WOLFSSH* ssh, word32 widthChar, word32 heightRows,
11393+
word32 widthPixels, word32 heightPixels)
11394+
{
11395+
ssh->widthChar = min(widthChar, TERMINAL_DIMENSION_MAX);
11396+
ssh->heightRows = min(heightRows, TERMINAL_DIMENSION_MAX);
11397+
ssh->widthPixels = min(widthPixels, TERMINAL_DIMENSION_MAX);
11398+
ssh->heightPixels = min(heightPixels, TERMINAL_DIMENSION_MAX);
11399+
}
11400+
#endif /* WOLFSSH_TERM */
11401+
11402+
1138611403
static int DoChannelRequest(WOLFSSH* ssh,
1138711404
byte* buf, word32 len, word32* idx)
1138811405
{
@@ -11470,23 +11487,26 @@ static int DoChannelRequest(WOLFSSH* ssh,
1147011487
else if (ChannelRequestIs(type, typeSz, "pty-req")) {
1147111488
char term[32];
1147211489
word32 termSz;
11490+
word32 widthChar, heightRows, widthPixels, heightPixels;
1147311491

1147411492
channel->ptyReq = 1; /* received a pty request */
1147511493
termSz = (word32)sizeof(term);
1147611494
ret = GetString(term, &termSz, buf, len, &begin);
1147711495
if (ret == WS_SUCCESS)
11478-
ret = GetUint32(&ssh->widthChar, buf, len, &begin);
11496+
ret = GetUint32(&widthChar, buf, len, &begin);
1147911497
if (ret == WS_SUCCESS)
11480-
ret = GetUint32(&ssh->heightRows, buf, len, &begin);
11498+
ret = GetUint32(&heightRows, buf, len, &begin);
1148111499
if (ret == WS_SUCCESS)
11482-
ret = GetUint32(&ssh->widthPixels, buf, len, &begin);
11500+
ret = GetUint32(&widthPixels, buf, len, &begin);
1148311501
if (ret == WS_SUCCESS)
11484-
ret = GetUint32(&ssh->heightPixels, buf, len, &begin);
11502+
ret = GetUint32(&heightPixels, buf, len, &begin);
1148511503
if (ret == WS_SUCCESS)
1148611504
ret = GetStringAlloc(ssh->ctx->heap,
1148711505
(char**)&ssh->modes, &ssh->modesSz,
1148811506
buf, len, &begin);
1148911507
if (ret == WS_SUCCESS) {
11508+
SetTerminalSize(ssh, widthChar, heightRows,
11509+
widthPixels, heightPixels);
1149011510
WLOG(WS_LOG_DEBUG, " term = %s", term);
1149111511
WLOG(WS_LOG_DEBUG, " widthChar = %u", ssh->widthChar);
1149211512
WLOG(WS_LOG_DEBUG, " heightRows = %u", ssh->heightRows);
@@ -11518,17 +11538,16 @@ static int DoChannelRequest(WOLFSSH* ssh,
1151811538
ret = GetUint32(&heightPixels, buf, len, &begin);
1151911539

1152011540
if (ret == WS_SUCCESS) {
11521-
WLOG(WS_LOG_DEBUG, " widthChar = %u", widthChar);
11522-
WLOG(WS_LOG_DEBUG, " heightRows = %u", heightRows);
11523-
WLOG(WS_LOG_DEBUG, " widthPixels = %u", widthPixels);
11524-
WLOG(WS_LOG_DEBUG, " heightPixels = %u", heightPixels);
11525-
ssh->widthChar = widthChar;
11526-
ssh->heightRows = heightRows;
11527-
ssh->widthPixels = widthPixels;
11528-
ssh->heightPixels = heightPixels;
11541+
SetTerminalSize(ssh, widthChar, heightRows,
11542+
widthPixels, heightPixels);
11543+
WLOG(WS_LOG_DEBUG, " widthChar = %u", ssh->widthChar);
11544+
WLOG(WS_LOG_DEBUG, " heightRows = %u", ssh->heightRows);
11545+
WLOG(WS_LOG_DEBUG, " widthPixels = %u", ssh->widthPixels);
11546+
WLOG(WS_LOG_DEBUG, " heightPixels = %u", ssh->heightPixels);
1152911547
if (ssh->termResizeCb) {
11530-
if (ssh->termResizeCb(ssh, widthChar, heightRows,
11531-
widthPixels, heightPixels,
11548+
if (ssh->termResizeCb(ssh,
11549+
ssh->widthChar, ssh->heightRows,
11550+
ssh->widthPixels, ssh->heightPixels,
1153211551
ssh->termCtx) != WS_SUCCESS) {
1153311552
ret = WS_FATAL_ERROR;
1153411553
}

‎tests/unit.c‎

Lines changed: 76 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6740,6 +6740,82 @@ static int test_DoChannelRequest(void)
67406740
goto done;
67416741
}
67426742
}
6743+
6744+
/* Dimensions are stored as sent, zero included, as others do.
6745+
* Oversized ones are clamped to what a struct winsize holds. */
6746+
{
6747+
static byte payWindowChange[] = {
6748+
0x00,0x00,0x00,0x00, /* channelId = 0 */
6749+
0x00,0x00,0x00,0x0D, /* typeSz = 13 */
6750+
0x77,0x69,0x6E,0x64,0x6F,0x77,0x2D, /* "window-" */
6751+
0x63,0x68,0x61,0x6E,0x67,0x65, /* "change" */
6752+
0x00, /* wantReply = 0 */
6753+
0x00,0x00,0x00,0x00, /* widthChar */
6754+
0x00,0x00,0x00,0x00, /* heightRows */
6755+
0x00,0x00,0x00,0x00, /* widthPixels */
6756+
0x00,0x00,0x00,0x00 /* heightPixels */
6757+
};
6758+
/* offsets of the four dimensions within the payload above */
6759+
const word32 wcOff = 22, hrOff = 26, wpOff = 30, hpOff = 34;
6760+
word32 idx2;
6761+
int ret2;
6762+
int d;
6763+
struct {
6764+
const char* label;
6765+
word32 widthChar;
6766+
word32 heightRows;
6767+
word32 widthPixels;
6768+
word32 heightPixels;
6769+
word32 expectWidth;
6770+
word32 expectRows;
6771+
word32 expectPixWidth;
6772+
word32 expectPixHeight;
6773+
int errBase;
6774+
} dimCases[] = {
6775+
/* establish a known size first */
6776+
{ "baseline", 120, 40, 960, 640, 120, 40, 960, 640, -1600 },
6777+
/* zeros are stored as sent, not merged with the previous size */
6778+
{ "zeroes", 0, 0, 0, 0, 0, 0, 0, 0, -1602 },
6779+
/* including a zero in one dimension only */
6780+
{ "zeroWidth", 0, 50, 0, 640, 0, 50, 0, 640, -1604 },
6781+
/* out of range is clamped, not wrapped to zero */
6782+
{ "wrapping", 0x10000, 0x10000, 0x10000, 0x10000,
6783+
65535, 65535, 65535, 65535, -1606 },
6784+
{ "huge", 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF,
6785+
65535, 65535, 65535, 65535, -1608 }
6786+
};
6787+
6788+
for (d = 0; d < (int)(sizeof(dimCases) / sizeof(dimCases[0])); d++) {
6789+
PutU32BE(payWindowChange + wcOff, dimCases[d].widthChar);
6790+
PutU32BE(payWindowChange + hrOff, dimCases[d].heightRows);
6791+
PutU32BE(payWindowChange + wpOff, dimCases[d].widthPixels);
6792+
PutU32BE(payWindowChange + hpOff, dimCases[d].heightPixels);
6793+
6794+
idx2 = 0;
6795+
ret2 = wolfSSH_TestDoChannelRequest(ssh, payWindowChange,
6796+
(word32)sizeof(payWindowChange), &idx2);
6797+
if (ret2 != WS_SUCCESS) {
6798+
printf("DoChannelRequest[%s]: ret=%d, expected=%d\n",
6799+
dimCases[d].label, ret2, WS_SUCCESS);
6800+
result = dimCases[d].errBase;
6801+
goto done;
6802+
}
6803+
if (ssh->widthChar != dimCases[d].expectWidth ||
6804+
ssh->heightRows != dimCases[d].expectRows ||
6805+
ssh->widthPixels != dimCases[d].expectPixWidth ||
6806+
ssh->heightPixels != dimCases[d].expectPixHeight) {
6807+
printf("DoChannelRequest[%s]: got %ux%u %ux%u, "
6808+
"expected %ux%u %ux%u\n", dimCases[d].label,
6809+
ssh->widthChar, ssh->heightRows,
6810+
ssh->widthPixels, ssh->heightPixels,
6811+
dimCases[d].expectWidth, dimCases[d].expectRows,
6812+
dimCases[d].expectPixWidth,
6813+
dimCases[d].expectPixHeight);
6814+
result = dimCases[d].errBase - 1;
6815+
goto done;
6816+
}
6817+
}
6818+
}
67436819
#endif /* WOLFSSH_SHELL && WOLFSSH_TERM */
67446820

67456821
done:

‎wolfssh/internal.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -560,6 +560,7 @@ enum NameIdType {
560560
#define TERMINAL_MODES_MAX_SZ 4096
561561
#define TERMINAL_WIDTH_DEFAULT 80 /* used when there is no terminal */
562562
#define TERMINAL_HEIGHT_DEFAULT 24
563+
#define TERMINAL_DIMENSION_MAX 65535 /* what struct winsize can hold */
563564
#define AEAD_IMP_IV_SZ 4
564565
#define AEAD_EXP_IV_SZ 8
565566
#define AEAD_NONCE_SZ (AEAD_IMP_IV_SZ+AEAD_EXP_IV_SZ)

0 commit comments

Comments
 (0)