Closed Bug 1080987 (CVE-2014-8638) Opened 10 years ago Closed 9 years ago

navigator.sendBeacon() doesn't satisfy CORS specification

Categories

(Core :: DOM: Core & HTML, defect)

35 Branch
x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla37
Tracking Status
firefox34 --- wontfix
firefox35 + fixed
firefox36 + fixed
firefox37 + fixed
firefox-esr31 35+ fixed
b2g-v1.4 --- wontfix
b2g-v2.0 --- wontfix
b2g-v2.0M --- wontfix
b2g-v2.1 --- fixed
b2g-v2.2 --- fixed

People

(Reporter: sdna.muneaki.nishimura, Assigned: ckerschb)

References

Details

(Keywords: sec-moderate, Whiteboard: [reporter-external][adv-main35+][adv-esr31.4+][b2g-adv-main2.2+])

Attachments

(2 files, 6 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/37.0.2062.124 Safari/537.36

Steps to reproduce:

Suppose alice.csrf.jp is a victim and mallory.csrf.jp is the attacker in following scenario.

1) As a preparation, set victim's cookie by accessing following URL.
http://alice.csrf.jp/cookie/set.php?domain=alice.csrf.jp&name=victim&value=secret

2) Launch attacker's page. Then, cross-origin beacon is invoked to the victim, i.e., alice.csrf.jp.
http://mallory.csrf.jp/beacon/

3) Analyze the HTTP request data sent to the victim via navigator.sendBeacon().


Actual results:

Request data sent from navigator.sendBeacon() has no Origin header.
Also, the request data contains victim's cookie automatically.



Expected results:

W3C's Beacon spec, http://www.w3.org/TR/beacon/, says Beacon request needs to be initialized as:
 - force Origin header flag : Set
 - credentials mode : omit.
 So, the request header should contain Origin and should NOT contain Cookie by default.
The latest editors draft says "credentials mode: include" (http://w3c.github.io/beacon/), but I confirm that I don't see the CORS Origin header in your example urls.

The request to alice.csrf.jp has content-type multipart/form-data which is defined as a "simple request". That shouldn't require a pre-flight even with an Origin header. In a simple request it's OK to send the cookies in the request, you're just not allowed to let the page read the returned response if the server doesn't send the allow-credentials header. But of course with sendBeacon() you don't get to read the response anyway.

I see in the implementation bug (bug 936340) there was discussion about doing CORS and setting up tests for it, but I don't know what they're testing. Presumably you'd need the full preflight CORS if the beacon data had a non-simple content-type (I think you could get arbitrary types using blobs).

Will need more exploration: if we're just not setting the Origin header for simple requests then it looks like we're violating the Beacon spec, but not really doing anything dangerous since a non-beacon page could generate the same requests. If we're not doing CORS at all even for non-simple requests then we've added a serious CSRF hazard.
Assignee: nobody → rlb
Status: UNCONFIRMED → NEW
Component: Untriaged → DOM
Ever confirmed: true
Product: Firefox → Core
In case of non-simple request, Origin is set in pre-flight but NOT set in successive POST request.

I made a example page invoking a non-simple beacon request with ArrayBuffer.
http://mallory.csrf.jp/beacon/index2.html
I found one more CORS violation which allows to bypass pre-flight for non-simple Beacon request.

Steps to reproduce:

1) Launch following attacker's page that invokes non-simple cross-origin Beacon with redirect.
http://mallory.csrf.jp/beacon/index3.html

Then, Beacon's target URL works like this:
- If request method is "OPTIONS", return valid pre-flight response headers
- If request method is "POST", redirect to victim's page, alice.csrf.jp, with status 308.

2) Analyze the HTTP request data sent to the victim via navigator.sendBeacon().

Actual results:

Non-simple cross-origin Beacon can be sent to victim with bypassing pre-flight.
Then, the Beacon request has no Origin header, and if attacker's redirector sets "Access-Control-Allow-Credentials:true" for pre-flight, the victim's cookie can also be sent.

Expected results:

Firefox has NOT to follow 30x redirect for actual request in case of cross-origin request with pre-flight.
This is explicitly prohibited by CORS specification.
http://www.w3.org/TR/cors/#cross-origin-request-with-preflight-0

