Kris Maglione [:kmag] Reporter
	Description • 8 years ago

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.

	Andrew Swan [:aswan] Assignee
	Comment 2 • 8 years ago

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

	Kris Maglione [:kmag] Reporter
	Comment 3 • 8 years ago

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.

	Andrew Swan [:aswan] Assignee
	Comment 4 • 8 years ago

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.

	Andrew Swan [:aswan] Assignee
	Comment 5 • 8 years ago

MozReview-Commit-ID: Hp82unF8h4v

	Ben Kelly [:bkelly, not reviewing]
	Comment 8 • 8 years ago

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

	Ben Kelly [:bkelly, not reviewing]
	Comment 9 • 8 years ago

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)

	Andrew Swan [:aswan] Assignee
	Comment 10 • 8 years ago

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

	Kris Maglione [:kmag] Reporter
	Comment 11 • 8 years ago

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

	Kris Maglione [:kmag] Reporter
	Comment 12 • 8 years ago

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.

	Ben Kelly [:bkelly, not reviewing]
	Comment 13 • 8 years ago

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

	Kris Maglione [:kmag] Reporter
	Comment 14 • 8 years ago

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

	Andrew Swan [:aswan] Assignee
	Comment 15 • 8 years ago

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

	Andrew Swan [:aswan] Assignee
	Comment 16 • 8 years ago

MozReview-Commit-ID: GN4Bc8RZQBn

	Andrew Swan [:aswan] Assignee
	Comment 17 • 8 years ago

MozReview-Commit-ID: BN9HKaHkbuC

	Kris Maglione [:kmag] Reporter
	Comment 18 • 8 years ago

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.

	Kris Maglione [:kmag] Reporter
	Comment 19 • 8 years ago

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.

	Andrew Swan [:aswan] Assignee
	Comment 20 • 8 years ago

MozReview-Commit-ID: GN4Bc8RZQBn

	Andrew Swan [:aswan] Assignee
	Comment 21 • 8 years ago

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.

	Al Billings [:abillings - ex-MoCo]
	Comment 22 • 8 years ago

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.

	Kris Maglione [:kmag] Reporter
	Comment 23 • 8 years ago

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.

	Kris Maglione [:kmag] Reporter
	Comment 24 • 8 years ago

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.

	Andrew Swan [:aswan] Assignee
	Comment 25 • 8 years ago

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

	Andrew Swan [:aswan] Assignee
	Comment 26 • 8 years ago

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

	Phil Ringnalda (:philor)
	Comment 27 • 8 years ago

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

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 28 • 8 years ago

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

Sec-high, Aurora51+, Beta50+

	Jorge Villalobos [:jorgev] (he/him)
	Comment 29 • 8 years ago

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?

	Kris Maglione [:kmag] Reporter
	Comment 30 • 8 years ago

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

	Jorge Villalobos [:jorgev] (he/him)
	Comment 31 • 8 years ago

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

	Kris Maglione [:kmag] Reporter
	Comment 32 • 8 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 33 • 8 years ago

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

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 34 • 8 years ago

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.

	Andrew Swan [:aswan] Assignee
	Comment 35 • 8 years ago

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

	Chuck Harmston [:chuck]
	Comment 36 • 8 years ago

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

	Kris Maglione [:kmag] Reporter
	Comment 37 • 8 years ago

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

	Andrew Swan [:aswan] Assignee
	Comment 38 • 8 years ago

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

	Al Billings [:abillings - ex-MoCo]
	Comment 39 • 8 years ago

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

	Vasilica Tamas, QA [:vtamas]
	Comment 40 • 8 years ago

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.

	Andrew Swan [:aswan] Assignee
	Comment 41 • 8 years ago

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

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

Yes, I think it is fine.

	Andrew Swan [:aswan] Assignee
	Comment 43 • 8 years ago

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

	Andrew Swan [:aswan] Assignee
	Comment 44 • 8 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 45 • 8 years ago

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