Last Comment Bug 313373 - window.controllers allows some evil things
: window.controllers allows some evil things
Status: RESOLVED FIXED
[sg:high] XSS, worse?
: fixed1.8, verified1.7.13
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla1.8rc1
Assigned To: Johnny Stenback (:jst, jst@mozilla.com)
: Hixie (not reading bugmail)
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2005-10-22 05:14 PDT by shutdown
Modified: 2007-04-01 15:28 PDT (History)
4 users (show)
dveditz: blocking1.7.13+
dveditz: blocking‑aviary1.0.8+
bob: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (2.65 KB, text/html)
2005-10-22 05:15 PDT, shutdown
no flags Details
Pass the original *vp through to the checkAccess hook when checking on write (3.96 KB, patch)
2005-10-24 17:09 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review
diff -w of the above. (3.63 KB, patch)
2005-10-24 17:10 PDT, Johnny Stenback (:jst, jst@mozilla.com)
mrbkap: review+
brendan: superreview+
dveditz: approval‑aviary1.0.8+
dveditz: approval1.7.13+
mtschrep: approval1.8rc1+
Details | Diff | Splinter Review
testcase 2 (2.87 KB, text/html)
2005-10-24 18:54 PDT, shutdown
no flags Details
XPConnect fix. (3.23 KB, patch)
2005-10-25 16:11 PDT, Johnny Stenback (:jst, jst@mozilla.com)
shaver: review+
brendan: superreview+
dveditz: approval‑aviary1.0.8+
dveditz: approval1.7.13+
asa: approval1.8rc2+
Details | Diff | Splinter Review
Updated XPConnect fix. (3.23 KB, patch)
2005-10-25 18:37 PDT, Johnny Stenback (:jst, jst@mozilla.com)
no flags Details | Diff | Splinter Review

Description shutdown 2005-10-22 05:14:03 PDT
controllers.appendController() and controllers.getControllerAt()
allows creation of false XPCOM objects.

Additionally, controllers.removeControllerAt(0) denies
some user operations including copy, paste, etc...
Comment 1 shutdown 2005-10-22 05:15:05 PDT
Created attachment 200435 [details]
testcase

Works on:
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.9a1) Gecko/20051021 Firefox/1.6a1
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.8b5) Gecko/20051021 Firefox/1.5
Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.7.12) Gecko/20051021 Firefox/1.0.7
Comment 2 shutdown 2005-10-23 04:15:37 PDT
Comment on attachment 200435 [details]
testcase

oops. wrong bug.
Comment 3 Daniel Veditz [:dveditz] 2005-10-24 10:25:19 PDT
Confirming, throwing Johnny's way. Can't "get" a controller ("Permission denied to create wrapper for object of class UnnamedClass") but this testcase shows you can easily substitute your own.

What are the legitimate uses of this object from untrusted web content? I can't find more than stub documentation on the web, is anyone actually using this? If not maybe we could hide the whole object.
Comment 4 Johnny Stenback (:jst, jst@mozilla.com) 2005-10-24 17:09:58 PDT
Created attachment 200681 [details] [diff] [review]
Pass the original *vp through to the checkAccess hook when checking on write
Comment 5 Johnny Stenback (:jst, jst@mozilla.com) 2005-10-24 17:10:45 PDT
Created attachment 200682 [details] [diff] [review]
diff -w of the above.
Comment 6 Blake Kaplan (:mrbkap) 2005-10-24 17:18:55 PDT
Comment on attachment 200682 [details] [diff] [review]
diff -w of the above.

