1398713 - WebExtensions code should pass a triggeringPrincipal when loading content as directed by extension (either from script or a manifest)

Status: RESOLVED FIXED

(WebExtensions :: Frontend, enhancement, P2)

Description

[:Gijs (he/him)]

Follow-up to bug 1380597. We should make sure docshell and co have the requisite information to do their own security checks when we loading content as directed by an extension.

Comment 1

[Andy McKay]

Recommended to fix in 58 and uplift to 57 if it seems stable, some concern that it might break things.

Comment 2

[Andy McKay]

Shane, perhaps comment 1 was a bit optimistic by me, is this still a P2?

Comment 3

[Shane Caraveo (:mixedpuppy)]

The fix that was done in bug 1380597 should be sufficient to address the primary problem.  I take it this would be an optimization.  ie. we probably don't need it in 58, but should get to it for 59/60.

Comment 4

[Shane Caraveo (:mixedpuppy)]

Attachment: add triggeringPrincipal to loadURI calls

The sidebar needed a touch more work to reliably get the extension principal.  However, is this going down the right path for passing through the triggeringPrincipal?  Am I missing any flags that are necessary?

Comment 5

[:Gijs (he/him)]

Comment on attachment 8948053 [details] [diff] [review]
add triggeringPrincipal to loadURI calls

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

Based on the IRC request, f+ for the loadURI-to-loadURIWithOptions stuff, which looks OK at a glance. I can't speak to whether these are all the callsites without a more thorough look.

The sidebar stuff is confusing. I wish we had time to refactor that.

::: browser/base/content/webext-panels.js
@@ +100,5 @@
> +  let addon = await AddonManager.getAddonByID(extensionId);
> +  if (!addon) {
> +    throw new Error(`No such addon for sidebar ${extensionId}`);
> +  }
> +  let extension = GlobalManager.extensionMap.get(addon.id);

This could do with an explanation as to why extensionId != addon.id and why we have both.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Attachment: add triggeringPrincipal to loadURI calls

handles tab creation now.

There are a couple places in devtools where we're calling loaduri, but I am not certain if we need to provide a triggeringprincipal there, or what the best approach is, ni?rpl

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hi Shane, thanks for the needinfo, I confirm that based on the changes applied by the attached patch to the browser.loadURI calls related to the other Extension pages, it looks that we should also apply the same kind of change to the devtools page and the devtools panel:

- devtools page: https://searchfox.org/mozilla-central/rev/f80722d4f3bfb722c5ec53880c4a7efb71285676/browser/components/extensions/ext-devtools.js#179
- devtools panel: https://searchfox.org/mozilla-central/rev/f80722d4f3bfb722c5ec53880c4a7efb71285676/browser/components/extensions/ext-devtools-panels.js#267

The devtools page is pretty similar to a background page (e.g. both extends HiddenExtensionPage) and it should be changed to:

    this.browser.loadURIWithFlags(this.url, {triggeringPrincipal: this.extension.principal});

And a similar change should be applied to the one from the devtools panel as well:

    browser.loadURIWithFlags(url, {triggeringPrincipal: extension.principal});

These are currently the only extension pages provided by the devtools APIs, there will be a third one for the devtools sidebar
once Bug 1398734 (Support devtools.panels.elements sidebar.setPage API method) has been completed (the patch attached contains an initial prototype for it, I'm currently waiting for some fixes on the devtools sidebar to land before updating it and move it into the review stage, but in the meantime I'm going to update that draft patches asap, just to avoid the risk of forgetting to apply the same changes before landing it).

Comment 8

[Shane Caraveo (:mixedpuppy)]

We should probably also set triggeringPrincipal when aboutNewTabService.newTabURL is controlled by an extension.  I'm not sure if there is a convenient way to handle that.  mstriemer, any thoughts on that?  See the patch here to see what I'm doing.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Attachment: add triggeringPrincipal to loadURI calls

add devtools triggeringprincipal.

I'm still not certain whether we want to add triggering principal for newtab being opened.  The extension is not really "triggering" the open.  There also seems to be multiple code locations newtab may be loaded from.

Comment 10

[Shane Caraveo (:mixedpuppy)]

