SSL certificate chain validation issues
Mark Doliner
mark at kingant.net
Mon Oct 13 02:37:38 EDT 2014
On Mon, Jun 23, 2014 at 7:51 AM, Daniel Atallah <datallah at pidgin.im> wrote:
> Perhaps I'm missing something, but it doesn't seem right that we're treating
> certs without basic constraints as if they are CAs.
I fixed this. It's an error if: cert is version 3 or higher and basic
constraints extension is absent, or extension is present and the CA
field is false. The relevant section of the specs are here:
https://tools.ietf.org/html/rfc2459#section-4.2.1.10
https://tools.ietf.org/html/rfc5280#section-4.2.1.9
> My preference would be to include something like my complete NSS fix even if
> we can't do the same for gnutls.
Sounds good to me. So let's use use my change for GnuTLS (adds checks
for basic constraints), and let's use Daniel's change for NSS (calls
out to the NSS API for verification).
I checked this into the 2.x.y branch of our private repo. Revision
2e4475087f04. Also attached the patch, in case anyone wants to
double-check it. I tested both the GnuTLS and NSS plugins with the
major IM protocols. I tested with and without man-in-the-middling
myself with a cert with a bad basic constraints setting and the patch
fixes the problem.
I will try to request CVEs tomorrow (Monday) night. I'll be requesting
CVEs for 4 issues: this one (SSL basic constraints), and the three
from Richard Johnson/Sourcefire VRT (VRT-2014-0203, VRT-2014-0204,
VRT-2014-0205). My understanding is that we DON'T need a CVE for any
of the Gadu-Gadu bugs. I haven't looked into those too much myself, so
please correct me if I'm wrong.
I'll probably try to tag, build tarballs, and email packagers either
Wednesday or Thursday night. I'll be out of town Friday through
Monday. I'll be able to do the release next Wednesday.
-------------- next part --------------
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
};
More information about the security
mailing list