Bug 1402363 (CVE-2017-7835)

when a redirect is blocked for mixed content blocking the original redirect response is returned instead

RESOLVED FIXED in Firefox 57

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: bkelly, Assigned: ckerschb)

Tracking

(Blocks 1 bug, {sec-moderate})

57 Branch
mozilla58
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox55 wontfix, firefox56 wontfix, firefox57 fixed, firefox58 fixed)

Details

(Whiteboard: [tor][domsecurity-active][adv-main57+][post-critsmash-triage])

Attachments

(2 attachments, 1 obsolete attachment)

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.
Add NI for questions in comment 0.
Flags: needinfo?(honzab.moz)
Flags: needinfo?(ckerschb)
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.
Anne, do you agree with Jake about this being a security issue?  How critical is it?  I'm not sure of the severity.
Flags: needinfo?(annevk)
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.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
Attachment #8911228 - Flags: review?(bugmail)
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.
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 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.
Attachment #8911228 - Flags: review?(bugmail)

Comment 8

2 years ago
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.
Flags: needinfo?(annevk)
(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
Flags: needinfo?(ckerschb) → needinfo?(kmckinley)
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.
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.
Flags: needinfo?(honzab.moz)

Comment 12

2 years ago
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.
Assignee: bkelly → ckerschb
Group: core-security → dom-core-security
(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.
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).
(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

2 years ago
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

2 years ago
s/confusing/confusion/. :-(
See Also: → 1403406
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.
Flags: needinfo?(kmckinley)
(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.
Priority: -- → P1
Whiteboard: [tor][domsecurity-active]
Duplicate of this bug: 1403406
Attachment #8911228 - Attachment is obsolete: true
Attachment #8913600 - Flags: review?(kmckinley)
Attachment #8913600 - Flags: review?(honzab.moz)
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?
Attachment #8913600 - Flags: review?(honzab.moz) → review+
Comment on attachment 8913600 [details] [diff] [review]
bug_1402363_mcb_redirect.patch

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

lgtm
Attachment #8913600 - Flags: review?(kmckinley) → review+
Comment on attachment 8913602 [details] [diff] [review]
bug_1402363_mcb_redirect_test.patch

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

lgtm
Attachment #8913602 - Flags: review?(kmckinley) → review+
(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.
Just chatted with freddyb, we both think it's sec-moderate.
Keywords: sec-moderate
I also verified that https://firefox-redirect-bug.glitch.me/ works correctly now and both requests fail.
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
Attachment #8913600 - Flags: approval-mozilla-beta?
Comment on attachment 8913602 [details] [diff] [review]
bug_1402363_mcb_redirect_test.patch

Approval Request Comment
See previous comment.
Attachment #8913602 - Flags: approval-mozilla-beta?
Blocks: 1404812
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 on attachment 8913600 [details] [diff] [review]
bug_1402363_mcb_redirect.patch

Sec-mod, still early in beta cycle, Beta57+
Attachment #8913600 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8913602 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
https://hg.mozilla.org/mozilla-central/rev/2773796df8a3
https://hg.mozilla.org/mozilla-central/rev/fcefba24074f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Group: dom-core-security → core-security-release
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.
Flags: needinfo?(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.
Flags: needinfo?(ckerschb) → needinfo?(dveditz)
I don't think we need it on ESR
Flags: needinfo?(dveditz)
Whiteboard: [tor][domsecurity-active] → [tor][domsecurity-active][adv-main57+]
Alias: CVE-2017-7835
Flags: qe-verify-
Whiteboard: [tor][domsecurity-active][adv-main57+] → [tor][domsecurity-active][adv-main57+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.