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

[Tilo Eckert <tilo.eckert@xxxxxxx>]

Hello Jakub,

the solution looks fine to me except for the assignment
sig->type_c = pubkey->type_c

The new test kind of proves my point that we should set it to the actual
sig type string that the server sends to us. The second inner for loop
successfully imported any signature blob with SSH_DIGEST_SHA1, even if
it was a SHA2 signature. The asserts incorrectly accepted that because
the public key type is ssh-rsa in both cases.

The two attached patches contain the necessary refactorings for your
branch and fix a few typos.

Regards,
Tilo

Am 26.11.2018 um 19:39 schrieb Jakub Jelen:
> Hello Tilo,
> Thank you for the valuable comments. See the comments inline.
> 
> On Mon, 2018-11-26 at 10:58 +0100, Tilo Eckert wrote:
>> Hello Jakub,
>>
>> I explored a similar solution to yours as an alternative, but
>> encountered the same ECDSA issue you mentioned which is caused by the
>> weird way that sig->type_c is set. It didn't make much sense to me
>> that
>> ssh_pki_import_signature_blob() reads the signature key type string
>> from
>> the signature blob, converts it to key type and hash type enums, only
>> to
>> convert them back to a key type string in pki_signature_from_blob(),
>> which does not yield the same string the server sent in case of
>> ECDSA.
> 
> I assume this is based on the history, when it was very easy to have
> type_c representation of the key type as a const string (either RSA or
> DSA). This approach gets more complicated when we have to deal with
> ECDSA keys and with the new SHA2 extension. It looks like the ECDSA
> implementation was not very tested so far. So lets fix it.
> 
>> The assignment "sig->type_c = pubkey->type_c" in your solution
>> troubles
>> me because the key type string from the public key and signature
>> portion
>> of the KEX message sent by the server are not the same string.
> 
> They are the same string for everything but RSA keys with SHA2
> extension. There is similar approach used for the ECDSA key import
> (setting default for all key types and overwriting for ECDSA based on
> the NID). But you are right, we should check first that the key matches
> base type of the signature before doing this. Added to my branch.
> 
>> If
>> anything, sig->type_c should be set to the value of variable "alg" in
>> ssh_pki_import_signature_blob(), which contains the actual key type
>> string the server included in its signature.
> 
> This would be also good solution, and it was also one of the
> possibilities that I was exploring initially, but the problem is that
> the string in type_c is constant and we would not be able to free it,
> unless we would do a lot of refactoring around, which I wanted to
> avoid.
> 
>> Instead of replacing the "ssh_match_group(..., server_key->type_c)"
>> check with "ssh_match_group(..., sig->type_c)", my alternative
>> solution
>> involved checking sig->type_c in addition to the verification of
>> server_key->type_c (as in my previous patch set).
> 
> I was also considering this check, but in the end decided it would be
> bogus -- the signature verification should not really pass if there
> would be key/signature mismatch, unless there would go something
> terribly wrong.
> 
> But it is always be safe than sorry. See below.
> 
>> I was thinking about
>> the case when a bad server sends incompatible server_key->type_c and
>> sig->type_c (e.g. server sends a public key with type "ssh-dss", but
>> the
>> key type string in the signature is "ssh-rsa").
> 
> This test should go to ssh_pki_signature_verify() to catch the same
> possible issue also in other places where we are validating signatures.
> And since I am "deriving" the signature type from the key type, the
> same check should also to the pki_signature_from_blob(), verifying that
> the base type of public key matches the received type of signature,
> before we assign the type_c from the public key to the signature.
> 
> This can be hardly tested with the real ssh session, but the unit tests
> should be able to catch it. I wrote a test to go through all the key
> types, try the import of non-matching signatures and verification of
> already created and imported keys and really, this could have crash
> badly.
> 
> I added few more checks and now the flags look sane and the cross-
> verify fails when it should.
> 
> I added these commits to my branch. Does this address your concerns?
> 
> https://gitlab.com/jjelen/libssh-mirror/commits/rsa-sha2-bug
> 
> Thank you,
> Jakub
> 

From f3942632e6f9ec7cea4e7aa25f8f72dc284a19d3 Mon Sep 17 00:00:00 2001
From: Tilo Eckert <tilo.eckert@xxxxxxx>
Date: Tue, 27 Nov 2018 16:39:50 +0100
Subject: [PATCH 1/2] src: Fix multiple typos

