425153 - It's unsafe to use non-chrome scripts, overlays and bindings

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[moz_bug_r_a4]

Please see bug 418356.

In addition to using mozIJSSubScriptLoader.loadSubScript, there are ways to
load non-chrome something that contains scripts.  Scripts that are loaded in
the following ways do not use implicit XPCNativeWrappers when accessing
content.

a)
var s = document.createElementNS(HTML_NS, "script");
s.src = "file://.../x.js";
document.documentElement.appendChild(s);

b)
// an overlay contains an inline <script> element
document.loadOverlay("file://.../x.xul", null);

c)
document.addBinding(elem, "file://.../x.xml#x");

e.g. Sub-Script/XUL Loader uses document.loadOverlay() with file: url.
http://mozilla.zeniko.ch/userchrome.js.html

Comment 1

[moz_bug_r_a4]

Attachment: testcase - test extension

Comment 2

[moz_bug_r_a4]

Attachment: testcase - html

Steps to reproduce:
1. Install test extension.
2. Click "test" buttons on the top of the browser's toolbox.

Actual Results:

--------------------
 <script> element
--------------------

chrome://ncs-test/content/sub.js
 -> [object XPCNativeWrapper [object HTMLDocument]] / #document
 JS frame :: chrome://ncs-test/content/sub.js :: anonymous :: line 4

chrome://NCS-TEST/content/sub.js
 -> [object XPCNativeWrapper [object HTMLDocument]] / #document
 JS frame :: chrome://ncs-test/content/sub.js :: anonymous :: line 4

CHROME://ncs-test/content/sub.js
 -> [object XPCNativeWrapper [object HTMLDocument]] / #document
 JS frame :: chrome://ncs-test/content/sub.js :: anonymous :: line 4

resource://ncs-test/sub.js
 -> [object HTMLDocument] / xxx
 JS frame :: resource://ncs-test/sub.js :: anonymous :: line 4

file://.../sub.js
 -> [object HTMLDocument] / xxx
 JS frame :: file://.../sub.js :: anonymous :: line 4

data:text/plain,...
 -> [object HTMLDocument] / xxx
 JS frame :: data:text/plain,... :: anonymous :: line 4


--------------------
 overlay
--------------------

chrome://nco-test/content/sub-overlay.xul
 -> [object XPCNativeWrapper [object HTMLDocument]] / #document
 JS frame :: chrome://nco-test/content/sub-overlay.xul :: anonymous :: line 5

chrome://NCO-TEST/content/sub-overlay.xul
 -> [object XPCNativeWrapper [object HTMLDocument]] / #document
 JS frame :: chrome://nco-test/content/sub-overlay.xul :: anonymous :: line 5

CHROME://nco-test/content/sub-overlay.xul
 -> [object XPCNativeWrapper [object HTMLDocument]] / #document
 JS frame :: chrome://nco-test/content/sub-overlay.xul :: anonymous :: line 5

resource://nco-test/sub-overlay.xul
 -> [object HTMLDocument] / xxx
 JS frame :: resource://nco-test/sub-overlay.xul :: anonymous :: line 5

file://.../sub-overlay.xul
 -> [object HTMLDocument] / xxx
 JS frame :: file://.../sub-overlay.xul :: anonymous :: line 5

data:application/vnd.mozilla.xul+xml,...
 -> [object HTMLDocument] / xxx
 JS frame :: data:application/vnd.mozilla.xul+xml,... :: anonymous :: line 5


--------------------
 binding
--------------------

chrome://ncb-test/content/sub.xml#x
 -> [object XPCNativeWrapper [object HTMLDocument]] / #document
 JS frame :: chrome://ncb-test/content/sub.xml :: anonymous :: line 7

chrome://NCB-TEST/content/sub.xml#x
 -> [object XPCNativeWrapper [object HTMLDocument]] / #document
 JS frame :: chrome://ncb-test/content/sub.xml :: anonymous :: line 7

CHROME://ncb-test/content/sub.xml#x
 -> [object XPCNativeWrapper [object HTMLDocument]] / #document
 JS frame :: chrome://ncb-test/content/sub.xml :: anonymous :: line 7

resource://ncb-test/sub.xml#x
 -> [object HTMLDocument] / xxx
 JS frame :: resource://ncb-test/sub.xml :: anonymous :: line 7

