Need to check permissions for programmatic content script injection

RESOLVED FIXED in Firefox 45

Status

()

Toolkit
WebExtensions: Untriaged
P1
normal
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: billm, Assigned: kmag, Mentored)

Tracking

({dev-doc-complete})

unspecified
mozilla45
dev-doc-complete
Points:
---
Dependency tree / graph
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox45 fixed)

Details

(Whiteboard: [permissions])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Reporter)

Description

2 years ago
Somehow I forgot to implement this.
https://developer.chrome.com/extensions/content_scripts#pi
(Reporter)

Updated

2 years ago
Priority: -- → P1
(Reporter)

Updated

2 years ago
Mentor: gkrizsanits@mozilla.com

Updated

2 years ago
Whiteboard: [permissions]

Updated

2 years ago
Blocks: 1214433

Updated

2 years ago
Flags: blocking-webextensions+
Depends on: 1227462
Blocks: 1227460
Blocks: 1227462
No longer depends on: 1227462
Blocks: 1227453
Blocks: 1227451
(Reporter)

Updated

2 years ago
Duplicate of this bug: 1227460
Added a not to https://developer.mozilla.org/en-US/Add-ons/WebExtensions/Chrome_incompatibilities
Keywords: dev-doc-needed

Updated

2 years ago
Assignee: nobody → kmaglione+bmo
Iteration: --- → 45.3 - Dec 14
Created attachment 8693979 [details]
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
Attachment #8693979 - Flags: review?(wmccloskey)
Created attachment 8693980 [details]
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
Attachment #8693980 - Flags: review?(wmccloskey)
Created attachment 8693981 [details]
MozReview Request: Bug 1190688: Part 1 - [webext] Implement the activeTab permission. r?billm

Bug 1190688: Part 1 - [webext] Implement the activeTab permission. r?billm
Attachment #8693981 - Flags: review?(wmccloskey)
Created attachment 8693982 [details]
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
Attachment #8693982 - Flags: review?(wmccloskey)
Hm. I guess reviewboard isn't smart enough to handle multiple bugs in one push properly.
(Reporter)

Comment 8

2 years ago
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
Attachment #8693979 - Flags: review?(wmccloskey) → review+
(Reporter)

Comment 9

2 years ago
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.
Attachment #8693980 - Flags: review?(wmccloskey) → review+
(Reporter)

Comment 10

2 years ago
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
Attachment #8693981 - Flags: review?(wmccloskey) → review+
(Reporter)

Comment 11

2 years ago
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.
Attachment #8693982 - Flags: review?(wmccloskey) → review+
(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.
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

2 years ago
Backout:
https://hg.mozilla.org/integration/fx-team/rev/de8bc7c996fe
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b54cce972f8c
https://hg.mozilla.org/mozilla-central/rev/efe43efb067a
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
(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.
Keywords: dev-doc-needed → dev-doc-complete

Comment 18

a year ago
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
(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

a year ago
(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.
You need to log in before you can comment on or make changes to this bug.