Closed Bug 1295324 (CVE-2016-9075) Opened 3 years ago Closed 3 years ago

WebExtensions can easily access the mozAddonManager API and use it to gain elevated privileges.

Categories

(WebExtensions :: Untriaged, defect, P1)

51 Branch
defect

Tracking

(firefox49 wontfix, firefox-esr45 unaffected, firefox50+ verified, firefox51+ verified, firefox52+ verified)

VERIFIED FIXED
mozilla52
Tracking Status
firefox49 --- wontfix
firefox-esr45 --- unaffected
firefox50 + verified
firefox51 + verified
firefox52 + verified

People

(Reporter: kmag, Assigned: aswan)

References

(Blocks 1 open bug)

Details

(Keywords: sec-high, Whiteboard: [adv-main50+] triaged)

Attachments

(3 files, 4 obsolete files)

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.
Attached file test-aom.xpi
Assignee: nobody → aswan
Priority: -- → P1
Whiteboard: triaged
(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?
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.
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.
MozReview-Commit-ID: Hp82unF8h4v
Attachment #8793567 - Flags: review?(bkelly)
Attachment #8793500 - Attachment is obsolete: true
Attachment #8793568 - Flags: review?(kmaglione+bmo)
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
Attachment #8793567 - Flags: review?(bkelly) → review+
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)
(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?
Flags: needinfo?(bkelly)
(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 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.
Attachment #8793568 - Flags: review?(kmaglione+bmo)
(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.
Flags: needinfo?(bkelly)
(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.
(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?
Flags: needinfo?(kmaglione+bmo)
Attachment #8793567 - Attachment is obsolete: true
Attachment #8793568 - Attachment is obsolete: true
MozReview-Commit-ID: GN4Bc8RZQBn
Attachment #8799010 - Flags: review?(kmaglione+bmo)
MozReview-Commit-ID: BN9HKaHkbuC
Attachment #8799011 - Flags: review?(kmaglione+bmo)
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.
Attachment #8799010 - Flags: review?(kmaglione+bmo) → review+
Attachment #8799011 - Flags: review?(kmaglione+bmo) → review+
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.
Flags: needinfo?(kmaglione+bmo)
Keywords: sec-moderatesec-high
Attached patch 360403Splinter Review
MozReview-Commit-ID: GN4Bc8RZQBn
Attachment #8799010 - Attachment is obsolete: true
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.
Attachment #8799046 - Flags: sec-approval?
Attachment #8799046 - Flags: review+
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.
Attachment #8799046 - Flags: sec-approval? → sec-approval+
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.
Flags: needinfo?(jorge)
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.
Keywords: leave-open
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
Attachment #8799046 - Flags: approval-mozilla-beta?
Attachment #8799046 - Flags: approval-mozilla-aurora?
Comment on attachment 8799046 [details] [diff] [review]
360403

Sec-high, Aurora51+, Beta50+
Attachment #8799046 - Flags: approval-mozilla-beta?
Attachment #8799046 - Flags: approval-mozilla-beta+
Attachment #8799046 - Flags: approval-mozilla-aurora?
Attachment #8799046 - Flags: approval-mozilla-aurora+
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?
Flags: needinfo?(jorge) → needinfo?(kmaglione+bmo)
(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.
Flags: needinfo?(kmaglione+bmo)
(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).
(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.
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.
Flags: needinfo?(aswan)
I don't feel strongly one way or the other but Al marked this wontfix for 49 in comment 22.
Flags: needinfo?(aswan)
Depends on: 1313996
Whiteboard: triaged → [adv-main50+] triaged
Alias: CVE-2016-9075
> 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.
It should be easy enough to add a special permission for internal add-ons so we can ignore this restriction.
Al, when should the tests for this bug land?  (The patch itself many weeks ago now).  This is also loosely related to bug 1308688
Flags: needinfo?(abillings)
Status: NEW → RESOLVED
Closed: 3 years ago
Flags: in-testsuite?
Keywords: leave-open
Resolution: --- → FIXED
(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.
Flags: needinfo?(abillings)
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.
Group: toolkit-core-security → core-security-release
(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?
Flags: needinfo?(abillings)
Yes, I think it is fine.
Flags: needinfo?(abillings) → needinfo?(aswan)
Flags: needinfo?(aswan)
Group: core-security-release
Depends on: 1280237
No longer depends on: 1280237
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.