lukasza: Please set the milestone you want to fix it by (M65?) and whatever severity you think it rates (Medium? Low?). Thanks!

This bug allows attackers to read limited amounts of information, so it seems to be a "medium" severity.

Let me tentatively aim to fix this in M65 - please shout if you think if we should target another version.

Re #2, Severity: Given the precondition of a compromised Renderer, I'm not sure this really ranks at Medium?

RE: #3, I can see how this could be ranked as Low (on the other hand, given Spectre, the compromised renderer might not really be a necessary precondition).

And obviously I wanted safelisted *response* headers (not "request" headers) as I wrote when opening the bug.  Based on https://fetch.spec.whatwg.org/#cors-safelisted-response-header-name, only the following headers would be allowed in requests blocked by XSDB:
- `Cache-Control`
- `Content-Language`
- `Content-Type`
- `Expires`
- `Last-Modified`
- `Pragma`
- headers listed in `Access-Control-Expose-Headers` header (except `Set-Cookie` and `Set-Cookie2`)

Interestingly, the last bullet only applies to CORS-allowed responses which today are ignored by XSDB: CrossSiteDocumentResourceHandler::ShouldBlockBasedOnHeaders:
  // Allow the response through if it has valid CORS headers.
  std::string cors_header;
  response->head.headers->GetNormalizedHeader("access-control-allow-origin",
                                              &cors_header);
  if (CrossSiteDocumentClassifier::IsValidCorsHeaderSet(initiator,
                                                        cors_header)) {
    return false;
  }
Not sure if this is important enough to need a separate bug...

+dcheng@ and kinuko@ for their thoughts on how to check presence of --disable-web-security availability in service worker's ExecutionContext

+dcheng@, jam@ and mmenke@ for their thoughts on the idea of having the test intercept and inspect http headers present network::mojom::URLLoaderClient::OnReceiveResponse IPC (and whether this would be possible / make sense in the post network servicification world)


I am a little bit stuck trying to create a test for this, because while the headers reach the renderer process, they are filtered out by the FetchManager::Loader::DidReceiveResponse -> FetchResponseData::CreateCORSFilteredResponse call, before the headers are exposed to javascript.

FWIW, the XSDB browser tests run with --disable-web-security, so I thought that maybe the call to CreateCORSFilteredResponse should be avoided base on blink::Settings::GetWebSecurityEnabled.  But... I have a hard time figuring out how to grab a blink::Settings object from within FetchManager::Loader - AFAICT the blink::Settings object can be retrieved from a Frame/Document, but not from a generic ExecutionContext (e.g. which doesn't necessarily have an associated document in case of a service worker).  dcheng@ points out that some things in blink::Settings are page-specific and wouldn't have made sense for a service worker.  Hmmm... :-(

Also - I've been trying to make the browser test go through the |fetch(...).then(...)| API, but AFAIK the XMLHttpRequest API also wouldn't expose all the headers (according to my reading of XMLHttpRequest::getResponseHeader).


Stepping back a little bit - maybe I should design the test differently?  For example - maybe the tests should intercept and inspect IPC/mojo messages carrying the headers, rather then relying on --disable-web-security.  OTOH, I am not sure how this would work in the network service work (maybe intercepting of the mojo messages should happen in the renderer process rather than in the browser process?  but then - how would a browser test drive this?).

Not sure what XSDB is (Cross-site Database seems the most obvious, but seems unlikely, since no database seems to be involved).

It looks like there's currently no way to grab a Clone of the URLLoaderFactory from the RenderFrameHostImpl to us, but that's the approach I'd suggest.  I'd modify RenderFrameHostImpl::CreateNetworkServiceDefaultFactoryAndObserve() to unconditionally populate network_service_connection_error_handler_holder_ with a clone of the URLLoaderFactory passed to the renderer (Instead of creating a new factory), and add a ForTesting() accessor for it.  Then you can use that, and it should give us exactly what we'd send to the renderer, regardless of future changes.

John may have a better idea.

+rockot@ [as suggested by dcheng@]

Thanks Matt!  Cloning and exposing URLLoaderFactory to browser tests lets me author tests and seems like a good strategy overall - I have a WIP CL using this approach at https://crrev.com/c/907323.

OTOH, AFAICT RenderFrameHostImpl::CreateNetworkServiceDefaultFactoryAndObserve is only called in presence of --enable-features=NetworkService cmdline flag.  I have a bit of trouble figuring out how to get a URLLoaderFactory in the old/default world.  Any hints?

PS. My apologies for not defining/explaining what XSDB is.  It stands for Cross-Site Document blocking.  A draft of an "explainer" is at https://crrev.com/c/891580.

PPS. XSDB will soon be renamed to XORB (Cross-Origin Response Blocking), because 1) it blocks cross-origin, not just cross-site responses and 2) document(blocked)-vs-resource(allowed) narrative was confusing.

What exactly are you trying to test, from the networking pov?

