From 46a4988b575b4e688a3d8c8a1e2bf1dcdea974c0 Mon Sep 17 00:00:00 2001 From: Mark Atwood Date: Thu, 9 Jul 2026 17:27:55 -0700 Subject: [PATCH] fix: harden peer-supplied numeric field parsing Replace atoi() and strtoull() on peer-controlled numeric fields with bounded, error-reporting parsers. atoi() has undefined behavior on overflow and cannot signal malformed input. strtoull() is C99, so it is unavailable on some ports, and it accepts a leading sign and leading whitespace that the SCP header does not permit. - #2881 src/wolfscp.c: add ScpParseUInt64 helper; parse SCP header file size / mtime / atime with digit validation and overflow rejection against a caller-supplied max, replacing the 3 strtoull call sites from de23a694. Also rejects "+1", " 1" and "\t1" on all three fields, and "-1" on the unbounded scpMTime and scpATime, which had taken the wrapped 0xFFFFFFFFFFFFFFFF. The helper takes the field length, dropping the temporary '\n' written over the separating space at each call site, and with it the include de23a694 had added for UINT32_MAX. - #2882 src/wolfterm.c: add parseArg helper using strtol; clamp invalid/out-of-range VT100 params to 0, replacing 2 atoi call sites (blocks peer "-1" wrapping to 0xFFFFFFFF). A param with bytes left over after its digits, such as "12x", clamps to 0 rather than parsing as a short value. strtol is C89 and needs no replacement. Inside USE_WINDOWS_API, so not built on Linux or macOS. Also drop the unused maxIdx from getArgs(). Extend test_ScpGetFileSize and test_ScpGetTimestamp in tests/unit.c for the fields the new parser rejects but strtoull() accepted: a signed or whitespace-padded field, an empty field, a field holding a NUL, and the 64-bit maximum and one past it on the timestamps. Drop the strtoull() and ERANGE references from those test comments. Issue: F-2881, F-2882 --- src/wolfscp.c | 106 ++++++++++++++++++++++++++++++------------------- src/wolfterm.c | 20 ++++++++-- tests/unit.c | 64 +++++++++++++++++++++++------ 3 files changed, 134 insertions(+), 56 deletions(-) diff --git a/src/wolfscp.c b/src/wolfscp.c index 66f517edf..441549673 100644 --- a/src/wolfscp.c +++ b/src/wolfscp.c @@ -40,7 +40,6 @@ #include #include -#include #ifdef NO_INLINE @@ -1076,6 +1075,54 @@ static int FindSpaceInString(byte* buf, word32 bufSz, word32* inOutIdx) return WS_SUCCESS; } +/* Parse a base-10 unsigned integer from an SCP header field of exactly + * len bytes. Every byte must be a digit, so a signed field such as "-1" + * or "+1" and any leading whitespace are rejected rather than parsed. + * Values above max are rejected instead of wrapping. + * + * Hand-rolled rather than using strtoull(), which is C99 and so is not + * available on every port, and which would accept the signed and + * whitespace-padded forms above. + * + * str - start of the field + * len - field length, up to but not including the separator + * max - largest accepted value + * out - [OUT] parsed value, untouched on failure + * + * returns WS_SUCCESS on success, WS_BAD_ARGUMENT if str or out is NULL, + * WS_SCP_BAD_MSG_E on a malformed or out-of-range field + */ +static int ScpParseUInt64(const char* str, word32 len, word64 max, + word64* out) +{ + word64 val = 0; + word32 i; + + if (str == NULL || out == NULL) + return WS_BAD_ARGUMENT; + + if (len == 0) + return WS_SCP_BAD_MSG_E; + + for (i = 0; i < len; i++) { + word64 d; + + if (str[i] < '0' || str[i] > '9') + return WS_SCP_BAD_MSG_E; + + /* check d against max first so the subtraction cannot underflow */ + d = (word64)(str[i] - '0'); + if (d > max || val > (max - d) / 10) + return WS_SCP_BAD_MSG_E; + + val = val * 10 + d; + } + + *out = val; + return WS_SUCCESS; +} + + /* Reads file size from beginning of string, expects space to be after, * places size in ssh->scpFileSz. * @@ -1101,26 +1148,14 @@ static int GetScpFileSize(WOLFSSH* ssh, byte* buf, word32 bufSz, ret = WS_SCP_BAD_MSG_E; if (ret == WS_SUCCESS) { - /* replace space with newline to terminate the size field, then parse - * with strtoull() which parses in 64-bit width, so a negative field - * such as "-1" wraps above UINT32_MAX and is rejected by the bound - * below instead of becoming a huge word32 size */ - char* endptr = NULL; - word64 fileSz; - - buf[spaceIdx] = '\n'; - errno = 0; - fileSz = (word64)strtoull((char*)(buf + idx), &endptr, 10); - buf[spaceIdx] = ' '; - - /* reject any parse error (e.g. ERANGE overflow), a non-numeric field - * (parse must consume every character up to the separator), and - * sizes too large for the word32 scpFileSz */ - if (errno != 0 || endptr != (char*)(buf + spaceIdx) || - fileSz > UINT32_MAX) { - ret = WS_SCP_BAD_MSG_E; - } - else { + /* the size field runs from idx up to the separating space; bound it + * to 0xFFFFFFFF to fit the word32 scpFileSz */ + word64 fileSz = 0; + + ret = ScpParseUInt64((const char*)(buf + idx), spaceIdx - idx, + 0xFFFFFFFFUL, &fileSz); + + if (ret == WS_SUCCESS) { ssh->scpFileSz = (word32)fileSz; /* increment idx to space, then eat trailing space */ @@ -1249,17 +1284,12 @@ static int GetScpTimestamp(WOLFSSH* ssh, byte* buf, word32 bufSz, /* read modification time */ if (ret == WS_SUCCESS) { - char* endptr = NULL; - - /* replace space with newline to terminate the field */ - buf[spaceIdx] = '\n'; - errno = 0; - ssh->scpMTime = (word64)strtoull((char*)(buf + idx), &endptr, 10); - buf[spaceIdx] = ' '; - - /* reject any parse error (e.g. ERANGE overflow) and a non-numeric - * field, then step past the separating space */ - if (errno != 0 || endptr != (char*)(buf + spaceIdx)) { + /* the field runs from idx up to the separating space, then step past + * that space; report the timestamp error rather than the bad message + * error the parser returns */ + if (ScpParseUInt64((const char*)(buf + idx), spaceIdx - idx, + W64LIT(0xFFFFFFFFFFFFFFFF), + &ssh->scpMTime) != WS_SUCCESS) { ret = WS_SCP_TIMESTAMP_E; } else if (spaceIdx + 1 < bufSz) { @@ -1296,15 +1326,9 @@ static int GetScpTimestamp(WOLFSSH* ssh, byte* buf, word32 bufSz, } if (ret == WS_SUCCESS) { - char* endptr = NULL; - /* replace space with newline for strtoull */ - buf[spaceIdx] = '\n'; - errno = 0; - ssh->scpATime = (word64)strtoull((char*)(buf + idx), &endptr, 10); - /* restore space, increment idx past it */ - buf[spaceIdx] = ' '; - - if (errno != 0 || endptr != (char*)(buf + spaceIdx)) { + if (ScpParseUInt64((const char*)(buf + idx), spaceIdx - idx, + W64LIT(0xFFFFFFFFFFFFFFFF), + &ssh->scpATime) != WS_SUCCESS) { ret = WS_SCP_TIMESTAMP_E; } else if (spaceIdx + 1 < bufSz) { diff --git a/src/wolfterm.c b/src/wolfterm.c index 3c86c0ad3..17512d0fe 100644 --- a/src/wolfterm.c +++ b/src/wolfterm.c @@ -317,11 +317,25 @@ 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 to 0 any param that is empty, has bytes left over after the + * digits, or falls outside 0..65535, which covers the VT100 params. */ +static word32 parseArg(const byte* s) +{ + char* endp; + long v = strtol((const char*)s, &endp, 10); + if (endp == (const char*)s || *endp != '\0' || 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) { word32 i = 0, numArgs = 0; - word32 maxIdx = WOLFSSH_MAX_CONSOLE_ARGS * 4; byte tmpBuf[WOLFSSH_MAX_CONSOLE_ARGS * 4]; word32 tmpBufIdx = 0; @@ -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; } @@ -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++; } diff --git a/tests/unit.c b/tests/unit.c index 633a50f4e..9dcdfe182 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -2693,10 +2693,10 @@ static int test_ScpGetFileMode(void) return result; } -/* Drive GetScpFileSize directly, covering the strtoull()-based parsing added to - * replace atoi: valid sizes up to UINT32_MAX are accepted, while negative, - * out-of-range, and non-numeric fields are rejected instead of silently - * wrapping into a bogus word32 size. */ +/* Drive GetScpFileSize directly, covering the digit-validating parsing added to + * replace atoi: valid sizes up to UINT32_MAX are accepted, while signed, + * whitespace-padded, empty, out-of-range, and non-numeric fields are rejected + * instead of silently wrapping into a bogus word32 size. */ static int test_ScpGetFileSize(void) { WOLFSSH_CTX* ctx = NULL; @@ -2704,13 +2704,17 @@ static int test_ScpGetFileSize(void) static const char* good[] = { "0 f\n", "1024 file.txt\n", + "007 f\n", /* leading zeros are accepted */ "4294967295 f\n" /* UINT32_MAX, the largest accepted size */ }; - static const word32 goodSz[] = { 0, 1024, 4294967295UL }; + static const word32 goodSz[] = { 0, 1024, 7, 4294967295UL }; static const char* bad[] = { - "-1 f\n", /* negative wraps above UINT32_MAX */ + "-1 f\n", /* a sign is not a digit */ + "+1 f\n", + "\t1 f\n", /* leading whitespace is not a digit */ + " f\n", /* empty field, separator comes first */ "4294967296 f\n", /* one past UINT32_MAX */ - "18446744073709551616 f\n", /* overflows 64-bit width (ERANGE) */ + "18446744073709551616 f\n", /* overflows the 64-bit accumulator */ "12x34 f\n", /* trailing junk before the separator */ "abc f\n" /* not numeric at all */ }; @@ -2759,26 +2763,44 @@ static int test_ScpGetFileSize(void) } } + /* a NUL byte inside the field does not make it a short field: parsing by + * length rejects "1\0" "23" rather than stopping at the NUL to accept 1 */ + if (result == 0) { + WMEMSET(tmp, 0, sizeof(tmp)); + WMEMCPY(tmp, "1\0" "23 f\n", 7); + idx = 0; + ret = wolfSSH_TestScpGetFileSize(ssh, (byte*)tmp, 7, &idx); + if (ret == WS_SUCCESS) + result = -435; + } + wolfSSH_free(ssh); wolfSSH_CTX_free(ctx); return result; } -/* Drive GetScpTimestamp directly, covering the strtoull()-based parsing of the +/* Drive GetScpTimestamp directly, covering the digit-validating parsing of the * "T 0 0" record: a well-formed record populates both times, - * while a missing 'T', malformed separator, non-numeric field, and overflowing - * field are all rejected. */ + * while a missing 'T', malformed separator, signed, whitespace-padded, + * non-numeric, and overflowing field are all rejected. These fields are + * unbounded, so a sign is the only thing keeping "-1" from being taken as the + * wrapped 64-bit maximum. */ static int test_ScpGetTimestamp(void) { WOLFSSH_CTX* ctx = NULL; WOLFSSH* ssh = NULL; static const char* bad[] = { "1609459200 0 1609459200 0\n", /* missing leading 'T' */ + "T-1 0 100 0\n", /* a sign is not a digit */ + "T+1 0 100 0\n", + "T\t1 0 100 0\n", /* leading whitespace */ "T12x 0 100 0\n", /* mtime not fully numeric */ - "T99999999999999999999999999 0 100 0\n", /* mtime overflow (ERANGE) */ + "T18446744073709551616 0 100 0\n", /* one past the 64-bit max */ + "T99999999999999999999999999 0 100 0\n", /* mtime overflow */ "T100 5 100 0\n", /* mtime separator not "0 " */ + "T100 0 -1 0\n", /* negative atime */ "T100 0 abc 0\n", /* atime not numeric */ - "T100 0 99999999999999999999999999 0\n" /* atime overflow (ERANGE) */ + "T100 0 99999999999999999999999999 0\n" /* atime overflow */ }; char tmp[64]; int result = 0; @@ -2823,6 +2845,24 @@ static int test_ScpGetTimestamp(void) } } + /* the fields are unbounded, so the 64-bit maximum itself is accepted */ + if (result == 0) { + WMEMSET(tmp, 0, sizeof(tmp)); + WSTRNCPY(tmp, "T18446744073709551615 0 18446744073709551614 0\n", + sizeof(tmp) - 1); + idx = 0; + ssh->scpMTime = 0; + ssh->scpATime = 0; + ret = wolfSSH_TestScpGetTimestamp(ssh, (byte*)tmp, + (word32)WSTRLEN(tmp), &idx); + if (ret != WS_SUCCESS) + result = -446; + else if (ssh->scpMTime != W64LIT(0xFFFFFFFFFFFFFFFF)) + result = -447; + else if (ssh->scpATime != W64LIT(0xFFFFFFFFFFFFFFFE)) + result = -448; + } + wolfSSH_free(ssh); wolfSSH_CTX_free(ctx); return result;