1467523 - (CVE-2018-12395) "Domain fronting" by rewriting Host header with webRequest

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P1)

Description

[Andrew Swan [:aswan]]

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

See https://bugzilla.mozilla.org/show_bug.cgi?id=1465094#c1

Comment 1

[Andrew Swan [:aswan]]

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?

Comment 2

[Shane Caraveo (:mixedpuppy)]

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.

Comment 3

[Shane Caraveo (:mixedpuppy)]

Attachment: prevent setting host/origin to restricted domains

Comment 4

[Andrew Swan [: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.

Comment 5

[Rob Wu [:robwu]]

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 6

[Andrew Swan [:aswan]]

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.

Comment 7

[Shane Caraveo (:mixedpuppy)]

(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.

Comment 8

[Shane Caraveo (:mixedpuppy)]

Attachment: 1467523 xpcshell tests

Comment 9

[Shane Caraveo (:mixedpuppy)]

Attachment: prevent setting host to restricted domains or without permission

Comment 10

[Andrew Swan [:aswan]]

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?

Comment 11

[Shane Caraveo (:mixedpuppy)]

(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.

Comment 12

[Shane Caraveo (:mixedpuppy)]

Attachment: prevent setting host to restricted domains or without permission

Comment 13

[Andrew Swan [: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 14

[Andrew Swan [:aswan]]

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

Comment 15

[Shane Caraveo (:mixedpuppy)]

Attachment: prevent setting host to restricted domains or without permission

Comment 16

[Andrew Swan [: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.

Comment 17

[Andrew Swan [:aswan]]

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

Comment 18

[Andrew Swan [:aswan]]

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 19

[Shane Caraveo (:mixedpuppy)]

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

Comment 20

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

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.

Comment 21

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/72c957a8df54e0b6725b236f246fbb1f94e55327
Bug 1467523 prevent setting host/origin to restricted domains, r=aswan

Comment 22

[Shane Caraveo (:mixedpuppy)]

leave open to push test patch at a later date.

Comment 23

[Rob Wu [:robwu]]

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.

Comment 24

[Shane Caraveo (:mixedpuppy)]

Yep, that was missed.

Comment 25

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1467523 followup fix for case insensitive header name check, r?aswan

Comment 26

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1467523 xpcshell tests for host header setting, r?aswan

Depends on D7750

Comment 27

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/203a8e58e64c86ba7b9cf8b66aec101b10eca3ec
Bug 1467523 followup fix for case insensitive header name check, r=aswan,robwu

Comment 28

[Julien Cristau [:jcristau]]

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 30

[Shane Caraveo (: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:

Comment 31

[Shane Caraveo (:mixedpuppy)]

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:

Comment 32

[Shane Caraveo (:mixedpuppy)]

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.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f439e5f9e3d3

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/133a99a8f3ca
https://hg.mozilla.org/releases/mozilla-esr60/rev/82176a4a9b14

Comment 36

[Shane Caraveo (:mixedpuppy)]

Attachment: webreq-domain-fronting.zip

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)

Comment 37

[Shane Caraveo (:mixedpuppy)]

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.

Comment 38

[Victor Carciu]

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.

Comment 39

[Victor Carciu]

Attachment: Postfix screenshot

Comment 40

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

Did we fix both Host and Origin request headers in the patch or just Host?

Comment 41

[Andrew Swan [:aswan]]

The patch only affects Host headers.  From comment 4 and comment 5 we agreed Origin headers are not an issue here.

Comment 42

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

(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.

Comment 43

[Shane Caraveo (:mixedpuppy)]

should probably checkin the test patch in D7751

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/autoland/rev/a13bd50f3cd4

Comment 45

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a13bd50f3cd4