/pidgin/main: 2e4475087f04: Fix basic constraints checking for b...
Mark Doliner
mark at kingant.net
Wed Oct 22 10:20:29 EDT 2014
Changeset: 2e4475087f048f1a1c473d5136149d8443ded85e
Author: Mark Doliner <mark at kingant.net>
Date: 2014-10-12 23:28 -0700
Branch: release-2.x.y
URL: https://hg.pidgin.im/pidgin/main/rev/2e4475087f04
Description:
Fix basic constraints checking for both our SSL plugins.
This was reported to our private security at 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!
diffstat:
COPYRIGHT | 1 +
ChangeLog | 14 +++-
libpurple/certificate.c | 78 ++++++++---------------
libpurple/certificate.h | 61 ++++++++++++++++++-
libpurple/plugins/ssl/ssl-gnutls.c | 37 +++++++++++-
libpurple/plugins/ssl/ssl-nss.c | 118 +++++++++++++++++++++++++++++++++++-
6 files changed, 248 insertions(+), 61 deletions(-)
diffs (truncated from 464 to 300 lines):
diff --git a/COPYRIGHT b/COPYRIGHT
--- a/COPYRIGHT
+++ b/COPYRIGHT
@@ -161,6 +161,7 @@ William Ehlhardt
Markus Elfring
Nelson Elhage
Ignacio J. Elia
+Kai Engert
Brian Enigma
Mattias Eriksson
Pat Erley
diff --git a/ChangeLog b/ChangeLog
--- a/ChangeLog
+++ b/ChangeLog
@@ -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 --git a/libpurple/certificate.c b/libpurple/certificate.c
--- a/libpurple/certificate.c
+++ b/libpurple/certificate.c
@@ -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 --git a/libpurple/certificate.h b/libpurple/certificate.h
--- a/libpurple/certificate.h
+++ b/libpurple/certificate.h
@@ -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 --git a/libpurple/plugins/ssl/ssl-gnutls.c b/libpurple/plugins/ssl/ssl-gnutls.c
--- a/libpurple/plugins/ssl/ssl-gnutls.c
+++ b/libpurple/plugins/ssl/ssl-gnutls.c
@@ -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,
More information about the Commits
mailing list