internal: skip guessed KEX packets on server only

Every supported KEX method opens with a client message, so only the
client can send a guessed packet (RFC 4253 section 7.1). DoKexInit()
sets ignoreNextKexMsg only on the server and logs a server's flag on
the client; DoKexDhReply() and DoKexDhGexGroup() no longer skip.

- tests: client KEXINIT with a wrong-guess follows flag does not skip
- tests: client KEXDH_REPLY and GEX_GROUP parse even with the flag set
- tests: the skip case helper drives only the server

Issue: F-10585

Author: ejohnstown

src/internal.c

@@ -6950,7 +6950,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;
@@ -8675,12 +8681,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;
     }
@@ -9402,14 +9402,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)) {

tests/regress.c

@@ -16493,19 +16493,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)
@@ -16516,7 +16525,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);
@@ -16535,6 +16544,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),
@@ -16548,29 +16562,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));
@@ -16581,10 +16592,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);
@@ -16637,13 +16676,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,
@@ -16652,8 +16688,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)