Disable secure DNS for certificate network fetches.

During the DoH auto-upgrade experiment we disabled OCSP/CRL fetches for
connections to DoH servers. This change takes a different approach to
breaking the potential deadlock and instead disables DoH for all
OCSP/CRL/AIA fetches.

Bug: 990827
Change-Id: I9885cac230aafaa41ba183b5799e771ba7c524e0
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1843334
Commit-Queue: Katharine Daly <dalyk@google.com>
Reviewed-by: Eric Roman <eroman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#746618}

net/cert_net/cert_net_fetcher_url_request.cc

@@ -506,6 +506,9 @@ void Job::StartURLRequest(URLRequestContext* context) {
   if (request_params_->http_method == HTTP_METHOD_POST)
     url_request_->set_method("POST");
   url_request_->set_allow_credentials(false);
+  // Disable secure DNS for hostname lookups triggered by certificate network
+  // fetches to prevent deadlock.
+  url_request_->SetDisableSecureDns(true);
   url_request_->Start();
 
   // Start a timer to limit how long the job runs for.

net/cert_net/cert_net_fetcher_url_request_unittest.cc

@@ -26,6 +26,7 @@
 #include "net/test/url_request/url_request_hanging_read_job.h"
 #include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
 #include "net/url_request/url_request_filter.h"
+#include "net/url_request/url_request_interceptor.h"
 #include "net/url_request/url_request_job_factory_impl.h"
 #include "net/url_request/url_request_test_util.h"
 #include "testing/gmock/include/gmock/gmock.h"
@@ -43,6 +44,8 @@ namespace {
 const base::FilePath::CharType kDocRoot[] =
     FILE_PATH_LITERAL("net/data/cert_net_fetcher_impl_unittest");
 
+const char kMockSecureDnsHostname[] = "mock.secure.dns.check";
+
 // A non-mock URLRequestContext which can access http:// urls.
 class RequestContext : public URLRequestContext {
  public:
@@ -247,6 +250,47 @@ class CertNetFetcherURLRequestTestWithHangingReadHandler
   void TearDown() override { URLRequestFilter::GetInstance()->ClearHandlers(); }
 };
 
+// Interceptor to check that secure DNS has been disabled.
+class SecureDnsInterceptor : public net::URLRequestInterceptor {
+ public:
+  explicit SecureDnsInterceptor(bool* invoked_interceptor)
+      : invoked_interceptor_(invoked_interceptor) {}
+  ~SecureDnsInterceptor() override = default;
+
+ private:
+  // URLRequestInterceptor implementation:
+  net::URLRequestJob* MaybeInterceptRequest(
+      net::URLRequest* request,
+      net::NetworkDelegate* network_delegate) const override {
+    EXPECT_TRUE(request->disable_secure_dns());
+    *invoked_interceptor_ = true;
+    return nullptr;
+  }
+
+  bool* invoked_interceptor_;
+};
+
+class CertNetFetcherURLRequestTestWithSecureDnsInterceptor
+    : public CertNetFetcherURLRequestTest,
+      public WithTaskEnvironment {
+ public:
+  CertNetFetcherURLRequestTestWithSecureDnsInterceptor()
+      : invoked_interceptor_(false) {}
+
+  void SetUp() override {
+    URLRequestFilter::GetInstance()->AddHostnameInterceptor(
+        "http", kMockSecureDnsHostname,
+        std::make_unique<SecureDnsInterceptor>(&invoked_interceptor_));
+  }
+
+  void TearDown() override { URLRequestFilter::GetInstance()->ClearHandlers(); }
+
+  bool invoked_interceptor() { return invoked_interceptor_; }
+
+ private:
+  bool invoked_interceptor_;
+};
+
 // Helper to start an AIA fetch using default parameters.
 WARN_UNUSED_RESULT std::unique_ptr<CertNetFetcher::Request> StartRequest(
     CertNetFetcher* fetcher,
@@ -653,6 +697,18 @@ TEST_F(CertNetFetcherURLRequestTestWithHangingReadHandler,
   VerifyFailure(ERR_ABORTED, request.get());
 }
 
+TEST_F(CertNetFetcherURLRequestTestWithSecureDnsInterceptor,
+       SecureDnsDisabled) {
+  CreateFetcher();
+  std::unique_ptr<net::CertNetFetcher::Request> request = StartRequest(
+      fetcher(),
+      GURL("http://" + std::string(kMockSecureDnsHostname) + "/cert.crt"));
+  Error actual_error;
+  std::vector<uint8_t> actual_body;
+  request->WaitForResult(&actual_error, &actual_body);
+  EXPECT_TRUE(invoked_interceptor());
+}
+
 }  // namespace
 
 }  // namespace net

net/cert_net/nss_ocsp_session_url_request.cc

@@ -258,6 +258,9 @@ class OCSPRequestSessionDelegateURLRequest : public OCSPRequestSessionDelegate,
         params->url, DEFAULT_PRIORITY, this, traffic_annotation);
     request_->SetLoadFlags(LOAD_DISABLE_CACHE);
     request_->set_allow_credentials(false);
