Relax cross-origin partial response requirements for CORS presence.

Relax cross-origin partial response requirements for CORS presence.

Per discussion on the bug, if the redirect passes a CORS we should allow
the mixing of origins.  DidPassCORSAccessCheck() will ensure each request
passes the crossorigin test.

Prior to this fix, crossOrigin redirects for video were always broken, this fix
also allows 'range' to be a simple header when a client has requested no preflight.

BUG=532569
TEST=new unittest, manually verified exploit fails if crossorigin set.

Committed: https://crrev.com/e11ea5ed677321f5fa24e8e77b01f8f57a0098a5
Cr-Commit-Position: refs/heads/master@{#355452}

[DaleCurtis, 2015-09-22 21:57:49]

dalecurtis@chromium.org changed reviewers:
+ evn@google.com, horo@chromium.org

[DaleCurtis, 2015-09-22 21:57:50]

+horo, evn to vet this.

[evn, 2015-09-23 10:11:54]

https://codereview.chromium.org/1356353003/diff/1/media/blink/buffered_data_s...
File media/blink/buffered_data_source.cc (right):

https://codereview.chromium.org/1356353003/diff/1/media/blink/buffered_data_s...
media/blink/buffered_data_source.cc:437: DidPassCORSAccessCheck();
where can I see the code for DidPassCORSAccessCheck()? it seems like it should
be checking for mode==cors somewhere, but I can't find it

[DaleCurtis, 2015-09-23 15:56:12]

https://codereview.chromium.org/1356353003/diff/1/media/blink/buffered_data_s...
File media/blink/buffered_data_source.cc (right):

https://codereview.chromium.org/1356353003/diff/1/media/blink/buffered_data_s...
media/blink/buffered_data_source.cc:437: DidPassCORSAccessCheck();
On 2015/09/23 10:11:54, evn wrote:
> where can I see the code for DidPassCORSAccessCheck()? it seems like it should
> be checking for mode==cors somewhere, but I can't find it

https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/buffer...

Code search has xrefs, so you can click around and explore from that link too.

[evn, 2015-09-23 19:29:54]

DidPassCORSAccessCheck() seems to just check if the element has a crossorigin
attribute, right?

[DaleCurtis, 2015-09-23 22:20:12]

Yes, it's expecting WebURLLoader to enforce the CORS option and fail to load if
CORS is not passed:

https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/buffer...

[horo, 2015-09-24 06:59:52]

Looks good.
But please add LayoutTest case in http/tests/media/mixed-range-response.html for
this change.

[chromium-reviews, 2015-09-24 07:59:59]

Thanks Dale, rather than keep on asking you questions I'll just say what I
had in mind :-).

  1. Changing/removing the crossorigin attribute in between requests
doesn't work, right? Because its only read at the beginning.

  2. If a Service Worker intercepts the request and makes it non-cors who
is checking the response was CORS enabled? I suspect it's the loader, but I
am not sure of that.

I think those are the two main issues I could imagine failing in this case.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[horo, 2015-09-25 03:17:06]

>  2. If a Service Worker intercepts the request and makes it non-cors who
> is checking the response was CORS enabled? I suspect it's the loader, but I
> am not sure of that.

When a Service Worker fetches a non-cors request to cross-origin server, it will
receive an opaque response.
But the Service Worker can't respond to CORS fetch event with the opaque
response.

>
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
> void RespondWithObserver::responseWasFulfilled(const ScriptValue& value)
> {
> ...
>   if (responseType == FetchResponseData::OpaqueType) {
>     if (m_requestMode != WebURLRequest::FetchRequestModeNoCORS) {
>       responseWasRejected(WebServiceWorkerResponseErrorResponseTypeOpaque);
>       return;
>     }
>     ...
>   }
>   ...
> }

[chromium-reviews, 2015-09-25 07:57:57]

OK thanks horo :-)

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[DaleCurtis, 2015-09-27 18:48:19]

On 2015/09/24 06:59:52, horo wrote:
> Looks good.
> But please add LayoutTest case in http/tests/media/mixed-range-response.html
for
> this change.

Hmmm, this is turning out to be hard since load-video.php/serve-video.php don't
understand cors. Some further pointers would be appreciated if you had a
specific idea in mind. Otherwise, I'll see if I can repurpose any of the
existing cors test server code files for this.

[horo, 2015-09-28 06:04:59]

