CVE-2023-1667:kex: Correctly handle last fields of KEXINIT also in the client side

Previously, the last two fields of KEXINIT were considered as always zero for
the key exchange. This was true for the sending side, but might have not been
true for the received KEXINIT from the peer.

This moves the construction of these two fields closer to their reading or
writing, instead of hardcoding them on the last possible moment before they go
as input to the hashing function.

This also allows accepting the first_kex_packet_follows on the client side, even
though there is no kex algorithm now that would allow this.

It also avoid memory leaks in case the server_set_kex() or ssh_set_client_kex()
gets called multiple times, ensuring the algorithms will not change under our
hands.

It also makes use of a new flag to track if we sent KEXINIT.

Previously, this was tracked only implicitly by the content of the
session->next_crypto->{server,client}_kex (local kex). If it was not set, we
considered it was not send. But given that we need to check the local kex even
before sending it when we receive first_kex_packet_follows flag in the KEXINIT,
this can no longer be used.

Signed-off-by: Jakub Jelen <jjelen@redhat.com>
Reviewed-by: Norbert Pocs <npocs@redhat.com>
Reviewed-by: Andreas Schneider <asn@cryptomilk.org>

include/libssh/session.h

@@ -79,6 +79,11 @@ enum ssh_pending_call_e {
 /* Do not accept new session channels (no-more-sessions@openssh.com) */
 #define SSH_SESSION_FLAG_NO_MORE_SESSIONS 0x0004
 
+/* The KEXINIT message can be sent first by either of the parties so this flag
+ * indicates that the message was already sent to make sure it is sent and avoid
+ * sending it twice during key exchange to simplify the state machine. */
+#define SSH_SESSION_FLAG_KEXINIT_SENT 0x0008
+
 /* codes to use with ssh_handle_packets*() */
 /* Infinite timeout */
 #define SSH_TIMEOUT_INFINITE -1

src/client.c

@@ -437,7 +437,7 @@ static void ssh_client_connection_callback(ssh_session session)
     case SSH_SESSION_STATE_KEXINIT_RECEIVED:
         set_status(session, 0.6f);
         ssh_list_kex(&session->next_crypto->server_kex);
-        if (session->next_crypto->client_kex.methods[0] == NULL) {
+        if ((session->flags & SSH_SESSION_FLAG_KEXINIT_SENT) == 0) {
             /* in rekeying state if next_crypto client_kex might be empty */
             rc = ssh_set_client_kex(session);
             if (rc != SSH_OK) {

src/kex.c

@@ -361,7 +361,17 @@ SSH_PACKET_CALLBACK(ssh_packet_kexinit)
     (void)user;
 
     if (session->session_state == SSH_SESSION_STATE_AUTHENTICATED) {
-        SSH_LOG(SSH_LOG_DEBUG, "Initiating key re-exchange");
+        if (session->dh_handshake_state == DH_STATE_FINISHED) {
+            SSH_LOG(SSH_LOG_DEBUG, "Peer initiated key re-exchange");
+            /* Reset the sent flag if the re-kex was initiated by the peer */
+            session->flags &= ~SSH_SESSION_FLAG_KEXINIT_SENT;
+        } else if (session->dh_handshake_state == DH_STATE_INIT_SENT) {
+            SSH_LOG(SSH_LOG_DEBUG, "Receeved peer kexinit answer");
+        } else {
+            ssh_set_error(session, SSH_FATAL,
+                          "SSH_KEXINIT received in wrong state");
+            goto error;
+        }
     } else if (session->session_state != SSH_SESSION_STATE_INITIAL_KEX) {
         ssh_set_error(session, SSH_FATAL,
                       "SSH_KEXINIT received in wrong state");
@@ -383,6 +393,11 @@ SSH_PACKET_CALLBACK(ssh_packet_kexinit)
                           "ssh_packet_kexinit: adding cookie failed");
             goto error;
         }
+
+        ok = server_set_kex(session);
+        if (ok == SSH_ERROR) {
+            goto error;
+        }
 #endif /* WITH_SERVER */
     } else {
         len = ssh_buffer_get_data(packet, crypto->server_kex.cookie, 16);
@@ -398,6 +413,11 @@ SSH_PACKET_CALLBACK(ssh_packet_kexinit)
                           "ssh_packet_kexinit: adding cookie failed");
             goto error;
         }
+
+        ok = ssh_set_client_kex(session);
+        if (ok == SSH_ERROR) {
+            goto error;
+        }
     }
 
     for (i = 0; i < SSH_KEX_METHODS; i++) {
@@ -446,23 +466,38 @@ SSH_PACKET_CALLBACK(ssh_packet_kexinit)
      * 'make_sessionid').
      */
 
