In bug 1222008 we added a flag that specifies that the tainting on a particular LoadInfo has been synthesized by a service worker. This allows the service worker to override the tainting that would have been otherwise set for the response. For example, if a page does `fetch(crossOriginURL)`, we start tainting the response immediately when we see the cross-origin URL. The intercepting service worker, though, can do `e.respondWith(fetch(sameOriginURL))` which overrides the tainting back to a same-origin "basic" response. The flag added in bug 1222008 makes sure our security code doesn't then come along and force the tainting back to something else. The problem, though, is we are currently copying the flag when a redirected channel's LoadInfo is created: https://searchfox.org/mozilla-central/rev/c621276fbdd9591f52009042d959b9e19b66d49f/netwerk/base/LoadInfo.cpp#399 I believe this means probably means its possible to bypass same-origin policy and read no-cors opaque responses by doing something like: 1. evil.com registers a service worker 2. evil.com makes some request that will be intercepted by the SW 3. The SW responds with a redirect status code and Location header pointing at a private resource on good.com. This sets the synthesized flag. 4. The redirect is initiated and re-enters the SW, but the SW does not call respondWith(). (This incorrectly preserves the synthesized flag from 4.) 5. The normal network response retrieves good.com's resource, but is returned as a basic response because the synthesized flag from 4 is still set. I'll see if I can write a test that demonstrates this.

Ok, this WPT change demonstrates the problem. We currently fail with: FAIL mode: "follow", cors generated cross-origin redirect response - assert_equals: The response typ e of response must match. URL: https://web-platform.test:8443/service-workers/service-worker/dummy?u rl=https%3A%2F%2Fwww1.web-platform.test%3A8443%2Fservice-workers%2Fservice-worker%2Fresources%2Fsimp le-cors.txt%3F&original-redirect-mode=follow&sw=gen expected "cors" but got "basic" FAIL mode: "follow", no-cors generated cross-origin redirect response - assert_equals: The response type of response must match. URL: https://web-platform.test:8443/service-workers/service-worker/dumm y?url=https%3A%2F%2Fwww1.web-platform.test%3A8443%2Fservice-workers%2Fservice-worker%2Fresources%2Fs imple-cors.txt%3F&original-redirect-mode=follow&sw=gen expected "opaque" but got "basic"

Comment on attachment 8984570 [details] [diff] [review] Align LoadInfo::mServiceWorkerTaintingSynthesized handling with other service worker fields. r=valentin This patch fixes a severe security issue in service worker interception code. As described in comment 0, we have some code in the LoadInfo which tracks the tainting level of a response. For example, normally when a no-cors request crosses an origin it becomes opaque tainted forever more. Service workers, however, have the ability to response with a response with a different tainting which should override. For example, the service worker could intercept that cross-origin no-cors request and instead synthesize a basic same-origin response. This is all well and good, but we have an added flag which pins the tainting at that service worker specified value. We need this because we trigger an extra redirect in order to communicate the Response.url that the service worker synthesized. Without that flag to pin the tainting value our security code ends up changing it again on this redirect. The bug is when the service worker synthesizes a basic same-origin response that has a 30x status code. In these cases we follow the redirect after the service worker interception. Now that new redirect channel should be allowed to be tainted again. Unfortunately we never clear the flag that pins the tainting value after service worker interception. So any real redirect after a service worker interception are pinned at the service workers original tainting. This means an evil site can arrange to read cross-origin no-cors credentials requests (very bad). This patch fixes by doing a few things: 1. Don't automatically copy the mServiceWorkerTaintingSynthesized flag in the LoadInfo copy constructor. Instead initialize to false. 2. Make the e10s and non-e10s channel implementation manually copy the flag value across to the new channel only for those special service worker redirects. 3. Expose SynthesizeServiceWorkerTainting() on nsILoadInfo in order to better support step (2). This fixes the WPT tests posted in this bug.

This bug allows a sites with a service worker to read cross-origin credentialed no-cors resources. This seems bad enough that it might be worth a chemspill.

Comment on attachment 8984570 [details] [diff] [review] Align LoadInfo::mServiceWorkerTaintingSynthesized handling with other service worker fields. r=valentin Review of attachment 8984570 [details] [diff] [review]: ----------------------------------------------------------------- Good catch.

