ServiceWorkerScriptURLLoader does not check for certificate errors properly |
||||||
Issue descriptionServiceWorkerScriptURLLoader::OnReceiveResponse checks for certificate errors by looking at response_head.cert_status. But that field is only populated when DevTools is open, per https://cs.chromium.org/chromium/src/content/public/common/resource_response_info.h?q=ResourceResponseInfo&sq=package:chromium&l=164. OnReceiveResponse should be looking at ssl_info.cert_status instead. We should also make the only-sometimes-populated fields of ResourceResponseInfo harder to use by accident. (Maybe make them base::Optional.) Service Worker people, can you please help triage? Does this mean we will load a Service Worker over a connection with a certificate error? Is this code enabled by default? The corresponding test is ServiceWorkerScriptURLLoaderTest.Error_CertificateError and it is broken. It sets a cert error on ssl_info.cert_status which is never looked at -- and then the test passes because the resource loads with an incorrect MIME type.
,
Nov 29 2017
Also I will probably fix this as part of 789720
,
Nov 30 2017
Yep it's only used by network service for now
,
Dec 1 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/03607654694d1a98b251f36c280bc4ff558d0e8b commit 03607654694d1a98b251f36c280bc4ff558d0e8b Author: Emily Stark <estark@google.com> Date: Fri Dec 01 04:14:03 2017 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} [modify] https://crrev.com/03607654694d1a98b251f36c280bc4ff558d0e8b/content/browser/frame_host/navigation_handle_impl_browsertest.cc [modify] https://crrev.com/03607654694d1a98b251f36c280bc4ff558d0e8b/content/browser/loader/resource_loader.cc [modify] https://crrev.com/03607654694d1a98b251f36c280bc4ff558d0e8b/content/browser/loader/resource_loader_unittest.cc [modify] https://crrev.com/03607654694d1a98b251f36c280bc4ff558d0e8b/content/browser/service_worker/service_worker_script_url_loader.cc [modify] https://crrev.com/03607654694d1a98b251f36c280bc4ff558d0e8b/content/browser/service_worker/service_worker_script_url_loader_unittest.cc [modify] https://crrev.com/03607654694d1a98b251f36c280bc4ff558d0e8b/content/browser/service_worker/service_worker_version.cc [modify] https://crrev.com/03607654694d1a98b251f36c280bc4ff558d0e8b/content/browser/service_worker/service_worker_write_to_cache_job.cc [modify] https://crrev.com/03607654694d1a98b251f36c280bc4ff558d0e8b/content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc [modify] https://crrev.com/03607654694d1a98b251f36c280bc4ff558d0e8b/content/browser/ssl/ssl_error_handler.cc [modify] https://crrev.com/03607654694d1a98b251f36c280bc4ff558d0e8b/content/browser/utility_process_host_impl.cc [modify] https://crrev.com/03607654694d1a98b251f36c280bc4ff558d0e8b/content/network/url_loader.cc [modify] https://crrev.com/03607654694d1a98b251f36c280bc4ff558d0e8b/content/network/url_loader_unittest.cc [modify] https://crrev.com/03607654694d1a98b251f36c280bc4ff558d0e8b/content/public/browser/certificate_request_result_type.h [modify] https://crrev.com/03607654694d1a98b251f36c280bc4ff558d0e8b/content/public/common/resource_response_info.h
,
Dec 1 2017
Think we can mark this fixed. Thanks!
,
Dec 1 2017
Thank you for fixing this!
,
Dec 1 2017
,
Dec 4 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/de7cb87f71cb13598225f75d3a96c49bc260b54a commit de7cb87f71cb13598225f75d3a96c49bc260b54a Author: Emily Stark <estark@google.com> Date: Mon Dec 04 19:02:33 2017 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-Original-Commit-Position: refs/heads/master@{#520850}(cherry picked from commit 03607654694d1a98b251f36c280bc4ff558d0e8b) Reviewed-on: https://chromium-review.googlesource.com/806315 Reviewed-by: Emily Stark <estark@chromium.org> Cr-Commit-Position: refs/branch-heads/3282@{#15} Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840} [modify] https://crrev.com/de7cb87f71cb13598225f75d3a96c49bc260b54a/content/browser/frame_host/navigation_handle_impl_browsertest.cc [modify] https://crrev.com/de7cb87f71cb13598225f75d3a96c49bc260b54a/content/browser/loader/resource_loader.cc [modify] https://crrev.com/de7cb87f71cb13598225f75d3a96c49bc260b54a/content/browser/loader/resource_loader_unittest.cc [modify] https://crrev.com/de7cb87f71cb13598225f75d3a96c49bc260b54a/content/browser/service_worker/service_worker_script_url_loader.cc [modify] https://crrev.com/de7cb87f71cb13598225f75d3a96c49bc260b54a/content/browser/service_worker/service_worker_script_url_loader_unittest.cc [modify] https://crrev.com/de7cb87f71cb13598225f75d3a96c49bc260b54a/content/browser/service_worker/service_worker_version.cc [modify] https://crrev.com/de7cb87f71cb13598225f75d3a96c49bc260b54a/content/browser/service_worker/service_worker_write_to_cache_job.cc [modify] https://crrev.com/de7cb87f71cb13598225f75d3a96c49bc260b54a/content/browser/service_worker/service_worker_write_to_cache_job_unittest.cc [modify] https://crrev.com/de7cb87f71cb13598225f75d3a96c49bc260b54a/content/browser/ssl/ssl_error_handler.cc [modify] https://crrev.com/de7cb87f71cb13598225f75d3a96c49bc260b54a/content/browser/utility_process_host_impl.cc [modify] https://crrev.com/de7cb87f71cb13598225f75d3a96c49bc260b54a/content/network/url_loader.cc [modify] https://crrev.com/de7cb87f71cb13598225f75d3a96c49bc260b54a/content/network/url_loader_unittest.cc [modify] https://crrev.com/de7cb87f71cb13598225f75d3a96c49bc260b54a/content/public/browser/certificate_request_result_type.h [modify] https://crrev.com/de7cb87f71cb13598225f75d3a96c49bc260b54a/content/public/common/resource_response_info.h
,
Mar 9 2018
This bug has been closed for more than 14 weeks. Removing security view restrictions. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot |
||||||
►
Sign in to add a comment |
||||||
Comment 1 by est...@chromium.org
, Nov 29 2017Labels: Proj-Servicification Security_Impact-None