diff --git a/src/internal.c b/src/internal.c index b2153e32..7b02857f 100644 --- a/src/internal.c +++ b/src/internal.c @@ -1394,6 +1394,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; @@ -6877,9 +6878,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); } @@ -14626,21 +14626,38 @@ 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); + word32 i; + + if (protoIdStr == NULL || + len < SSH_PROTO_SZ + 1 + SSH_PROTO_EOL_SZ || + 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); 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-\""); + 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]; + + 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; } @@ -14655,7 +14672,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); } diff --git a/src/ssh.c b/src/ssh.c index 83f41d76..57089ec9 100644 --- a/src/ssh.c +++ b/src/ssh.c @@ -3505,19 +3505,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 f67c8408..f77fdbed 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -609,20 +609,40 @@ static int test_DoProtoId(void) /* 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 }, - { "wrong version prefix", "SSH-2-this_is_my_app\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 }, + { "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\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 }, }; 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'; + WMEMSET(justRightProtoId, 'a', sizeof(justRightProtoId)); + WMEMCPY(justRightProtoId, "SSH-2.0-", sizeof("SSH-2.0-") - 1); + justRightProtoId[255] = '\0'; + justRightProtoId[254] = '\n'; + justRightProtoId[253] = '\r'; for (i = 0; i < pc; i++) { ret = wolfSSH_CTX_SetSshProtoIdStr(clientCtx, protoIds[i].id); @@ -634,6 +654,14 @@ static int test_DoProtoId(void) : "WS_BAD_ARGUMENT"); failures++; } + if ((ret == WS_SUCCESS) && clientCtx->sshProtoIdStrSz != + WSTRLEN(protoIds[i].id)) { + fprintf(stderr, + "\t[protoId %d] \"%s\" FAIL: stored sshProtoIdSz " + "was not retained\n", + i, protoIds[i].name); + failures++; + } } } diff --git a/wolfssh/internal.h b/wolfssh/internal.h index be5905af..28d950dd 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -587,6 +587,7 @@ 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" */ #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 */ @@ -893,6 +894,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 */ diff --git a/wolfssh/ssh.h b/wolfssh/ssh.h index f768fe9e..24debc58 100644 --- a/wolfssh/ssh.h +++ b/wolfssh/ssh.h @@ -665,11 +665,13 @@ 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' * 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