Client Out Of Order Messaging Checking

1. Add macro for logging an expected message.
2. Add an expected message ID to the HandshakeInfo.
3. Add a message ID for "none (0)".
4. Add a check in IsMessageAllowedClient() for the expected message ID.
   Clear it if successful.
5. The KEXDH messages sent to the server have expected responses. Set
   them if sending the message is successful.
6. Add the set of message ID ranges and macros for testing if a message
   ID is in a specific range.
7. Add flags for having sent the kexinit message and received it. Tweak
   the checks for isKeying and these flags.
8. IsMessageAllowedClient() to check for appropriate messages at the
   appropriate time during the connect.

Author: ejohnstown

src/internal.c

@@ -612,8 +612,8 @@ INLINE static int IsMessageAllowedKeying(WOLFSSH *ssh, byte msg)
         return 0;
     }
 
-    /* case of resending SSH_MSG_KEXINIT */
-    if (msg == MSGID_KEXINIT) {
+    /* case of peer resending SSH_MSG_KEXINIT */
+    if ((ssh->isKeying & WOLFSSH_PEER_IS_KEYING) && msg == MSGID_KEXINIT) {
         WLOG(WS_LOG_DEBUG, "Message ID %u not allowed by during rekeying", msg);
         ssh->error = WS_REKEYING;
         return 0;
@@ -632,34 +632,50 @@ INLINE static int IsMessageAllowedKeying(WOLFSSH *ssh, byte msg)
 #ifndef NO_WOLFSSH_SERVER
 INLINE static int IsMessageAllowedServer(WOLFSSH *ssh, byte msg)
 {
+    /* Only the server should send these messages, never receive. */
+    if (msg == MSGID_SERVICE_ACCEPT) {
+        WLOG(WS_LOG_DEBUG, "Message ID %u not allowed by %s %s",
+                msg, "client", "ever");
+        return 0;
+    }
+
+    /* Transport Layer Generic messages are always allowed. */
+    if (MSGIDLIMIT_TRANS_GEN(msg)) {
+        return 1;
+    }
+
     /* Has client userauth started? */
+    /* Allows the server to receive up to KEXDH GEX Request during KEX. */
     if (ssh->acceptState < ACCEPT_KEYED) {
-        if (msg > MSGID_KEXDH_LIMIT) {
+        if (msg > MSGID_KEXDH_GEX_REQUEST) {
             return 0;
         }
     }
     /* Is server userauth complete? */
     if (ssh->acceptState < ACCEPT_SERVER_USERAUTH_SENT) {
+        /* The server should only receive the user auth request message,
+         * it should not accept the other user auth messages, it sends
+         * them. (>50) */
         /* Explicitly check for messages not allowed before user
          * authentication has comleted. */
-        if (msg >= MSGID_USERAUTH_LIMIT) {
-            WLOG(WS_LOG_DEBUG, "Message ID %u not allowed by server "
-                    "before user authentication is complete", msg);
+        if (MSGIDLIMIT_POST_USERAUTH(msg)) {
+            WLOG(WS_LOG_DEBUG, "Message ID %u not allowed by %s %s",
+                    msg, "server", "before user authentication is complete");
             return 0;
         }
         /* Explicitly check for the user authentication messages that
          * only the server sends, it shouldn't receive them. */
-        if ((msg > MSGID_USERAUTH_RESTRICT) &&
+        if ((msg > MSGID_USERAUTH_REQUEST) &&
             (msg != MSGID_USERAUTH_INFO_RESPONSE)) {
-            WLOG(WS_LOG_DEBUG, "Message ID %u not allowed by server "
-                    "during user authentication", msg);
+            WLOG(WS_LOG_DEBUG, "Message ID %u not allowed by %s %s",
+                    msg, "server", "during user authentication");
             return 0;
         }
     }
     else {
-        if (msg >= MSGID_USERAUTH_RESTRICT && msg < MSGID_USERAUTH_LIMIT) {
-            WLOG(WS_LOG_DEBUG, "Message ID %u not allowed by server "
-                    "after user authentication", msg);
+        if (msg >= MSGID_USERAUTH_REQUEST && msg < MSGID_GLOBAL_REQUEST) {
+            WLOG(WS_LOG_DEBUG, "Message ID %u not allowed by %s %s",
+                    msg, "server", "after user authentication");
             return 0;
         }
     }
@@ -672,37 +688,95 @@ INLINE static int IsMessageAllowedServer(WOLFSSH *ssh, byte msg)
 #ifndef NO_WOLFSSH_CLIENT
 INLINE static int IsMessageAllowedClient(WOLFSSH *ssh, byte msg)
 {
-    /* Has client userauth started? */
-    if (ssh->connectState < CONNECT_CLIENT_KEXDH_INIT_SENT) {
-        if (msg >= MSGID_KEXDH_LIMIT) {
+    /* Only the client should send these messages, never receive. */
+    if (msg == MSGID_SERVICE_REQUEST || msg == MSGID_USERAUTH_REQUEST) {
+        WLOG(WS_LOG_DEBUG, "Message ID %u not allowed by %s %s",
+                msg, "client", "ever");
+        return 0;
+    }
+
+    if (msg == MSGID_SERVICE_ACCEPT) {
+        if (ssh->connectState == CONNECT_CLIENT_USERAUTH_REQUEST_SENT) {
+            return 1;
+        }
+        else {
+            WLOG(WS_LOG_DEBUG, "Message ID %u not allowed by %s %s",
+                    msg, "client", "after starting user auth");
             return 0;
         }
     }
-    /* Is client userauth complete? */
-    if (ssh->connectState < CONNECT_SERVER_USERAUTH_ACCEPT_DONE) {
-        /* Explicitly check for messages not allowed before user
-         * authentication has comleted. */
-        if (msg >= MSGID_USERAUTH_LIMIT) {
-            WLOG(WS_LOG_DEBUG, "Message ID %u not allowed by client "
-                    "before user authentication is complete", msg);
+
+    /* Transport Layer Generic messages are always allowed. */
+    if (MSGIDLIMIT_TRANS_GEN(msg)) {
+        return 1;
+    }
+
+    /* Is KEX complete? */
+    if (MSGIDLIMIT_TRANS(msg)) {
+        if (ssh->isKeying & WOLFSSH_PEER_IS_KEYING) {
+            /* MSGID_KEXINIT not allowed when keying. */
+            if (msg == MSGID_KEXINIT) {
+                WLOG(WS_LOG_DEBUG, "Message ID %u not allowed by %s %s",
+                        msg, "client", "when keying");
+                return 0;
+            }
+
+            /* Error if expecting a specific message and didn't receive. */
+            if (ssh->handshake && ssh->handshake->expectMsgId != MSGID_NONE) {
+                if (msg != ssh->handshake->expectMsgId) {
+                    WLOG(WS_LOG_DEBUG,
+                            "Message ID %u not the expected message %u",
+                            msg, ssh->handshake->expectMsgId);
+                    return 0;
+                }
+                else {
+                    /* Got the expected message, clear expectation. */
+                    ssh->handshake->expectMsgId = MSGID_NONE;
+                    return 1;
+                }
+            }
+        }
+        else {
+            /* MSGID_KEXINIT only allowed when not keying. */
+            if (msg == MSGID_KEXINIT) {
+                return 1;
+            }
+
+            /* All other transport KEX and ALGO messages are not allowed
+             * when not keying. */
+            WLOG(WS_LOG_DEBUG, "Message ID %u not allowed by %s %s",
+                    msg, "client", "when not keying");
             return 0;
         }
-        /* Explicitly check for the user authentication message that
-         * only the client sends, it shouldn't receive it. */
-        if (msg == MSGID_USERAUTH_RESTRICT) {
-            WLOG(WS_LOG_DEBUG, "Message ID %u not allowed by client "
-                    "during user authentication", msg);
+    }
+
+    /* Is client userauth complete? */
+    if (ssh->connectState >= CONNECT_KEYED
+            && ssh->connectState < CONNECT_SERVER_USERAUTH_ACCEPT_DONE) {
+        /* The endpoints should not allow message IDs greater than or
+         * equal to msgid 80 before user authentication is complete.
+         * Per RFC 4252 section 6. */
+        if (MSGIDLIMIT_POST_USERAUTH(msg)) {
+            WLOG(WS_LOG_DEBUG, "Message ID %u not allowed by %s %s",
+                    msg, "client", "before user authentication is complete");
             return 0;
         }
+        else if (MSGIDLIMIT_AUTH(msg)) {
+            return 1;
+        }
     }
     else {
-        if (msg >= MSGID_USERAUTH_RESTRICT && msg < MSGID_USERAUTH_LIMIT) {
-            WLOG(WS_LOG_DEBUG, "Message ID %u not allowed by client "
-                    "after user authentication", msg);
+        if (MSGIDLIMIT_POST_USERAUTH(msg)) {
+            return 1;
+        }
+        else if (MSGIDLIMIT_AUTH(msg)) {
+            WLOG(WS_LOG_DEBUG, "Message ID %u not allowed by %s %s",
+                    msg, "client", "after user authentication");
             return 0;
         }
     }
-    return 1;
+
+    return 0;
 }
 #endif /* NO_WOLFSSH_CLIENT */
 
@@ -711,7 +785,7 @@ INLINE static int IsMessageAllowedClient(WOLFSSH *ssh, byte msg)
  * Returns 1 if allowed 0 if not allowed. */
 INLINE static int IsMessageAllowed(WOLFSSH *ssh, byte msg, byte state)
 {
-    if (state == WS_MSG_SEND && !IsMessageAllowedKeying(ssh, msg)) {
+    if (!IsMessageAllowedKeying(ssh, msg)) {
         return 0;
     }
 
@@ -725,6 +799,7 @@ INLINE static int IsMessageAllowed(WOLFSSH *ssh, byte msg, byte state)
         return IsMessageAllowedClient(ssh, msg);
     }
 #endif /* NO_WOLFSSH_CLIENT */
+    (void)state;
     return 0;
 }
 
@@ -5872,8 +5947,10 @@ static int DoKexDhReply(WOLFSSH* ssh, byte* buf, word32 len, word32* idx)
         ret = GenerateKeys(ssh, hashId, !ssh->handshake->useEccMlKem);
     }
 
-    if (ret == WS_SUCCESS)
+    if (ret == WS_SUCCESS) {
         ret = SendNewKeys(ssh);
+        ssh->handshake->expectMsgId = MSGID_NEWKEYS;
+    }
 
     if (sigKeyBlock_ptr)
         WFREE(sigKeyBlock_ptr, ssh->ctx->heap, DYNTYPE_PRIVKEY);
@@ -10648,8 +10725,9 @@ int SendKexInit(WOLFSSH* ssh)
         ret = BundlePacket(ssh);
     }
 
-    if (ret == WS_SUCCESS)
+    if (ret == WS_SUCCESS) {
         ret = wolfSSH_SendPacket(ssh);
+    }
 
     if (ret != WS_WANT_WRITE && ret != WS_SUCCESS)
         PurgePacket(ssh);
@@ -12613,6 +12691,11 @@ int SendKexDhGexRequest(WOLFSSH* ssh)
     if (ret == WS_SUCCESS)
         ret = wolfSSH_SendPacket(ssh);
 
+    if (ret == WS_SUCCESS) {
+        WLOG_EXPECT_MSGID(MSGID_KEXDH_GEX_GROUP);
+        ssh->handshake->expectMsgId = MSGID_KEXDH_GEX_GROUP;
+    }
+
     WLOG(WS_LOG_DEBUG, "Leaving SendKexDhGexRequest(), ret = %d", ret);
     return ret;
 }
@@ -12701,6 +12784,7 @@ int SendKexDhInit(WOLFSSH* ssh)
 #endif
     int ret = WS_SUCCESS;
     byte msgId = MSGID_KEXDH_INIT;
+    byte expectMsgId = MSGID_KEXDH_REPLY;
     byte e[MAX_KEX_KEY_SZ+1]; /* plus 1 in case of padding. */
     word32 eSz = (word32)sizeof(e);
     byte  ePad = 0;
@@ -12752,6 +12836,7 @@ int SendKexDhInit(WOLFSSH* ssh)
             generator = ssh->handshake->generator;
             generatorSz = ssh->handshake->generatorSz;
             msgId = MSGID_KEXDH_GEX_INIT;
+            expectMsgId = MSGID_KEXDH_GEX_REPLY;
             break;
 #endif
 #ifndef WOLFSSH_NO_ECDH_SHA2_NISTP256
@@ -12963,6 +13048,11 @@ int SendKexDhInit(WOLFSSH* ssh)
     if (ret == WS_SUCCESS)
         ret = wolfSSH_SendPacket(ssh);
 
+    if (ret == WS_SUCCESS) {
+        WLOG_EXPECT_MSGID(expectMsgId);
+        ssh->handshake->expectMsgId = expectMsgId;
+    }
+
     WLOG(WS_LOG_DEBUG, "Leaving SendKexDhInit(), ret = %d", ret);
     return ret;
 }

wolfssh/internal.h

@@ -607,6 +607,7 @@ typedef struct Keys {
 
 
 typedef struct HandshakeInfo {
+    byte expectMsgId;
     byte kexId;
     byte kexIdGuess;
     byte kexHashId;
@@ -1183,6 +1184,8 @@ enum ProcessReplyStates {
 
 
 enum WS_MessageIds {
+    MSGID_NONE = 0,
+
     MSGID_DISCONNECT = 1,
     MSGID_IGNORE = 2,
     MSGID_UNIMPLEMENTED = 3,
@@ -1238,19 +1241,56 @@ enum WS_MessageIds {
 };
 
 
-/* Allows the server to receive up to KEXDH GEX Request during KEX. */
-#define MSGID_KEXDH_LIMIT MSGID_KEXDH_GEX_REQUEST
-
-/* The endpoints should not allow message IDs greater than or
- * equal to msgid 80 before user authentication is complete.
- * Per RFC 4252 section 6. */
-#define MSGID_USERAUTH_LIMIT 80
+/* The following message ID ranges are described in RFC 5251, section 7. */
+enum WS_MessageIdLimits {
+/* Transport Layer Protocol: */
+    MSGIDLIMIT_TRANS_MIN = 1,
+    MSGIDLIMIT_TRANS_GEN_MIN = 1,
+    MSGIDLIMIT_TRANS_GEN_MAX = 19,
+    MSGIDLIMIT_TRANS_ALGO_MIN = 20,
+    MSGIDLIMIT_TRANS_ALGO_MAX = 29,
+    MSGIDLIMIT_TRANS_KEX_MIN = 30,
+    MSGIDLIMIT_TRANS_KEX_MAX = 49,
+    MSGIDLIMIT_TRANS_MAX = 49,
+/* User Authentication Protocol: */
+    MSGIDLIMIT_AUTH_MIN = 50,
+    MSGIDLIMIT_AUTH_GEN_MIN = 50,
+    MSGIDLIMIT_AUTH_GEN_MAX = 59,
+    MSGIDLIMIT_AUTH_METH_MIN = 60,
+    MSGIDLIMIT_AUTH_METH_MAX = 79,
+    MSGIDLIMIT_AUTH_MAX = 79,
+/* Connection Protocol: */
+    MSGIDLIMIT_CONN_MIN = 80,
+    MSGIDLIMIT_CONN_GEN_MIN = 80,
+    MSGIDLIMIT_CONN_GEN_MAX = 89,
+    MSGIDLIMIT_CONN_CHAN_MIN = 90,
+    MSGIDLIMIT_CONN_CHAN_MAX = 127,
+    MSGIDLIMIT_CONN_MAX = 127,
+/* Reserved For Client Protocols: */
+    MSGIDLIMIT_RESERVED_MIN = 128,
+    MSGIDLIMIT_RESERVED_MAX = 191,
+/* Local Extensions: */
+    MSGIDLIMIT_EXTENDED_MIN = 192,
+    MSGIDLIMIT_EXTENDED_MAX = 255,
+};
 
-/* The client should only send the user auth request message
- * (50), it should not accept it. The server should only receive
- * the user auth request message, it should not accept the other
- * user auth messages, it sends them. (>50) */
-#define MSGID_USERAUTH_RESTRICT 50
+/* Message ID bounds checking. */
+#define MSGIDLIMIT_BOUND(x,y,z) ((x) >= (y) && (x) <= (z))
+#define MSGIDLIMIT_COMP(x,name) \
+    MSGIDLIMIT_BOUND((x),MSGIDLIMIT_##name##_MIN,MSGIDLIMIT_##name##_MAX)
+#define MSGIDLIMIT_TRANS(x) MSGIDLIMIT_COMP((x),TRANS)
+#define MSGIDLIMIT_TRANS_GEN(x) MSGIDLIMIT_COMP((x),TRANS_GEN)
+#define MSGIDLIMIT_TRANS_ALGO(x) MSGIDLIMIT_COMP((x),TRANS_ALGO)
+#define MSGIDLIMIT_TRANS_KEX(x) MSGIDLIMIT_COMP((x),TRANS_KEX)
+#define MSGIDLIMIT_AUTH(x) MSGIDLIMIT_COMP((x),AUTH)
+#define MSGIDLIMIT_AUTH_GEN(x) MSGIDLIMIT_COMP((x),AUTH_GEN)
+#define MSGIDLIMIT_AUTH_METH(x) MSGIDLIMIT_COMP((x),AUTH_METH)
+#define MSGIDLIMIT_CONN(x) MSGIDLIMIT_COMP((x),CONN)
+#define MSGIDLIMIT_CONN_GEN(x) MSGIDLIMIT_COMP((x),CONN_GEN)
+#define MSGIDLIMIT_CONN_CHAN(x) MSGIDLIMIT_COMP((x),CONN_CHAN)
+#define MSGIDLIMIT_RESERVED(x) MSGIDLIMIT_COMP((x),RESERVED)
+#define MSGIDLIMIT_EXTENDED(x) MSGIDLIMIT_COMP((x),EXTENDED)
+#define MSGIDLIMIT_POST_USERAUTH(x) ((x) >= MSGIDLIMIT_CONN_MIN)
 
 
 #define CHANNEL_EXTENDED_DATA_STDERR WOLFSSH_EXT_DATA_STDERR

wolfssh/log.h

@@ -81,6 +81,8 @@ WOLFSSH_API void wolfSSH_Log(enum wolfSSH_LogLevel,
     #define WLOG(...) WC_DO_NOTHING
 #endif
 
+#define WLOG_EXPECT_MSGID(x) WLOG(WS_LOG_DEBUG, "Expecting message %d", (x))
+
 #ifdef __cplusplus
 }
 #endif