Re: Bugs when using rsa-sha2 (+patches)

[Tilo Eckert <tilo.eckert@xxxxxxx>]

Hi Jakub,

the new patch set contains my previous rsa-sha2 related patches as well
as fixes for the rsa-sha2 downgrade / ssh-rsa upgrade issues and the
ECDSA signature bypass when ECC is turned off. Please check if my fixes
are sufficient.

Initially I attempted to verify hostkey and signature algos separately,
but that didn't work out very well. It would have meant a lot of
redundancy. After realizing that hostkey and signature need to be
considered together, performing the necessary checks in the new hostkey
type validation function seemed straight-forward.

If you or someone else has an idea on how to talk OpenSSH into being a
naughty server or if you can come up with some other way to emulate an
rsa-sha2 downgrade attack, feel free to add a test.

Regards,
Tilo

>> The function ssh_pki_signature_verify_blob() is used by server
>> (messages.c) and client code (packet_cb.c). So, I think one way to
>> fix
>> it is to remove the ssh_pki_import_signature_blob() call from
>> ssh_pki_signature_verify_blob(), move it to the two callers and pass
>> sig
>> instead of sig_blob to the verify function. That would allow us to
>> check
>> the signature type in ssh_packet_newkeys() depending on the allowed
>> hostkey types.
> 
> That sounds like a way to go. The ssh_pki_signature_verify_blob()
> unfortunately does not have slightest idea what is the allowed list to
> verify against.
> 
> We will not have the same problem in the server (messages.c), because
> server does not have any configuration which would allow to disable
> some authentication algorithms.
> 
>> I think there may be another unrelated security issue in
>> ssh_pki_signature_verify_blob(): When compiling without ECC support
>> and
>> the server sends an unexpected ECDSA signature, it would be imported
>> successfully, "if (key->type == SSH_KEYTYPE_ECDSA)" would be true,
>> but
>> the block does nothing. The function returns rc, which is still 0
>> from
>> the import. Maybe I missed something, but this looks to me like a
>> signature verification bypass. We should return SSH_ERROR if ECC is
>> unsupported.
> 
> I think you are right here. Setting the rc = SSH_ERROR in the else
> branch should fix this problem, but there might be more issues like
> this throughout the code.
> 
> Would you like to submit a patch for these two issues?
> 
> Thanks,
> 

From 463e7cd33561ccbf59570da662ba02fd7279731c Mon Sep 17 00:00:00 2001
From: Tilo Eckert <tilo.eckert@xxxxxxx>
Date: Tue, 13 Nov 2018 15:45:47 +0100
Subject: [PATCH 1/7] pki: Add hostkey type validation function

Signed-off-by: Tilo Eckert <tilo.eckert@xxxxxxx>
---
 include/libssh/pki.h |  1 +
 src/pki.c            | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+)

diff --git a/include/libssh/pki.h b/include/libssh/pki.h
index 241cfdd1..af56f11a 100644
--- a/include/libssh/pki.h
+++ b/include/libssh/pki.h
@@ -140,4 +140,5 @@ ssh_public_key ssh_pki_convert_key_to_publickey(const ssh_key key);
 ssh_private_key ssh_pki_convert_key_to_privatekey(const ssh_key key);
 
 int ssh_key_algorithm_allowed(ssh_session session, const char *type);
+int ssh_hostkey_algorithm_allowed(ssh_session session, const char *type);
 #endif /* PKI_H_ */
diff --git a/src/pki.c b/src/pki.c
index 623c70f2..232327fa 100644
--- a/src/pki.c
+++ b/src/pki.c
@@ -296,6 +296,43 @@ int ssh_key_algorithm_allowed(ssh_session session, const char *type)
     return ssh_match_group(allowed_list, type);
 }
 
