1210413 - anonymous CORS sends cookies to cross-origin redirects in e10s mode

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

While working on bug 1206124 I discovered a problem with our CORS implementation in e10s.  If the first URL visited is same-origin, but that then redirects to a cross-origin URL, cookies will always be sent to the cross-origin server.

This happens because e10s necko channels do not propagate changes to the load flags back from the child to the parent on redirect.  So nsCORSListenerProxy dutifully sets LOAD_ANONYMOUS, but the parent side never sees it.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Test CORS credentials on cross-origin redirects. r=ckerschb

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Propagate new channel load flags from child to parent on redirect. r=mcmanus

Comment 3

[Patrick McManus [:mcmanus]]

Comment on attachment 8668483 [details] [diff] [review]
P1 Propagate new channel load flags from child to parent on redirect. r=mcmanus

Review of attachment 8668483 [details] [diff] [review]:
-----------------------------------------------------------------

jason definitely has the most expertise here.. but the patch makes sense to me

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=77205f538bc0

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

I'm not sure how far back this goes, but I think we should uplift at least to aurora since we're running e10s by default there.

Comment 6

[Jason Duell]

Comment on attachment 8668483 [details] [diff] [review]
P1 Propagate new channel load flags from child to parent on redirect. r=mcmanus

Review of attachment 8668483 [details] [diff] [review]:
-----------------------------------------------------------------

I think we need to be more intelligent about this.  As written this patch will result in LOAD_REPLACE no longer being set on a redirected channel, which is bad.

The airtight solution would be to keep some kind of flag delta (which flags were set/unset on the child in between the initial asyncOpen and redirect verify time) and merge them in.  But I suspect it would be good enough if we just duplicate the flag manipulation logic in HttpBaseChannel::SetupReplacementChannel:

  https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp?from=HttpBaseChannel.cpp#2420

i.e. always set LOAD_REPLACE, unset nsICachingChannel::LOAD_CHECK_OFFLINE_CACHE, and possibly unset INHIBIT_PERSISTENT_CACHING (we could add a bool mRedirectClearedPersistentCaching that we set in SetupReplacementChannel and use it to know if we should clear it again).

Comment 7

[Jason Duell]

Comment on attachment 8668483 [details] [diff] [review]
P1 Propagate new channel load flags from child to parent on redirect. r=mcmanus

Review of attachment 8668483 [details] [diff] [review]:
-----------------------------------------------------------------

It has been pointed out to me that SetupReplacementChannel gets called on both the child/parent, so the flag manipulation I mentioned has already been done.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Propagate new channel load flags from child to parent on redirect. r=jduell

Add an additional check and assertion to verify the overriding flags include LOAD_REPLACE.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8668472 [details] [diff] [review]
P2 Test CORS credentials on cross-origin redirects. r=ckerschb

Review of attachment 8668472 [details] [diff] [review]:
-----------------------------------------------------------------

I would prefer if Jonas can review so we are not missing a corner case within a security bug.

Comment 10

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

Comment on attachment 8668472 [details] [diff] [review]
P2 Test CORS credentials on cross-origin redirects. r=ckerschb

Review of attachment 8668472 [details] [diff] [review]:
-----------------------------------------------------------------

If I understand correctly, the problem is that when all of the following is true

* xhr.withCredentials=false
* request starts as same-origin
* request is redirected to cross-origin

then we end up sending cookies to the redirected URL. Is that the problem?

What we should do is *not* send cookies to the redirected URL. But we should *allow* it to respond with "access-control-allow-origin: *" and not with "Access-Control-Allow-Credentials: true", just like with any other credential-less request. Is that correct?

If so, does this test really test that?

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Jonas Sicking (:sicking) from comment #10)
> If I understand correctly, the problem is that when all of the following is
> true
> 
> * xhr.withCredentials=false
> * request starts as same-origin
> * request is redirected to cross-origin
> 
> then we end up sending cookies to the redirected URL. Is that the problem?

Correct.

> What we should do is *not* send cookies to the redirected URL. But we should
> *allow* it to respond with "access-control-allow-origin: *" and not with
> "Access-Control-Allow-Credentials: true", just like with any other
> credential-less request. Is that correct?

My tests here use "access-control-allow-origin: <specific origin>", but I can add a case for *.

