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

NEW
Unassigned

Status

()

P3
normal
a year ago
16 days ago

People

(Reporter: lidel, Unassigned)

Tracking

(4 keywords)

60 Branch
addon-compat, compat, dev-doc-needed, DevAdvocacy
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [domsecurity-backlog2])

Attachments

(1 attachment, 1 obsolete attachment)

1.25 KB, application/zip
Details
(Reporter)

Description

a year ago
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

Updated

11 months ago
Flags: needinfo?(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.
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(honzab.moz)
Flags: needinfo?(bugs)
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
Flags: needinfo?(honzab.moz)
Oh, interesting difference.  For lots of reasons I think we need the behavior that we currently exhibit.  Lets get documentation on mdn about this.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 10 months ago
Flags: needinfo?(bugs)
Keywords: dev-doc-needed
Resolution: --- → WONTFIX
(Reporter)

Comment 4

10 months ago
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
> 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?
Status: RESOLVED → REOPENED
Ever confirmed: true
Flags: needinfo?(honzab.moz)
Resolution: WONTFIX → ---
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.
Flags: needinfo?(honzab.moz)
Jason, do you know who could chime in on this issue?  First para in comment 1 sums up the issue.
Flags: needinfo?(jduell.mcbugs)

Updated

9 months ago
Product: Toolkit → WebExtensions

Comment 8

8 months ago
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

8 months ago
Could you give me an idea of the best person to ask questions about this?
Flags: needinfo?(mixedpuppy)
This bug isn't closed yet so no need to look at it for dev-doc.
Flags: needinfo?(mixedpuppy)
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
Flags: needinfo?(mixedpuppy)
Posted patch wip1 (obsolete) — Splinter Review
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6e429f8ea9e0c4bb0d995cfae37096a8c4827d87
Assignee: nobody → honzab.moz
Status: REOPENED → ASSIGNED
Attachment #9011282 - Flags: feedback?(mixedpuppy)
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.
(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*
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(jduell.mcbugs)
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.
Attachment #9011282 - Flags: feedback?(mixedpuppy) → feedback+
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 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.
Attachment #9011282 - Flags: review?(dd.mozilla)
(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.
Thanks.  I take this as an r- then.  This also moves the bug to a different component.
Assignee: honzab.moz → nobody
Status: ASSIGNED → NEW
Component: Request Handling → DOM: Security
Product: WebExtensions → Core
Comment on attachment 9011282 [details] [diff] [review]
wip1

see comment 18 for r- rational.
Attachment #9011282 - Attachment is obsolete: true
Attachment #9011282 - Flags: review?(dd.mozilla)
Attachment #9011282 - Flags: review-
Priority: -- → P3
Whiteboard: [domsecurity-backlog2]
Duplicate of this bug: 1495223
Summary: webRequest.onBeforeRequest fetch redirect gets blocked due to bogus CORS error → webRequest.onBeforeRequest/onHeadersReceived fetch redirect gets blocked due to bogus CORS error

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

Keywords: addon-compat, compat, DevAdvocacy
You need to log in before you can comment on or make changes to this bug.