1308688 - (CVE-2017-5389) Prevent WebExtensions from modifying requests to hosts with mozAddonManager permissions

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, defect, P1)

Description

[Kris Maglione [:kmag]]

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

As a follow-up to bug 1295324, we also need to prevent extensions from modifying requests to sites with elevated permissions, since they can gain the same elevated privileges by modifying CSP headers and then redirecting script requests to inject malicious content.

Comment 1

[Andrew Swan [:aswan]]

Attachment: Expose mozAddonManager allowed hosts to chrome

MozReview-Commit-ID: JI3qMmCtYFT

Comment 2

[Andrew Swan [:aswan]]

Attachment: webrequest.patch

MozReview-Commit-ID: B9YbBr3fMuO

Comment 3

[Andrew Swan [:aswan]]

The first patch was originally proposed and r+'ed on bug 1295324, but eventually discarded in favor of a different approach.
Since it touches webidl it needs a dom peer, but I'll wait until we've got agreement on the second patch.
Test case also coming shortly in a separate patch so it can be embargoed.

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8804457 [details] [diff] [review]
webrequest.patch

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

We also need to check the origin of the request so that it isn't possible to modify (e.g.) script load requests from AMO.

Comment 5

[Andrew Swan [:aswan]]

Attachment: webrequest.patch

MozReview-Commit-ID: B9YbBr3fMuO

Comment 6

[Andrew Swan [:aswan]]

Attachment: tests

MozReview-Commit-ID: 8DOZbPY7FZ9

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8804940 [details] [diff] [review]
webrequest.patch

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

::: toolkit/modules/addons/WebRequest.jsm
@@ +605,5 @@
> +      if (!result || !opts.blocking
> +          || AddonManagerPermissions.isHostPermitted(uri.host)
> +          || Services.scriptSecurityManager.isSystemPrincipal(originPrincipal)
> +          || (originPrincipal.URI.host
> +              && AddonManagerPermissions.isHostPermitted(originPrincipal.URI.host))) {

We should probably just use the originPrincipal for these last two, and not worry about the triggeringPrincipal. The former matters much more, in this case.

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8804941 [details] [diff] [review]
tests

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

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_permission.html
@@ +25,5 @@
> +      return {};
> +    }, {urls: ["<all_urls>"]}, ["blocking"]);
> +
> +  }
> +      

Nit: Trailing space.

@@ +32,5 @@
> +      permissions: ["webRequest", "webRequestBlocking", "<all_urls>"],
> +    },
> +    background,
> +  };
> +        

Trailing space.

Comment 9

[Andrew Swan [:aswan]]

Attachment: webrequest.patch

MozReview-Commit-ID: B9YbBr3fMuO

Comment 10

[Andrew Swan [:aswan]]

Attachment: tests

MozReview-Commit-ID: 8DOZbPY7FZ9

Comment 11

[Andrew Swan [:aswan]]

Comment on attachment 8804454 [details] [diff] [review]
Expose mozAddonManager allowed hosts to chrome

Ben, you r+'ed this patch earlier for a different bug before we discarded it for a different approach.  But just in time for Halloween, it is rising from the grave...

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8804454 [details] [diff] [review]
Expose mozAddonManager allowed hosts to chrome

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

::: toolkit/mozapps/extensions/AddonManagerWebAPI.cpp
@@ +33,5 @@
> +        host.Equals("addons-dev.allizom.org") ||
> +        host.Equals("discovery.addons-dev.allizom.org") ||
> +        host.Equals("testpilot.stage.mozaws.net") ||
> +        host.Equals("testpilot.dev.mozaws.net") ||
> +        host.Equals("example.com")) {

I think all of these should be `EqualsLiteral()`.  Also, do you need handle mixed case since host names are case insensitive?  If so, use `LowerCaseEqualsLiteral()`.

@@ +58,3 @@
>    }
>  
>    nsCString host;

This could be nsAutoCString to avoid memory allocation.