file://.../sub.xml#x
 -> [object HTMLDocument] / xxx
 JS frame :: file://.../sub.xml :: anonymous :: line 7

data:text/xml,...
 -> [object HTMLDocument] / xxx
 JS frame :: data:text/xml,... :: anonymous :: line 7

Comment 3

[Brandon Sterne (:bsterne)]

This should block, as adding these further restrictions might break more extensions.

Comment 4

[Mike Schroepfer]

Per comment 3 - moving to list

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Let's try to do this in a dot-release asap.

Comment 6

[Boris Zbarsky [:bzbarsky]]

I'm not sure there's a change we could make here that wouldn't be a serious compat break.  Are we OK with that on branch?  If not, do we have a plan for a non-compat-breaking change?

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Why would it be a "serious" compat break? Are extensions really doing the things mentioned in comment 0 a lot? And if/when they do it, are they really expecting to not get wrappers?

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Why would it be a "serious" compat break? 

Because for some of the things in comment 0 the solution we adopt might have to be not allowing them at all, not just wrapping stuff...  If we're sure we can avoid that, great.  I'm not sure of that.

Comment 9

[Johnny Stenback (:jst)]

The only way I can think of fixing this w/o simply preventing all the cases in comment 0 (which is probably out of the question) is to add code like we did in the subscript loader where we prefix the script filename that we pass to the JS engine in these cases with the chrome:// URI that's doing the load. That way the decision about whether or not to use XPCNativeWrappers is made based on the chrome:// URI initiating these loads and not on URI being loaded.

Seems like that could be done either for RC1 (assuming someone has time to code that up within a few days), or for a dot release.

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Also note that all the examples in comment 0 also works for http urls. I.e. if an extension loads scripts from the authors site or some such.

Comment 11

[Boris Zbarsky [:bzbarsky]]

So using <script src> in chrome would always give the wrong filename?  Or at least when the URI is not a chrome URI?  That's what it would take with that approach to fix this bug...

Comment 12

[Johnny Stenback (:jst)]

In error messages etc you would see the wrong file name, yes.

Comment 13

[Samuel Sidler (old account; do not CC)]

Marking as blocking 1.8.1.15.

Assigning to Johnny since he fixed bug 418356.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

itym 1.8.1.16? at least, that's how it's marked ... if it's not making .15, it won't block 1.9.0.1

Comment 15

[Samuel Sidler (old account; do not CC)]

(In reply to comment #14)
> itym 1.8.1.16? at least, that's how it's marked ... if it's not making .15, it
> won't block 1.9.0.1

I meant 1.8.1.15, but it got pushed since then. Is 1.9.0.1 supposed to be in sync with 2.0.0.15 which is scheduled to go out on July 2? I thought it was supposed to be in sync with 1.8.1.16...

Comment 16

[Johnny Stenback (:jst)]

Not blocking on this one after all.

Comment 17

[Daniel Veditz [:dveditz]]

Pushing this out another release since jst is on vacation.

jst: appealing the blocking-minus decision for 1.9.1 -- we don't know how many extensions are vulnerable because of this.

Comment 18

[Johnny Stenback (:jst)]

Yeah, 1.9.1+.

Comment 19

[Johnny Stenback (:jst)]

Attachment: First stab at a fix, similar to how we've fixed this types of problems in the past.

So this adds more "loading_filename -> loaded_filename" script filename fun, and it also simply prevents XBL w/o wrapper automation from ever loading into a chrome document with wrapper automation. The one case that this doesn't get right is the case where the chrome document isn't loaded from a chrome URL, i.e. a chrome document that comes from a javascript: or data: URI. Not sure what to do about that case. Thoughts?

Comment 20

[Boris Zbarsky [:bzbarsky]]

This seems to cry out for some nsContentUtils code refactoring...

How do we do wrapper automation for documents that come from data:/javascript: right now?  I'd think we don't, right?

Comment 21

[Johnny Stenback (:jst)]

Attachment: Updated fix to be less cut-n-past'y

Same change as above, with more code sharing.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 347676 [details] [diff] [review]
Updated fix to be less cut-n-past'y

No need for the nsContentUtils:: prefix on IsChromeDoc in the nsContentUtils method.  Looks good either way, though.

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 347676 [details] [diff] [review]
Updated fix to be less cut-n-past'y

looks nice

Comment 24

[Johnny Stenback (:jst)]

For the record, I do *not* think it's safe to take this change on the for 2.0 at this time, not w/o baking on the trunk for a while. Extensions could break, and the only way to really test that is to get this out into some large:ish number of people. We're aiming to get this into 3.1b2, but I doubt we'll have enough time to bake this long enough to to feel good about it before we ship 2.0.0.19 :(

Comment 25

[Johnny Stenback (:jst)]

Patch landed, marking FIXED.

Comment 26

[Samuel Sidler (old account; do not CC)]

Marking fixed for real. :)

