1295324 - (CVE-2016-9075) WebExtensions can easily access the mozAddonManager API and use it to gain elevated privileges.

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Kris Maglione [:kmag]]

See attached testcase. Simply install it (in Firefox 49 or later), and it immediately installs AdBlock Plus.

Given AMO's CSP, we could probably prevent this by a) preventing the object from being accessed via wrappers, and b) preventing extensions from meddling with CSP headers.

But the only real solution I see is to prevent WebExtensions from executing scripts on any site with these kinds of elevated privileges.

Comment 1

[Kris Maglione [:kmag]]

Attachment: test-aom.xpi

Comment 2

[Andrew Swan [:aswan]]

(In reply to Kris Maglione [:kmag] from comment #0)
> Given AMO's CSP, we could probably prevent this by a) preventing the object
> from being accessed via wrappers, and b) preventing extensions from meddling
> with CSP headers.

I think we briefly discussed this on IRC a while back but for posterity here, can you elaborate on this?  Why isn't a by itself sufficient?

Comment 3

[Kris Maglione [:kmag]]

It's just way too fragile. Even if we're fairly sure we can prevent the extension from executing scripts in the page context, we can't really prevent it from manipulating the page content or other JS objects so that the existing page scripts call it for us.

Comment 4

[Andrew Swan [:aswan]]

Got it.  Assuming we should go with the "prevent WebExtensions from executing scripts on any site with these kinds of elevated privileges" technique, I'd like to get on the same page about implementation before starting.

It looks like the simplest solution would be filter out privileged pages when creating the host permissions list (whiteListedHosts).  My initial reaction is that that seems roundabout, but the alternative would be a much bigger change.  Or am I overlooking something?
Also the list of hosts to which mozAddonManager is exposed is currently buried in c++ code, I'll have to get that list up to js, ugh.

Comment 5

[Andrew Swan [:aswan]]

Attachment: Expose mozAddonManager allowed hosts to chrome

MozReview-Commit-ID: Hp82unF8h4v

Comment 6

[Andrew Swan [:aswan]]

Attachment: Expose mozAddonManager allowed hosts to chrome

Comment 7

[Andrew Swan [:aswan]]

Attachment: Don't allow content scripts on pages with mozAddonManager

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

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

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

A few nits, but overall looks reasonable.

