Closed Bug 1243602 Opened 4 years ago Closed 4 years ago

browser.* properties are exposed even without permissions

Categories

(WebExtensions :: Untriaged, defect, P2)

defect

Tracking

(firefox47 fixed)

RESOLVED FIXED
mozilla47
Iteration:
47.3 - Mar 7
Tracking Status
firefox47 --- fixed

People

(Reporter: billm, Assigned: mattw, Mentored)

References

Details

(Whiteboard: [triaged][good first bug])

Attachments

(1 file, 5 obsolete files)

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.
Flags: blocking-webextensions+
Priority: -- → P2
Whiteboard: [triaged]
Flags: blocking-webextensions+ → blocking-webextensions?
nice to have - Kris is checking if this is fixed with other patch
Assignee: nobody → kmaglione+bmo
Flags: blocking-webextensions? → blocking-webextensions-
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
Assignee: kmaglione+bmo → nobody
Mentor: kmaglione+bmo
Flags: blocking-webextensions- → blocking-webextensions?
Whiteboard: [triaged] → [triaged][good first bug]
:/ I somehow managed to get midaired on this...
Flags: blocking-webextensions? → blocking-webextensions-
Assignee: nobody → mwein
Attachment #8717702 - Flags: review?(kmaglione+bmo)
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.
Attachment #8717702 - Flags: review?(kmaglione+bmo)
Attachment #8718519 - Flags: review?(kmaglione+bmo)
Attachment #8717702 - Attachment is obsolete: true
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 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.
Attachment #8718519 - Flags: review?(kmaglione+bmo)
Attachment #8719607 - Flags: review?(kmaglione+bmo)
Attachment #8718519 - Attachment is obsolete: true
Attachment #8719608 - Flags: review?(kmaglione+bmo)
Attachment #8719607 - Attachment is obsolete: true
Attachment #8719607 - Flags: review?(kmaglione+bmo)
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.
Attachment #8719608 - Flags: review?(kmaglione+bmo) → review+
Cool, I had some free time so I decided to finish it up early.
Keywords: checkin-needed
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.
Iteration: --- → 47.2 - Feb 22
Flags: needinfo?(mwein)
Iteration: 47.2 - Feb 22 → 47.3 - Mar 7
Attachment #8719608 - Attachment is obsolete: true
Attachment #8723355 - Flags: review+
Keywords: checkin-needed
Attachment #8723355 - Attachment is obsolete: true
Attachment #8723356 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/96732e2e7174
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.