Signed-off-by: Tilo Eckert <tilo.eckert@xxxxxxx>
---
 src/config.c                  | 2 +-
 src/libcrypto.c               | 2 +-
 src/pki.c                     | 2 +-
 src/pki_crypto.c              | 2 +-
 src/pki_gcrypt.c              | 2 +-
 src/pki_mbedcrypto.c          | 2 +-
 tests/unittests/torture_pki.c | 4 ++--
 7 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/src/config.c b/src/config.c
index a2c1cc45..7461d7d9 100644
--- a/src/config.c
+++ b/src/config.c
@@ -510,7 +510,7 @@ static int ssh_config_parse_line(ssh_session session, const char *line,
                 }
 
                 ssh_set_error(session, SSH_FATAL,
-                              "line %d: ERROR - Match all can not be combined with "
+                              "line %d: ERROR - Match all cannot be combined with "
                               "other Match attributes", count);
                 SAFE_FREE(x);
                 return -1;
diff --git a/src/libcrypto.c b/src/libcrypto.c
index 9c482aa0..d1f93978 100644
--- a/src/libcrypto.c
+++ b/src/libcrypto.c
@@ -519,7 +519,7 @@ static void evp_cipher_init(struct ssh_cipher_struct *cipher) {
         break;
         /* ciphers not using EVP */
     case SSH_AEAD_CHACHA20_POLY1305:
-        SSH_LOG(SSH_LOG_WARNING, "The ChaCha cipher can not be handled here");
+        SSH_LOG(SSH_LOG_WARNING, "The ChaCha cipher cannot be handled here");
         break;
     case SSH_NO_CIPHER:
         SSH_LOG(SSH_LOG_WARNING, "No valid ciphertype found");
diff --git a/src/pki.c b/src/pki.c
index debe2b7f..f866f0fe 100644
--- a/src/pki.c
+++ b/src/pki.c
@@ -1933,7 +1933,7 @@ int ssh_pki_signature_verify(ssh_session session,
             sig->type_c);
 
     if (key->type != sig->type) {
-        SSH_LOG(SSH_LOG_WARN, "Can not verify %s signature with %s key",
+        SSH_LOG(SSH_LOG_WARN, "Cannot verify %s signature with %s key",
                 sig->type_c, key->type_c);
         return SSH_ERROR;
     }
diff --git a/src/pki_crypto.c b/src/pki_crypto.c
index 46bb859b..811a6c9e 100644
--- a/src/pki_crypto.c
+++ b/src/pki_crypto.c
@@ -1796,7 +1796,7 @@ int pki_signature_verify(ssh_session session,
     int nid;
 
     if (key->type != sig->type) {
-        SSH_LOG(SSH_LOG_WARN, "Can not verify %s signature with %s key",
+        SSH_LOG(SSH_LOG_WARN, "Cannot verify %s signature with %s key",
                 sig->type_c, key->type_c);
         return SSH_ERROR;
     }
diff --git a/src/pki_gcrypt.c b/src/pki_gcrypt.c
index 6e239615..b2ed66e2 100644
--- a/src/pki_gcrypt.c
+++ b/src/pki_gcrypt.c
@@ -2033,7 +2033,7 @@ int pki_signature_verify(ssh_session session,
     gcry_error_t err;
 
     if (key->type != sig->type) {
-        SSH_LOG(SSH_LOG_WARN, "Can not verify %s signature with %s key",
+        SSH_LOG(SSH_LOG_WARN, "Cannot verify %s signature with %s key",
                 sig->type_c, key->type_c);
         return SSH_ERROR;
     }
diff --git a/src/pki_mbedcrypto.c b/src/pki_mbedcrypto.c
index 3f739696..b17de492 100644
--- a/src/pki_mbedcrypto.c
+++ b/src/pki_mbedcrypto.c
@@ -1007,7 +1007,7 @@ int pki_signature_verify(ssh_session session, const ssh_signature sig, const
     mbedtls_md_type_t md = 0;
 
     if (key->type != sig->type) {
-        SSH_LOG(SSH_LOG_WARN, "Can not verify %s signature with %s key",
+        SSH_LOG(SSH_LOG_WARN, "Cannot verify %s signature with %s key",
                 sig->type_c, key->type_c);
         return SSH_ERROR;
     }
diff --git a/tests/unittests/torture_pki.c b/tests/unittests/torture_pki.c
index f98a3d3b..8269bbb8 100644
--- a/tests/unittests/torture_pki.c
+++ b/tests/unittests/torture_pki.c
@@ -210,7 +210,7 @@ static void torture_pki_verify_mismatch(void **state)
             assert_true(rc == SSH_OK);
             assert_true(verify_key != NULL);
 
-            /* Should gradefully fail, but not crash */
+            /* Should gracefully fail, but not crash */
             rc = pki_signature_verify(session,
                                       sign,
                                       verify_key,
@@ -240,7 +240,7 @@ static void torture_pki_verify_mismatch(void **state)
                 assert_string_equal(new_sig->type_c, key->type_c);
                 assert_string_equal(new_sig->type_c, signature_types[sig_type]);
 
-                /* The verificaiton should not work */
+                /* The verification should not work */
                 rc = pki_signature_verify(session,
                                           new_sig,
                                           verify_key,
-- 
2.18.0

From ece95f8e017893226221771780e7c42102e9e91a Mon Sep 17 00:00:00 2001
From: Tilo Eckert <tilo.eckert@xxxxxxx>
Date: Tue, 27 Nov 2018 17:00:26 +0100
Subject: [PATCH 2/2] pki: Store the actual server signature type string in
 sig->type_c

pki_signature_from_blob() now accepts the signature type string as
parameter and stores a copy of the string sent by the server in the
signature struct instead of indirectly inferring it.

A flag is introduced to the signature struct so that the signature
type string can be free()d in ssh_signature_free() if it is heap memory.

Signed-off-by: Tilo Eckert <tilo.eckert@xxxxxxx>
---
 include/libssh/pki.h                  |  3 +++
 include/libssh/pki_priv.h             |  3 ++-
 src/pki.c                             |  8 ++++++--
 src/pki_crypto.c                      | 11 ++++++++---
 src/pki_gcrypt.c                      | 11 ++++++++---
 src/pki_mbedcrypto.c                  | 11 ++++++++---
 tests/unittests/torture_pki.c         | 15 +++++++++++----
 tests/unittests/torture_pki_ed25519.c |  4 ++--
 8 files changed, 48 insertions(+), 18 deletions(-)

diff --git a/include/libssh/pki.h b/include/libssh/pki.h
index 17c142c1..28c4473d 100644
--- a/include/libssh/pki.h
+++ b/include/libssh/pki.h
@@ -67,9 +67,12 @@ struct ssh_key_struct {
     enum ssh_keytypes_e cert_type;
 };
 
+#define SSH_SIGNATURE_FLAG_TYPEC_HEAP  0x0001 /* type_c is heap memory */
+
 struct ssh_signature_struct {
     enum ssh_keytypes_e type;
     enum ssh_digest_e hash_type;
+    int flags;
     const char *type_c;
 #if defined(HAVE_LIBGCRYPT)
     gcry_sexp_t dsa_sig;
diff --git a/include/libssh/pki_priv.h b/include/libssh/pki_priv.h
index 30af8d86..07a6ff69 100644
--- a/include/libssh/pki_priv.h
+++ b/include/libssh/pki_priv.h
@@ -113,7 +113,8 @@ ssh_string pki_signature_to_blob(const ssh_signature sign);
 ssh_signature pki_signature_from_blob(const ssh_key pubkey,
                                       const ssh_string sig_blob,
                                       enum ssh_keytypes_e type,
-                                      enum ssh_digest_e hash_type);
+                                      enum ssh_digest_e hash_type,
+                                      const char* type_c);
 int pki_signature_verify(ssh_session session,
                          const ssh_signature sig,
                          const ssh_key key,
diff --git a/src/pki.c b/src/pki.c
index f866f0fe..8169e12c 100644
--- a/src/pki.c
+++ b/src/pki.c
@@ -536,6 +536,9 @@ void ssh_signature_free(ssh_signature sig)
         case SSH_KEYTYPE_UNKNOWN:
             break;
     }
+    if ((sig->flags & SSH_SIGNATURE_FLAG_TYPEC_HEAP) && sig->type_c != NULL) {
+        free((void*)sig->type_c);
+    }
 
     SAFE_FREE(sig);
 }
@@ -1902,16 +1905,17 @@ int ssh_pki_import_signature_blob(const ssh_string sig_blob,
     alg = ssh_string_get_char(algorithm);
     type = ssh_key_type_from_signature_name(alg);
     hash_type = ssh_key_hash_from_name(alg);
-    ssh_string_free(algorithm);
 
     blob = ssh_buffer_get_ssh_string(buf);
     ssh_buffer_free(buf);
     if (blob == NULL) {
+        ssh_string_free(algorithm);
         return SSH_ERROR;
     }
 
-    sig = pki_signature_from_blob(pubkey, blob, type, hash_type);
+    sig = pki_signature_from_blob(pubkey, blob, type, hash_type, alg);
     ssh_string_free(blob);
+    ssh_string_free(algorithm);
     if (sig == NULL) {
         return SSH_ERROR;
     }
diff --git a/src/pki_crypto.c b/src/pki_crypto.c
index 811a6c9e..5f5a1a58 100644
--- a/src/pki_crypto.c
+++ b/src/pki_crypto.c
@@ -1592,7 +1592,8 @@ errout:
 ssh_signature pki_signature_from_blob(const ssh_key pubkey,
                                       const ssh_string sig_blob,
                                       enum ssh_keytypes_e type,
-                                      enum ssh_digest_e hash_type)
+                                      enum ssh_digest_e hash_type,
+                                      const char* type_c)
 {
     ssh_signature sig;
     ssh_string r;
@@ -1614,7 +1615,12 @@ ssh_signature pki_signature_from_blob(const ssh_key pubkey,
 
     sig->type = type;
     sig->hash_type = hash_type;
-    sig->type_c = pubkey->type_c; /* for all types but RSA */
+    sig->flags = SSH_SIGNATURE_FLAG_TYPEC_HEAP;
+    sig->type_c = strdup(type_c);
+    if (sig->type_c == NULL) {
+        ssh_signature_free(sig);
+        return NULL;
+    }
 
     len = ssh_string_len(sig_blob);
 
@@ -1680,7 +1686,6 @@ ssh_signature pki_signature_from_blob(const ssh_key pubkey,
         case SSH_KEYTYPE_RSA:
         case SSH_KEYTYPE_RSA1:
             sig = pki_signature_from_rsa_blob(pubkey, sig_blob, sig);
-            sig->type_c = ssh_key_signature_to_char(type, hash_type);
             break;
         case SSH_KEYTYPE_ECDSA:
 #ifdef HAVE_OPENSSL_ECC
diff --git a/src/pki_gcrypt.c b/src/pki_gcrypt.c
index b2ed66e2..c659eeef 100644
--- a/src/pki_gcrypt.c
+++ b/src/pki_gcrypt.c
@@ -1840,7 +1840,8 @@ ssh_string pki_signature_to_blob(const ssh_signature sig)
 ssh_signature pki_signature_from_blob(const ssh_key pubkey,
                                       const ssh_string sig_blob,
                                       enum ssh_keytypes_e type,
-                                      enum ssh_digest_e hash_type)
+                                      enum ssh_digest_e hash_type,
+                                      const char* type_c)
 {
     ssh_signature sig;
     gcry_error_t err;
@@ -1861,7 +1862,12 @@ ssh_signature pki_signature_from_blob(const ssh_key pubkey,
 
     sig->type = type;
     sig->hash_type = hash_type;
-    sig->type_c = pubkey->type_c; /* for all types but RSA */
+    sig->flags = SSH_SIGNATURE_FLAG_TYPEC_HEAP;
+    sig->type_c = strdup(type_c);
+    if (sig->type_c == NULL) {
+        ssh_signature_free(sig);
+        return NULL;
+    }
 
     len = ssh_string_len(sig_blob);
 
@@ -1927,7 +1933,6 @@ ssh_signature pki_signature_from_blob(const ssh_key pubkey,
                 ssh_signature_free(sig);
                 return NULL;
             }
-            sig->type_c = ssh_key_signature_to_char(type, hash_type);
             break;
         case SSH_KEYTYPE_ED25519:
 		rc = pki_ed25519_sig_from_blob(sig, sig_blob);
diff --git a/src/pki_mbedcrypto.c b/src/pki_mbedcrypto.c
index b17de492..cdcb121b 100644
--- a/src/pki_mbedcrypto.c
+++ b/src/pki_mbedcrypto.c
@@ -892,7 +892,8 @@ errout:
 ssh_signature pki_signature_from_blob(const ssh_key pubkey,
                                       const ssh_string sig_blob,
                                       enum ssh_keytypes_e type,
-                                      enum ssh_digest_e hash_type)
+                                      enum ssh_digest_e hash_type,
+                                      const char* type_c)
 {
     ssh_signature sig = NULL;
     int rc;
@@ -910,12 +911,16 @@ ssh_signature pki_signature_from_blob(const ssh_key pubkey,
 
     sig->type = type;
     sig->hash_type = hash_type;
-    sig->type_c = pubkey->type_c; /* for all types but RSA */
+    sig->flags = SSH_SIGNATURE_FLAG_TYPEC_HEAP;
+    sig->type_c = strdup(type_c);
+    if (sig->type_c == NULL) {
+        ssh_signature_free(sig);
+        return NULL;
+    }
 
     switch(type) {
         case SSH_KEYTYPE_RSA:
             sig = pki_signature_from_rsa_blob(pubkey, sig_blob, sig);
-            sig->type_c = ssh_key_signature_to_char(type, hash_type);
             break;
         case SSH_KEYTYPE_ECDSA: {
             ssh_buffer b;
diff --git a/tests/unittests/torture_pki.c b/tests/unittests/torture_pki.c
index 8269bbb8..82260e85 100644
--- a/tests/unittests/torture_pki.c
+++ b/tests/unittests/torture_pki.c
@@ -167,7 +167,8 @@ static void torture_pki_verify_mismatch(void **state)
             import_sig = pki_signature_from_blob(key,
                                                  blob,
                                                  sig_type,
-                                                 hash);
+                                                 hash,
+                                                 sign->type_c);
             assert_non_null(import_sig);
             assert_int_equal(import_sig->type, key->type);
             if (hash == SSH_DIGEST_AUTO) {
@@ -230,15 +231,21 @@ static void torture_pki_verify_mismatch(void **state)
             new_sig = pki_signature_from_blob(verify_key,
                                               blob,
                                               sig_type,
-                                              SSH_DIGEST_SHA1);
+                                              import_sig->hash_type,
+                                              import_sig->type_c);
             if (sig_type != key_type) {
                 assert_true(new_sig == NULL);
             } else {
                 /* Importing with the same key type should work */
                 assert_true(new_sig != NULL);
                 assert_int_equal(new_sig->type, key->type);
-                assert_string_equal(new_sig->type_c, key->type_c);
-                assert_string_equal(new_sig->type_c, signature_types[sig_type]);
+                if (key_type == SSH_KEYTYPE_RSA) {
+                   assert_string_equal(key->type_c, "ssh-rsa");
+                   assert_string_equal(new_sig->type_c, hash_signatures[new_sig->hash_type]);
+                } else {
+                   assert_string_equal(new_sig->type_c, key->type_c);
+                   assert_string_equal(new_sig->type_c, signature_types[sig_type]);
+                }
 
                 /* The verification should not work */
                 rc = pki_signature_verify(session,
diff --git a/tests/unittests/torture_pki_ed25519.c b/tests/unittests/torture_pki_ed25519.c
index e97b426e..9e449cf0 100644
--- a/tests/unittests/torture_pki_ed25519.c
+++ b/tests/unittests/torture_pki_ed25519.c
@@ -467,7 +467,7 @@ static void torture_pki_ed25519_verify(void **state){
     assert_true(rc == SSH_OK);
 
     ssh_string_fill(blob, ref_signature, ED25519_SIG_LEN);
-    sig = pki_signature_from_blob(pubkey, blob, SSH_KEYTYPE_ED25519, SSH_DIGEST_AUTO);
+    sig = pki_signature_from_blob(pubkey, blob, SSH_KEYTYPE_ED25519, SSH_DIGEST_AUTO, ssh_key_type_to_char(SSH_KEYTYPE_ED25519));
     assert_true(sig != NULL);
 
     rc = pki_ed25519_verify(pubkey, sig, HASH, sizeof(HASH));
@@ -504,7 +504,7 @@ static void torture_pki_ed25519_verify_bad(void **state){
     for (i=0; i < ED25519_SIG_LEN; ++i){
         ssh_string_fill(blob, ref_signature, ED25519_SIG_LEN);
         ((uint8_t *)ssh_string_data(blob))[i] ^= 0xff;
-        sig = pki_signature_from_blob(pubkey, blob, SSH_KEYTYPE_ED25519, SSH_DIGEST_AUTO);
+        sig = pki_signature_from_blob(pubkey, blob, SSH_KEYTYPE_ED25519, SSH_DIGEST_AUTO, ssh_key_type_to_char(SSH_KEYTYPE_ED25519));
         assert_true(sig != NULL);
 
         rc = pki_ed25519_verify(pubkey, sig, HASH, sizeof(HASH));
-- 
2.18.0