+/**
+ * @brief Checks the given host key against the configured allowed
+ * host key algorithm types
+ *
+ * @param[in] session The SSH session
+ * @param[in] type    The host key algorithm to check
+ * @returns           1 if the host key algorithm is allowed, 0 otherwise
+ */
+int ssh_hostkey_algorithm_allowed(ssh_session session, const char *type)
+{
+    const char *allowed_types;
+
+    allowed_types = session->opts.wanted_methods[SSH_HOSTKEYS];
+    if (allowed_types == NULL) {
+       return 0; /* should never get here */
+    }
+
+    if (ssh_match_group(allowed_types, type)) {
+        return 1;
+    }
+    if (strcmp(type, "ssh-rsa") == 0) {
+        /*
+         * If rsa-sha2 hostkey algo is used, the server returns ssh-rsa as
+         * host key type. Hence, we need to check if one of the rsa-sha2
+         * variants is in the allowed hostkey types list.
+         */
+        if (ssh_match_group(allowed_types, "rsa-sha2-256")) {
+            return 1;
+        }
+        if (ssh_match_group(allowed_types, "rsa-sha2-512")) {
+            return 1;
+        }
+    }
+
+    return 0;
+}
+
 /**
  * @brief Convert a key type to a hash type. This is usually unambiguous
  * for all the key types, unless the SHA2 extension (RFC 8332) is
-- 
2.18.0

From 29e5a998615b43090b3b72655022bd41a9ae41c2 Mon Sep 17 00:00:00 2001
From: Tilo Eckert <tilo.eckert@xxxxxxx>
Date: Tue, 13 Nov 2018 15:49:48 +0100
Subject: [PATCH 2/7] packet: Use new hostkey type validation function

The new function accepts hostkeys of type ssh-rsa
if rsa-sha2-256/512 is the only accepted hostkey type

Signed-off-by: Tilo Eckert <tilo.eckert@xxxxxxx>
---
 src/packet_cb.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/src/packet_cb.c b/src/packet_cb.c
index af5b966c..784ca33d 100644
--- a/src/packet_cb.c
+++ b/src/packet_cb.c
@@ -190,8 +190,7 @@ SSH_PACKET_CALLBACK(ssh_packet_newkeys){
 
     /* check if public key from server matches user preferences */
     if (session->opts.wanted_methods[SSH_HOSTKEYS]) {
-        if(!ssh_match_group(session->opts.wanted_methods[SSH_HOSTKEYS],
-                            server_key->type_c)) {
+        if (!ssh_hostkey_algorithm_allowed(session, server_key->type_c)) {
             ssh_set_error(session,
                           SSH_FATAL,
                           "Public key from server (%s) doesn't match user "
-- 
2.18.0

From 8327c3598c35b3834769c822b2849570eb778e30 Mon Sep 17 00:00:00 2001
From: Tilo Eckert <tilo.eckert@xxxxxxx>
Date: Tue, 13 Nov 2018 15:50:48 +0100
Subject: [PATCH 3/7] tests: Fix rsa-sha2 tests

Signed-off-by: Tilo Eckert <tilo.eckert@xxxxxxx>
---
 tests/client/torture_hostkey.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/client/torture_hostkey.c b/tests/client/torture_hostkey.c
index 928137d5..a930a293 100644
--- a/tests/client/torture_hostkey.c
+++ b/tests/client/torture_hostkey.c
@@ -163,7 +163,7 @@ static void torture_hostkey_ecdsa(void **state) {
 static void torture_hostkey_rsa_sha256(void **state) {
     struct torture_state *s = *state;
     ssh_session session = s->ssh.session;
-    char rsa[] = "rsa-sha2-256,ssh-rsa";
+    char rsa[] = "rsa-sha2-256";
 
     int rc;
 
@@ -182,7 +182,7 @@ static void torture_hostkey_rsa_sha256(void **state) {
 static void torture_hostkey_rsa_sha512(void **state) {
     struct torture_state *s = *state;
     ssh_session session = s->ssh.session;
-    char rsa[] = "rsa-sha2-512,ssh-rsa";
+    char rsa[] = "rsa-sha2-512";
 
     int rc;
 
-- 
2.18.0

From 2daf48ca2c16edf53f7f891e0b4424a8448afdd4 Mon Sep 17 00:00:00 2001
From: Tilo Eckert <tilo.eckert@xxxxxxx>
Date: Tue, 20 Nov 2018 10:22:33 +0100
Subject: [PATCH 4/7] pki: Return SSH_ERROR if ECC is disabled instead of
 accepting unverified signature

Signed-off-by: Tilo Eckert <tilo.eckert@xxxxxxx>
---
 src/pki.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/pki.c b/src/pki.c
index 232327fa..2fa053d4 100644
--- a/src/pki.c
+++ b/src/pki.c
@@ -1992,6 +1992,8 @@ int ssh_pki_signature_verify_blob(ssh_session session,
                                   key,
                                   ehash,
                                   elen);
+#else
+        rc = SSH_ERROR;
 #endif
     } else if (key->type == SSH_KEYTYPE_ED25519) {
         rc = pki_signature_verify(session, sig, key, digest, dlen);
-- 
2.18.0

From 18ff296ba6eed8f897fdae7e42cd1effe3006f8d Mon Sep 17 00:00:00 2001
From: Tilo Eckert <tilo.eckert@xxxxxxx>
Date: Wed, 21 Nov 2018 10:56:14 +0100
Subject: [PATCH 5/7] pki: Move signature import to callers of
 ssh_pki_signature_verify_blob()

Signed-off-by: Tilo Eckert <tilo.eckert@xxxxxxx>
---
 include/libssh/pki.h |  2 +-
 src/messages.c       | 23 ++++++++++++++++-------
 src/packet_cb.c      | 17 +++++++++++++----
 src/pki.c            | 11 +----------
 4 files changed, 31 insertions(+), 22 deletions(-)

diff --git a/include/libssh/pki.h b/include/libssh/pki.h
index af56f11a..f345e9f4 100644
--- a/include/libssh/pki.h
+++ b/include/libssh/pki.h
@@ -111,7 +111,7 @@ int ssh_pki_import_signature_blob(const ssh_string sig_blob,
                                   const ssh_key pubkey,
                                   ssh_signature *psig);
 int ssh_pki_signature_verify_blob(ssh_session session,
-                                  ssh_string sig_blob,
+                                  ssh_signature sig,
                                   const ssh_key key,
                                   unsigned char *digest,
                                   size_t dlen);
diff --git a/src/messages.c b/src/messages.c
index 9c3e0672..53651a93 100644
--- a/src/messages.c
+++ b/src/messages.c
@@ -730,6 +730,7 @@ static ssh_buffer ssh_msg_userauth_build_digest(ssh_session session,
  */
 SSH_PACKET_CALLBACK(ssh_packet_userauth_request){
   ssh_message msg = NULL;
+  ssh_signature sig = NULL;
   char *service = NULL;
   char *method = NULL;
   int cmp;
@@ -863,17 +864,25 @@ SSH_PACKET_CALLBACK(ssh_packet_userauth_request){
             goto error;
         }
 
-        rc = ssh_pki_signature_verify_blob(session,
-                                           sig_blob,
-                                           msg->auth_request.pubkey,
-                                           ssh_buffer_get(digest),
-                                           ssh_buffer_get_len(digest));
+        rc = ssh_pki_import_signature_blob(sig_blob, msg->auth_request.pubkey, &sig);
+        if (rc == SSH_OK) {
+            rc = ssh_pki_signature_verify_blob(session,
+                                               sig,
+                                               msg->auth_request.pubkey,
+                                               ssh_buffer_get(digest),
+                                               ssh_buffer_get_len(digest));
+        }
+
         ssh_string_free(sig_blob);
         ssh_buffer_free(digest);
+        ssh_signature_free(sig);
+        sig_blob = NULL;
+        digest = NULL;
+        sig = NULL;
         if (rc < 0) {
             SSH_LOG(
                     SSH_LOG_PACKET,
-                    "Received an invalid  signature from peer");
+                    "Received an invalid signature from peer");
             msg->auth_request.signature_state = SSH_PUBLICKEY_STATE_WRONG;
             goto error;
         }
@@ -1124,7 +1133,7 @@ SSH_PACKET_CALLBACK(ssh_packet_channel_open){
     ssh_set_error(session,SSH_FATAL, "Invalid state when receiving channel open request (must be authenticated)");
     goto error;
   }
-  
+
   if (strcmp(type_c,"session") == 0) {
     msg->channel_request_open.type = SSH_CHANNEL_SESSION;
     SAFE_FREE(type_c);
diff --git a/src/packet_cb.c b/src/packet_cb.c
index 784ca33d..1d70951e 100644
--- a/src/packet_cb.c
+++ b/src/packet_cb.c
@@ -138,6 +138,7 @@ error:
 
 SSH_PACKET_CALLBACK(ssh_packet_newkeys){
   ssh_string sig_blob = NULL;
+  ssh_signature sig = NULL;
   int rc;
   (void)packet;
   (void)user;
@@ -188,6 +189,13 @@ SSH_PACKET_CALLBACK(ssh_packet_newkeys){
         return SSH_ERROR;
     }
 
+    /* get the server signature */
+    rc = ssh_pki_import_signature_blob(sig_blob, server_key, &sig);
+    ssh_string_burn(sig_blob);
+    if (rc < 0) {
+        goto error;
+    }
+
     /* check if public key from server matches user preferences */
     if (session->opts.wanted_methods[SSH_HOSTKEYS]) {
         if (!ssh_hostkey_algorithm_allowed(session, server_key->type_c)) {
@@ -197,18 +205,19 @@ SSH_PACKET_CALLBACK(ssh_packet_newkeys){
                           "preference (%s)",
                           server_key->type_c,
                           session->opts.wanted_methods[SSH_HOSTKEYS]);
+            ssh_signature_free(sig);
+            sig = NULL;
             goto error;
         }
     }
 
     rc = ssh_pki_signature_verify_blob(session,
-                                       sig_blob,
+                                       sig,
                                        server_key,
                                        session->next_crypto->secret_hash,
                                        session->next_crypto->digest_len);
-    ssh_string_burn(sig_blob);
-    ssh_string_free(sig_blob);
-    sig_blob = NULL;
+    ssh_signature_free(sig);
+    sig = NULL;
     if (rc == SSH_ERROR) {
       goto error;
     }
diff --git a/src/pki.c b/src/pki.c
index 2fa053d4..d78216bc 100644
--- a/src/pki.c
+++ b/src/pki.c
@@ -1957,24 +1957,17 @@ int ssh_pki_import_signature_blob(const ssh_string sig_blob,
 }
 
 int ssh_pki_signature_verify_blob(ssh_session session,
-                                  ssh_string sig_blob,
+                                  ssh_signature sig,
                                   const ssh_key key,
                                   unsigned char *digest,
                                   size_t dlen)
 {
-    ssh_signature sig;
     int rc;
 
-    rc = ssh_pki_import_signature_blob(sig_blob, key, &sig);
-    if (rc < 0) {
-        return SSH_ERROR;
-    }
-
     SSH_LOG(SSH_LOG_FUNCTIONS,
             "Going to verify a %s type signature",
             sig->type_c);
 
-
     if (key->type == SSH_KEYTYPE_ECDSA) {
 #if HAVE_ECC
         unsigned char ehash[EVP_DIGEST_LEN] = {0};
@@ -2039,8 +2032,6 @@ int ssh_pki_signature_verify_blob(ssh_session session,
                                   hlen);
     }
 
-    ssh_signature_free(sig);
-
     return rc;
 }
 
-- 
2.18.0

From e6f6d14e155ceafd51f5d0da67c8810044ad2d2d Mon Sep 17 00:00:00 2001
From: Tilo Eckert <tilo.eckert@xxxxxxx>
Date: Wed, 21 Nov 2018 11:03:59 +0100
Subject: [PATCH 6/7] pki: Rename ssh_pki_signature_verify_blob() to
 ssh_pki_signature_verify()

Signed-off-by: Tilo Eckert <tilo.eckert@xxxxxxx>
---
 include/libssh/pki.h | 10 +++++-----
 src/messages.c       | 10 +++++-----
 src/packet_cb.c      | 10 +++++-----
 src/pki.c            | 10 +++++-----
 4 files changed, 20 insertions(+), 20 deletions(-)

diff --git a/include/libssh/pki.h b/include/libssh/pki.h
index f345e9f4..4839e5c1 100644
--- a/include/libssh/pki.h
+++ b/include/libssh/pki.h
@@ -110,11 +110,11 @@ int ssh_pki_export_signature_blob(const ssh_signature sign,
 int ssh_pki_import_signature_blob(const ssh_string sig_blob,
                                   const ssh_key pubkey,
                                   ssh_signature *psig);
-int ssh_pki_signature_verify_blob(ssh_session session,
-                                  ssh_signature sig,
-                                  const ssh_key key,
-                                  unsigned char *digest,
-                                  size_t dlen);
+int ssh_pki_signature_verify(ssh_session session,
+                             ssh_signature sig,
+                             const ssh_key key,
+                             unsigned char *digest,
+                             size_t dlen);
 
 /* SSH Public Key Functions */
 int ssh_pki_export_pubkey_blob(const ssh_key key,
diff --git a/src/messages.c b/src/messages.c
index 53651a93..437494ce 100644
--- a/src/messages.c
+++ b/src/messages.c
@@ -866,11 +866,11 @@ SSH_PACKET_CALLBACK(ssh_packet_userauth_request){
 
         rc = ssh_pki_import_signature_blob(sig_blob, msg->auth_request.pubkey, &sig);
         if (rc == SSH_OK) {
-            rc = ssh_pki_signature_verify_blob(session,
-                                               sig,
-                                               msg->auth_request.pubkey,
-                                               ssh_buffer_get(digest),
-                                               ssh_buffer_get_len(digest));
+            rc = ssh_pki_signature_verify(session,
+                                          sig,
+                                          msg->auth_request.pubkey,
+                                          ssh_buffer_get(digest),
+                                          ssh_buffer_get_len(digest));
         }
 
         ssh_string_free(sig_blob);
diff --git a/src/packet_cb.c b/src/packet_cb.c
index 1d70951e..c7a878bd 100644
--- a/src/packet_cb.c
+++ b/src/packet_cb.c
@@ -211,11 +211,11 @@ SSH_PACKET_CALLBACK(ssh_packet_newkeys){
         }
     }
 
-    rc = ssh_pki_signature_verify_blob(session,
-                                       sig,
-                                       server_key,
-                                       session->next_crypto->secret_hash,
-                                       session->next_crypto->digest_len);
+    rc = ssh_pki_signature_verify(session,
+                                  sig,
+                                  server_key,
+                                  session->next_crypto->secret_hash,
+                                  session->next_crypto->digest_len);
     ssh_signature_free(sig);
     sig = NULL;
     if (rc == SSH_ERROR) {
diff --git a/src/pki.c b/src/pki.c
index d78216bc..a72e8f49 100644
--- a/src/pki.c
+++ b/src/pki.c
@@ -1956,11 +1956,11 @@ int ssh_pki_import_signature_blob(const ssh_string sig_blob,
     return SSH_OK;
 }
 
-int ssh_pki_signature_verify_blob(ssh_session session,
-                                  ssh_signature sig,
-                                  const ssh_key key,
-                                  unsigned char *digest,
-                                  size_t dlen)
+int ssh_pki_signature_verify(ssh_session session,
+                             ssh_signature sig,
+                             const ssh_key key,
+                             unsigned char *digest,
+                             size_t dlen)
 {
     int rc;
 
-- 
2.18.0

From 319325fefc8322a4194308c0bdd7f48f8a19e15a Mon Sep 17 00:00:00 2001
From: Tilo Eckert <tilo.eckert@xxxxxxx>
Date: Wed, 21 Nov 2018 11:09:21 +0100
Subject: [PATCH 7/7] pki: Prevent downgrade to SHA-1 if rsa-sha2 hostkeys are
 allowed, but not ssh-rsa

Signed-off-by: Tilo Eckert <tilo.eckert@xxxxxxx>
---
 include/libssh/pki.h |  5 ++++-
 src/packet_cb.c      |  4 ++--
 src/pki.c            | 44 +++++++++++++++++++++++++++-----------------
 3 files changed, 33 insertions(+), 20 deletions(-)

diff --git a/include/libssh/pki.h b/include/libssh/pki.h
index 4839e5c1..281ff6c3 100644
--- a/include/libssh/pki.h
+++ b/include/libssh/pki.h
@@ -140,5 +140,8 @@ ssh_public_key ssh_pki_convert_key_to_publickey(const ssh_key key);
 ssh_private_key ssh_pki_convert_key_to_privatekey(const ssh_key key);
 
 int ssh_key_algorithm_allowed(ssh_session session, const char *type);
-int ssh_hostkey_algorithm_allowed(ssh_session session, const char *type);
+int ssh_hostkey_algorithm_allowed(ssh_session session,
+                                  const char *type,
+                                  enum ssh_keytypes_e sig_key_type,
+                                  enum ssh_digest_e sig_hash_type);
 #endif /* PKI_H_ */
diff --git a/src/packet_cb.c b/src/packet_cb.c
index c7a878bd..79462a11 100644
--- a/src/packet_cb.c
+++ b/src/packet_cb.c
@@ -196,9 +196,9 @@ SSH_PACKET_CALLBACK(ssh_packet_newkeys){
         goto error;
     }
 
-    /* check if public key from server matches user preferences */
+    /* check if public key type and signature type from server match user preferences */
     if (session->opts.wanted_methods[SSH_HOSTKEYS]) {
-        if (!ssh_hostkey_algorithm_allowed(session, server_key->type_c)) {
+        if (!ssh_hostkey_algorithm_allowed(session, server_key->type_c, sig->type, sig->hash_type)) {
             ssh_set_error(session,
                           SSH_FATAL,
                           "Public key from server (%s) doesn't match user "
diff --git a/src/pki.c b/src/pki.c
index a72e8f49..183e8b1a 100644
--- a/src/pki.c
+++ b/src/pki.c
@@ -300,35 +300,45 @@ int ssh_key_algorithm_allowed(ssh_session session, const char *type)
  * @brief Checks the given host key against the configured allowed
  * host key algorithm types
  *
- * @param[in] session The SSH session
- * @param[in] type    The host key algorithm to check
- * @returns           1 if the host key algorithm is allowed, 0 otherwise
+ * @param[in] session       The SSH session
+ * @param[in] type          The host key algorithm to check
+ * @param[in] sig_key_type  The key algorithm in the signature
+ * @param[in] sig_hash_type The hash algorithm in the signature
+ * @returns                 1 if the host key algorithm is allowed, 0 otherwise
  */
-int ssh_hostkey_algorithm_allowed(ssh_session session, const char *type)
+int ssh_hostkey_algorithm_allowed(ssh_session session,
+                                  const char *type,
+                                  enum ssh_keytypes_e sig_key_type,
+                                  enum ssh_digest_e sig_hash_type)
 {
     const char *allowed_types;
 
     allowed_types = session->opts.wanted_methods[SSH_HOSTKEYS];
     if (allowed_types == NULL) {
-       return 0; /* should never get here */
+        return 0; /* should never get here */
     }
 
-    if (ssh_match_group(allowed_types, type)) {
-        return 1;
-    }
     if (strcmp(type, "ssh-rsa") == 0) {
-        /*
-         * If rsa-sha2 hostkey algo is used, the server returns ssh-rsa as
-         * host key type. Hence, we need to check if one of the rsa-sha2
-         * variants is in the allowed hostkey types list.
-         */
-        if (ssh_match_group(allowed_types, "rsa-sha2-256")) {
-            return 1;
+       /*
+        * If rsa-sha2 hostkey algo is used, the server returns "ssh-rsa" as
+        * host key type, but an "rsa-sha2-256" or "rsa-sha2-512" signature type.
+        */
+        if (sig_key_type != SSH_KEYTYPE_RSA) {
+            return 0;
         }
-        if (ssh_match_group(allowed_types, "rsa-sha2-512")) {
-            return 1;
+        if (sig_hash_type == SSH_DIGEST_SHA256) {
+            /* server sent us an "rsa-sha2-256" hostkey, is it allowed? */
+           return ssh_match_group(allowed_types, "rsa-sha2-256");
+        } else if (sig_hash_type == SSH_DIGEST_SHA512) {
+            /* server sent us an "rsa-sha2-256" hostkey, is it allowed? */
+            return ssh_match_group(allowed_types, "rsa-sha2-512");
+        } else if (sig_hash_type == SSH_DIGEST_SHA1 || sig_hash_type == SSH_DIGEST_AUTO) {
+            return ssh_match_group(allowed_types, "ssh-rsa");
         }
     }
+    if (ssh_match_group(allowed_types, type)) {
+        return 1;
+    }
 
     return 0;
 }
-- 
2.18.0