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

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: sdna.muneaki.nishimura, Assigned: Ehsan)

Tracking

({csectype-sop, sec-critical})

41 Branch
mozilla41
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +
in-testsuite +

Firefox Tracking Flags

(e10s+, firefox38 unaffected, firefox39 unaffected, firefox40+ disabled, firefox41+ fixed, firefox-esr31 unaffected, firefox-esr38 unaffected)

Details

Attachments

(13 attachments, 1 obsolete attachment)

254.63 KB, image/png
Details
2.32 KB, patch
mcmanus
: review+
Details | Diff | Splinter Review
2.24 KB, patch
mcmanus
: review+
Details | Diff | Splinter Review
1.95 KB, patch
jdm
: review+
Details | Diff | Splinter Review
6.90 KB, patch
jdm
: review+
Details | Diff | Splinter Review
6.03 KB, patch
Details | Diff | Splinter Review
6.39 KB, patch
jdm
: review+
Details | Diff | Splinter Review
5.69 KB, patch
jdm
: review+
Details | Diff | Splinter Review
8.78 KB, patch
jdm
: review+
Details | Diff | Splinter Review
14.98 KB, patch
jdm
: review+
Details | Diff | Splinter Review
10.31 KB, patch
jdm
: review+
Details | Diff | Splinter Review
9.98 KB, patch
jdm
: review+
Details | Diff | Splinter Review
8.03 KB, patch
jdm
: review+
Details | Diff | Splinter Review
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.
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.
Assignee: nobody → nsm.nikhil
Flags: sec-bounty?
Flags: needinfo?(nsm.nikhil)
Keywords: sec-critical
Josh, this may be related to redirection too.
Flags: needinfo?(nsm.nikhil)
jdm is at tribe today so I've asked Ehsan to take a look.
One thing to note here is that this bug is e10s specific.
Summary: SOP bypass with the service worker and 30x redirect → [e10s] SOP bypass with the service worker and 30x redirect
FWIW I have disabled service workers on Nightly and Aurora until this and bug 1162018 get fixed.
Assignee: nsm.nikhil → ehsan
This is not service worker specific.
Component: DOM: Service Workers → DOM: Security
Summary: [e10s] SOP bypass with the service worker and 30x redirect → [e10s] SOP bypass with 30x redirect
(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...
Summary: [e10s] SOP bypass with 30x redirect → [e10s] SOP bypass with the service worker and 30x redirect
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.
Flags: needinfo?(josh)
Bug 1168208 will cover the required refactoring.  I didn't want to do the refactoring in a closed bug.
Posted patch The cache changes for the fix (obsolete) — Splinter Review
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.
Attachment #8612933 - Flags: review?(bkelly)
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!
Attachment #8612933 - Flags: review?(bkelly) → review+
(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.
Component: DOM: Security → DOM: Service Workers
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.
Attachment #8614364 - Flags: review?(josh) → review-
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()))`?
Attachment #8614367 - Flags: review?(josh) → review+
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())?
Attachment #8614370 - Flags: review?(josh) → review+
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.
Attachment #8614365 - Flags: review?(josh)
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.
(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?
Flags: needinfo?(josh)
We could fulfill the original request by calling ResetInterception, I suppose...
Flags: needinfo?(josh)
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.
Attachment #8614364 - Flags: review- → review+
Attachment #8614365 - Flags: review+
Attachment #8615975 - Flags: review?(josh) → review+
Attachment #8615994 - Flags: review?(josh) → review+
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)
Attachment #8614363 - Flags: review?(michal.novotny) → review+
Attachment #8614362 - Flags: review?(michal.novotny) → review+
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.
Attachment #8616039 - Flags: review?(josh) → review+
Attachment #8616142 - Flags: review?(josh) → review+
Attachment #8616154 - Flags: review?(josh) → review+
(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.
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.
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: needinfo?(ehsan)
Flags: needinfo?(abillings)
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
(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.
Flags: needinfo?(ehsan)
Flags: in-testsuite? → in-testsuite+
Flags: needinfo?(abillings)
Flags: sec-bounty? → sec-bounty+
Group: core-security → core-security-release
Group: core-security-release
Depends on: 1204596
Depends on: 1313740
You need to log in before you can comment on or make changes to this bug.