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.

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

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

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.

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 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.

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

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).

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.

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.

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.

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 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.

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 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.).

(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

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.

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 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.

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

Too late for 59.

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.