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.

Josh, this may be related to redirection too.

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.

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

This is not service worker specific.

(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...

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.

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

Marking disabled per comment 5.

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 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!

(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 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 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 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 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.

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?

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

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 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 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.

(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.

(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.