[evolution-data-server] Bug #606181 - Accepting bad SSL certificate applies to any hostname

[Milan Crha <mcrha src gnome org>]

commit aa31c2a326bee13ea0f558dac05f1bcf73a10936
Author: Matt McCutchen <matt mattmccutchen net>
Date:   Fri May 4 14:28:03 2012 +0200

    Bug #606181 - Accepting bad SSL certificate applies to any hostname
    
    Change the Camel certdb to look up certificates by expected hostname.
    
    This way, accepting a bad certificate for one mail server does not give
    it a free pass to impersonate the user's other mail servers.  Storing a
    second bad certificate for the same server will replace the first, but
    that should be OK (Mozilla PSM works the same way).
    
    The camel-cert.db format is unchanged except that it can now contain
    multiple entries for the same certificate with different hostnames, and
    if it contains multiple certificates for the same hostname, all but the
    last will be dropped (becoming permanent the next time the certdb is
    saved).
    
    Users who were taking advantage of evolution-data-server's previous,
    vulnerable behavior of accepting a certificate for a hostname other than
    the originally user-approved one will get bad certificate dialogs and
    will need to re-approve the certificate for the desired hostname(s).
    
    Note: Case insensitive compare of host names added by mcrha.

 camel/camel-certdb.c         |   52 ++++++++++++++++++++++++++++++-----------
 camel/camel-certdb.h         |   13 ++++++++--
 camel/camel-tcp-stream-ssl.c |   41 ++++++++++++++++++++-------------
 configure.ac                 |    2 +-
 4 files changed, 74 insertions(+), 34 deletions(-)
---
diff --git a/camel/camel-certdb.c b/camel/camel-certdb.c
index f56696c..f95e57a 100644
--- a/camel/camel-certdb.c
+++ b/camel/camel-certdb.c
@@ -63,6 +63,16 @@ static void cert_set_string (CamelCertDB *certdb, CamelCert *cert, gint string,
 
 G_DEFINE_TYPE (CamelCertDB, camel_certdb, CAMEL_TYPE_OBJECT)
 
+static gboolean
+certdb_str_equal_casecmp (gconstpointer str1,
+			  gconstpointer str2)
+{
+	if (!str1 || !str2)
+		return str1 == str2;
+
+	return g_ascii_strcasecmp (str1, str2) == 0;
+}
+
 static void
 certdb_finalize (GObject *object)
 {
@@ -127,7 +137,7 @@ camel_certdb_init (CamelCertDB *certdb)
 	certdb->cert_chunks = NULL;
 
 	certdb->certs = g_ptr_array_new ();
-	certdb->cert_hash = g_hash_table_new (g_str_hash, g_str_equal);
+	certdb->cert_hash = g_hash_table_new (g_str_hash, certdb_str_equal_casecmp);
 
 	certdb->priv->db_lock = g_mutex_new ();
 	certdb->priv->io_lock = g_mutex_new ();
@@ -269,7 +279,13 @@ camel_certdb_load (CamelCertDB *certdb)
 		if (cert == NULL)
 			goto error;
 
-		camel_certdb_add (certdb, cert);
+		/* NOTE: If we are upgrading from an evolution-data-server version
+		 * prior to the change to look up certs by hostname (bug 606181),
+		 * this "put" will result in duplicate entries for the same
+		 * hostname being dropped.  The change will become permanent on
+		 * disk the next time the certdb is dirtied for some reason and
+		 * has to be saved. */
+		camel_certdb_put (certdb, cert);
 	}
 
 	camel_certdb_unlock (certdb, CAMEL_CERTDB_IO_LOCK);
@@ -427,8 +443,8 @@ camel_certdb_touch (CamelCertDB *certdb)
 }
 
 CamelCert *
