+++ This bug was initially created as a clone of Bug #1173811 +++ We are not propagating CORS tainting across service worker interceptions. This is mainly just leaks headers on CORS responses to content script. Per the spec, we should only allow these headers to be read: `Cache-Control` `Content-Language` `Content-Type` `Expires` `Last-Modified` `Pragma` Or those headers specified in Access-Control-Expose-Headers by the server. Its not possible to read cookies since even basic responses filter those headers out. I'm setting this as sec-high, but maybe it should be lower since its just leaking header information from the cross-origin server. I'm writing some tests in bug 1212669 that show this problem. For now I am going to disable the new test checks in test_fetch_cors.js if the SW is present, but those checks should be re-enabled when this is fixed. Marking as v1 for now. It would be nice to punt this to v3 as I'm not sure how we will fix it without real tainting in the channel.

To fix this we probably want to do: 1) Add a tainting enumeration to the nsIHttpChannelInternal. 2) Make FetchDriver check the tainting flag and set its own tainting to match. 3) Make XHR check the tainting flag for CORS tainting when exposing headers. We won't try to make things check for the opaque tainting yet, but instead continue with the redirect plan in bug 1173811.

I guess I will take this as the tainting piece may overlap with my work in bug 1201160.

Depends on tests in bug 1212669. Still to do: 1) Figure out why I'm hitting an assert on unknown response type. 2) Add an xhr cors tainting test.

I think it would be good to put the tainting information in nsILoadInfo. That way callers don't have to worry about knowing if the channel is a http://-channel or a data:-channel.

Fix the assertion bug. I will likely refactor a bit here and elsewhere.

This test change was originally reviewed as part of a larger patch in bug 1212669. I'm including it here to avoid uplifting that other sec bug.

My patches here actually fix the opaque tainting issue for intercepted fetch() covered in bug 1173811. This matters because some of the wpt tests need to be fixed to account for that. Bug 1173811 also fixes opaque tainting for all other uses, though, so we still need it. Depend on bug 1173811 for those wpt test fixes, though.

