Preserve cert error code instead of transforming to ERR_INSECURE_RESPONSE

When denying a request with a certificate error, we've historically cancelled
it with a net error code of net::ERR_INSECURE_RESPONSE in
content/browser/ssl/ssl_error_handler.cc, instead of the actual error code that
was encountered (e.g. net::ERR_CERT_DATE_INVALID). This is a little awkward for
committed interstitials [1] because we won't be able to determine from the error
code along whether an error page is for a certificate error, and there are places
where we have only the error code available, not supplemental info like the SSLInfo
or CertStatus. (See for example NetErrorHelperCore::ErrorPageInfo.) This comes up
in other contexts as well where we only have the net error code available (see bug
for details).

Along the way, this change fixes a bug in ServiceWorkerScriptURLLoader. This
loader was using response_head.cert_status to detect certificate errors, but
that field was not being populated by URLLoader (and was only populated by
ResourceLoader when devtools was open). So this CL populates cert_status
unconditionally to avoid this problem. The corresponding test was buggily
passing: it was expecting ERR_INSECURE_RESPONSE due to a cert error, but was
actually producing ERR_INSECURE_RESPONSE due to an unrelated error (unexpected
mime type). Converting certificate errors to use the more specific certificate
error codes instead of ERR_INSECURE_RESPONSE should prevent this test from
regressing again. Finally, fixing the Service Worker certificate checking
revealed a number of other SW tests which were expecting certificate errors to
be ignored via --ignore-certificate-errors, which broke once the SW bug had been
fixed. This CL adds --ignore-certificate-errors support to the network service,
so that those tests pass again.

[1] https://docs.google.com/document/d/1rEBpw5V-Nn1UIi8CIFa5ZZvwlR08SkY3CogvWE2UMFs/edit

Bug: 789720,789682
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I963b172dcfa400ef256e89c7c94d3f91b8bc5697
Reviewed-on: https://chromium-review.googlesource.com/797536
Commit-Queue: Emily Stark <estark@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#520850}

content/browser/frame_host/navigation_handle_impl_browsertest.cc

@@ -531,7 +531,7 @@ IN_PROC_BROWSER_TEST_F(NavigationHandleImplBrowserTest, VerifyRedirect) {
 }
 
 // Ensure that a certificate error results in a committed navigation with
