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

Author: aidankeefe2022

src/internal.c

@@ -14168,6 +14168,28 @@ 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) {
+        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);
+        return WS_BAD_ARGUMENT;
+    }
+    return WS_SUCCESS;
+}
+
+
 int SendProtoId(WOLFSSH* ssh)
 {
     int ret = WS_SUCCESS;

src/ssh.c

@@ -3472,10 +3472,18 @@ int wolfSSH_GetMaxAuthAttempts(WOLFSSH* ssh)
 int wolfSSH_CTX_SetSshProtoIdStr(WOLFSSH_CTX* ctx,
                                           const char* protoIdStr)
 {
+    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) {
+        return ret;
+    }
+
     ctx->sshProtoIdStr = protoIdStr;
     return WS_SUCCESS;
 }

tests/unit.c

@@ -557,42 +557,33 @@ static int test_DoProtoId(void)
         }
     }
 
-    /* A non-conforming local proto ID doesn't reject a conforming peer. */
+    /* Ensure a malformed local protoId cannot be loaded. */
     {
-        static const ProtoIdTestVector customTv = {
-            "custom local proto ID accepts peer",
-            "SSH-2.0-OpenSSH_8.9\r\n",
-            0, WS_SUCCESS, WOLFSSH_ENDPOINT_CLIENT
+        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 },
+            { "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 },
         };
-        ProtoIdTestState state;
-
-        state.tv = &customTv;
-        state.offset = 0;
+        int pc = (int)(sizeof(protoIds) / sizeof(protoIds[0]));
 
-        wolfSSH_SetIORecv(clientCtx, RecvFromPtr);
-        if (wolfSSH_CTX_SetSshProtoIdStr(clientCtx,
-                    "SSH-2.0_NonConforming\r\n") != WS_SUCCESS) {
-            fprintf(stderr, "\t\"%s\" FAIL: SetSshProtoIdStr failed\n",
-                    customTv.name);
-            failures++;
-        }
-        else {
-            ssh = wolfSSH_new(clientCtx);
-            if (ssh == NULL) {
-                fprintf(stderr, "\t\"%s\" FAIL: wolfSSH_new returned NULL\n",
-                        customTv.name);
+        for (i = 0; i < pc; i++) {
+            ret = wolfSSH_CTX_SetSshProtoIdStr(clientCtx, protoIds[i].id);
+            if ((ret == WS_SUCCESS) != protoIds[i].expectSuccess) {
+                fprintf(stderr,
+                        "\t[protoId %d] \"%s\" FAIL: got %d, expected %s\n",
+                        i, protoIds[i].name, ret,
+                        protoIds[i].expectSuccess ? "WS_SUCCESS"
+                                                  : "WS_BAD_ARGUMENT");
                 failures++;
             }
-            else {
-                wolfSSH_SetIOReadCtx(ssh, &state);
-                ret = wolfSSH_TestDoProtoId(ssh);
-                if (ret != customTv.expected) {
-                    fprintf(stderr, "\t\"%s\" FAIL: got %d, expected %d\n",
-                            customTv.name, ret, customTv.expected);
-                    failures++;
-                }
-                wolfSSH_free(ssh);
-            }
         }
     }
 

wolfssh/internal.h

@@ -716,6 +716,8 @@ enum NameIdType {
 #ifndef WOLFSSH_MAX_BANNER_LINES
     #define WOLFSSH_MAX_BANNER_LINES 10
 #endif
+/* RFC 4253 4.2. Documented as a literal 255 on
+ * wolfSSH_CTX_SetSshProtoIdStr() in the public ssh.h; keep in sync. */
 #define WOLFSSH_PROTOID_LIMIT 255
 
 /* Keep track of keying state for both sides of the connection.
@@ -1645,6 +1647,7 @@ WOLFSSH_LOCAL int DoProtoId(WOLFSSH* ssh);
 WOLFSSH_LOCAL int wolfSSH_SendPacket(WOLFSSH* ssh);
 WOLFSSH_LOCAL int wolfSSH_OutputPending(WOLFSSH* ssh);
 WOLFSSH_LOCAL int SendProtoId(WOLFSSH* ssh);
+WOLFSSH_LOCAL int ValidateProtoId(const char* protoIdStr, word32 len);
 WOLFSSH_LOCAL int SendKexInit(WOLFSSH* ssh);
 WOLFSSH_LOCAL int SendKexDhInit(WOLFSSH* ssh);
 WOLFSSH_LOCAL int SendKexDhReply(WOLFSSH* ssh);

wolfssh/ssh.h

@@ -650,6 +650,17 @@ WOLFSSH_API int wolfSSH_SetUsername(WOLFSSH* ssh, const char* username);
 WOLFSSH_API char* wolfSSH_GetUsername(WOLFSSH* ssh);
 
 WOLFSSH_API int wolfSSH_CTX_SetBanner(WOLFSSH_CTX* ctx, const char* newBanner);
+/* ProtoIdStr is checked for validity and will be rejected unless
+ * it adheres to these criteria:
+ * MUST begin with "SSH-2.0-"
+ * 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
+ * If these are not adhered to the function will return WS_BAD_ARGUMENT
+ * and not load the ProtoId in to 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. */
 WOLFSSH_API int wolfSSH_CTX_SetSshProtoIdStr(WOLFSSH_CTX* ctx,
         const char* protoIdStr);
 /* Set the server-side limit on failed userauth attempts per connection. The