I think we don't need to worry about newtab.  It has to be a moz-ext url in the first place so we should be fine.

Comment 11

[Shane Caraveo (:mixedpuppy)]

I verified manually that adding the triggeringprincipal is working.  I had to revert the patch from bug 1380597 to do so.  After that, setPopup accepts about:addons, but when attempting to open the action you get:

Security Error: Content at moz-extension://b88825c6-6036-c44a-b3af-74a653bf512b/ may not load or link to about:addons.

And the panel fails to open (or for sidebar is blank).  I'm not sure how to automate a test, or even provide QA steps, on these due to bug 1380597.

Otherwise existing tests pass locally.

Comment 12

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8948788 [details] [diff] [review]
add triggeringPrincipal to loadURI calls

I'll just move this to r?.  Gijs for browser/sidebar related stuff, Aswan for extension specific stuff.  The caveat is that I'm unclear how to test as I mentioned in comment 11.

Comment 13

[Andrew Swan [:aswan]]

I don't really feel qualified to review this.  Kris will be back next week, can it wait until then when he can do the review?

Comment 14

[:Gijs (he/him)]

Comment on attachment 8948788 [details] [diff] [review]
add triggeringPrincipal to loadURI calls

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

This looks sane to me.

> I'm still not certain whether we want to add triggering principal for newtab being opened.
> The extension is not really "triggering" the open. There also seems to be
> multiple code locations newtab may be loaded from. 

