313373 - window.controllers allows some evil things

Status: RESOLVED FIXED

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

Description

[shutdown]

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]

Attachment: 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]

Comment on attachment 200435 [details]
testcase

oops. wrong bug.

Comment 3

[Daniel Veditz [:dveditz]]

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)]

Attachment: Pass the original *vp through to the checkAccess hook when checking on write

Comment 5

[Johnny Stenback (:jst)]

Attachment: diff -w of the above.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 200682 [details] [diff] [review]
diff -w of the above.

r=mrbkap

Comment 7

[Johnny Stenback (:jst)]

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]]

Comment on attachment 200682 [details] [diff] [review]
diff -w of the above.

sr=me, thanks.

/be

Comment 9

[Johnny Stenback (:jst)]

Fix checked in on the trunk.

Comment 10

[Johnny Stenback (:jst)]

Fixed on the branch too.

Comment 11

[shutdown]

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]

Attachment: testcase 2

New testcase that works on the patched version.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Clearing fixed resolution based on comment 11.

Comment 14

[Johnny Stenback (:jst)]

Attachment: 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 emeritus)]

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]]

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)]

Attachment: 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)]

Fix checked in on the trunk. Marking FIXED again.

Comment 19

[Asa Dotzler [:asa]]

Jesse, can you help us understand how this might be exploitable? (fix the status whiteboard)

Comment 20

[Jesse Ruderman]

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]]

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)]

Fix checked in on the 1.8 branch.

Comment 23

[Daniel Veditz [:dveditz]]

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]]

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)]

Fix landed on the 1.0.1 and 1.7 branches.

Comment 26

[Tracy Walker [:tracy]]

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