I don't see why the "Access-Control-Allow-Credentials" header value is significant.  The browser shouldn't attempt to send the cookies to a cross-origin server at all when xhr.withCredentials is false.

Are you asking me to write some more cases for xhr.withCredentials=true to verify we only support it when Access-Control-Allow-Credentials is true?  Would this trigger a pre-flight that would fail for a redirect anyway?  It seems a preflight would be necessary to avoid leaking credentials since we can't know the response header value until after we send the cookies.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Actually, I already have a case that verify the redirect cross-origin server can set Access-Control-Allow-Credentials:

           { pass: 1,
             method: "GET",
             hops: [{ server: origin,
                      cookie: escape("a=1"),
                    },
                    { server: origin,
                      cookie: escape("a=1"),
                    },
                    { server: "http://example.com",
                      allowOrigin: origin,
                      allowCred: 1,
                      cookie: escape("a=2"),
                    },
                    ],
             withCred: 1,
           },

And a test that verifies we do not send the cookie if ACAC is not set:

           // expected fail because allow-credentials CORS header is not set
           { pass: 0,
             method: "GET",
             hops: [{ server: origin,
                      cookie: escape("a=1"),
                    },
                    { server: origin,
                      cookie: escape("a=1"),
                    },
                    { server: "http://example.com",
                      allowOrigin: origin,
                      cookie: escape("a=2"),
                    },
                    ],
             withCred: 1,
           },

So I think this is covered in the patch.  Unless I don't understand what you are asking for you.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Jonas Sicking (:sicking) from comment #10)
> What we should do is *not* send cookies to the redirected URL. But we should
> *allow* it to respond with "access-control-allow-origin: *" and not with
> "Access-Control-Allow-Credentials: true", just like with any other
> credential-less request. Is that correct?

