1243602 - browser.* properties are exposed even without permissions

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

If you call registerSchemaAPI("foo", "foo", ...), then we're only supposed to create browser.foo if an extension has the "foo" permission. However, it looks like we always create a browser.foo property, but you're only allowed to use functions/events on it if you have the correct permission. This is different from what Chrome does.

Comment 1

[:shell escalante]

nice to have - Kris is checking if this is fixed with other patch

Comment 2

[Kris Maglione [:kmag]]

This should be pretty easy to fix.

I think the easiest thing to do is to add a `shouldInject` callback to `schemaWrapper` in Extension.jsm[1], that takes a namespace and a name, and returns true if `schemaApi[ns]` exists. Then `Schemas.inject` in Schemas.jsm[2] can call that callback before injecting each entry. There will be some special cases, though, particularly extensionTypes, which we don't currently pre-create.

It'll also need tests in tests/xpcshell/test_ext_schemas.js[3] to make sure that the forbidden namespaces/names are not injected.

[1] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/Extension.jsm#348
[2] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/Schemas.jsm#821
[3] https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/test/xpcshell/test_ext_schemas.js

Comment 3

[Kris Maglione [:kmag]]

:/ I somehow managed to get midaired on this...

Comment 4

[Matthew Wein [:mattw]]

Attachment: don't expose browser.* properties that lack required permissions

Comment 5

[Kris Maglione [:kmag]]

Comment on attachment 8717702 [details] [diff] [review]
don't expose browser.* properties that lack required permissions

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

We should also add a mochitest to check that at least one API an extension doesn't have permissions for doesn't appear in the `browser` object.

::: toolkit/components/extensions/Extension.jsm
@@ +399,5 @@
>  
> +          shouldInject(ns, name) {
> +            let specialCases = {
> +              "extensionTypes": true,
> +            }

We should use an array or a Set for this. Also, ideally, it should be created outside of this function, and have a name like `extraApis`.

But, the more I think about it, the more I think we should probably just add an `extensionTypes` property to `schemaApi` containing an empty object.

@@ +400,5 @@
> +          shouldInject(ns, name) {
> +            let specialCases = {
> +              "extensionTypes": true,
> +            }
> +            return (ns in specialCases) || (schemaApi[ns] && schemaApi[ns][name]);

We currently only enforce permissions at the API level, so there's no need to check whether the name is present. Also, not all properties are functions (and some are getters), so they could be present, but false.

It should be enough to check `ns in schemaApis`.

::: toolkit/components/extensions/test/xpcshell/test_ext_schemas.js
@@ +286,5 @@
>      tally("call", ns, name, args);
>    },
>  
> +  shouldInject(ns, name) {
> +    tally("shouldInject", ns, name);

We should add some tests for cases where this returns false, too. Maybe create a couple of new namespaces, explicitly return false for one of them, and make sure only the expected one appears in `root`.

I don't think we need the `tally` call here. It only ever stores one call, and the call order is nondeterministic, so we can't really do anything with it.

Comment 6

[Matthew Wein [:mattw]]

Attachment: don't expose browser.* properties that lack required permissions

Comment 7

[Kris Maglione [:kmag]]

Comment on attachment 8718519 [details] [diff] [review]
don't expose browser.* properties that lack required permissions

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

This looks good. I'd r+ it, but it's going to conflict with bug 1246787, and it's going to be a bit tricky to rebase, so we should wait until that merges to mozilla-central.

::: toolkit/components/extensions/Schemas.jsm
@@ +1146,5 @@
>    inject(dest, wrapperFuncs) {
>      for (let [namespace, ns] of this.namespaces) {
>        let obj = Cu.createObjectIn(dest, {defineAs: namespace});
>        for (let [name, entry] of ns) {
> +        if (wrapperFuncs.shouldInject(namespace)) {

We should still pass the name, even if we're not currently making use of it.

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8718519 [details] [diff] [review]
don't expose browser.* properties that lack required permissions

Can you update this now that bug 1246787 has landed? It should just be a matter of updating the `ns` references to use `path` instead.

Comment 9

[Matthew Wein [:mattw]]

Attachment: don't expose browser.* properties that lack required permissions

Comment 10

[Matthew Wein [:mattw]]

Attachment: don't expose browser.* properties that lack required permissions

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8719608 [details] [diff] [review]
don't expose browser.* properties that lack required permissions

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

Heh. Well, I wasn't expecting you to update it today, but I'll take it.

Comment 12

[Matthew Wein [:mattw]]

Cool, I had some free time so I decided to finish it up early.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/593676d0f08f

Comment 14

[Carsten Book [:Tomcat]]

backed this out in https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=3b8c69102f12 since one of this 2 pushes caused https://treeherder.mozilla.org/logviewer.html#?job_id=7344923&repo=fx-team

Comment 15

[Kris Maglione [:kmag]]

It looks like we're doing the right thing, here. That property isn't available on Chrome without the webRequest permission either, and the test doesn't actually use it, so we should just remove the second argument from that sendMessage call. I think it was just mistakenly copied from the webRequest tests.

Comment 16

[Matthew Wein [:mattw]]

Attachment: don't expose browser.* properties that lack required permissions

Comment 17

[Matthew Wein [:mattw]]

Attachment: don't expose browser.* properties that lack required permissions

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/96732e2e7174

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/96732e2e7174