On 2015/09/27 18:48:19, DaleCurtis wrote:
> On 2015/09/24 06:59:52, horo wrote:
> > Looks good.
> > But please add LayoutTest case in http/tests/media/mixed-range-response.html
> for
> > this change.
> 
> Hmmm, this is turning out to be hard since load-video.php/serve-video.php
don't
> understand cors. Some further pointers would be appreciated if you had a
> specific idea in mind. Otherwise, I'll see if I can repurpose any of the
> existing cors test server code files for this.

We can make load-video.php/serve-video.php support CORS by adding
"Access-Control-Allow-Origin" header.
I created a CL to add LayoutTest. https://codereview.chromium.org/1369193002

But I think there is a bug in redirected CORS request handling of
BufferedResourceLoader.
This test must succeed without your patch. But it fails.
>
https://codereview.chromium.org/1369193002/diff/1/third_party/WebKit/LayoutTe...
> "Mixing same CORS-allowed remote-origin responses with crossOrigin=anonymous
must succeed."

When BufferedResourceLoader sends the HTTP request, it sets "Range" header. 

https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/buffer...
And it sets "options.preflightPolicy = WebURLLoaderOptions::PreventPreflight".

https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/buffer...
So DocumentThreadableLoader doesn't send CORS-preflight even though the "Range"
header is not a simple header.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

The "Range" header is not a simple header, so it is not copied to
m_simpleRequestHeaders.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
When DocumentThreadableLoader sends the redirected request it copies the header
from m_simpleRequestHeaders which doesn't contain "Range" header.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

So the server returns the data from the first byte and the HTTP status is 200
not 206.

https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...
BufferedResourceLoader::didReceiveResponse() will handle 200 as an error.

https://code.google.com/p/chromium/codesearch#chromium/src/media/blink/buffer...

So in the current implementation Chrome can't load CORS enabled redirected media
resources.
Is this a known issue?

[DaleCurtis, 2015-09-28 16:58:28]

dalecurtis@chromium.org changed reviewers:
+ hubbe@chromium.org

[DaleCurtis, 2015-09-28 16:58:29]

Thanks for the test tip! I admittedly hadn't tried, just looked at the server
code and saw no cors support, sorry!

No this isn't a known issue, +hubbe who's looked at this code more recently than
I have in case he has any ideas. I'll try to take a look later today too.

[DaleCurtis, 2015-09-28 21:54:18]

dalecurtis@chromium.org changed reviewers:
+ sigbjorn@opera.com

[DaleCurtis, 2015-09-28 21:54:19]

+sigbjorn, specifying PreventPreflight for range requests seems incorrect given
the comments on the code review (callers should only use PreventPreflight for
simple headers) and the CORS spec (range is not a simple header),
https://codereview.chromium.org/69813010 -- @sigbjorn, do you have some further
context to add in this case?

[hubbe, 2015-09-28 23:05:24]

On 2015/09/28 21:54:19, DaleCurtis wrote:
> +sigbjorn, specifying PreventPreflight for range requests seems incorrect
given
> the comments on the code review (callers should only use PreventPreflight for
> simple headers) and the CORS spec (range is not a simple header),
> https://codereview.chromium.org/69813010 -- @sigbjorn, do you have some
further
> context to add in this case?

Unless some sites use the "Range" header to implement magic side effects, it
should be perfectly safe to bypass preflight checks and just include the Range
header. The spec says that the server should treat that as an "actual" request,
which is what we want.

      /Hubbe

[sigbjorn, 2015-09-29 07:06:51]

On 2015/09/28 21:54:19, DaleCurtis wrote:
> @sigbjorn, do you have some further
> context to add in this case?

No, but sigbjornf might have ;)

I don't seem to have edit rights, so leaving it to you to add him to cc.

[sof, 2015-09-29 07:31:32]

On 2015/09/28 21:54:19, DaleCurtis wrote:
> +sigbjorn, specifying PreventPreflight for range requests seems incorrect
given
> the comments on the code review (callers should only use PreventPreflight for
> simple headers) and the CORS spec (range is not a simple header),
> https://codereview.chromium.org/69813010 -- @sigbjorn, do you have some
further
> context to add in this case?

If I interpret the final message of the
http://lists.w3.org/Archives/Public/public-webappsec/2014Jan/0071.html thread
correctly, then Fetch might extend "simple headers" to include Range:, but
that's partly up to us to decide on allowing. Did anyone make progress on this
since the issue was raised? 

