Closed Bug 1111834 (CVE-2015-0807) Opened 5 years ago Closed 5 years ago

CORS request after preflight should not follow 30x redirect

Categories

(Core :: DOM: Security, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox35 --- wontfix
firefox36 + wontfix
firefox37 + fixed
firefox38 + fixed
firefox39 --- fixed
firefox-esr31 37+ fixed
b2g-v1.4 --- fixed
b2g-v2.0 --- fixed
b2g-v2.0M --- fixed
b2g-v2.1 --- fixed
b2g-v2.1S --- fixed
b2g-v2.2 --- fixed
b2g-master --- fixed

People

(Reporter: ckerschb, Assigned: ckerschb)

References

Details

(Keywords: sec-high, Whiteboard: [adv-main37+][adv-esr31.6+])

Attachments

(2 files, 3 obsolete files)

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
Depends on: CVE-2014-8638
Since Bug 1080987 was marked sec-high, I am also going to mark this one as sec-high.
Keywords: sec-high
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Flags: sec-bounty?
Adding a bounty request from bug 1080987 since this one is higher rated and forked from it. Bounty would be for Muneaki Nishimura.
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.
(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.
Flags: needinfo?(mozilla)
Group: dom-core-security
Attached patch bug_1111834_cors_redirect.patch (obsolete) — Splinter Review
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?
Attachment #8560166 - Flags: review?(jonas)
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?
Thanks for the hint - that makes the fix for this patch way easier.
Attachment #8552588 - Attachment is obsolete: true
Attachment #8560166 - Attachment is obsolete: true
Attachment #8560166 - Flags: review?(jonas)
Attachment #8566841 - Flags: review?(jonas)
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.
Attachment #8566842 - Flags: review?(jonas)
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.
Attachment #8566841 - Flags: sec-approval?
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
Attachment #8566841 - Flags: sec-approval? → sec-approval+
Whiteboard: Please wait until after merge to land, e.g after Feb 24
This can land now.
Flags: needinfo?(mozilla)
https://hg.mozilla.org/mozilla-central/rev/ac621df2174a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Christoph - This impacts Firefox 37, 38, and ESR 31. Can you please request uplift for these channels?
Flags: needinfo?(mozilla)
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
Flags: needinfo?(mozilla)
Attachment #8566841 - Flags: approval-mozilla-esr31?
Attachment #8566841 - Flags: approval-mozilla-beta?
Attachment #8566841 - Flags: approval-mozilla-aurora?
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?
Flags: needinfo?(mozilla)
Attachment #8566841 - Flags: approval-mozilla-esr31?
Attachment #8566841 - Flags: approval-mozilla-esr31+
Attachment #8566841 - Flags: approval-mozilla-beta?
Attachment #8566841 - Flags: approval-mozilla-beta+
Attachment #8566841 - Flags: approval-mozilla-aurora?
Attachment #8566841 - Flags: approval-mozilla-aurora+
(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?
Flags: needinfo?(mozilla) → needinfo?(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?
Flags: sec-bounty?
Flags: sec-bounty+
Flags: needinfo?(dveditz)
No. I just wasn't aware of that policy for sec-high and sec-crit tests.
Depends on: 1140431
Whiteboard: [adv-main37+][adv-esr31.6+]
Alias: CVE-2015-0807
Group: dom-core-security
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!
Attachment #8566842 - Attachment is obsolete: true
Flags: needinfo?(dveditz)
Attachment #8630551 - Flags: review+
Sure, we could land this toward the end of the week
Flags: needinfo?(dveditz)
Flags: in-testsuite? → in-testsuite+
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.