1080987 - (CVE-2014-8638) navigator.sendBeacon() doesn't satisfy CORS specification

Status: RESOLVED FIXED

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

Description

[Muneaki Nishimura]

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.

Comment 1

[Daniel Veditz [:dveditz]]

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.

Comment 2

[Muneaki Nishimura]

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

Comment 3

[Muneaki Nishimura]

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.

Comment 4

[Daniel Veditz [:dveditz]]

This maybe belongs to sworkman now?

Comment 5

[Sid Stamm [:geekboy or :sstamm]]

I think richard implemented sendBeacon.  Richard?

Comment 6

[Steve Workman [:sworkman] (INACTIVE)]

Clearing my needinfo until Richard responds.

Comment 7

[Richard Barnes [:rbarnes]]

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.

Comment 8

[Steve Workman [:sworkman] (INACTIVE)]

Christoph is going to look into it.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

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?

Comment 10

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

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.

Comment 11

[Richard Barnes [:rbarnes]]

Jonas's guidance seems correct to me.  I don't really have anything more to add.

Comment 12

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1080987.patch

Jonas, you are right, all we have to do is call ->Init() on the nsCORSListenerProxy. Do you wanna review that?

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1080987_tests.patch

Comment 14

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

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.

Comment 15

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

Actually, I wonder if the current test will work in e10s. Might be worth using an sjs file to make sure that it does.

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1080987_tests.patch

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.

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1080987_tests.patch

oops - uploaded the patch before my final hq export.

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1080987_tests.patch

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.

Comment 21

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

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.

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

Attachment: 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!

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

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 24

[Muneaki Nishimura]

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.

Comment 25

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

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

Comment 26

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 27

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

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

Comment 28

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

I don't know what "redirect issue" you're referring to?

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #28)
> I don't know what "redirect issue" you're referring to?

See Comment 24.

Comment 30

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

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.

Comment 31

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 32

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1080987_tests.patch

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.

Comment 33

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

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.

Comment 34

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1080987_tests.patch

Incorporated Jonas' nit from Comment 33 - Carrying over r+ from Jonas.

Comment 35

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 36

[Muneaki Nishimura]

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.

Comment 37

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

I'm downgrading the security rating on this one. I've added the bounty request to the other bug.

Comment 38

[Christoph Kerschbaumer [:ckerschb]]

 https://hg.mozilla.org/integration/mozilla-inbound/rev/6f7c93a5f71f
 https://hg.mozilla.org/integration/mozilla-inbound/rev/d90fcdb1eb71

Comment 39

[(no longer active)]

I'm adding an assertion which would have caught this over in bug 1080987 to make this harder to misuse.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 41

[Christoph Kerschbaumer [:ckerschb]]

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

Comment 42

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

That's ok with me.

Comment 43

[Christoph Kerschbaumer [:ckerschb]]

Thanks Jonas - clearing my needinfo as well.

Comment 44

[Christoph Kerschbaumer [:ckerschb]]

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.

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ac5d2e7a5855
https://hg.mozilla.org/releases/mozilla-aurora/rev/425e86d0d740

https://hg.mozilla.org/releases/mozilla-beta/rev/865547ed437c
https://hg.mozilla.org/releases/mozilla-beta/rev/7db3b347535a

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/33a453356d75
https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/db859391c06c

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/0f5e3f25e2aa
https://hg.mozilla.org/releases/mozilla-esr31/rev/658e4b467ae5