kex: server fix for first_kex_packet_follows

Ensure to honor the 'first_kex_packet_follow' field when processing
KEXINIT messages in the 'ssh_packet_kexinit' callback.  Until now
libssh would assume that this field is always unset (zero).  But
some clients may set this (dropbear at or beyond version 2013.57),
and it needs to be included when computing the session ID.

Also include logic for handling wrongly-guessed key exchange algorithms.
Save whether a client's guess is wrong in a new field in the session
struct: when set, the next KEX_DHINIT message to be processed will be
ignored per RFC 4253, 7.1.

While here, update both 'ssh_packet_kexinit' and 'make_sessionid' to
use softabs with a 4 space indent level throughout, and also convert
various error-checking to store intermediate values into an explicit
'rc'.

Signed-off-by: Jon Simons <jon@jonsimons.org>
Reviewed-by: Andreas Schneider <asn@cryptomilk.org>

1 files changed, 149 insertions, 64 deletions

diff --git a/src/kex.c b/src/kex.c
index 4eb45d22..c5dd2130 100644
--- a/src/kex.c
+++ b/src/kex.c
@@ -275,87 +275,172 @@ char *ssh_find_matching(const char *available_d, const char *preferred_d){
     return NULL;
 }
 
-SSH_PACKET_CALLBACK(ssh_packet_kexinit){
-	int server_kex=session->server;
-  ssh_string str = NULL;
-  char *strings[KEX_METHODS_SIZE];
-  int i;
+/**
+ * @internal
+ * @brief returns whether the first client key exchange algorithm matches
+ *        the first server key exchange algorithm
+ * @returns whether the first client key exchange algorithm matches
+ *          the first server key exchange algorithm
+ */
+static int is_first_kex_packet_follows_guess_wrong(const char *client_kex,
+                                                   const char *server_kex) {
+    int is_wrong = 1;
+    char **server_kex_tokens = NULL;
+    char **client_kex_tokens = tokenize(client_kex);
+
+    if (client_kex_tokens == NULL) {
+        goto out;
+    }
 
-  (void)type;
-  (void)user;
-  memset(strings, 0, sizeof(strings));
-  if (session->session_state == SSH_SESSION_STATE_AUTHENTICATED){
-      SSH_LOG(SSH_LOG_WARNING, "Other side initiating key re-exchange");
-  } else if(session->session_state != SSH_SESSION_STATE_INITIAL_KEX){
-  	ssh_set_error(session,SSH_FATAL,"SSH_KEXINIT received in wrong state");
-  	goto error;
-  }
-  if (server_kex) {
-      if (buffer_get_data(packet,session->next_crypto->client_kex.cookie,16) != 16) {
-        ssh_set_error(session, SSH_FATAL, "ssh_packet_kexinit: no cookie in packet");
-        goto error;
-      }
+    if (client_kex_tokens[0] == NULL) {
+        goto freeout;
+    }
 
-      if (hashbufin_add_cookie(session, session->next_crypto->client_kex.cookie) < 0) {
-        ssh_set_error(session, SSH_FATAL, "ssh_packet_kexinit: adding cookie failed");
-        goto error;
-      }
-  } else {
-      if (buffer_get_data(packet,session->next_crypto->server_kex.cookie,16) != 16) {
-        ssh_set_error(session, SSH_FATAL, "ssh_packet_kexinit: no cookie in packet");
-        goto error;
-      }
+    server_kex_tokens = tokenize(server_kex);
+    if (server_kex_tokens == NULL) {
+        goto freeout;
+    }
 
-      if (hashbufin_add_cookie(session, session->next_crypto->server_kex.cookie) < 0) {
-        ssh_set_error(session, SSH_FATAL, "ssh_packet_kexinit: adding cookie failed");
+    is_wrong = (strcmp(client_kex_tokens[0], server_kex_tokens[0]) != 0);
+
+    SAFE_FREE(server_kex_tokens[0]);
+    SAFE_FREE(server_kex_tokens);
+freeout:
+    SAFE_FREE(client_kex_tokens[0]);
+    SAFE_FREE(client_kex_tokens);
+out:
+    return is_wrong;
+}
+
+SSH_PACKET_CALLBACK(ssh_packet_kexinit){
+    int i;
+    int server_kex=session->server;
+    ssh_string str = NULL;
+    char *strings[KEX_METHODS_SIZE];
+    int rc = SSH_ERROR;
+
+    uint8_t first_kex_packet_follows = 0;
+    uint32_t kexinit_reserved = 0;
+
+    (void)type;
+    (void)user;
+
+    memset(strings, 0, sizeof(strings));
+    if (session->session_state == SSH_SESSION_STATE_AUTHENTICATED){
+        SSH_LOG(SSH_LOG_WARNING, "Other side initiating key re-exchange");
+    } else if(session->session_state != SSH_SESSION_STATE_INITIAL_KEX){
+        ssh_set_error(session,SSH_FATAL,"SSH_KEXINIT received in wrong state");
         goto error;
-      }
-  }
+    }
 
-  for (i = 0; i < KEX_METHODS_SIZE; i++) {
-    str = buffer_get_ssh_string(packet);
-    if (str == NULL) {
-      break;
+    if (server_kex) {
+        rc = buffer_get_data(packet,session->next_crypto->client_kex.cookie, 16);
+        if (rc != 16) {
+            ssh_set_error(session, SSH_FATAL, "ssh_packet_kexinit: no cookie in packet");
+            goto error;
+        }
+
+        rc = hashbufin_add_cookie(session, session->next_crypto->client_kex.cookie);
+        if (rc < 0) {
+            ssh_set_error(session, SSH_FATAL, "ssh_packet_kexinit: adding cookie failed");
+            goto error;
+        }
+    } else {
+        rc = buffer_get_data(packet,session->next_crypto->server_kex.cookie, 16);
+        if (rc != 16) {
+            ssh_set_error(session, SSH_FATAL, "ssh_packet_kexinit: no cookie in packet");
+            goto error;
+        }
+
+        rc = hashbufin_add_cookie(session, session->next_crypto->server_kex.cookie);
+        if (rc < 0) {
+            ssh_set_error(session, SSH_FATAL, "ssh_packet_kexinit: adding cookie failed");
+            goto error;
+        }
     }
 
-    if (buffer_add_ssh_string(session->in_hashbuf, str) < 0) {
-    	ssh_set_error(session, SSH_FATAL, "Error adding string in hash buffer");
-      goto error;
+    for (i = 0; i < KEX_METHODS_SIZE; i++) {
+        str = buffer_get_ssh_string(packet);
+        if (str == NULL) {
+          break;
+        }
+
+        rc = buffer_add_ssh_string(session->in_hashbuf, str);
+        if (rc < 0) {
+            ssh_set_error(session, SSH_FATAL, "Error adding string in hash buffer");
+            goto error;
+        }
+
+        strings[i] = ssh_string_to_char(str);
+        if (strings[i] == NULL) {
+            ssh_set_error_oom(session);
+            goto error;
+        }
+        ssh_string_free(str);
+        str = NULL;
     }
 
-    strings[i] = ssh_string_to_char(str);
-    if (strings[i] == NULL) {
-    	ssh_set_error_oom(session);
-      goto error;
+    /* copy the server kex info into an array of strings */
+    if (server_kex) {
+        for (i = 0; i < SSH_KEX_METHODS; i++) {
+            session->next_crypto->client_kex.methods[i] = strings[i];
+        }
+    } else { /* client */
+        for (i = 0; i < SSH_KEX_METHODS; i++) {
+            session->next_crypto->server_kex.methods[i] = strings[i];
+        }
     }
-    ssh_string_free(str);
-    str = NULL;
-  }
 
-  /* copy the server kex info into an array of strings */
-  if (server_kex) {
-    for (i = 0; i < SSH_KEX_METHODS; i++) {
-      session->next_crypto->client_kex.methods[i] = strings[i];
+    /*
+     * Handle the two final fields for the KEXINIT message (RFC 4253 7.1):
+     *
+     *      boolean      first_kex_packet_follows
+     *      uint32       0 (reserved for future extension)
+     *
+     * Notably if clients set 'first_kex_packet_follows', it is expected
+     * that its value is included when computing the session ID (see
+     * 'make_sessionid').
+     */
+    rc = buffer_get_u8(packet, &first_kex_packet_follows);
+    if (rc != 1) {
+        goto error;
     }
-  } else { /* client */
-    for (i = 0; i < SSH_KEX_METHODS; i++) {
-      session->next_crypto->server_kex.methods[i] = strings[i];
+
+    rc = buffer_add_u8(session->in_hashbuf, first_kex_packet_follows);
+    if (rc < 0) {
+        goto error;
     }
-  }
 
-  session->session_state=SSH_SESSION_STATE_KEXINIT_RECEIVED;
-  session->dh_handshake_state=DH_STATE_INIT;
-  session->ssh_connection_callback(session);
-  return SSH_PACKET_USED;
+    rc = 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 (server_kex && first_kex_packet_follows) {
+      session->first_kex_follows_guess_wrong =
+        is_first_kex_packet_follows_guess_wrong(session->next_crypto->client_kex.methods[SSH_KEX],
+                                                session->next_crypto->server_kex.methods[SSH_KEX]);
+    }
+
+    session->session_state = SSH_SESSION_STATE_KEXINIT_RECEIVED;
+    session->dh_handshake_state = DH_STATE_INIT;
+    session->ssh_connection_callback(session);
+    return SSH_PACKET_USED;
+
 error:
-  ssh_string_free(str);
-  for (i = 0; i < SSH_KEX_METHODS; i++) {
-    SAFE_FREE(strings[i]);
-  }
+    ssh_string_free(str);
+    for (i = 0; i < SSH_KEX_METHODS; i++) {
+        SAFE_FREE(strings[i]);
+    }
 
-  session->session_state = SSH_SESSION_STATE_ERROR;
+    session->session_state = SSH_SESSION_STATE_ERROR;
 
-  return SSH_PACKET_USED;
+    return SSH_PACKET_USED;
 }
 
 void ssh_list_kex(struct ssh_kex_struct *kex) {