Tom S [:evilpie] Assignee
	Description • 7 years ago

Currently management.get doesn't check the addon's type at all. This allows for example to retrieve information about installed language packs.

browser.management.get("af-ZA@dictionaries.addons.mozilla.org")

I think this a quite minor leak, feel free to unrestrict.

	Andy McKay
	Comment 1 • 7 years ago

I suppose that it would increase fingerprinting possibilities and should limit down to just WebExtensions? I would probably unrestrict this, don't think its a security bug.

	Tom S [:evilpie] Assignee
	Comment 2 • 7 years ago

I am not quite sure how to test this? Can we install language packs?

	Shane Caraveo (:mixedpuppy)
	Comment 3 • 7 years ago

(In reply to Tom Schuster [:evilpie] from comment #2)
> Created attachment 8912917 [details] [diff] [review]
> Restrict addon types for management.get
> 
> I am not quite sure how to test this? Can we install language packs?

You should be able to create a simple experiment extension to install and test with, experiments also should not show up here I think.

	:Gijs (he/him)
	Comment 4 • 7 years ago

Unrestricting per comment #0 and comment 1.

	Tom S [:evilpie] Assignee
	Comment 5 • 7 years ago

I tried testing this with a experiment extension, but it turns out those just have an addon type of "extension". So we probably  need something else to test, or should block them with some additional check as well.

	Tom S [:evilpie] Assignee
	Comment 6 • 7 years ago

I added some explicit error throwing to management.get. The test doesn't work as per above.

	Tom S [:evilpie] Assignee
	Comment 7 • 7 years ago

Do we really want to block API extensions? Anyway I implemented that and the test works now. I also centralized the check to one function.

	Shane Caraveo (:mixedpuppy)
	Comment 8 • 7 years ago

Comment on attachment 8913002 [details] [diff] [review]
Check for disallowed addon types in management API


>@@ -160,26 +163,30 @@ const getManagementListener = (extension
> 
> this.management = class extends ExtensionAPI {
>   getAPI(context) {
>     let {extension} = context;
>     return {
>       management: {
>         async get(id) {
>           let addon = await AddonManager.getAddonByID(id);
>-          if (!addon.isSystem) {
>-            // If the extension is enabled get it and use it for more data.
>-            let ext = addon.isWebExtension && GlobalManager.extensionMap.get(addon.id);
>-            return getExtensionInfoForAddon(ext, addon);
>+          if (!addon) {
>+            throw new ExtensionError(`No such addon ${id}`);
>           }
>+          if (!checkAllowedAddon(addon)) {
>+            throw new ExtensionError("get not allowed for this addon");
>+          }

These errors look like they add potential fingerprinting sources.  Why do we need to throw anything here?

>+          // If the extension is enabled get it and use it for more data.
>+          let ext = addon.isWebExtension && GlobalManager.extensionMap.get(addon.id);

I think we can drop the "addon.isWebExtension", probably from all the api.  Or at a minimum check it in checkAllowedAddon.

>+          return getExtensionInfoForAddon(ext, addon);
>         },

>diff --git a/toolkit/mozapps/extensions/internal/XPIProvider.jsm b/toolkit/mozapps/extensions/internal/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/internal/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/internal/XPIProvider.jsm
>@@ -5327,16 +5327,21 @@ AddonWrapper.prototype = {

>+  get isAPIExtension() {
>+    dump(`type: ${addonFor(this).type}\n`);
>+    return addonFor(this).type == "apiextension";
>+  },
>+

dont dump.

	Shane Caraveo (:mixedpuppy)
	Comment 9 • 7 years ago

(In reply to Shane Caraveo (:mixedpuppy) from comment #8)

> >+          if (!addon) {
> >+            throw new ExtensionError(`No such addon ${id}`);
> >           }
> >+          if (!checkAllowedAddon(addon)) {
> >+            throw new ExtensionError("get not allowed for this addon");
> >+          }
> 
> These errors look like they add potential fingerprinting sources.  Why do we
> need to throw anything here?

on second cup o coffee I think this doesn't matter, but probably still not worth throwing errors for.

	Shane Caraveo (:mixedpuppy)
	Comment 10 • 7 years ago

(In reply to Shane Caraveo (:mixedpuppy) from comment #9)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #8)
> 
> > >+          if (!addon) {
> > >+            throw new ExtensionError(`No such addon ${id}`);
> > >           }
> > >+          if (!checkAllowedAddon(addon)) {
> > >+            throw new ExtensionError("get not allowed for this addon");
> > >+          }

Sorry for the goose chase.  These are fine, just look at the other items I mentioned.

	Tom S [:evilpie] Assignee
	Comment 11 • 7 years ago

So I guess only allowing WebExtensions is fine, because on 57 everything is a WebExtension, or it's already a system addon?

	Tom S [:evilpie] Assignee
	Comment 12 • 7 years ago

Mhm, I should test if calling GlobalManager.extensionMap.get on non-extensions is actually fine? I guess we have really have three cases to consider: webextensions, webextension themes and other themes.

	Pulsebot
	Comment 13 • 7 years ago

Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/33ef79c5fe06
Limit management.get to allowed addon types. r=mixedpuppy

	Tom S [:evilpie] Assignee
	Comment 14 • 7 years ago

Actually this is fine, because only webextensions are included GlobalManager.extensionMap, and this doesn't hurt themes.
I am going to prepare a combined patch with bug 1403723 for uplift.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 15 • 7 years ago

Backed out for failing eslint at toolkit/components/extensions/test/xpcshell/test_ext_experiments.js:162: 'boringAddon' is already declared in the upper scope:

https://hg.mozilla.org/integration/mozilla-inbound/rev/766866efab697e5dd1e55b7f7a9329a5d17e6ace

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=33ef79c5fe06fb8aee5ca32fd700b5a5091899ff&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=134907593&repo=mozilla-inbound

 TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/extensions/test/xpcshell/test_ext_experiments.js:162:11 | 'boringAddon' is already declared in the upper scope. (no-shadow)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/extensions/test/xpcshell/test_ext_experiments.js:167:13 | 'addon' is assigned a value but never used. (no-unused-vars)
TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/toolkit/components/extensions/test/xpcshell/test_ext_experiments.js:167:13 | 'addon' is already declared in the upper scope. (no-shadow)

	Pulsebot
	Comment 16 • 7 years ago

Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f0577a6a7102
Limit management.get to allowed addon types. r=mixedpuppy

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 17 • 7 years ago
bugherder

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

	Tom S [:evilpie] Assignee
	Comment 18 • 7 years ago

Combined with bug 1403723.

	Tom S [:evilpie] Assignee
	Comment 19 • 7 years ago

Comment on attachment 8916041 [details]
Combined patch for mozilla-beta

Approval Request Comment
[Feature/Bug causing the regression]: bug 1282981
[User impact if declined]: Problems with WebExtensions, minor fingerprinting risk
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Has been on Nightly for two days
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None (bug 1403723 is already integrated)
[Is the change risky?]: No
[Why is the change risky/not risky?]: This extension API is not widely used
[String changes made/needed]:

	Shane Caraveo (:mixedpuppy)
	Comment 20 • 7 years ago

combined patch looks ok.  My personal taste would lean towards uplifting both bugs separately, but it's not a big difference.

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

Comment on attachment 8916041 [details]
Combined patch for mozilla-beta

Recent problem and must fix as 57 only supports webext, let's uplift

	Ryan VanderMeulen [:RyanVM]
	Comment 22 • 7 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/870135d74339

	marius.santa
	Comment 23 • 7 years ago

Is manual testing required on this bug? If yes, please provide some STR and the proper webextension(if required) or set the “qe-verify-“ flag.

	Tom S [:evilpie] Assignee
	Comment 24 • 7 years ago

Manual testing isn't required, this is covered by automatic tests and there never was a test extension.