1193837 - Need to check permissions for programmatic content script injection

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

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

Somehow I forgot to implement this.
https://developer.chrome.com/extensions/content_scripts#pi

Comment 2

[Kris Maglione [:kmag]]

Added a not to https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Chrome_incompatibilities

Comment 3

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1193837: Part 1 - Cache the last known inner window ID of remote browsers in parent process. r?billm

Bug 1193837: Part 1 - Cache the last known inner window ID of remote browsers in parent process. r?billm

Comment 4

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1193837: Part 2 - [webext] Enforce host matching and load URI restrictions on tabs.executeScript and insertCSS. r?billm

Bug 1193837: Part 2 - [webext] Enforce host matching and load URI restrictions on tabs.executeScript and insertCSS. r?billm

Comment 5

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1190688: Part 1 - [webext] Implement the activeTab permission. r?billm

Bug 1190688: Part 1 - [webext] Implement the activeTab permission. r?billm

Comment 6

[Kris Maglione [:kmag]]

Attachment: MozReview Request: Bug 1190688: Part 2 - [webext] Add tests for executeScript permission checks. r?billm

Bug 1190688: Part 2 - [webext] Add tests for executeScript permission checks. r?billm

Comment 7

[Kris Maglione [:kmag]]

Hm. I guess reviewboard isn't smart enough to handle multiple bugs in one push properly.

Comment 8

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

Comment on attachment 8693979 [details]
MozReview Request: Bug 1193837: Part 1 - Cache the last known inner window ID of remote browsers in parent process. r?billm

https://reviewboard.mozilla.org/r/26637/#review24225

Comment 9

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

Comment on attachment 8693980 [details]
MozReview Request: Bug 1193837: Part 2 - [webext] Enforce host matching and load URI restrictions on tabs.executeScript and insertCSS. r?billm

https://reviewboard.mozilla.org/r/26639/#review24211

::: browser/components/extensions/ext-tabs.js:493
(Diff revision 1)
> +          matchesHost: extension.whiteListedHosts.serialize(),

We actually already have a copy of whiteListedHosts in the content process, although it's broken. It's in the BrowserExtensionContent's whiteListedHosts property, although I forgot to call the MatchPattern constructor on it to deserialize it. Can you please fix this and just use this copy (perhaps from DocumentManager.executeScript) rather than sending it down each time? I don't see how it could change.

