1111834 - (CVE-2015-0807) CORS request after preflight should not follow 30x redirect

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

Follow up on Bug 1080987. As explained in [1], a CORS reqeust after preflight shouldn't follow 30x redirect, see spec [2].

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1080987#c3
[2] http://www.w3.org/TR/cors/#cross-origin-request-with-preflight-0

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Since Bug 1080987 was marked sec-high, I am also going to mark this one as sec-high.

Comment 2

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

Adding a bounty request from bug 1080987 since this one is higher rated and forked from it. Bounty would be for Muneaki Nishimura.

Comment 3

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

Has there been any traction on this issue? It would be good to get this fixed on trunk and affected branches as a sec-high issue.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: wip_bug_1111834_cors_redirect_tests.patch

(In reply to Al Billings [:abillings] from comment #3)
> Has there been any traction on this issue? It would be good to get this
> fixed on trunk and affected branches as a sec-high issue.

I wrote a testcase last week (adding a WIP patch) so I can reproduce the issue. This week I am super busy with other things, but I put this bug high up in my priority list for next week.

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1111834_cors_redirect.patch

Jonas, I am not completely sure if this is the right way of fixing the problem. Here it goes:
* In case of a preflight, we create a BeaconRedirectSink and call setNotificationCallbacks.
* In case of a redirect, we check if it's a internal redirect or a same origin redirect, otherwise we abort the redirect.

Also, what is a good way of testing that a redirect didn't succeed? I provided a WIP testcase as attachment a while ago - but I need to turn into a valid mochitest - any suggestions?

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Too late for 36...

Comment 7

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

Comment on attachment 8560166 [details] [diff] [review]
bug_1111834_cors_redirect.patch

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

Can you use nsContentUtils::SameOriginChecker instead?

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1111834_cors_redirect_2.patch

Thanks for the hint - that makes the fix for this patch way easier.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1111834_cors_redirect_tests.patch

Unfortunately it's not that trivial to test that a redirect did not succeed. Currently I am waiting 2 seconds before I query the result from the server to make sure the redirect didin't happen. I think that's the best option we have.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8566841 [details] [diff] [review]
bug_1111834_cors_redirect_2.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Quite easily given the instructions in the initial bug report:
https://bugzilla.mozilla.org/show_bug.cgi?id=1080987#c3

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

Which older supported branches are affected by this flaw?
Every branch after navigator.sendBeacon() landned, see
'Implement Navigator sendBeacon in Bug 936340.'

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?

How likely is this patch to cause regressions; how much testing does it need?
It's a one line change, that's why I think it's very unlikely that it's cuasing regressions. Further it only affects Beacon requests that do need a preflight channel.

Comment 11

[Daniel Veditz [:dveditz]]

Comment on attachment 8566841 [details] [diff] [review]
bug_1111834_cors_redirect_2.patch

Since this has been rejected for Fx36 given we have release candidates already, we should wait to land this after the merge. At that point we'll need to get the fix on 
  m-central (39)
  m-aurora (38)
  m-beta (37)
  esr31 (31.6)

a=dveditz for landing after Feb 24

Comment 12

[Daniel Veditz [:dveditz]]

This can land now.

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ac621df2174a

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

Christoph - This impacts Firefox 37, 38, and ESR 31. Can you please request uplift for these channels?

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8566841 [details] [diff] [review]
bug_1111834_cors_redirect_2.patch

> [Approval Request Comment]
> If this is not a sec:{high,crit} bug, please state case for ESR consideration:
> User impact if declined:
As explained in https://bugzilla.mozilla.org/show_bug.cgi?id=1080987#c3, the users cookie might be leaked to an attacker.

> Fix Landed on Version:
39

> Risk to taking this patch (and alternatives if risky): 
The risk is low, because:
* it only affects non simple send beacon requests
* it's a two line code change so sendBeacon does not follow redirects

> String or UUID changes made by this patch: 
no

> Approval Request Comment
> [Feature/regressing bug #]:
Implement Navigator sendBeacon in Bug 936340.

> [User impact if declined]:
described above

> [Describe test coverage new/current, TreeHerder]:
Provided an automatic test but the test hasn't landed on mc yet.

> [Risks and why]:
described above

> [String/UUID change made/needed]:
no

Comment 17

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8566841 [details] [diff] [review]
bug_1111834_cors_redirect_2.patch

Approved for 37 Beta 4, 38, and ESR 31.

What is holding up landing of the test?

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #17)
> Comment on attachment 8566841 [details] [diff] [review]
> bug_1111834_cors_redirect_2.patch
> 
> Approved for 37 Beta 4, 38, and ESR 31.
> 
> What is holding up landing of the test?

Dan, can we land the test for this?

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/992a9b4bb74d
https://hg.mozilla.org/releases/mozilla-beta/rev/46aa1d78bd2e
https://hg.mozilla.org/releases/mozilla-esr31/rev/30cd218c32ff

Comment 20

[Daniel Veditz [:dveditz]]

(In reply to Lawrence Mandel [:lmandel] (use needinfo) from comment #17)
> Comment on attachment 8566841 [details] [diff] [review]
> What is holding up landing of the test?

No Release user of Firefox has this fix and won't for several weeks, and the test shows how to trigger the security bug. We normally hold tests for sec-high and critical bugs until after the release. Do you have some extra concern about this particular bug?

Comment 21

[Lawrence Mandel [:lmandel] (use needinfo)]

No. I just wasn't aware of that policy for sec-high and sec-crit tests.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/46aa1d78bd2e
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/bc0b61e4a36e
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/72fbcc0811e5
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/cbde23a36e3f

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1s/rev/bc0b61e4a36e
https://hg.mozilla.org/releases/mozilla-b2g32_v2_0m/rev/72fbcc0811e5

Comment 24

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1111834_cors_redirect_tests.patch

The patch for this problem landed in 39 but the testcase never landed. Just clearing my needinfo queue and realized we should land it now.

I rebased the patch; carrying over r+ from Jonas.

Dan, I think it's time to land this, right? If so, could you clear your needinfo and set the checkin-needed flag? Otherwise please let me know. Thanks!

Comment 25

[Daniel Veditz [:dveditz]]

Sure, we could land this toward the end of the week

Comment 26

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8630551 [details] [diff] [review]
bug_1111834_cors_redirect_tests.patch

https://hg.mozilla.org/integration/mozilla-inbound/rev/3403c122b6b2

Comment 27

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3403c122b6b2