net: CertVerifyProcBuiltin: if EV rev checking hits deadline, try DV anyway.

Bug: 1004507
Change-Id: Ic61fc9e819ef26d2eb5a128d37a65001d7d212fe
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1822901
Commit-Queue: Matt Mueller <mattm@chromium.org>
Reviewed-by: Eric Roman <eroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#700776}

net/cert/cert_verify_proc_builtin.cc

@@ -43,6 +43,9 @@ constexpr uint32_t kPathBuilderIterationLimit = 25000;
 constexpr base::TimeDelta kMaxVerificationTime =
     base::TimeDelta::FromSeconds(60);
 
+constexpr base::TimeDelta kPerAttemptMinVerificationTimeLimit =
+    base::TimeDelta::FromSeconds(5);
+
 DEFINE_CERT_ERROR_ID(kPathLacksEVPolicy, "Path does not have an EV policy");
 
 RevocationPolicy NoRevocationChecking() {
@@ -589,13 +592,8 @@ int AssignVerifyResult(X509Certificate* input_cert,
 // This implementation is simplistic, and looks only for the presence of the
 // kUnacceptableSignatureAlgorithm error somewhere among the built paths.
 bool CanTryAgainWithWeakerDigestPolicy(const CertPathBuilder::Result& result) {
-  for (const auto& path : result.paths) {
-    if (path->errors.ContainsError(
-            cert_errors::kUnacceptableSignatureAlgorithm))
-      return true;
-  }
-
-  return false;
+  return result.AnyPathContainsError(
+      cert_errors::kUnacceptableSignatureAlgorithm);
 }
 
 int CertVerifyProcBuiltin::VerifyInternal(
@@ -672,6 +670,12 @@ int CertVerifyProcBuiltin::VerifyInternal(
     const auto& cur_attempt = attempts[cur_attempt_index];
     verification_type = cur_attempt.verification_type;
 
+    // If a previous attempt used up most/all of the deadline, extend the
+    // deadline a little bit to give this verification attempt a chance at
+    // success.
+    deadline = std::max(
+        deadline, base::TimeTicks::Now() + kPerAttemptMinVerificationTimeLimit);
+
     // Run the attempt through the path builder.
     result = TryBuildPath(
         target, &intermediates, ssl_trust_store.get(), verification_time,
@@ -679,9 +683,23 @@ int CertVerifyProcBuiltin::VerifyInternal(
         flags, ocsp_response, crl_set, net_fetcher_.get(), ev_metadata,
         &checked_revocation_for_some_path);
 
-    if (result.HasValidPath() || result.exceeded_deadline)
+    if (result.HasValidPath())
       break;
 
+    if (result.exceeded_deadline) {
+      if (verification_type == VerificationType::kEV &&
+          result.AnyPathContainsError(cert_errors::kUnableToCheckRevocation)) {
+        // EV verification failed due to deadline exceeded and unable to check
+        // revocation. Try the non-EV attempt even though the deadline has been
+        // reached, since a revocation checking failure on EV should be a
+        // soft-fail. (Since the non-EV attempt generally will not be using
+        // revocation checking it hopefully won't hit the deadline too.)
+        continue;
+      }
+      // Otherwise, stop immediately if an attempt exceeds the deadline.
+      break;
+    }
+
     // If this path building attempt (may have) failed due to the chain using a
     // weak signature algorithm, enqueue a similar attempt but with weaker
     // signature algorithms (SHA1) permitted.

net/cert/cert_verify_proc_builtin_unittest.cc

@@ -10,6 +10,7 @@
 #include "net/base/test_completion_callback.h"
 #include "net/cert/cert_verify_proc.h"
 #include "net/cert/crl_set.h"
+#include "net/cert/ev_root_ca_metadata.h"
 #include "net/cert/internal/system_trust_store.h"
 #include "net/cert_net/cert_net_fetcher_impl.h"
 #include "net/log/net_log_with_source.h"
@@ -296,4 +297,77 @@ TEST_F(CertVerifyProcBuiltinTest, RevocationCheckDeadlineOCSP) {
   EXPECT_THAT(error, IsOk());
 }
 
+#if defined(PLATFORM_USES_CHROMIUM_EV_METADATA)
+// Tests that if the verification deadline is exceeded during EV revocation
+// checking, the certificate is verified as non-EV.
+TEST_F(CertVerifyProcBuiltinTest, EVRevocationCheckDeadline) {
+  std::unique_ptr<CertBuilder> leaf, intermediate, root;
+  CreateChain(&leaf, &intermediate, &root);
+  ASSERT_TRUE(leaf && intermediate && root);
+
+  // Add test EV policy to leaf and intermediate.
+  static const char kEVTestCertPolicy[] = "1.2.3.4";
+  leaf->SetCertificatePolicies({kEVTestCertPolicy});
+  intermediate->SetCertificatePolicies({kEVTestCertPolicy});
+
+  const base::TimeDelta timeout_increment =
+      CertNetFetcherImpl::GetDefaultTimeoutForTesting() +
+      base::TimeDelta::FromMilliseconds(1);
+  const int expected_request_count =
+      GetCertVerifyProcBuiltinTimeLimitForTesting() / timeout_increment + 1;
+
+  EmbeddedTestServer test_server(EmbeddedTestServer::TYPE_HTTP);
+  ASSERT_TRUE(test_server.InitializeAndListen());
+
+  // Set up the test intermediate to have enough OCSP urls that if all the
+  // requests hang the deadline will be exceeded.
+  std::vector<GURL> ocsp_urls;
+  std::vector<base::RunLoop> runloops(expected_request_count);
+  for (int i = 0; i < expected_request_count; ++i) {
+    std::string path = base::StringPrintf("/hung/%i", i);
+    ocsp_urls.emplace_back(test_server.GetURL(path));
+    test_server.RegisterRequestHandler(
+        base::BindRepeating(&test_server::HandlePrefixedRequest, path,
+                            base::BindRepeating(&HangRequestAndCallback,
+                                                runloops[i].QuitClosure())));
+  }
+  intermediate->SetCaIssuersAndOCSPUrls({}, ocsp_urls);
+
+  test_server.StartAcceptingConnections();
+
+  // Consider the root of the test chain a valid EV root for the test policy.
+  ScopedTestEVPolicy scoped_test_ev_policy(
+      EVRootCAMetadata::GetInstance(),
+      X509Certificate::CalculateFingerprint256(root->GetCertBuffer()),
+      kEVTestCertPolicy);
+
+  scoped_refptr<X509Certificate> chain = leaf->GetX509CertificateChain();
+  ASSERT_TRUE(chain.get());
+
+  CertVerifyResult verify_result;
+  TestCompletionCallback verify_callback;
+  Verify(chain.get(), "www.example.com",
+         /*flags=*/0,
+         /*additional_trust_anchors=*/{root->GetX509Certificate()},
+         &verify_result, verify_callback.callback());
+
+  for (int i = 0; i < expected_request_count; i++) {
+    // Wait for request #|i| to be made.
+    runloops[i].Run();
+    // Advance virtual time to cause the timeout task to become runnable.
+    task_environment().AdvanceClock(timeout_increment);
+  }
+
+  // Once |expected_request_count| requests have been made and timed out, the
+  // overall deadline should be reached, causing the EV verification attempt to
+  // fail.
+  int error = verify_callback.WaitForResult();
+  // EV uses soft-fail revocation checking, therefore verification result
+  // should be OK but not EV.
+  EXPECT_THAT(error, IsOk());
+  EXPECT_FALSE(verify_result.cert_status & CERT_STATUS_IS_EV);
+  EXPECT_TRUE(verify_result.cert_status & CERT_STATUS_REV_CHECKING_ENABLED);
+}
+#endif  // defined(PLATFORM_USES_CHROMIUM_EV_METADATA)
+
 }  // namespace net

net/cert/internal/path_builder.cc

@@ -525,6 +525,15 @@ bool CertPathBuilder::Result::HasValidPath() const {
   return GetBestValidPath() != nullptr;
 }
 
+bool CertPathBuilder::Result::AnyPathContainsError(CertErrorId error_id) const {
+  for (const auto& path : paths) {
+    if (path->errors.ContainsError(error_id))
+      return true;
+  }
+
+  return false;
+}
+
 const CertPathBuilderResultPath* CertPathBuilder::Result::GetBestValidPath()
     const {
   const CertPathBuilderResultPath* result_path = GetBestPathPossiblyInvalid();

net/cert/internal/path_builder.h

@@ -116,6 +116,9 @@ class NET_EXPORT CertPathBuilder {
     // Returns true if there was a valid path.
     bool HasValidPath() const;
 
+    // Returns true if any of the attempted paths contain |error_id|.
+    bool AnyPathContainsError(CertErrorId error_id) const;
+
     // Returns the CertPathBuilderResultPath for the best valid path, or nullptr
     // if there was none.
     const CertPathBuilderResultPath* GetBestValidPath() const;

net/test/cert_builder.cc

@@ -291,6 +291,36 @@ void CertBuilder::SetSubjectAltName(const std::string& dns_name) {
   SetExtension(SubjectAltNameOid(), FinishCBB(cbb.get()));
 }
 
+void CertBuilder::SetCertificatePolicies(
+    const std::vector<std::string>& policy_oids) {
+  // From RFC 5280:
+  //    certificatePolicies ::= SEQUENCE SIZE (1..MAX) OF PolicyInformation
+  //
+  //    PolicyInformation ::= SEQUENCE {
+  //         policyIdentifier   CertPolicyId,
+  //         policyQualifiers   SEQUENCE SIZE (1..MAX) OF
+  //                                 PolicyQualifierInfo OPTIONAL }
+  //
+  //    CertPolicyId ::= OBJECT IDENTIFIER
+  bssl::ScopedCBB cbb;
+  CBB certificate_policies;
+  ASSERT_TRUE(CBB_init(cbb.get(), 64));
+  ASSERT_TRUE(
+      CBB_add_asn1(cbb.get(), &certificate_policies, CBS_ASN1_SEQUENCE));
+  for (const auto& oid : policy_oids) {
+    CBB policy_information, policy_identifier;
+    ASSERT_TRUE(CBB_add_asn1(&certificate_policies, &policy_information,
+                             CBS_ASN1_SEQUENCE));
+    ASSERT_TRUE(
+        CBB_add_asn1(&policy_information, &policy_identifier, CBS_ASN1_OBJECT));
+    ASSERT_TRUE(
+        CBB_add_asn1_oid_from_text(&policy_identifier, oid.data(), oid.size()));
+    ASSERT_TRUE(CBB_flush(&certificate_policies));
+  }
+
+  SetExtension(CertificatePoliciesOid(), FinishCBB(cbb.get()));
+}
+
 void CertBuilder::SetValidity(base::Time not_before, base::Time not_after) {
   // From RFC 5280:
   //   Validity ::= SEQUENCE {

net/test/cert_builder.h

@@ -74,6 +74,10 @@ class CertBuilder {
   // Sets the SAN for the certificate to a single dNSName.
   void SetSubjectAltName(const std::string& dns_name);
 
+  // Sets the certificatePolicies extension with the specified policyIdentifier
+  // OIDs, which must be specified in dotted string notation (e.g. "1.2.3.4").
+  void SetCertificatePolicies(const std::vector<std::string>& policy_oids);
+
   void SetValidity(base::Time not_before, base::Time not_after);
 
   // Sets the signature algorithm for the certificate to either