1402363 - (CVE-2017-7835) when a redirect is blocked for mixed content blocking the original redirect response is returned instead

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Ben Kelly [:bkelly, not reviewing]]

Jake Archibald at google noticed some weird behavior.  See this demo:

  https://firefox-redirect-bug.glitch.me/

Source code is at:

  https://glitch.com/edit/#!/firefox-redirect-bug?path=public/script.js:1:0

In this case he is trying to fail a fetch() call by redirecting from an https:// origin to an http:// origin.  This does produce the failure message:

  Blocked loading mixed active content “http://example.com/”

But the fetch() succeeds and the 302 response is returned instead.

I instrumented FetchDriver a bit and its actually not seeing an AsyncOnChannelRedirect() call.  Instead its just getting an OnStartRequest() with a channel with a successful mStatus.

Christoph, is this a problem with our AsyncOpen2() security checks?  Or is it possible an http cache issue, Honza?

I'm marking this a secure bug since Jake told me that some sites include private information in redirects that should not be exposed to scripts.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Add NI for questions in comment 0.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

In the short term I can fix this by failing the fetch if we get OnStartRequest with a redirect status when redirect mode is not manual.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Anne, do you agree with Jake about this being a security issue?  How critical is it?  I'm not sure of the severity.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Fail unexpected redirect responses in FetchDriver. r=asuth

Andrew, this patch makes us fail any unexpected 30x responses from necko at the FetchDriver.  In general we should not see these, but it seems they can happen when the redirect is failed for things like mixed content.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8911228 [details] [diff] [review]
P1 Fail unexpected redirect responses in FetchDriver. r=asuth

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

