diff --git a/src/internal.c b/src/internal.c index 81e41ff9a..63a3c89e6 100644 --- a/src/internal.c +++ b/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,50 @@ 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; + + /* 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_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_MIN, 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; + } + + /* 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); + return WS_BAD_ARGUMENT; + } + } + return WS_SUCCESS; } @@ -14214,7 +14243,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); } @@ -24251,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); diff --git a/src/ssh.c b/src/ssh.c index ae7d0b2a0..b18ab041a 100644 --- a/src/ssh.c +++ b/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; } diff --git a/tests/unit.c b/tests/unit.c index 1007afcae..2278ec74c 100644 --- a/tests/unit.c +++ b/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,6 +581,9 @@ 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 const struct { @@ -564,17 +591,44 @@ static int test_DoProtoId(void) 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 }, + { "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[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[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, @@ -584,8 +638,80 @@ static int test_DoProtoId(void) : "WS_BAD_ARGUMENT"); failures++; } + 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 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); diff --git a/wolfssh/internal.h b/wolfssh/internal.h index 588e0c867..58b58f3df 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -576,6 +576,10 @@ 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" */ +/* 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 */ @@ -862,6 +866,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 */ @@ -1936,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); diff --git a/wolfssh/ssh.h b/wolfssh/ssh.h index 545ef9e3c..c47a3aca8 100644 --- a/wolfssh/ssh.h +++ b/wolfssh/ssh.h @@ -656,11 +656,17 @@ 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' + * 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 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