+    // Disable secure DNS for hostname lookups triggered by certificate network
+    // fetches to prevent deadlock.
+    request_->SetDisableSecureDns(true);
 
     if (!params->extra_request_headers.IsEmpty())
       request_->SetExtraRequestHeaders(params->extra_request_headers);

net/cert_net/nss_ocsp_unittest.cc

@@ -60,6 +60,7 @@ class AiaResponseHandler : public URLRequestInterceptor {
   URLRequestJob* MaybeInterceptRequest(
       URLRequest* request,
       NetworkDelegate* network_delegate) const override {
+    EXPECT_TRUE(request->disable_secure_dns());
     ++const_cast<AiaResponseHandler*>(this)->request_count_;
 
     return new URLRequestTestJob(request, network_delegate, headers_,

net/dns/dns_transaction.cc

@@ -399,15 +399,8 @@ class DnsHTTPAttempt : public DnsAttempt, public URLRequest::Delegate {
     request_->SetExtraRequestHeaders(extra_request_headers);
     // Disable secure DNS for any DoH server hostname lookups to avoid deadlock.
     request_->SetDisableSecureDns(true);
-    // Bypass proxy settings and certificate-related network fetches (currently
-    // just OCSP and CRL requests) to avoid deadlock. AIA requests and the
-    // Negotiate scheme for HTTP authentication may also cause deadlocks, but
-    // these deadlocks can be resolved from the DoH server side (e.g. the server
-    // can send a certificate chain that is complete from the client's
-    // perspective to prevent the client from sending AIA requests).
     request_->SetLoadFlags(request_->load_flags() | LOAD_DISABLE_CACHE |
-                           LOAD_BYPASS_PROXY |
-                           LOAD_DISABLE_CERT_NETWORK_FETCHES);
+                           LOAD_BYPASS_PROXY);
     request_->set_allow_credentials(false);
   }
 

net/url_request/url_request_unittest.cc

@@ -9589,6 +9589,22 @@ static const SHA256HashValue kOCSPTestCertSPKI = {{
 // generates.
 static const char kOCSPTestCertPolicy[] = "1.3.6.1.4.1.11129.2.4.1";
 
+// Interceptor to check that secure DNS has been disabled.
+class SecureDnsInterceptor : public net::URLRequestInterceptor {
+ public:
+  SecureDnsInterceptor() = default;
+  ~SecureDnsInterceptor() override = default;
+
+ private:
+  // URLRequestInterceptor implementation:
+  net::URLRequestJob* MaybeInterceptRequest(
+      net::URLRequest* request,
+      net::NetworkDelegate* network_delegate) const override {
+    EXPECT_TRUE(request->disable_secure_dns());
+    return nullptr;
+  }
+};
+
 class HTTPSOCSPTest : public HTTPSRequestTest {
  public:
   HTTPSOCSPTest()
@@ -9609,6 +9625,9 @@ class HTTPSOCSPTest : public HTTPSRequestTest {
     cert_net_fetcher_->SetURLRequestContext(&context_);
     context_.cert_verifier()->SetConfig(GetCertVerifierConfig());
 
+    net::URLRequestFilter::GetInstance()->AddHostnameInterceptor(
+        "http", "127.0.0.1", std::make_unique<SecureDnsInterceptor>());
+
     scoped_refptr<X509Certificate> root_cert =
         ImportCertFromFile(GetTestCertsDirectory(), "ocsp-test-root.pem");
     ASSERT_TRUE(root_cert);
@@ -9619,6 +9638,10 @@ class HTTPSOCSPTest : public HTTPSRequestTest {
 #endif
   }
 
+  void TearDown() override {
+    net::URLRequestFilter::GetInstance()->ClearHandlers();
+  }
+
   void DoConnectionWithDelegate(
       const SpawnedTestServer::SSLOptions& ssl_options,
       TestDelegate* delegate,

services/network/public/cpp/cert_verifier/cert_net_fetcher_test.cc

@@ -30,7 +30,9 @@ CertNetFetcherTestUtilRealLoader::~CertNetFetcherTestUtilRealLoader() = default;
 
 CertNetFetcherTestUtilRealLoader::CertNetFetcherTestUtilRealLoader()
     : test_shared_url_loader_factory_(
-          base::MakeRefCounted<network::TestSharedURLLoaderFactory>()),
+          base::MakeRefCounted<network::TestSharedURLLoaderFactory>(
+              nullptr /* network_service */,
+              true /* is_trusted */)),
       receiver_(test_shared_url_loader_factory_.get(),
                 std::move(pending_receiver_)) {}
 }  // namespace cert_verifier

services/network/public/cpp/cert_verifier/cert_net_fetcher_url_loader.cc

@@ -487,6 +487,10 @@ void Job::StartURLLoader(network::mojom::URLLoaderFactory* factory) {
   request->url = request_params_->url;
   if (request_params_->http_method == HTTP_METHOD_POST)
     request->method = net::HttpRequestHeaders::kPostMethod;
+  // Disable secure DNS for hostname lookups triggered by certificate network
+  // fetches to prevent deadlock.
+  request->trusted_params = network::ResourceRequest::TrustedParams();
+  request->trusted_params->disable_secure_dns = true;
   request->credentials_mode = network::mojom::CredentialsMode::kOmit;
   url_loader_ =
       network::SimpleURLLoader::Create(std::move(request), traffic_annotation);

services/network/public/cpp/cert_verifier/cert_net_fetcher_url_loader_unittest.cc

@@ -24,6 +24,8 @@
 #include "net/test/gtest_util.h"
 #include "net/test/test_with_task_environment.h"
 #include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
+#include "net/url_request/url_request_filter.h"
+#include "net/url_request/url_request_interceptor.h"
 #include "services/network/public/cpp/cert_verifier/cert_net_fetcher_test.h"
 #include "services/network/public/mojom/url_loader.mojom.h"
 #include "services/network/test/test_url_loader_factory.h"
@@ -44,6 +46,8 @@ const base::FilePath::CharType kDocRoot[] =
 
 const char kMockURL[] = "http://mock.hanging.read/";
 
+const char kMockSecureDnsHostname[] = "mock.secure.dns.check";
+
 // Wait for the request to complete, and verify that it completed successfully
 // with the indicated bytes.
 void VerifySuccess(const std::string& expected_body,
@@ -197,6 +201,49 @@ class CertNetFetcherURLLoaderTest : public PlatformTest {
   bool use_hanging_url_loader_ = false;
 };
 
+// Interceptor to check that secure DNS has been disabled.
+class SecureDnsInterceptor : public net::URLRequestInterceptor {
+ public:
+  explicit SecureDnsInterceptor(bool* invoked_interceptor)
+      : invoked_interceptor_(invoked_interceptor) {}
+  ~SecureDnsInterceptor() override = default;
+
+ private:
+  // URLRequestInterceptor implementation:
+  net::URLRequestJob* MaybeInterceptRequest(
+      net::URLRequest* request,
+      net::NetworkDelegate* network_delegate) const override {
+    EXPECT_TRUE(request->disable_secure_dns());
+    *invoked_interceptor_ = true;
+    return nullptr;
+  }
+
+  bool* invoked_interceptor_;
+};
+
+class CertNetFetcherURLLoaderTestWithSecureDnsInterceptor
+    : public CertNetFetcherURLLoaderTest,
+      public net::WithTaskEnvironment {
+ public:
+  CertNetFetcherURLLoaderTestWithSecureDnsInterceptor()
+      : invoked_interceptor_(false) {}
+
+  void SetUp() override {
+    net::URLRequestFilter::GetInstance()->AddHostnameInterceptor(
+        "http", kMockSecureDnsHostname,
+        std::make_unique<SecureDnsInterceptor>(&invoked_interceptor_));
+  }
+
+  void TearDown() override {
+    net::URLRequestFilter::GetInstance()->ClearHandlers();
+  }
+
+  bool invoked_interceptor() { return invoked_interceptor_; }
+
+ private:
+  bool invoked_interceptor_;
+};
+
 // Helper to start an AIA fetch using default parameters.
 WARN_UNUSED_RESULT std::unique_ptr<net::CertNetFetcher::Request> StartRequest(
     net::CertNetFetcher* fetcher,
@@ -600,6 +647,17 @@ TEST_F(CertNetFetcherURLLoaderTest, ShutdownCancelsRequests) {
   VerifyFailure(net::ERR_ABORTED, request.get());
 }
 
+TEST_F(CertNetFetcherURLLoaderTestWithSecureDnsInterceptor, SecureDnsDisabled) {
+  CreateFetcher();
+  std::unique_ptr<net::CertNetFetcher::Request> request = StartRequest(
+      fetcher(),
+      GURL("http://" + std::string(kMockSecureDnsHostname) + "/cert.crt"));
+  net::Error actual_error;
+  std::vector<uint8_t> actual_body;
+  request->WaitForResult(&actual_error, &actual_body);
+  EXPECT_TRUE(invoked_interceptor());
+}
+
 }  // namespace
 
 }  // namespace cert_verifier

services/network/public/cpp/cert_verifier/nss_ocsp_session_url_loader.cc

@@ -97,6 +97,10 @@ void OCSPRequestSessionDelegateURLLoader::StartLoad(
   request->url = params->url;
   request->credentials_mode = network::mojom::CredentialsMode::kOmit;
   request->load_flags = net::LOAD_DISABLE_CACHE;
+  // Disable secure DNS for hostname lookups triggered by certificate network
+  // fetches to prevent deadlock.
+  request->trusted_params = network::ResourceRequest::TrustedParams();
+  request->trusted_params->disable_secure_dns = true;
 
   if (!params->extra_request_headers.IsEmpty())
     request->headers = params->extra_request_headers;

services/network/public/cpp/cert_verifier/nss_ocsp_session_url_loader_unittest.cc

@@ -110,6 +110,8 @@ class OCSPRequestSessionDelegateURLLoaderTest : public ::testing::Test {
     old_interceptor_ = base::BindLambdaForTesting(
         [this](const network::ResourceRequest& request) {
           EXPECT_EQ(request.url, intercept_url_);
+          EXPECT_TRUE(request.trusted_params.has_value());
+          EXPECT_TRUE(request.trusted_params->disable_secure_dns);
           num_loaders_created_++;
         });
     loader_factory_->SetInterceptor(old_interceptor_);
@@ -469,6 +471,8 @@ class NssHttpURLLoaderTest : public net::TestWithTaskEnvironment {
     loader_factory_.SetInterceptor(base::BindLambdaForTesting(
         [this](const network::ResourceRequest& request) {
           EXPECT_EQ(request.url, intercept_url_);
+          EXPECT_TRUE(request.trusted_params.has_value());
+          EXPECT_TRUE(request.trusted_params->disable_secure_dns);
           num_loaders_created_++;
         }));