1450965 - webRequest.onBeforeRequest/onHeadersReceived fetch redirect gets blocked due to bogus CORS error

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P2)

Description

[Marcin Rataj]

Attachment: ipfs-companion-cors-issue-436-mvp.zip

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180323154952

Steps to reproduce:

Issue was initially reported at: https://github.com/ipfs-shipyard/ipfs-companion/issues/436

For some reason webRequest.onBeforeRequest is not able to redirect fetch request made to a third-party host back to the same path at origin host without tripping CORS error. 
Not sure if this is a bug or security feature, but Chrome does not block this, so I assume Firefox CORS error is a false-positive. 

It may be tricky to replicate, so I created a PoC extension that demonstrates the problem (attached  [ipfs-companion-cors-issue-436-mvp.zip][1])

1. Make sure every other extension that uses `onBeforeRequest` is disabled 
2. Load attached [poc extension][1] into Firefox and Chrome
3. Extension automatically opens up the [test page][2] from `ipfs.io` host
    3.1. page provides two buttons for fetching the same resource from two different gateways: `ipfs.io` (the host used for loading test page) and `ipfs.infura.io` (3rd party host)
    3.2. the extension registers `onBeforeRequest` hook that redirects `fetch` calls for the resource at `ipfs.infura.io` back to the same path at `ipfs.io` host (this will trigger the bug)
    3.3. NOTE: both hosts used in test have proper CORS headers
4. Open Console
5. Press  first button (OK)
6. Press second button (CORS ERROR in Firefox)


[1]: https://github.com/ipfs-shipyard/ipfs-companion/files/1871344/ipfs-companion-cors-issue-436-mvp.zip
[2]: https://ipfs.io/ipfs/QmNXjqCXL8YnmPY3McLz33HEdNsen3wjfDjRRh96WYVNBW


Actual results:

Only the first button results in a successful fetch, second button triggers CORS error.
Firefox blocks JS fetch due to CORS.

Screenshot: https://user-images.githubusercontent.com/157609/38248315-40494be4-3748-11e8-8d41-ce565d77babb.png




Expected results:

Both buttons should result in a successful fetch and `This is a string downloaded via JS fetch` being printed to the console.
The same extension works in Chrome and there is no CORS error for internally (307) redirected fetch:

Screenshot: https://user-images.githubusercontent.com/157609/38248306-3967a10e-3748-11e8-9c60-e0be908688c1.png

Comment 1

[Shane Caraveo (:mixedpuppy)]

The extension is redirecting an xhr request that would not otherwise result in a cors violation to a domain that also should not be a cors violation.  However, the request is still blocked due to cors.

This is the basic STR

page on ipfs.io makes xhr request to ipfs.infura.io
extension redirects the xhr request to ipfs.io

expected:

request is successful

actual:

request is blocked due to cors.

notes: 
- both domains set "Access-Control-Allow-Origin: *"
- disabling the extension and clicking the second button results in a successful cross-domain fetch.


ISTM that this should work.  Let see if we can get some input from others who are probably more familiar with how the cors listener is working.

Comment 2

[Honza Bambas (:mayhemer)]

I made a quick test with the extension, btw - thanks for it!  and apparently even if the response to [1] has the appropriate headers, we don't even make the request when redirectTo is used on the channel, it's simply happening too soon.

According [2] and the redirectUrl notes, you can use redirectUrl from either onBeforeRequest or from onHeadersReceived.  Apparently Chrome does perform the request and waits for the response even though you used redirectUrl from onBeforeRequest.  We intercept (redirect) the channel before the request happens, which IMO makes more sense.

I changed the extension to hook onHeadersReceived instead and it works as expected.

When/if exactly to perform the redirect when redirectUrl is used from onBeforeRequest is a question more for web extension people writing the API specs, than me.

[1] https://ipfs.infura.io/ipfs/QmNuSGLMECZ7bdwGBZ2QbXm2kkEXEuPPDcf2HqyVPpKfc8
[2] https://developer.chrome.com/extensions/webRequest

Comment 3

[Shane Caraveo (:mixedpuppy)]

Oh, interesting difference.  For lots of reasons I think we need the behavior that we currently exhibit.  Lets get documentation on mdn about this.

Comment 4

[Marcin Rataj]

I may be missing some context here, but.. 
if CORS error is real,  why was it marked as WONTFIX?

The way I see it CORS handling around redirectUrl in webRequest.onBeforeRequest API exhibits broken characteristics. 
Fetch should detect context change caused by redirectUrl. Instead, it fails due to CORS.

Switching to onHeadersReceived is not a viable workaround:
The whole point of returning redirectUrl in onBeforeRequest is to AVOID sending request to original URL and doing redirect BEFORE original request is sent. 
If we can't use it and have to rely on onHeadersReceived then it is too late -- request was already sent to third party server and response headers were received. This is a huge privacy leak for users who want to redirect requests to a local daemon instead of public server. 