Comment 27

[Samuel Sidler (old account; do not CC)]

Johnny, how are you coming on branch patches for this bug? (What, you need more than a day? Fer shame! ;) )

Comment 28

[Johnny Stenback (:jst)]

(In reply to comment #27)
> Johnny, how are you coming on branch patches for this bug? (What, you need more
> than a day? Fer shame! ;) )

See comment #24 :(

Comment 29

[Daniel Veditz [:dveditz]]

OK, we can wait for trunk baking.

I'm going to lower the severity to moderate since stock Firefox doesn't do any of the dangerous things AFAIK. If we know of a popular addon that does this it would be critical again.

Comment 30

[Boris Zbarsky [:bzbarsky]]

So this apparently broke Ubiquity.  That's expected: they're loading a chrome:// URI in a content docshell (a tab), then loading resource:// scripts from that chrome page.  With this patch those scripts get XPCNativeWrapper protection, and since the docshell is content they get XPCNativeWrappers for their own DOM objects.  The resource:// script is some off-the-shelf JS library that tries to use delete to remove an expando property on one of those DOM objects.  But XPC_NW_DelProperty throws NS_ERROR_XPC_SECURITY_MANAGER_VETO on such attempts.

Comment 31

[Boris Zbarsky [:bzbarsky]]

Oh, and the point is that we should document this change from 3.0 for add-on developers.

Comment 32

[Christopher Blizzard (:blizzard)]

Another one for the 3.1 extensions docs.

Comment 33

[Daniel Veditz [:dveditz]]

Moving this to 1.9.0.7 (at least) so we get a better idea what the 3.1 change has broken besides Ubiquity.

Comment 34

[Samuel Sidler (old account; do not CC)]

And, while we get that better idea, Johnny, can you work up 1.9.0 and 1.8.1 patches?

Comment 35

[Daniel Veditz [:dveditz]]

Using a partial scan of addons that's a couple months old

document.addBinding() was only used by AdBlockPlus, and it used a chrome URI. 

document.loadOverlay() was used quite a bit. There were no loads of file: or resource: explicitly, but quite a few load variables instead of literal strings and I haven't checked those but I'm feeling OK about this one.

But lots and lots of 'em create script elements :-(

And as noted in comment 0, not all addons are hosted on AMO and my search won't find any like userChromeJS

Comment 36

[Samuel Sidler (old account; do not CC)]

Based on comment 35, my gut feeling is that we don't take this in 1.9.0 at all and that we *definitely* don't take it in 1.8.1. Dan, sound reasonable?

Comment 37

[Daniel Veditz [:dveditz]]

If extensions aren't using this pattern then it's safe to land this, but we don't need it. If there are vulnerable extensions we will break them and thus we can't land this compatibility change on the stable branch. There appear to be too many potentially affected to work with the one or two that break.

This has landed in 1.9.1, and as extensions upgrade for 3.1-compatibility they will fix their extensions to not use this pattern and users will not be at risk. After 3.1 ships we may be able to turn this on on the branch.

Comment 38

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090206 Shiretoko/3.1b3pre and Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090206 Minefield/3.2a1pre


Results from testcase in comment #1:

---------
binding
---------
chrome://ncb-test/content/sub.xml#x
 -> [object XPCNativeWrapper [object HTMLDocument]] / #document
 JS frame :: chrome://ncb-test/content/sub.xml :: anonymous :: line 7

chrome://NCB-TEST/content/sub.xml#x
 -> [object XPCNativeWrapper [object HTMLDocument]] / #document
 JS frame :: chrome://ncb-test/content/sub.xml :: anonymous :: line 7

CHROME://ncb-test/content/sub.xml#x
 -> [object XPCNativeWrapper [object HTMLDocument]] / #document
 JS frame :: chrome://ncb-test/content/sub.xml :: anonymous :: line 7

