1207556 - navigator.sendBeacon ignores Access-Control-* headers and 4xx status code in a response of preflight

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Muneaki Nishimura]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.33 Safari/537.36

Steps to reproduce:

1. Send a cross-origin non-simple request by navigator.sendBeacon.
2. In order to reject an unexpected cross-origin request, the server returns
  - 4xx status code (e.g. 400 Bad Request)
  - Access-Control-Allow-Methods: GET (because a method of sendBeacon is always POST)
  - Access-Control-Allow-Origin: http://other-origin
as a response of preflight request.

Live demo is here: http://mallory.csrf.jp/beacon/40x.html


Actual results:

Nightly 44 ignores above 3 blocks, and an actual non-simple request is finally sent to the server.


Expected results:

An actual non-simple request should not be sent.

Note that the latest Firefox stable release (41.0) works correctly but Nightly doesn't.
There may be regression of CORS handling in Nightly (44.0).

Comment 1

[Daniel Veditz [:dveditz]]

The beacon OPTIONS request doesn't even have an Origin: header or any other CORS headers (the subsequent POST does, though). In Nightly.

When I first load the page I see a console warning saying that the cross-origin request was blocked because other-origin.jp is not mallory.csrf.jp. Of course that's after the POST has been made so it's too late, and sendBeacon() doesn't get any data anyway. When reloading the page I don't get that console error.

42 (last dev edition before merge) seems to be working fine, so something we did in Fx43

Comment 2

[Daniel Veditz [:dveditz]]

The only change to the Navigator::SendBeacon() code in that range is bug 1199049. Of course the bug could be elsewhere, but a 14-part "Move CORS preflights to happen inside Necko" bug seems likely enough to check first.

Why is the checkin r=jdm? from the bug it looks like Jonas reviewed this part, and jduell reviewed many of the other parts.
http://hg.mozilla.org/mozilla-central/rev/f3e4bb1f935c

Comment 3

[(no longer active)]