Is there any workaround apart from onHeadersReceived? 

ps. And to clarify Chrome status: it does not send original request if I hijack and redirect it in onBeforeRequest. 
It just logs 'fake' HTTP 307 (internal redirect) for redirectUrl, see screenshot: https://ipfs.io/ipfs/QmQB27MJvgnGPRhUNd4cawB7HwMR3WKb4ZwhuPeknZMc3n

Comment 5

[Shane Caraveo (:mixedpuppy)]

> ps. And to clarify Chrome status: it does not send original request if I
> hijack and redirect it in onBeforeRequest. 
> It just logs 'fake' HTTP 307 (internal redirect) for redirectUrl, see
> screenshot:
> https://ipfs.io/ipfs/QmQB27MJvgnGPRhUNd4cawB7HwMR3WKb4ZwhuPeknZMc3n

Hrm, I closed the bug because Honza's look contradicted this.  

Honza, do internal redirects get processed for CORS?  Should we just do internal redirects if it happens from onBeforeRequest?

Comment 6

[Honza Bambas (:mayhemer)]

The problem is a bit elsewhere.  I didn't close the bug, I just summarized observations so that someone else may make an educated conclusion on it.  We probably need to somehow act on this.

We require a CORS check on the fetch() for https://ipfs.infura.io/ipfs/... which also examines possible redirects on the channel serving that fetch.  We do that because SEC_REQUIRE_CORS_DATA_INHERITS has been set on the fetch'es load info (expected).

The `redirectUrl` (internally doing old-channel.redirectTo()) induced redirect is a regular redirect and not an internal one.  I believe that is the more correct approach and we want to perform security checks on it.

In this case the CORS check depends on the response, hence I'm not sure how to treat this.

I'm not sure we want to treat redirectTo() before making the request as internal and thus skip security checking.

Not sure who to ask on the conclusion either.

Comment 7

[Shane Caraveo (:mixedpuppy)]

Jason, do you know who could chime in on this issue?  First para in comment 1 sums up the issue.

Comment 8

[Irene Smith]

I'm probably missing the point here. What should I be talking about? It seems onHeadersReceived is not the place to perform a redirect, so what should be be recommending?

Comment 9

[Irene Smith]

Could you give me an idea of the best person to ask questions about this?

Comment 10

[Shane Caraveo (:mixedpuppy)]

This bug isn't closed yet so no need to look at it for dev-doc.

Comment 11

[Honza Bambas (:mayhemer)]

We fail exactly at [1], because the "Access-Control-Allow-Origin" response header has not been found - the request has not been made yet.  

What Chrome seams to do is to add the "Access-Control-Allow-Origin" response header artificially on the current request (in out terminology - the old channel, the one we are calling redirectTo() on) with the value of the origin the request has been made from, apparently to internally pass CORS checks.  This is an option 1 to fix this, add the header when we are still in the "opening" phase.  This could be enhancement of the redirectTo method signature (to pass origin allowance).

Option 2 is to enhance bug 1434357.  In this case at [2], when |kind| is "opening", we want to behave as if tainting has been synthesized.  This needs careful evaluation.  A quick hack in nsCORSListenerProxy::CheckRequestApproved proves it works.

Shane, what do you think?  I have a patch for option 1 that may just need some polishing and sec evaluation.


[1] https://searchfox.org/mozilla-central/rev/62ce7b988d85e160ab96624001f0d1c33b0089ee/netwerk/protocol/http/nsCORSListenerProxy.cpp#599
[2] https://searchfox.org/mozilla-central/rev/62ce7b988d85e160ab96624001f0d1c33b0089ee/toolkit/modules/addons/WebRequest.jsm#843

Comment 12

[Honza Bambas (:mayhemer)]

Attachment: wip1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e429f8ea9e0c4bb0d995cfae37096a8c4827d87

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 9011282 [details] [diff] [review]
wip1