r=mrbkap
Comment 7 Johnny Stenback (:jst, jst@mozilla.com) 2005-10-24 17:24:30 PDT
To elaborate on what's going on here a bit... This isn't window.controller's fault, that's only used to get a double-wrapped JS object. The testcase relies on the fact that xpconnect doesn't do the "right" security check when calling a method on a double-wrapped JS object. The check that XPConnect does is to check if the caller can call method (or get/set) on the double-wrapped JS object, but in this case the double-wrapped JS object is from a different origin than the actual callee (which XPConnect finds on the proto chain), which is the prototype that has been set to a window object from a different origin. The patch in this bug blocks the hole by preventing setting an object's prototype to an object from a different origin. That check was already done, but it was broken and checked the wrong thing...
Comment 8 Brendan Eich [:brendan] 2005-10-24 17:24:38 PDT
Comment on attachment 200682 [details] [diff] [review]
diff -w of the above.

sr=me, thanks.

/be
Comment 9 Johnny Stenback (:jst, jst@mozilla.com) 2005-10-24 17:32:27 PDT
Fix checked in on the trunk.
Comment 10 Johnny Stenback (:jst, jst@mozilla.com) 2005-10-24 17:45:14 PDT
Fixed on the branch too.
Comment 11 shutdown 2005-10-24 18:51:13 PDT
Current testcase no longer works on firefox pacifica-trunk/2005102417.
But, the problem is not fixed yet.

Steps:
1. load "about:blank" into the frame.
2. link __proto__ chain of the object to be double-wrapped.
3. load the target (victim) site into the frame.

This way, you can bypass the security check.
Comment 12 shutdown 2005-10-24 18:54:16 PDT
Created attachment 200694 [details]
testcase 2

New testcase that works on the patched version.
Comment 13 Blake Kaplan (:mrbkap) 2005-10-25 12:57:20 PDT
Clearing fixed resolution based on comment 11.
Comment 14 Johnny Stenback (:jst, jst@mozilla.com) 2005-10-25 16:11:45 PDT
Created attachment 200806 [details] [diff] [review]
XPConnect fix.

This patch makes xpconnect do the right thing in cases like this. Right now, in this case (calling a method on a doubly wrapped object that's implemented by a wrapped native on the doubly wrapped object's prototype) we'll end up bypassing the code that forwards the call to the original JS object and instead calling directly on the wrapped native, which bypasses security checks etc. With this change, a method call (or property get/set) on a doubly wrapped JS object always ends up going through XPConnect's forwarding mechansim (calling through to the nsXPCWrappedJS for the original object). That stops this exploit, early on even since the xpconnect method calling code (which calls the method on the original object) gets interrupted by a failing resolve call when looking up the method. The reason for the failing resolve is that the security manager stops the resolve (through security checks in DOM classinfo).
Comment 15 Mike Shaver (:shaver -- probably not reading bugmail closely) 2005-10-25 16:19:48 PDT
Comment on attachment 200806 [details] [diff] [review]
XPConnect fix.

> #ifdef XPC_IDISPATCH_SUPPORT
>     // If IDispatch is enabled and we're QI'ing to IDispatch
>-    else if(nsXPConnect::IsIDispatchEnabled() && aIID.Equals(NSID_IDISPATCH))
>+    if(nsXPConnect::IsIDispatchEnabled() && aIID.Equals(NSID_IDISPATCH))

Is this a needed change for this bug?  (Do we have test coverage for
IDispatch anywhere?)