"3. This is the actual request. Apply the make a request steps and observe the request rules below while making the request. If the response has an HTTP status code of 301, 302, 303, 307, or 308
Apply the cache and network error steps."

From the victim's perspective, when the request comes without Origin header from UA, then, the victim may handle it as a same-origin Beacon request because W3C's Beacon spec doesn't require to set Origin for same-origin request. I think current impl. of Firefox has potential of CSRF.
Flags: sec-bounty?
Whiteboard: [reporter-external]
This maybe belongs to sworkman now?
Flags: needinfo?(sworkman)
I think richard implemented sendBeacon.  Richard?
Flags: needinfo?(rlb)
Clearing my needinfo until Richard responds.
Flags: needinfo?(sworkman)
This looks like a general issue with our CORS implementation.  Beacon just calls through to NS_StartCORSPreflight in cases where a CORS preflight is needed.  So the malicious server in the example would trip up any CORS request, not just a beacon.

So it seems like this falls within Steve's scope.
Assignee: rlb → nobody
Flags: needinfo?(rlb) → needinfo?(sworkman)
Christoph is going to look into it.
Flags: needinfo?(sworkman)
Assignee: nobody → mozilla
Keywords: sec-high
It seems, there are multiple problems described in that bug, lets start with the one in comment 0:

Here are the intetresting values before deciding whether we need a cors preflight in SendBeacon() or not:
> http://mxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp#1187

> documentURI: http://mallory.csrf.jp/beacon/
> uri: http://alice.csrf.jp/csp/log.php
> crossOrigin: yes
> contentType: multipart/form-data

In which case the contentType of 'multipart/form-data' indicates a simple request and hence doesn't require a CORS preflight, which seems correct to me. Using CORS, we do set the origin-header here:
> http://mxr.mozilla.org/mozilla-central/source/dom/base/nsCrossSiteListenerProxy.cpp#806

Nevertheless, we are simply calling AsyncOpen in the non-CORS case:
http://mxr.mozilla.org/mozilla-central/source/dom/base/Navigator.cpp#1207
without setting the origin-header.

Probably our implementation predates the concept of the 'force-origin-header flag'.
Note, force-origin-header flag is concept of fetch-spec: https://fetch.spec.whatwg.org/#force-origin-header-flag, not CORS.

Richard, Jonas, what is the best way of fixing that?
Flags: needinfo?(rlb)
Flags: needinfo?(jonas)
As far as I can tell looking at the code we're doing preflights as needed already.

And the fact that we are sending cookies are per spec and not any less safe than <form> has been for almost two decades.

The only thing that sounds wrong is that we're not setting an Origin header. I think you need to make sure to call nsCORSListenerProxy::Init. Look at how/when the XHR code does that.
Flags: needinfo?(jonas)
Jonas's guidance seems correct to me.  I don't really have anything more to add.
Flags: needinfo?(rlb)
Group: dom-core-security
Jonas, you are right, all we have to do is call ->Init() on the nsCORSListenerProxy. Do you wanna review that?
Attachment #8525665 - Flags: review?(jonas)
Attached patch bug_1080987_tests.patch (obsolete) — Splinter Review
Attachment #8525666 - Flags: review?(jonas)
Comment on attachment 8525666 [details] [diff] [review]
bug_1080987_tests.patch

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