The vulnerability mentioned on that thread strikes me as a dated, temporary,
problem, but then I'm not up-to-date in any way on what mitigations that apache
and others now have in place for those "wide" Range requests.

[evn, 2015-09-29 08:48:07]

You can make the attack they are trying to protect against in Apache with
Service Workers.

[DaleCurtis, 2015-09-29 18:58:58]

Hmm, so it sounds like we don't want to add range to simple headers. What about
passing range through when clients explicitly request PreventPreflight though?
As far as I can tell that's not a web facing option.

[DaleCurtis, 2015-09-29 18:59:56]

dalecurtis@chromium.org changed reviewers:
+ sigbjornf@opera.com
- sigbjorn@opera.com

[DaleCurtis, 2015-09-29 21:53:54]

I'm heading OOO on Oct2, so I've gone ahead and uploaded a patch set which
merges horo@'s tests and an extension to include "range" as a simple header when
prevent preflight is requested. This passes all tests. If we can't resolve this
before I leave, I'll hand it over to hubbe@.

[hubbe, 2015-10-05 17:28:08]

lgtm

[hubbe, 2015-10-05 23:29:54]

Can I please get some l-g-t-m-s here?
people are chomping at the bits to get this landed.

[horo, 2015-10-06 06:48:53]

horo@chromium.org changed reviewers:
+ tyoshino@chromium.org

[horo, 2015-10-06 06:48:53]

+tyoshion@ who is a specialist in CORS.

[tyoshino (SeeGerritForStatus), 2015-10-07 07:49:08]

On 2015/10/06 06:48:53, horo wrote:
> +tyoshion@ who is a specialist in CORS.

I agree that we should basically keep the Range header considered to be
non-simple for XHR and Fetch to prevent the reported vulnerability. This fix
should be made only for the media element exceptionally for now, yes.

