From 4553b3c659628c466d8b82a61b23ae0740489273 Mon Sep 17 00:00:00 2001 From: Aidan Keefe Date: Fri, 4 Sep 2026 12:42:06 -0600 Subject: [PATCH] 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 --- src/internal.c | 23 ++++++- tests/unit.c | 150 +++++++++++++++++++++++++++++++++++++-------- wolfssh/internal.h | 4 ++ wolfssh/ssh.h | 4 ++ 4 files changed, 152 insertions(+), 29 deletions(-) diff --git a/src/internal.c b/src/internal.c index 7b02857f..5ef5f155 100644 --- a/src/internal.c +++ b/src/internal.c @@ -14628,12 +14628,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; } @@ -14643,6 +14646,15 @@ 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; @@ -14650,7 +14662,7 @@ int ValidateProtoId(const char* protoIdStr, word32 len) 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); @@ -25496,6 +25508,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); diff --git a/tests/unit.c b/tests/unit.c index f77fdbed..ffb2798b 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -514,6 +514,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) { @@ -607,44 +631,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[sizeof(tooLongProtoId) - 1] = '\0'; + tooLongProtoId[sizeof(tooLongProtoId) - 2] = '\n'; + tooLongProtoId[sizeof(tooLongProtoId) - 3] = '\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[sizeof(justRightProtoId) - 1] = '\0'; + justRightProtoId[sizeof(justRightProtoId) - 2] = '\n'; + justRightProtoId[sizeof(justRightProtoId) - 3] = '\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, @@ -654,10 +688,18 @@ 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: stored sshProtoIdSz " + "\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 sshProtoIdStrSz " "was not retained\n", i, protoIds[i].name); failures++; @@ -665,6 +707,62 @@ static int test_DoProtoId(void) } } + /* 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); diff --git a/wolfssh/internal.h b/wolfssh/internal.h index 28d950dd..a0153980 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -588,6 +588,9 @@ enum NameIdType { #define LENGTH_SZ UINT32_SZ #define SSH_PROTO_SZ 8 /* "SSH-2.0-" */ #define SSH_PROTO_EOL_SZ 2 /* "\r\n" */ +/* Minimum size for a valid proto id * + * "SSH-2.0-" "\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 */ @@ -1999,6 +2002,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); diff --git a/wolfssh/ssh.h b/wolfssh/ssh.h index 24debc58..e31e9f37 100644 --- a/wolfssh/ssh.h +++ b/wolfssh/ssh.h @@ -667,6 +667,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