Closed Bug 1467523 (CVE-2018-12395) Opened 3 years ago Closed 2 years ago

"Domain fronting" by rewriting Host header with webRequest

Categories

(WebExtensions :: General, defect, P1)

60 Branch
defect

Tracking

(firefox-esr6063+ verified, firefox61 wontfix, firefox62 wontfix, firefox63+ verified, firefox64+ verified)

VERIFIED FIXED
mozilla64
Tracking Status
firefox-esr60 63+ verified
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 + verified
firefox64 + verified

People

(Reporter: aswan, Assigned: mixedpuppy)

Details

(Keywords: sec-moderate, Whiteboard: [adv-main63+][adv-esr60.3+])

Attachments

(6 files, 3 obsolete files)

+++ This bug was initially created as a clone of Bug #1465094 +++

See https://bugzilla.mozilla.org/show_bug.cgi?id=1465094#c1
I think the immediate solution here is to either
a. Require that an extension that tries to rewrite the Host header have a host/origin permission for the target, or
b. Just forbid rewriting the Host header altogether

An audit of standard HTTP headers to look for other places that could be similarly abused would be good too...

Shane, can you take this?
Flags: needinfo?(mixedpuppy)
Assignee: nobody → mixedpuppy
Flags: needinfo?(mixedpuppy)
Priority: -- → P2
The only two standard headers I see that might have any impact would be host and origin.  Host can be used by bypass extension permissions in specific situations, but I think it really only matters in the case of our restricted domains.
Attachment #8984287 - Flags: review?(aswan)
(In reply to Shane Caraveo (:mixedpuppy) from comment #2)
> The only two standard headers I see that might have any impact would be host
> and origin.  Host can be used by bypass extension permissions in specific
> situations, but I think it really only matters in the case of our restricted
> domains.

I don't see why changing the Origin header is a problem.
In any case, why do you think this only matters for restricted domains?  An extension of course needs to have a host permission for the original host, as I said in comment 1 I think it should also have a host permission for the value it is changing to, otherwise it can cause something to be cached for a host it does not have permission for.
The problem with the Host request header is that it can result in a (cached) response for a completely different website (served by the same server infrastructure).

There are two problems with that:
1) An extension can persistently poison the cache for the site for which it has permissions.
2) An extension can read the response of the site for which it does not have permissions.

The Origin request header does not have this issue; the worst that can happen is that somehow a server mistakenly believes that a request comes from another domain. But this is outside the scope of extensions. An extension can also strip the Origin header and the site would have to believe that this is a same-origin request, send a request directly, or from a content script.
Comment on attachment 8984287 [details]
prevent setting host/origin to restricted domains

