1190201 - CORS after preflight should not follow same origin redirect when using asyncOpen2() in sendBeacon

Status: RESOLVED WORKSFORME

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

First, the following issues were introduced within Bug 1182537 and do *only* occur in combination with AsyncOpen2. Bug 1182537 introduced the usage of AsyncOpen2() for SendBeacon, so only SendBeacon is affected at the moment. Classifying this as a sensitive bug anyway till we got it resolved!


There are two things we have to update with respect to CORS prelfight channels in combination with asyncOpen2():

(1) we have to make sure that CORS doesn't follow same origin redirects. We should also extend test_beaconCORSRedirect.html to cover that case.

(2) We have to copy the loadInfo before creating the preflight channel [1]. Otherwise the securityManager sets the initalSecurityFlagsDone flag on the loadInfo [2] and contentPolicies and other checks are bypassed for the actual reqeust.

[1] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCORSListenerProxy.cpp#1280
[2] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsContentSecurityManager.cpp#246

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1190201_cors_redirect_asyncopen2_tests.patch

Extended the test to include not only CORS *cross origin* redirect after preflight but also CORS *same origin* redirect after preflight.

I applied this test patch to current Aurora and it works just fine. To emphasize again, we just have to fix the current code in mozilla-central.

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Jonas, we are facing yet another problem with preflights and asyncOpen2().
We are not sending the "origin" header with the actual channel in case of preflights.

In the old version the preflightListener was wrapped within the corsListenerProxy which did the necessary setup, see pseudo stack:
> nsCORSListenerProxy::UpdateChannel() [1]
> nsCORSListenerProxy::Init()
> NS_StartCORSPreflight()

but the new version only sets up the preflightListener not using the corsListenerProxy anymore:
http://hg.mozilla.org/mozilla-central/diff/dc7fa51079ad/dom/security/nsCORSListenerProxy.cpp#l1.130

Should we bring back the corsListenerProxy for that purpose?

[1] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCORSListenerProxy.cpp#967

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #2)
> Jonas, we are facing yet another problem with preflights and asyncOpen2().
> We are not sending the "origin" header with the actual channel in case of
> preflights.
> 
> In the old version the preflightListener was wrapped within the
> corsListenerProxy which did the necessary setup, see pseudo stack:
> > nsCORSListenerProxy::UpdateChannel() [1]
> > nsCORSListenerProxy::Init()
> > NS_StartCORSPreflight()
> 
> but the new version only sets up the preflightListener not using the
> corsListenerProxy anymore:
> http://hg.mozilla.org/mozilla-central/diff/dc7fa51079ad/dom/security/
> nsCORSListenerProxy.cpp#l1.130
> 
> Should we bring back the corsListenerProxy for that purpose?
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/dom/security/
> nsCORSListenerProxy.cpp#967

Jonas, nevermind about Comment 2.

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1190201_cors_redirect_asyncopen2.patch

In fact, what we can do is introduce a new security flag to nsILoadInfo which indicates CORS with preflights (we can reuse that flag later when we decide to move preflights into asyncOpen2 as well).

All we have to do is, copy the loadInfo for preflight channels and within the content security manger we can inspect that flag after we have checked for initialSecurityChecksDone. At that time we now we must have been going through a redirect and cancel the channel.

That should work, right?

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Oh, there might be another issue, what if there is a CORS preflight channel in combination with CSPs upgrade-insecure-requests or HSTS where we use internal redirects? Might we then cancel the preflight channel because we call the contentSecurityManager again after the internal redirect?

Comment 6

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

Comment on attachment 8642571 [details] [diff] [review]
bug_1190201_cors_redirect_asyncopen2.patch

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

r=me with that fixed.

::: dom/base/Navigator.cpp
@@ +1213,5 @@
> +
> +  nsSecurityFlags securityFlags = nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS;
> +  if (needsCorsWithPreflight) {
> +    securityFlags |= nsILoadInfo::SEC_REQUIRE_CORS_WITH_PREFLIGHT;
> +  }

Inline this into the NewChannel call?

::: dom/security/nsContentSecurityManager.cpp
@@ +279,5 @@
> +    // make sure the CORS channels with preflight are not allowed
> +    // to follow any redirects
> +    if (loadInfo->GetRequireCORSPreflight()) {
> +      return NS_ERROR_DOM_BAD_URI;
> +    }

Rather than doing this when initialSecurityCheckDone is set, can you check the length of the redirect chain?

What we're really trying to prevent here is redirects, so doing that seems more correct and less likely to break.

I worry for example that the initialSecurityCheckDone flag might be set when we have inner channels.

Comment 7

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

Comment on attachment 8642571 [details] [diff] [review]
bug_1190201_cors_redirect_asyncopen2.patch

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

::: dom/base/Navigator.cpp
@@ +1213,5 @@
> +
> +  nsSecurityFlags securityFlags = nsILoadInfo::SEC_REQUIRE_CORS_DATA_INHERITS;
> +  if (needsCorsWithPreflight) {
> +    securityFlags |= nsILoadInfo::SEC_REQUIRE_CORS_WITH_PREFLIGHT;
> +  }

Inline this into the NewChannel call?