+    rc = ssh_buffer_get_u8(packet, &first_kex_packet_follows);
+    if (rc != 1) {
+        goto error;
+    }
+
+    rc = ssh_buffer_add_u8(session->in_hashbuf, first_kex_packet_follows);
+    if (rc < 0) {
+        goto error;
+    }
+
+    rc = ssh_buffer_add_u32(session->in_hashbuf, kexinit_reserved);
+    if (rc < 0) {
+        goto error;
+    }
+
+    /*
+     * Remember whether 'first_kex_packet_follows' was set and the client
+     * guess was wrong: in this case the next SSH_MSG_KEXDH_INIT message
+     * must be ignored.
+     */
+    if (first_kex_packet_follows) {
+        char **client_methods = crypto->client_kex.methods;
+        char **server_methods = crypto->server_kex.methods;
+        session->first_kex_follows_guess_wrong =
+            cmp_first_kex_algo(client_methods[SSH_KEX],
+                               server_methods[SSH_KEX]) ||
+            cmp_first_kex_algo(client_methods[SSH_HOSTKEYS],
+                               server_methods[SSH_HOSTKEYS]);
+    }
+
 #ifdef WITH_SERVER
     if (server_kex) {
-        rc = ssh_buffer_get_u8(packet, &first_kex_packet_follows);
-        if (rc != 1) {
-            goto error;
-        }
-
-        rc = ssh_buffer_add_u8(session->in_hashbuf, first_kex_packet_follows);
-        if (rc < 0) {
-            goto error;
-        }
-
-        rc = ssh_buffer_add_u32(session->in_hashbuf, kexinit_reserved);
-        if (rc < 0) {
-            goto error;
-        }
-
         /*
          * If client sent a ext-info-c message in the kex list, it supports
          * RFC 8308 extension negotiation.
@@ -534,19 +569,6 @@ SSH_PACKET_CALLBACK(ssh_packet_kexinit)
                     session->extensions & SSH_EXT_SIG_RSA_SHA256 ? "SHA256" : "",
                     session->extensions & SSH_EXT_SIG_RSA_SHA512 ? " SHA512" : "");
         }
-
-        /*
-         * Remember whether 'first_kex_packet_follows' was set and the client
-         * guess was wrong: in this case the next SSH_MSG_KEXDH_INIT message
-         * must be ignored.
-         */
-        if (first_kex_packet_follows) {
-          session->first_kex_follows_guess_wrong =
-            cmp_first_kex_algo(session->next_crypto->client_kex.methods[SSH_KEX],
-                               session->next_crypto->server_kex.methods[SSH_KEX]) ||
-            cmp_first_kex_algo(session->next_crypto->client_kex.methods[SSH_HOSTKEYS],
-                               session->next_crypto->server_kex.methods[SSH_HOSTKEYS]);
-        }
     }
 #endif /* WITH_SERVER */
 
@@ -702,14 +724,18 @@ int ssh_set_client_kex(ssh_session session)
     int i;
     size_t kex_len, len;
 
+    /* Skip if already set, for example for the rekey or when we do the guessing
+     * it could have been already used to make some protocol decisions. */
+    if (client->methods[0] != NULL) {
+        return SSH_OK;
+    }
+
     ok = ssh_get_random(client->cookie, 16, 0);
     if (!ok) {
         ssh_set_error(session, SSH_FATAL, "PRNG error");
         return SSH_ERROR;
     }
 
-    memset(client->methods, 0, SSH_KEX_METHODS * sizeof(char **));
-
     /* Set the list of allowed algorithms in order of preference, if it hadn't
      * been set yet. */
     for (i = 0; i < SSH_KEX_METHODS; i++) {
@@ -960,11 +986,22 @@ int ssh_send_kex(ssh_session session)
         goto error;
     }
 
+    /* Prepare also the first_kex_packet_follows and reserved to 0 */
+    rc = ssh_buffer_add_u8(session->out_hashbuf, 0);
+    if (rc < 0) {
+        goto error;
+    }
+    rc = ssh_buffer_add_u32(session->out_hashbuf, 0);
+    if (rc < 0) {
+        goto error;
+    }
+
     rc = ssh_packet_send(session);
     if (rc == SSH_ERROR) {
         return -1;
     }
 
+    session->flags |= SSH_SESSION_FLAG_KEXINIT_SENT;
     SSH_LOG(SSH_LOG_PACKET, "SSH_MSG_KEXINIT sent");
     return 0;
 
@@ -1206,33 +1243,6 @@ int ssh_make_sessionid(ssh_session session)
         client_hash = session->in_hashbuf;
     }
 
-    /*
-     * Handle the two final fields for the KEXINIT message (RFC 4253 7.1):
-     *
-     *      boolean      first_kex_packet_follows
-     *      uint32       0 (reserved for future extension)
-     */
-    rc = ssh_buffer_add_u8(server_hash, 0);
-    if (rc < 0) {
-        goto error;
-    }
-    rc = ssh_buffer_add_u32(server_hash, 0);
-    if (rc < 0) {
-        goto error;
-    }
-
-    /* These fields are handled for the server case in ssh_packet_kexinit. */
-    if (session->client) {
-        rc = ssh_buffer_add_u8(client_hash, 0);
-        if (rc < 0) {
-            goto error;
-        }
-        rc = ssh_buffer_add_u32(client_hash, 0);
-        if (rc < 0) {
-            goto error;
-        }
-    }
-
     rc = ssh_dh_get_next_server_publickey_blob(session, &server_pubkey_blob);
     if (rc != SSH_OK) {
         goto error;

src/server.c

@@ -92,7 +92,11 @@ int server_set_kex(ssh_session session)
     size_t len;
     int ok;
 
-    ZERO_STRUCTP(server);
+    /* Skip if already set, for example for the rekey or when we do the guessing
+     * it could have been already used to make some protocol decisions. */
+    if (server->methods[0] != NULL) {
+        return SSH_OK;
+    }
 
     ok = ssh_get_random(server->cookie, 16, 0);
     if (!ok) {
@@ -366,7 +370,7 @@ static void ssh_server_connection_callback(ssh_session session)
         break;
     case SSH_SESSION_STATE_KEXINIT_RECEIVED:
         set_status(session, 0.6f);
-        if (session->next_crypto->server_kex.methods[0] == NULL) {
+        if ((session->flags & SSH_SESSION_FLAG_KEXINIT_SENT) == 0) {
             rc = server_set_kex(session);
             if (rc == SSH_ERROR) {
                 goto error;