Blob Blame History Raw
From 32d6c776ad5c489efea9f380fdac296a63209ea5 Mon Sep 17 00:00:00 2001
From: Alexander Scheel <ascheel@redhat.com>
Date: Wed, 4 Sep 2019 08:33:14 -0400
Subject: [PATCH] Fix root certificate validation

When the Leaf and Chain OCSP checking policy is enabled in
CryptoManager, JSS will switch to alternative certificate verification
logic in JSSL_DefaultCertAuthCallback. In this method, the root
certificate was incorrectly trusted without being verified to exist in
the trust store.

This patch cleans up the logic in JSSL_verifyCertPKIX and makes it
more explicit in addition to fixing the error.

Fixes CVE-2019-14823

Signed-off-by: Alexander Scheel <ascheel@redhat.com>
---
 org/mozilla/jss/ssl/common.c | 239 ++++++++++++++++++++---------------
 1 file changed, 138 insertions(+), 101 deletions(-)

diff --git a/org/mozilla/jss/ssl/common.c b/org/mozilla/jss/ssl/common.c
index 8c2a2240..030588c8 100644
--- a/org/mozilla/jss/ssl/common.c
+++ b/org/mozilla/jss/ssl/common.c
@@ -936,170 +936,207 @@ finish:
     return root; 
 }
 