Add-ons can set the target for new tab loads, right? How do we sanitize that target? That should basically do a checkloadURI check (either explicitly or when the load happens) to check that the add-on in question has the privileges to load that page (to ensure that it can't set it to chrome://whatever or file:///etc/passwd , etc. etc.).

Comment 15

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to :Gijs from comment #14)
> Add-ons can set the target for new tab loads, right? How do we sanitize that
> target? That should basically do a checkloadURI check (either explicitly or
> when the load happens) to check that the add-on in question has the
> privileges to load that page (to ensure that it can't set it to
> chrome://whatever or file:///etc/passwd , etc. etc.).

For the tabs API the urls are checked using this helper (called `checkLoadURL`, which uses `scriptSecurityManager.checkLoadURIWithPrincipal` internally to check if the url is allowed for the given
extension principal):

- https://searchfox.org/mozilla-central/rev/a5abf843f8fac8530aa5de6fb40e16547bb4a47a/toolkit/components/extensions/ExtensionUtils.jsm#620-642 

Then the `checkLoadURL` helper is used by tabs.create and tabs.update:

- Firefox Desktop
  tabs.create: https://searchfox.org/mozilla-central/rev/a5abf843f8fac8530aa5de6fb40e16547bb4a47a/browser/components/extensions/ext-tabs.js#390-392
  tabs.update: https://searchfox.org/mozilla-central/rev/a5abf843f8fac8530aa5de6fb40e16547bb4a47a/browser/components/extensions/ext-tabs.js#511-513

- Firefox for Android
  tabs.create: https://searchfox.org/mozilla-central/rev/a5abf843f8fac8530aa5de6fb40e16547bb4a47a/mobile/android/components/extensions/ext-tabs.js#250-252
  tabs.update: https://searchfox.org/mozilla-central/rev/a5abf843f8fac8530aa5de6fb40e16547bb4a47a/mobile/android/components/extensions/ext-tabs.js#302-304

As a side note, the existent test cases that tests the invalid urls on tabs.create and tabs.update do not explicitly include any "chrome://" or "file://" yet:

- https://searchfox.org/mozilla-central/rev/a5abf843f8fac8530aa5de6fb40e16547bb4a47a/browser/components/extensions/test/browser/browser_ext_tabs_create_invalid_url.js#56-58
- https://searchfox.org/mozilla-central/rev/a5abf843f8fac8530aa5de6fb40e16547bb4a47a/browser/components/extensions/test/browser/browser_ext_tabs_update_url.js#85-104
- https://searchfox.org/mozilla-central/rev/a5abf843f8fac8530aa5de6fb40e16547bb4a47a/mobile/android/components/extensions/test/mochitest/test_ext_tabs_update_url.html#91-110

Comment 16

[:Gijs (he/him)]

I interpreted :mixedpuppy's remark as being about the "new tab" page, which AIUI webextensions can configure these days. I assume this is unrelated to tabs.create/update.

Comment 17

[Mark Striemer [:mstriemer]]

The schema enforces that the new tab URL is a "strictRelativeURL". This appears to be called once without the extension's URL and once with it. The URL needs to be invalid on its own, and valid relative to the extension's URL. So any absolute URL should fail that check. If it does pass the check then `checkLoadURL()` is called.

"newtab": {"$ref": "ExtensionURL"}
https://searchfox.org/mozilla-central/rev/a5abf843f8fac8530aa5de6fb40e16547bb4a47a/browser/components/extensions/schemas/url_overrides.json#13

{
  "id": "ExtensionURL",
  "type": "string",
  "format": "strictRelativeUrl"
}
https://searchfox.org/mozilla-central/rev/a5abf843f8fac8530aa5de6fb40e16547bb4a47a/toolkit/components/extensions/schemas/manifest.json#375

FORMATS.strictRelativeUrl
https://searchfox.org/mozilla-central/rev/a5abf843f8fac8530aa5de6fb40e16547bb4a47a/toolkit/components/extensions/Schemas.jsm#922

Would fail here (valid context URL)
https://searchfox.org/mozilla-central/rev/a5abf843f8fac8530aa5de6fb40e16547bb4a47a/toolkit/components/extensions/Schemas.jsm#936

Would fail here (invalid context URL)
https://searchfox.org/mozilla-central/rev/a5abf843f8fac8530aa5de6fb40e16547bb4a47a/toolkit/components/extensions/Schemas.jsm#914

If valid, checkLoadURL is called
https://searchfox.org/mozilla-central/rev/a5abf843f8fac8530aa5de6fb40e16547bb4a47a/toolkit/components/extensions/Schemas.jsm#916

Comment 18

[Kris Maglione [:kmag]]

Comment on attachment 8948788 [details] [diff] [review]
add triggeringPrincipal to loadURI calls

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

Sorry for the delay. There are probably some other places we need to worry about, but this is a good start. r=me

::: browser/base/content/webext-panels.js
@@ +11,5 @@
>                                 "resource://gre/modules/ExtensionParent.jsm");
> +XPCOMUtils.defineLazyGetter(this, "GlobalManager", () => {
> +  const {GlobalManager} = ChromeUtils.import("resource://gre/modules/Extension.jsm", {});
> +  return GlobalManager;
> +});

Please use WebExtensionPolicy.getByID instead. I'd rather avoid doing this outside of core WebExtension code as much as possible.

@@ +101,3 @@
>    };
>    getBrowser(sidebar).then(browser => {
> +    browser.loadURIWithFlags(extensionUrl, {triggeringPrincipal: extension.principal});

Please just create a codebase principal from policy.getURL() for now. It might be worth doing something fancier if we ever make the principals fancy again.

::: browser/components/extensions/ext-tabs.js
@@ +515,5 @@
> +            let options = {
> +              flags: updateProperties.loadReplace
> +                      ? Ci.nsIWebNavigation.LOAD_FLAGS_REPLACE_HISTORY
> +                      : Ci.nsIWebNavigation.LOAD_FLAGS_NONE,
> +              triggeringPrincipal: extension.principal,

s/extension/context/

There are places where the principal may have unusual origin attributes, and this might make a difference.

Comment 19

[Shane Caraveo (:mixedpuppy)]

Attachment: pass triggeringPrincipal when using browser.loadURI

Comment 20

[Shane Caraveo (:mixedpuppy)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ebe79fd25f936a2243e3860bc707e1927ffec6d

Comment 21

[Shane Caraveo (:mixedpuppy)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95a6bd5b90452da6eee11eb8ee53ef80c91565c6
Bug 1398713 pass triggeringPrincipal when using browser.loadURI, r=Gijs,kmag

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/95a6bd5b9045

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Too late for 59.

Comment 24

[Boris Zbarsky [:bzbarsky]]

This breaks extensions that try to load about:blank in a tab: that about:blank ends up inheriting the extension principal, with a weird CSP, etc, etc.  See bug 1446251 for one manifestation that results.