Review of attachment 9011282 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +2313,5 @@
> +
> +  // We may want to rewrite origin allowance, hence we need an
> +  // artificial response head.
> +  if (!mResponseHead) {
> +    mResponseHead = new nsHttpResponseHead();

note that this artificial head will be rewriting by taking a cached response or by any network response.

Comment 14

[Honza Bambas (:mayhemer)]

(In reply to Honza Bambas (:mayhemer) from comment #13)
> note that this artificial head will be rewriting by taking a cached response
> or by any network response.

Will be *rewritten*

Comment 15

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 9011282 [details] [diff] [review]
wip1

This all seems reasonable.  I think we could avoid the changes to channelwrapper by using channel.originURI.prePath, but I'm also ok with the wrapper changes.

Comment 16

[Honza Bambas (:mayhemer)]

OK, I will write a test for this as well.  uri.prePath is not the same as origin.  we may also need to handle different schemes than http.  and as this is actually a security check, I really want to use the proper way.

Comment 17

[Honza Bambas (:mayhemer)]

Comment on attachment 9011282 [details] [diff] [review]
wip1

Dragana, can you please review the necko parts here?  I intend to write a test (when I have time) and then review again from :mixedpuppy and land.

Comment 18

[Dragana Damjanovic [:dragana]]

(In reply to Honza Bambas (:mayhemer) from comment #11)
> We fail exactly at [1], because the "Access-Control-Allow-Origin" response
> header has not been found - the request has not been made yet.  
> 
> What Chrome seams to do is to add the "Access-Control-Allow-Origin" response
> header artificially on the current request (in out terminology - the old
> channel, the one we are calling redirectTo() on) with the value of the
> origin the request has been made from, apparently to internally pass CORS
> checks.  This is an option 1 to fix this, add the header when we are still
> in the "opening" phase.  This could be enhancement of the redirectTo method
> signature (to pass origin allowance).
> 
> Option 2 is to enhance bug 1434357.  In this case at [2], when |kind| is
> "opening", we want to behave as if tainting has been synthesized.  This
> needs careful evaluation.  A quick hack in
> nsCORSListenerProxy::CheckRequestApproved proves it works.
> 
> Shane, what do you think?  I have a patch for option 1 that may just need
> some polishing and sec evaluation.
> 
> 
> [1]
> https://searchfox.org/mozilla-central/rev/
> 62ce7b988d85e160ab96624001f0d1c33b0089ee/netwerk/protocol/http/
> nsCORSListenerProxy.cpp#599
> [2]
> https://searchfox.org/mozilla-central/rev/
> 62ce7b988d85e160ab96624001f0d1c33b0089ee/toolkit/modules/addons/WebRequest.
> jsm#843

I prefer option 2. It is more explicit what we are doing here. The other is a real hack.

Comment 19

[Honza Bambas (:mayhemer)]

Thanks.  I take this as an r- then.  This also moves the bug to a different component.

Comment 20

[Honza Bambas (:mayhemer)]

Comment on attachment 9011282 [details] [diff] [review]
wip1

see comment 18 for r- rational.

Comment 22

[Dietrich Ayala (:dietrich)]

Hi Christoph! Now that product/component changed there's no more progress. Can you help with getting this prioritized?

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Dietrich Ayala (:dietrich) from comment #22)

Hi Christoph! Now that product/component changed there's no more progress. Can you help with getting this prioritized?

We are triaging bugs tonight - I am clearing flags so it shows up in our queries and we can re-evaluate the priority.

Comment 25

[Christoph Kerschbaumer [:ckerschb]]

Dragana, there are two duplicates of this bug and it already had a WIP patch at some point. I am not sure on the priority of this bug - can you weigh in? Or should/can we put it back in the backlog?

Comment 26

[Guru Prasad]

I'm one of the users who filed a duplicate bug. In my opinion, webextensions allow packaging of web_resources to enhance/alter functionality of the browser. In that regard, this bug severely cripples the ability to alter functionality of websites as we can no longer package modifications and redirect requests to the packaged content.

The only remaining alternate is to redirect the URLs to an external website which is a security risk (iirc, AMO does not allow this) and additionally makes the manual review process even more cumbersome than it already is. I guess what I'm trying to get at is that all of this essentially means that there are absolutely no alternatives available at the moment.

Comment 27

[Dragana Damjanovic [:dragana]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #25)

Dragana, there are two duplicates of this bug and it already had a WIP patch at some point. I am not sure on the priority of this bug - can you weigh in? Or should/can we put it back in the backlog?

WebExtension team should decide on priority for this bug.
If we want to land on possible solution in comment 11, we will need someone from security team to take a look if the propose solutions are ok.

Comment 28

[Shane Caraveo (:mixedpuppy)]

We've lived with it for a long time, so I wouldn't necessarily P1 it. I would P2 it so long as we can have forward movement on a fix.

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

Basti, can you take a look at this one as well please? Looking at comment 11 and also bug 1434357 should provide some guidance to get started - thanks!

Comment 30

[Sebastian Streich [:sstreich]]

Attachment: Bug 1450965 Skip Cors Check for Early WebExtention Redirects r=ckerschb

Comment 31

[Honza Bambas (:mayhemer)]

Sebastian, hi, I did not forget about your patch, it's just that I don't have time right now to look at it. I'll try to do the review sometimes this week. Thanks and sorry for delays.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/308ef98e6e4b
Skip Cors Check for Early WebExtention Redirects r=mayhemer

Comment 33

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/308ef98e6e4b

Comment 34

[Richard Bloor]

Hi, would it be possible to get some details on the documentation changes needed? Thank you!

Comment 35

[Shane Caraveo (:mixedpuppy)]

The dev-doc was about documenting that you couldn't redirect (assuming we were not going to fix it), but since this bug is fixed, nothing is needed.