diff --git a/apps/wolfssh/common.c b/apps/wolfssh/common.c index 03b4ac633..790f7ca3e 100644 --- a/apps/wolfssh/common.c +++ b/apps/wolfssh/common.c @@ -31,6 +31,7 @@ #include #include #include +#include #include "apps/wolfssh/common.h" #ifndef USE_WINDOWS_API #include @@ -382,10 +383,8 @@ int ClientPublicKeyCheck(const byte* pubKey, word32 pubKeySz, void* ctx) printf("This key matches other servers:\n"); otherMatch = 1; } - if (otherMatch) { - printf("\t%s:%u: %s\n", - knownHostsName, lineCount, name); - } + printf("\t%s:%u: %s\n", + knownHostsName, lineCount, name); } } } @@ -789,9 +788,13 @@ void ClientFreeBuffers(void) } if (userPrivateKeyAlloc && userPrivateKey != NULL) { + wc_ForceZero(userPrivateKey, userPrivateKeySz); WFREE(userPrivateKey, NULL, DYNTYPE_PRIVKEY); userPrivateKey = userPrivateKeyBuf; - userPrivateKeySz = sizeof(userPrivateKeyBuf); userPrivateKeyAlloc = 0; } + + wc_ForceZero(userPrivateKeyBuf, sizeof(userPrivateKeyBuf)); + userPrivateKeySz = 0; + wc_ForceZero(userPassword, sizeof(userPassword)); } diff --git a/examples/client/common.c b/examples/client/common.c index 2a40b8e87..30c513832 100644 --- a/examples/client/common.c +++ b/examples/client/common.c @@ -33,6 +33,7 @@ #include #include #include +#include #ifdef WOLFSSH_TPM #include @@ -1123,18 +1124,41 @@ void ClientFreeBuffers(const char* pubKeyName, const char* privKeyName, if (pubKeyName != NULL && userPublicKey != NULL && userPublicKey != userPublicKeyBuf) { WFREE(userPublicKey, heap, DYNTYPE_PRIVKEY); + userPublicKey = userPublicKeyBuf; + userPublicKeySz = 0; } - if (privKeyName != NULL && userPrivateKey != NULL && - userPrivateKeyAlloc) { - WFREE(userPrivateKey, heap, DYNTYPE_PRIVKEY); + if (privKeyName != NULL && userPrivateKey != NULL) { + wc_ForceZero(userPrivateKey, userPrivateKeySz); + if (userPrivateKeyAlloc) { + WFREE(userPrivateKey, heap, DYNTYPE_PRIVKEY); + userPrivateKey = userPrivateKeyBuf; + userPrivateKeyAlloc = 0; + } } #ifdef WOLFSSH_KEYBOARD_INTERACTIVE - for (entry = 0; entry < keyboardResponseCount; entry++) { - WFREE(keyboardResponses[entry], NULL, 0); + if (keyboardResponses != NULL && keyboardResponseLengths != NULL) { + for (entry = 0; entry < keyboardResponseCount; entry++) { + if (keyboardResponses[entry] != NULL && + keyboardResponseLengths[entry] != 0) { + wc_ForceZero(keyboardResponses[entry], + keyboardResponseLengths[entry]); + } + WFREE(keyboardResponses[entry], NULL, 0); + } + } + if (keyboardResponses != NULL) { + WFREE(keyboardResponses, NULL, 0); + keyboardResponses = NULL; + } + if (keyboardResponseLengths != NULL) { + WFREE(keyboardResponseLengths, NULL, 0); + keyboardResponseLengths = NULL; } - WFREE(keyboardResponses, NULL, 0); - WFREE(keyboardResponseLengths, NULL, 0); + keyboardResponseCount = 0; #endif + wc_ForceZero(userPrivateKeyBuf, sizeof(userPrivateKeyBuf)); + userPrivateKeySz = 0; + wc_ForceZero(userPassword, sizeof(userPassword)); } diff --git a/src/certman.c b/src/certman.c index 89a6f8265..ee34f0b3f 100644 --- a/src/certman.c +++ b/src/certman.c @@ -216,7 +216,7 @@ int wolfSSH_CERTMAN_VerifyCerts_buffer(WOLFSSH_CERTMAN* cm, WLOG_ENTER(); - if (cm == NULL || certs == NULL) { + if (cm == NULL || certs == NULL || certsCount == 0) { return WS_BAD_ARGUMENT; } diff --git a/src/internal.c b/src/internal.c index eefde3360..4461f9afc 100644 --- a/src/internal.c +++ b/src/internal.c @@ -4457,7 +4457,7 @@ static int DoKexInit(WOLFSSH* ssh, byte* buf, word32 len, word32* idx) algoId = MatchIdLists(side, list, listSz, cannedList, cannedListSz); if (algoId == ID_UNKNOWN) { WLOG(WS_LOG_DEBUG, "Unable to negotiate Server Host Key Algo"); - return WS_MATCH_KEY_ALGO_E; + ret = WS_MATCH_KEY_ALGO_E; } else { ssh->handshake->pubKeyId = algoId; @@ -4621,12 +4621,14 @@ static int DoKexInit(WOLFSSH* ssh, byte* buf, word32 len, word32* idx) } } - /* Skip the "for future use" length. */ + /* RFC 4253 7.1 reserved field: fixed uint32 0, not a length prefix. */ if (ret == WS_SUCCESS) { - WLOG(WS_LOG_DEBUG, "DKI: For Future Use"); + WLOG(WS_LOG_DEBUG, "DKI: Reserved"); ret = GetUint32(&skipSz, buf, len, &begin); - if (ret == WS_SUCCESS) - begin += skipSz; + if (ret == WS_SUCCESS && skipSz != 0) { + WLOG(WS_LOG_DEBUG, "DKI: non-zero reserved field"); + ret = WS_PARSE_E; + } } if (ret == WS_SUCCESS) { @@ -6307,10 +6309,10 @@ static int DoNewKeys(WOLFSSH* ssh, byte* buf, word32 len, word32* idx) int ret = WS_SUCCESS; WOLFSSH_UNUSED(buf); - WOLFSSH_UNUSED(len); WOLFSSH_UNUSED(idx); - if (ssh == NULL || ssh->handshake == NULL) + /* RFC 4253 7.3: SSH_MSG_NEWKEYS has no payload. */ + if (ssh == NULL || ssh->handshake == NULL || len != 0) ret = WS_BAD_ARGUMENT; if (ret == WS_SUCCESS) { @@ -10874,7 +10876,7 @@ int DoProtoId(WOLFSSH* ssh) } if (ssh->inputBuffer.length >= 4 - && WSTRNCASECMP((char*)ssh->inputBuffer.buffer, "SSH-", 4) == 0) + && WSTRNCMP((char*)ssh->inputBuffer.buffer, "SSH-", 4) == 0) break; if (!allowBanner) { @@ -10903,8 +10905,8 @@ int DoProtoId(WOLFSSH* ssh) eolSz = (*eol == '\r') ? 2 : 1; if (ssh->inputBuffer.length >= SSH_PROTO_SZ - && WSTRNCASECMP((char*)ssh->inputBuffer.buffer, - ssh->ctx->sshProtoIdStr, SSH_PROTO_SZ) == 0) { + && WSTRNCMP((char*)ssh->inputBuffer.buffer, + ssh->ctx->sshProtoIdStr, SSH_PROTO_SZ) == 0) { if (ssh->ctx->side == WOLFSSH_ENDPOINT_SERVER) ssh->clientState = CLIENT_VERSION_DONE; @@ -18072,10 +18074,9 @@ int wolfSSH_TestDoKexInit(WOLFSSH* ssh, byte* buf, word32 len, word32* idx) return DoKexInit(ssh, buf, len, idx); } -int wolfSSH_TestDoNewKeys(WOLFSSH* ssh) +int wolfSSH_TestDoNewKeys(WOLFSSH* ssh, byte* buf, word32 len, word32* idx) { - /* DoNewKeys ignores buf/len/idx (marked WOLFSSH_UNUSED internally). */ - return DoNewKeys(ssh, NULL, 0, NULL); + return DoNewKeys(ssh, buf, len, idx); } void wolfSSH_TestFreeHandshake(WOLFSSH* ssh) diff --git a/src/wolfterm.c b/src/wolfterm.c index aaa682949..6b507b152 100644 --- a/src/wolfterm.c +++ b/src/wolfterm.c @@ -424,11 +424,21 @@ static int wolfSSH_DoControlSeq(WOLFSSH* ssh, WOLFSSH_HANDLE handle, byte* buf, if (ssh->escState == WS_ESC_CSI || ssh->escBufSz > 0) { /* check for left overs */ if (ssh->escBufSz > 0) { - word32 tmpSz = min(bufSz - *idx, - WOLFSSH_MAX_CONSOLE_ARGS - (word32)ssh->escBufSz); + word32 tmpSz; + + if (ssh->escBufSz >= WOLFSSL_MAX_ESCBUF) { + WLOG(WS_LOG_ERROR, "escBuf state exceeds capacity"); + return WS_FATAL_ERROR; + } + tmpSz = min(bufSz - *idx, + (word32)WOLFSSL_MAX_ESCBUF - (word32)ssh->escBufSz); WMEMCPY(ssh->escBuf + ssh->escBufSz, buf + *idx, tmpSz); i = 0; numArgs = getArgs(ssh->escBuf, ssh->escBufSz + tmpSz, &i, args); + if (i >= ssh->escBufSz + tmpSz) { + ssh->escBufSz += tmpSz; + return WS_WANT_READ; + } c = ssh->escBuf[i++]; if (!isCommand(c)) { /* expecting a command when in CSI state */ diff --git a/tests/api.c b/tests/api.c index e2f8ad5fb..0ef5ec61e 100644 --- a/tests/api.c +++ b/tests/api.c @@ -645,6 +645,8 @@ static void test_wolfSSH_CTX_UsePrivateKey_buffer_pem(void) static void test_wolfSSH_CertMan(void) { #ifdef WOLFSSH_CERTMAN + /* This chunk of test is checking the innards of the WOLFSSH_CERTMAN + * struct which has a private declaration at the moment. */ { WOLFSSH_CERTMAN* cm = NULL; @@ -668,7 +670,29 @@ static void test_wolfSSH_CertMan(void) AssertNotNull(cmRef->heap); AssertEQ(cmRef->heap, fakeHeap); } -#endif +#endif /* WOLFSSH_CERTMAN */ + +#ifdef WOLFSSH_CERTS + { + /* VerifyCerts_buffer must reject certsCount == 0; otherwise the + * inner loops short-circuit and the function returns WS_SUCCESS + * without verifying anything. */ + WOLFSSH_CERTMAN* cm; + unsigned char dummy[1] = { 0 }; + + cm = wolfSSH_CERTMAN_new(NULL); + AssertNotNull(cm); + + AssertIntEQ(WS_BAD_ARGUMENT, + wolfSSH_CERTMAN_VerifyCerts_buffer(cm, dummy, sizeof(dummy), 0)); + AssertIntEQ(WS_BAD_ARGUMENT, + wolfSSH_CERTMAN_VerifyCerts_buffer(NULL, dummy, sizeof(dummy), 1)); + AssertIntEQ(WS_BAD_ARGUMENT, + wolfSSH_CERTMAN_VerifyCerts_buffer(cm, NULL, 0, 1)); + + wolfSSH_CERTMAN_free(cm); + } +#endif /* WOLFSSH_CERTS */ } diff --git a/tests/regress.c b/tests/regress.c index ded08c989..b6b049244 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -2167,6 +2167,43 @@ static void TestFirstPacketFollows(void) TestFirstPacketFollowsSkipped(); } +/* RFC 4253 7.1: the trailing uint32 in KEXINIT is reserved and must be zero. + * DoKexInit used to advance begin by that value (treating it as a length); + * the current code rejects any non-zero value with WS_PARSE_E. Lock the + * strict-rejection branch in so a regression that re-relaxes the check or + * reverts to skipping skipSz bytes would fail this test. */ +static void TestKexInitReservedNonZeroRejected(void) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + byte payload[512]; + word32 payloadSz; + word32 idx = 0; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + AssertNotNull(ctx); + + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AssertIntEQ(wolfSSH_SetAlgoListKex(ssh, FPF_KEX_GOOD), WS_SUCCESS); + AssertIntEQ(wolfSSH_SetAlgoListKey(ssh, FPF_KEY_GOOD), WS_SUCCESS); + + payloadSz = BuildKexInitPayload(ssh, FPF_KEX_GOOD, FPF_KEY_GOOD, + 0, payload, (word32)sizeof(payload)); + + /* BuildKexInitPayload puts the reserved uint32 in the final 4 bytes. + * Overwrite them with a non-zero value to exercise the strict branch. */ + AssertTrue(payloadSz >= UINT32_SZ); + (void)AppendUint32(payload, (word32)sizeof(payload), + payloadSz - UINT32_SZ, 0xDEADBEEFu); + + AssertIntEQ(wolfSSH_TestDoKexInit(ssh, payload, payloadSz, &idx), + WS_PARSE_E); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + #if !defined(WOLFSSH_NO_AES_CBC) && !defined(WOLFSSH_NO_AES_CTR) \ && !defined(WOLFSSH_NO_HMAC_SHA1) && !defined(WOLFSSH_NO_HMAC_SHA2_256) static void TestIndependentAlgoNegotiation(void) @@ -2963,7 +3000,7 @@ static void TestDoNewKeys(void) /* Peer has sent NewKeys; self has already sent its own (not keying). */ ssh->isKeying = WOLFSSH_PEER_IS_KEYING; - AssertIntEQ(wolfSSH_TestDoNewKeys(ssh), WS_SUCCESS); + AssertIntEQ(wolfSSH_TestDoNewKeys(ssh, NULL, 0, NULL), WS_SUCCESS); /* handshake freed by DoNewKeys. */ AssertTrue(ssh->handshake == NULL); @@ -3018,7 +3055,7 @@ static void TestDoNewKeys(void) WMEMCPY(&savedPeerKeys, &ssh->handshake->peerKeys, sizeof(Keys)); ssh->isKeying = WOLFSSH_PEER_IS_KEYING; - AssertIntEQ(wolfSSH_TestDoNewKeys(ssh), WS_SUCCESS); + AssertIntEQ(wolfSSH_TestDoNewKeys(ssh, NULL, 0, NULL), WS_SUCCESS); AssertTrue(ssh->handshake == NULL); @@ -3072,7 +3109,7 @@ static void TestDoNewKeys(void) WMEMCPY(&savedPeerKeys, &ssh->handshake->peerKeys, sizeof(Keys)); ssh->isKeying = WOLFSSH_PEER_IS_KEYING; - AssertIntEQ(wolfSSH_TestDoNewKeys(ssh), WS_SUCCESS); + AssertIntEQ(wolfSSH_TestDoNewKeys(ssh, NULL, 0, NULL), WS_SUCCESS); AssertTrue(ssh->handshake == NULL); @@ -3126,7 +3163,11 @@ static void TestDoNewKeys(void) WMEMCPY(&savedPeerKeys, &ssh->handshake->peerKeys, sizeof(Keys)); ssh->isKeying = WOLFSSH_PEER_IS_KEYING; - AssertIntEQ(wolfSSH_TestDoNewKeys(ssh), WS_SUCCESS); + /* Exercise the len != 0 rejection while handshake is still allocated, + * so the guard is reached and not short-circuited by handshake == NULL. */ + AssertIntEQ(wolfSSH_TestDoNewKeys(ssh, NULL, 1, NULL), WS_BAD_ARGUMENT); + AssertNotNull(ssh->handshake); + AssertIntEQ(wolfSSH_TestDoNewKeys(ssh, NULL, 0, NULL), WS_SUCCESS); AssertTrue(ssh->handshake == NULL); @@ -3190,6 +3231,7 @@ int main(int argc, char** argv) && !defined(WOLFSSH_NO_CURVE25519_SHA256) \ && !defined(WOLFSSH_NO_RSA_SHA2_256) TestFirstPacketFollows(); + TestKexInitReservedNonZeroRejected(); #endif #if !defined(WOLFSSH_NO_ECDH_SHA2_NISTP256) && !defined(WOLFSSH_NO_RSA) \ && !defined(WOLFSSH_NO_CURVE25519_SHA256) \ diff --git a/tests/unit.c b/tests/unit.c index 21d3c332a..b037c4c6c 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -87,13 +87,21 @@ static const ProtoIdTestVector protoIdTestVectors[] = { "SSH-2.0-wolfSSHv" LIBWOLFSSH_VERSION_STRING "\n", 0, WS_SUCCESS, WOLFSSH_ENDPOINT_CLIENT }, - /* Case-insensitive match. DoProtoId uses WSTRNCASECMP. */ + /* Case rejection. DoProtoId uses WSTRNCMP. */ { "lowercase ssh prefix", "ssh-2.0-wolfSSHv" LIBWOLFSSH_VERSION_STRING "\r\n", - 0, WS_SUCCESS, WOLFSSH_ENDPOINT_CLIENT }, + 0, WS_SOCKET_ERROR_E, WOLFSSH_ENDPOINT_CLIENT }, { "mixed case SSH prefix", "Ssh-2.0-wolfSSHv" LIBWOLFSSH_VERSION_STRING "\r\n", - 0, WS_SUCCESS, WOLFSSH_ENDPOINT_CLIENT }, + 0, WS_SOCKET_ERROR_E, WOLFSSH_ENDPOINT_CLIENT }, + + /* Case rejection. DoProtoId uses WSTRNCMP. */ + { "lowercase ssh prefix", + "ssh-2.0-wolfSSHv" LIBWOLFSSH_VERSION_STRING "\r\n", + 0, WS_VERSION_E, WOLFSSH_ENDPOINT_SERVER }, + { "mixed case SSH prefix", + "Ssh-2.0-wolfSSHv" LIBWOLFSSH_VERSION_STRING "\r\n", + 0, WS_VERSION_E, WOLFSSH_ENDPOINT_SERVER }, /* OpenSSH peer identification. */ { "OpenSSH version string", diff --git a/wolfssh/internal.h b/wolfssh/internal.h index 1909d96d5..97238f478 100644 --- a/wolfssh/internal.h +++ b/wolfssh/internal.h @@ -1358,8 +1358,9 @@ enum WS_MessageIdLimits { word32 len, word32* idx); WOLFSSH_API int wolfSSH_TestDoKexInit(WOLFSSH* ssh, byte* buf, word32 len, word32* idx); + WOLFSSH_API int wolfSSH_TestDoNewKeys(WOLFSSH* ssh, byte* buf, + word32 len, word32* idx); WOLFSSH_API int wolfSSH_TestGenerateKeys(WOLFSSH* ssh, byte hashId); - WOLFSSH_API int wolfSSH_TestDoNewKeys(WOLFSSH* ssh); WOLFSSH_API void wolfSSH_TestFreeHandshake(WOLFSSH* ssh); WOLFSSH_API int wolfSSH_TestDoKexDhInit(WOLFSSH* ssh, byte* buf, word32 len, word32* idx);