From 54b08ff3fc836de28d2ff50c53f2913fcef0903b Mon Sep 17 00:00:00 2001 From: Aidan Keefe Date: Mon, 31 Aug 2026 17:01:14 -0600 Subject: [PATCH] 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 --- src/internal.c | 22 +++++++++++++++++++ src/ssh.c | 8 +++++++ tests/unit.c | 53 +++++++++++++++++++--------------------------- wolfssh/internal.h | 3 +++ wolfssh/ssh.h | 11 ++++++++++ 5 files changed, 66 insertions(+), 31 deletions(-) diff --git a/src/internal.c b/src/internal.c index 5b385b1c8..6d09b1028 100644 --- a/src/internal.c +++ b/src/internal.c @@ -13196,6 +13196,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; diff --git a/src/ssh.c b/src/ssh.c index 9388c8d86..6797bf9ac 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -3447,10 +3447,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; } diff --git a/tests/unit.c b/tests/unit.c index 22c07a602..df1b423ae 100644 --- a/tests/unit.c +++ b/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); - } } } diff --git a/wolfssh/internal.h b/wolfssh/internal.h index 2627602e0..4330cc470 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -708,6 +708,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. @@ -1547,6 +1549,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); diff --git a/wolfssh/ssh.h b/wolfssh/ssh.h index 110f96abd..c71386d28 100644 --- a/wolfssh/ssh.h +++ b/wolfssh/ssh.h @@ -588,6 +588,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