(In reply to Daniel Veditz [:dveditz] from comment #2)
> The only change to the Navigator::SendBeacon() code in that range is bug
> 1199049. Of course the bug could be elsewhere, but a 14-part "Move CORS
> preflights to happen inside Necko" bug seems likely enough to check first.

If this is a recent regression then that is definitely my fault.

> Why is the checkin r=jdm? from the bug it looks like Jonas reviewed this
> part, and jduell reviewed many of the other parts.
> http://hg.mozilla.org/mozilla-central/rev/f3e4bb1f935c

I screwed up the commit message, sorry about that.  Tons of people reviewed the patches on that bug...

Comment 4

[Daniel Veditz [:dveditz]]

It's possible this is ckershb as well: he landed the switch to AsyncOpen2. Since that landed during Fx42 I thought it was safe, but apparently it got backed out of 42 on the Aurora branch, so that's a difference too.

Need to regression-test before and after that bug landed in Fx42 nightlies, or before and after that was backed out on Dev Edition builds.

Comment 5

[(no longer active)]

OK, I know what's going on here.  This bug affects AsyncOpen2 callers that need CORS preflights.

The cause of the bug is that if we have used AsyncOpen, the load info object that we use for the preflight channel has mInitialSecurityCheckDone on it set to true, so by the time that we AsyncOpen2 the preflight channel here <https://dxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsCORSListenerProxy.cpp?offset=0#1380>, our load info object thinks that the CORS checks have already been done, which means that we return early here <https://dxr.mozilla.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp?offset=100#325>, therefore we don't set up an nsCORSListenerProxy for the preflight channel, so we don't perform any actual CORS checks during preflight.

I made a hacky patch that "fixes" this issue, but it's clearly not the kind of fix we want to see here.  I am not sure how best to fix this.  One option would be to create a new load info object and copy the bits that we want from the original channel to it, but that seems error prone.  Another option would be to just never AsyncOpen2() preflight channels, but I think we're trying to get rid of AsyncOpen().  Another option would be to add a new flag to nsILoadInfo to signal that we should do the security checks regardless of what the load info object thinks, but that seems to go against the current philosophy behind these checks as far as I understand.

Christoph, Jonas, what do you guys prefer?

Comment 6

[(no longer active)]

Attachment: Horribe hack!

Comment 7

[(no longer active)]

BTW to state the obvious, we should *not* move XHR or fetch to AsyncOpen2 before this is fixed.  I couldn't find the bug for fetch.

Comment 8

[(no longer active)]

BTW I don't think bug 1199049 is at fault here.  This was broken before we moved CORS preflights to happen inside Necko, as far as I can tell.

Comment 9

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

nsCORSListenerProxy::StartCORSPreflight should create a new LoadInfo object which is a clone of the one living on the "real" channel. I'm not sure if that regressed in bug 1199049, or if we never did.

One thing to be aware of those is that by the time that we get to nsCORSListenerProxy::StartCORSPreflight, the original LoadInfo will already have the mInitialSecurityCheckDone flag set. So we need to make sure that the cloned LoadInfo does not have it set.

Comment 10

[(no longer active)]

(In reply to Jonas Sicking (:sicking) from comment #9)
> nsCORSListenerProxy::StartCORSPreflight should create a new LoadInfo object
> which is a clone of the one living on the "real" channel. I'm not sure if
> that regressed in bug 1199049, or if we never did.

We never did that.

I need to add a clone() method to nsILoadInfo then.

> One thing to be aware of those is that by the time that we get to
> nsCORSListenerProxy::StartCORSPreflight, the original LoadInfo will already
> have the mInitialSecurityCheckDone flag set. So we need to make sure that
> the cloned LoadInfo does not have it set.

Yes, exactly!  I'll write a patch tomorrow.

Comment 11

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

(In reply to Ehsan Akhgari (don't ask for review please) from comment #10)
> I need to add a clone() method to nsILoadInfo then.

Please just add another nsLoadInfo ctor. I prefer JS not creating LoadInfo objects.

Comment 12

[(no longer active)]

FWIW, test_beaconPreflight.html is supposed to have tested this stuff, but it is completely broken in several ways:

* It tests the return value of sendBeacon, which would be true regardless of what happens during the preflight.
* It makes a same origin request(!!) which, well, doesn't need CORS at all.
* It was using beacon-handler.sjs that has no knowledge of preflights.

After fixing the test locally, I realized that if it had been written correctly, it would have caught this bug.

Comment 13

[(no longer active)]

Attachment: Stop reusing the loadinfo in StartCORSPreflight

(The commit message is a lie, obviously.)

Comment 14

[(no longer active)]

Attachment: Part 2: Fix the beacon CORS preflight tests

Comment 15

[Christoph Kerschbaumer [:ckerschb]]

I am not sure if we are rushing it too much here. I am working on Bug 1190201 which should fix this problem as well. We initially landed Bug 1182537 so that Navigator.sendBeacon relies on AsyncOpen2() in Firefox 42, but we backed it out again because we knew there was a problem with preflights, so it's in 43 at the moment.

Bug 1190201 is currently blocked by the redirectchain bug on LoadInfo (Bug 1194052) which I am about to fix at this very moment. We can obviously land Ehsans patch as an interim solution! However, adding a copy constructor which does not copy all the bits in an object but rather resets and intializes EnforceSecurityFlags as well as IntialSecurityChecksDone to false seems like a scary change to me.

Comment 16

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

I don't think bug 1190201 will fix this.

Comment 17

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

Comment on attachment 8665601 [details] [diff] [review]
Stop reusing the loadinfo in StartCORSPreflight

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

::: netwerk/base/LoadInfo.cpp
@@ +93,5 @@
> +  , mInnerWindowID(rhs.mInnerWindowID)
> +  , mOuterWindowID(rhs.mOuterWindowID)
> +  , mParentOuterWindowID(rhs.mParentOuterWindowID)
> +  , mEnforceSecurity(false)
> +  , mInitialSecurityCheckDone(false)

I wish there was a way to call the default copy-ctor directly here, and then just set the two members that we want to clear.

But I take it that's not possible. Though you could add a dummy-argument as to make this not override the default copy-ctor and then call the copy-ctor here?

Alternatively, maybe make .Clone() call the default copy-ctor and then set the two member?

::: netwerk/protocol/http/nsCORSListenerProxy.cpp
@@ +1317,5 @@
>      return NS_ERROR_FAILURE;
>    }
>  
> +  nsCOMPtr<nsILoadInfo> loadInfo = static_cast<mozilla::LoadInfo*>
> +    (originalLoadInfo.get())->Clone();

Unless we make .clone() do the call-copy-ctor-and-then-override-two-members, why not call the copy-ctor directly here?

Comment 18

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #16)
> I don't think bug 1190201 will fix this.

Why not? The patch within Bug 1190201 [1] also creates a copy of the loadInfo for the preflight channel within StartCorsPreflight.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8642571&action=diff

Comment 19

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

Ah, but it does copy the mInitialSecurityCheckDone flag, so it still wouldn't fix this bug ;-)

Comment 20

[(no longer active)]

(In reply to Jonas Sicking (:sicking) from comment #17)
> Comment on attachment 8665601 [details] [diff] [review]
> Stop reusing the loadinfo in StartCORSPreflight
> 
> Review of attachment 8665601 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/base/LoadInfo.cpp
> @@ +93,5 @@
> > +  , mInnerWindowID(rhs.mInnerWindowID)
> > +  , mOuterWindowID(rhs.mOuterWindowID)
> > +  , mParentOuterWindowID(rhs.mParentOuterWindowID)
> > +  , mEnforceSecurity(false)
> > +  , mInitialSecurityCheckDone(false)
> 
> I wish there was a way to call the default copy-ctor directly here, and then
> just set the two members that we want to clear.
> 
> But I take it that's not possible. Though you could add a dummy-argument as
> to make this not override the default copy-ctor and then call the copy-ctor
> here?
> 
> Alternatively, maybe make .Clone() call the default copy-ctor and then set
> the two member?

I don't know how to do the first, and the third suggestion seems better than the second, so I'll do that.

But did you note that I am also default initializing mRedirectChain since we haven't seen any redirects associated with the new load info object?  I want to double check that my choice there was not lost in transit, so needinfoing you.

Comment 21

[(no longer active)]

(In reply to Jonas Sicking (:sicking) from comment #19)
> Ah, but it does copy the mInitialSecurityCheckDone flag, so it still
> wouldn't fix this bug ;-)

Yeah I also double checked and it doesn't fix this bug.  Also it seems like that patch needs to be rebased on top of bug 1199049, FWIW.

Comment 22

[(no longer active)]

Comment on attachment 8665601 [details] [diff] [review]
Stop reusing the loadinfo in StartCORSPreflight

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  I don't think very easily.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No.  I am lying in the commit message because of this.

Which older supported branches are affected by this flaw? Only Aurora.

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

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

How likely is this patch to cause regressions; how much testing does it need?  I have tested it locally, and it also has an automated test.

Comment 23

[(no longer active)]

Comment on attachment 8665602 [details] [diff] [review]
Part 2: Fix the beacon CORS preflight tests

[Security approval request comment]
Please see comment 12.  As far as I can tell, currently we don't have any proper tests for CORS preflights in sendBeacon, so I would like to checkin the test together with the patch.  I am requesting approval separately to give the security team a chance to evaluate whether the tests should be checked in.  If the answer is no, I would like to file a new public bug about just fixing test_beaconPreflight.html at least (without checking in test_beaconPreflightFailure.html which is the test that shows what this bug is about.)

Comment 24

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

Making sure that mRedirectChain is empty seems good.

Comment 25

[(no longer active)]

Attachment: Stop reusing the loadinfo in StartCORSPreflight

With review comments addressed.

Comment 26

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

Sec-approval+. Since this is only Aurora and Trunk, check everything in.

Comment 27

[(no longer active)]

Comment on attachment 8665601 [details] [diff] [review]
Stop reusing the loadinfo in StartCORSPreflight

Approval Request Comment
[Feature/regressing bug #]: Bug 1182537
[User impact if declined]: Comment 0
[Describe test coverage new/current, TreeHerder]: Has a test
[Risks and why]: Should be low risk
[String/UUID change made/needed]: None

Comment 28

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

This ended up leaking all over the place. Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b82bd17c5a25

Comment 29

[(no longer active)]

(In reply to Wes Kocher (:KWierso) from comment #28)
> This ended up leaking all over the place. Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b82bd17c5a25

Using the default copy ctor is a mistake, since among other things, it copies mRefCnt, which means that we leak the cloned load info object as its mRefCnt never reaches zero.  I'll go back to using the explicit copy constructor in the original patch...

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1d10dde2b2f8
https://hg.mozilla.org/mozilla-central/rev/f67e85427b01

Comment 31

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8665601 [details] [diff] [review]
Stop reusing the loadinfo in StartCORSPreflight

Approved for uplift to aurora. Looks like and the tests should both land together.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2bed87c716f6
https://hg.mozilla.org/releases/mozilla-aurora/rev/0cfc3ec58048