::: dom/webidl/AddonManager.webidl
@@ +81,5 @@
>  };
> +
> +[ChromeOnly,Exposed=System,HeaderFile="mozilla/AddonManagerWebAPI.h"]
> +interface AddonManagerPermissions {
> +  static boolean isHostPermitted(DOMString host);

Is there any reason not to put this static on AddonManager directly?  Do you need it to be available when IsAPIEnabled() is false or something?

::: toolkit/mozapps/extensions/AddonManagerWebAPI.cpp
@@ +18,5 @@
>  namespace mozilla {
>  using namespace mozilla::dom;
>  
> +static bool
> +IsValidHost(const nsACString& host) {

Unless you are exposing this method to other code modules I think its slightly preferred to put it in the anonymous namespace:

  namespace {
    bool
    IsValidHost(const nsACString& aHost) {
      // ...
    }
  } // anonymous namespace

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

I think this can just be `return IsValidHost(host);`

@@ +67,3 @@
>      return true;
>    }
> +  

nit: trailing white space

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8793568 [details] [diff] [review]
Don't allow content scripts on pages with mozAddonManager

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

::: toolkit/components/extensions/ExtensionContent.jsm
@@ +129,5 @@
>      let uri = window.document.documentURIObject;
> +
> +    // If mozAddonManager is present on this page, don't allow
> +    // content scripts.
> +    if (AddonManagerPermissions.isHostPermitted(uri.host)) {

As an alternative to this, can you not check for the existence of the AddonManager directly?  Something like:

  if (window.navigator.mozAddonManager instanceof AddonManager)

Comment 10

[Andrew Swan [:aswan]]

(In reply to Ben Kelly [:bkelly] from comment #9)
> ::: toolkit/components/extensions/ExtensionContent.jsm
> @@ +129,5 @@
> >      let uri = window.document.documentURIObject;
> > +
> > +    // If mozAddonManager is present on this page, don't allow
> > +    // content scripts.
> > +    if (AddonManagerPermissions.isHostPermitted(uri.host)) {
> 
> As an alternative to this, can you not check for the existence of the
> AddonManager directly?  Something like:
> 
>   if (window.navigator.mozAddonManager instanceof AddonManager)

Hey, that does sound much better.  To make this work though, I need to expose the AddonManager bindings into chrome code in the content process (I'll also rename the interface to AddonManagerWebAPI or something to avoid colliding with AddonManager from AddonManager.jsm).  I'm a novice with webidl, adding "Exposed=System" or "Exposed=(System,Window") caused this build error:
WebIDL.WebIDLError: error: Interface member has larger exposure set than the interface itself
Can you help me find the right magic for the bit before the AddonManager interface in AddonManager.webidl?

Comment 11

[Kris Maglione [:kmag]]

(In reply to Ben Kelly [:bkelly] from comment #8)
> ::: dom/webidl/AddonManager.webidl
> @@ +81,5 @@
> >  };
> > +
> > +[ChromeOnly,Exposed=System,HeaderFile="mozilla/AddonManagerWebAPI.h"]
> > +interface AddonManagerPermissions {
> > +  static boolean isHostPermitted(DOMString host);
> 
> Is there any reason not to put this static on AddonManager directly?  Do you
> need it to be available when IsAPIEnabled() is false or something?

The AddonManager module is only exposed via `navigator`, which doesn't exist in the JSM scopes we need to access this from.

Comment 12

[Kris Maglione [:kmag]]

Comment on attachment 8793568 [details] [diff] [review]
Don't allow content scripts on pages with mozAddonManager

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

::: toolkit/components/extensions/ExtensionContent.jsm
@@ +5,5 @@
>  "use strict";
>  
>  this.EXPORTED_SYMBOLS = ["ExtensionContent"];
>  
> +/* globals ExtensionContent, AddonManagerPermissions */

This should be added to .eslintrc instead, since it's not specific to this file. Also, please keep globals lists sorted when possible.

@@ +129,5 @@
>      let uri = window.document.documentURIObject;
> +
> +    // If mozAddonManager is present on this page, don't allow
> +    // content scripts.
> +    if (AddonManagerPermissions.isHostPermitted(uri.host)) {

That would work here, but I was expecting this check to happen in MatchPattern.jsm, instead...

If we do go this route, though, I'd rather just check that it's defined, rather than using an `instanceof` check.

::: toolkit/components/extensions/test/mochitest/mochitest.ini
@@ +51,5 @@
>  [test_ext_contentscript_create_iframe.html]
>  [test_ext_contentscript_devtools_metadata.html]
>  [test_ext_contentscript_exporthelpers.html]
>  [test_ext_contentscript_css.html]
> +[test_ext_contentscript_permission.html]

Please move this test to a separate patch so it can be landed after the fixes make it to release.

::: toolkit/components/extensions/test/mochitest/test_ext_contentscript_permission.html
@@ +16,5 @@
> +add_task(function* test_contentscript() {
> +  function background() {
> +    browser.test.onMessage.addListener(msg => {
> +      browser.tabs.executeScript({
> +        code: "true;",

We should probably use a more distinct sentinel here.

@@ +32,5 @@
> +    },
> +    background,
> +  };
> +
> +  let win = window.open("http://example.com/");

We should also check with explicit permissions for the site, and with active activeTab permissions, just to be safe.

@@ +45,5 @@
> +  yield SpecialPowers.pushPrefEnv({
> +    set: [["extensions.webapi.testing", true]],
> +  });
> +
> +  extension.sendMessage("run");

Should probably reload the page between checks, just to be safe.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrew Swan [:aswan] from comment #10)
> Hey, that does sound much better.  To make this work though, I need to
> expose the AddonManager bindings into chrome code in the content process
> (I'll also rename the interface to AddonManagerWebAPI or something to avoid
> colliding with AddonManager from AddonManager.jsm).  I'm a novice with
> webidl, adding "Exposed=System" or "Exposed=(System,Window") caused this
> build error:
> WebIDL.WebIDLError: error: Interface member has larger exposure set than the
> interface itself
> Can you help me find the right magic for the bit before the AddonManager
> interface in AddonManager.webidl?

To be honest I'm not sure how [Exposed] plays with jsm's and whatnot.  You could either stick with your original plan or :bz could probably answer this.  Unfortunately I'm short on time to investigate myself at the moment.

Comment 14

[Kris Maglione [:kmag]]

(In reply to Ben Kelly [Mostly PTO, back Oct 10][:bkelly] from comment #13)
> (In reply to Andrew Swan [:aswan] from comment #10)
> > Hey, that does sound much better.  To make this work though, I need to
> > expose the AddonManager bindings into chrome code in the content process
> > (I'll also rename the interface to AddonManagerWebAPI or something to avoid
> > colliding with AddonManager from AddonManager.jsm).  I'm a novice with
> > webidl, adding "Exposed=System" or "Exposed=(System,Window") caused this
> > build error:
> > WebIDL.WebIDLError: error: Interface member has larger exposure set than the
> > interface itself
> > Can you help me find the right magic for the bit before the AddonManager
> > interface in AddonManager.webidl?
> 
> To be honest I'm not sure how [Exposed] plays with jsm's and whatnot.  You
> could either stick with your original plan or :bz could probably answer
> this.  Unfortunately I'm short on time to investigate myself at the moment.

[Esposed=System] makes things accessible to JSMs. We'd need [Exposed=(System,Window)] on the interface itself for [Exposed=System] to work on the static method. But given the nature of this API, I think we'd be better off not trying to expose the AddonManager interface in JSMs.

Comment 15

[Andrew Swan [:aswan]]

(In reply to Kris Maglione [:kmag] from comment #14)
> [Esposed=System] makes things accessible to JSMs. We'd need
> [Exposed=(System,Window)] on the interface itself for [Exposed=System] to
> work on the static method. But given the nature of this API, I think we'd be
> better off not trying to expose the AddonManager interface in JSMs.

The point was just to expose it to do the instanceof check.  The alternative would be just to check for a mozAddonManager property that != undefined, but then if a page happened to create a property on navigator with that name, that would suppress content scripts.  Maybe we can just live with that?

Comment 16

[Andrew Swan [:aswan]]

Attachment: Don't load content scripts on pages with mozAddonManager

MozReview-Commit-ID: GN4Bc8RZQBn

Comment 17

[Andrew Swan [:aswan]]

Attachment: Test for content scripts with mozAddonManager

MozReview-Commit-ID: BN9HKaHkbuC

Comment 18

[Kris Maglione [:kmag]]

Comment on attachment 8799010 [details] [diff] [review]
Don't load content scripts on pages with mozAddonManager

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

Please remove the summary from this commit before landing, and only include the bug number and reviewer.

Comment 19

[Kris Maglione [:kmag]]

Bumping this to sec-high since, even though this is something legacy add-ons can already do, it is a pretty severe privilege escalation for sandboxed WebExtensions.

Comment 20

[Andrew Swan [:aswan]]

Attachment: 360403

MozReview-Commit-ID: GN4Bc8RZQBn

Comment 21

[Andrew Swan [:aswan]]

Comment on attachment 8799046 [details] [diff] [review]
360403

Carrying over :kmag's r+ from a previous copy of this patch.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Constructing the exploit would be relatively easy, actually carrying out the exploit requires the attacker to trick the victim into installing a malicious extension.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The patch doesn't exactly "paint a bulls-eye" but it does point clearly toward the problem.

Which older supported branches are affected by this flaw?
This problem has existed since mozAddonManager originally shipped in 48.

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?
The patch is simple enough that backports should be trivial.

How likely is this patch to cause regressions; how much testing does it need?
It is unlikely to cause regressions, and it includes tests.

Comment 22

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

sec-approval+ for checkin *without* tests. We don't check in tests for security bugs normally until after the effected code ships to the public. Otherwise, the tests can 0day us.

We'll want Aurora and Beta patches made and nominated as well for going in after trunk is green.

Comment 23

[Kris Maglione [:kmag]]

It occurs to me that this is an issue for Greasemonkey user scripts, too... We should probably reach out to its developers to get it fixed and then blocklist affected versions.

Comment 24

[Kris Maglione [:kmag]]

Also, I think we are going to need to go the host permission checking rout, after all, and add a check to MatchPattern.jsm. It's still possible for extensions to exploit this using the webRequest API, by changing CSP headers and then redirecting a script request from AMO to a malicious third-party.

I'll file a follow-up bug for that. Also adding the leave-open flag to this bug until tests are landed.

Comment 25

[Andrew Swan [:aswan]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/63e401d16993f2b17a69c92eabd385d2f2e256fb
Bug 1295324 r=kmag a=al

Comment 26

[Andrew Swan [:aswan]]

Comment on attachment 8799046 [details] [diff] [review]
360403

Approval Request Comment
[Feature/regressing bug #]:
Security vulnerability with extensions and mozAddonManager

[User impact if declined]:
Malicious extensions may be able to access the mozAddonManager api and use that to install extensions that have greater privileges.

[Describe test coverage new/current, TreeHerder]:
Tests are in a separate patch attached to this bug, which will be embargoed until the fix is widely distributed.

[Risks and why]: 
This patch addresses the vulnerability described in the bug.

[String/UUID change made/needed]:
N/A

Comment 27

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/63e401d16993

Comment 28

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

Comment on attachment 8799046 [details] [diff] [review]
360403

Sec-high, Aurora51+, Beta50+

Comment 29

[Jorge Villalobos [:jorgev] (he/him)]

This is blocking all content scripts from working on domains that expose `mozAddonManager`, right? Other than addons.mozilla.org, what domains are affected? Is there no workaround for someone who wants to write a content script for those domains?

As for the outreach, this would be specifically for add-ons attempting to access `mozAddonManager`, right?

Comment 30

[Kris Maglione [:kmag]]

(In reply to Jorge Villalobos [:jorgev] from comment #29)
> This is blocking all content scripts from working on domains that expose
> `mozAddonManager`, right? Other than addons.mozilla.org, what domains are
> affected?

Yes. At the moment, just testpilot.firefox.com:

http://searchfox.org/mozilla-central/rev/3e03a4064eb585d96f28023785a5c242969878a6/toolkit/mozapps/extensions/AddonManagerWebAPI.cpp#42-44

> Is there no workaround for someone who wants to write a content script for
> those domains?

No, for the moment at least, they'd just have to write a legacy bootstrap
add-on. There's no safe way to load user scripts into those pages.

Hopefully, in the future, we'll be able to add a special WebExtension
permission that requires manual review, but for now, it's not really an
option.

> As for the outreach, this would be specifically for add-ons attempting to
> access `mozAddonManager`, right?

Just for add-ons that load third-party user scripts, like Greasemonkey or
Scriptish, and WebExtensions that currently load user scripts into AMO.

However, we can't do any outreach to the second group in general until *after*
this merges to release. There might be some special cases that we can contact
sooner.

Comment 31

[Jorge Villalobos [:jorgev] (he/him)]

(In reply to Kris Maglione [:kmag] from comment #30)
> Hopefully, in the future, we'll be able to add a special WebExtension
> permission that requires manual review, but for now, it's not really an
> option.

In the interim, dropping content scripts should include a console message to make it clear why this is happening. Maybe this is something that also needs to wait for the merge to release, but at the moment the experience is far from ideal (I noticed that with one of my add-ons on Nightly this morning and had no idea until I saw this bug).

Comment 32

[Kris Maglione [:kmag]]

(In reply to Jorge Villalobos [:jorgev] from comment #31)
> In the interim, dropping content scripts should include a console message to
> make it clear why this is happening. Maybe this is something that also needs
> to wait for the merge to release, but at the moment the experience is far
> from ideal (I noticed that with one of my add-ons on Nightly this morning
> and had no idea until I saw this bug).

Can you file a follow-up (security) bug to add a console message? We might
have to start with a vague message and add more details once the fix is in
release.

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/181a64b1364b
https://hg.mozilla.org/releases/mozilla-beta/rev/e6bc67cc7d76

Comment 34

[Liz Henry (:lizzard) (relman/hg->git project)]

We are planning a 49.0.2 release next week. If you think that it needs to be on 49 asap please request uplift to mozilla-release. If it can wait till 50 is out on Nov. 8th then let's wait.

Comment 35

[Andrew Swan [:aswan]]

I don't feel strongly one way or the other but Al marked this wontfix for 49 in comment 22.

Comment 36

[Chuck Harmston [:chuck]]

> Yes. At the moment, just testpilot.firefox.com:
>
> http://searchfox.org/mozilla-central/rev/3e03a4064eb585d96f28023785a5c242969878a6/toolkit/mozapps/extensions/AddonManagerWebAPI.cpp#42-44

Best I can tell, these changes only apply to WebExtensions. Test Pilot is currently an SDK add-on that uses a PageMods to create a communication channel between our add-on and the website. If that's not affected, we're fine.

It's worth noting that we do hope to move Test Pilot to a WebExtension in the near future (ref #1280233), and this change would be a blocker to that.

Comment 37

[Kris Maglione [:kmag]]

It should be easy enough to add a special permission for internal add-ons so we can ignore this restriction.

Comment 38

[Andrew Swan [:aswan]]

Al, when should the tests for this bug land?  (The patch itself many weeks ago now).  This is also loosely related to bug 1308688

Comment 39

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

(In reply to Andrew Swan [:aswan] from comment #38)
> Al, when should the tests for this bug land?  (The patch itself many weeks
> ago now).  This is also loosely related to bug 1308688

Dan and I discussed this and think it can be checked in after the work week (i.e. week after next). We're hoping for more Firefox 50 uptake before running a risk of revealing the fix.

Comment 40

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce this issue on Firefox 51.0a1 (2016-08-15) under Windows 10 64-bit.

Manually verified this bug using the webextension from Comment 1 and I confirm that the AdBlock Plus is no longer installed on Firefox 53.0a1 (2016-12-01), Firefox 52.0a2 (2016-12-01/02), Firefox 51.0b5 (20161129164126) and Firefox 50.0.2 (20161129173726) under Windows 10 64-bit and Ubuntu 16.-4 32-bit.

Comment 41

[Andrew Swan [:aswan]]

(In reply to Al Billings [:abillings] from comment #39)
> (In reply to Andrew Swan [:aswan] from comment #38)
> > Al, when should the tests for this bug land?  (The patch itself many weeks
> > ago now).  This is also loosely related to bug 1308688
> 
> Dan and I discussed this and think it can be checked in after the work week
> (i.e. week after next). We're hoping for more Firefox 50 uptake before
> running a risk of revealing the fix.

Just a double check, we're now ready to check in the tests for this bug?

Comment 42

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

Yes, I think it is fine.

Comment 43

[Andrew Swan [:aswan]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab439ec85d1556fdeaf5aafb5261bfa452a69f4e
Bug 1295324 Test for content scripts with mozAddonManager r=kmag

Comment 44

[Andrew Swan [:aswan]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7c4457f610d002b6385d6de0ffc7ce0203aac5e
Bug 1295324 Skip test on android r=me a=test-only

Comment 45

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ab439ec85d15
https://hg.mozilla.org/mozilla-central/rev/d7c4457f610d