Tighten MAC guards, boolean decode and KEX guess

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;

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,29 +14718,26 @@ 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;
     byte payload[8];
     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);
     AssertNotNull(ssh);
     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)

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"));