TLS: really delay key lookup until it is necessary

Even if the certificate has a RSA public key, be sure to lookup the key
only if it is an actual RSA key exchange. Move the hashtable to the
secrets module to enable reuse.

Change-Id: I39010831079d3b65d5d4368ec97d02491c1615a5
Reviewed-on: https://code.wireshark.org/review/30854
Petri-Dish: Peter Wu <peter@lekensteyn.nl>
Tested-by: Petri Dish Buildbot
Reviewed-by: Anders Broman <a.broman58@gmail.com>

6 files changed, 76 insertions, 60 deletions

diff --git a/epan/dissectors/packet-dtls.c b/epan/dissectors/packet-dtls.c
index e1cfaf4109..1252acd55f 100644
--- a/epan/dissectors/packet-dtls.c
+++ b/epan/dissectors/packet-dtls.c
@@ -46,6 +46,7 @@
 #include <epan/exported_pdu.h>
 #include <epan/decode_as.h>
 #include <epan/proto_data.h>
+#include <epan/secrets.h>   /* for privkey_hash_table_new */
 #include <wsutil/str_util.h>
 #include <wsutil/strtoi.h>
 #include <wsutil/utf8_entities.h>
@@ -247,8 +248,7 @@ dtls_parse_uat(void)
   }
 
   /* parse private keys string, load available keys and put them in key hash*/
-  dtls_key_hash = g_hash_table_new_full(tls_private_key_hash,
-      tls_private_key_equal, g_free, tls_private_key_free);
+  dtls_key_hash = privkey_hash_table_new();
 
   ssl_set_debug(dtls_debug_file_name);
 
diff --git a/epan/dissectors/packet-tls-utils.c b/epan/dissectors/packet-tls-utils.c
index 7af38064a3..6e274ddda4 100644
--- a/epan/dissectors/packet-tls-utils.c
+++ b/epan/dissectors/packet-tls-utils.c
@@ -30,6 +30,7 @@
 #include <epan/asn1.h>
 #include <epan/proto_data.h>
 #include <epan/oids.h>
+#include <epan/secrets.h>
 
 #include <wsutil/filesystem.h>
 #include <wsutil/file_util.h>
@@ -3033,7 +3034,7 @@ end:
 static int
 ssl_decrypt_pre_master_secret(SslDecryptSession *ssl_session,
                               StringInfo *encrypted_pre_master,
-                              gnutls_privkey_t pk);
+                              GHashTable *key_hash);
 #endif /* HAVE_LIBGNUTLS */
 
 static gboolean
