1280235 - Allow APIs to be locked down

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P2)

Description

[Andy McKay]

If we want to allow telemetry access, we'll need to limit that down to Mozilla only. I believe Google does this by having an allow list of acceptable add-on ids. There might be a better way, I think Kris has some ideas around this.

Comment 1

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Hey Kris - Do you have a strategy in mind for locking down Mozilla-internal APIs? I'd be game to work on a patch, if you're willing to mentor (or can find someone to mentor).

Comment 2

[Kris Maglione [:kmag]]

This requires that we start signing system and Test Pilot extensions with special certificates with a particular OU. I don't know how far along that effort is, at this point. Andrew might.

Comment 3

[Andy McKay]

I do and we have regular meetings with Cory and Wil about the signing. We need to get the signing done first then we can proceed on with locking them down. I will ping you on IRC.

Comment 4

[Andrew Swan [:aswan]]

Attachment: Bug 1280235 Part 1 remove createAddonDetails from XPIProvider

Review commit: https://reviewboard.mozilla.org/r/188616/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188616/

Comment 5

[Andrew Swan [:aswan]]

Attachment: Bug 1280235 Part 2 Allow webextensions apis to be limited to "Mozilla Extensions" signed extensions

I'm not very happy with this but here it is for your feedback.
I also considered adding a new property to individual schema entry types
called "requiresPrivileged" or something instead of hijacking a special
permission string but that was going to make some already very messy code
even more messy.  Maybe its so messy already that the extra mess wouldn't
be noticeable...

Review commit: https://reviewboard.mozilla.org/r/188618/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/188618/

Comment 6

[Kris Maglione [:kmag]]

Comment on attachment 8917686 [details]
Bug 1280235 Part 2 Allow webextensions apis to be limited to "Mozilla Extensions" signed extensions

https://reviewboard.mozilla.org/r/188618/#review194260

Let's just add a "mozillaAddons" permission and check the privileged flag before we accept it, the way we do for "geckoProfiler"

Comment 7

[Andrew Swan [:aswan]]

Comment on attachment 8917686 [details]
Bug 1280235 Part 2 Allow webextensions apis to be limited to "Mozilla Extensions" signed extensions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188618/diff/1-2/

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8917685 [details]
Bug 1280235 Part 1 remove createAddonDetails from XPIProvider

https://reviewboard.mozilla.org/r/188616/#review194646

Comment 9

[Kris Maglione [:kmag]]

Comment on attachment 8917686 [details]
Bug 1280235 Part 2 Allow webextensions apis to be limited to "Mozilla Extensions" signed extensions

https://reviewboard.mozilla.org/r/188618/#review194648

