From 0cd5a69a4c2d965b6ba99780f56d000d82a78916 Mon Sep 17 00:00:00 2001 From: Dana Keeler Date: Wed, 15 Jun 2022 18:20:13 +0000 Subject: [PATCH] Bug 1769150 - try all known EV policy OIDs found in a certificate when verifying for EV r=jschanck Before this patch, the certificate verifier would only attempt to build a trusted path to a root with the first recognized EV OID in the end-entity certificate. Thus, if an end-entity certificate had more than one EV OID, it could fail to verify as EV if an intermediate or root had the "wrong" EV OID. This patch addresses this shortcoming by trying to build a path with each recognized EV OID in the end-entity certificate until it finds one that works. Differential Revision: https://phabricator.services.mozilla.com/D149319 --- security/certverifier/CertVerifier.cpp | 9 ++-- security/certverifier/ExtendedValidation.cpp | 45 +++++++++---------- security/certverifier/ExtendedValidation.h | 17 +++---- .../manager/ssl/tests/unit/test_ev_certs.js | 14 +++--- 4 files changed, 41 insertions(+), 44 deletions(-) diff --git a/security/certverifier/CertVerifier.cpp b/security/certverifier/CertVerifier.cpp index 0a4b0a9688bc..1d5a3844c3ed 100644 --- a/security/certverifier/CertVerifier.cpp +++ b/security/certverifier/CertVerifier.cpp @@ -561,10 +561,10 @@ Result CertVerifier::VerifyCert( ? NSSCertDBTrustDomain::LocalOnlyOCSPForEV : NSSCertDBTrustDomain::FetchOCSPForEV; - CertPolicyId evPolicy; - bool foundEVPolicy = GetFirstEVPolicy(certBytes, evPolicy); + nsTArray evPolicies; + GetKnownEVPolicies(certBytes, evPolicies); rv = Result::ERROR_UNKNOWN_ERROR; - if (foundEVPolicy) { + for (const auto& evPolicy : evPolicies) { NSSCertDBTrustDomain trustDomain( trustSSL, evOCSPFetching, mOCSPCache, pinArg, mOCSPTimeoutSoft, mOCSPTimeoutHard, mCertShortLifetimeInDays, MIN_RSA_BITS, @@ -594,6 +594,9 @@ Result CertVerifier::VerifyCert( break; } } + if (rv == Success) { + break; + } if (flags & FLAG_MUST_BE_EV) { rv = Result::ERROR_POLICY_VALIDATION_FAILED; break; diff --git a/security/certverifier/ExtendedValidation.cpp b/security/certverifier/ExtendedValidation.cpp index 6a78bffbad01..67b144b6a4c0 100644 --- a/security/certverifier/ExtendedValidation.cpp +++ b/security/certverifier/ExtendedValidation.cpp @@ -1276,34 +1276,35 @@ nsresult LoadExtendedValidationInfo() { return NS_OK; } -// Helper function for GetFirstEVPolicy(): reads an EV Policy if there is one -bool FindMatchingEVPolicy(Reader& idReader, - mozilla::pkix::CertPolicyId& policy) { +// Helper function for GetKnownEVPolicies(): reads an EV Policy if there is one, +// and appends it to the given list of CertPolicyIds. +void FindMatchingEVPolicy(Reader& idReader, + nsTArray& policies) { Input cabForumEVIdBytes; Result rv = cabForumEVIdBytes.Init(sCABForumEVId.bytes, sCABForumEVId.numBytes); if (rv == Success && idReader.MatchRest(cabForumEVIdBytes)) { - policy = sCABForumEVId; - return true; + policies.AppendElement(sCABForumEVId); + return; } for (const CertPolicyId& id : sEVInfoIds) { Input idBytes; rv = idBytes.Init(id.bytes, id.numBytes); if (rv == Success && idReader.MatchRest(idBytes)) { - policy = id; - return true; + policies.AppendElement(id); + return; } } - return false; } -bool GetFirstEVPolicy(const nsTArray& certBytes, - /*out*/ mozilla::pkix::CertPolicyId& policy) { +void GetKnownEVPolicies( + const nsTArray& certBytes, + /*out*/ nsTArray& policies) { Input certInput; Result rv = certInput.Init(certBytes.Elements(), certBytes.Length()); if (rv != Success) { - return false; + return; } // we don't use the certificate for path building, so this parameter // doesn't matter @@ -1311,15 +1312,15 @@ bool GetFirstEVPolicy(const nsTArray& certBytes, BackCert cert(certInput, notUsedForPaths, nullptr); rv = cert.Init(); if (rv != Success) { - return false; + return; } - const Input* policies = cert.GetCertificatePolicies(); - if (!policies) { - return false; + const Input* extensionInput = cert.GetCertificatePolicies(); + if (!extensionInput) { + return; } - Reader extension(*policies); + Reader extension(*extensionInput); Reader certificatePolicies; // certificatePolicies ::= SEQUENCE SIZE (1..MAX) OF PolicyInformation // PolicyInformation ::= SEQUENCE { @@ -1329,7 +1330,7 @@ bool GetFirstEVPolicy(const nsTArray& certBytes, // CertPolicyId ::= OBJECT IDENTIFIER rv = der::ExpectTagAndGetValue(extension, der::SEQUENCE, certificatePolicies); if (rv != Success || !extension.AtEnd()) { - return false; + return; } do { @@ -1337,22 +1338,18 @@ bool GetFirstEVPolicy(const nsTArray& certBytes, rv = der::ExpectTagAndGetValue(certificatePolicies, der::SEQUENCE, policyInformation); if (rv != Success) { - return false; + return; } Reader policyOid; rv = der::ExpectTagAndGetValue(policyInformation, der::OIDTag, policyOid); if (rv != Success) { - return false; + return; } // we don't validate policy qualifiers here - if (FindMatchingEVPolicy(policyOid, policy)) { - return true; - } + FindMatchingEVPolicy(policyOid, policies); } while (!certificatePolicies.AtEnd()); - - return false; } } // namespace psm diff --git a/security/certverifier/ExtendedValidation.h b/security/certverifier/ExtendedValidation.h index fba4ba24333c..c090f6d8e36a 100644 --- a/security/certverifier/ExtendedValidation.h +++ b/security/certverifier/ExtendedValidation.h @@ -21,19 +21,16 @@ namespace psm { nsresult LoadExtendedValidationInfo(); /** - * Finds the first policy OID in the given cert that is known to be an EV policy - * OID. + * Finds all policy OIDs in the given cert that are known to be EV policy OIDs. * * @param cert - * The cert to find the first EV policy of. - * @param policy - * The found policy. - * @param policyOidTag - * The OID tag of the found policy. - * @return true if a suitable policy was found, false otherwise. + * The bytes of the cert to find the EV policies of. + * @param policies + * The found policies. */ -bool GetFirstEVPolicy(const nsTArray& cert, - /*out*/ mozilla::pkix::CertPolicyId& policy); +void GetKnownEVPolicies( + const nsTArray& cert, + /*out*/ nsTArray& policies); // CertIsAuthoritativeForEVPolicy does NOT evaluate whether the cert is trusted // or distrusted. diff --git a/security/manager/ssl/tests/unit/test_ev_certs.js b/security/manager/ssl/tests/unit/test_ev_certs.js index b20bdb7188a8..2ebb63de2e36 100644 --- a/security/manager/ssl/tests/unit/test_ev_certs.js +++ b/security/manager/ssl/tests/unit/test_ev_certs.js @@ -220,9 +220,14 @@ add_task(async function plainExpectSuccessEVTests() { await ensureVerifiesAsEV("reverse-order-oids-path"); // In this case, the end-entity has both the CA/B Forum OID and the test OID // (in that order). The intermediate has the CA/B Forum OID. Since the - // implementation uses the first EV policy it encounters in the end-entity as - // the required one, this successfully verifies as EV. + // implementation tries all EV policies it encounters, this successfully + // verifies as EV. await ensureVerifiesAsEV("cabforum-and-test-oid-ee-cabforum-oid-int-path"); + // In this case, the end-entity has both the test OID and the CA/B Forum OID + // (in that order). The intermediate has only the CA/B Forum OID. Since the + // implementation tries all EV policies it encounters, this successfully + // verifies as EV. + await ensureVerifiesAsEV("test-and-cabforum-oid-ee-cabforum-oid-int-path"); }); // These fail for various reasons to verify as EV, but fallback to DV should @@ -236,11 +241,6 @@ add_task(async function expectDVFallbackTests() { // CA/B Forum OID. Since the CA/B Forum OID is not treated the same as the // anyPolicy OID, this will not verify as EV. await ensureVerifiesAsDV("test-oid-ee-cabforum-oid-int-path"); - // In this case, the end-entity has both the test OID and the CA/B Forum OID - // (in that order). The intermediate has only the CA/B Forum OID. Since the - // implementation uses the first EV policy it encounters in the end-entity as - // the required one, this fails to verify as EV. - await ensureVerifiesAsDV("test-and-cabforum-oid-ee-cabforum-oid-int-path"); }); // Test that removing the trust bits from an EV root causes verifications