Skip to content

fix: harden peer-supplied atoi parsing #1095

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
MarkAtwood wants to merge 1 commit into
base: master
Choose a base branch
from
Open

fix: harden peer-supplied atoi parsing #1095

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
48 changes: 42 additions & 6 deletions src/wolfscp.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1068,6 +1068,35 @@ static int FindSpaceInString(byte* buf, word32 bufSz, word32* inOutIdx)
return WS_SUCCESS;
}

/* Parse a base-10 unsigned integer from a newline- or NUL-terminated
* string (the SCP header field). Rejects empty input, any non-digit
* character, and values that overflow max. Returns WS_SUCCESS on
* success, WS_SCP_BAD_MSG_E on malformed/out-of-range input. */
Comment on lines +1071 to +1074
static int ScpParseUInt64(const char* str, word64 max, word64* out)

@aidangarske aidangarske Jul 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [Medium] SCP numeric parser lacks regression tests for malformed and overflow fields
💡 SUGGEST test

The PR adds ScpParseUInt64 and routes SCP file size, mtime, and atime through it, but there are no tests covering the new parser behavior. Existing SCP internal tests only expose GetScpFileMode and ExtractFileName, so the new rejection paths for empty fields, non-digits, and overflows are untested.

Recommendation: Add a WOLFSSH_TEST_INTERNAL wrapper for the changed SCP numeric parsing path or for ReceiveScpMessage, then cover valid max values, overflow by one, empty fields, signs, and embedded garbage for file size and timestamps.

{
word64 val = 0;
int digits = 0;

if (str == NULL || out == NULL)
return WS_BAD_ARGUMENT;

while (*str >= '0' && *str <= '9') {
word64 d = (word64)(*str - '0');
if (val > (max - d) / 10)
return WS_SCP_BAD_MSG_E;
Comment on lines +1085 to +1086
val = val * 10 + d;
digits++;
str++;
}

if (digits == 0 || (*str != '\n' && *str != '\0'))
return WS_SCP_BAD_MSG_E;

*out = val;
return WS_SUCCESS;
}


/* Reads file size from beginning of string, expects space to be after,
* places size in ssh->scpFileSz.
*
Expand All @@ -1093,9 +1122,14 @@ static int GetScpFileSize(WOLFSSH* ssh, byte* buf, word32 bufSz,
ret = WS_SCP_BAD_MSG_E;

if (ret == WS_SUCCESS) {
/* replace space with newline for atoi */
/* replace space with newline for parsing */
buf[spaceIdx] = '\n';
ssh->scpFileSz = atoi((char *)(buf + idx));
{
word64 scpFileSz64 = 0;
ret = ScpParseUInt64((char *)(buf + idx), 0xFFFFFFFFUL,
&scpFileSz64);
ssh->scpFileSz = (word32)scpFileSz64;
}
Comment on lines +1128 to +1132

/* restore space, increment idx to space */
buf[spaceIdx] = ' ';
Expand Down Expand Up @@ -1225,9 +1259,10 @@ static int GetScpTimestamp(WOLFSSH* ssh, byte* buf, word32 bufSz,

/* read modification time */
if (ret == WS_SUCCESS) {
/* replace space with newline for atoi */
/* replace space with newline for parsing */
buf[spaceIdx] = '\n';
ssh->scpMTime = atoi((char*)(buf + idx));
ret = ScpParseUInt64((char*)(buf + idx), 0xFFFFFFFFFFFFFFFFULL,
&ssh->scpMTime);

/* restore space, increment idx past it */
buf[spaceIdx] = ' ';
Expand Down Expand Up @@ -1264,9 +1299,10 @@ static int GetScpTimestamp(WOLFSSH* ssh, byte* buf, word32 bufSz,
}

if (ret == WS_SUCCESS) {
/* replace space with newline for atoi */
/* replace space with newline for parsing */
buf[spaceIdx] = '\n';
ssh->scpATime = atoi((char*)(buf + idx));
ret = ScpParseUInt64((char*)(buf + idx), 0xFFFFFFFFFFFFFFFFULL,
&ssh->scpATime);

/* restore space, increment idx past it */
buf[spaceIdx] = ' ';
Expand Down
18 changes: 16 additions & 2 deletions src/wolfterm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,20 @@ static int isCommand(byte c)
}


/* Bounded VT100 parameter parse. atoi() has UB on overflow (C99 7.20.1),
* cannot report errors, and lets peer-supplied "-1" wrap to 0xFFFFFFFF.
* Clamp invalid/out-of-range params to 0 (0..65535 covers VT100 params). */
static word32 parseArg(const byte* s)

@aidangarske aidangarske Jul 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [Medium] VT100 argument bounds behavior lacks targeted Windows tests
💡 SUGGEST test

The PR changes Windows VT100 argument parsing semantics for negative and out-of-range values, but the existing wolfSSH_ConvertConsole tests only cover basic/truncated escape handling. There is no targeted coverage proving that -1, 65536, overflow strings, empty args, and malformed params clamp to 0 while valid params still pass through.

Recommendation: Extend the USE_WINDOWS_API console tests with CSI sequences that exercise valid values, -1, 65536, very large decimal strings, empty args, and trailing-junk malformed args.

{
char* endp;
long v = strtol((const char*)s, &endp, 10);

@aidangarske aidangarske Jul 10, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 [Medium] VT100 parser still accepts trailing junk after digits
💡 SUGGEST bug

The PR introduces parseArg as the new bounded parser and the comment says invalid params are clamped to 0, but the implementation only checks whether strtol consumed at least one character. Inputs such as 12x, 12-3, or 1? parse as 12 or 1 instead of being treated as invalid. Since getArgs passes the whole VT100 parameter substring to parseArg, this leaves a class of malformed peer-controlled parameters accepted by the new validation boundary.

Suggestion:

Suggested change
long v = strtol((const char*)s, &endp, 10);
long v = strtol((const char*)s, &endp, 10);
if (endp == (const char*)s || *endp != '\0' || v < 0 || v > 65535L) {
v = 0;
}
return (word32)v;

if (endp == (const char*)s || v < 0 || v > 65535L) {
v = 0;
}
return (word32)v;
}


/* returns the number of args found */
static int getArgs(byte* buf, word32 bufSz, word32* idx, word32* out)
{
Expand All @@ -331,7 +345,7 @@ static int getArgs(byte* buf, word32 bufSz, word32* idx, word32* out)

if (buf[*idx + i] == ';') {
tmpBuf[tmpBufIdx] = '\0';
out[numArgs] = atoi(tmpBuf);
out[numArgs] = parseArg(tmpBuf);
numArgs++;
tmpBufIdx = 0;
}
Expand All @@ -343,7 +357,7 @@ static int getArgs(byte* buf, word32 bufSz, word32* idx, word32* out)

if (i > 0 && tmpBufIdx > 0) {
tmpBuf[tmpBufIdx] = '\0';
out[numArgs] = atoi(tmpBuf);
out[numArgs] = parseArg(tmpBuf);
numArgs++;
}

Expand Down
Loading