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;