Closed Bug 425153 Opened 14 years ago Closed 13 years ago

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

Categories

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

x86
Windows XP
defect

Tracking

()

VERIFIED FIXED
mozilla1.9.1b2

People

(Reporter: moz_bug_r_a4, Assigned: jst)

References

Details

(Keywords: dev-doc-complete, verified1.9.1, Whiteboard: [sg:critical vector] critical for addons that do this)

Attachments

(3 files, 1 obsolete file)

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
Attached file 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
Flags: blocking1.9?
This should block, as adding these further restrictions might break more extensions.
Assignee: dveditz → nobody
Component: Security → DOM: Mozilla Extensions
QA Contact: toolkit → general
Whiteboard: [sg:critical]
Per comment 3 - moving to list
Flags: blocking1.9? → blocking1.9+
Priority: -- → P2
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.14?
Let's try to do this in a dot-release asap.
Flags: wanted1.9.0.x+
Flags: blocking1.9-
Flags: blocking1.9+
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?
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?
> 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.
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.
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.
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...
In error messages etc you would see the wrong file name, yes.
Marking as blocking 1.8.1.15.

Assigning to Johnny since he fixed bug 418356.
Assignee: nobody → jst
Flags: blocking1.8.1.15? → blocking1.8.1.15+
Flags: blocking1.9.0.1?
Flags: blocking1.8.1.16+
Flags: blocking1.8.1.15+
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
Flags: blocking1.9.0.1? → blocking1.9.0.1-
(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...
Flags: blocking1.9.0.1- → blocking1.9.0.1?
Flags: blocking1.9.0.1? → blocking1.9.0.1-
Whiteboard: [sg:critical] → [sg:critical][1.9.0.2+]
Flags: blocking1.9.0.2+
Whiteboard: [sg:critical][1.9.0.2+] → [sg:critical][needs 1.8/1.9 patches]
Flags: blocking1.9.1?
Flags: blocking1.9.0.3+
Flags: blocking1.9.0.2+
Flags: blocking1.8.1.18+
Flags: blocking1.8.1.17+
Flags: blocking1.9.1? → blocking1.9.1+
Not blocking on this one after all.
Flags: blocking1.9.1+ → blocking1.9.1-
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.
Flags: blocking1.9.1?
Flags: blocking1.9.1-
Flags: blocking1.9.0.5+
Flags: blocking1.9.0.4+
Flags: blocking1.8.1.19+
Flags: blocking1.8.1.18+
Yeah, 1.9.1+.
Flags: blocking1.9.1? → blocking1.9.1+
Target Milestone: --- → mozilla1.9.1b2
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?
Attachment #346809 - Flags: superreview?(jonas)
Attachment #346809 - Flags: review?(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?
Whiteboard: [sg:critical][needs 1.8/1.9 patches] → [sg:critical][needs r/sr bz/sicking]
Same change as above, with more code sharing.
Attachment #346809 - Attachment is obsolete: true
Attachment #347676 - Flags: superreview?(bzbarsky)
Attachment #347676 - Flags: review?(jonas)
Attachment #346809 - Flags: superreview?(jonas)
Attachment #346809 - Flags: review?(bzbarsky)
Attachment #347676 - Flags: superreview?(bzbarsky) → superreview+
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 on attachment 347676 [details] [diff] [review]
Updated fix to be less cut-n-past'y

looks nice
Attachment #347676 - Flags: review?(jonas) → review+
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 :(
Whiteboard: [sg:critical][needs r/sr bz/sicking] → [sg:critical]
Patch landed, marking FIXED.
Marking fixed for real. :)
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [sg:critical] → [sg:critical][needs branch patches]
Johnny, how are you coming on branch patches for this bug? (What, you need more than a day? Fer shame! ;) )
(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 :(
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.
Flags: blocking1.9.0.6+
Flags: blocking1.9.0.5+
Flags: blocking1.8.1.19-
Flags: blocking1.8.1.19+
Whiteboard: [sg:critical][needs branch patches] → [sg:moderate] potentially critical if addons do this [needs branch patches]
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.
Oh, and the point is that we should document this change from 3.0 for add-on developers.
Keywords: dev-doc-needed
Another one for the 3.1 extensions docs.
Depends on: 469898
Moving this to 1.9.0.7 (at least) so we get a better idea what the 3.1 change has broken besides Ubiquity.
Flags: blocking1.9.0.6+ → blocking1.9.0.7+
And, while we get that better idea, Johnny, can you work up 1.9.0 and 1.8.1 patches?
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
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?
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.
Flags: blocking1.9.0.7+
Whiteboard: [sg:moderate] potentially critical if addons do this [needs branch patches] → [sg:critical vector] critical for addons that do this
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
Status: RESOLVED → VERIFIED
Flags: blocking1.9.0.8?
Flags: blocking1.9.0.8? → blocking1.9.0.8-
We need to document that with this change data: bindings are no longer allowed in chrome packages that get XPCNativeWrapper automation...
Depends on: 479839
Depends on: 490197
Depends on: 500968
Depends on: 513731
Component: DOM: Mozilla Extensions → DOM
Group: core-security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.