-// the error code net::ERR_INSECURE_RESPONSE on the handle.
+// the appropriate error code on the handle.
 IN_PROC_BROWSER_TEST_F(NavigationHandleImplBrowserTest,
                        VerifyCertErrorFailure) {
   if (!IsBrowserSideNavigationEnabled()) {
@@ -549,7 +549,7 @@ IN_PROC_BROWSER_TEST_F(NavigationHandleImplBrowserTest,
 
   EXPECT_TRUE(observer.has_committed());
   EXPECT_TRUE(observer.is_error());
-  EXPECT_EQ(net::ERR_INSECURE_RESPONSE, observer.net_error_code());
+  EXPECT_EQ(net::ERR_CERT_COMMON_NAME_INVALID, observer.net_error_code());
 }
 
 // Ensure that the IsRendererInitiated() method on NavigationHandle behaves
@@ -959,7 +959,7 @@ IN_PROC_BROWSER_TEST_F(NavigationHandleImplBrowserTest, ThrottleDeferFailure) {
 
   EXPECT_TRUE(observer.has_committed());
   EXPECT_TRUE(observer.is_error());
-  EXPECT_EQ(net::ERR_INSECURE_RESPONSE, observer.net_error_code());
+  EXPECT_EQ(net::ERR_CERT_COMMON_NAME_INVALID, observer.net_error_code());
 }
 
 // Ensure that a NavigationThrottle can block the navigation and collapse the

content/browser/loader/resource_loader.cc

@@ -109,11 +109,11 @@ void PopulateResourceResponse(
         net::IsLegacySymantecCert(request->ssl_info().public_key_hashes);
     response->head.cert_validity_start =
         request->ssl_info().cert->valid_start();
+    response->head.cert_status = request->ssl_info().cert_status;
     if (info->ShouldReportRawHeaders()) {
       // Only pass these members when the network panel of the DevTools is open,
       // i.e. ShouldReportRawHeaders() is set. These data are used to populate
       // the requests in the security panel too.
-      response->head.cert_status = request->ssl_info().cert_status;
       response->head.ssl_connection_status =
           request->ssl_info().connection_status;
       response->head.ssl_key_exchange_group =

content/browser/loader/resource_loader_unittest.cc

@@ -596,6 +596,15 @@ class HTTPSSecurityInfoResourceLoaderTest : public ResourceLoaderTest {
   const GURL test_https_redirect_url_;
 };
 
+TEST_F(HTTPSSecurityInfoResourceLoaderTest, CertStatusOnResponse) {
+  SetUpResourceLoaderForUrl(test_https_url());
+  loader_->StartRequest();
+  raw_ptr_resource_handler_->WaitUntilResponseComplete();
+  base::RunLoop().RunUntilIdle();
+  EXPECT_EQ(kTestCertError,
+            raw_ptr_resource_handler_->resource_response()->head.cert_status);
+}
+
 // Tests that client certificates are requested with ClientCertStore lookup.
 TEST_F(ClientCertResourceLoaderTest, WithStoreLookup) {
   // Set up the test client cert store.

content/browser/service_worker/service_worker_script_url_loader.cc

@@ -6,6 +6,7 @@
 
 #include <memory>
 #include "base/numerics/safe_conversions.h"
+#include "components/network_session_configurator/common/network_switches.h"
 #include "content/browser/appcache/appcache_response.h"
 #include "content/browser/service_worker/service_worker_cache_writer.h"
 #include "content/browser/service_worker/service_worker_context_core.h"
@@ -158,14 +159,13 @@ void ServiceWorkerScriptURLLoader::OnReceiveResponse(
   }
 
   // Check the certificate error.
-  // TODO(nhiroki): Ignore the certificate error when the
-  // --ignore-certificate-errors flag etc are specified.
-  // See ShouldIgnoreSSLError() in service_worker_write_to_cache_job.cc.
-  if (net::IsCertStatusError(response_head.cert_status)) {
+  if (net::IsCertStatusError(response_head.cert_status) &&
+      !base::CommandLine::ForCurrentProcess()->HasSwitch(
+          switches::kIgnoreCertificateErrors)) {
     // TODO(nhiroki): Show an error message equivalent to kSSLError in
     // service_worker_write_to_cache_job.cc.
-    CommitCompleted(
-        network::URLLoaderCompletionStatus(net::ERR_INSECURE_RESPONSE));
+    CommitCompleted(network::URLLoaderCompletionStatus(
+        net::MapCertStatusToNetError(response_head.cert_status)));
     return;
   }
 

content/browser/service_worker/service_worker_script_url_loader_unittest.cc

@@ -90,17 +90,16 @@ class MockNetworkURLLoaderFactory final : public mojom::URLLoaderFactory {
     ResourceResponseHead response_head;
     response_head.headers = info.headers;
     response_head.headers->GetMimeType(&response_head.mime_type);
-    base::Optional<net::SSLInfo> ssl_info;
     if (response.has_certificate_error) {
-      ssl_info.emplace();
-      ssl_info->cert_status = net::CERT_STATUS_DATE_INVALID;
+      response_head.cert_status = net::CERT_STATUS_DATE_INVALID;
     }
 
     if (response_head.headers->response_code() == 307) {
       client->OnReceiveRedirect(net::RedirectInfo(), response_head);
       return;
     }
-    client->OnReceiveResponse(response_head, ssl_info, nullptr);
+    client->OnReceiveResponse(response_head, base::nullopt /* ssl_info */,
+                              nullptr /* downloaded_file */);
 
     // Pass the response body to the client.
     uint32_t bytes_written = response.body.size();
@@ -383,7 +382,7 @@ TEST_F(ServiceWorkerScriptURLLoaderTest, Error_CertificateError) {
 
   // The request should be failed because of the response with the certificate
   // error.
-  EXPECT_EQ(net::ERR_INSECURE_RESPONSE, client_.completion_status().error_code);
+  EXPECT_EQ(net::ERR_CERT_DATE_INVALID, client_.completion_status().error_code);
   EXPECT_FALSE(client_.has_received_response());
 
   // The response shouldn't be stored in the storage.

content/browser/service_worker/service_worker_version.cc

@@ -1824,6 +1824,8 @@ ServiceWorkerStatusCode ServiceWorkerVersion::DeduceStartWorkerFailureReason(
   const net::URLRequestStatus& main_script_status =
       script_cache_map()->main_script_status();
   if (main_script_status.status() != net::URLRequestStatus::SUCCESS) {
+    if (net::IsCertificateError(main_script_status.error()))
+      return SERVICE_WORKER_ERROR_SECURITY;
     switch (main_script_status.error()) {
       case net::ERR_INSECURE_RESPONSE:
       case net::ERR_UNSAFE_REDIRECT:

content/browser/service_worker/service_worker_write_to_cache_job.cc

@@ -303,10 +303,13 @@ void ServiceWorkerWriteToCacheJob::OnSSLCertificateError(
   DCHECK_EQ(net_request_.get(), request);
   TRACE_EVENT0("ServiceWorker",
                "ServiceWorkerWriteToCacheJob::OnSSLCertificateError");
-  if (ShouldIgnoreSSLError(request))
+  if (ShouldIgnoreSSLError(request)) {
     request->ContinueDespiteLastError();
-  else
-    NotifyStartErrorHelper(net::ERR_INSECURE_RESPONSE, kSSLError);
+  } else {
+    NotifyStartErrorHelper(
+        net::Error(net::MapCertStatusToNetError(ssl_info.cert_status)),
+        kSSLError);
+  }
 }
 
 void ServiceWorkerWriteToCacheJob::OnResponseStarted(net::URLRequest* request,
@@ -331,7 +334,9 @@ void ServiceWorkerWriteToCacheJob::OnResponseStarted(net::URLRequest* request,
   // So we check cert_status here.
   if (net::IsCertStatusError(request->ssl_info().cert_status) &&
       !ShouldIgnoreSSLError(request)) {
-    NotifyStartErrorHelper(net::ERR_INSECURE_RESPONSE, kSSLError);
+    NotifyStartErrorHelper(net::Error(net::MapCertStatusToNetError(
+                               request->ssl_info().cert_status)),
+                           kSSLError);
     return;
   }
 

content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc

@@ -473,7 +473,7 @@ TEST_F(ServiceWorkerWriteToCacheJobTest, SSLCertificateError) {
   request_->Start();
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(net::URLRequestStatus::FAILED, request_->status().status());
-  EXPECT_EQ(net::ERR_INSECURE_RESPONSE, request_->status().error());
+  EXPECT_EQ(net::ERR_CERT_DATE_INVALID, request_->status().error());
   EXPECT_EQ(kInvalidServiceWorkerResourceId,
             version_->script_cache_map()->LookupResourceId(script_url_));
 }
@@ -508,7 +508,7 @@ TEST_F(ServiceWorkerWriteToCacheLocalhostTest, SSLCertificateError) {
   request_->Start();
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(net::URLRequestStatus::FAILED, request_->status().status());
-  EXPECT_EQ(net::ERR_INSECURE_RESPONSE, request_->status().error());
+  EXPECT_EQ(net::ERR_CERT_DATE_INVALID, request_->status().error());
   EXPECT_EQ(kInvalidServiceWorkerResourceId,
             version_->script_cache_map()->LookupResourceId(script_url_));
 }
@@ -534,7 +534,7 @@ TEST_F(ServiceWorkerWriteToCacheLocalhostTest, CertStatusError) {
   request_->Start();
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(net::URLRequestStatus::FAILED, request_->status().status());
-  EXPECT_EQ(net::ERR_INSECURE_RESPONSE, request_->status().error());
+  EXPECT_EQ(net::ERR_CERT_DATE_INVALID, request_->status().error());
   EXPECT_EQ(kInvalidServiceWorkerResourceId,
             version_->script_cache_map()->LookupResourceId(script_url_));
 }
@@ -545,7 +545,7 @@ TEST_F(ServiceWorkerWriteToCacheJobTest, CertStatusError) {
   request_->Start();
   base::RunLoop().RunUntilIdle();
   EXPECT_EQ(net::URLRequestStatus::FAILED, request_->status().status());
-  EXPECT_EQ(net::ERR_INSECURE_RESPONSE, request_->status().error());
+  EXPECT_EQ(net::ERR_CERT_DATE_INVALID, request_->status().error());
   EXPECT_EQ(kInvalidServiceWorkerResourceId,
             version_->script_cache_map()->LookupResourceId(script_url_));
 }

content/browser/ssl/ssl_error_handler.cc

@@ -77,13 +77,12 @@ void SSLErrorHandler::DenyRequest() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (delegate_thread_ == BrowserThread::UI) {
     if (delegate_)
-      delegate_->CancelSSLRequest(net::ERR_INSECURE_RESPONSE, &ssl_info());
+      delegate_->CancelSSLRequest(cert_error_, &ssl_info());
     return;
   }
-  BrowserThread::PostTask(
-      BrowserThread::IO, FROM_HERE,
-      base::BindOnce(&CompleteCancelRequest, delegate_, ssl_info(),
-                     net::ERR_INSECURE_RESPONSE));
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
+                          base::BindOnce(&CompleteCancelRequest, delegate_,
+                                         ssl_info(), cert_error_));
 }
 
 void SSLErrorHandler::ContinueRequest() {

content/browser/utility_process_host_impl.cc

@@ -289,6 +289,7 @@ bool UtilityProcessHostImpl::StartProcess() {
     // Browser command-line switches to propagate to the utility process.
     static const char* const kSwitchNames[] = {
       switches::kHostResolverRules,
+      switches::kIgnoreCertificateErrors,
       switches::kIgnoreCertificateErrorsSPKIList,
       switches::kLogNetLog,
       switches::kNoSandbox,

content/network/url_loader.cc

@@ -68,6 +68,7 @@ void PopulateResourceResponse(net::URLRequest* request,
         net::IsLegacySymantecCert(request->ssl_info().public_key_hashes);
     response->head.cert_validity_start =
         request->ssl_info().cert->valid_start();
+    response->head.cert_status = request->ssl_info().cert_status;
   }
 
   response->head.request_start = request->creation_time();

content/network/url_loader_unittest.cc

@@ -35,8 +35,10 @@
 #include "net/base/load_flags.h"
 #include "net/base/mime_sniffer.h"
 #include "net/base/net_errors.h"
+#include "net/test/cert_test_util.h"
 #include "net/test/embedded_test_server/embedded_test_server.h"
 #include "net/test/embedded_test_server/http_response.h"
+#include "net/test/test_data_directory.h"
 #include "net/test/url_request/url_request_failed_job.h"
 #include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
 #include "net/url_request/url_request.h"
@@ -44,6 +46,7 @@
 #include "net/url_request/url_request_interceptor.h"
 #include "net/url_request/url_request_job.h"
 #include "net/url_request/url_request_status.h"
+#include "net/url_request/url_request_test_job.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "url/gurl.h"
 
@@ -1207,4 +1210,61 @@ TEST_F(URLLoaderTest, SSLInfoOnComplete) {
             client()->completion_status().ssl_info.value().cert_status);
 }
 
+// A mock URLRequestJob which simulates an HTTPS request with a certificate
+// error.
+class MockHTTPSURLRequestJob : public net::URLRequestTestJob {
+ public:
+  MockHTTPSURLRequestJob(net::URLRequest* request,
+                         net::NetworkDelegate* network_delegate,
+                         const std::string& response_headers,
+                         const std::string& response_data,
+                         bool auto_advance)
+      : net::URLRequestTestJob(request,
+                               network_delegate,
+                               response_headers,
+                               response_data,
+                               auto_advance) {}
+
+  // net::URLRequestTestJob:
+  void GetResponseInfo(net::HttpResponseInfo* info) override {
+    // Get the original response info, but override the SSL info.
+    net::URLRequestJob::GetResponseInfo(info);
+    info->ssl_info.cert =
+        net::ImportCertFromFile(net::GetTestCertsDirectory(), "ok_cert.pem");
+    info->ssl_info.cert_status = net::CERT_STATUS_DATE_INVALID;
+  }
+
+ private:
+  ~MockHTTPSURLRequestJob() override {}
+
+  DISALLOW_COPY_AND_ASSIGN(MockHTTPSURLRequestJob);
+};
+
+class MockHTTPSJobURLRequestInterceptor : public net::URLRequestInterceptor {
+ public:
+  explicit MockHTTPSJobURLRequestInterceptor() {}
+  ~MockHTTPSJobURLRequestInterceptor() override {}
+
+  // net::URLRequestInterceptor:
+  net::URLRequestJob* MaybeInterceptRequest(
+      net::URLRequest* request,
+      net::NetworkDelegate* network_delegate) const override {
+    return new MockHTTPSURLRequestJob(request, network_delegate, std::string(),
+                                      "dummy response", true);
+  }
+};
+
+// Tests that |cert_status| is set on the resource response.
+TEST_F(URLLoaderTest, CertStatusOnResponse) {
+  net::URLRequestFilter::GetInstance()->ClearHandlers();
+  net::URLRequestFilter::GetInstance()->AddHostnameInterceptor(
+      "https", "example.test",
+      std::unique_ptr<net::URLRequestInterceptor>(
+          new MockHTTPSJobURLRequestInterceptor()));
+
+  EXPECT_EQ(net::OK, Load(GURL("https://example.test/")));
+  EXPECT_EQ(net::CERT_STATUS_DATE_INVALID,
+            client()->response_head().cert_status);
+}
+
 }  // namespace content

content/public/browser/certificate_request_result_type.h

@@ -16,7 +16,8 @@ enum CertificateRequestResultType {
   // Cancels the request synchronously using a net::ERR_ABORTED.
   CERTIFICATE_REQUEST_RESULT_TYPE_CANCEL,
 
-  // Denies the request synchronously using a net::ERR_INSECURE_RESPONSE.
+  // Denies the request synchronously using the certificate error code that was
+  // encountered.
   CERTIFICATE_REQUEST_RESULT_TYPE_DENY,
 };
 

content/public/common/resource_response_info.h

@@ -159,8 +159,7 @@ struct CONTENT_EXPORT ResourceResponseInfo {
   std::vector<std::string> certificate;
 
   // Bitmask of status info of the SSL certificate. See cert_status_flags.h for
-  // values. Only present if the renderer process set report_raw_headers to
-  // true.
+  // values.
   net::CertStatus cert_status;
 
   // Information about the SSL connection itself. See