Skoll review and Github review

Added more test vectors as well as locking in behavior of VerifyProtoId
Added round trip test for proto id
Comment fixes
New define for proto id min size

Author: aidankeefe2022

src/internal.c

@@ -14187,12 +14187,15 @@ int ValidateProtoId(const char* protoIdStr, word32 len)
 {
     word32 i;
 
+    /* The length check must stay first: every check below indexes into
+     * protoIdStr or subtracts from the unsigned len. The minimum is the
+     * "SSH-2.0-" prefix plus one body byte plus CRLF. */
     if (protoIdStr == NULL ||
-            len < SSH_PROTO_SZ + 1 + SSH_PROTO_EOL_SZ ||
+            len < SSH_PROTO_MIN ||
             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);
+                SSH_PROTO_MIN, WOLFSSH_PROTOID_LIMIT);
         return WS_BAD_ARGUMENT;
     }
 
@@ -14202,14 +14205,23 @@ int ValidateProtoId(const char* protoIdStr, word32 len)
         return WS_BAD_ARGUMENT;
     }
 
+    /* RFC 4253 section 4.2 splits the line as "SSH-2.0-" softwareversion
+     * [SP comments] CRLF. A leading space would make softwareversion
+     * empty, so reject it. */
+    if (protoIdStr[SSH_PROTO_SZ] == ' ') {
+        WLOG(WS_LOG_ERROR, "Proto Id was invalid: the body must start with a "
+                "non-space character");
+        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];
-
+        /* spaces are intetionally allowed */
         if (c < 0x20 || c > 0x7e) {
             WLOG(WS_LOG_ERROR, "Proto Id was invalid: byte %u is "
                     "not printable US-ASCII", i);
@@ -24268,6 +24280,11 @@ int wolfSSH_TestDoProtoId(WOLFSSH* ssh)
     return DoProtoId(ssh);
 }
 
+int wolfSSH_TestSendProtoId(WOLFSSH* ssh)
+{
+    return SendProtoId(ssh);
+}
+
 int wolfSSH_TestIsMessageAllowed(WOLFSSH* ssh, byte msg, byte state)
 {
     return IsMessageAllowed(ssh, msg, state);

tests/unit.c

@@ -464,6 +464,30 @@ static const ProtoIdScriptVector protoIdScriptVectors[] = {
       WOLFSSH_ENDPOINT_CLIENT, WS_VERSION_E },
 };
 
+/* Capture-to-buffer send callback for the SendProtoId() vectors. The
+ * proto ID is the first thing on the wire, so everything the callback
+ * sees is the ID line itself. */
+typedef struct ProtoIdSendState {
+    byte   buf[WOLFSSH_PROTOID_LIMIT + 1];
+    word32 len;
+} ProtoIdSendState;
+
+static int ProtoIdCaptureSend(WOLFSSH* ssh, void* buf, word32 sz, void* ctx)
+{
+    ProtoIdSendState* s = (ProtoIdSendState*)ctx;
+
+    WOLFSSH_UNUSED(ssh);
+
+    if (sz > sizeof(s->buf) - s->len)
+        return WS_CBIO_ERR_GENERAL;
+
+    WMEMCPY(s->buf + s->len, buf, sz);
+    s->len += sz;
+
+    return (int)sz;
+}
+
+
 /* DoProtoId() Unit Test */
 static int test_DoProtoId(void)
 {
@@ -557,44 +581,54 @@ static int test_DoProtoId(void)
         }
     }
 