@@ +62,5 @@
>    if (NS_FAILED(rv)) {
>      return false;
>    }
>  
> +  if (IsValidHost(host)) {

This can just be `return IsValidHost(host);`.  No need for the if-statement and separate return statements.

Comment 13

[Andrew Swan [:aswan]]

Comment on attachment 8805669 [details] [diff] [review]
webrequest.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Tests are in a separate patch.  The changes in this patch point in the general direction of the vulnerability but one needs a deeper understanding of the vulnerability to construct an exploit.  Moreover, anybody who understands the vulnerability and is paying attention has probably already seen bug 1295324 so they're unlikely to figure out much new from seeing this one.

Which older supported branches are affected by this flaw?
Everything since mozAddonManager shipped in 48.

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I haven't actually tried it but the changes should backport with little difficulty.

How likely is this patch to cause regressions; how much testing does it need?
Tests are included.  This could cause existing addons that use WebRequest.jsm to stop working when used on sites like AMO, but I don't think there's any way to avoid that (we had the same situation with bug 1295324 which lead to bug 1310082...)

Comment 14

[Andrew Swan [:aswan]]

Attachment: Expose mozAddonManager allowed hosts to chrome

MozReview-Commit-ID: JI3qMmCtYFT

Comment 15

[Andrew Swan [:aswan]]

Comment on attachment 8805704 [details] [diff] [review]
Expose mozAddonManager allowed hosts to chrome

carrying over r+ from bkelly after applying his suggested changes.

Comment 16

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

This is waaaaay too late for 50, which is on its final beta already.

This needs to go in two weeks after we ship, which is now Nov 15, making this a "sec-approval+ for checkin on 11/29" patch. Tests can't go in until after we ship the fix in a final release build so this approval is only for the non-tests patch.

Once it is on trunk, we'll want to backport it to Aurora and Beta.

Comment 17

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8805669 [details] [diff] [review]
webrequest.patch

Just a drive by comment.

- loadingPrincipal can be null, are we certain it is never null in this case?
- I've been looking at WebRequest.jsm as a base library with access to all, and cutting off access in ext-webrequest for things we don't want webextensions to see.  That's just my approach, not sure it really matters.

Comment 18

[Andrew Swan [:aswan]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dfb42f181ef9ada2ebce56d4f1beb0eb857b5487
Bug 1308688 Expose mozAddonManager allowed hosts to chrome r=bkelly

https://hg.mozilla.org/integration/mozilla-inbound/rev/72ff2080ca120c16fd3b035c54b9ae8957fe83ed
Bug 1308688 r=kmag

Comment 19

[Andrew Swan [:aswan]]

I basically needed to rewrite the patch to accomodate all the changes since this was originally approved, I added a check that loadingPrincipal is not null.
On the second point, we don't pass enough information to the handlers to make the decision inside ext-webrequest.js.  Instead of changing that interface, I took the simpler route, partly since this will become irrelevant soon enough when non-webextension addons disappear.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

Should we be considering this for the 50.1 release?

Comment 21

[Andrew Swan [:aswan]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #20)
> Should we be considering this for the 50.1 release?

Perhaps but I think the security folks are better equipped to make that decision.

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/dfb42f181ef9
https://hg.mozilla.org/mozilla-central/rev/72ff2080ca12

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Please do nominate this for Aurora/Beta approval ASAP so we can hopefully get it uplifted in time for 51b6 at least.

Comment 24

[Andrew Swan [:aswan]]

Comment on attachment 8805704 [details] [diff] [review]
Expose mozAddonManager allowed hosts to chrome

Approval Request Comment
[Feature/Bug causing the regression]:
mozAddonManager (bug 1245571)

[User impact if declined]:
The patches here address a security vulnerability, without these patches, the vulnerability will be present.

[Is this code covered by automated tests?]:
Yes, though they are in a separate patch which will presumably remain embargoed for some time

[Has the fix been verified in Nightly?]:
other than automated testing, not yet

[Needs manual test from QE? If yes, steps to reproduce]: 
steps to reproduce are described at a high level in the original bug.  Kris or I could probably produce a working extension to use for testing

[List of other uplifts needed for the feature/fix]:
just the other patch attached to this bug

[Is the change risky?]:
[Why is the change risky/not risky?]:
this patch is not at all risky it just adds a new function exposed to javascript.  the second patch is riskier since it changes the behavior of the webextensions webRequest api but the changes are covered by unit tests.

[String changes made/needed]:
none

Comment 25

[Andrew Swan [:aswan]]

Attachment: webrequest.patch

MozReview-Commit-ID: IinAfopVsw8

Comment 26

[Andrew Swan [:aswan]]

Comment on attachment 8815593 [details] [diff] [review]
webrequest.patch

This is the original webrequest.patch that had r+ from kmag and sec-approval from abillings rebased onto inbound.  see comment 24 for uplift details.

Comment 27

[Gerry Chang [:gchang]]

Comment on attachment 8805704 [details] [diff] [review]
Expose mozAddonManager allowed hosts to chrome

Fix a sec-high. Beta51+ and Aurora52+. Should be in 51 beta 6.

Comment 28

[Marcia Knous [:marcia]]

Tracking 53- as this is resolved fixed.

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6a3264ad059f
https://hg.mozilla.org/releases/mozilla-aurora/rev/150c4dd9fd30

Comment 30

[Carsten Book [:Tomcat]]

needs rebasing for beta

Comment 31

[Andrew Swan [:aswan]]

(In reply to Carsten Book [:Tomcat] from comment #30)
> needs rebasing for beta

If it was just the second patch that failed to apply, the original patch should apply cleanly:
https://bug1308688.bmoattachments.org/attachment.cgi?id=8805669

I'm sorry I have to step away for a moment and can't actually verify this myself right now, but I can do so later today if the above doesn't work or the sheriffs don't have the time or interest in trying that patch.

Comment 32

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

Hello guys, do we need this in 50.1.0? Just checking since it's a sec-high.

Comment 33

[Shane Caraveo (:mixedpuppy)]

Ritu, this and bug 1320057 should go into 50.1, unless we're ok with doing a gofaster push, but that seems like a pain when a release is 2 weeks away.

Comment 34

[Shane Caraveo (:mixedpuppy)]

arg.  sorry, that last message got on the wrong bug.

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9b936f748c46
https://hg.mozilla.org/releases/mozilla-beta/rev/82e0d9a22463

Comment 36

[Ryan VanderMeulen [:RyanVM]]

Backed out from Beta for test_ext_webrequest.html failures.
https://hg.mozilla.org/releases/mozilla-beta/rev/b28660caae81

https://treeherder.mozilla.org/logviewer.html#?job_id=2040832&repo=mozilla-beta

Comment 37

[Marcia Knous [:marcia]]

Tracking 53+ for this sec high issue.

Comment 38

[Andrew Swan [:aswan]]

Attachment: Expose mozAddonManager allowed hosts to chrome

MozReview-Commit-ID: Igw1YsSP2Fv

Comment 39

[Andrew Swan [:aswan]]

Attachment: webrequest-beta.patch

MozReview-Commit-ID: IcH7YXuEJbW

Comment 40

[Andrew Swan [:aswan]]

So the original patch was too aggressive in blocking header modifications for loads without a loading principal.  This was responsible for the mochitest failures on beta, and a user separately reported that this broke top level loads with user agent switcher.  But this suggests that we lost some test coverage with bug 1314492.  Shane, can you look into that?
Anyhow, I've updated new beta patches and will add a follow-up for central and aurora momentarily.
As for whether we want uplift to release, I think that's up to security, clearing my needinfo for that.

Comment 41

[Kris Maglione [:kmag]]

Comment on attachment 8816198 [details] [diff] [review]
webrequest-beta.patch

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

Can you also add a test to [1] in m-c and Aurora to make sure this doesn't regress again?

[1]: http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_webRequest.js

Comment 42

[Shane Caraveo (:mixedpuppy)]

Andrew, loads without a loadingPrincipal are top level loads, so yeah, they shouldn't be blocked.  I missed that aspect of the patch in my driveby.  Bug 1273138 broke top level loads, bug 1318800 (pending uplift to aurora) fixed that as well as fixing tests for that situation.  I think landing this bug may have beat bug 1318800, thus the tests didn't catch it.

Comment 43

[Andrew Swan [:aswan]]

But from a quick glance, it looks like bug 1318800 covers passive listeners, but not blocking listeners that make changes to headers.  We had coverage of that prior to bug 1314492 but we apparently don't have it now.  That's what Kris is asking for in comment 41.

Comment 44

[Andrew Swan [:aswan]]

Attachment: followup

MozReview-Commit-ID: GPPJm1Hlx88

Comment 45

[Andrew Swan [:aswan]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d05dfdbfac25ffb73193b11f1c951f66bee4544
Bug 1308688 followup r=kmag

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/df4d31969686

https://hg.mozilla.org/releases/mozilla-beta/rev/7f9a7d1ed122
https://hg.mozilla.org/releases/mozilla-beta/rev/df6600c5e97d

Comment 47

[Andrew Swan [:aswan]]

(In reply to Kris Maglione [:kmag] from comment #41)
> Can you also add a test to [1] in m-c and Aurora to make sure this doesn't
> regress again?

This bug is getting awfully cluttered, taking that to bug 1321615

Comment 48

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6d05dfdbfac2

Comment 49

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

(In reply to Andrew Swan [:aswan] from comment #21)
> (In reply to Ryan VanderMeulen [:RyanVM] from comment #20)
> > Should we be considering this for the 50.1 release?
> 
> Perhaps but I think the security folks are better equipped to make that
> decision.

What is the argument for needing this in 50.1 versus taking it in 51 (which is the default). I'm loathe to take non-desperate changes in 50.1 due to the fact that there are zero betas for 50.1.

Comment 50

[Andrew Swan [:aswan]]

(In reply to Al Billings [:abillings] from comment #49)
> (In reply to Andrew Swan [:aswan] from comment #21)
> > (In reply to Ryan VanderMeulen [:RyanVM] from comment #20)
> > > Should we be considering this for the 50.1 release?
> > 
> > Perhaps but I think the security folks are better equipped to make that
> > decision.
> 
> What is the argument for needing this in 50.1 versus taking it in 51 (which
> is the default). I'm loathe to take non-desperate changes in 50.1 due to the
> fact that there are zero betas for 50.1.

Nothing specific other than the fact it was classified sec-high.  Sounds like leaving it as is (fixed in 51 and newer but nothing else) would be reasonable.

Comment 51

[Andrew Swan [:aswan]]

What's the timeline for landing the tests for this bug?

Comment 52

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

I'd suggest next week. We'll have had 51 out for two weeks then.

Comment 53

[Andrew Swan [:aswan]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ac2e09d98cb55fb9440b0cb9267ed365293aa9a
Bug 1308688 tests r=kmag

Comment 54

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e2c43c2604ab for eslint failures, https://treeherder.mozilla.org/logviewer.html#?job_id=76542224&repo=mozilla-inbound

Comment 55

[Andrew Swan [:aswan]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d070f532ca2d35470e6996b0fd3e58cd351f9c06
Bug 1308688 tests r=kmag

Comment 56

[Phil Ringnalda (:philor)]

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

Comment 57

[Andrew Swan [:aswan]]

Last question on this one, the tests are now landed in central, should they be uplifted and if so, how far?

Comment 58

[Daniel Veditz [:dveditz]]

We obviously have to land any fixups for existing tests (I don't think I saw any on this one).

We don't /have/ to uplift the unit tests for this fix, but then we will need to manually verify the fix on those branches. Sometimes uplifted fixes don't work because they built (unawares) on some more recent behavior that's not in the branches.