-camel_certdb_get_cert (CamelCertDB *certdb,
-                       const gchar *fingerprint)
+camel_certdb_get_host (CamelCertDB *certdb,
+                       const gchar *hostname)
 {
 	CamelCert *cert;
 
@@ -436,7 +452,7 @@ camel_certdb_get_cert (CamelCertDB *certdb,
 
 	camel_certdb_lock (certdb, CAMEL_CERTDB_DB_LOCK);
 
-	cert = g_hash_table_lookup (certdb->cert_hash, fingerprint);
+	cert = g_hash_table_lookup (certdb->cert_hash, hostname);
 	if (cert)
 		camel_certdb_cert_ref (certdb, cert);
 
@@ -446,21 +462,26 @@ camel_certdb_get_cert (CamelCertDB *certdb,
 }
 
 void
-camel_certdb_add (CamelCertDB *certdb,
+camel_certdb_put (CamelCertDB *certdb,
                   CamelCert *cert)
 {
+	CamelCert *old_cert;
+
 	g_return_if_fail (CAMEL_IS_CERTDB (certdb));
 
 	camel_certdb_lock (certdb, CAMEL_CERTDB_DB_LOCK);
 
-	if (g_hash_table_lookup (certdb->cert_hash, cert->fingerprint)) {
-		camel_certdb_unlock (certdb, CAMEL_CERTDB_DB_LOCK);
-		return;
+	/* Replace an existing entry with the same hostname. */
+	old_cert = g_hash_table_lookup (certdb->cert_hash, cert->hostname);
+	if (old_cert) {
+		g_hash_table_remove (certdb->cert_hash, cert->hostname);
+		g_ptr_array_remove (certdb->certs, old_cert);
+		camel_certdb_cert_unref (certdb, old_cert);
 	}
 
 	camel_certdb_cert_ref (certdb, cert);
 	g_ptr_array_add (certdb->certs, cert);
-	g_hash_table_insert (certdb->cert_hash, cert->fingerprint, cert);
+	g_hash_table_insert (certdb->cert_hash, cert->hostname, cert);
 
 	certdb->flags |= CAMEL_CERTDB_DIRTY;
 
@@ -468,15 +489,18 @@ camel_certdb_add (CamelCertDB *certdb,
 }
 
 void
-camel_certdb_remove (CamelCertDB *certdb,
-                     CamelCert *cert)
+camel_certdb_remove_host (CamelCertDB *certdb,
+                          const gchar *hostname)
 {
+	CamelCert *cert;
+
 	g_return_if_fail (CAMEL_IS_CERTDB (certdb));
 
 	camel_certdb_lock (certdb, CAMEL_CERTDB_DB_LOCK);
 
-	if (g_hash_table_lookup (certdb->cert_hash, cert->fingerprint)) {
-		g_hash_table_remove (certdb->cert_hash, cert->fingerprint);
+	cert = g_hash_table_lookup (certdb->cert_hash, hostname);
+	if (cert) {
+		g_hash_table_remove (certdb->cert_hash, hostname);
 		g_ptr_array_remove (certdb->certs, cert);
 		camel_certdb_cert_unref (certdb, cert);
 
diff --git a/camel/camel-certdb.h b/camel/camel-certdb.h
index 6d5bf5f..f8bc380 100644
--- a/camel/camel-certdb.h
+++ b/camel/camel-certdb.h
@@ -147,10 +147,17 @@ gint camel_certdb_save (CamelCertDB *certdb);
 
 void camel_certdb_touch (CamelCertDB *certdb);
 
-CamelCert *camel_certdb_get_cert (CamelCertDB *certdb, const gchar *fingerprint);
+/* The lookup key was changed from fingerprint to hostname to fix bug 606181. */
 
-void camel_certdb_add (CamelCertDB *certdb, CamelCert *cert);
-void camel_certdb_remove (CamelCertDB *certdb, CamelCert *cert);
+/* Get the certificate for the given hostname, if any. */
+CamelCert *camel_certdb_get_host (CamelCertDB *certdb, const gchar *hostname);
+
+/* Store cert for cert->hostname, replacing any existing certificate for the
+ * same hostname. */
+void camel_certdb_put (CamelCertDB *certdb, CamelCert *cert);
+
+/* Remove any user-accepted certificate for the given hostname. */
+void camel_certdb_remove_host (CamelCertDB *certdb, const gchar *hostname);
 
 CamelCert *camel_certdb_cert_new (CamelCertDB *certdb);
 void camel_certdb_cert_ref (CamelCertDB *certdb, CamelCert *cert);
diff --git a/camel/camel-tcp-stream-ssl.c b/camel/camel-tcp-stream-ssl.c
index db53eb1..73b05a1 100644
--- a/camel/camel-tcp-stream-ssl.c
+++ b/camel/camel-tcp-stream-ssl.c
@@ -226,8 +226,8 @@ ssl_auth_cert (gpointer data,
 }
 #endif
 
-CamelCert *camel_certdb_nss_cert_get (CamelCertDB *certdb, CERTCertificate *cert);
-CamelCert *camel_certdb_nss_cert_add (CamelCertDB *certdb, CERTCertificate *cert);
+CamelCert *camel_certdb_nss_cert_get (CamelCertDB *certdb, CERTCertificate *cert, const gchar *hostname);
+CamelCert *camel_certdb_nss_cert_convert (CamelCertDB *certdb, CERTCertificate *cert);
 void camel_certdb_nss_cert_set (CamelCertDB *certdb, CamelCert *ccert, CERTCertificate *cert);
 
 static gchar *
@@ -271,16 +271,21 @@ cert_fingerprint (CERTCertificate *cert)
 /* lookup a cert uses fingerprint to index an on-disk file */
 CamelCert *
 camel_certdb_nss_cert_get (CamelCertDB *certdb,
-                           CERTCertificate *cert)
+                           CERTCertificate *cert,
+                           const gchar *hostname)
 {
 	gchar *fingerprint;
 	CamelCert *ccert;
 
+	ccert = camel_certdb_get_host (certdb, hostname);
+	if (ccert == NULL)
+		return NULL;
+
 	fingerprint = cert_fingerprint (cert);
-	ccert = camel_certdb_get_cert (certdb, fingerprint);
-	if (ccert == NULL) {
+	if (strcmp (fingerprint, ccert->fingerprint) != 0) {
+		/* The saved certificate is not the one we wanted. */
 		g_free (fingerprint);
-		return ccert;
+		return NULL;
 	}
 
 	if (ccert->rawcert == NULL) {
@@ -327,10 +332,10 @@ camel_certdb_nss_cert_get (CamelCertDB *certdb,
 	return ccert;
 }
 
-/* add a cert to the certdb */
+/* Create a CamelCert corresponding to the NSS cert, with unknown trust. */
 CamelCert *
-camel_certdb_nss_cert_add (CamelCertDB *certdb,
-                           CERTCertificate *cert)
+camel_certdb_nss_cert_convert (CamelCertDB *certdb,
+                               CERTCertificate *cert)
 {
 	CamelCert *ccert;
 	gchar *fingerprint;
@@ -346,10 +351,6 @@ camel_certdb_nss_cert_add (CamelCertDB *certdb,
 	camel_cert_set_trust (certdb, ccert, CAMEL_CERT_TRUST_UNKNOWN);
 	g_free (fingerprint);
 
-	camel_certdb_nss_cert_set (certdb, ccert, cert);
-
-	camel_certdb_add (certdb, ccert);
-
 	return ccert;
 }
 
@@ -444,6 +445,7 @@ ssl_bad_cert (gpointer data,
 	gboolean accept;
 	CamelCertDB *certdb = NULL;
 	CamelCert *ccert = NULL;
+	gboolean ccert_is_new = FALSE;
 	gchar *prompt, *cert_str, *fingerprint;
 	CamelTcpStreamSSL *ssl;
 	CERTCertificate *cert;
@@ -459,10 +461,14 @@ ssl_bad_cert (gpointer data,
 		return SECFailure;
 
 	certdb = camel_certdb_get_default ();
-	ccert = camel_certdb_nss_cert_get (certdb, cert);
+	ccert = camel_certdb_nss_cert_get (certdb, cert, ssl->priv->expected_host);
 	if (ccert == NULL) {
-		ccert = camel_certdb_nss_cert_add (certdb, cert);
+		ccert = camel_certdb_nss_cert_convert (certdb, cert);
 		camel_cert_set_hostname (certdb, ccert, ssl->priv->expected_host);
+		/* Don't put in the certdb yet.  Since we can only store one
+		 * entry per hostname, we'd rather not ruin any existing entry
+		 * for this hostname if the user rejects the new certificate. */
+		ccert_is_new = TRUE;
 	}
 
 	if (ccert->trust == CAMEL_CERT_TRUST_UNKNOWN) {
@@ -495,7 +501,10 @@ ssl_bad_cert (gpointer data,
 		g_free (prompt);
 
 		accept = button_id != 0;
-		camel_certdb_nss_cert_set (certdb, ccert, cert);
+		if (ccert_is_new) {
+			camel_certdb_nss_cert_set (certdb, ccert, cert);
+			camel_certdb_put (certdb, ccert);
+		}
 
 		switch (button_id) {
 		case 0: /* Reject */
diff --git a/configure.ac b/configure.ac
index a0d82d5..1452b1f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -103,7 +103,7 @@ LIBEBOOK_CURRENT=16
 LIBEBOOK_REVISION=1
 LIBEBOOK_AGE=3
 
-LIBCAMEL_CURRENT=34
+LIBCAMEL_CURRENT=35
 LIBCAMEL_REVISION=0
 LIBCAMEL_AGE=0