[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH] kex: server fix to include first_kex_packet_follows


On 4/9/14, 2:05 AM, Andreas Schneider wrote:
> On Tuesday 08 April 2014 17:00:28 Jon Simons wrote:
>> On 3/27/14, 6:03 PM, Jon Simons wrote:...
>>
>>> Attached is an updated patch.
>>
>> There is a bug in the previous patch here -- though that patch fixes
>> the original problem for the case that 'first_kex_packet_follows' is
>> set and the client's guessed key exchange algorithm is correct, it
>> is not complete in that it does not include logic for the case that
>> the guess is incorrect.
>>
>> Attached is an updated patch which fixes that by using a field in the
>> session struct to ignore the first KEX_DHINIT message encountered
>> after an incorrect guess.
>>
> 
> The patch doesn't apply on master, it has several issues with dh.c.
> 
> Is this only for v0-6?

Ah the previous patch yes was based off of v0-6.  I've attached now an updated
patch that is based off of master @ad1313c2e5cf273aec7bf5415876d389ea8d8ae7.

The main conflict was that on master a few call sites are now converted to
using 'ssh_buffer_add_data' instead of 'buffer_add_data'; on v0-6 the change
area is using 'buffer_add_data'.


Thanks Again,
-Jon
From f7962f432de190102114906ec9e56c1c699a1fd8 Mon Sep 17 00:00:00 2001
From: Jon Simons <jon@xxxxxxxxxxxxx>
Date: Wed, 9 Apr 2014 15:24:04 -0700
Subject: [PATCH] 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@xxxxxxxxxxxxx>
---
 include/libssh/session.h |   9 ++
 src/dh.c                 | 358 +++++++++++++++++++++++++----------------------
 src/kex.c                | 213 +++++++++++++++++++---------
 src/server.c             |   9 ++
 4 files changed, 354 insertions(+), 235 deletions(-)

diff --git a/include/libssh/session.h b/include/libssh/session.h
index 8480135..29bdd60 100644
--- a/include/libssh/session.h
+++ b/include/libssh/session.h
@@ -127,6 +127,15 @@ struct ssh_session_struct {
     struct ssh_agent_state_struct *agent_state;
     struct ssh_auth_auto_state_struct *auth_auto_state;
 
+    /*
+     * RFC 4253, 7.1: if the first_kex_packet_follows flag was set in
+     * the received SSH_MSG_KEXINIT, but the guess was wrong, this
+     * field will be set such that the following guessed packet will
+     * be ignored.  Once that packet has been received and ignored,
+     * this field is cleared.
+     */
+    int first_kex_follows_guess_wrong;
+
     ssh_buffer in_hashbuf;
     ssh_buffer out_hashbuf;
     struct ssh_crypto_struct *current_crypto;
diff --git a/src/dh.c b/src/dh.c
index ca2c2bf..3255ac7 100644
--- a/src/dh.c
+++ b/src/dh.c
@@ -587,218 +587,234 @@ error:
   return SSH_ERROR;
 }
 