Re-reading the question, I think this is asking about with xhr.withCredentials=false.  In which case its covered by this test case in the patch:

           { pass: 1,
             method: "GET",
             hops: [{ server: origin,
                      cookie: escape("a=1"),
                    },
                    { server: origin,
                      cookie: escape("a=1"),
                    },
                    { server: "http://example.com",
                      allowOrigin: origin,
                      noCookie: 1,
                    },
                    ],
             withCred: 0,

Its just using a specific origin instead of *.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8668600 [details] [diff] [review]
P1 Propagate new channel load flags from child to parent on redirect. r=jduell

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

It would be necessary to get a same-origin request to redirect to a cross-origin evil.com site such that credentials are leaked.  This is probably not obvious from this patch since someone reading it would need to infer what LOAD_FLAGS are not being lost in e10s mode.  The would have to do some digging to figure out its LOAD_ANONYMOUS.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

The tests in P2 probably do make it obvious what the problem is.  They are in dom/security/test/cors and deal with redirects+credentials.

I'm not sure how we can obfuscate this reasonably, though.

Which older supported branches are affected by this flaw?

Probably all older branches with e10s enabled.

If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

Backport should be easy.

How likely is this patch to cause regressions; how much testing does it need?

Minimal risk.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Test CORS credentials on cross-origin redirects. r=sicking

Add test cases for redirect to cross-origin URL with Allow-Origin: '*' with and without Allow-Credentials.

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Test CORS credentials on cross-origin redirects. r=sicking

Sorry, loaded wrong patch.  This should be the correct one.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

I'm going to guess this is sec-high since its SOP related and that is what other SOP bugs have used recently.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8668600 [details] [diff] [review]
P1 Propagate new channel load flags from child to parent on redirect. r=jduell

dveditz suggested over IRC that we don't need to worry about obfuscating the tests if we uplift to aurora immediately.  So requesting aurora approval now so I can do a simultaneous landing.

Approval Request Comment
[Feature/regressing bug #]: e10s networking
[User impact if declined]: Sites can trick the browser into leaking user credentials in e10s mode.
[Describe test coverage new/current, TreeHerder]: New mochitests in P2 that verify behavior.
[Risks and why]: Minimal as the fix only requires passing a single integer (load flags) back from child process to parent process.
[String/UUID change made/needed]: None

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 8668600 [details] [diff] [review]
P1 Propagate new channel load flags from child to parent on redirect. r=jduell

[Triage Comment]
sec-approval and aurora a=dveditz

It's OK to check in (and uplift) the tests, too, because this is disabled on the release channels.

Comment 20

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

> I don't see why the "Access-Control-Allow-Credentials" header value is
> significant.  The browser shouldn't attempt to send the cookies to a
> cross-origin server at all when xhr.withCredentials is false.

I might have misunderstood the problem a bit.

There's two code paths in the CORS code. One which handles the cors-checks when we are in a "no credentials" mode, and one which handles cors-checks when we are in the "send credentials" mode.

When xhr.withCredentials=false, we want to do two things.

* We want to not send any cookies.
* We want to make sure to run the "no credentials" code path.

I was thinking that we get both of these wrong. Which wouldn't actually be a security problem but rather just a "broken API" problem.

Maybe what you are saying is that we are just getting the first bullet wrong. That would definitely be a security problem.

Either way, it would probably be good to ensure that we get both these parts right. What I'm saying is that a good way to make sure that we get the second part right is by not sending a "Access-Control-Allow-Credentials" header and make sure that we still pass security checks.

> Would this trigger a pre-flight that would fail for a redirect anyway?

No. The rules for when a preflight happens is unaffected by if cookies are enabled or not.

Comment 21

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

Comment on attachment 8669667 [details] [diff] [review]
P2 Test CORS credentials on cross-origin redirects. r=sicking

Review of attachment 8669667 [details] [diff] [review]:
-----------------------------------------------------------------

This is great!

::: dom/security/test/cors/file_CrossSiteXHR_server.sjs
@@ +23,5 @@
>  
> +  if (query.hop) {
> +    query.hop = parseInt(query.hop, 10);
> +    hops = eval(query.hops);
> +    query.allowOrigin = hops[query.hop-1].allowOrigin;

Nit: It might be nice to add

var hop = hops[query.hop-1];

and then use 'hop' everywhere.

@@ +82,5 @@
>        request.getHeader("Origin"));
>      return;
>    }
>  
> +  if (query.cookie) {

Why this change? As far as I can tell the only change is that it means that

{
  ...
  cookie: "", // or 0, false or undefined
  ...
}

is treated as if the cookie property didn't exist. That doesn't seem important or good?

@@ +100,5 @@
>        }
>      });
>    }
>  
> +  if (query.noCookie && request.hasHeader("Cookie")) {

Same question here.

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Jonas Sicking (:sicking) from comment #21)
> > +  if (query.cookie) {
> 
> Why this change? As far as I can tell the only change is that it means that
> 
> {
>   ...
>   cookie: "", // or 0, false or undefined
>   ...
> }
> 
> is treated as if the cookie property didn't exist. That doesn't seem
> important or good?

I can put this one back to |"cookie" in query|.

> > +  if (query.noCookie && request.hasHeader("Cookie")) {
> 
> Same question here.

This one I needed to change to match how we handle allowCred, etc.  Its because we always write a value in the hop override code here:
 
  query.noCookie = hops[query.hop-1].noCookie;
 
This is consistent with how allowCred, allowOrigin, and allowHeaders is implemented.  So I'm inclined to keep it this way unless you object.

Comment 23

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

That's fine.

Comment 24

[Ben Kelly [:bkelly, not reviewing]]

mozilla-central:
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/acc9ad51de77
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2b5ed74e92fc

mozilla-aurora:
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/314a38a41491
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/c0293e4a0d7d

Comment 25

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Ben Kelly [:bkelly] from comment #24)
> mozilla-central:
> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/acc9ad51de77
> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/2b5ed74e92fc

These were landed on *mozilla-inbound*, not central.  Sorry for any confusion.

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/acc9ad51de77

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2b5ed74e92fc

Comment 28

[Christiane Ruetten [INACTIVE]]

Dan, you set previous b2g releases as affected, yet Ben wasn't sure how far back this goes in comment 5. Did someone test this? That would imply that it goes loooong back.

Was fixed in Firefox 43+, hence marking b2g-master fixed.

Comment 29

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Christiane Ruetten [:cr] from comment #28)
> Dan, you set previous b2g releases as affected, yet Ben wasn't sure how far
> back this goes in comment 5. Did someone test this? That would imply that it
> goes loooong back.

I'm fairly certain this is in every version of b2g, but I have not tested.  AFAIK we do not have a way to provide security updates to those old releases of b2g.

Comment 30

[Daniel Veditz [:dveditz]]

I don't know that anyone tested the old versions of b2g, "affected" set by educated guessing.