window.controllers allows some evil things

RESOLVED FIXED in mozilla1.8rc1

Status

()

Core
DOM
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: shutdown, Assigned: jst)

Tracking

({fixed1.8, verified1.7.13})

Trunk
mozilla1.8rc1
fixed1.8, verified1.7.13
Points:
---
Bug Flags:
blocking1.7.13 +
blocking-aviary1.0.8 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:high] XSS, worse?)

Attachments

(6 attachments)

(Reporter)

Description

12 years ago
controllers.appendController() and controllers.getControllerAt()
allows creation of false XPCOM objects.

Additionally, controllers.removeControllerAt(0) denies
some user operations including copy, paste, etc...
(Reporter)

Comment 1

12 years ago
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
(Reporter)

Updated

12 years ago
Attachment #200435 - Attachment is obsolete: true
(Reporter)

Comment 2

12 years ago
Comment on attachment 200435 [details]
testcase

oops. wrong bug.
Attachment #200435 - Attachment is obsolete: false
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.
Assignee: general → jst
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking1.8rc1+
Flags: blocking1.7.13+
Flags: blocking-aviary1.0.8+
Whiteboard: [sg:high] XSS, worse?
(Assignee)

Comment 4

12 years ago
Created attachment 200681 [details] [diff] [review]
Pass the original *vp through to the checkAccess hook when checking on write
(Assignee)

Comment 5

12 years ago
Created attachment 200682 [details] [diff] [review]
diff -w of the above.
Attachment #200682 - Flags: superreview?(brendan)
Attachment #200682 - Flags: review?(mrbkap)
Comment on attachment 200682 [details] [diff] [review]
diff -w of the above.

r=mrbkap
Attachment #200682 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 7

12 years ago
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...
Status: NEW → ASSIGNED
OS: Windows 98 → All
Hardware: PC → All
Target Milestone: --- → mozilla1.8rc1
Comment on attachment 200682 [details] [diff] [review]
diff -w of the above.

sr=me, thanks.

/be
Attachment #200682 - Flags: superreview?(brendan)
Attachment #200682 - Flags: superreview+
Attachment #200682 - Flags: approval1.8rc1?
(Assignee)

Comment 9

12 years ago
Fix checked in on the trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Attachment #200682 - Flags: approval1.7.13?
Attachment #200682 - Flags: approval-aviary1.0.8?

Updated

12 years ago
Attachment #200682 - Flags: approval1.8rc1? → approval1.8rc1+
(Assignee)

Comment 10

12 years ago
Fixed on the branch too.
Keywords: fixed1.8
(Reporter)

Comment 11

12 years ago
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.
(Reporter)

Comment 12

12 years ago
Created attachment 200694 [details]
testcase 2

New testcase that works on the patched version.
Clearing fixed resolution based on comment 11.
Status: RESOLVED → REOPENED
Keywords: fixed1.8
Resolution: FIXED → ---
(Assignee)

Comment 14

12 years ago
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).
Attachment #200806 - Flags: superreview?(brendan)
Attachment #200806 - Flags: review?(shaver)
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.)
Attachment #200806 - Flags: review?(shaver) → review+
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
Attachment #200806 - Flags: superreview?(brendan)
Attachment #200806 - Flags: superreview+
Attachment #200806 - Flags: approval1.8rc1?
(Assignee)

Comment 17

12 years ago
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.
(Assignee)

Comment 18

12 years ago
Fix checked in on the trunk. Marking FIXED again.
Status: REOPENED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
(Assignee)

Updated

12 years ago
Attachment #200806 - Flags: approval1.7.13?
Attachment #200806 - Flags: approval-aviary1.0.8?

Comment 19

12 years ago
Jesse, can you help us understand how this might be exploitable? (fix the status whiteboard)
Flags: blocking1.8rc1+ → blocking1.8rc1?

Comment 20

12 years ago
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

12 years ago
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.
Flags: blocking1.8rc1? → blocking1.8rc2?

Updated

12 years ago
Attachment #200806 - Flags: approval1.8rc1? → approval1.8rc2+
(Assignee)

Comment 22

12 years ago
Fix checked in on the 1.8 branch.
Keywords: fixed1.8

Updated

12 years ago
Flags: blocking1.8rc2?

Updated

12 years ago
Flags: testcase+
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.
Attachment #200806 - Flags: approval1.7.13?
Attachment #200806 - Flags: approval1.7.13+
Attachment #200806 - Flags: approval-aviary1.0.8?
Attachment #200806 - Flags: approval-aviary1.0.8+
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.
Attachment #200682 - Flags: approval1.7.13?
Attachment #200682 - Flags: approval1.7.13+
Attachment #200682 - Flags: approval-aviary1.0.8?
Attachment #200682 - Flags: approval-aviary1.0.8+
(Assignee)

Comment 25

12 years ago
Fix landed on the 1.0.1 and 1.7 branches.
Keywords: fixed-aviary1.0.8, fixed1.7.13
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
Keywords: fixed-aviary1.0.8, fixed1.7.13 → verified-aviary1.0.8, verified1.7.13
Group: security

Updated

11 years ago
Flags: in-testsuite+ → in-testsuite?
You need to log in before you can comment on or make changes to this bug.