From 32d6c776ad5c489efea9f380fdac296a63209ea5 Mon Sep 17 00:00:00 2001 From: Alexander Scheel 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 --- 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