1375023 - Have switchToTabHavingURI() provide the correct triggeringPrincipal

Status: RESOLVED FIXED

(Core :: DOM: Security, enhancement, P2)

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1375023_triggeringprincipal_switchtotabhavinguri.patch

Gijs, before we can move on to Bug 1374741 [Have openUILinkIn() provide the correct triggeringPrincipal] we need to fix up callsites of switchToTabHavingURI() which pass the params object on to openLinkIn(). Please note that I've only updated callsites where the second argument [aOpenNew] is true, otherwise OpenUILinkIn() is not called [see 1].

Further, even though I am fairly confident that using systemPrincipal is correct in all of those cases, I still think it's worth the effort to set the triggeringPrincipal where the call actually originates instead of using some fallback mechanism.

What do you think?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6fd78287f62dbb5edbe1667fc429106a20e78bd6

[1] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#8155-8160

Comment 2

[:Gijs (he/him)]

Comment on attachment 8879971 [details] [diff] [review]
bug_1375023_triggeringprincipal_switchtotabhavinguri.patch

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

r=me besides the webextensions bits. I also haven't checked if Services is available in all these contexts, I presume you have? :-)

::: browser/components/extensions/ext-browser.js
@@ +65,5 @@
>    }
>  
>    if (extension.manifest.options_ui.open_in_tab) {
> +    window.switchToTabHavingURI(extension.manifest.options_ui.page, true, {
> +      triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal(),

I don't know if system principal is right here. :aswan should know whether this makes sense for webextensions. I don't know what calls this openOptionsPage api.

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8879971 [details] [diff] [review]
bug_1375023_triggeringprincipal_switchtotabhavinguri.patch

Andrew, I know you are currenlty not accepting review requests, but since Gijs suggested you, I am setting a ni? with the hope you can take a minute to review ext-browser.js? To provide some backround info: Ultimately we would like to know what triggered a load to happen, hence we are passing a triggeringPrincipal all the way from JS frontend land to docshell code, so we can then set the correct triggeringprincial in the loadinfo of a channel and reason about security.

Question is: Is extension.manifest.options_ui.page provided by our code, meaning that it can't be influenced by the web? If so, then using SystemPrincipal as the triggeringprincipal is fine. If however, that URI can be set by the web, then we have to go back and find a different principal. thanks!

Comment 4

[Andrew Swan [:aswan]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #3)
> Question is: Is extension.manifest.options_ui.page provided by our code,
> meaning that it can't be influenced by the web? If so, then using
> SystemPrincipal as the triggeringprincipal is fine. If however, that URI can
> be set by the web, then we have to go back and find a different principal.
> thanks!

This value is derived from the contents of a webextension manifest but modulo bugs in the webextensions code, it should always be a moz-extension: URL.  I'm not sure if that answers your question?  Regardless, I just got back from PTO, happy to answer any further questions and I should be quicker to respond now.

Comment 5

[Frederik Braun [:freddy]]

Christoph asked me to comment on potential security risks. It makses a lot of sense to test the URL lives in the respective namespace of the extension (and not any other extension). Unless we already ensure that during add-on load (but please convince me with code, that this is the case).

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Frederik Braun [:freddyb] from comment #5)
> Christoph asked me to comment on potential security risks. It makses a lot
> of sense to test the URL lives in the respective namespace of the extension
> (and not any other extension). Unless we already ensure that during add-on
> load (but please convince me with code, that this is the case).

Andrew, are we doing any kind of validation for extension.manifest.options_ui.page? I am a little worried that a malicious add-on could provide a malicious URL (Is that happening here [1]?) and could potentially load local files. If that is the case, then I suppose we are already facing that problem in our current code and need to fix it. As a first start I think we need to make sure that we are always load a moz-extension: URI if that is the purpose of that function.

[1] https://developer.mozilla.org/en-US/Add-ons/WebExtensions/manifest.json/options_ui

Comment 7

[Andrew Swan [:aswan]]

(In reply to Frederik Braun [:freddyb] from comment #5)
> Christoph asked me to comment on potential security risks. It makses a lot
> of sense to test the URL lives in the respective namespace of the extension
> (and not any other extension). Unless we already ensure that during add-on
> load (but please convince me with code, that this is the case).

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #6)
> Andrew, are we doing any kind of validation for
> extension.manifest.options_ui.page?

Like I said in comment 4, it should always be a moz-extension URL for the extension that is calling openOptionsPage().  The validation code is a little spread out but the data originally comes from the field in the manifest that Christoph cited in comment 6.

The code that validates the manifest applies the rule here:
http://searchfox.org/mozilla-central/rev/ae94cfb36d804727dfb38f2750bfcaab4621df6e/toolkit/components/extensions/schemas/manifest.json#133
which references a type called "ExtensionURL" defined here:
http://searchfox.org/mozilla-central/rev/ae94cfb36d804727dfb38f2750bfcaab4621df6e/toolkit/components/extensions/schemas/manifest.json#259-263

The use of "strictRelativeUrl" causes this code to be run when the manifest is parsed:
http://searchfox.org/mozilla-central/rev/ae94cfb36d804727dfb38f2750bfcaab4621df6e/toolkit/components/extensions/Schemas.jsm#842-851
That code should filter out anything that isn't just a relative path.  Finally, a complete moz-extension: url is built using that relative path here:
http://searchfox.org/mozilla-central/rev/ae94cfb36d804727dfb38f2750bfcaab4621df6e/toolkit/components/extensions/Schemas.jsm#824-831

Comment 8

[Christoph Kerschbaumer [:ckerschb]]

Paul and Freddy suggested that the triggeringprincipal could be the extension principal, which sounds reasonable to me actually. Because I guess the extension principal is really triggering the load here. Does that sounds reasonable? If so, how can I query that extension principal. Is it extension.principal?

Comment 9

[Andrew Swan [:aswan]]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #8)
> Paul and Freddy suggested that the triggeringprincipal could be the
> extension principal, which sounds reasonable to me actually. Because I guess
> the extension principal is really triggering the load here. Does that sounds
> reasonable? If so, how can I query that extension principal. Is it
> extension.principal?

Yes to both.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1375023_triggeringprincipal_switchtotabhavinguri.patch

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8880718 [details] [diff] [review]
bug_1375023_triggeringprincipal_switchtotabhavinguri.patch

Thanks Andrew, can you please sign off on it and then this is ready to go. thanks for your help

Comment 12

[Andrew Swan [:aswan]]

Comment on attachment 8880718 [details] [diff] [review]
bug_1375023_triggeringprincipal_switchtotabhavinguri.patch

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

I just looked at the changes in browser/components/extensions, which look good.
It looks like Gijs already reviewed the rest of the patch?

Comment 13

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Andrew Swan [:aswan] from comment #12)
> It looks like Gijs already reviewed the rest of the patch?

Yes, Gijs reviewed the other parts. Let's make sure everything is fine before landing this:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae21e6c3e9ee966081cf5bf2078f946ed7947834

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62c9b3a66c74
Have switchToTabHavingURI() provide the correct triggeringPrincipal. r=gijs,aswan

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/62c9b3a66c74