resource://ncb-test/sub.xml#x
 -> null

file:///Users/tchung/Library/Application%20Support/Firefox/Profiles/mqcdg41m.191nightly/extensions/non-chrome-s-o-b@test-test/binding/sub.xml#x
 -> undefined

data:text/xml,%3Cbindings%20xmlns%3D%22http%3A%2F%2Fwww.mozilla.org%2Fxbl%22%3E%0D%0A%20%20%3Cbinding%20id%3D%22x%22%3E%0D%0A%20%20%20%20%3Cimplementation%3E%0D%0A%20%20%20%20%20%20%3Cconstructor%3E%3C!%5BCDATA%5B%0D%0Ax_test4.o.x%20%3D%20function(w)%20%7B%0D%0A%09var%20s%3B%0D%0A%09try%20%7B%20s%20%3D%20Components.stack%3B%20%7D%0D%0A%09catch%20(e)%20%7B%20s%20%3D%20e%3B%20%7D%0D%0A%09var%20d%20%3D%20w.content.document%3B%0D%0A%09return%20d.toString()%20%2B%20%22%20%2F%20%22%20%2B%20d.nodeName%20%2B%20%22%5Cn%20%22%20%2B%20s%3B%0D%0A%7D%3B%0D%0A%20%20%20%20%20%20%5D%5D%3E%3C%2Fconstructor%3E%0D%0A%20%20%20%20%3C%2Fimplementation%3E%0D%0A%20%20%3C%2Fbinding%3E%0D%0A%3C%2Fbindings%3E%0D%0A
 -> undefined


---------
overlay
---------
chrome://nco-test/content/sub-overlay.xul
 -> [object XPCNativeWrapper [object HTMLDocument]] / #document
 JS frame :: chrome://nco-test/content/sub-overlay.xul :: anonymous :: line 5

chrome://NCO-TEST/content/sub-overlay.xul
 -> [object XPCNativeWrapper [object HTMLDocument]] / #document
 JS frame :: chrome://nco-test/content/sub-overlay.xul :: anonymous :: line 5

CHROME://nco-test/content/sub-overlay.xul
 -> [object XPCNativeWrapper [object HTMLDocument]] / #document
 JS frame :: chrome://nco-test/content/sub-overlay.xul :: anonymous :: line 5

resource://nco-test/sub-overlay.xul
 -> [object XPCNativeWrapper [object HTMLDocument]] / #document
 JS frame :: chrome://browser/content/browser.xul -> resource://nco-test/sub-overlay.xul :: anonymous :: line 5

file:///Users/tchung/Library/Application%20Support/Firefox/Profiles/mqcdg41m.191nightly/extensions/non-chrome-s-o-b@test-test/overlay/sub-overlay.xul
 -> [object XPCNativeWrapper [object HTMLDocument]] / #document
 JS frame :: chrome://browser/content/browser.xul -> file:///Users/tchung/Library/Application%20Support/Firefox/Profiles/mqcdg41m.191nightly/extensions/non-chrome-s-o-b@test-test/overlay/sub-overlay.xul :: anonymous :: line 5

data:application/vnd.mozilla.xul+xml,%3Coverlay%20xmlns%3D%22http%3A%2F%2Fwww.mozilla.org%2Fkeymaster%2Fgatekeeper%2Fthere.is.only.xul%22%3E%0D%0A%3Cscript%3E%3C!%5BCDATA%5B%0D%0Ax_test3.o.x%20%3D%20function(w)%20%7B%0D%0A%09var%20s%3B%0D%0A%09try%20%7B%20s%20%3D%20Components.stack%3B%20%7D%0D%0A%09catch%20(e)%20%7B%20s%20%3D%20e%3B%20%7D%0D%0A%09var%20d%20%3D%20w.content.document%3B%0D%0A%09return%20d.toString()%20%2B%20%22%20%2F%20%22%20%2B%20d.nodeName%20%2B%20%22%5Cn%20%22%20%2B%20s%3B%0D%0A%7D%3B%0D%0A%5D%5D%3E%3C%2Fscript%3E%0D%0A%3C%2Foverlay%3E%0D%0A
 -> [object XPCNativeWrapper [object HTMLDocument]] / #document
 JS frame :: chrome://browser/content/browser.xul -> data:application/vnd.mozilla.xul+xml,%3Coverlay%20xmlns%3D%22http%3A%2F%2Fwww.mozilla.org%2Fkeymaster%2Fgatekeeper%2Fthere.is.only.xul%22%3E%0D%0A%3Cscript%3E%3C!%5BCDATA%5B%0D%0Ax_test3.o.x%20%3D%20function(w)%20%7B%0D%0A%09var%20s%3B%0D%0A%09try%20%7B%20s%20%3D%20Components.stack%3B%20%7D%0D%0A%09catch%20(e)%20%7B%20s%20%3D%20e%3B%20%7D%0D%0A%09var%20d%20%3D%20w.content.document%3B%0D%0A%09return%20d.toString()%20%2B%20%22%20%2F%20%22%20%2B%20d.nodeName%20%2B%20%22%5Cn%20%22%20%2B%20s%3B%0D%0A%7D%3B%0D%0A%5D%5D%3E%3C%2Fscript%3E%0D%0A%3C%2Foverlay%3E%0D%0A :: anonymous :: line 5

