fix: harden peer-supplied atoi parsing

src/wolfscp.c

@@ -40,7 +40,6 @@
 #include <wolfssh/log.h>
 
 #include <errno.h>
-#include <stdint.h>
 
 
 #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) {

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++;
     }
 

tests/unit.c

@@ -2693,24 +2693,28 @@ 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;
     WOLFSSH* ssh = NULL;
     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<mtime> 0 <atime> 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;