RE: #12

I am trying to verify that only an empty body + a limited set of headers are sent to the renderer process (to avoid exposing cross-origin responses to the threat of Spectre).

Thanks.

You can do this today without production code changes. Look at content::URLLoaderInterceptor. Once the test page makes the request, your browsertest's URLLoaderInterceptor (make sure to pass in boolean for the 2nd constructor parameter) will be called. You can return false from your callback, which will pass the request to the real URLLoaderFactory. However before you return, you can swap in a different URLLoaderClientPtr in the RequestParams struct.

(plz ping me over chat if you run into any issues, happy to unblock)

btw I just saw that Doug had documented this method of hooking, see https://docs.google.com/document/d/1OyBYvN0dwvpqfSZBdsfZ29iTFqGnVS2sdiPV14Z-Fto/edit#heading=h.ldq9r3n1cu83

Thanks jam@ and mmenke@ - I now have a working test and I am trying (with nick@'s help) to add header blocking to CrossSiteDocumentResourceHandler (slightly tricky because I need to change/postpone the order in which we call into the next_handler_).

Just as an FYI, CrossSiteDocumentResourceHandler is going to have to go away when we ship the network service, to be replaced with something else.  Sounds like you're already making a full integration test, which should make sure we don't ship the network service until this is working in that path, too, just thought I'd mention it.

@lukasza since this is a new feature, can you describe what inputs you need to do this blocking? And does this apply for both frame and subresource requests?

It would be great if this can land using alternative approaches, i.e. content::URLLoaderThrottle, as those work with the network service today and will avoid us having to rewrite this in the next quarter as Matt mentions. Brief intro at https://docs.google.com/document/d/1OyBYvN0dwvpqfSZBdsfZ29iTFqGnVS2sdiPV14Z-Fto/edit#heading=h.1qofpgue7ip1 but happy to chat more

XSDB is a fresh, but not a new/under-development feature - nick@ and creis@ implemented it in Dec 2018 - r522016.  If I can block the headers within the existing design, then I'll be happy with such an incremental change.  OTOH, if a redesign will make blocking the headers easier, then I am all ears :-)

XSDB only applies to subresource requests.

XSDB bases its block-or-allow decision on
1) the headers (Content-Type, X-Content-Type-Options: nosniff) and
2) sniffing the beginning of the response body (if the Content-Type header says text/html but the body doesn't sniff as html then no blocking will occur).

I only took a brief look at the URLLoaderThrottle, but it doesn't seem to fit - AFAICT it doesn't let the throttle look-at/sniff the response body.

Re 2, I thought we sniff all subresource requests. So by the time the renderer sees a subresource request, we would know the sniffed mime type and the headers from the response. Would you still need to sniff again?

CL under review @ https://crrev.com/c/907323

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7aa64c8089a805b1435a2ee9f768d8dc47d97b6b

commit 7aa64c8089a805b1435a2ee9f768d8dc47d97b6b
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Wed Feb 21 18:48:58 2018

Fix mime sniffer, so OnReadCompleted(EOF) is replayed if needed.

This CL adds a unit tests that results in the following calls into
MimeSniffingResourceHandler:
- ...
- OnWillRead()
- OnReadCompleted(10)
- OnWillRead()
- OnReadCompleted(0)
- OnResponseCompleted(...)

Before this CL, the downstream handler would get the following calls:
- ...
- OnWillRead()
- ...
- OnReadCompleted(10)
- OnResponseCompleted(...)

After this CL, the downstream handler will get the following calls:
- ...
- OnWillRead()
- ...
- OnReadCompleted(10)
- OnWillRead()       <- new call - added/fixed by this CL
- OnReadCompleted(0) <- new call - added/fixed by this CL
- OnResponseCompleted(...)

The lack of the 2 calls highlighted above is/was preventing unit testing
of CrossSiteDocumentResourceHandler together with
MimeSniffingResourceHandler.

Bug:  808205 
Change-Id: I840fcc702917267b874e660f88d6291655724e9d
Reviewed-on: https://chromium-review.googlesource.com/927111
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538166}
[modify] https://crrev.com/7aa64c8089a805b1435a2ee9f768d8dc47d97b6b/content/browser/loader/mime_sniffing_resource_handler.cc
[modify] https://crrev.com/7aa64c8089a805b1435a2ee9f768d8dc47d97b6b/content/browser/loader/mime_sniffing_resource_handler.h
[modify] https://crrev.com/7aa64c8089a805b1435a2ee9f768d8dc47d97b6b/content/browser/loader/mime_sniffing_resource_handler_unittest.cc
[modify] https://crrev.com/7aa64c8089a805b1435a2ee9f768d8dc47d97b6b/content/browser/loader/test_resource_handler.cc

A status update:
- the fix will hopefully land later this week
- we probably won't try to merge the fix to M65, because the fix ended up being quite complex