Comment on attachment 8984570 [details] [diff] [review] Align LoadInfo::mServiceWorkerTaintingSynthesized handling with other service worker fields. r=valentin [Security approval request comment] How easily could an exploit be constructed based on the patch? The patch is clearly about the tainting of responses returned from service workers and how we handle that in regards to redirects. I don't think it makes the issue of incorrectly freezing the tainting obvious, but someone could probably figure that out if the inspected the code. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? I have a WPT test written, but I will land it separately later. The comments do not discuss the issue of keeping the tainting freezing too long. Which older supported branches are affected by this flaw? Beta and release channel. ESR has the code, but service workers are disabled. If not all supported branches, which bug introduced the flaw? This was introduced in bug 1222008. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? I believe this should backport cleanly, but I have not tried that yet. How likely is this patch to cause regressions; how much testing does it need? We have pretty good test coverage for this feature, although it did miss the issue in comment 0. I believe the risk of regressions is low, but it is a complex interaction between service workers, networking, and redirects. Its possible that a regression could occur. Approval Request Comment [Feature/Bug causing the regression]: Bug 1222008 [User impact if declined]: A site with a service worker can read cross-origin credentialed resources without CORS. So evil.com can read a credentialed bank.com page, etc. [Is this code covered by automated tests?]: I have a WPT test written that triggers this, but I will land it separately. [Has the fix been verified in Nightly?]: No, due to security issue it has not landed yet. [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: Moderate [Why is the change risky/not risky?]: The code change is relatively small, but the interaction between networking, service workers, and redirects is complex. There is some risk there is another corner case that we missed and also don't have a test for. [String changes made/needed]: None

Try is green, btw.

(In reply to Ben Kelly [:bkelly] from comment #7) > Which older supported branches are affected by this flaw? > > Beta and release channel. ESR has the code, but service workers are > disabled. That's only true in ESR52, right? ESR60 is affected by this, isn't it? sec-approval+ and I'll approve beta.

No, esr 60 has service workers disabled as well. I'll wait for the release flag to be resolved before landing.

I would rather we not delay getting this into Nightly and hopefully tomorrow's b13 build if at all possible. It would be better if we had a least a little bit of Nightly/Beta bake time before making any chemspill decisions for 60 anyway.

Ok, I will work on landing it tonight. I just thought we tried to do simultaneous landings for sec bugs.

https://hg.mozilla.org/integration/mozilla-inbound/rev/29598a3b4cc09aef566178a33b3c5d7dbd36da17 https://hg.mozilla.org/releases/mozilla-beta/rev/44883c246d62061847e07dcba4fd88f55eb45362

Oh, and I see now that release channel was marked wontfix in comment 9. I missed that reading the mail on my phone.

(In reply to Ben Kelly [:bkelly] from comment #14) > Oh, and I see now that release channel was marked wontfix in comment 9. I > missed that reading the mail on my phone. Release management can overrule me but I see no discussion of chemspilling over this. If this is a public 0day or we think this is so serious that we *must* release it early, we can reconsider landing it on release.

At what point would be people be comfortable with me landing the WPT tests? I assume after FF61 merges to release, but how long after?

We usually try to wait until a couple weeks after release IIRC. NI Al to confirm.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #18) > We usually try to wait until a couple weeks after release IIRC. NI Al to > confirm. Yes, after we ship the fix in a release, we wait about four weeks to make sure we've fixed most of our users who are vulnerable. USe the in-testsuite? flag to track, which Ryan set.

Ben, since this needs a QE verification, can you please provide some specific steps in order for me to test this fix properly? Thank you!

(In reply to Anca Soncutean [:Anca], Desktop Release QA from comment #20) > Ben, since this needs a QE verification, can you please provide some > specific steps in order for me to test this fix properly? Thank you! I don't have an easy way to reproduce this in the wild currently. The WPT tests I attached in this bug exercise the problem, though.

After further discussions, we concluded to remove qe-verify+ flag, since there are no clear steps on how to test manually this issue.

Comment on attachment 8984570 [details] [diff] [review] Align LoadInfo::mServiceWorkerTaintingSynthesized handling with other service worker fields. r=valentin per earlier comments this is wontfix for 60

sec-high is the correct rating according to https://wiki.mozilla.org/Security_Severity_Ratings