::: browser/components/extensions/ext-tabs.js:501
(Diff revision 1)
> +          if (context.checkLoadURL(url, { allowInheritsPrincipal: true })) {

So this allows us to load content scripts from http and stuff, right? Do you know if Chrome allows that? It seems dangerous since we can't fully audit what the add-on is doing to the page.

::: toolkit/components/extensions/Extension.jsm:228
(Diff revision 1)
> +      flags |= ssm.DISALLOW_SCRIPT_OR_DATA;

It seems like it would make sense to use DISALLOW_INHERIT_PRINCIPAL here.

::: toolkit/components/extensions/ExtensionContent.jsm:117
(Diff revision 1)
> +  this.matches_host_ = new MatchPattern(this.options.matchesHost || null);

If you do the check in DocumentManager.executeScript, then I think we can remove this.

Comment 10

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

Comment on attachment 8693981 [details]
MozReview Request: Bug 1190688: Part 1 - [webext] Implement the activeTab permission. r?billm

https://reviewboard.mozilla.org/r/26641/#review24219

::: browser/components/extensions/ext-tabs.js:489
(Diff revision 1)
> +        if (TabManager.for(extension).hasActiveTabPermission(tab)) {
> +          // If we have the "activeTab" permission for this tab, ignore
> +          // the host whitelist.
> +          options.matchesHost = ["<all_urls>"];
> +        } else {
> +          options.matchesHost = extension.whiteListedHosts.serialize();
> +        }

I guess I see why you did it this way now. I still think it might be cleaner if you just pass down a boolean that skips permission checking.

::: browser/components/extensions/ext-utils.js:274
(Diff revision 1)
> +  // WeakMap[tab => inner-window-id<int>]

Please say a bit more about what purpose this serves. Also, maybe call it activeTabValidity or something.

::: browser/components/extensions/ext-utils.js:406
(Diff revision 1)
> -    if (!window.gBrowser) {
> +// chreates one if it doesn't already exist.

creates

Comment 11

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

Comment on attachment 8693982 [details]
MozReview Request: Bug 1190688: Part 2 - [webext] Add tests for executeScript permission checks. r?billm

https://reviewboard.mozilla.org/r/26643/#review24223

::: browser/components/extensions/test/browser/browser_ext_tabs_executeScript.js:11
(Diff revision 1)
> -    background: function() {
> +    background: "!" + function(contentSetup) {

I think parens would be a lot clearer, but I guess this is kinda cool.

Comment 12

[Kris Maglione [:kmag]]

(In reply to Bill McCloskey (:billm) from comment #9)
> ::: browser/components/extensions/ext-tabs.js:501
> (Diff revision 1)
> > +          if (context.checkLoadURL(url, { allowInheritsPrincipal: true })) {
> 
> So this allows us to load content scripts from http and stuff, right? Do you
> know if Chrome allows that? It seems dangerous since we can't fully audit
> what the add-on is doing to the page.

That's actually been nagging at me. I think we should only allow
scripts from the add-on. I just checked, and Chrome only accepts
relative URLs.

> ::: toolkit/components/extensions/Extension.jsm:228
> (Diff revision 1)
> > +      flags |= ssm.DISALLOW_SCRIPT_OR_DATA;
> 
> It seems like it would make sense to use DISALLOW_INHERIT_PRINCIPAL here.

That's what I originally had, but the IDL suggests that it's now a
deprecated alias for DISALLOW_SCRIPT_OR_DATA. I guess if someone ever
decides to remove it, they'll have to fix existing uses first, though.

> ::: toolkit/components/extensions/ExtensionContent.jsm:117
> (Diff revision 1)
> > +  this.matches_host_ = new MatchPattern(this.options.matchesHost || null);
> 
> If you do the check in DocumentManager.executeScript, then I think we can
> remove this.

I'm worried that might cause problems when we add support for
allFrames and runAt.

(In reply to Bill McCloskey (:billm) from comment #10)
> I guess I see why you did it this way now. I still think it might be cleaner
> if you just pass down a boolean that skips permission checking.

I thought about that, but it passing a list of filters seemed so cheap
relative to injecting a script that I decided not to worry about it,
and it seemed cleaner to have all of the logic in one place.

Comment 13

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/c3307a5ac126e1347fc1cec02e5932544b904d51
Bug 1193837: Part 1 - Cache the last known inner window ID of remote browsers in parent process. r=billm

https://hg.mozilla.org/integration/fx-team/rev/ebe2433705a55c4b91d5726ed81574e3bcb5c6fb
Bug 1193837: Part 2 - [webext] Enforce host matching and load URI restrictions on tabs.executeScript and insertCSS. r=billm

Comment 14

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/de8bc7c996fe

Comment 15

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/fx-team/rev/b54cce972f8c259f0fbaae447c495a17a02f333d
Bug 1193837: Part 1 - Cache the last known inner window ID of remote browsers in parent process. r=billm

https://hg.mozilla.org/integration/fx-team/rev/efe43efb067aedd1a2550aaa3ca21a6bd402afea
Bug 1193837: Part 2 - [webext] Enforce host matching and load URI restrictions on tabs.executeScript and insertCSS. r=billm

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b54cce972f8c
https://hg.mozilla.org/mozilla-central/rev/efe43efb067a

Comment 17

[Will Bamberg [:wbamberg]]

(In reply to Kris Maglione [:kmag] from comment #2)
> Added a not to
> https://developer.mozilla.org/en-US/Add-ons/WebExtensions/
> Chrome_incompatibilities

I've removed this note.

Comment 18

[devan.shah14]

Hello

I am using Firefox extensions (xpi) to access the webpage document which contains something like document.validationObject = {}, note this is an expando that is added to the document, but in the chrome extensions env I am not able to access changes to the document.validationObject which were made. Would this change be related to this issue? Works in FF 38? 

Aside from document expando properties, unable to even access expando properties on elements from chrome env. Was this a security path, is there a workaround?

Thanks

Comment 19

[Will Bamberg [:wbamberg]]

(In reply to devan.shah14 from comment #18)
> Hello
> 
> I am using Firefox extensions (xpi) to access the webpage document which
> contains something like document.validationObject = {}, note this is an
> expando that is added to the document, but in the chrome extensions env I am
> not able to access changes to the document.validationObject which were made.
> Would this change be related to this issue? Works in FF 38? 
> 
> Aside from document expando properties, unable to even access expando
> properties on elements from chrome env. Was this a security path, is there a
> workaround?
> 
> Thanks

It sounds like this is related to "Xray vision".

When code in a privileged scope (e.g. extension code) tries to access objects in a less-privileged scope (e.g. web content) then it sees those objects with "Xray vision", meaning that:

* it doesn't see expandos added by the less-privileged code
* if DOM properties have been redefined by the less-privileged code, it sees the original definition

There's more on this here: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Xray_vision

Xray vision is a security mechanism designed to make it harder for malicious content code to trick privileged code. One way around it is for the privileged code to "waive Xrays" https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Xray_vision#Waiving_Xray_vision.

Comment 20

[devan.shah14]

(In reply to Will Bamberg [:wbamberg] from comment #19)
> (In reply to devan.shah14 from comment #18)
> > Hello
> > 
> > I am using Firefox extensions (xpi) to access the webpage document which
> > contains something like document.validationObject = {}, note this is an
> > expando that is added to the document, but in the chrome extensions env I am
> > not able to access changes to the document.validationObject which were made.
> > Would this change be related to this issue? Works in FF 38? 
> > 
> > Aside from document expando properties, unable to even access expando
> > properties on elements from chrome env. Was this a security path, is there a
> > workaround?
> > 
> > Thanks
> 
> It sounds like this is related to "Xray vision".
> 
> When code in a privileged scope (e.g. extension code) tries to access
> objects in a less-privileged scope (e.g. web content) then it sees those
> objects with "Xray vision", meaning that:
> 
> * it doesn't see expandos added by the less-privileged code
> * if DOM properties have been redefined by the less-privileged code, it sees
> the original definition
> 
> There's more on this here:
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Xray_vision
> 
> Xray vision is a security mechanism designed to make it harder for malicious
> content code to trick privileged code. One way around it is for the
> privileged code to "waive Xrays"
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/
> Xray_vision#Waiving_Xray_vision.

Thanks Will, I implemented XRay_Vision and it works for me.