>+    JSObject* jsobj = CallQueryInterfaceOnJSObject(ccx, self->GetJSObject(),
>+                                                   aIID);
>+    if (jsobj)
>+    {
>+        // We can't use XPConvert::JSObject2NativeInterface() here
>+        // since that can find a XPCWrappedNative direcly on the 

"directly"

>+        // checks etc by calling directly trough to a native found on

"through".

>+        // In stead, simply do the nsXPCWrappedJS part of

"Instead"

I would really really like a test case for this that we can automate well. r=shaver, because I think it'll fix it, but it's a _huge_ change to take on the branch right now.  (It may still be the minimal change that will fix it, but it makes my tummy feel funny, and I don't think it's because I'm in love with jst.)
Comment 16 Brendan Eich [:brendan] 2005-10-25 18:31:59 PDT
Comment on attachment 200806 [details] [diff] [review]
XPConnect fix.

sr=me, and unlike shaver I am more relaxed about a little harmless else-after return elimination -- but it could wait for the trunk, for sure.

BTW, there is at least one guy (Garrett?) testing ActiveX controls, which use IDispatch.  dbradley patched a regression found by this guy recently.

Good fix, there is usally a single "funnel" in XPConnect in which to make a fix -- the trick is finding the right funnel in the forest! ;-)

/be
Comment 17 Johnny Stenback (:jst, jst@mozilla.com) 2005-10-25 18:37:41 PDT
Created attachment 200824 [details] [diff] [review]
Updated XPConnect fix.

Same thing with the comments cleaned up. I'm more than happy to leave the else-after-return out of this change for the branch.
Comment 18 Johnny Stenback (:jst, jst@mozilla.com) 2005-10-25 19:25:54 PDT
Fix checked in on the trunk. Marking FIXED again.
Comment 19 Asa Dotzler [:asa] 2005-10-26 11:30:49 PDT
Jesse, can you help us understand how this might be exploitable? (fix the status whiteboard)
Comment 20 Jesse Ruderman 2005-10-26 15:14:26 PDT
The demo demonstrates XSS, which is enough to steal cookies, steal passwords, steal information from intranet servers, and defeat CSRF protection on most sites.

Like most XSS holes, this hole can be combined with any hole that lets a site load chrome in a frame to create an arbitrary code execution exploit.  (I verified that this XSS hole can be used this way by commenting out two lines in the testcase that load pages in the iframe, changing "document.cookie" to "Components.classes", and using DOM Inspector to load about:config in the frame before clicking the link.)

I don't understand the bug or exploit well enough to say whether this bug can be used to do more than XSS without combining it with another hole.
Comment 21 Asa Dotzler [:asa] 2005-10-31 14:40:39 PST
moving out to the 1.8 rc2 ride-along candidate list. We'll consider taking this if we do an RC2. Brendan and Shaver, can you guys help us understand what's risky (or not so risky) about this patch and what kind of testing we'd want to feel good about shipping this on the branch so soon before the release.
Comment 22 Johnny Stenback (:jst, jst@mozilla.com) 2005-10-31 18:07:29 PST
Fix checked in on the 1.8 branch.
Comment 23 Daniel Veditz [:dveditz] 2006-02-02 15:27:22 PST
Comment on attachment 200806 [details] [diff] [review]
XPConnect fix.

a=dveditz for drivers, please add fixed-aviary1.0.8 and fixed1.7.13 keywords when checked in.
Comment 24 Daniel Veditz [:dveditz] 2006-02-02 15:27:34 PST
Comment on attachment 200682 [details] [diff] [review]
diff -w of the above.

a=dveditz for drivers, please add fixed-aviary1.0.8 and fixed1.7.13 keywords when checked in.
Comment 25 Johnny Stenback (:jst, jst@mozilla.com) 2006-02-06 15:19:53 PST
Fix landed on the 1.0.1 and 1.7 branches.
Comment 26 Tracy Walker [:tracy] 2006-02-14 14:21:17 PST
verified with:
Windows:
Moz - Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060214
Fx - Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.13) Gecko/20060214
Firefox/1.0.8
Macintosh:
Moz - Mozilla/5.0 (Macintosh; U;PPC Mac OS X Mach-O; en-US; rv:1.7.13)
Gecko/20060214 Firefox/1.0.8
Fx - Mozilla/5.0 (Macintosh; U;PPC Mac OS X Mach-O; en-US; rv:1.7.13)
Gecko/20060214 Firefox/1.0.8
Linux
Moz - Mozilla/5.0 (X11; U;Linux i686; en-US; rv:1.7.13) Gecko/20060214
Fx -  Mozilla/5.0 (X11; U;Linux i686; en-US; rv:1.7.13) Gecko/20060214
Firefox/1.0.8

Note You need to log in before you can comment on or make changes to this bug.