-/* Verify a cert using explicit PKIX call.
- * For now only used in OCSP AIA context.
- * The result of this call will be a full chain
- * and leaf network AIA ocsp validation.
- * The policy param will be used in the future to
- * handle more scenarios.
- */
-
-SECStatus JSSL_verifyCertPKIX(CERTCertificate *cert,
-      SECCertificateUsage certificateUsage,secuPWData *pwdata, int ocspPolicy,
-      CERTVerifyLog *log, SECCertificateUsage *usage) 
+/* Internal helper for the below call. */
+static SECStatus
+JSSL_verifyCertPKIXInternal(CERTCertificate *cert,
+    SECCertificateUsage certificateUsage, secuPWData *pwdata, int ocspPolicy,
+    CERTVerifyLog *log, SECCertificateUsage *usage,
+    CERTCertList *trustedCertList)
 {
-
-    /* put the first set of possible flags internally here first */
-    /* later there could be a more complete list to choose from */
-    /* support our hard core fetch aia ocsp policy for now */
-
-    static PRUint64 ocsp_Enabled_Hard_Policy_LeafFlags[2] = {
+    /* Put the first set of possible flags internally here first. Later
+     * there could be a more complete list to choose from; for now we only
+     * support our hard core fetch AIA OCSP policy. Note that we disable
+     * CRL fetching as Dogtag doesn't support it. Additionally, enable OCSP
+     * checking on the chained CA certificates. Since NSS/PKIX's
+     * CERT_GetClassicOCSPEnabledHardFailurePolicy doesn't do what we want,
+     * we construct the policy ourselves. */
+    PRUint64 ocsp_Enabled_Hard_Policy_LeafFlags[2] = {
         /* crl */
-        0,
+        CERT_REV_M_DO_NOT_TEST_USING_THIS_METHOD,
         /* ocsp */
         CERT_REV_M_TEST_USING_THIS_METHOD |
-        CERT_REV_M_FAIL_ON_MISSING_FRESH_INFO
+            CERT_REV_M_FAIL_ON_MISSING_FRESH_INFO
     };
 
-    static PRUint64 ocsp_Enabled_Hard_Policy_ChainFlags[2] = {
+    PRUint64 ocsp_Enabled_Hard_Policy_ChainFlags[2] = {
         /* crl */
-        0,
+        CERT_REV_M_DO_NOT_TEST_USING_THIS_METHOD,
         /* ocsp */
         CERT_REV_M_TEST_USING_THIS_METHOD |
-        CERT_REV_M_FAIL_ON_MISSING_FRESH_INFO
+            CERT_REV_M_FAIL_ON_MISSING_FRESH_INFO
     };
 
-    static CERTRevocationMethodIndex
-        ocsp_Enabled_Hard_Policy_Method_Preference = {
-            cert_revocation_method_ocsp
-        };
-
-    static CERTRevocationFlags ocsp_Enabled_Hard_Policy = {
-    { /* leafTests */
-      2,
-      ocsp_Enabled_Hard_Policy_LeafFlags,
-      1,
-      &ocsp_Enabled_Hard_Policy_Method_Preference,
-      0 },
-    { /* chainTests */
-      2,
-      ocsp_Enabled_Hard_Policy_ChainFlags,
-      1,
-      &ocsp_Enabled_Hard_Policy_Method_Preference,
-      0 }
+    CERTRevocationMethodIndex ocsp_Enabled_Hard_Policy_Method_Preference[1] = {
+        cert_revocation_method_ocsp
     };
 
-    /* for future expansion */
+    CERTRevocationFlags ocsp_Enabled_Hard_Policy = {
+        /* CERTRevocationTests - leafTests */
+        {
+            /* number_of_defined_methods */
+            2,
+            /* cert_rev_flags_per_method */
+            ocsp_Enabled_Hard_Policy_LeafFlags,
+            /* number_of_preferred_methods */
+            1,
+            /* preferred_methods */
+            ocsp_Enabled_Hard_Policy_Method_Preference,
+            /* cert_rev_method_independent_flags */
+            0
+        },
+        /* CERTRevocationTests - chainTests */
+        {
+            /* number_of_defined_methods */
+            2,
+            /* cert_rev_flags_per_method */
+            ocsp_Enabled_Hard_Policy_ChainFlags,
+            /* number_of_preferred_methods */
+            1,
+            /* preferred_methods */
+            ocsp_Enabled_Hard_Policy_Method_Preference,
+            /* cert_rev_method_independent_flags */
+            0
+        }
+    };
 
-    CERTValOutParam cvout[20] = {0};
-    CERTValInParam cvin[20] = {0};
+    /* The size of these objects are defined here based upon maximum possible
+     * inputs. A dynamic allocation could reallocate based upon actual usage,
+     * however this would affect the size by at most one or two. Note that,
+     * due to the required usage of cert_pi_end/cert_po_end, these sizes are
+     * inflated by one. */
+    CERTValOutParam cvout[3] = {{0}};
+    CERTValInParam cvin[6] = {{0}};
 
+    int usageIndex = -1;
     int inParamIndex = 0;
     int outParamIndex = 0;
-    CERTRevocationFlags *rev = NULL;
 
-    CERTCertList *trustedCertList = NULL;
-
-    PRBool fetchCerts = PR_FALSE;
-
-    SECCertUsage certUsage = certUsageSSLClient /* 0 */;
-    
     SECStatus res =  SECFailure;
-    if(cert == NULL) {
+
+    if (cert == NULL) {
         goto finish;
     }
 
-    if(ocspPolicy != OCSP_LEAF_AND_CHAIN_POLICY) {
+    if (ocspPolicy != OCSP_LEAF_AND_CHAIN_POLICY) {
         goto finish;
     }
 
-    /* Force the strict ocsp network check on chain
-       and leaf.
-    */
-
-    fetchCerts = PR_TRUE;   
-    rev = &ocsp_Enabled_Hard_Policy;
-
-    /* fetch aia over net */
- 
+    /* Enable live AIA fetching over the network. */
     cvin[inParamIndex].type = cert_pi_useAIACertFetch;
-    cvin[inParamIndex].value.scalar.b = fetchCerts;
-    inParamIndex++; 
-
-    /* time */
+    cvin[inParamIndex].value.scalar.b = PR_TRUE;
+    inParamIndex++;
 
+    /* By setting the time to zero, we choose the current time when the
+     * check is performed. */
     cvin[inParamIndex].type = cert_pi_date;
-    cvin[inParamIndex].value.scalar.time = PR_Now();
+    cvin[inParamIndex].value.scalar.time = 0;
     inParamIndex++;
 
-    /* flags */
-
+    /* Force the strict OCSP check on both the leaf and its chain. */
     cvin[inParamIndex].type = cert_pi_revocationFlags;
-    cvin[inParamIndex].value.pointer.revocation = rev;
+    cvin[inParamIndex].value.pointer.revocation = &ocsp_Enabled_Hard_Policy;
     inParamIndex++;
 
-    /* establish trust anchor */
-
-    /* We need to convert the SECCertificateUsage to a SECCertUsage to obtain
-     * the root.
-    */
-
-    SECCertificateUsage testUsage = certificateUsage;
-    while (0 != (testUsage = testUsage >> 1)) { certUsage++; }
-
-    CERTCertificate *root = getRoot(cert,certUsage);
-
-    /* Try to add the root as the trust anchor so all the
-       other memebers of the ca chain will get validated.
-    */
-
-    if( root != NULL ) {
-        trustedCertList = CERT_NewCertList();
-        CERT_AddCertToListTail(trustedCertList, root);        
-
+    /* Establish a trust anchor if it is passed to us. NOTE: this trust anchor
+     * must previously be validated before it is passed to us here. */
+    if (trustedCertList != NULL) {
         cvin[inParamIndex].type = cert_pi_trustAnchors;
         cvin[inParamIndex].value.pointer.chain = trustedCertList;
-
         inParamIndex++;
     }
 
+    /* Done establishing input parameters. */
     cvin[inParamIndex].type = cert_pi_end;
 
-    if(log != NULL) {
+    /* When we need to log rationale for failure, pass it as an output
+     * parameter. */
+    if (log != NULL) {
         cvout[outParamIndex].type = cert_po_errorLog;
         cvout[outParamIndex].value.pointer.log = log;
         outParamIndex ++;
     }
 
-    int usageIndex = 0;
-    if(usage != NULL) {
+    /* When we need to inquire about the resulting certificate usage, pass it
+     * here. */
+    if (usage != NULL) {
         usageIndex = outParamIndex;
         cvout[outParamIndex].type = cert_po_usages;
         cvout[outParamIndex].value.scalar.usages = 0;
         outParamIndex ++;
     }
 
+    /* Done establishing output parameters. */
     cvout[outParamIndex].type = cert_po_end;
 
+    /* Call into NSS's PKIX library to validate our certificate. */
     res = CERT_PKIXVerifyCert(cert, certificateUsage, cvin, cvout, &pwdata);
 
 finish:
-    /* clean up any trusted cert list */
-
+    /* Clean up any certificates in the trusted certificate list. This was
+     * a passed input parameter, but by taking ownership of it and clearing it,
+     * we enable tail calls to this function. */
     if (trustedCertList) {
+        /* CERT_DestroyCertList destroys interior certs for us. */
         CERT_DestroyCertList(trustedCertList);
         trustedCertList = NULL;
     }
 
-    /* CERT_DestroyCertList destroys interior certs for us. */
-
-    if(root) {
-       root = NULL;
-    }
-
-    if(res == SECSuccess && usage) {
+    if (res == SECSuccess && usage && usageIndex != -1) {
         *usage = cvout[usageIndex].value.scalar.usages;
     }
 
     return res;
 }
+
+/* Verify a cert using an explicit PKIX call. For now only perform this call
+ * when the OCSP policy is set to leaf and chain. Performs a blocking, online
+ * OCSP status refresh. The result of this call will be a full-chain OCSP
+ * validation.
+ *
+ * In the future, we'll use ocspPolicy to condition around additional policies
+ * and handle them all with this method (and a call to PKIX).
+ *
+ * Note that this currently requires the certificate to be added directly
+ * to the NSS DB. We can't otherwise validate against root certificates in
+ * the default NSS DB.
+ */
+SECStatus JSSL_verifyCertPKIX(CERTCertificate *cert,
+      SECCertificateUsage certificateUsage, secuPWData *pwdata, int ocspPolicy,
+      CERTVerifyLog *log, SECCertificateUsage *usage)
+{
+    SECCertUsage certUsage = certUsageSSLClient /* 0 */;
+
+    /* We need to convert the SECCertificateUsage to a SECCertUsage to obtain
+     * the root.
+     */
+
+    SECCertificateUsage testUsage = certificateUsage;
+    while (0 != (testUsage = testUsage >> 1)) { certUsage++; }
+
+    CERTCertificate *root = getRoot(cert, certUsage);
+
+    // Two cases: either the root is present, or it isn't.
+    if (root == NULL) {
+        /* In this case, we've had a hard time finding the root. In all
+         * likelihood, the following call will fail to validate the end cert
+         * as well and thus fail to validate. I don't believe there's a risk
+         * in trying it however. */
+        return JSSL_verifyCertPKIXInternal(cert, certificateUsage, pwdata,
+                                           ocspPolicy, log, usage, NULL);
+    } else {
+        /* In this case, we've found the root certificate. Before passing it
+         * to the leaf, explicitly validate it with strict OCSP checking. Then
+         * validate the leaf certificate with a known and trusted root
+         * certificate. */
+        SECStatus ret = JSSL_verifyCertPKIXInternal(root, certificateUsageSSLCA,
+            pwdata, ocspPolicy, log, usage, NULL);
+        if (ret != SECSuccess) {
+            return ret;
+        }
+
+        CERTCertList *rootList = CERT_NewCertList();
+        CERT_AddCertToListTail(rootList, root);
+        return JSSL_verifyCertPKIXInternal(cert, certificateUsage, pwdata,
+                                           ocspPolicy, log, usage, rootList);
+    }
+}
-- 
2.21.0