@@ +1273,4 @@
>      // we need to set the sameOriginChecker as a notificationCallback
>      // so we can tell the channel not to follow redirects
>      nsCOMPtr<nsIInterfaceRequestor> soc = nsContentUtils::SameOriginChecker();
>      channel->SetNotificationCallbacks(soc);

Also, remove this same-origin-checker.

::: dom/security/nsContentSecurityManager.cpp
@@ +279,5 @@
> +    // make sure the CORS channels with preflight are not allowed
> +    // to follow any redirects
> +    if (loadInfo->GetRequireCORSPreflight()) {
> +      return NS_ERROR_DOM_BAD_URI;
> +    }

Rather than doing this when initialSecurityCheckDone is set, can you check the length of the redirect chain?

What we're really trying to prevent here is redirects, so doing that seems more correct and less likely to break.

I worry for example that the initialSecurityCheckDone flag might be set when we have inner channels.

Comment 8

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

Sorry about the double-review. The second one is the correct one.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8642541 [details] [diff] [review]
bug_1190201_cors_redirect_asyncopen2_tests.patch

(In reply to Jonas Sicking (:sicking) from comment #8)
> Sorry about the double-review. The second one is the correct one.

Jonas, no problem at all and thanks for the review, but please also take a look at the tests. I thought I flagged you for those as well.

Comment 10

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

Comment on attachment 8642541 [details] [diff] [review]
bug_1190201_cors_redirect_asyncopen2_tests.patch

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

I don't actually fully follow what these tests are doing. But as long as you are testing that:
* redirect of a preflighted cross-origin beacon *fails*
* redirect of a preflighted same-origin to cross-origin *fails*
* redirect of a non-preflighted cross-origin beacon *succeeds*
* redirect of a same-origin redirect *succeeds*


If so, rs=me

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #7)
> Rather than doing this when initialSecurityCheckDone is set, can you check
> the length of the redirect chain?

Unfortunately no, because we append the principal to the redirectChain right after opening the channel succeeded, see:
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#211

> What we're really trying to prevent here is redirects, so doing that seems
> more correct and less likely to break.
> 
> I worry for example that the initialSecurityCheckDone flag might be set when
> we have inner channels.

Thinking about this again in terms of CSPs updgrade-insecure-requests or also HSTS [1] then we would have an internal redirect, which means we would set the 'initalSecurityChecksDone' flag when we call AsyncOpen(2) the first time on the preflight channel, then we internally redirect and call asyncOpen(2) again, which would then block the channel from opening.

I suppose we need a different approach! An alternative approach would be to create a new loadinfo whenever we are dealing with an internal redirect. Basically copy all the bits in the loadInfo but 'reset' initialSecurityChecksDone. Internal redirects are always messy I suppose!


[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#330

Comment 12

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

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #11)
> (In reply to Jonas Sicking (:sicking) from comment #7)
> > Rather than doing this when initialSecurityCheckDone is set, can you check
> > the length of the redirect chain?
> 
> Unfortunately no, because we append the principal to the redirectChain right
> after opening the channel succeeded, see:
> http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/
> nsHttpChannel.cpp#211

Hrm.. That's actually pretty annoying. I know that we debated back and forth when to add it, but this seems to indicate that we made the wrong decision.

Security checks usually happen before we successfully open the channel, and it's in those security checks that we generally will want to have access to the redirect chain.

Can we change this to add to the redirect chain before we start the actual redirect?

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #12)
> (In reply to Christoph Kerschbaumer [:ckerschb] from comment #11)
> > (In reply to Jonas Sicking (:sicking) from comment #7)
> > > Rather than doing this when initialSecurityCheckDone is set, can you check
> > > the length of the redirect chain?
> > 
> > Unfortunately no, because we append the principal to the redirectChain right
> > after opening the channel succeeded, see:
> > http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/
> > nsHttpChannel.cpp#211
> 
> Hrm.. That's actually pretty annoying. I know that we debated back and forth
> when to add it, but this seems to indicate that we made the wrong decision.
> 
> Security checks usually happen before we successfully open the channel, and
> it's in those security checks that we generally will want to have access to
> the redirect chain.
> 
> Can we change this to add to the redirect chain before we start the actual
> redirect?

I think so - let me check back with Honza first though.

Comment 14

[Christoph Kerschbaumer [:ckerschb]]

:nsm pointed out that we are calling the wrong constructor of ::nsCorsListenerProxy() whenever we are dealing with a preflight channel.

In Detail:
From within NS_StartCORSPreflight we call AsyncOpen2() which calls a different constructor of ::nsCorsListenerProxy() then the one that is used for preflights [1] where one sets mPreflight to true [2] instead of false.

Good news, we can rely on SEC_REQUIRE_CORS_WITH_PREFLIGHT to call one or the other constructor of ::nsCorsListenerProxy() from within DoCORSChecks().

[1] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCORSListenerProxy.cpp#1360
[2] http://mxr.mozilla.org/mozilla-central/source/dom/security/nsCORSListenerProxy.cpp#459

Comment 15

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

FetchDriver and XHR already do this correct, so this only affects sendBeacon. I have patches in the works that will fix this.

Comment 16

[Tanvi Vyas[:tanvi]]

Jonas, are you still working on this bug?

Comment 17

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

This was fixed in bug 1226909