1164397 - [e10s] SOP bypass with the service worker and 30x redirect

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Muneaki Nishimura]

Attachment: screen capture I reproduced

Step to Reproduce: 

(1) Login to Facebook
(2) Launch https://mallory.csrf.jp/sw/index3.html
(3) Reload the page, you can see the message "The service worker is successfully set."
(4) Push the "Read Facebook" button in the page, then the profile page of Facebook is opened in a new window.
(5) Wait for 5 seconds, then an alert dialog is shown on the page and it contains DOM contents of Facebook opened in a new window

Vulnerability:

In detail, the page opened by step (2) is controlled by a service worker and the worker tries to fetch all of contents requested through the page.
When you push the "Read Facebook" button, then the following page is opened in a new window.
https://mallory.csrf.jp/sw/redirect.php?s=308&u=https://www.facebook.com/profile.php
This page is a simple redirector to the profile page of Facebook, www.facebook.com/profile.php, with 308 status code.
The important thing here is that the redirector page is also goverened by the same service worker as the page opened by (2).
Then you can see the location bar of the child page remains the attacker's domain, however, the profile page of Facebook is already loaded in the child window, see the screen capture I attached.
This means that the Facebook page is loaded as the attacker's origin, i.e., mallory.csrf.jp.
So the attacker can retrieve it's page contents from the parent window and steal the sensitive information in the Facebook, as step (5).

Expected Behavior:

The attacker domain, mallory.csrf.jp, shouldn't read page contents of Facebook.

Comment 1

[Daniel Veditz [:dveditz]]

Confirmed! (horrifying)

I assume 40 is affected since the pref was turned on in that build (bug 1154900) but I haven't tested it yet. The bug is NOT reproducible in 39 even if you enable the pref: the popup simply redirects to facebook (as shown in the URL bar) and the same-origin policy correctly protects against access.

Nikhil: is this yours? If not could you please pass it along.

Comment 2

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Josh, this may be related to redirection too.

Comment 3

[Andrew Overholt [:overholt]]

jdm is at tribe today so I've asked Ehsan to take a look.

Comment 4

[(no longer active)]

One thing to note here is that this bug is e10s specific.

Comment 5

[(no longer active)]

FWIW I have disabled service workers on Nightly and Aurora until this and bug 1162018 get fixed.

Comment 6

[(no longer active)]

This is not service worker specific.

Comment 7

[(no longer active)]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #6)
> This is not service worker specific.

That wasn't right...  It is SW related...

Comment 8

[Josh Matthews [:jdm]]

Ehsan and I talked through this, and one potential solution is an extension of the fix from bug 1133763. There are a lot of channel properties that are not being propagated from the final channel associated with the Response of the `fetch` request; we currently copy the status, headers, body, and security information, but this bug is caused by missing values for things like the channel's load flags, final URI, principal, etc. We could create an opaque type that gets extracted from a channel, stored in InternalResponse, and passed to nsIInterceptedChannel and through to the underlying necko channel. This would ensure that the relevant channel properties are copied and overwritten by code in netwerk/, minimizing the future maintenance burden and the threat of similar errors if new security-related data is added to channels in the future.

Comment 9

[(no longer active)]

Bug 1168208 will cover the required refactoring.  I didn't want to do the refactoring in a closed bug.

Comment 10

[Andrew McCreight [:mccr8]]

Marking disabled per comment 5.

Comment 11

[(no longer active)]

Attachment: The cache changes for the fix

This is just the cache parts required for the fix.  Most likely I won't need to persist the original URL in the ChannelInfo, but since I'm submitting this for review before Ben goes on vacation, I might as well submit the whole patch, and potentially remove the response_original_url field later when landing.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8612933 [details] [diff] [review]
The cache changes for the fix

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

How do the response_original_url and response_redirected_url correspond to the response_url column?  Are they always different or possibly the same?

If there is any overlap, it would be nice to avoid storing the same URL in the database twice.  We can do that optimization later, of course.  Please just document their relationship.

Thanks!

Comment 13

[(no longer active)]