It seems simpler and more robust to write this as a sjs file which checks that the appropriate header is set on the server side. But up to you.
Attachment #8525666 - Flags: review?(jonas) → review+
Actually, I wonder if the current test will work in e10s. Might be worth using an sjs file to make sure that it does.
(In reply to Jonas Sicking (:sicking) from comment #14)
> Comment on attachment 8525666 [details] [diff] [review]
> bug_1080987_tests.patch
> 
> Review of attachment 8525666 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It seems simpler and more robust to write this as a sjs file which checks
> that the appropriate header is set on the server side. But up to you.

But then you have to report back to the testpage that the header was set, right? Which then makes the test complicated again.

Or is there a way to basically do the
> is(header, expectedHeader, ...)
assertion in the *.sjs file?
Attached patch bug_1080987_tests.patch (obsolete) — Splinter Review
Jonas, I don't think there is a less complicated way to write this test. I left a detailed description in the test-code. If you want, we could also use setTimeOut() and wait a certain amount of time after the sendBeacon() before we try to query the result from the *.sjs file. I think the approach in this patch however consumes the least amount of time.
Attachment #8525666 - Attachment is obsolete: true
Attachment #8528137 - Flags: review?(jonas)
Attached patch bug_1080987_tests.patch (obsolete) — Splinter Review
oops - uploaded the patch before my final hq export.
Attachment #8528137 - Attachment is obsolete: true
Attachment #8528137 - Flags: review?(jonas)
Attachment #8528140 - Flags: review?(jonas)
Comment on attachment 8528140 [details] [diff] [review]
bug_1080987_tests.patch

Just chattet with Jonas - apparently we can use setObjectState and getObjectState to clear that test up a little bit. Clearing review until then.
Attachment #8528140 - Flags: review?(jonas)
Attached patch bug_1080987_tests.patch (obsolete) — Splinter Review
Modified the test so we block on the server in case the beacon() request sets the header after the first attempt to query the header using xhr from the server. We try to respond asynchronously in 500 millisecond intervals.
Attachment #8528140 - Attachment is obsolete: true
Attachment #8531957 - Flags: review?(jonas)
Comment on attachment 8531957 [details] [diff] [review]
bug_1080987_tests.patch

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

::: dom/tests/mochitest/beacon/beacon-originheader-handler.sjs
@@ +10,5 @@
> + * header of the beacon request.
> + */
> +
> +// 'timer' is global to avoid getting GCed which would cancel the timer
> +var timer;

Are you sure this works? I don't actually know. An alternative to using a timer would be to use get/setObjectState and store the response. That way you could make handleRequest get back the previous response and finish it if it is pending.

But if you are sure this works, and doesn't fail intermittently, then this is fine too.

@@ +23,5 @@
> +      attemptUnblock(response)}, 500, nsITimer.TYPE_ONE_SHOT);
> +    return;
> +  }
> +  response.setHeader("Cache-Control", "no-cache", false);
> +  response.setHeader("Content-Type", "text/html", false);

Nit: "text/plain" would be more accurate. Or just leave this out.