--------------------
 <script> element
--------------------
chrome://ncs-test/content/sub.js
 -> [object XPCNativeWrapper [object HTMLDocument]] / #document
 JS frame :: chrome://ncs-test/content/sub.js :: anonymous :: line 4

chrome://NCS-TEST/content/sub.js
 -> [object XPCNativeWrapper [object HTMLDocument]] / #document
 JS frame :: chrome://ncs-test/content/sub.js :: anonymous :: line 4

CHROME://ncs-test/content/sub.js
 -> [object XPCNativeWrapper [object HTMLDocument]] / #document
 JS frame :: chrome://ncs-test/content/sub.js :: anonymous :: line 4

resource://ncs-test/sub.js
 -> [object XPCNativeWrapper [object HTMLDocument]] / #document
 JS frame :: chrome://browser/content/browser.xul -> resource://ncs-test/sub.js :: anonymous :: line 4

file:///Users/tchung/Library/Application%20Support/Firefox/Profiles/mqcdg41m.191nightly/extensions/non-chrome-s-o-b@test-test/script/sub.js
 -> [object XPCNativeWrapper [object HTMLDocument]] / #document
 JS frame :: chrome://browser/content/browser.xul -> file:///Users/tchung/Library/Application%20Support/Firefox/Profiles/mqcdg41m.191nightly/extensions/non-chrome-s-o-b@test-test/script/sub.js :: anonymous :: line 4

data:text/plain,%0D%0Ax_test2.o.x%20%3D%20function(w)%20%7B%0D%0A%09var%20s%3B%0D%0A%09try%20%7B%20s%20%3D%20Components.stack%3B%20%7D%0D%0A%09catch%20(e)%20%7B%20s%20%3D%20e%3B%20%7D%0D%0A%09var%20d%20%3D%20w.content.document%3B%0D%0A%09return%20d.toString()%20%2B%20%22%20%2F%20%22%20%2B%20d.nodeName%20%2B%20%22%5Cn%20%22%20%2B%20s%3B%0D%0A%7D%3B%0D%0A
 -> [object XPCNativeWrapper [object HTMLDocument]] / #document
 JS frame :: chrome://browser/content/browser.xul -> data:text/plain,%0D%0Ax_test2.o.x%20%3D%20function(w)%20%7B%0D%0A%09var%20s%3B%0D%0A%09try%20%7B%20s%20%3D%20Components.stack%3B%20%7D%0D%0A%09catch%20(e)%20%7B%20s%20%3D%20e%3B%20%7D%0D%0A%09var%20d%20%3D%20w.content.document%3B%0D%0A%09return%20d.toString()%20%2B%20%22%20%2F%20%22%20%2B%20d.nodeName%20%2B%20%22%5Cn%20%22%20%2B%20s%3B%0D%0A%7D%3B%0D%0A :: anonymous :: line 4

Comment 39

[Boris Zbarsky [:bzbarsky]]

We need to document that with this change data: bindings are no longer allowed in chrome packages that get XPCNativeWrapper automation...

Comment 40

[Eric Shepherd [:sheppy]]

I've added a note on this to both:

https://developer.mozilla.org/En/Firefox_3.5_for_developers#Other_improvements

and

https://developer.mozilla.org/En/XPCNativeWrapper