(In reply to Ben Kelly [PTO till June 8][:bkelly] from comment #12)
> How do the response_original_url and response_redirected_url correspond to
> the response_url column?  Are they always different or possibly the same?

I ended up dropping response_original_url.

response_redirected_url will almost always be empty, and when it is a non-empty string, it will always be different than response_url.

> If there is any overlap, it would be nice to avoid storing the same URL in
> the database twice.  We can do that optimization later, of course.  Please
> just document their relationship.

Will do.

Comment 14

[(no longer active)]

Attachment: Part 1: Use the original channel URI for constructing the cache entry key when we're dealing with an intercepted channel

Comment 15

[(no longer active)]

Attachment: Part 2: Add an API for overriding the original URI on HttpChannelBase

Comment 16

[(no longer active)]

Attachment: Part 3: Add an API for overriding the original URI on nsJARChannel

Comment 17

[(no longer active)]

Attachment: Part 4: Add infromation about whether a channel was redirected to ChannelInfo

Comment 18

[(no longer active)]

Attachment: Part 5: Save the redirected flag and the redirected URI in the DOM cache

Comment 19

[(no longer active)]

Attachment: Part 6: Add a test case for the service worker responding with a redirected Response

Comment 20

[(no longer active)]

Attachment: Part 7: Add a test case for the redirected Response object being stored in the DOM Cache

Comment 21

[Josh Matthews [:jdm]]

Comment on attachment 8614364 [details] [diff] [review]
Part 3: Add an API for overriding the original URI on nsJARChannel

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

I'm not convinced this is necessary since app URIs can't redirect. I feel like we should just shout loudly if we ever hit the code path that would call this instead.

Comment 22

[Josh Matthews [:jdm]]

Comment on attachment 8614367 [details] [diff] [review]
Part 6: Add a test case for the service worker responding with a redirected Response

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

::: dom/workers/test/serviceworkers/fetch/origin/index.sjs
@@ +1,3 @@
> +function handleRequest(request, response) {
> +  response.setStatusLine(null, 308, "Permanent Redirect");
> +  response.setHeader("Location", "http://example.org/tests/dom/workers/test/serviceworkers/fetch/origin/realindex.html", false);

Did we want to test https as well?

::: dom/workers/test/serviceworkers/fetch/origin/origin_test.js
@@ +1,4 @@
> +self.addEventListener("fetch", function(event) {
> +  event.respondWith(fetch(event.request.clone()).then(function(response) {
> +    return response;
> +  }));

Isn't this the same as `event.respondWith(fetch(event.request.clone()))`?

Comment 23

[Josh Matthews [:jdm]]

Comment on attachment 8614370 [details] [diff] [review]
Part 7: Add a test case for the redirected Response object being stored in the DOM Cache

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

::: dom/workers/test/serviceworkers/fetch/origin/origin_test.js
@@ +17,5 @@
> +          return c.match(prefix + 'index.sjs');
> +        })
> +    );
> +  } else {
> +    event.respondWith(fetch(event.request.clone()).then(function(response) {

event.respondWith(fetch(event.request.clone())?

Comment 24

[Josh Matthews [:jdm]]

Comment on attachment 8614365 [details] [diff] [review]
Part 4: Add infromation about whether a channel was redirected to ChannelInfo

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

::: dom/fetch/ChannelInfo.cpp
@@ +122,5 @@
> +      net::HttpBaseChannel* httpBaseChannel =
> +        static_cast<net::HttpBaseChannel*>(httpChannel.get());
> +      httpBaseChannel->OverrideOriginalURI(redirectedURI);
> +    } else {
> +      if (NS_WARN_IF(!jarChannel)) {

We should complain loudly here if we have a JAR channel that's been redirected, since that shouldn't be possible.

Comment 25

[Josh Matthews [:jdm]]

One option is to return NS_ERROR_NOT_AVAILABLE, and modify FinishResponse::Run (https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerEvents.cpp?from=finishresponse&case=true#126) to cancel the channel if SetChannelInfo fails.

Comment 26

[(no longer active)]

(In reply to Josh Matthews [:jdm] from comment #25)
> One option is to return NS_ERROR_NOT_AVAILABLE, and modify
> FinishResponse::Run
> (https://dxr.mozilla.org/mozilla-central/source/dom/workers/
> ServiceWorkerEvents.cpp?from=finishresponse&case=true#126) to cancel the
> channel if SetChannelInfo fails.

Canceling JAR channels doesn't seem to work.  Any other suggestions?

Comment 27

[Josh Matthews [:jdm]]

We could fulfill the original request by calling ResetInterception, I suppose...

Comment 28

[(no longer active)]

I and Josh talked about this extensively today.  We have to override the original URI on JAR channels as well, otherwise this bug would basically not be fixed for app:// URIs.

Comment 29

[(no longer active)]

Attachment: Part 8: Add a test case for the service worker for an app:// URI responding with a redirected Response

Comment 30

[(no longer active)]

Attachment: Part 9: Add a test case for the service worker for an app:// URI responding with a redirected HTTPS response

Comment 31

[(no longer active)]

Attachment: Part 10: Add a test case for the service worker for an app:// URI responding with cached HTTP and HTTPS responses

Comment 32

[Patrick McManus [:mcmanus]]

Comment on attachment 8614363 [details] [diff] [review]
Part 2: Add an API for overriding the original URI on HttpChannelBase

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

::: netwerk/protocol/http/HttpBaseChannel.h
@@ +248,5 @@
>      const NetAddr& GetSelfAddr() { return mSelfAddr; }
>      const NetAddr& GetPeerAddr() { return mPeerAddr; }
>  
>      nsresult OverrideSecurityInfo(nsISupports* aSecurityInfo);
> +    void OverrideOriginalURI(nsIURI* aRedirectedURI);

let's just call it OverrideURI(nsIURI *aURI)

Comment 33

[Patrick McManus [:mcmanus]]

Comment on attachment 8614365 [details] [diff] [review]
Part 4: Add infromation about whether a channel was redirected to ChannelInfo

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

::: dom/fetch/ChannelInfo.cpp
@@ +93,3 @@
>      if (httpChannel) {
>        net::HttpBaseChannel* httpBaseChannel =
>          static_cast<net::HttpBaseChannel*>(httpChannel.get());

I'm not sure how safe this is.. unless there is some other mechanism that enforces a mozilla http channel implementation upstream.

I know I've seen extensions overload nsIChannel (generally putting a shim in between the extension and our code).. plausibly they could do nsIHttpChannel too.

Comment 34

[(no longer active)]

Attachment: Part 11: Add a test case for the service worker responding to HTTP normal and cached Responses

Comment 35

[(no longer active)]

Attachment: Part 12: Add a test case for the service worker responding to normal and cached HTTP->HTTPS responses

Comment 36

[(no longer active)]

(In reply to Patrick McManus [:mcmanus] from comment #33)
> Comment on attachment 8614365 [details] [diff] [review]
> Part 4: Add infromation about whether a channel was redirected to ChannelInfo
> 
> Review of attachment 8614365 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/fetch/ChannelInfo.cpp
> @@ +93,3 @@
> >      if (httpChannel) {
> >        net::HttpBaseChannel* httpBaseChannel =
> >          static_cast<net::HttpBaseChannel*>(httpChannel.get());
> 
> I'm not sure how safe this is.. unless there is some other mechanism that
> enforces a mozilla http channel implementation upstream.
> 
> I know I've seen extensions overload nsIChannel (generally putting a shim in
> between the extension and our code).. plausibly they could do nsIHttpChannel
> too.

That's a good point.  My patch in bug 1172139 fixes this.

Comment 37

[Carsten Book [:Tomcat]]

landed on central:

Changeset	Link	Bug
76e8c42fe894	https://hg.mozilla.org/mozilla-central/rev/76e8c42fe894	1164397
7d5686845634	https://hg.mozilla.org/mozilla-central/rev/7d5686845634	1164397
24d7d4dfab29	https://hg.mozilla.org/mozilla-central/rev/24d7d4dfab29	1164397
435358bf7c12	https://hg.mozilla.org/mozilla-central/rev/435358bf7c12	1164397
095f8f54dcd2	https://hg.mozilla.org/mozilla-central/rev/095f8f54dcd2	1164397
9fe75f3e2131	https://hg.mozilla.org/mozilla-central/rev/9fe75f3e2131	1164397
c28b933c091d	https://hg.mozilla.org/mozilla-central/rev/c28b933c091d	1164397
cbc748ab81ee	https://hg.mozilla.org/mozilla-central/rev/cbc748ab81ee	1164397
a675f30287e9	https://hg.mozilla.org/mozilla-central/rev/a675f30287e9	1164397
c8adea6c5009	https://hg.mozilla.org/mozilla-central/rev/c8adea6c5009	1164397
952ffb707f78	https://hg.mozilla.org/mozilla-central/rev/952ffb707f78	1164397
451715a0e1dc	https://hg.mozilla.org/mozilla-central/rev/451715a0e1dc	1164397


Ehsan, i guess this would have needed security approval when this landed on mozilla-inbound.

Comment 38

[(no longer active)]

(In reply to Carsten Book [:Tomcat] from comment #37)
> Ehsan, i guess this would have needed security approval when this landed on
> mozilla-inbound.

No, this only affects mozilla-central.