SSL certificate chain validation issues

[Mark Doliner]

Attached patch that adds basic constraint checking for both gnutls and
nss. I think we should go with this in 2.x.y, and save more elaborate
fixes for the default branch.

Anyone want to review this and make sure it looks sane? I think the
gnutls code is basically the same as what I sent in my previous email.
The libnss checks were inspired by the cert_VerifyCertChainOld()
function. I'm hoping that this code is all temporary, and in we'll
switch to using higher-level cert verification functions in default.
-------------- next part --------------
diff -r c4ebf4d738f9 libpurple/certificate.h
--- a/libpurple/certificate.h	Tue Jun 17 23:14:13 2014 -0700
+++ b/libpurple/certificate.h	Sun Jun 22 17:08:16 2014 -0700
@@ -197,7 +197,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" and issuer set the
+	 * CA flag to true in the basic constraints extension.
 	 *  @see purple_certificate_signed_by() */
 	gboolean (*signed_by)(PurpleCertificate *crt, PurpleCertificate *issuer);
 	/**
diff -r c4ebf4d738f9 libpurple/plugins/ssl/ssl-gnutls.c
--- a/libpurple/plugins/ssl/ssl-gnutls.c	Tue Jun 17 23:14:13 2014 -0700
+++ b/libpurple/plugins/ssl/ssl-gnutls.c	Sun Jun 22 17:08:16 2014 -0700
@@ -872,7 +872,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) {
 
@@ -901,6 +901,26 @@ x509_certificate_signed_by(PurpleCertifi
 		return FALSE;
 	}
 
+	/* Ensure issuer has CA flag set to true in the basic constraints
+	   extension. */
+	ret = gnutls_x509_crt_get_basic_constraints(issuer_dat, NULL, NULL, NULL);
+	if (ret == GNUTLS_E_REQUESTED_DATA_NOT_AVAILABLE) {
+		/* Issuer certificate is missing the basic constraints extension.
+		   Allow it to act as a CA. */
+	} else if (ret <= 0) {
+		/* Issuer certificate has basic constraints extension and the CA flag
+		   is set to false. Reject the certificate. */
+		gchar *issuer_id;
+
+		issuer_id = purple_certificate_get_unique_id(issuer);
+		purple_debug_info("gnutls/x509", "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 c4ebf4d738f9 libpurple/plugins/ssl/ssl-nss.c
--- a/libpurple/plugins/ssl/ssl-nss.c	Tue Jun 17 23:14:13 2014 -0700
+++ b/libpurple/plugins/ssl/ssl-nss.c	Sun Jun 22 17:08:16 2014 -0700
@@ -734,6 +734,44 @@ x509_destroy_certificate(PurpleCertifica
 	g_free(crt);
 }
 
+/**
+ * This function was copied from
+ * https://mxr.mozilla.org/security/source/security/nss/lib/certdb/certv3.c
+ * and is licensed under the terms of the Mozilla Public License, v. 2.0.
+ *
+ * This function only exists here because the version in libnss doesn't seem
+ * to be exported even though it's listed in nss/cert.h.
+ * See https://bugzilla.mozilla.org/show_bug.cgi?id=621045
+ *
+ * In the future we should get rid of this function and instead switch to
+ * using higher level verification functions, such as CERT_VerifyCertNow()
+ * or CERT_VerifyCert().
+ */
+static SECStatus
+my_CERT_FindBasicConstraintExten(CERTCertificate *cert,
+		CERTBasicConstraints *value)
+{
+	SECItem encodedExtenValue;
+	SECStatus rv;
+
+	encodedExtenValue.data = NULL;
+	encodedExtenValue.len = 0;
+
+	rv = CERT_FindCertExtension(cert, SEC_OID_X509_BASIC_CONSTRAINTS,
+			&encodedExtenValue);
+	if (rv != SECSuccess) {
+		return rv;
+	}
+
+	rv = CERT_DecodeBasicConstraintValue(value, &encodedExtenValue);
+
+	/* free the raw extension data */
+	PORT_Free(encodedExtenValue.data);
+	encodedExtenValue.data = NULL;
+
+	return rv;
+}
+
 /** Determines whether one certificate has been issued and signed by another
  *
  * @param crt       Certificate to check the signature of
@@ -748,6 +786,7 @@ x509_signed_by(PurpleCertificate * crt,
 {
 	CERTCertificate *subjectCert;
 	CERTCertificate *issuerCert;
+	CERTBasicConstraints basicConstraint;
 	SECStatus st;
 
 	issuerCert = X509_NSS_DATA(issuer);
@@ -756,9 +795,63 @@ x509_signed_by(PurpleCertificate * crt,
 	subjectCert = X509_NSS_DATA(crt);
 	g_return_val_if_fail(subjectCert, FALSE);
 
+	/* Ensure that crt issuer matches the name on the issuer cert. */
+	/* TODO: Could/should use CERT_VerifyCertName() here? Even better would be
+	   to stop doing all this crap ourselves and use CERT_VerifyCertNow() or
+	   CERT_VerifyCert() directly. */
 	if (subjectCert->issuerName == NULL || issuerCert->subjectName == NULL
-			|| PORT_Strcmp(subjectCert->issuerName, issuerCert->subjectName) != 0)
+			|| PORT_Strcmp(subjectCert->issuerName, issuerCert->subjectName) != 0) {
+		purple_debug_info("nss/x509", "Certificate %s is issued by %s, which "
+				"does not match %s.\n",
+				subjectCert->subjectName ? subjectCert->subjectName : "(null)",
+				subjectCert->issuerName ? subjectCert->issuerName : "(null)",
+				issuerCert->subjectName ? issuerCert->subjectName : "(null)");
 		return FALSE;
+	}
+
+	/* Ensure issuer has CA flag set to true in the basic constraints
+	   extension. */
+	st = my_CERT_FindBasicConstraintExten(issuerCert, &basicConstraint);
+	if (st == SECSuccess) {
+		/* Successfully read basic constraints extension from issuer cert. */
+		if (basicConstraint.isCA == PR_FALSE) {
+			/* CA flag is set to false! */
+			purple_debug_error("nss/x509", "CA flag is set to false in the "
+					"basic constraints extension for issuer cert %s.\n",
+					issuerCert->subjectName ? issuerCert->subjectName : "(null)");
+			return FALSE;
+		}
+		/* CA flag is set to true. Great. */
+	} else {
+		/* Error reading basic constraints extension from issuer cert. */
+		if (PORT_GetError() != SEC_ERROR_EXTENSION_NOT_FOUND) {
+			/* Unknown error. */
+			purple_debug_error("nss/x509", "Error attempting to read basic "
+					"constraints from issuer cert %s. ret=%d\n",
+					issuerCert->subjectName ? issuerCert->subjectName : "(null)",
+					PORT_GetError());
+			return FALSE;
+		}
+		/* Basic constraints extension is absent. Allow issuer to act as a CA. */
+	}
+
+	/* Make sure issuer cert is an SSL CA. */
+	if (!(issuerCert->nsCertType & NS_CERT_TYPE_SSL_CA)) {
+		purple_debug_error("nss/x509", "Issuer cert %s is not an SSL CA.\n",
+				issuerCert->subjectName ? issuerCert->subjectName : "(null)");
+		return FALSE;
+	}
+
+	/* Make sure key usage allows cert signing. Would ideally call
+	   CERT_CheckKeyUsage here, but that function seems to not be exported
+	   even though it's listed in nss/cert.h. */
+	if (!(issuerCert->keyUsage & KU_KEY_CERT_SIGN)) {
+		purple_debug_error("nss/x509",
+				"Issuer cert %s does not allow signing.\n",
+				issuerCert->subjectName ? issuerCert->subjectName : "(null)");
+		return FALSE;
+	}
+
 	st = CERT_VerifySignedData(&subjectCert->signatureWrap, issuerCert, PR_Now(), NULL);
 	return st == SECSuccess;
 }