::: dom/fetch/FetchDriver.cpp
@@ +480,5 @@
>  FetchDriver::OnStartRequest(nsIRequest* aRequest,
>                              nsISupports* aContext)
>  {
>    workers::AssertIsOnMainThread();
> +  printf_stderr("### ### [%p] FetchDriver::%s\n", this, __func__);

I'll remove the printfs.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

The problem with fixing this at the Fetch API layer, though, is that other necko consumers may have the same problem.  It seems XHR probably needs to be fixed as well.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8911228 [details] [diff] [review]
P1 Fail unexpected redirect responses in FetchDriver. r=asuth

Lets wait and see if this can be fixed in necko before proceeding here.

Comment 8

[Anne (:annevk)]

Okay, so we expose the content of a redirect when it goes from HTTPS to HTTP? That is somewhat problematic given https://fetch.spec.whatwg.org/#atomic-http-redirect-handling. The site would first need to be XSS'd successfully though before an attacker could steal such a token and the site would need to redirect to an HTTP service which seems less likely and would already reveal the token to network attackers as well. Unless I'm missing something I don't think this is severe.

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Ben Kelly [:bkelly] from comment #0)
> Christoph, is this a problem with our AsyncOpen2() security checks?  Or is
> it possible an http cache issue, Honza?

At first, I was thinking it can't be a problem for AsyncOpen2(), since I was thinking we have tests for that. Now I looked at the code and compared CSP and MixedContentBlocker redirect handling. It seems for CSP, we explicitly Cancel() the old channel [1], while for MixedContent we don't do that [2]. Looking at the comments of AsyncOnChannelRedirect() [3] that shouldn't be a problem though, but also wanted to make sure. Nevertheless, the fetch should go through [2] and returning an error from within that function should prevent the channel from loading any data.

@Honza: Is the above a potential issue?

@ Kate (Tanvi is not accepting ni? requests): I thought we have tests for mixed content redirect handling, but I couldn't fine any in dom/security/test/mixedcontentblocker/. There are only wpt tests for redirect handling. If we don't have mixed content redirect tests, could you please file a bug for that? We definitely need those.

[1] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsCSPService.cpp#336
[2] https://dxr.mozilla.org/mozilla-central/source/dom/security/nsMixedContentBlocker.cpp#397
[3] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIChannelEventSink.idl#66

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

I believe nsCORSListenerProxy also does an explicit cancel:

https://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsCORSListenerProxy.cpp#741

I think the explicit cancel is probably what we want here as well.

Christoph, do you mind taking this bug however we decide we want it to go?  I think it would be best to fix in the common code instead of in fetch, xhr, other consumers, etc.

Comment 11

[Honza Bambas (:mayhemer)]

So, necko's nsHttpChannel explicitly provides the redirect 30X response to the consumer (mListener) when the redirect is vetoed.  This is by design and won't change.

What you want is to explicitly cancel the source channel to not get 30X but a gecko loading error code in OnStartRequest/OnStopRequest instead.  What nsCORSListenerProxy does is correct.

What's suggested in comment 9 - to call Cancel(NS_BINDING_ABORTED) or similar error on the source channel - seems correct to me.

Comment 12

[Anne (:annevk)]

It does seem problematic that each caller needs to account for this. Ideally that gets abstracted somehow so we don't get this leak if we ever introduce another way to load code.

Comment 13

[Honza Bambas (:mayhemer)]

(In reply to Anne (:annevk) from comment #12)
> It does seem problematic that each caller needs to account for this. Ideally
> that gets abstracted somehow so we don't get this leak if we ever introduce
> another way to load code.

The source channel cancellation MUST be handled above necko.  We want to provide the content of the 30X response by default.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

I think there is some confusion.  There are kind of 3 layers here:

1. nsIChannel implementations
2. Security proxies and listeners that enforce CORS, mixed content, CSP, etc.
3. End client code like fetch(), xhr, document loading, etc.

Right now its up to layer (3) to cancel on the blocked redirect for mixed content.  For things like CORS and CSP we cancel at layer (2).  We just need to cancel at layer (2) for mixed content as well.

I think Honza is saying we can't do the cancellation at layer (1).

Comment 15

[Honza Bambas (:mayhemer)]

(In reply to Ben Kelly [:bkelly] from comment #14)
> I think there is some confusion.  There are kind of 3 layers here:
> 
> 1. nsIChannel implementations
> 2. Security proxies and listeners that enforce CORS, mixed content, CSP, etc.
> 3. End client code like fetch(), xhr, document loading, etc.
> 
> Right now its up to layer (3) to cancel on the blocked redirect for mixed
> content.  For things like CORS and CSP we cancel at layer (2).  We just need
> to cancel at layer (2) for mixed content as well.
> 
> I think Honza is saying we can't do the cancellation at layer (1).

Yes, exactly, thanks for clarifying this.

Comment 16

[Anne (:annevk)]

I'm sorry for causing confusing. I understood what Honza meant. I was just saying that it not being in 2 surprised me.

Comment 17

[Anne (:annevk)]

s/confusing/confusion/. :-(

Comment 18

[Kate McKinley [:kmckinley, :Kate]]

In the tree, the only mixed-content blocker blocker tests that include tests for redirection are in browser/base/content/test/siteIdentity/browser_mcb_redirect.js, and they don't cover this case.

Created bug 1403406 for testing and adding the Cancel to MCB, but maybe it should be in this instead.

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Kate McKinley [:kmckinley, :Kate] from comment #18)
> In the tree, the only mixed-content blocker blocker tests that include tests
> for redirection are in
> browser/base/content/test/siteIdentity/browser_mcb_redirect.js, and they
> don't cover this case.
> 
> Created bug 1403406 for testing and adding the Cancel to MCB, but maybe it
> should be in this instead.

I'll fix the problem in this bug and will also add the required tests within this bug - thanks kate.

Comment 21

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1402363_mcb_redirect.patch

Comment 22

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1402363_mcb_redirect_test.patch

Comment 23

[Honza Bambas (:mayhemer)]

Comment on attachment 8913600 [details] [diff] [review]
bug_1402363_mcb_redirect.patch

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

::: dom/security/nsMixedContentBlocker.cpp
@@ +395,5 @@
>                    requestingPrincipal,
>                    &decision);
> +  if (NS_FAILED(rv)) {
> +    autoCallback.DontCallback();
> +    aOldChannel->Cancel(NS_ERROR_DOM_BAD_URI);

Why exactly NS_ERROR_DOM_BAD_URI?

Comment 24

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8913600 [details] [diff] [review]
bug_1402363_mcb_redirect.patch

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

lgtm

Comment 25

[Kate McKinley [:kmckinley, :Kate]]

Comment on attachment 8913602 [details] [diff] [review]
bug_1402363_mcb_redirect_test.patch

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

lgtm

Comment 26

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Honza Bambas (:mayhemer) from comment #23)
> Why exactly NS_ERROR_DOM_BAD_URI?

No particular reason. Mostly because we use NS_ERROR_DOM_BAD_URI when blocking redirects within nsCORSListenerProxy and nsCSPService.

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

Just chatted with freddyb, we both think it's sec-moderate.

Comment 28

[Christoph Kerschbaumer [:ckerschb]]

I also verified that https://firefox-redirect-bug.glitch.me/ works correctly now and both requests fail.

Comment 29

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8913600 [details] [diff] [review]
bug_1402363_mcb_redirect.patch

Approval Request Comment
Currently we don't block mixed content redirects. If an HTTPS page hits a 302 to HTTP, then mixed content blocker should block the load which we didn't do correctly.

[Feature/Bug causing the regression]:
Bug 418354 - Mixed content detection does not take redirection into account

[User impact if declined]:
End users might get a false impression, because the pages state seems to be secure but the underlying redirect from HTTPS to HTTP exposes the user to tampering and eavesdropping attacks.

[Is this code covered by automated tests?]:
Yes

[Has the fix been verified in Nightly?]:
not yet.

[Needs manual test from QE? If yes, steps to reproduce]:
Not really, but there is also the testpage from comment 0.

[List of other uplifts needed for the feature/fix]:
XXX

[Is the change risky?]:
Nope, not risky, we are just correctly cancelling the old channel on the network layer if redirect is blocked.

[Why is the change risky/not risky?]:
see previous

[String changes made/needed]:
no

Comment 30

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8913602 [details] [diff] [review]
bug_1402363_mcb_redirect_test.patch

Approval Request Comment
See previous comment.

Comment 31

[Christoph Kerschbaumer [:ckerschb]]

Maybe this bug is the reason we had to disable the mcb_redirect tests when landing Bug 418354, see:
  https://bugzilla.mozilla.org/attachment.cgi?id=8507083&action=diff

I filed Bug 1404812 to investigate.

Comment 32

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8913600 [details] [diff] [review]
bug_1402363_mcb_redirect.patch

Sec-mod, still early in beta cycle, Beta57+

Comment 33

[Christoph Kerschbaumer [:ckerschb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2773796df8a337e5de31811acb4662ce67955f93
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcefba24074f60e2d3296996e20a8dd8bc6bebe0

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e28505bd7c04
https://hg.mozilla.org/releases/mozilla-beta/rev/37d18f6628dc

Comment 35

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

https://hg.mozilla.org/mozilla-central/rev/2773796df8a3
https://hg.mozilla.org/mozilla-central/rev/fcefba24074f

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Is this something you think we should consider for ESR52 backport as well? Note that we typically only take sec-high/crit patches unless there's a good reason for doing so.

Comment 37

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #36)
> Is this something you think we should consider for ESR52 backport as well?
> Note that we typically only take sec-high/crit patches unless there's a good
> reason for doing so.

Dan, what do you think? I would rather like to see that fixed in ESR52 as well, allowing mixed content redirects might be troublesome even though we only categorized this bug as sec-moderate.

Comment 38

[Daniel Veditz [:dveditz]]

I don't think we need it on ESR