::: toolkit/components/extensions/Extension.jsm:1220
(Diff revision 2)
>  
> +  async _parseManifest() {
> +    let manifest = await super.parseManifest();
> +    if (manifest.permissions.has("mozillaAddons") &&
> +        this.addonData.addon.signedState !== AddonManager.SIGNEDSTATE_PRIVILEGED) {
> +      manifest.permissions.delete("mozillaAddons");

We should emit a warning here, too.

::: toolkit/components/extensions/Extension.jsm:1222
(Diff revision 2)
> +    let manifest = await super.parseManifest();
> +    if (manifest.permissions.has("mozillaAddons") &&
> +        this.addonData.addon.signedState !== AddonManager.SIGNEDSTATE_PRIVILEGED) {
> +      manifest.permissions.delete("mozillaAddons");
> +      let i = manifest.manifest.permissions.indexOf("mozillaAddons");
> +      if (i >= 0) {

This is always true.

::: toolkit/components/extensions/test/xpcshell/test_ext_schemas_privileged.js:43
(Diff revision 2)
> +      paths: [["privileged"]],
> +    },
> +  };
> +
> +  const catMan = Cc["@mozilla.org/categorymanager;1"].getService(Ci.nsICategoryManager);
> +  catMan.addCategoryEntry("webextension-modules", "test",

s/test/&-privileged/

::: toolkit/components/extensions/test/xpcshell/test_ext_schemas_privileged.js:46
(Diff revision 2)
> +  AddonTestUtils.init(global);
> +  AddonTestUtils.overrideCertDB();

Nit: Please do these at the top-level.

::: toolkit/components/extensions/test/xpcshell/test_ext_schemas_privileged.js:59
(Diff revision 2)
> +      manifest: {
> +        applications: {gecko: {id: "[e-mail]"}},
> +        permissions: ["mozillaAddons"],
> +      },
> +      background() {
> +        browser.test.sendMessage("result", browser.privileged !== undefined);

Nit: `browser.privileged instanceof Object`, or something, please.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:4464
(Diff revision 2)
>        let params = {
>          id: aAddon.id,
>          version: aAddon.version,
>          installPath: aFile.clone(),
> -        resourceURI: getURIForResourceInFile(aFile, "")
> +        resourceURI: getURIForResourceInFile(aFile, ""),
> +        addon: aAddon,

We should probably just pass the signed state here, or something. aAddon is usually an internal add-on object, of some sort or other, and the exact type will change from call to call. That's probably not a huge deal now that legacy add-ons are gone, but I still don't think we want to expose them externally.

Comment 10

[Andrew Swan [:aswan]]

Comment on attachment 8917686 [details]
Bug 1280235 Part 2 Allow webextensions apis to be limited to "Mozilla Extensions" signed extensions

https://reviewboard.mozilla.org/r/188618/#review194648

> This is always true.

You mean if it is in manifest.permissions that it also has to be in manifest.manifest.permissions?  I think the chances are slim but some future regression could upset the conditions that cause it to be true but that in that case just throwing here would be better than the alternative (splice()ing the last element off manifest.manifest.permissions!)
Are you proposing something different here?

Comment 11

[Andrew Swan [:aswan]]

Comment on attachment 8917685 [details]
Bug 1280235 Part 1 remove createAddonDetails from XPIProvider

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188616/diff/1-2/

Comment 12

[Andrew Swan [:aswan]]

Comment on attachment 8917686 [details]
Bug 1280235 Part 2 Allow webextensions apis to be limited to "Mozilla Extensions" signed extensions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188618/diff/2-3/

Comment 13

[Andrew Swan [:aswan]]

Comment on attachment 8917686 [details]
Bug 1280235 Part 2 Allow webextensions apis to be limited to "Mozilla Extensions" signed extensions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188618/diff/3-4/

Comment 14

[Andrew Swan [:aswan]]

Comment on attachment 8917686 [details]
Bug 1280235 Part 2 Allow webextensions apis to be limited to "Mozilla Extensions" signed extensions

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/188618/diff/4-5/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/71ea8cf2a217
Part 1 remove createAddonDetails from XPIProvider r=kmag
https://hg.mozilla.org/integration/autoland/rev/d4961b5813af
Part 2 Allow webextensions apis to be limited to "Mozilla Extensions" signed extensions r=kmag

Comment 16

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/71ea8cf2a217
https://hg.mozilla.org/mozilla-central/rev/d4961b5813af

Comment 17

[Richard Newman [:rnewman]]

Is this documented anywhere? I can't find it in

https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/permissions

If not, can we get a paragraph somewhere?

Comment 18

[Andrew Swan [:aswan]]

I'm not sure that's the right place to document it, that permission is only usable from extensions that are signed as "Mozilla Extensions" and the "Browser Extensions" section on MDN is focused on cross-browser extension standards.  Where it covers Firefox-specific features, they are still features that are available to all extensions.
If you have an application for this, perhaps we can discuss it via email or IRC?

Comment 19

[Richard Newman [:rnewman]]

(In reply to Andrew Swan [:aswan] from comment #18)

> If you have an application for this, perhaps we can discuss it via email or
> IRC?

I'm mainly hoping that folks who want to expose APIs for use by Mozilla WebExtensions won't have to reach out to individual people, and will be able to discover this capability!

We have some specific experimentation/future work desires around storage and sync, but it would be awesome if I could point engineers to docs rather than people.

Is that a realistic expectation, or is this something we intentionally want to limit awareness of within Mozilla?

Comment 20

[marius.santa]

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

Comment 21

[Andrew Swan [:aswan]]

(In reply to Richard Newman [:rnewman] from comment #17)
> If not, can we get a paragraph somewhere?

Added a note to https://wiki.mozilla.org/Add-ons/InternalSigning

Comment 22

[Richard Newman [:rnewman]]

Thanks, aswan!