Comment on attachment 8673894 [details] [diff] [review] P4 Set channel tainting in FetchEvent.respondWith(). r=ehsan Review of attachment 8673894 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/fetch/InternalResponse.cpp @@ +120,5 @@ > > +Tainting > +InternalResponse::GetTainting() const > +{ > + if (mType == ResponseType::Cors) { Uber-nit: do you mind reworking this using a switch statement please? That would be a bit easier to read. ::: dom/workers/ServiceWorkerEvents.cpp @@ +171,3 @@ > { > AssertIsOnMainThread(); > + MOZ_ASSERT(aLoadInfo); I checked with Christoph. In the odd case of an add-on implementing an http protocol handler, the load info may in fact be null. So please do a null check here instead to avoid a crash in that case. (Note that there is ongoing work to guarantee that loadinfo always exists on HTTP channels but that hasn't landed yet, and I don't think it will be backported to 43 in case we uplift this patch.)

Comment on attachment 8673895 [details] [diff] [review] P5 Make XHR respect channel tainting. r=ehsan Review of attachment 8673895 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine to me, but I would like Jonas to double check it as well. Thanks!

(In reply to Ehsan Akhgari (don't ask for review please) from comment #19) > ::: dom/workers/ServiceWorkerEvents.cpp > @@ +171,3 @@ > > { > > AssertIsOnMainThread(); > > + MOZ_ASSERT(aLoadInfo); > > I checked with Christoph. In the odd case of an add-on implementing an http > protocol handler, the load info may in fact be null. So please do a null > check here instead to avoid a crash in that case. > > (Note that there is ongoing work to guarantee that loadinfo always exists on > HTTP channels but that hasn't landed yet, and I don't think it will be > backported to 43 in case we uplift this patch.) We won't intercept a channel without LoadInfo: https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#2315

Comment on attachment 8673891 [details] [diff] [review] P1 Add a Tainting enumeration. r=jduell Review of attachment 8673891 [details] [diff] [review]: ----------------------------------------------------------------- Good but I'd suggest a name/filename change. I'll leave the bikeshedding to you--no need for re-review. ::: netwerk/base/Tainting.h @@ +30,5 @@ > +// was initiated through fetch() or when a service worker interception occurs. > +// In the future we should set the tainting value within necko so that it is > +// consistently applied. Once that is done consumers can replace checks against > +// the final URL and CORS mode with checks against tainting. > +enum class Tainting : uint8_t Given how much different technology browsers incorporate over time, I think it's slightly rash to grab "mozilla::Tainting" for CORS tainting (surely some other concept of "tainting" is likely to come along in some other part of the browser some day?). So I'd suggest "CorsTainting" or something like that.

Comment on attachment 8673892 [details] [diff] [review] P2 Add tainting information to nsILoadInfo. r=jduell Review of attachment 8673892 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/base/nsILoadInfo.idl @@ +433,5 @@ > + * Determine the associated channel's current tainting. Note, this can > + * change due to a service worker intercept, so it should be checked after > + * OnStartRequest() fires. > + */ > + readonly attribute unsigned long tainting; Is there any chance we'll ever want to be able to distinguish "tainting unset/unavailable" from BASIC? We could have flag==0 mean "unset" if so.

(In reply to Jason Duell [:jduell] (needinfo? me) from comment #23) > Is there any chance we'll ever want to be able to distinguish "tainting > unset/unavailable" from BASIC? We could have flag==0 mean "unset" if so. I think this would add a lot of error handling code all over the place for a value which is really conceptually "default to Basic". I'd prefer to just default to basic and save that extra boilerplate. I can't think of case where we need an "unset" value. And the spec clearly defaults to basic: https://fetch.spec.whatwg.org/#concept-request-response-tainting

(In reply to Ben Kelly [:bkelly] from comment #21) > (In reply to Ehsan Akhgari (don't ask for review please) from comment #19) > > ::: dom/workers/ServiceWorkerEvents.cpp > > @@ +171,3 @@ > > > { > > > AssertIsOnMainThread(); > > > + MOZ_ASSERT(aLoadInfo); > > > > I checked with Christoph. In the odd case of an add-on implementing an http > > protocol handler, the load info may in fact be null. So please do a null > > check here instead to avoid a crash in that case. > > > > (Note that there is ongoing work to guarantee that loadinfo always exists on > > HTTP channels but that hasn't landed yet, and I don't think it will be > > backported to 43 in case we uplift this patch.) > > We won't intercept a channel without LoadInfo: > > https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/ > HttpBaseChannel.cpp#2315 Good point! So you should get rid of the error checking code in FetchDriver::OnStartRequest in part 3.

(In reply to Ben Kelly [:bkelly] from comment #24) > (In reply to Jason Duell [:jduell] (needinfo? me) from comment #23) > > Is there any chance we'll ever want to be able to distinguish "tainting > > unset/unavailable" from BASIC? We could have flag==0 mean "unset" if so. > > I think this would add a lot of error handling code all over the place for a > value which is really conceptually "default to Basic". I'd prefer to just > default to basic and save that extra boilerplate. I can't think of case > where we need an "unset" value. And the spec clearly defaults to basic: > > https://fetch.spec.whatwg.org/#concept-request-response-tainting I also think defaulting to basic makes sense.

> Good point! So you should get rid of the error checking code in > FetchDriver::OnStartRequest in part 3. Well, I can change that to already_AddRefed() version of GetLoadInfo(), but I would like to keep the null check. One day I plan to make necko set the tainting for all channels. So I don't want FetchDriver to bake in the "only intercept" can taint assumption. The ServiceWorkerEvents.cpp assert is part of interception directly.

Comment on attachment 8673895 [details] [diff] [review] P5 Make XHR respect channel tainting. r=ehsan Review of attachment 8673895 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/nsXMLHttpRequest.cpp @@ +1563,1 @@ > return NS_OK; This will break for jar: channels, won't it? Since we support SW interception for jar channels the tainting might end up being != Basic. However the code further down will bail with an error if it's not a http channel.

Ok, this simplifies the XHR tainting check and avoids any non-http concerns. I moved the tainting checkout out of CheckChannelForCrossSiteRequest() since that method is really about checking the request before touching the network, while tainting is an after-network sort of thing. The XHR tainting test I added in P7 still passes.

Comment on attachment 8675124 [details] [diff] [review] P5 Make XHR respect channel tainting. r=ehsan r=sicking Review of attachment 8675124 [details] [diff] [review]: ----------------------------------------------------------------- Looks great. Longer term we can hopefully get rid of the XML_HTTP_REQUEST_USE_XSITE_AC flag and have XHR just look at the taining flag in the loadinfo.

Since I'm formalizing the tainting here I think we should stop abusing it for the opaqueredirect case. The spec doesn't use tainting for that and it can easily be done as a FetchDriver-specific flag. At this point I don't think it will ever migrate into necko itself.

After talking with Jonas, I decided to go with LoadTainting to make LoadInfo, LoadGroup, etc. Also removes OpaqueRedirect and makes the numerical enum values more explicit.

I will finish rebasing and addressing action items on remaining patches tomorrow.

Comment on attachment 8676554 [details] [diff] [review] P0 Do not abuse fetch response tainting to create opaqueredirect responses. r=ehsan [Security approval request comment] How easily could an exploit be constructed based on the patch? It would be quite hard to actually craft an exploit solely from this. It merely reveals some headers on CORS tainted responses. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? Yes, but its low impact. Which older supported branches are affected by this flaw? Service workers are enabled on aurora, so it impacts 43 as well. It may not be worth uplifting, however, given the merge is about 2 weeks away. If not all supported branches, which bug introduced the flaw? Since service workers were first added. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? I have not attempted a backport yet. Its probably doable with some rebasing. How likely is this patch to cause regressions; how much testing does it need? This only affects service workers. Minimal risk.

Comment on attachment 8676559 [details] [diff] [review] P1 Add a LoadTainting enumeration. r=jduell See comment 40.

Comment on attachment 8676573 [details] [diff] [review] P2 Add LoadTainting information to nsILoadInfo. r=jduell See comment 40.

Comment on attachment 8676577 [details] [diff] [review] P3 Make FetchDriver look for the channel Tainting value. r=ehsan See comment 40.

Comment on attachment 8676583 [details] [diff] [review] P4 Set channel tainting in FetchEvent.respondWith(). r=ehsan See comment 40.

Comment on attachment 8677076 [details] [diff] [review] P5 Make XHR respect channel tainting. r=ehsan r=sicking See comment 40.

Comment on attachment 8672621 [details] [diff] [review] P6 Check for CORS response type in mochitests. r=sicking See comment 40.

Comment on attachment 8673897 [details] [diff] [review] P7 Verify xhr respects service worker CORS tainting. r=ehsan See comment 40.

Comment on attachment 8676554 [details] [diff] [review] P0 Do not abuse fetch response tainting to create opaqueredirect responses. r=ehsan sec-approval+ for trunk. You don't need to ask for every patch though, just once normally. Does this affect Beta or released 41 (or ESR38)?

Comment on attachment 8672621 [details] [diff] [review] P6 Check for CORS response type in mochitests. r=sicking If this affects a shipped version, tests should not go in until we publicly ship a fix so we don't 0 day ourselves.

(In reply to Al Billings [:abillings] from comment #49) > sec-approval+ for trunk. You don't need to ask for every patch though, just > once normally. Well, dveditz specifically asked me to do it this way on every patch. :-) > Does this affect Beta or released 41 (or ESR38)? No. Service workers are disabled by default on all release branches.

(In reply to Ben Kelly [:bkelly] from comment #51) > Well, dveditz specifically asked me to do it this way on every patch. :-) At least on that one bug where it wasn't clear if you were asking to check in tests for a chemspill vuln. If there are multiple patches on a bug the sec-approval request needs to be clear one way or another. The other way is to make it clear in the request whether the tests are going to be checked in, or how you are going to remember to check them in if you don't plan to, and which of the multiple patches apply to which branches (if any are relevant). Clarity is the key, ambiguity leads to mistakes.

There was an xpcshell test failure in bug 1173811. New try with patch to fix that: https://treeherder.mozilla.org/#/jobs?repo=try&revision=696faf93484c I need bug 1173811 to stick before I can land this patch.

Update nsILoadInfo uuid.

remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/6b7feb3ce230 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/342a49e40ecf remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/51686c6d9cf4 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/b229b58995ff remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/9ba30f8e195b remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/24afee5857ea remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1dfd5116b0e2 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/78d977bb4596

https://hg.mozilla.org/mozilla-central/rev/6b7feb3ce230 https://hg.mozilla.org/mozilla-central/rev/342a49e40ecf https://hg.mozilla.org/mozilla-central/rev/51686c6d9cf4 https://hg.mozilla.org/mozilla-central/rev/b229b58995ff https://hg.mozilla.org/mozilla-central/rev/9ba30f8e195b https://hg.mozilla.org/mozilla-central/rev/24afee5857ea https://hg.mozilla.org/mozilla-central/rev/1dfd5116b0e2 https://hg.mozilla.org/mozilla-central/rev/78d977bb4596

Does this still affect 43? Maybe not since we disabled some service workers stuff there.

It does not affect 43.