The scenario where non-simple requests are issued without preceding preflights
is not yet well considered in the Fetch Standard. But HTTP and CORS are
state-less. The sequence (preflight then actual) is not a kind of handshake. As
far as we're sure that we won't introduce any vulnerability, it's fine to skip
the preflight. EventSource sends "Last-Event-ID" header but prevents preflight
being issued by the ThreadableLoader as we're sure it's safe
(https://bugs.webkit.org/show_bug.cgi?id=65694,
https://bugs.webkit.org/show_bug.cgi?id=61862). Ideally, this should be
described in the HTML spec (currently not).

The range request is already there and is controlled by the media element. The
detailed behavior is not controllable by a malicious script. So, I think it's
safe to skip preflight for the range requests. I read the following:
- http://seclists.org/fulldisclosure/2011/Aug/237
- http://seclists.org/bugtraq/2007/Jan/83

One concern I just come up with but different from the issue being discussed
here is that an attacker may be able to respond with a multipart response with a
lot of parts with some Content-Range values and then redirect the following
request like the test case in bug505829 to have the victim to issue fragmented
range request to upload.wikimedia.org. Maybe no problem but just curious.

I believe use of UseAccessControl correctly guards us from the information leak.
DocumentThreadableLoader needs to be modified

Please make "range" in DocumentThreadableLoader configurable, and let the user
pass it to a ThreadableLoader. It's a user dependent parameter. E.g.
additionalHeadersToTreatAsSimple argument. Last-Event-ID should also fit it (no
need to address in this CL. Separate CL is fine), I think.

[tyoshino (SeeGerritForStatus), 2015-10-07 07:56:24]

On 2015/10/07 07:49:08, tyoshino wrote:
> One concern I just come up with but different from the issue being discussed
> here is that an attacker may be able to respond with a multipart response with
a
> lot of parts with some Content-Range values and then redirect the following
> request like the test case in bug505829 to have the victim to issue fragmented
> range request to http://upload.wikimedia.org. Maybe no problem but just
curious.

As multipart response with Content-Range is at least not coalesced in the
BufferedResourceHandler (as ThreadableLoaderClient, it receives data in order),
this is not specific to the media element. So, never mind. Need to be discussed
somewhere else.

[tyoshino (SeeGerritForStatus), 2015-10-07 07:57:16]

On 2015/10/07 07:56:24, tyoshino wrote:
> On 2015/10/07 07:49:08, tyoshino wrote:
> > One concern I just come up with but different from the issue being discussed
> > here is that an attacker may be able to respond with a multipart response
with
> a
> > lot of parts with some Content-Range values and then redirect the following
> > request like the test case in bug505829 to have the victim to issue
fragmented
> > range request to http://upload.wikimedia.org. Maybe no problem but just
> curious.
> 
> As multipart response with Content-Range is at least not coalesced in the
> BufferedResourceHandler (as ThreadableLoaderClient, it receives data in
order),
> this is not specific to the media element. So, never mind. Need to be
discussed
> somewhere else.

s/ThreadableLoaderClient/WebURLLoaderClient/

[tyoshino (SeeGerritForStatus), 2015-10-07 08:12:42]

On 2015/10/07 07:57:16, tyoshino wrote:
> On 2015/10/07 07:56:24, tyoshino wrote:
> > On 2015/10/07 07:49:08, tyoshino wrote:
> > > One concern I just come up with but different from the issue being
discussed
> > > here is that an attacker may be able to respond with a multipart response
> with
> > a
> > > lot of parts with some Content-Range values and then redirect the
following
> > > request like the test case in bug505829 to have the victim to issue
> fragmented
> > > range request to http://upload.wikimedia.org. Maybe no problem but just
> > curious.
> > 
> > As multipart response with Content-Range is at least not coalesced in the
> > BufferedResourceHandler (as ThreadableLoaderClient, it receives data in
> order),
> > this is not specific to the media element. So, never mind. Need to be
> discussed
> > somewhere else.
> 
> s/ThreadableLoaderClient/WebURLLoaderClient/

Just checked that BufferedResourceLoader::ParseContentRange doesn't accept such
a fragmented response.

[tyoshino (SeeGerritForStatus), 2015-10-08 05:56:41]

I wrote a very long reply but what I want you to change is just the following.

On 2015/10/07 07:49:08, tyoshino wrote:
> Please make "range" in DocumentThreadableLoader configurable, and let the user
> pass it to a ThreadableLoader. It's a user dependent parameter. E.g.
> additionalHeadersToTreatAsSimple argument. Last-Event-ID should also fit it
(no
> need to address in this CL. Separate CL is fine), I think.

[tyoshino (SeeGerritForStatus), 2015-10-08 14:37:38]

tyoshino@chromium.org changed reviewers:
+ mkwst@chromium.org

[tyoshino (SeeGerritForStatus), 2015-10-08 14:38:42]

+mkwst

I'd like Mike to check if my analysis #29- is correct.

[DaleCurtis, 2015-10-19 23:30:36]

On 2015/10/08 05:56:41, tyoshino wrote:
> I wrote a very long reply but what I want you to change is just the following.
> 
> On 2015/10/07 07:49:08, tyoshino wrote:
> > Please make "range" in DocumentThreadableLoader configurable, and let the
user
> > pass it to a ThreadableLoader. It's a user dependent parameter. E.g.
> > additionalHeadersToTreatAsSimple argument. Last-Event-ID should also fit it
> (no
> > need to address in this CL. Separate CL is fine), I think.

To clarify, are you suggesting:
- We add a new WebURLLoaderOptions parameter like
"additionalHeaderKeysToTreatAsSimple" (note this would only be the key and not
the value).
- Pass "range" via the new field in WebURLLoaderOptions to
WebFrame::createAssociatedURLLoader() in BufferedResourceLoader::Start()
- Propagate that constant and value into either ThreadableLoaderOptions or
ResourceLoaderOptions.
- Use the new value in DocumentThreadedLoader constructor when filling out the
m_simpleRequestHeaders field.

If so, this seems okay to me, but is more powerful than I was proposing (a
general purpose scheme for forcing simple headers versus only allowing range as
a simple header in limited circumstance). Again if you're okay with this, I'll
go ahead and make that change after your response.

[DaleCurtis, 2015-10-21 00:25:23]

I pinged Mike via e-mail. tyoshino@ can you respond to my last comment? Thanks!

[tyoshino (SeeGerritForStatus), 2015-10-21 04:24:08]

On 2015/10/21 00:25:23, DaleCurtis wrote:
> I pinged Mike via e-mail. tyoshino@ can you respond to my last comment?
Thanks!

Oh, sorry! Right, that's exactly what I suggested.

As this is P1, in case you encounter any difficulties that would lead to
significant increase of time to fix this, please feel free to postpone it by
leaving TODO.

[Mike West, 2015-10-21 07:05:40]

On 2015/10/21 at 04:24:08, tyoshino wrote:
> On 2015/10/21 00:25:23, DaleCurtis wrote:
> > I pinged Mike via e-mail. tyoshino@ can you respond to my last comment?
Thanks!
> 
> Oh, sorry! Right, that's exactly what I suggested.
> 
> As this is P1, in case you encounter any difficulties that would lead to
significant increase of time to fix this, please feel free to postpone it by
leaving TODO.

I'm a bit skeptical about adding the ability to easily bypass the preflight
requirement. It shouldn't be simple, as it's not something we actually want
folks to do on a regular basis. I would prefer to maintain the current level of
annoyance for these kinds of changes, in the hopes that we can stop making them.
:)

