Attachment 948786 Details for Bug 1154908 – patch from upstream

[patch] patch from upstream

CVE-2014-3694.diff (text/plain), 17.37 KB, created by Murray McAllister on 2014-10-21 03:17:13 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Murray McAllister

Created: 2014-10-21 03:17:13 UTC

Size: 17.37 KB

patch

obsolete

>This is revision 2e4475087f04.
>
>
>
>Fix basic constraints checking for both our SSL plugins.
>
>This was reported to our private security@pidgin.im mailing list
>by an anonymous person and Jacob Appelbaum of the Tor project.
>
>The general problem is described by Moxie Marlinspike here:
>http://www.thoughtcrime.org/ie-ssl-chain.txt
>
>Turns out BOTH of our SSL/TLS plugins are vulnerable to this. It allows
>a malicious man-in-the-middle to impersonate an https server accessed by
>Pidgin.
>
>The fix for this was difficult. We'd really like to just delegate all cert
>validate to the NSS or GnuTLS plugins and not do any of it ourselves, because
>they're experts and we're not. And this is essentially the change we made for
>NSS. However, this was difficult for GnuTLS because we need a context that we
>don't have access to in the right function. We could have done it, but it
>would have been a little hacky. So for our GnuTLS plugin we added basic
>constraints checking ourselves. In Pidgin 3.0.0 would should clean this up
>and remove a lot of internal cert validation and ALWAYS delegate to the
>SSL/TLS library.
>
>The NSS parts of this patch were written by Kai Engert and Daniel Atallah.
>I wrote the GnuTLS parts.
>
>We'll be requesting a CVE number for this.
>
>Also, my thanks to Jacob Appelbaum and Moxie Marlinspike for their efforts
>over many years to improve the security of the software that we use on a
>daily basis. They are both stand-out citizens who have made contributions
>to protect the privacy of all internet users. Thanks, guys!
>
>
>
>diff -r db951baf06ac -r 2e4475087f04 COPYRIGHT
>--- a/COPYRIGHT	Thu Oct 09 20:56:08 2014 -0700
>+++ b/COPYRIGHT	Sun Oct 12 23:28:58 2014 -0700
>@@ -161,6 +161,7 @@ William Ehlhardt
> Markus Elfring
> Nelson Elhage
> Ignacio J. Elia
>+Kai Engert
> Brian Enigma
> Mattias Eriksson
> Pat Erley
>diff -r db951baf06ac -r 2e4475087f04 ChangeLog
>--- a/ChangeLog	Thu Oct 09 20:56:08 2014 -0700
>+++ b/ChangeLog	Sun Oct 12 23:28:58 2014 -0700
>@@ -1,9 +1,17 @@
> Pidgin and Finch: The Pimpin' Penguin IM Clients That're Good for the Soul
> 
>-version 2.10.10 (?/?/?):
>+version 2.10.10 (10/22/14):
> 	General:
>-	* Allow and prefer TLS 1.2 and 1.1 when using libnss. (Elrond and
>-	  Ashish Gupta) (#15909)
>+	* Check the basic constraints extension when validating SSL/TLS
>+	  certificates. This fixes a security hole that allowed a malicious
>+	  man-in-the-middle to impersonate an IM server or any other https
>+	  endpoint. This affected both the NSS and GnuTLS plugins. (Discovered
>+	  by an anonymous person and Jacob Appelbaum of the Tor Project, with
>+	  thanks to Moxie Marlinspike for first publishing about this type of
>+	  vulnerability. Thanks to Kai Engert for guidance and for some of the
>+	  NSS changes).
>+	* Allow and prefer TLS 1.2 and 1.1 when using the NSS plugin for SSL.
>+	  (Elrond and Ashish Gupta) (#15909)
> 
> 	libpurple3 compatibility:
> 	* Encrypted account passwords are preserved until the new one is set.
>diff -r db951baf06ac -r 2e4475087f04 libpurple/certificate.c
>--- a/libpurple/certificate.c	Thu Oct 09 20:56:08 2014 -0700
>+++ b/libpurple/certificate.c	Sun Oct 12 23:28:58 2014 -0700
>@@ -41,50 +41,6 @@ static GList *cert_verifiers = NULL;
> /** List of registered Pools */
> static GList *cert_pools = NULL;
> 
>-/*
>- * TODO: Merge this with PurpleCertificateVerificationStatus for 3.0.0 */
>-typedef enum {
>-	PURPLE_CERTIFICATE_UNKNOWN_ERROR = -1,
>-
>-	/* Not an error */
>-	PURPLE_CERTIFICATE_NO_PROBLEMS = 0,
>-
>-	/* Non-fatal */
>-	PURPLE_CERTIFICATE_NON_FATALS_MASK = 0x0000FFFF,
>-
>-	/* The certificate is self-signed. */
>-	PURPLE_CERTIFICATE_SELF_SIGNED = 0x01,
>-
>-	/* The CA is not in libpurple's pool of certificates. */
>-	PURPLE_CERTIFICATE_CA_UNKNOWN = 0x02,
>-
>-	/* The current time is before the certificate's specified
>-	 * activation time.
>-	 */
>-	PURPLE_CERTIFICATE_NOT_ACTIVATED = 0x04,
>-
>-	/* The current time is after the certificate's specified expiration time */
>-	PURPLE_CERTIFICATE_EXPIRED = 0x08,
>-
>-	/* The certificate's subject name doesn't match the expected */
>-	PURPLE_CERTIFICATE_NAME_MISMATCH = 0x10,
>-
>-	/* No CA pool was found. This shouldn't happen... */
>-	PURPLE_CERTIFICATE_NO_CA_POOL = 0x20,
>-
>-	/* Fatal */
>-	PURPLE_CERTIFICATE_FATALS_MASK = 0xFFFF0000,
>-
>-	/* The signature chain could not be validated. Due to limitations in the
>-	 * the current API, this also indicates one of the CA certificates in the
>-	 * chain is expired (or not yet activated). FIXME 3.0.0 */
>-	PURPLE_CERTIFICATE_INVALID_CHAIN = 0x10000,
>-
>-	/* The signature has been revoked. */
>-	PURPLE_CERTIFICATE_REVOKED = 0x20000,
>-
>-	PURPLE_CERTIFICATE_LAST = 0x40000,
>-} PurpleCertificateInvalidityFlags;
> 
> static const gchar *
> invalidity_reason_to_string(PurpleCertificateInvalidityFlags flag)
>@@ -776,10 +732,11 @@ static GList *x509_ca_certs = NULL;
> /** Used for lazy initialization purposes. */
> static gboolean x509_ca_initialized = FALSE;
> 
>-/** Adds a certificate to the in-memory cache, doing nothing else */
>+/** Adds a certificate to the in-memory cache, and mark it as trusted */
> static gboolean
> x509_ca_quiet_put_cert(PurpleCertificate *crt)
> {
>+	gboolean ret;
> 	x509_ca_element *el;
> 
> 	/* lazy_init calls this function, so calling lazy_init here is a
>@@ -791,12 +748,20 @@ x509_ca_quiet_put_cert(PurpleCertificate
> 	/* TODO: Perhaps just check crt->scheme->name instead? */
> 	g_return_val_if_fail(crt->scheme == purple_certificate_find_scheme(x509_ca.scheme_name), FALSE);
> 
>-	el = g_new0(x509_ca_element, 1);
>-	el->dn = purple_certificate_get_unique_id(crt);
>-	el->crt = purple_certificate_copy(crt);
>-	x509_ca_certs = g_list_prepend(x509_ca_certs, el);
>+	ret = TRUE;
> 
>-	return TRUE;
>+	if (crt->scheme->register_trusted_tls_cert) {
>+		ret = (crt->scheme->register_trusted_tls_cert)(crt, TRUE);
>+	}
>+
>+	if (ret) {
>+		el = g_new0(x509_ca_element, 1);
>+		el->dn = purple_certificate_get_unique_id(crt);
>+		el->crt = purple_certificate_copy(crt);
>+		x509_ca_certs = g_list_prepend(x509_ca_certs, el);
>+	}
>+
>+	return ret;
> }
> 
> /* Since the libpurple CertificatePools get registered before plugins are
>@@ -927,6 +892,7 @@ x509_ca_uninit(void)
> 	g_list_free(x509_ca_certs);
> 	x509_ca_certs = NULL;
> 	x509_ca_initialized = FALSE;
>+	/** TODO: the cert store in the SSL implementation wouldn't be cleared by this */
> 	g_list_foreach(x509_ca_paths, (GFunc)g_free, NULL);
> 	g_list_free(x509_ca_paths);
> 	x509_ca_paths = NULL;
>@@ -1185,6 +1151,10 @@ x509_tls_peers_put_cert(const gchar *id,
> 	keypath = purple_certificate_pool_mkpath(&x509_tls_peers, id);
> 	ret = purple_certificate_export(keypath, crt);
> 
>+	if (crt->scheme->register_trusted_tls_cert) {
>+		ret = (crt->scheme->register_trusted_tls_cert)(crt, FALSE);
>+	}
>+
> 	g_free(keypath);
> 	return ret;
> }
>@@ -1606,6 +1576,14 @@ x509_tls_cached_unknown_peer(PurpleCerti
> 
> 	peer_crt = (PurpleCertificate *) chain->data;
> 
>+	if (peer_crt->scheme->verify_cert) {
>+		/** Make sure we've loaded the CA certs (which causes NSS to trust them) */
>+		g_return_if_fail(x509_ca_lazy_init());
>+		peer_crt->scheme->verify_cert(vrq, &flags);
>+		x509_tls_cached_complete(vrq, flags);
>+		return;
>+	}
>+
> 	/* TODO: Figure out a way to check for a bad signature, as opposed to
> 	   "not self-signed" */
> 	if ( purple_certificate_signed_by(peer_crt, peer_crt) ) {
>diff -r db951baf06ac -r 2e4475087f04 libpurple/certificate.h
>--- a/libpurple/certificate.h	Thu Oct 09 20:56:08 2014 -0700
>+++ b/libpurple/certificate.h	Sun Oct 12 23:28:58 2014 -0700
>@@ -46,6 +46,51 @@ typedef enum
> 	PURPLE_CERTIFICATE_VALID = 1
> } PurpleCertificateVerificationStatus;
> 
>+/*
>+ * TODO: Merge this with PurpleCertificateVerificationStatus for 3.0.0 */
>+typedef enum {
>+	PURPLE_CERTIFICATE_UNKNOWN_ERROR = -1,
>+
>+	/* Not an error */
>+	PURPLE_CERTIFICATE_NO_PROBLEMS = 0,
>+
>+	/* Non-fatal */
>+	PURPLE_CERTIFICATE_NON_FATALS_MASK = 0x0000FFFF,
>+
>+	/* The certificate is self-signed. */
>+	PURPLE_CERTIFICATE_SELF_SIGNED = 0x01,
>+
>+	/* The CA is not in libpurple's pool of certificates. */
>+	PURPLE_CERTIFICATE_CA_UNKNOWN = 0x02,
>+
>+	/* The current time is before the certificate's specified
>+	 * activation time.
>+	 */
>+	PURPLE_CERTIFICATE_NOT_ACTIVATED = 0x04,
>+
>+	/* The current time is after the certificate's specified expiration time */
>+	PURPLE_CERTIFICATE_EXPIRED = 0x08,
>+
>+	/* The certificate's subject name doesn't match the expected */
>+	PURPLE_CERTIFICATE_NAME_MISMATCH = 0x10,
>+
>+	/* No CA pool was found. This shouldn't happen... */
>+	PURPLE_CERTIFICATE_NO_CA_POOL = 0x20,
>+
>+	/* Fatal */
>+	PURPLE_CERTIFICATE_FATALS_MASK = 0xFFFF0000,
>+
>+	/* The signature chain could not be validated. Due to limitations in the
>+	 * the current API, this also indicates one of the CA certificates in the
>+	 * chain is expired (or not yet activated). FIXME 3.0.0 */
>+	PURPLE_CERTIFICATE_INVALID_CHAIN = 0x10000,
>+
>+	/* The signature has been revoked. */
>+	PURPLE_CERTIFICATE_REVOKED = 0x20000,
>+
>+	PURPLE_CERTIFICATE_LAST = 0x40000,
>+} PurpleCertificateInvalidityFlags;
>+
> typedef struct _PurpleCertificate PurpleCertificate;
> typedef struct _PurpleCertificatePool PurpleCertificatePool;
> typedef struct _PurpleCertificateScheme PurpleCertificateScheme;
>@@ -197,7 +242,8 @@ struct _PurpleCertificateScheme
> 	 */
> 	void (* destroy_certificate)(PurpleCertificate * crt);
> 
>-	/** Find whether "crt" has a valid signature from issuer "issuer"
>+	/** Find whether "crt" has a valid signature from "issuer," including
>+	 * appropriate values for the CA flag in the basic constraints extension.
> 	 *  @see purple_certificate_signed_by() */
> 	gboolean (*signed_by)(PurpleCertificate *crt, PurpleCertificate *issuer);
> 	/**
>@@ -258,8 +304,17 @@ struct _PurpleCertificateScheme
> 	 */
> 	GSList * (* import_certificates)(const gchar * filename);
> 
>-	void (*_purple_reserved1)(void);
>-	void (*_purple_reserved2)(void);
>+	/**
>+	 * Register a certificate as "trusted."
>+	 */
>+	gboolean (* register_trusted_tls_cert)(PurpleCertificate *crt, gboolean ca);
>+
>+	/**
>+	 * Verify that a certificate is valid, performing all necessary checks
>+	 * including date range, valid cert chain, recognized and valid CAs, etc.
>+	 */
>+	void (* verify_cert)(PurpleCertificateVerificationRequest *vrq, PurpleCertificateInvalidityFlags *flags);
>+
> 	void (*_purple_reserved3)(void);
> };
> 
>diff -r db951baf06ac -r 2e4475087f04 libpurple/plugins/ssl/ssl-gnutls.c
>--- a/libpurple/plugins/ssl/ssl-gnutls.c	Thu Oct 09 20:56:08 2014 -0700
>+++ b/libpurple/plugins/ssl/ssl-gnutls.c	Sun Oct 12 23:28:58 2014 -0700
>@@ -925,7 +925,7 @@ x509_certificate_signed_by(PurpleCertifi
> 	crt_dat = X509_GET_GNUTLS_DATA(crt);
> 	issuer_dat = X509_GET_GNUTLS_DATA(issuer);
> 
>-	/* First, let's check that crt.issuer is actually issuer */
>+	/* Ensure crt issuer matches the name on the issuer cert. */
> 	ret = gnutls_x509_crt_check_issuer(crt_dat, issuer_dat);
> 	if (ret <= 0) {
> 
>@@ -954,6 +954,41 @@ x509_certificate_signed_by(PurpleCertifi
> 		return FALSE;
> 	}
> 
>+	/* Check basic constraints extension (if it exists then the CA flag must
>+	   be set to true, and it must exist for certs with version 3 or higher. */
>+	ret = gnutls_x509_crt_get_basic_constraints(issuer_dat, NULL, NULL, NULL);
>+	if (ret == GNUTLS_E_REQUESTED_DATA_NOT_AVAILABLE) {
>+		if (gnutls_x509_crt_get_version(issuer_dat) >= 3) {
>+			/* Reject cert (no basic constraints and cert version is >= 3). */
>+			gchar *issuer_id = purple_certificate_get_unique_id(issuer);
>+			purple_debug_info("gnutls/x509", "Rejecting cert because the "
>+					"basic constraints extension is missing from issuer cert "
>+					"for %s. The basic constraints extension is required on "
>+					"all version 3 or higher certs (this cert is version %d).",
>+					issuer_id ? issuer_id : "(null)",
>+					gnutls_x509_crt_get_version(issuer_dat));
>+			g_free(issuer_id);
>+			return FALSE;
>+		} else {
>+			/* Allow cert (no basic constraints and cert version is < 3). */
>+			purple_debug_info("gnutls/x509", "Basic constraint extension is "
>+					"missing from issuer cert for %s. Allowing this because "
>+					"the cert is version %d and the basic constraints "
>+					"extension is only required for version 3 or higher "
>+					"certs.", issuer_id ? issuer_id : "(null)",
>+					gnutls_x509_crt_get_version(issuer_dat));
>+		}
>+	} else if (ret <= 0) {
>+		/* Reject cert (CA flag is false in basic constraints). */
>+		gchar *issuer_id = purple_certificate_get_unique_id(issuer);
>+		purple_debug_info("gnutls/x509", "Rejecting cert because the CA flag "
>+				"is set to false in the basic constraints extension for "
>+				"issuer cert %s. ret=%d\n",
>+				issuer_id ? issuer_id : "(null)", ret);
>+		g_free(issuer_id);
>+		return FALSE;
>+	}
>+
> 	/* Now, check the signature */
> 	/* The second argument is a ptr to an array of "trusted" issuer certs,
> 	   but we're only using one trusted one */
>diff -r db951baf06ac -r 2e4475087f04 libpurple/plugins/ssl/ssl-nss.c
>--- a/libpurple/plugins/ssl/ssl-nss.c	Thu Oct 09 20:56:08 2014 -0700
>+++ b/libpurple/plugins/ssl/ssl-nss.c	Sun Oct 12 23:28:58 2014 -0700
>@@ -45,6 +45,7 @@
> #include <nspr.h>
> #include <nss.h>
> #include <nssb64.h>
>+#include <ocsp.h>
> #include <pk11func.h>
> #include <prio.h>
> #include <secerr.h>
>@@ -53,6 +54,11 @@
> #include <sslerr.h>
> #include <sslproto.h>
> 
>+/* There's a bug in some versions of this header that requires that some of
>+   the headers above be included first. This is true for at least libnss
>+   3.15.4. */
>+#include <certdb.h>
>+
> /* This is defined in NSPR's <private/pprio.h>, but to avoid including a
>  * private header we duplicate the prototype here */
> NSPR_API(PRFileDesc*)  PR_ImportTCPSocket(PRInt32 osfd);
>@@ -182,6 +188,9 @@ ssl_nss_init_nss(void)
> 	}
> #endif /* NSS >= 3.14 */
> 
>+	/** Disable OCSP Checking until we can make that use our HTTP & Proxy stuff */
>+	CERT_EnableOCSPChecking(PR_FALSE);
>+
> 	_identity = PR_GetUniqueIdentity("Purple");
> 	_nss_methods = PR_GetDefaultIOMethods();
> }
>@@ -952,7 +961,7 @@ x509_times (PurpleCertificate *crt, time
> 		    Handling is different for signed vs unsigned 32-bit types.
> 		 */
> 		if (*activation != nss_activ) {
>-		       	if (nss_activ < 0) {
>+			if (nss_activ < 0) {
> 				purple_debug_warning("nss",
> 					"Setting Activation Date to epoch to handle pre-epoch value\n");
> 				*activation = 0;
>@@ -990,6 +999,108 @@ x509_times (PurpleCertificate *crt, time
> 	return TRUE;
> }
> 
>+static gboolean
>+x509_register_trusted_tls_cert(PurpleCertificate *crt, gboolean ca)
>+{
>+	CERTCertDBHandle *certdb = CERT_GetDefaultCertDB();
>+	CERTCertificate *crt_dat;
>+	CERTCertTrust trust;
>+
>+	g_return_val_if_fail(crt, FALSE);
>+	g_return_val_if_fail(crt->scheme == &x509_nss, FALSE);
>+
>+	crt_dat = X509_NSS_DATA(crt);
>+	g_return_val_if_fail(crt_dat, FALSE);
>+
>+	purple_debug_info("nss", "Trusting %s\n", crt_dat->subjectName);
>+
>+	if (ca && !CERT_IsCACert(crt_dat, NULL)) {
>+		purple_debug_error("nss",
>+			"Refusing to set non-CA cert as trusted CA\n");
>+		return FALSE;
>+	}
>+
>+	if (crt_dat->isperm) {
>+		purple_debug_info("nss",
>+			"Skipping setting trust for cert in permanent DB\n");
>+		return TRUE;
>+	}
>+
>+	if (ca) {
>+		trust.sslFlags = CERTDB_TRUSTED_CA | CERTDB_TRUSTED_CLIENT_CA;
>+	} else {
>+		trust.sslFlags = CERTDB_TRUSTED;
>+	}
>+	trust.emailFlags = 0;
>+	trust.objectSigningFlags = 0;
>+
>+	CERT_ChangeCertTrust(certdb, crt_dat, &trust);
>+
>+	return TRUE;
>+}
>+
>+static void x509_verify_cert(PurpleCertificateVerificationRequest *vrq, PurpleCertificateInvalidityFlags *flags)
>+{
>+	CERTCertDBHandle *certdb = CERT_GetDefaultCertDB();
>+	CERTCertificate *crt_dat;
>+	PRTime now = PR_Now();
>+	SECStatus          rv;
>+	PurpleCertificate *first_cert = vrq->cert_chain->data;
>+	CERTVerifyLog log;
>+
>+	crt_dat = X509_NSS_DATA(first_cert);
>+
>+	log.arena = PORT_NewArena(512);
>+	log.head = log.tail = NULL;
>+	log.count = 0;
>+	rv = CERT_VerifyCert(certdb, crt_dat, PR_TRUE, certUsageSSLServer, now, NULL, &log);
>+
>+	if (rv != SECSuccess || log.count > 0) {
>+		CERTVerifyLogNode *node   = NULL;
>+		unsigned int depth = (unsigned int)-1;
>+
>+		for (node = log.head; node; node = node->next) {
>+			if (depth != node->depth) {
>+				depth = node->depth;
>+				purple_debug_error("nss", "CERT %d. %s %s:\n", depth,
>+					node->cert->subjectName,
>+					depth ? "[Certificate Authority]": "");
>+			}
>+			purple_debug_error("nss", "  ERROR %ld: %s\n", node->error,
>+				PR_ErrorToName(node->error));
>+			switch (node->error) {
>+				case SEC_ERROR_EXPIRED_CERTIFICATE:
>+					*flags |= PURPLE_CERTIFICATE_EXPIRED;
>+					break;
>+				case SEC_ERROR_REVOKED_CERTIFICATE:
>+					*flags |= PURPLE_CERTIFICATE_REVOKED;
>+					break;
>+				case SEC_ERROR_UNTRUSTED_ISSUER:
>+					if (crt_dat->isRoot) {
>+						*flags |= PURPLE_CERTIFICATE_SELF_SIGNED;
>+					} else {
>+						*flags |= PURPLE_CERTIFICATE_CA_UNKNOWN;
>+					}
>+					break;
>+				case SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED:
>+				case SEC_ERROR_BAD_SIGNATURE:
>+				default:
>+					*flags |= PURPLE_CERTIFICATE_INVALID_CHAIN;
>+			}
>+			if (node->cert)
>+				CERT_DestroyCertificate(node->cert);
>+		}
>+	} else {
>+		rv = CERT_VerifyCertName(crt_dat, vrq->subject_name);
>+		if (rv != SECSuccess) {
>+			purple_debug_error("nss", "Cert chain valid, but name not verified\n");
>+			*flags |= PURPLE_CERTIFICATE_NAME_MISMATCH;
>+		}
>+	}
>+
>+	PORT_FreeArena(log.arena, PR_FALSE);
>+}
>+
> static PurpleCertificateScheme x509_nss = {
> 	"x509",                          /* Scheme name */
> 	N_("X.509 Certificates"),        /* User-visible scheme name */
>@@ -1005,9 +1116,8 @@ static PurpleCertificateScheme x509_nss 
> 	x509_check_name,                 /* Check subject name */
> 	x509_times,                      /* Activation/Expiration time */
> 	x509_importcerts_from_file,      /* Multiple certificate import function */
>-
>-	NULL,
>-	NULL,
>+	x509_register_trusted_tls_cert,  /* Register a certificate as trusted for TLS */
>+	x509_verify_cert,                /* Verify that the specified cert chain is trusted */
> 	NULL
> };
>

Actions: View | Diff

Attachments on bug 1154908: 948786