-
-/*
-static void sha_add(ssh_string str,SHACTX ctx){
-    sha1_update(ctx,str,string_len(str)+4);
-#ifdef DEBUG_CRYPTO
-    ssh_print_hexa("partial hashed sessionid",str,string_len(str)+4);
-#endif
-}
-*/
-
 int make_sessionid(ssh_session session) {
-  ssh_string num = NULL;
-  ssh_string str = NULL;
-  ssh_buffer server_hash = NULL;
-  ssh_buffer client_hash = NULL;
-  ssh_buffer buf = NULL;
-  uint32_t len;
-  int rc = SSH_ERROR;
-
-  buf = ssh_buffer_new();
-  if (buf == NULL) {
-    return rc;
-  }
-
-  str = ssh_string_from_char(session->clientbanner);
-  if (str == NULL) {
-    goto error;
-  }
-
-  if (buffer_add_ssh_string(buf, str) < 0) {
-    goto error;
-  }
-  ssh_string_free(str);
-
-  str = ssh_string_from_char(session->serverbanner);
-  if (str == NULL) {
-    goto error;
-  }
-
-  if (buffer_add_ssh_string(buf, str) < 0) {
-    goto error;
-  }
-
-  if (session->client) {
-    server_hash = session->in_hashbuf;
-    client_hash = session->out_hashbuf;
-  } else {
-    server_hash = session->out_hashbuf;
-    client_hash = session->in_hashbuf;
-  }
+    ssh_string num = NULL;
+    ssh_string str = NULL;
+    ssh_buffer server_hash = NULL;
+    ssh_buffer client_hash = NULL;
+    ssh_buffer buf = NULL;
+    uint32_t len;
+    int rc = SSH_ERROR;
+
+    buf = ssh_buffer_new();
+    if (buf == NULL) {
+        return rc;
+    }
 
-  if (buffer_add_u32(server_hash, 0) < 0) {
-    goto error;
-  }
-  if (buffer_add_u8(server_hash, 0) < 0) {
-    goto error;
-  }
-  if (buffer_add_u32(client_hash, 0) < 0) {
-    goto error;
-  }
-  if (buffer_add_u8(client_hash, 0) < 0) {
-    goto error;
-  }
+    str = ssh_string_from_char(session->clientbanner);
+    if (str == NULL) {
+        goto error;
+    }
 
-  len = ntohl(buffer_get_rest_len(client_hash));
-  if (buffer_add_u32(buf,len) < 0) {
-    goto error;
-  }
-  if (ssh_buffer_add_data(buf, buffer_get_rest(client_hash),
-        buffer_get_rest_len(client_hash)) < 0) {
-    goto error;
-  }
+    rc = buffer_add_ssh_string(buf, str);
+    if (rc < 0) {
+        goto error;
+    }
+    ssh_string_free(str);
 
-  len = ntohl(buffer_get_rest_len(server_hash));
-  if (buffer_add_u32(buf, len) < 0) {
-    goto error;
-  }
-  if (ssh_buffer_add_data(buf, buffer_get_rest(server_hash),
-        buffer_get_rest_len(server_hash)) < 0) {
-    goto error;
-  }
+    str = ssh_string_from_char(session->serverbanner);
+    if (str == NULL) {
+        goto error;
+    }
 
-  len = ssh_string_len(session->next_crypto->server_pubkey) + 4;
-  if (ssh_buffer_add_data(buf, session->next_crypto->server_pubkey, len) < 0) {
-    goto error;
-  }
-  if(session->next_crypto->kex_type == SSH_KEX_DH_GROUP1_SHA1 ||
-     session->next_crypto->kex_type == SSH_KEX_DH_GROUP14_SHA1) {
+    rc = buffer_add_ssh_string(buf, str);
+    if (rc < 0) {
+        goto error;
+    }
 
-    num = make_bignum_string(session->next_crypto->e);
-    if (num == NULL) {
-      goto error;
+    if (session->client) {
+        server_hash = session->in_hashbuf;
+        client_hash = session->out_hashbuf;
+    } else {
+        server_hash = session->out_hashbuf;
+        client_hash = session->in_hashbuf;
     }
 
-    len = ssh_string_len(num) + 4;
-    if (ssh_buffer_add_data(buf, num, len) < 0) {
-      goto error;
+    /*
+     * 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 = buffer_add_u8(server_hash, 0);
+    if (rc < 0) {
+        goto error;
+    }
+    rc = buffer_add_u32(server_hash, 0);
+    if (rc < 0) {
+        goto error;
     }
 
-    ssh_string_free(num);
-    num = make_bignum_string(session->next_crypto->f);
-    if (num == NULL) {
-      goto error;
+    /* These fields are handled for the server case in ssh_packet_kexinit. */
+    if (session->client) {
+        rc = buffer_add_u8(client_hash, 0);
+        if (rc < 0) {
+            goto error;
+        }
+        rc = buffer_add_u32(client_hash, 0);
+        if (rc < 0) {
+            goto error;
+        }
     }
 
-    len = ssh_string_len(num) + 4;
-    if (ssh_buffer_add_data(buf, num, len) < 0) {
-      goto error;
+    len = ntohl(buffer_get_rest_len(client_hash));
+    rc = buffer_add_u32(buf,len);
+    if (rc < 0) {
+        goto error;
+    }
+    rc = ssh_buffer_add_data(buf, buffer_get_rest(client_hash),
+                              buffer_get_rest_len(client_hash));
+    if (rc < 0) {
+        goto error;
     }
 
-    ssh_string_free(num);
-#ifdef HAVE_ECDH
-  } else if (session->next_crypto->kex_type == SSH_KEX_ECDH_SHA2_NISTP256){
-    if(session->next_crypto->ecdh_client_pubkey == NULL ||
-            session->next_crypto->ecdh_server_pubkey == NULL){
-        SSH_LOG(SSH_LOG_WARNING, "ECDH parameted missing");
+    len = ntohl(buffer_get_rest_len(server_hash));
+    rc = buffer_add_u32(buf, len);
+    if (rc < 0) {
         goto error;
     }
-    rc = buffer_add_ssh_string(buf,session->next_crypto->ecdh_client_pubkey);
+    rc = ssh_buffer_add_data(buf, buffer_get_rest(server_hash),
+                                  buffer_get_rest_len(server_hash));
     if (rc < 0) {
         goto error;
     }
-    rc = buffer_add_ssh_string(buf,session->next_crypto->ecdh_server_pubkey);
+
+    len = ssh_string_len(session->next_crypto->server_pubkey) + 4;
+    rc = ssh_buffer_add_data(buf, session->next_crypto->server_pubkey, len);
     if (rc < 0) {
         goto error;
     }
+
+    if (session->next_crypto->kex_type == SSH_KEX_DH_GROUP1_SHA1 ||
+        session->next_crypto->kex_type == SSH_KEX_DH_GROUP14_SHA1) {
+
+        num = make_bignum_string(session->next_crypto->e);
+        if (num == NULL) {
+            goto error;
+        }
+
+        len = ssh_string_len(num) + 4;
+        rc = ssh_buffer_add_data(buf, num, len);
+        if (rc < 0) {
+            goto error;
+        }
+
+        ssh_string_free(num);
+        num = make_bignum_string(session->next_crypto->f);
+        if (num == NULL) {
+            goto error;
+        }
+
+        len = ssh_string_len(num) + 4;
+        rc = ssh_buffer_add_data(buf, num, len);
+        if (rc < 0) {
+          goto error;
+        }
+
+        ssh_string_free(num);
+#ifdef HAVE_ECDH
+    } else if (session->next_crypto->kex_type == SSH_KEX_ECDH_SHA2_NISTP256) {
+        if (session->next_crypto->ecdh_client_pubkey == NULL ||
+            session->next_crypto->ecdh_server_pubkey == NULL) {
+            SSH_LOG(SSH_LOG_WARNING, "ECDH parameted missing");
+            goto error;
+        }
+        rc = buffer_add_ssh_string(buf,session->next_crypto->ecdh_client_pubkey);
+        if (rc < 0) {
+            goto error;
+        }
+        rc = buffer_add_ssh_string(buf,session->next_crypto->ecdh_server_pubkey);
+        if (rc < 0) {
+            goto error;
+        }
 #endif
 #ifdef HAVE_CURVE25519
-  } else if(session->next_crypto->kex_type == SSH_KEX_CURVE25519_SHA256_LIBSSH_ORG){
-	  rc = buffer_add_u32(buf, htonl(CURVE25519_PUBKEY_SIZE));
-	  rc += ssh_buffer_add_data(buf, session->next_crypto->curve25519_client_pubkey,
-			  CURVE25519_PUBKEY_SIZE);
-	  rc += buffer_add_u32(buf, htonl(CURVE25519_PUBKEY_SIZE));
-	  rc += ssh_buffer_add_data(buf, session->next_crypto->curve25519_server_pubkey,
-			  CURVE25519_PUBKEY_SIZE);
-	  if (rc != SSH_OK) {
-		  goto error;
-      }
+    } else if (session->next_crypto->kex_type == SSH_KEX_CURVE25519_SHA256_LIBSSH_ORG) {
+        rc = buffer_add_u32(buf, htonl(CURVE25519_PUBKEY_SIZE));
+        rc += ssh_buffer_add_data(buf, session->next_crypto->curve25519_client_pubkey,
+                                       CURVE25519_PUBKEY_SIZE);
+        rc += buffer_add_u32(buf, htonl(CURVE25519_PUBKEY_SIZE));
+        rc += ssh_buffer_add_data(buf, session->next_crypto->curve25519_server_pubkey,
+                                       CURVE25519_PUBKEY_SIZE);
+        if (rc != SSH_OK) {
+                goto error;
+        }
 #endif
-  }
-  num = make_bignum_string(session->next_crypto->k);
-  if (num == NULL) {
-    goto error;
-  }
+    }
 
-  len = ssh_string_len(num) + 4;
-  if (ssh_buffer_add_data(buf, num, len) < 0) {
-    goto error;
-  }
+    num = make_bignum_string(session->next_crypto->k);
+    if (num == NULL) {
+        goto error;
+    }
+
+    len = ssh_string_len(num) + 4;
+    rc = ssh_buffer_add_data(buf, num, len);
+    if (rc < 0) {
+        goto error;
+    }
 
 #ifdef DEBUG_CRYPTO
-  ssh_print_hexa("hash buffer", ssh_buffer_get_begin(buf), ssh_buffer_get_len(buf));
+    ssh_print_hexa("hash buffer", ssh_buffer_get_begin(buf), ssh_buffer_get_len(buf));
 #endif
 
-  switch(session->next_crypto->kex_type){
+    switch (session->next_crypto->kex_type) {
     case SSH_KEX_DH_GROUP1_SHA1:
     case SSH_KEX_DH_GROUP14_SHA1:
-      session->next_crypto->digest_len = SHA_DIGEST_LENGTH;
-      session->next_crypto->mac_type = SSH_MAC_SHA1;
-      session->next_crypto->secret_hash = malloc(session->next_crypto->digest_len);
-      if(session->next_crypto->secret_hash == NULL){
-        ssh_set_error_oom(session);
-        goto error;
-      }
-      sha1(buffer_get_rest(buf), buffer_get_rest_len(buf),
-                session->next_crypto->secret_hash);
-      break;
+        session->next_crypto->digest_len = SHA_DIGEST_LENGTH;
+        session->next_crypto->mac_type = SSH_MAC_SHA1;
+        session->next_crypto->secret_hash = malloc(session->next_crypto->digest_len);
+        if (session->next_crypto->secret_hash == NULL) {
+            ssh_set_error_oom(session);
+            goto error;
+        }
+        sha1(buffer_get_rest(buf), buffer_get_rest_len(buf),
+                                   session->next_crypto->secret_hash);
+        break;
     case SSH_KEX_ECDH_SHA2_NISTP256:
     case SSH_KEX_CURVE25519_SHA256_LIBSSH_ORG:
-      session->next_crypto->digest_len = SHA256_DIGEST_LENGTH;
-      session->next_crypto->mac_type = SSH_MAC_SHA256;
-      session->next_crypto->secret_hash = malloc(session->next_crypto->digest_len);
-      if(session->next_crypto->secret_hash == NULL){
-        ssh_set_error_oom(session);
-        goto error;
-      }
-      sha256(buffer_get_rest(buf), buffer_get_rest_len(buf),
-          session->next_crypto->secret_hash);
-      break;
-  }
-  /* During the first kex, secret hash and session ID are equal. However, after
-   * a key re-exchange, a new secret hash is calculated. This hash will not replace
-   * but complement existing session id.
-   */
-  if (!session->next_crypto->session_id){
-      session->next_crypto->session_id = malloc(session->next_crypto->digest_len);
-      if (session->next_crypto->session_id == NULL){
-          ssh_set_error_oom(session);
-          goto error;
-      }
-      memcpy(session->next_crypto->session_id, session->next_crypto->secret_hash,
-              session->next_crypto->digest_len);
-  }
+        session->next_crypto->digest_len = SHA256_DIGEST_LENGTH;
+        session->next_crypto->mac_type = SSH_MAC_SHA256;
+        session->next_crypto->secret_hash = malloc(session->next_crypto->digest_len);
+        if (session->next_crypto->secret_hash == NULL) {
+            ssh_set_error_oom(session);
+            goto error;
+        }
+        sha256(buffer_get_rest(buf), buffer_get_rest_len(buf),
+                                     session->next_crypto->secret_hash);
+        break;
+    }
+    /* During the first kex, secret hash and session ID are equal. However, after
+     * a key re-exchange, a new secret hash is calculated. This hash will not replace
+     * but complement existing session id.
+     */
+    if (!session->next_crypto->session_id) {
+        session->next_crypto->session_id = malloc(session->next_crypto->digest_len);
+        if (session->next_crypto->session_id == NULL) {
+            ssh_set_error_oom(session);
+            goto error;
+        }
+        memcpy(session->next_crypto->session_id, session->next_crypto->secret_hash,
+                session->next_crypto->digest_len);
+    }
 #ifdef DEBUG_CRYPTO
-  printf("Session hash: \n");
-  ssh_print_hexa("secret hash", session->next_crypto->secret_hash, session->next_crypto->digest_len);
-  ssh_print_hexa("session id", session->next_crypto->session_id, session->next_crypto->digest_len);
+    printf("Session hash: \n");
+    ssh_print_hexa("secret hash", session->next_crypto->secret_hash, session->next_crypto->digest_len);
+    ssh_print_hexa("session id", session->next_crypto->session_id, session->next_crypto->digest_len);
 #endif
 
-  rc = SSH_OK;
+    rc = SSH_OK;
 error:
-  ssh_buffer_free(buf);
-  ssh_buffer_free(client_hash);
-  ssh_buffer_free(server_hash);
+    ssh_buffer_free(buf);
+    ssh_buffer_free(client_hash);
+    ssh_buffer_free(server_hash);
 
-  session->in_hashbuf = NULL;
-  session->out_hashbuf = NULL;
+    session->in_hashbuf = NULL;
+    session->out_hashbuf = NULL;
 
-  ssh_string_free(str);
-  ssh_string_free(num);
+    ssh_string_free(str);
+    ssh_string_free(num);
 
-  return rc;
+    return rc;
 }
 
 int hashbufout_add_cookie(ssh_session session) {
diff --git a/src/kex.c b/src/kex.c
index 4eb45d2..c5dd213 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) {
diff --git a/src/server.c b/src/server.c
index b87c6e5..005effe 100644
--- a/src/server.c
+++ b/src/server.c
@@ -174,6 +174,15 @@ SSH_PACKET_CALLBACK(ssh_packet_kexdh_init){
     SSH_LOG(SSH_LOG_RARE,"Invalid state for SSH_MSG_KEXDH_INIT");
     goto error;
   }
+
+  /* If first_kex_packet_follows guess was wrong, ignore this message. */
+  if (session->first_kex_follows_guess_wrong != 0) {
+    SSH_LOG(SSH_LOG_RARE, "first_kex_packet_follows guess was wrong, "
+                          "ignoring first SSH_MSG_KEXDH_INIT message");
+    session->first_kex_follows_guess_wrong = 0;
+    goto error;
+  }
+
   switch(session->next_crypto->kex_type){
       case SSH_KEX_DH_GROUP1_SHA1:
       case SSH_KEX_DH_GROUP14_SHA1:
-- 
1.8.4.21.g992c386


Follow-Ups:
Re: [PATCH] kex: server fix to include first_kex_packet_followsAndreas Schneider <asn@xxxxxxxxxxxxxx>
Archive administrator: postmaster@lists.cynapses.org