With regard to `range`, I agree with Anne's analysis on the thread that Sigbjorn
referenced: this doesn't seem like a wide-spread source of vulnerability that is
pervasive on the net, but an issue with a specific subset of a specific server
(and it's 4 years old by now (and the underlying issue has been mitigated in the
HTTP spec (http://trac.tools.ietf.org/wg/httpbis/trac/changeset/2157)). For the
future, I'd suggest that we file a bug against Fetch to add `range` as a simple
header to see if there are other vulnerabilities I'm missing. For the moment,
this patch LGTM as-is.

[tyoshino (SeeGerritForStatus), 2015-10-21 07:29:02]

On 2015/10/21 07:05:40, Mike West (slow) wrote:
> On 2015/10/21 at 04:24:08, tyoshino wrote:
> > On 2015/10/21 00:25:23, DaleCurtis wrote:
> > > I pinged Mike via e-mail. tyoshino@ can you respond to my last comment?
> Thanks!
> > 
> > Oh, sorry! Right, that's exactly what I suggested.
> > 
> > As this is P1, in case you encounter any difficulties that would lead to
> significant increase of time to fix this, please feel free to postpone it by
> leaving TODO.
> 
> I'm a bit skeptical about adding the ability to easily bypass the preflight
> requirement. It shouldn't be simple, as it's not something we actually want

Good point. But the key for circumventing the preflight requirement is
|preflightPolicy|. It's already exposed for users in easy-to-use form. The list
of header I suggested to add just tells DocumentThreadableLoader which headers
to preserve on redirect or not.

Maybe additionalHeaderKeysToTreatAsSimple() is not the right name to give to the
list, and ideally the interface shouldn't be like this (combination of
preflightPolicy and this hard-to-correctly-understand list of header names).

OK, please proceed with the current patch set as-is. I agree with Mike in
general that adding a new argument may deliver misleading message to users.

> folks to do on a regular basis. I would prefer to maintain the current level
of
> annoyance for these kinds of changes, in the hopes that we can stop making
them.
> :)
> 
> With regard to `range`, I agree with Anne's analysis on the thread that
Sigbjorn
> referenced: this doesn't seem like a wide-spread source of vulnerability that
is
> pervasive on the net, but an issue with a specific subset of a specific server
> (and it's 4 years old by now (and the underlying issue has been mitigated in
the
> HTTP spec (http://trac.tools.ietf.org/wg/httpbis/trac/changeset/2157)). For
the
> future, I'd suggest that we file a bug against Fetch to add `range` as a
simple
> header to see if there are other vulnerabilities I'm missing. For the moment,
> this patch LGTM as-is.

[DaleCurtis, 2015-10-21 17:39:12]

Thanks for the reviews everyone, CQ'ing this CL based on the current approvals.
Please yell if you feel otherwise.

[DaleCurtis, 2015-10-21 17:39:16]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2015-10-21 17:40:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1356353003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1356353003/20001

[commit-bot: I haz the power, 2015-10-21 18:52:58]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-21 18:53:00]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[DaleCurtis, 2015-10-21 23:13:06]

The CQ bit was checked by dalecurtis@chromium.org

[DaleCurtis, 2015-10-21 23:13:07]

The patchset sent to the CQ was uploaded after l-g-t-m from hubbe@chromium.org,
mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1356353003/#ps40001
(title: "Revert accidental php change.")

[commit-bot: I haz the power, 2015-10-21 23:14:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1356353003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1356353003/40001

[commit-bot: I haz the power, 2015-10-22 00:26:34]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-22 00:27:19]

Patchset 3 (id:??) landed as
https://crrev.com/e11ea5ed677321f5fa24e8e77b01f8f57a0098a5
Cr-Commit-Position: refs/heads/master@{#355452}