@@ -3163,12 +3164,8 @@ ssl_generate_pre_master_secret(SslDecryptSession *ssl_session,
 
 #ifdef HAVE_LIBGNUTLS
         /* Try to lookup an appropriate RSA private key to decrypt the Encrypted Pre-Master Secret. */
-        gnutls_privkey_t pk = NULL;
         if (ssl_session->cert_key_id) {
-            pk = (gnutls_privkey_t)g_hash_table_lookup(key_hash, ssl_session->cert_key_id);
-        }
-        if (pk) {
-            if (ssl_decrypt_pre_master_secret(ssl_session, &encrypted_pre_master, pk))
+            if (ssl_decrypt_pre_master_secret(ssl_session, &encrypted_pre_master, key_hash))
                 return TRUE;
 
             ssl_debug_printf("%s: can't decrypt pre-master secret\n",
@@ -3599,8 +3596,10 @@ end:
 /* Decrypt RSA pre-master secret using RSA private key. {{{ */
 static gboolean
 ssl_decrypt_pre_master_secret(SslDecryptSession *ssl_session,
-    StringInfo *encrypted_pre_master, gnutls_privkey_t pk)
+    StringInfo *encrypted_pre_master, GHashTable *key_hash)
 {
+    int ret;
+
     if (!encrypted_pre_master)
         return FALSE;
 
@@ -3618,12 +3617,18 @@ ssl_decrypt_pre_master_secret(SslDecryptSession *ssl_session,
         return FALSE;
     }
 
+    gnutls_privkey_t pk = (gnutls_privkey_t)g_hash_table_lookup(key_hash, ssl_session->cert_key_id);
+
     ssl_print_string("pre master encrypted", encrypted_pre_master);
     ssl_debug_printf("%s: RSA_private_decrypt\n", G_STRFUNC);
     const gnutls_datum_t epms = { encrypted_pre_master->data, encrypted_pre_master->data_len };
     gnutls_datum_t pms = { 0 };
-    // uses mechanism CKM_RSA_PKCS (corresponds to RSAES-PKCS1-v1_5)
-    int ret = gnutls_privkey_decrypt_data(pk, 0, &epms, &pms);
+    if (pk) {
+        // Try to decrypt using the RSA keys table from (D)TLS preferences.
+        ret = gnutls_privkey_decrypt_data(pk, 0, &epms, &pms);
+    } else {
+        ret = GNUTLS_E_NO_CERTIFICATE_FOUND;
+    }
     if (ret < 0) {
         ssl_debug_printf("%s: decryption failed: %d (%s)\n", G_STRFUNC, ret, gnutls_strerror(ret));
         return FALSE;
@@ -4267,7 +4272,7 @@ ssl_find_private_key_by_pubkey(SslDecryptSession *ssl,
                                gnutls_datum_t *subjectPublicKeyInfo)
 {
     gnutls_pubkey_t pubkey = NULL;
-    guchar key_id[20];
+    cert_key_id_t key_id;
     size_t key_id_len = sizeof(key_id);
     int r;
 
@@ -4296,7 +4301,7 @@ ssl_find_private_key_by_pubkey(SslDecryptSession *ssl,
     }
 
     /* Generate a 20-byte SHA-1 hash. */
-    r = gnutls_pubkey_get_key_id(pubkey, 0, key_id, &key_id_len);
+    r = gnutls_pubkey_get_key_id(pubkey, 0, key_id.key_id, &key_id_len);
     if (r < 0) {
         ssl_debug_printf("%s: failed to extract key id from pubkey: %s\n",
                 G_STRFUNC, gnutls_strerror(r));
@@ -4309,8 +4314,9 @@ ssl_find_private_key_by_pubkey(SslDecryptSession *ssl,
         goto end;
     }
 
-    ssl_print_data("Certificate.KeyID", key_id, key_id_len);
-    ssl->cert_key_id = (guint8 *)wmem_memdup(wmem_file_scope(), key_id, key_id_len);
+    ssl_print_data("Certificate.KeyID", key_id.key_id, key_id_len);
+    ssl->cert_key_id = wmem_new(wmem_file_scope(), cert_key_id_t);
+    *ssl->cert_key_id = key_id;
 
 end:
     gnutls_pubkey_deinit(pubkey);
@@ -4510,35 +4516,6 @@ ssl_hash  (gconstpointer v)
 
     return hash;
 }
-
-#ifdef HAVE_LIBGNUTLS
-gboolean
-tls_private_key_equal (gconstpointer v, gconstpointer v2)
-{
-    /* key ID length (SHA-1 hash, per GNUTLS_KEYID_USE_SHA1) */
-    return !memcmp(v, v2, 20);
-}
-
-guint
-tls_private_key_hash (gconstpointer v)
-{
-    guint        l, hash = 0;
-    const guint8 *cur = (const guint8 *)v;
-
-    /* The public key' SHA-1 hash (which maps to a private key) has a uniform
-     * distribution, hence simply xor'ing them should be sufficient. */
-    for (l = 0; l < 20; l += 4, cur += 4)
-        hash ^= pntoh32(cur);
-
-    return hash;
-}
-void
-tls_private_key_free(gpointer data)
-{
-    gnutls_privkey_t pkey = (gnutls_privkey_t)data;
-    gnutls_privkey_deinit(pkey);
-}
-#endif
 /* Functions for TLS/DTLS sessions and RSA private keys hashtables. }}} */
 
 /* Handling of association between tls/dtls ports and clear text protocol. {{{ */
diff --git a/epan/dissectors/packet-tls-utils.h b/epan/dissectors/packet-tls-utils.h
index 8697d494be..0bf7a76dc7 100644
--- a/epan/dissectors/packet-tls-utils.h
+++ b/epan/dissectors/packet-tls-utils.h
@@ -408,6 +408,8 @@ typedef struct _SslSession {
 /* RFC 5246, section 8.1 says that the master secret is always 48 bytes */
 #define SSL_MASTER_SECRET_LENGTH        48
 
+struct cert_key_id; /* defined in epan/secrets.h */
+
 /* This holds state information for a SSL conversation */
 typedef struct _SslDecryptSession {
     guchar _master_secret[SSL_MASTER_SECRET_LENGTH];
@@ -434,7 +436,7 @@ typedef struct _SslDecryptSession {
     SslDecoder *server_new;
     SslDecoder *client_new;
 #if defined(HAVE_LIBGNUTLS)
-    guint8     *cert_key_id;   /**< SHA-1 Key ID of public key in certificate. */
+    struct cert_key_id *cert_key_id;   /**< SHA-1 Key ID of public key in certificate. */
 #endif
     StringInfo psk;
     StringInfo app_data_segment;
@@ -611,19 +613,6 @@ tls13_cipher_create(const char *label_prefix, int cipher_algo, int cipher_mode,
 
 
 /* Common part between TLS and DTLS dissectors */
-/* Hash Functions for RSA private keys table */
-
-#ifdef HAVE_LIBGNUTLS
-extern gboolean
-tls_private_key_equal (gconstpointer v, gconstpointer v2);
-
-extern guint
-tls_private_key_hash  (gconstpointer v);
-
-extern void
-tls_private_key_free(gpointer key);
-#endif  /* HAVE_LIBGNUTLS */
-
 
 /* handling of association between tls/dtls ports and clear text protocol */
 extern void
diff --git a/epan/dissectors/packet-tls.c b/epan/dissectors/packet-tls.c
index 32e5f77206..28632dcbd8 100644
--- a/epan/dissectors/packet-tls.c
+++ b/epan/dissectors/packet-tls.c
@@ -336,8 +336,7 @@ ssl_parse_uat(void)
         }
     }
     /* parse private keys string, load available keys and put them in key hash*/
-    ssl_key_hash = g_hash_table_new_full(tls_private_key_hash,
-            tls_private_key_equal, g_free, tls_private_key_free);
+    ssl_key_hash = privkey_hash_table_new();
 
 
     if (ntlsdecrypt > 0) {
diff --git a/epan/secrets.c b/epan/secrets.c
index 08ed299a3e..c539a95e06 100644
--- a/epan/secrets.c
+++ b/epan/secrets.c
@@ -9,9 +9,17 @@
  * SPDX-License-Identifier: GPL-2.0-or-later
  */
 
+#include "config.h"
+
 #include "secrets.h"
 #include <wiretap/wtap.h>
 
+#include <string.h>
+#ifdef HAVE_LIBGNUTLS
+#include <gnutls/gnutls.h>
+#include <gnutls/abstract.h>
+#endif  /* HAVE_LIBGNUTLS */
+
 /** Maps guint32 secrets_type -> secrets_block_callback_t. */
 static GHashTable *secrets_callbacks;
 
@@ -44,6 +52,34 @@ secrets_wtap_callback(guint32 secrets_type, const void *secrets, guint size)
     }
 }
 
+#ifdef HAVE_LIBGNUTLS
+static guint
+key_id_hash(gconstpointer key)
+{
+    const cert_key_id_t *key_id = (const cert_key_id_t *)key;
+    const guint32 *dw = (const guint32 *)key_id->key_id;
+
+    /* The public key' SHA-1 hash (which maps to a private key) has a uniform
+     * distribution, hence simply xor'ing them should be sufficient. */
+    return dw[0] ^ dw[1] ^ dw[2] ^ dw[3] ^ dw[4];
+}
+
+static gboolean
+key_id_equal(gconstpointer a, gconstpointer b)
+{
+    const cert_key_id_t *key_id_a = (const cert_key_id_t *)a;
+    const cert_key_id_t *key_id_b = (const cert_key_id_t *)b;
+
+    return !memcmp(key_id_a, key_id_b, sizeof(*key_id_a));
+}
+
+GHashTable *
+privkey_hash_table_new(void)
+{
+    return g_hash_table_new_full(key_id_hash, key_id_equal, g_free, (GDestroyNotify)gnutls_privkey_deinit);
+}
+#endif  /* HAVE_LIBGNUTLS */
+
 /*
  * Editor modelines  -  https://www.wireshark.org/tools/modelines.html
  *
diff --git a/epan/secrets.h b/epan/secrets.h
index de2cb05ee2..90de6382c9 100644
--- a/epan/secrets.h
+++ b/epan/secrets.h
@@ -46,6 +46,15 @@ enum secrets_scope {
 };
 #endif
 
+#ifdef HAVE_LIBGNUTLS
+/** Identifier for a RSA public key (a SHA-1 hash). */
+struct cert_key_id {
+    guint8 key_id[20];
+};
+typedef struct cert_key_id cert_key_id_t;
+#endif  /* HAVE_LIBGNUTLS */
+
+
 /**
  * Callback for the wiretap secrets provider (wtap_new_secrets_callback_t).
  */
@@ -65,4 +74,10 @@ typedef void (*secrets_block_callback_t)(const void *secrets, guint size);
  * @param cb Callback to be invoked for new secrets.
  */
 void secrets_register_type(guint32 secrets_type, secrets_block_callback_t cb);
+
+#ifdef HAVE_LIBGNUTLS
+/** Returns a new hash table, mapping cert_key_id_t -> gnutls_privkey_t. */
+GHashTable *privkey_hash_table_new(void);
+#endif  /* HAVE_LIBGNUTLS */
+
 #endif /* __SECRETS_H__ */