Validate a custom SSH proto ID string

wolfSSH_CTX_SetSshProtoIdStr() now rejects a string that is not
CRLF-terminated, exceeds 255 bytes counting the terminator, or
carries a CR or LF in the body. DoKexInit() subtracts the two
terminator bytes from the length when hashing it, so an
unterminated string underflowed the hash length.

Issue: F-10571

partial progress

Author: aidankeefe2022

src/internal.c

@@ -1337,6 +1337,7 @@ WOLFSSH_CTX* CtxInit(WOLFSSH_CTX* ctx, byte side, void* heap)
     ctx->maxPacketSz = DEFAULT_MAX_PACKET_SZ;
     ctx->maxAuthAttempts = DEFAULT_MAX_AUTH_ATTEMPTS;
     ctx->sshProtoIdStr = sshProtoIdStr;
+    ctx->sshProtoIdStrSz = (word32)(sizeof(sshProtoIdStr) - 1);
     ctx->algoListKex = cannedKexAlgoNames;
     if (side == WOLFSSH_ENDPOINT_CLIENT) {
         ctx->algoListKey = cannedKeyAlgoNamesHostKey;
@@ -6533,9 +6534,8 @@ static int DoKexInit(WOLFSSH* ssh, byte* buf, word32 len, word32* idx)
         }
 
         if (ret == WS_SUCCESS) {
-            byte SSH_PROTO_EOL_SZ = 2;
-
-            strSz = (word32)WSTRLEN(ssh->ctx->sshProtoIdStr) - SSH_PROTO_EOL_SZ;
+            /* The ID is hashed without its terminator. */
+            strSz = ssh->ctx->sshProtoIdStrSz - SSH_PROTO_EOL_SZ;
             c32toa(strSz, scratchLen);
             ret = HashUpdate(hash, hashId, scratchLen, LENGTH_SZ);
         }
@@ -14185,21 +14185,38 @@ int DoProtoId(WOLFSSH* ssh)
 /* Validates a locally configured proto ID string */
 int ValidateProtoId(const char* protoIdStr, word32 len)
 {
-    /* Length is checked first: the prefix, terminator, and body checks below
-     * index and subtract from len. The minimum is the "SSH-2.0-" prefix plus
-     * one body byte plus CRLF. */
-    if (protoIdStr == NULL || len < SSH_PROTO_SZ + 3 ||
-            protoIdStr[len-1] != '\n' || protoIdStr[len-2] != '\r' ||
-            len > WOLFSSH_PROTOID_LIMIT ||
-            WSTRNCMP(protoIdStr, sshProtoIdPrefix, SSH_PROTO_SZ) != 0 ||
-            WSTRNSTR(protoIdStr, "\n", len - 2) != NULL ||
-            WSTRNSTR(protoIdStr, "\r", len - 2) != NULL) {
+    word32 i;
+
+    if (protoIdStr == NULL ||
+            len < SSH_PROTO_SZ + 1 + SSH_PROTO_EOL_SZ ||
+            len > WOLFSSH_PROTOID_LIMIT) {
+        WLOG(WS_LOG_ERROR, "Proto Id was invalid: it must be between %d and "
+                "%d bytes, counting the prefix and the terminator",
+                SSH_PROTO_SZ + 1 + SSH_PROTO_EOL_SZ, WOLFSSH_PROTOID_LIMIT);
+        return WS_BAD_ARGUMENT;
+    }
+
+    if (WSTRNCMP(protoIdStr, sshProtoIdPrefix, SSH_PROTO_SZ) != 0) {
         WLOG(WS_LOG_ERROR, "Proto Id was invalid: it must start with "
-                "\"SSH-2.0-\", end in \\r\\n, be no longer than %d bytes, "
-                "and must not contain \\r or \\n in the body of the line",
-                WOLFSSH_PROTOID_LIMIT);
+                "\"SSH-2.0-\"");
         return WS_BAD_ARGUMENT;
     }
+
+    if (protoIdStr[len - 1] != '\n' || protoIdStr[len - 2] != '\r') {
+        WLOG(WS_LOG_ERROR, "Proto Id was invalid: it must end in \\r\\n");
+        return WS_BAD_ARGUMENT;
+    }
+
+    for (i = 0; i < len - SSH_PROTO_EOL_SZ; i++) {
+        byte c = (byte)protoIdStr[i];
+
+        if (c < 0x20 || c > 0x7e) {
+            WLOG(WS_LOG_ERROR, "Proto Id was invalid: byte %u is "
+                    "not printable US-ASCII", i);
+            return WS_BAD_ARGUMENT;
+        }
+    }
+
     return WS_SUCCESS;
 }
 
@@ -14214,7 +14231,7 @@ int SendProtoId(WOLFSSH* ssh)
 
     if (ret == WS_SUCCESS) {
         WLOG(WS_LOG_DEBUG, "%s", ssh->ctx->sshProtoIdStr);
-        sshProtoIdStrSz = (word32)WSTRLEN(ssh->ctx->sshProtoIdStr);
+        sshProtoIdStrSz = ssh->ctx->sshProtoIdStrSz;
         ret = GrowBuffer(&ssh->outputBuffer, sshProtoIdStrSz);
     }
 

src/ssh.c

