diff --git a/src/internal.c b/src/internal.c index 3416a7871..fe5ca5598 100644 --- a/src/internal.c +++ b/src/internal.c @@ -5650,7 +5650,7 @@ int GetBoolean(byte* v, const byte* buf, word32 len, word32* idx) int result = WS_BUFFER_E; if (*idx < len) { - *v = buf[*idx]; + *v = (buf[*idx] != 0); *idx += BOOLEAN_SZ; result = WS_SUCCESS; } @@ -6832,7 +6832,13 @@ static int DoKexInit(WOLFSSH* ssh, byte* buf, word32 len, word32* idx) if (ret == WS_SUCCESS) { WLOG(WS_LOG_DEBUG, " packet follows: %s", kexPacketFollows ? "yes" : "no"); - if (kexPacketFollows + /* Every supported KEX starts with a client message, so only a + * client can send a guessed packet. Ignore the flag from a + * server. */ + if (kexPacketFollows && side != WOLFSSH_ENDPOINT_SERVER) { + WLOG(WS_LOG_DEBUG, " ignoring server's packet follows flag"); + } + else if (kexPacketFollows && (kexIdGuess != ssh->handshake->kexId || pubKeyIdGuess != ssh->handshake->pubKeyId)) { ssh->handshake->ignoreNextKexMsg = 1; @@ -8540,12 +8546,6 @@ static int DoKexDhReply(WOLFSSH* ssh, byte* buf, word32 len, word32* idx) return ret; } - if (ret == WS_SUCCESS) { - if (SkipGuessedKexMsg(ssh, "server's KEXDH_REPLY message", - len, idx)) - return WS_SUCCESS; - } - if (ret == WS_SUCCESS && len < LENGTH_SZ*2 + *idx) { ret = WS_BUFFER_E; } @@ -9269,14 +9269,6 @@ static int DoKexDhGexGroup(WOLFSSH* ssh, ret = WS_BAD_ARGUMENT; if (ret == WS_SUCCESS) { - /* A conformant server sends GROUP only in response to the client's - * REQUEST, so it should never set first_packet_follows here. Discard - * the message defensively if a peer sets it anyway, mirroring the other - * Do* handlers. */ - if (SkipGuessedKexMsg(ssh, "server's KEXDH_GEX_GROUP message", - len, idx)) - return WS_SUCCESS; - begin = *idx; ret = GetMpint(&primeGroupSz, &primeGroup, buf, len, &begin); if (ret == WS_SUCCESS && primeGroupSz > (MAX_KEX_KEY_SZ + 1)) { @@ -14386,6 +14378,7 @@ static INLINE int CreateMac(WOLFSSH* ssh, const byte* in, word32 inSz, break; #endif +#ifndef WOLFSSH_NO_HMAC_SHA2_256 case ID_HMAC_SHA2_256: { Hmac hmac; @@ -14404,6 +14397,7 @@ static INLINE int CreateMac(WOLFSSH* ssh, const byte* in, word32 inSz, wc_HmacFree(&hmac); } break; +#endif #ifndef WOLFSSH_NO_HMAC_SHA2_512 case ID_HMAC_SHA2_512: @@ -14461,8 +14455,13 @@ static INLINE int VerifyMac(WOLFSSH* ssh, const byte* in, word32 inSz, ret = WS_SUCCESS; break; +#ifndef WOLFSSH_NO_HMAC_SHA1 case ID_HMAC_SHA1: +#endif +#ifndef WOLFSSH_NO_HMAC_SHA1_96 case ID_HMAC_SHA1_96: +#endif +#if !defined(WOLFSSH_NO_HMAC_SHA1) || !defined(WOLFSSH_NO_HMAC_SHA1_96) ret = wc_HmacSetKey(&hmac, WC_SHA, ssh->peerKeys.macKey, ssh->peerKeys.macKeySz); if (ret == WS_SUCCESS) @@ -14474,7 +14473,9 @@ static INLINE int VerifyMac(WOLFSSH* ssh, const byte* in, word32 inSz, if (ret == WS_SUCCESS && ConstantCompare(checkMac, mac, ssh->peerMacSz) != 0) ret = WS_VERIFY_MAC_E; break; +#endif +#ifndef WOLFSSH_NO_HMAC_SHA2_256 case ID_HMAC_SHA2_256: ret = wc_HmacSetKey(&hmac, WC_SHA256, ssh->peerKeys.macKey, ssh->peerKeys.macKeySz); @@ -14487,7 +14488,9 @@ static INLINE int VerifyMac(WOLFSSH* ssh, const byte* in, word32 inSz, if (ret == WS_SUCCESS && ConstantCompare(checkMac, mac, ssh->peerMacSz) != 0) ret = WS_VERIFY_MAC_E; break; +#endif +#ifndef WOLFSSH_NO_HMAC_SHA2_512 case ID_HMAC_SHA2_512: ret = wc_HmacSetKey(&hmac, WC_SHA512, ssh->peerKeys.macKey, ssh->peerKeys.macKeySz); @@ -14500,6 +14503,7 @@ static INLINE int VerifyMac(WOLFSSH* ssh, const byte* in, word32 inSz, if (ret == WS_SUCCESS && ConstantCompare(checkMac, mac, ssh->peerMacSz) != 0) ret = WS_VERIFY_MAC_E; break; +#endif default: ret = WS_INVALID_ALGO_ID; diff --git a/tests/regress.c b/tests/regress.c index 991d03052..6b32f318d 100644 --- a/tests/regress.c +++ b/tests/regress.c @@ -14649,19 +14649,28 @@ typedef struct { const char* keyList; byte firstPacketFollows; byte expectIgnore; + byte endpointType; } FirstPacketFollowsCase; static const FirstPacketFollowsCase firstPacketFollowsCases[] = { { "follows=0, guesses irrelevant: flag stays off", - FPF_KEX_BAD "," FPF_KEX_GOOD, FPF_KEY_BAD "," FPF_KEY_GOOD, 0, 0 }, + FPF_KEX_BAD "," FPF_KEX_GOOD, FPF_KEY_BAD "," FPF_KEY_GOOD, 0, 0, + WOLFSSH_ENDPOINT_SERVER }, { "follows=1, both guesses match: do not skip", - FPF_KEX_GOOD, FPF_KEY_GOOD, 1, 0 }, + FPF_KEX_GOOD, FPF_KEY_GOOD, 1, 0, WOLFSSH_ENDPOINT_SERVER }, { "follows=1, KEX guess wrong: skip", - FPF_KEX_BAD "," FPF_KEX_GOOD, FPF_KEY_GOOD, 1, 1 }, + FPF_KEX_BAD "," FPF_KEX_GOOD, FPF_KEY_GOOD, 1, 1, + WOLFSSH_ENDPOINT_SERVER }, { "follows=1, host-key guess wrong: skip", /* regression case */ - FPF_KEX_GOOD, FPF_KEY_BAD "," FPF_KEY_GOOD, 1, 1 }, + FPF_KEX_GOOD, FPF_KEY_BAD "," FPF_KEY_GOOD, 1, 1, + WOLFSSH_ENDPOINT_SERVER }, { "follows=1, both guesses wrong: skip", - FPF_KEX_BAD "," FPF_KEX_GOOD, FPF_KEY_BAD "," FPF_KEY_GOOD, 1, 1 }, + FPF_KEX_BAD "," FPF_KEX_GOOD, FPF_KEY_BAD "," FPF_KEY_GOOD, 1, 1, + WOLFSSH_ENDPOINT_SERVER }, + /* A server never sends a guessed packet; the client ignores the flag. */ + { "client, server follows=1, both guesses wrong: do not skip", + FPF_KEX_BAD "," FPF_KEX_GOOD, FPF_KEY_BAD "," FPF_KEY_GOOD, 1, 0, + WOLFSSH_ENDPOINT_CLIENT }, }; static void RunFirstPacketFollowsCase(const FirstPacketFollowsCase* tc) @@ -14672,7 +14681,7 @@ static void RunFirstPacketFollowsCase(const FirstPacketFollowsCase* tc) word32 payloadSz; word32 idx = 0; - ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); + ctx = wolfSSH_CTX_new(tc->endpointType, NULL); AssertNotNull(ctx); ssh = wolfSSH_new(ctx); @@ -14691,6 +14700,11 @@ static void RunFirstPacketFollowsCase(const FirstPacketFollowsCase* tc) (void)wolfSSH_TestDoKexInit(ssh, payload, payloadSz, &idx); AssertNotNull(ssh->handshake); + /* The algorithm ids are set only after first_packet_follows is read. */ + if (ssh->handshake->peerEncryptId == ID_NONE) { + Fail(("DoKexInit parses past first_packet_follows (%s)", + tc->description), ("peerEncryptId == ID_NONE")); + } if (ssh->handshake->ignoreNextKexMsg != tc->expectIgnore) { Fail(("ignoreNextKexMsg == %u (%s)", tc->expectIgnore, tc->description), @@ -14704,10 +14718,10 @@ static void RunFirstPacketFollowsCase(const FirstPacketFollowsCase* tc) typedef int (*FirstPacketFollowsSkipFn)(WOLFSSH* ssh, byte* buf, word32 len, word32* idx); -/* With ignoreNextKexMsg set, the target Do* handler must consume the packet, - * clear the flag, and not advance the peer's state past KEXINIT_DONE. */ +/* With ignoreNextKexMsg set, the server's Do* handler must consume the packet, + * clear the flag, and not advance the client's state past KEXINIT_DONE. */ static void RunFirstPacketFollowsSkipCase(FirstPacketFollowsSkipFn fn, - const char* label, byte endpointType, byte initState) + const char* label) { WOLFSSH_CTX* ctx; WOLFSSH* ssh; @@ -14715,7 +14729,7 @@ static void RunFirstPacketFollowsSkipCase(FirstPacketFollowsSkipFn fn, word32 idx = 0; int ret; - ctx = wolfSSH_CTX_new(endpointType, NULL); + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_SERVER, NULL); AssertNotNull(ctx); ssh = wolfSSH_new(ctx); @@ -14723,10 +14737,7 @@ static void RunFirstPacketFollowsSkipCase(FirstPacketFollowsSkipFn fn, AssertNotNull(ssh->handshake); ssh->handshake->ignoreNextKexMsg = 1; - if (endpointType == WOLFSSH_ENDPOINT_SERVER) - ssh->clientState = initState; - else - ssh->serverState = initState; + ssh->clientState = CLIENT_KEXINIT_DONE; /* Garbage payload that must never be parsed when skipped. */ WMEMSET(payload, 0xAB, sizeof(payload)); @@ -14737,10 +14748,38 @@ static void RunFirstPacketFollowsSkipCase(FirstPacketFollowsSkipFn fn, } AssertIntEQ(idx, sizeof(payload)); AssertIntEQ(ssh->handshake->ignoreNextKexMsg, 0); - if (endpointType == WOLFSSH_ENDPOINT_SERVER) - AssertIntEQ(ssh->clientState, initState); - else - AssertIntEQ(ssh->serverState, initState); + AssertIntEQ(ssh->clientState, CLIENT_KEXINIT_DONE); + + wolfSSH_free(ssh); + wolfSSH_CTX_free(ctx); +} + +/* The client never discards a server KEX message as a guess; even with the + * flag forced on, the handler must parse the packet and reject the garbage. */ +static void RunFirstPacketFollowsClientNoSkipCase(FirstPacketFollowsSkipFn fn, + const char* label) +{ + WOLFSSH_CTX* ctx; + WOLFSSH* ssh; + byte payload[8]; + word32 idx = 0; + int ret; + + ctx = wolfSSH_CTX_new(WOLFSSH_ENDPOINT_CLIENT, NULL); + AssertNotNull(ctx); + ssh = wolfSSH_new(ctx); + AssertNotNull(ssh); + AssertNotNull(ssh->handshake); + + ssh->handshake->ignoreNextKexMsg = 1; + ssh->serverState = SERVER_KEXINIT_DONE; + WMEMSET(payload, 0xAB, sizeof(payload)); + + ret = fn(ssh, payload, sizeof(payload), &idx); + if (ret == WS_SUCCESS) + Fail(("%s rejects the packet on a client", label), ("%d", ret)); + AssertIntEQ(idx, 0); + AssertIntEQ(ssh->handshake->ignoreNextKexMsg, 1); wolfSSH_free(ssh); wolfSSH_CTX_free(ctx); @@ -14791,13 +14830,10 @@ static void RunFirstPacketFollowsCrossBoundaryCase(FirstPacketFollowsSkipFn fn, static void TestFirstPacketFollowsSkipped(void) { - RunFirstPacketFollowsSkipCase(wolfSSH_TestDoKexDhInit, - "DoKexDhInit", WOLFSSH_ENDPOINT_SERVER, CLIENT_KEXINIT_DONE); + RunFirstPacketFollowsSkipCase(wolfSSH_TestDoKexDhInit, "DoKexDhInit"); #ifndef WOLFSSH_NO_DH_GEX_SHA256 RunFirstPacketFollowsSkipCase(wolfSSH_TestDoKexDhGexRequest, - "DoKexDhGexRequest", WOLFSSH_ENDPOINT_SERVER, CLIENT_KEXINIT_DONE); - RunFirstPacketFollowsSkipCase(wolfSSH_TestDoKexDhGexGroup, - "DoKexDhGexGroup", WOLFSSH_ENDPOINT_CLIENT, SERVER_KEXINIT_DONE); + "DoKexDhGexRequest"); /* Guess/negotiation straddling the GEX boundary, both directions. */ #ifndef NO_WOLFSSH_SERVER RunFirstPacketFollowsCrossBoundaryCase(wolfSSH_TestDoKexDhInit, @@ -14806,8 +14842,12 @@ static void TestFirstPacketFollowsSkipped(void) "DoKexDhGexRequest->KEXDH_INIT", MSGID_KEXDH_INIT); #endif /* NO_WOLFSSH_SERVER */ #endif /* WOLFSSH_NO_DH_GEX_SHA256 */ - RunFirstPacketFollowsSkipCase(wolfSSH_TestDoKexDhReply, - "DoKexDhReply", WOLFSSH_ENDPOINT_CLIENT, SERVER_KEXINIT_DONE); + RunFirstPacketFollowsClientNoSkipCase(wolfSSH_TestDoKexDhReply, + "DoKexDhReply"); +#ifndef WOLFSSH_NO_DH_GEX_SHA256 + RunFirstPacketFollowsClientNoSkipCase(wolfSSH_TestDoKexDhGexGroup, + "DoKexDhGexGroup"); +#endif } static void TestFirstPacketFollows(void) diff --git a/tests/unit.c b/tests/unit.c index e98000624..8c097c4c5 100644 --- a/tests/unit.c +++ b/tests/unit.c @@ -4772,6 +4772,35 @@ static int test_GetString_zeroDestSz(void) return result; } +/* GetBoolean() stores any nonzero wire byte as 1 (RFC 4251 5). */ +static int test_GetBoolean(void) +{ + static const byte wire[] = { 0x00, 0x01, 0x02, 0x80, 0xFF }; + static const byte expect[] = { 0, 1, 1, 1, 1 }; + word32 idx = 0; + word32 i; + byte v; + + for (i = 0; i < (word32)sizeof(wire); i++) { + v = 0xAA; + if (GetBoolean(&v, wire, (word32)sizeof(wire), &idx) != WS_SUCCESS) + return -1; + if (v != expect[i]) + return -2; + if (idx != i + 1) + return -3; + } + + /* past the end */ + v = 0xAA; + if (GetBoolean(&v, wire, (word32)sizeof(wire), &idx) != WS_BUFFER_E) + return -4; + if (v != 0xAA || idx != (word32)sizeof(wire)) + return -5; + + return 0; +} + /* DoChannelWindowAdjust adds the peer's advertised bytes to peerWindowSz. * A crafted bytesToAdd that would wrap the word32 must be rejected with * WS_OVERFLOW_E and leave the window untouched; a value that fits must be @@ -22823,6 +22852,10 @@ int wolfSSH_UnitTest(int argc, char** argv) (unitResult == 0 ? "SUCCESS" : "FAILED")); testResult = testResult || unitResult; + unitResult = test_GetBoolean(); + printf("GetBoolean: %s\n", (unitResult == 0 ? "SUCCESS" : "FAILED")); + testResult = testResult || unitResult; + unitResult = test_GetString_zeroDestSz(); printf("GetString_zeroDestSz: %s\n", (unitResult == 0 ? "SUCCESS" : "FAILED"));