Status update:
- Almost ready to land - just need to fix one recently added layout test that is incompatible with header blocking.  Hopefully the CL can land later today.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0753a97df25d497e872e2945848907ca9a6661bf

commit 0753a97df25d497e872e2945848907ca9a6661bf
Author: Lukasz Anforowicz <lukasza@chromium.org>
Date: Thu Mar 01 17:29:18 2018

XSDB: Block most response headers in addition to the response body.

After this CL, CrossSiteDocumentResourceHandler won't call
|next_handler_|'s OnResponseStarted until it has made the allow-vs-block
decision.  Until that time network::ResourceResponse (which among other
things include the response headers) is stored temporarily in a
CrossSiteDocumentResourceHandler's field.

Postponing the call to next handler's OnResponseStarted confuses
MimeSniffingResourceHandler (which does the same thing to its downstream
handlers, but doesn't expect this treatment from its upstream handler).
The CL works around that by changing the relative order of the handlers
(after CL, MimeSniffingResourceHandler is before
CrossSiteDocumentResourceHandler).  This change requires that
the first CrossSiteDocumentResourceHandler::OnWillRead always allocates
a |local_buffer_|, because MimeSniffingResourceHandler will make the
first call to OnWillRead before letting CrossSiteDocumentResourceHandler
determine |should_block_based_on_headers_| by calling OnResponseStarted.

This CL adds CrossSiteDocumentResourceHandler unit tests that also cover
MimeSniffingResourceHandler in the stream of resource handlers.  The new
tests help ensure that CrossSiteDocumentResourceHandler interoperates
well with MimeSniffingResourceHandler (wrt call order and also the
sniffed mime type).  OTOH, the old tests are retained, because driving
CrossSiteDocumentResourceHandler more directly allows the tests to make
more assumptions about the internal state and make more test assertions
(e.g. assert that downstream buffer differs until the block-or-allow
decision is made).

Bug:  808205 
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_mojo
Change-Id: I304df0b40c71dd212a469b6b3cc4c13bed533fb0
Reviewed-on: https://chromium-review.googlesource.com/907323
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Reviewed-by: Nick Carter <nick@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#540199}
[modify] https://crrev.com/0753a97df25d497e872e2945848907ca9a6661bf/chrome/browser/net/errorpage_browsertest.cc
[modify] https://crrev.com/0753a97df25d497e872e2945848907ca9a6661bf/content/browser/loader/cross_origin_read_blocking_explainer.md
[modify] https://crrev.com/0753a97df25d497e872e2945848907ca9a6661bf/content/browser/loader/cross_site_document_blocking_browsertest.cc
[modify] https://crrev.com/0753a97df25d497e872e2945848907ca9a6661bf/content/browser/loader/cross_site_document_resource_handler.cc
[modify] https://crrev.com/0753a97df25d497e872e2945848907ca9a6661bf/content/browser/loader/cross_site_document_resource_handler.h
[modify] https://crrev.com/0753a97df25d497e872e2945848907ca9a6661bf/content/browser/loader/cross_site_document_resource_handler_unittest.cc
[modify] https://crrev.com/0753a97df25d497e872e2945848907ca9a6661bf/content/browser/loader/resource_dispatcher_host_impl.cc
[modify] https://crrev.com/0753a97df25d497e872e2945848907ca9a6661bf/content/public/test/url_loader_interceptor.h
[add] https://crrev.com/0753a97df25d497e872e2945848907ca9a6661bf/content/test/data/cross_site_document_blocking/headers-test.json
[add] https://crrev.com/0753a97df25d497e872e2945848907ca9a6661bf/content/test/data/cross_site_document_blocking/headers-test.json.mock-http-headers
[modify] https://crrev.com/0753a97df25d497e872e2945848907ca9a6661bf/content/test/data/cross_site_document_blocking/service_worker.js
[modify] https://crrev.com/0753a97df25d497e872e2945848907ca9a6661bf/net/test/url_request/url_request_mock_data_job.cc
[modify] https://crrev.com/0753a97df25d497e872e2945848907ca9a6661bf/net/test/url_request/url_request_mock_data_job.h
[modify] https://crrev.com/0753a97df25d497e872e2945848907ca9a6661bf/testing/buildbot/filters/mojo.fyi.network_content_browsertests.filter
[modify] https://crrev.com/0753a97df25d497e872e2945848907ca9a6661bf/third_party/WebKit/LayoutTests/flag-specific/site-per-process/http/tests/inspector-protocol/network/multiple-headers-expected.txt
[modify] https://crrev.com/0753a97df25d497e872e2945848907ca9a6661bf/third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/multiple-headers-expected.txt
[modify] https://crrev.com/0753a97df25d497e872e2945848907ca9a6661bf/third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/multiple-headers.js
[modify] https://crrev.com/0753a97df25d497e872e2945848907ca9a6661bf/third_party/WebKit/LayoutTests/http/tests/inspector-protocol/network/resources/multiple-headers.php

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