1212904 - FetchEvent.respondWith() should propagate CORS tainting

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add a Tainting enumeration. r=ehsan

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Store Tainting information on http channels. r=jduell

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make FetchDriver look for the channel Tainting value. r=ehsan

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Set channel tainting in FetchEvent.respondWith(). r=ehsan

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Make XHR respect channel tainting. r=ehsan

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Test for CORS response tainting in fetch mochitests. r=ehsan

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.

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Set channel tainting in FetchEvent.respondWith(). r=ehsan

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

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P6 Check for CORS response type in mochitests. r=sicking

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.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

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 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add a Tainting enumeration. r=jduell

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add tainting information to nsILoadInfo. r=jduell

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make FetchDriver look for the channel Tainting value. r=ehsan

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Set channel tainting in FetchEvent.respondWith(). r=ehsan

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Make XHR respect channel tainting. r=ehsan

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P7 Verify xhr respects service worker CORS tainting. r=ehsan

Comment 19

[(no longer active)]

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 20

[(no longer active)]

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!

Comment 21

[Ben Kelly [:bkelly, not reviewing]]

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

[Jason Duell]

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 23

[Jason Duell]

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.

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

(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

Comment 25

[(no longer active)]

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

Comment 26

[(no longer active)]

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

Comment 27

[Ben Kelly [:bkelly, not reviewing]]

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

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Make XHR respect channel tainting. r=ehsan r=sicking

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 30

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 31

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P0 Do not abuse fetch response tainting to create opaqueredirect responses. r=ehsan

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.

Comment 32

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Add a LoadTainting enumeration. r=jduell

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.

Comment 33

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add LoadTainting information to nsILoadInfo. r=jduell

Comment 34

[Ben Kelly [:bkelly, not reviewing]]

Attachment: bug1212904_p2_loadinfotaint.patch

Comment 35

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add LoadTainting information to nsILoadInfo. r=jduell

Comment 36

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Make FetchDriver look for the channel Tainting value. r=ehsan

Comment 37

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Set channel tainting in FetchEvent.respondWith(). r=ehsan

Comment 38

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 39

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P5 Make XHR respect channel tainting. r=ehsan r=sicking

Comment 40

[Ben Kelly [:bkelly, not reviewing]]

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 41

[Ben Kelly [:bkelly, not reviewing]]

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

See comment 40.

Comment 42

[Ben Kelly [:bkelly, not reviewing]]

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

See comment 40.

Comment 43

[Ben Kelly [:bkelly, not reviewing]]

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

See comment 40.

Comment 44

[Ben Kelly [:bkelly, not reviewing]]

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

See comment 40.

Comment 45

[Ben Kelly [:bkelly, not reviewing]]

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

See comment 40.

Comment 46

[Ben Kelly [:bkelly, not reviewing]]

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

See comment 40.

Comment 47

[Ben Kelly [:bkelly, not reviewing]]

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

See comment 40.

Comment 48

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cbadf914367

Comment 49

[Al Billings [:abillings - ex-MoCo]]

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 50

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 51

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 52

[Daniel Veditz [:dveditz]]

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

Comment 53

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 54

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Add LoadTainting information to nsILoadInfo. r=jduell

Update nsILoadInfo uuid.

Comment 55

[Ben Kelly [:bkelly, not reviewing]]

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

Comment 56

[Carsten Book [:Tomcat]]

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

Comment 57

[Liz Henry (:lizzard) (relman/hg->git project)]

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

Comment 58

[Ben Kelly [:bkelly, not reviewing]]

It does not affect 43.