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

@@ -561,10 +561,10 @@ Result CertVerifier::VerifyCert(
               ? NSSCertDBTrustDomain::LocalOnlyOCSPForEV
               : NSSCertDBTrustDomain::FetchOCSPForEV;
 
-      CertPolicyId evPolicy;
-      bool foundEVPolicy = GetFirstEVPolicy(certBytes, evPolicy);
+      nsTArray<CertPolicyId> 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;

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<mozilla::pkix::CertPolicyId>& 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<uint8_t>& certBytes,
-                      /*out*/ mozilla::pkix::CertPolicyId& policy) {
+void GetKnownEVPolicies(
+    const nsTArray<uint8_t>& certBytes,
+    /*out*/ nsTArray<mozilla::pkix::CertPolicyId>& 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<uint8_t>& 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<uint8_t>& 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<uint8_t>& 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

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<uint8_t>& cert,
-                      /*out*/ mozilla::pkix::CertPolicyId& policy);
+void GetKnownEVPolicies(
+    const nsTArray<uint8_t>& cert,
+    /*out*/ nsTArray<mozilla::pkix::CertPolicyId>& policies);
 
 // CertIsAuthoritativeForEVPolicy does NOT evaluate whether the cert is trusted
 // or distrusted.

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