@@ -3472,19 +3472,23 @@ int wolfSSH_GetMaxAuthAttempts(WOLFSSH* ssh)
 int wolfSSH_CTX_SetSshProtoIdStr(WOLFSSH_CTX* ctx,
                                           const char* protoIdStr)
 {
+    word32 protoIdStrSz;
     int ret;
+
     WLOG(WS_LOG_DEBUG, "Entering wolfSSH_CTX_SetSshProtoIdStr()");
 
     if (!ctx || !protoIdStr) {
         return WS_BAD_ARGUMENT;
     }
 
-    if ((ret = ValidateProtoId(protoIdStr, (word32)WSTRLEN(protoIdStr))) !=
-            WS_SUCCESS) {
+    protoIdStrSz = (word32)WSTRLEN(protoIdStr);
+    ret = ValidateProtoId(protoIdStr, protoIdStrSz);
+    if (ret != WS_SUCCESS) {
         return ret;
     }
 
     ctx->sshProtoIdStr = protoIdStr;
+    ctx->sshProtoIdStrSz = protoIdStrSz;
     return WS_SUCCESS;
 }
 

tests/unit.c

@@ -559,20 +559,40 @@ static int test_DoProtoId(void)
 
     /* Ensure a malformed local protoId cannot be loaded. */
     {
+        static char tooLongProtoId[257];
+        static char justRightProtoId[256];
         static const struct {
             const char* name;
             const char* id;
             int expectSuccess;
         } protoIds[] = {
-            { "conforming custom ID", "SSH-2.0-this_is_my_app\r\n", 1 },
-            { "wrong version prefix", "SSH-2-this_is_my_app\r\n",   0 },
+            { "conforming custom ID", "SSH-2.0-this_is_my_app\r\n",  1 },
+            { "shortest valid Id",    "SSH-2.0-t\r\n",               1 },
+            { "exact len custom ID",  justRightProtoId,              1 },
+            { "wrong version prefix", "SSH-2-this_is_my_app\r\n",    0 },
+            { "bad casing prefix",    "sSH-2.0-this_is_my_app\r\n",  0 },
             { "LF terminator only",   "SSH-2.0-this_is_my_app\n",    0 },
             { "CR terminator only",   "SSH-2.0-this_is_my_app\r",    0 },
             { "empty string",         "",                            0 },
             { "prefix with no body",  "SSH-2.0-\r\n",                0 },
-            { "missing prefix",       "hello\r\n",                   0 },
+            { "missing prefix",       "hello-this-is\r\n",           0 },
+            { "non ascii char",       "SSH-2.0-\x90s\r\n",           0 },
+            { "Body End in CR",       "SSH-2.0-s\r\r\n",             0 },
+            { "Body End in TAB",      "SSH-2.0-s\t\r\n",             0 },
+            { "Body Have bad char",   "SSH-2.0-\x02-a\t\r\n",        0 },
+            { "too long id",          tooLongProtoId,                0 },
         };
         int pc = (int)(sizeof(protoIds) / sizeof(protoIds[0]));
+        WMEMSET(tooLongProtoId, 'a', sizeof(tooLongProtoId));
+        WMEMCPY(tooLongProtoId, "SSH-2.0-", sizeof("SSH-2.0-") - 1);
+        tooLongProtoId[256] = '\0';
+        tooLongProtoId[255] = '\n';
+        tooLongProtoId[254] = '\r';
+        WMEMSET(justRightProtoId, 'a', sizeof(justRightProtoId));
+        WMEMCPY(justRightProtoId, "SSH-2.0-", sizeof("SSH-2.0-") - 1);
+        justRightProtoId[255] = '\0';
+        justRightProtoId[254] = '\n';
+        justRightProtoId[253] = '\r';
 
         for (i = 0; i < pc; i++) {
             ret = wolfSSH_CTX_SetSshProtoIdStr(clientCtx, protoIds[i].id);

wolfssh/internal.h

@@ -576,6 +576,7 @@ enum NameIdType {
 #define UINT32_SZ 4
 #define LENGTH_SZ UINT32_SZ
 #define SSH_PROTO_SZ 8 /* "SSH-2.0-" */
+#define SSH_PROTO_EOL_SZ 2 /* "\r\n" */
 #define TERMINAL_MODE_SZ 5 /* opcode byte + argument uint32 */
 #define TERMINAL_MODES_MAX_SZ 4096
 #define TERMINAL_WIDTH_DEFAULT 80 /* used when there is no terminal */
@@ -862,6 +863,7 @@ struct WOLFSSH_CTX {
     const char* algoListMac;
     const char* algoListKeyAccepted;
     word32 bannerSz;
+    word32 sshProtoIdStrSz;           /* validated, counting the CRLF */
     word32 windowSz;
     word32 maxPacketSz;
     word32 maxAuthAttempts;           /* server cap on failed userauth */

wolfssh/ssh.h

@@ -656,11 +656,13 @@ WOLFSSH_API int wolfSSH_CTX_SetBanner(WOLFSSH_CTX* ctx, const char* newBanner);
  * MUST be between 11 and 255 bytes in length, counting the "SSH-2.0-"
  *     prefix and the trailing "\r\n"
  * MUST end with '\r\n'
- * MUST NOT contain '\r' or '\n' in the body
+ * MUST carry only printable US-ASCII (0x20 - 0x7e) in the body, which
+ *     rules out an embedded '\r' or '\n'
  * If these are not adhered to the function will return WS_BAD_ARGUMENT
- * and not load the ProtoId in to the WOLFSSH_CTX struct.
+ * and not load the ProtoId into the WOLFSSH_CTX struct.
  * ProtoIdStr is stored by reference and is not copied, so it must remain
- * valid for the lifetime of the WOLFSSH_CTX. */
+ * valid and unmodified for the lifetime of the WOLFSSH_CTX. It is validated
+ * once, here; a later in-place rewrite of the buffer is not revalidated. */
 WOLFSSH_API int wolfSSH_CTX_SetSshProtoIdStr(WOLFSSH_CTX* ctx,
         const char* protoIdStr);
 /* Set the server-side limit on failed userauth attempts per connection. The