::: dom/tests/mochitest/beacon/test_beaconOriginHeader.html
@@ +37,5 @@
> +
> +function queryHeaderFromServer() {
> +  var xhr = new XMLHttpRequest();
> +  xhr.open("GET", "beacon-originheader-handler.sjs?unblock", true);
> +  xhr.onreadystatechange = function() {

Use xhr.onload = function() { ...

That way you don't need the readyState check below.

@@ +43,5 @@
> +      return;
> +    }
> +    if (xhr.responseText === ORIGIN_HEADER) {
> +      ok(true, "SendBeacon sends right origin header");
> +      SimpleTest.finish();

Change these three lines to

is(xhr.responseText, ORIGIN_HEADER, "...");
SimpleTest.finish();

@@ +48,5 @@
> +    }
> +  };
> +  xhr.onerror = function() {
> +    ok(false, "xhr request returned error");
> +    SimpleTest.finish();

You don't really need this.

@@ +60,5 @@
> +  formData.append('name', 'value');
> +  navigator.sendBeacon(BEACON_URL, formData);
> +
> +  // start quering the result from the server
> +  SimpleTest.executeSoon(queryHeaderFromServer);

You can run this synchronously. No need to waste time to go through the event loop.
Attachment #8531957 - Flags: review?(jonas) → review+
Attached patch bug_1080987_tests.patch (obsolete) — Splinter Review
(In reply to Jonas Sicking (:sicking) from comment #21)
> Are you sure this works?
I tried various combinations, running the XML first, so the server waits till it sends the respsonse and vice versa. Works fine!

> > +  xhr.onerror = function() {
> You don't really need this.

Agreed, but in case something goes wrong we don't have to wait for the expensive timeout. I think having 3 lines of code is the better alternative here. Would like to keep it rather than deleting it.

Carrying over r+ from Jonas.
Attachment #8531957 - Attachment is obsolete: true
Attachment #8533293 - Flags: review+
Comment on attachment 8525665 [details] [diff] [review]
bug_1080987.patch

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

It's very easy to trigger that incorrect behavior. Please have a look at the first comment, that explains a proof of concept exploitation in detail.

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

Which older supported branches are affected by this flaw?
Navigator.sendBeacon() landed in mozilla30.

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

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?
Given that we only add the origin header to a beacon request, the risk that this causes any regressions is very small because the change doesn't affect anything besides adding that origin header to beacon requests.
Attachment #8525665 - Flags: sec-approval?
At first I'd like to say thank you for fixing the bug swiftly.

Here, there are two wrong things in Beacon:
1. Simple req. and actual req. after preflight doesn't send Origin header.
2. Actual CORS req. after preflight follows 30x redirect (prohibitted by CORS spec).

It seems your patch has fix only for 1. but not 2.
If you fix 2. later after the sec-approval is granted or if your patch covers both 1. and 2., please ignore my message.
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #22)
> Created attachment 8533293 [details] [diff] [review]
> bug_1080987_tests.patch
> 
> (In reply to Jonas Sicking (:sicking) from comment #21)
> > Are you sure this works?
> I tried various combinations, running the XML first, so the server waits
> till it sends the respsonse and vice versa. Works fine!

My concern is if GC happens at the wrong time that the timer will get killed. I *think* that the global will be held alive through the timer closure, but I'm not 100% sure. Note that it's a timing issue, so simply running various combinations isn't going to necessarily trigger GC at the "wrong" time.
(In reply to Jonas Sicking (:sicking) from comment #25)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #22)
> > Created attachment 8533293 [details] [diff] [review]
> > bug_1080987_tests.patch
> > 
> > (In reply to Jonas Sicking (:sicking) from comment #21)
> > > Are you sure this works?
> > I tried various combinations, running the XML first, so the server waits
> > till it sends the respsonse and vice versa. Works fine!
> 
> My concern is if GC happens at the wrong time that the timer will get
> killed. I *think* that the global will be held alive through the timer
> closure, but I'm not 100% sure.

The global should be kept alive, hence my comment on top:
// 'timer' is global to avoid getting GCed which would cancel the timer

> Note that it's a timing issue, so simply
> running various combinations isn't going to necessarily trigger GC at the
> "wrong" time.

I initially thought you are talking about timing issues in terms of what runs first, the beacon() request vs. the xhr request. Anyway, I don't have a problem with rewriting the test. I will do that when I get some time.

But since you are already here, would you like to get the redirect issue from Comment 25 fixed in this bug?
sec-approval+ to check this into trunk on 12/16 or later. Please prepare and nominate patches for affected branches as well (Aurora, Beta, ESR31).
Whiteboard: [reporter-external] → [reporter-external] [checkin 12/16]
Attachment #8525665 - Flags: sec-approval? → sec-approval+
I don't know what "redirect issue" you're referring to?
(In reply to Jonas Sicking (:sicking) from comment #28)
> I don't know what "redirect issue" you're referring to?

See Comment 24.
Ugh! CORS with preflights are clearly still too hard to implement correctly in Gecko. This is why we need to move the implementation into necko and make it be as simple as setting a securityflag when creating the channel. :(

Anyway, what you need to do is to implement the logic that lives in
nsXMLHttpRequest::AsyncOnChannelRedirect. Including the parts that are in CheckChannelForCrossSiteRequest.

But that should definitely be a separate bug.
I filed 'Bug 1111834 - CORS request after preflight should not follow 30x redirect' as a followup to this one where we can fix the redirect-issure discussed in Comment 3 and also Comment 24.
Attached patch bug_1080987_tests.patch (obsolete) — Splinter Review
Jonas, I rewrote the test to not use a timer and instead rely on setObjectState() and getObjectState(). I verified that the tests works correctly no matter which of the two requests hits the server faster.
Attachment #8533293 - Attachment is obsolete: true
Attachment #8536835 - Flags: review?(jonas)
Comment on attachment 8536835 [details] [diff] [review]
bug_1080987_tests.patch

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

Thanks!

::: dom/tests/mochitest/beacon/beacon-originheader-handler.sjs
@@ +13,5 @@
> +  if (request.queryString == "queryheader") {
> +    var header = getState("originHeader");
> +    // if the beacon already stored the header - return.
> +    if (header) {
> +      response.write(header);

call setState("originHeader", ""); here so that if someone runs the test twice, that it still works. Not strictly needed for mochitests, but makes development a whole lot more pleasant.

@@ +32,5 @@
> +  getObjectState("xhr-response", function(xhrResponse) {
> +    if (!xhrResponse) {
> +      return;
> +    }
> +    xhrResponse.write(header);

Same here.
Attachment #8536835 - Flags: review?(jonas) → review+
Whiteboard: [reporter-external] [checkin 12/16] → [reporter-external]
Incorporated Jonas' nit from Comment 33 - Carrying over r+ from Jonas.
Attachment #8536835 - Attachment is obsolete: true
Attachment #8537981 - Flags: review+
Comment on attachment 8525665 [details] [diff] [review]
bug_1080987.patch

Al, given that this bug has been in Firefox for quite some time and also since it's just a missing origin header, I think we could downgrade to sec-medium and we could land the patch together with the tests. Deveditz feels the same way as I do.

Deleted the request info for now, if you want me to, I can fill it out for aurora, beta, esr31.
Flags: needinfo?(abillings)
Attachment #8525665 - Flags: approval-mozilla-esr31?
Attachment #8525665 - Flags: approval-mozilla-beta?
Attachment #8525665 - Flags: approval-mozilla-aurora?
I agree with that you downgrade this bug to sec-moderate since it can only send simple cross origin request same as <form>.

Howeber, as I commented on Comment 24, there are two bugs.
One is here Bug 1080987 and another one was forked as Bug 1111834 by Comment 31.
By combinating both two bugs, an attacker can send non-simple cross origin request with bypassing CORS restriction and I think this scenario meets your sec-high criteria.

If you downgrade this issue, I'm grad if you can take over the sec-bounty flag to Bug 1111834. Thanks.
I'm downgrading the security rating on this one. I've added the bounty request to the other bug.
Flags: needinfo?(abillings)
Keywords: sec-highsec-moderate
Target Milestone: --- → mozilla37
Blocks: 1113449
I'm adding an assertion which would have caught this over in bug 1080987 to make this harder to misuse.
https://hg.mozilla.org/mozilla-central/rev/6f7c93a5f71f
https://hg.mozilla.org/mozilla-central/rev/d90fcdb1eb71

Please request b2g34 approval for v2.1 as well. Sec-moderates generally aren't being taken for v1.4/v2.0 at this point, so I'm setting them to wontfix. If you feel that's in error, feel free to set them to affected and argue your case for approval.
Status: NEW → RESOLVED
Closed: 9 years ago
Flags: needinfo?(mozilla)
Flags: in-testsuite+
Resolution: --- → FIXED
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #40)
> https://hg.mozilla.org/mozilla-central/rev/6f7c93a5f71f
> https://hg.mozilla.org/mozilla-central/rev/d90fcdb1eb71
> 
> Please request b2g34 approval for v2.1 as well. Sec-moderates generally
> aren't being taken for v1.4/v2.0 at this point, so I'm setting them to
> wontfix. If you feel that's in error, feel free to set them to affected and
> argue your case for approval.

Thanks Ryan - but I guess Jonas is in a better position to answer that question.
Flags: needinfo?(jonas)
That's ok with me.
Flags: needinfo?(jonas)
Thanks Jonas - clearing my needinfo as well.
Flags: needinfo?(mozilla)
Comment on attachment 8525665 [details] [diff] [review]
bug_1080987.patch

Hopefully this is the right request to get approval for b2g34 v2.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #):
Implement Navigator sendBeacon in Bug 936340.

User impact if declined:
Beacons are sent without 'origin' header.

Testing completed:
Added a testcase which already landed on inbound.

Risk to taking this patch (and alternatives if risky):
Risk is very low, only adding the origin header to the request.

String or UUID changes made by this patch:
No uuid changes.
Attachment #8525665 - Flags: approval-mozilla-b2g34?
Attachment #8525665 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8525665 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8525665 - Flags: approval-mozilla-b2g34? → approval-mozilla-b2g34+
Flags: sec-bounty?
Group: dom-core-security
Attachment #8525665 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Whiteboard: [reporter-external] → [reporter-external][adv-main35+][adv-esr31.4+]
Alias: CVE-2014-8638
Summary: navigator.sendBeacon() doesn't satisfy CORS spec. → navigator.sendBeacon() doesn't satisfy CORS specification
Whiteboard: [reporter-external][adv-main35+][adv-esr31.4+] → [reporter-external][adv-main35+][adv-esr31.4+][b2g-adv-main2.2+]
Group: core-security → core-security-release
Group: core-security-release
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.