+    {
+    static char tooLongProtoId[WOLFSSH_PROTOID_LIMIT + 2];
+    static char justRightProtoId[WOLFSSH_PROTOID_LIMIT + 1];
     /* 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 },
-            { "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-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 },
+            { "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 },
+            /* Pin the printable-ASCII range as inclusive at both ends: an
+             * interior 0x20 (the RFC 4253 "SP comments" suffix) and a
+             * 0x7e must both be accepted. */
+            { "body w/ SP comments",   "SSH-2.0-app comment\r\n",     1 },
+            { "body w/ tilde",         "SSH-2.0-app~1\r\n",           1 },
+            /* Failing Tests */
+            { "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-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 starts w/ space",  "SSH-2.0-\x20-a-b\r\n",        0 },
+            { "body has embedded TAB", "SSH-2.0-\x7e-a\t\r\n",        0 },
+            { "too long id",           tooLongProtoId,                0 },
+            { "null pointer",          NULL,                          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';
+        tooLongProtoId[WOLFSSH_PROTOID_LIMIT + 1] = '\0';
+        tooLongProtoId[WOLFSSH_PROTOID_LIMIT]     = '\n';
+        tooLongProtoId[WOLFSSH_PROTOID_LIMIT - 1] = '\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';
+        justRightProtoId[WOLFSSH_PROTOID_LIMIT]     = '\0';
+        justRightProtoId[WOLFSSH_PROTOID_LIMIT - 1] = '\n';
+        justRightProtoId[WOLFSSH_PROTOID_LIMIT - 2] = '\r';
 
         for (i = 0; i < pc; i++) {
+            const char* prevId = clientCtx->sshProtoIdStr;
             ret = wolfSSH_CTX_SetSshProtoIdStr(clientCtx, protoIds[i].id);
             if ((ret == WS_SUCCESS) != protoIds[i].expectSuccess) {
                 fprintf(stderr,
@@ -604,17 +638,81 @@ static int test_DoProtoId(void)
                                                   : "WS_BAD_ARGUMENT");
                 failures++;
             }
-            if ((ret == WS_SUCCESS) && clientCtx->sshProtoIdStrSz !=
-                    WSTRLEN(protoIds[i].id)) {
+            if (!protoIds[i].expectSuccess &&
+                clientCtx->sshProtoIdStr != prevId) {
+                fprintf(stderr,
+                        "\t[protoId %d] \"%s\" FAIL: invalid proto id "
+                        "was stored\n",
+                        i, protoIds[i].name);
+                failures++;
+            }
+            if (clientCtx->sshProtoIdStrSz !=
+                    (word32)WSTRLEN(clientCtx->sshProtoIdStr)) {
                 fprintf(stderr,
-                        "\t[protoId %d] \"%s\" FAIL: stored sshProtoIdSz "
+                        "\t[protoId %d] \"%s\" FAIL: stored sshProtoIdStrSz "
                         "was not retained\n",
                         i, protoIds[i].name);
                 failures++;
             }
         }
     }
 
+    /* A configured proto ID must reach the wire byte for byte. */
+    {
+        static const char* const sendIds[] = {
+            "SSH-2.0-this_is_my_app\r\n",
+            "SSH-2.0-t\r\n",
+            "SSH-2.0-app comment\r\n",
+            justRightProtoId,
+        };
+        int sc = (int)(sizeof(sendIds) / sizeof(sendIds[0]));
+
+        wolfSSH_SetIOSend(clientCtx, ProtoIdCaptureSend);
+
+        for (i = 0; i < sc; i++) {
+            ProtoIdSendState sendState;
+            word32 expectSz = (word32)WSTRLEN(sendIds[i]);
+
+            ret = wolfSSH_CTX_SetSshProtoIdStr(clientCtx, sendIds[i]);
+            if (ret != WS_SUCCESS) {
+                fprintf(stderr,
+                        "\t[send %d] FAIL: set proto id returned %d\n",
+                        i, ret);
+                failures++;
+                continue;
+            }
+
+            ssh = wolfSSH_new(clientCtx);
+            if (ssh == NULL) {
+                fprintf(stderr,
+                        "\t[send %d] FAIL: wolfSSH_new returned NULL\n", i);
+                failures++;
+                continue;
+            }
+
+            WMEMSET(&sendState, 0, sizeof(sendState));
+            wolfSSH_SetIOWriteCtx(ssh, &sendState);
+
+            ret = wolfSSH_TestSendProtoId(ssh);
+            if (ret != WS_SUCCESS) {
+                fprintf(stderr,
+                        "\t[send %d] FAIL: SendProtoId returned %d\n",
+                        i, ret);
+                failures++;
+            }
+            else if (sendState.len != expectSz ||
+                     WMEMCMP(sendState.buf, sendIds[i], expectSz) != 0) {
+                fprintf(stderr,
+                        "\t[send %d] FAIL: wrote %u bytes, expected the "
+                        "%u byte proto id back verbatim\n",
+                        i, sendState.len, expectSz);
+                failures++;
+            }
+            wolfSSH_free(ssh);
+        }
+    }
+    }
+
     wolfSSH_CTX_free(serverCtx);
     wolfSSH_CTX_free(clientCtx);
 

wolfssh/internal.h

@@ -577,6 +577,9 @@ enum NameIdType {
 #define LENGTH_SZ UINT32_SZ
 #define SSH_PROTO_SZ 8 /* "SSH-2.0-" */
 #define SSH_PROTO_EOL_SZ 2 /* "\r\n" */
+/* Minium size for a valid proto id           *
+ * "SSH-2.0-" <at least one body char> "\r\n" */
+#define SSH_PROTO_MIN (SSH_PROTO_SZ + 1 + SSH_PROTO_EOL_SZ)
 #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 */
@@ -1938,6 +1941,7 @@ enum WS_MessageIdLimits {
 
 #ifdef WOLFSSH_TEST_INTERNAL
     WOLFSSH_API int wolfSSH_TestDoProtoId(WOLFSSH* ssh);
+    WOLFSSH_API int wolfSSH_TestSendProtoId(WOLFSSH* ssh);
     WOLFSSH_API int wolfSSH_TestIsMessageAllowed(WOLFSSH* ssh, byte msg,
             byte state);
     WOLFSSH_API int wolfSSH_TestDoReceive(WOLFSSH* ssh);

wolfssh/ssh.h

@@ -658,6 +658,10 @@ WOLFSSH_API int wolfSSH_CTX_SetBanner(WOLFSSH_CTX* ctx, const char* newBanner);
  * MUST end with '\r\n'
  * MUST carry only printable US-ASCII (0x20 - 0x7e) in the body, which
  *     rules out an embedded '\r' or '\n'
+ * MUST NOT begin the body with a space; RFC 4253 section 4.2 reads the
+ *     body as softwareversion [SP comments], so a leading space would
+ *     make softwareversion empty. A space later in the body is accepted
+ *     and starts the optional comments field.
  * If these are not adhered to the function will return WS_BAD_ARGUMENT
  * and not load the ProtoId into the WOLFSSH_CTX struct.
  * ProtoIdStr is stored by reference and is not copied, so it must remain