Andrew Swan [:aswan] Reporter
	Description • 6 years ago

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

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

	Andrew Swan [:aswan] Reporter
	Comment 1 • 6 years ago

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?

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 2 • 6 years ago

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.

	Andrew Swan [:aswan] Reporter
	Comment 4 • 6 years ago

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

	Rob Wu [:robwu]
	Comment 5 • 6 years ago

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.

	Andrew Swan [:aswan] Reporter
	Comment 6 • 6 years ago

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.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 7 • 6 years ago

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

	Andrew Swan [:aswan] Reporter
	Comment 10 • 6 years ago

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?

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 11 • 6 years ago

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

	Andrew Swan [:aswan] Reporter
	Comment 13 • 6 years ago

(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

	Andrew Swan [:aswan] Reporter
	Comment 14 • 6 years ago

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

	Andrew Swan [:aswan] Reporter
	Comment 16 • 6 years ago

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.

	Andrew Swan [:aswan] Reporter
	Comment 17 • 6 years ago

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

	Andrew Swan [:aswan] Reporter
	Comment 18 • 6 years ago

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?

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 19 • 6 years ago

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

	Al Billings [:abillings - ex-MoCo]
	Comment 20 • 6 years ago

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.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 21 • 6 years ago

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

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 22 • 6 years ago

leave open to push test patch at a later date.

	Rob Wu [:robwu]
	Comment 23 • 6 years ago

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.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 24 • 6 years ago

Yep, that was missed.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 26 • 6 years ago

Depends on D7750

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 27 • 6 years ago

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

	Julien Cristau [:jcristau]
	Comment 28 • 6 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 29 • 6 years ago

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.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 30 • 6 years ago

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:

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 31 • 6 years ago

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:

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 32 • 6 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 33 • 6 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 34 • 6 years ago
uplift

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

	Ryan VanderMeulen [:RyanVM]
	Comment 35 • 6 years ago
uplift

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

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 36 • 6 years ago

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)

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 37 • 6 years ago

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.

	Victor Carciu
	Comment 38 • 6 years ago

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.

	Al Billings [:abillings - ex-MoCo]
	Comment 40 • 6 years ago

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

	Andrew Swan [:aswan] Reporter
	Comment 41 • 6 years ago

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

	Al Billings [:abillings - ex-MoCo]
	Comment 42 • 6 years ago

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