In addition to comment 4, we need tests.
Depending on what sort of sec classification this gets, they may need to be a separate patch.
Attachment #8984287 - Flags: review?(aswan) → review-
(In reply to Andrew Swan [:aswan] (on PTO until 6/25) from comment #4)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #2)
> > The only two standard headers I see that might have any impact would be host
> > and origin.  Host can be used by bypass extension permissions in specific
> > situations, but I think it really only matters in the case of our restricted
> > domains.
> 
> I don't see why changing the Origin header is a problem.
> In any case, why do you think this only matters for restricted domains?  An
> extension of course needs to have a host permission for the original host,
> as I said in comment 1 I think it should also have a host permission for the
> value it is changing to, otherwise it can cause something to be cached for a
> host it does not have permission for.

I guess that I feel like a permission for an alternate host doesn't really address anything, the cache could still be messed up.  Users are not going to understand that situation enough to notice that an extension could do this.  OTOH it probably doesn't matter.

Denying setting host altogether feels too heavy handed, there could be entirely valid reasons to set host.

Maybe dveditz can chime in here as well.
Flags: needinfo?(dveditz)
Product: Toolkit → WebExtensions
Attachment #8984287 - Attachment is obsolete: true
Comment on attachment 8988586 [details] [diff] [review]
prevent setting host to restricted domains or without permission

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

I'm finding this kind of hard to follow.  It looks like you're trying to implement something general that can be extended later to other headers, but its not clear to me that we'll ever need that (if we do, I think that would be the time to implement the general solution).  In any case, with the current implementation you need to make a distinction between headers on a request and on a response, but this is done by passing `opts` on requests and not on responses.  If we're keeping the general approach, lets just always pass opts and add a property to opts to distinguish requests from responses.  But I would prefer just replacing the general `checkRestrictedHeaderValue` with a more targeted check for the Host header in requests.

Also still missing a test.

::: toolkit/modules/addons/WebRequest.jsm
@@ +147,5 @@
> +const checkRestrictedHeaderValue = (name, value, opts = null) => {
> +  if (name !== "host") {
> +    return;
> +  }
> +  let url = `https://${value}`;

This needs a trailing slash to be a valid url

@@ +151,5 @@
> +  let url = `https://${value}`;
> +  if (opts) {
> +    let {filter, extension} = opts;
> +
> +    if ((filter.urls && !filter.urls.matches(url)) ||

Why would we require this?
Attachment #8988586 - Flags: review?(aswan) → review-
(In reply to Andrew Swan [:aswan] from comment #10)
> Comment on attachment 8988586 [details] [diff] [review]
> prevent setting host to restricted domains or without permission
> 
> Review of attachment 8988586 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm finding this kind of hard to follow.  It looks like you're trying to
> implement something general that can be extended later to other headers, but
> its not clear to me that we'll ever need that (if we do, I think that would
> be the time to implement the general solution).  In any case, with the
> current implementation you need to make a distinction between headers on a
> request and on a response, but this is done by passing `opts` on requests
> and not on responses.  

It is actually determined by only implementing support for this in the RequestHeaderChanger class and not the ResponseHeaderChangerClass.  That's why I don't pass it for the response headers.  

> If we're keeping the general approach, lets just
> always pass opts and add a property to opts to distinguish requests from
> responses.  But I would prefer just replacing the general
> `checkRestrictedHeaderValue` with a more targeted check for the Host header
> in requests.



> Also still missing a test.

See the other patch.

> @@ +151,5 @@
> > +  let url = `https://${value}`;
> > +  if (opts) {
> > +    let {filter, extension} = opts;
> > +
> > +    if ((filter.urls && !filter.urls.matches(url)) ||
> 
> Why would we require this?

I guess checking against the listener filters is not really necessary, but it basically treats this like the rest of the api:  permission and filter need to overlap.
Attachment #8989224 - Flags: review?(aswan)
Attachment #8988585 - Flags: review?(aswan)
(In reply to Shane Caraveo (:mixedpuppy) from comment #11)
> > I'm finding this kind of hard to follow.  It looks like you're trying to
> > implement something general that can be extended later to other headers, but
> > its not clear to me that we'll ever need that (if we do, I think that would
> > be the time to implement the general solution).  In any case, with the
> > current implementation you need to make a distinction between headers on a
> > request and on a response, but this is done by passing `opts` on requests
> > and not on responses.  
> 
> It is actually determined by only implementing support for this in the
> RequestHeaderChanger class and not the ResponseHeaderChangerClass.  That's
> why I don't pass it for the response headers.  

That's awkward and confusing.  This code is overly general, why not just pass the extension down to setHeader() unconditionally
Comment on attachment 8989224 [details] [diff] [review]
prevent setting host to restricted domains or without permission

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

::: toolkit/modules/addons/WebRequest.jsm
@@ +148,5 @@
> +  let url = `https://${value}/`;
> +  let {extension} = opts;
> +
> +  if (extension) {
> +    if (!extension.allowedOrigins.matches(url)) {

This is going to instantiate an nsIURI internally, duplicating the construction done on line 157.  Please build just a single instance and pass it both here and to isRestrictedURI below
Attachment #8989224 - Attachment is obsolete: true
Attachment #8989224 - Flags: review?(aswan)
Comment on attachment 8988585 [details] [diff] [review]
1467523 xpcshell tests

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

Thanks, I do think that is clearer.  Just one last nit: I think that `checkRestrictedHeaderValue` could be named to something more specific (eg checkHostHeader or something).
Since this doesn't have a security classification yet, don't forget to get sec-approval before landing.
Attachment #8988585 - Flags: review?(aswan) → review+
Comment on attachment 8990733 [details] [diff] [review]
prevent setting host to restricted domains or without permission

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

Damn it, comment 16 was meant to apply to this patch
Attachment #8990733 - Flags: review?(aswan) → review+
Comment on attachment 8988585 [details] [diff] [review]
1467523 xpcshell tests

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

::: toolkit/components/extensions/test/xpcshell/test_ext_webRequest_host.js
@@ +15,5 @@
> +server.registerPathHandler("/return_headers.sjs", (request, response) => {
> +  response.setHeader("Content-Type", "text/plain", false);
> +
> +  let headers = {};
> +  // Why on earth is this a nsISimpleEnumerator...

probably because this is really old code.  But since you just care about the Host header, why do you need to iterate over all the headers here?
Comment on attachment 8990733 [details] [diff] [review]
prevent setting host to restricted domains or without permission

[Security approval request comment]
How easily could an exploit be constructed based on the patch?  easy, but has to be targeted at domains on shared ip

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?  probably, not certain how big a problem this issue is though.

Which older supported branches are affected by this flaw? all

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?  We could probably get this onto esr 60 and uplift to beta, not certain it's worth trying to backport to anything else

How likely is this patch to cause regressions; how much testing does it need?  If there are any valid use cases for changing the host header those extensions will suffer
Attachment #8990733 - Flags: sec-approval?
Making this a sec-moderate in discussion with Dveditz. As such, it doesn't need sec-approval to land on trunk so I'm clearing it.
Attachment #8990733 - Flags: sec-approval?
leave open to push test patch at a later date.
Flags: needinfo?(dveditz)
Keywords: leave-open
This bug is not properly fixed, because you're checking for lowercase "host". The header comparison should be case-insensitive.
The original test case from https://bugzilla.mozilla.org/show_bug.cgi?id=1465094#c1 still shows that the bug can be exploited.
Flags: needinfo?(mixedpuppy)
Yep, that was missed.
Flags: needinfo?(mixedpuppy)
Priority: P2 → P1
Leave-open for the test patch would make this a pain to track, so I'll close this.

https://hg.mozilla.org/mozilla-central/rev/203a8e58e64c
Status: NEW → RESOLVED
Closed: 2 years ago
Flags: in-testsuite?
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
We should probably get Beta and ESR60 uplift requests on this too. It grafts cleanly as-landed to both. And yeah, let's move tests to a follow-up bug at this point since this bug's history is already a bit messy.
Flags: needinfo?(mixedpuppy)
Comment on attachment 8990733 [details] [diff] [review]
prevent setting host to restricted domains or without permission

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: webrequest

User impact if declined: possible spoofing of websites on shared hosts

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: There is  test extension in bug 1465094 commen 1

List of other uplifts needed: see followup patch

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): fairly minor changes with both automated and manual test available.  change does have potential high impact on important extensions that use webrequest but I think the spoofing issue is important.

String changes made/needed: none

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: see user impact in beta request.  would be good to fix on a long lived release.

User impact if declined: 

Fix Landed on Version: 

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): 

String or UUID changes made by this patch:
Flags: needinfo?(mixedpuppy)
Attachment #8990733 - Flags: approval-mozilla-esr60?
Attachment #8990733 - Flags: approval-mozilla-beta?
Comment on attachment 9014474 [details]
Bug 1467523 followup fix for case insensitive header name check, r?aswan

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None

User impact if declined: 

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: Yes

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): This is a followup fix to the first patch, see requests on that.

String changes made/needed: 

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: 

User impact if declined: 

Fix Landed on Version: 

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): 

String or UUID changes made by this patch:
Attachment #9014474 - Flags: approval-mozilla-esr60?
Attachment #9014474 - Flags: approval-mozilla-beta?
Comment on attachment 8990733 [details] [diff] [review]
prevent setting host to restricted domains or without permission

This one is already on beta, just the followup needs to move.
Attachment #8990733 - Flags: approval-mozilla-beta?
Comment on attachment 8990733 [details] [diff] [review]
prevent setting host to restricted domains or without permission

Fixes a possible WebExtension spoofing issue. Approved for 63.0b14 and ESR 60.3.
Attachment #8990733 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Attachment #9014474 - Flags: approval-mozilla-esr60?
Attachment #9014474 - Flags: approval-mozilla-esr60+
Attachment #9014474 - Flags: approval-mozilla-beta?
Attachment #9014474 - Flags: approval-mozilla-beta+
Group: toolkit-core-security → core-security-release
extension from 1465094#c1 for QE, STR is:

Load the extension in Firefox, and the extension will try to read https://addons.mozilla.org/robots.txt

Expected result ("Without domain fronting"):
Failed to fetch: TypeError: NetworkError when attempting to fetch resource.

Actual result ("With domain fronting"):
(the actual response of AMO/robots.txt)
As discussed with QE on slack, the "without domain fronting" link in the test hits a cors error and will not complete.  I think that is fine, the important fix is that "with domain fronting" gets the network error.  Prior to the last patch here, that request completes (you see robots.txt).  With the last patch here, you get the network error.
Verified as fixed using FF Nightly(buildid:20181015100128) FF Beta(buildid:20181011200118) and Firefox ESR(buildid 20181012094822) on Windows 10x64 and macOS 10.13.6.

I will attach a postfix screenshot.
Status: RESOLVED → VERIFIED
Attached image Postfix screenshot
Whiteboard: [adv-main63+][adv-esr60.3+]
Alias: CVE-2018-12395
Did we fix both Host and Origin request headers in the patch or just Host?
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(aswan)
The patch only affects Host headers.  From comment 4 and comment 5 we agreed Origin headers are not an issue here.
Flags: needinfo?(aswan)
(In reply to Andrew Swan [:aswan] from comment #41)
> The patch only affects Host headers.  From comment 4 and comment 5 we agreed
> Origin headers are not an issue here.

Thanks.
Flags: needinfo?(mixedpuppy)

should probably checkin the test patch in D7751

Keywords: checkin-needed
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.