323367 - When calling XPCNativeWrapper's toString method, content-defined toString method can be called

Status: RESOLVED FIXED

(Core :: Security, defect, P2)

Description

[moz_bug_r_a4]

If an object's toString method is redefined by content, when toString method of
the XPCNativeWrapper for that object is called, content-defined toString method
ends up being called.

  window.toString = function() { return "xxx"; };
  xWin = new XPCNativeWrapper(window);
  s = xWin.toString();

s is "xxx".

Note, content-defined toString method cannot return an object.

  window.toString = function() { return new String("xxx"); };
  xWin = new XPCNativeWrapper(window);
  s = xWin.toString();

s is "[object Window]".


This bug could be a potential vulnerability.

content:
  <iframe src="http://www.yahoo.com/"></iframe>
  cookieGetter = Components.lookupMethod(frames[0].document, "cookie");
  Selection.prototype.toString = cookieGetter;

chrome:
  s = content.getSelection().toString();

s is a cookie value set by http://www.yahoo.com/.

Comment 1

[moz_bug_r_a4]

Attachment: testcase 1 - xWin.toString(), xWin.getSelection().toString() and xWin.getSelection().getRangeAt(0).toString()

Comment 2

[moz_bug_r_a4]

Attachment: testcase 2 - Selection.prototype.toString = cookieGetter;

Comment 3

[moz_bug_r_a4]

cookieGetter = Components.lookupMethod(frames[0].document, "cookie");

Don't we need to do security check on Components.lookupMethod, for preventing
future exploits?

Comment 4

[moz_bug_r_a4]

XPC_NW_toString calls JS_ValueToString on the unwrapped JS object, and then
js_ValueToString calls js_DefaultValue.  In js_DefaultValue, js_TryMethod calls
content-defined toString method.  If content-defined toString method returns
non-primitive, XPC_WN_Shared_Convert is called.

Comment 5

[Daniel Veditz [:dveditz]]

Several questions come to mind...

Why isn't chrome getting the native Selection toString()? I thought split wrappers prevented content from this kind of overriding?

The call doesn't have chrome privs (maybe that answers the first question), but if it's running with the content principal how is it getting the cookie?

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

I got this one.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Partial fix

This fix needs testing and comments. There's also a remaining call to JS_ValueToString, but I'm not convinced that it's a real problem (since the result will be wrapped in our normal XPCNativeWrapper toString jazz).

Brendan are there any cases where this fix will be too strict?

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Potential fix

This, I believe, is the safe way to implement XPCNativeWrapper's toString method.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 210935 [details] [diff] [review]
Potential fix

>Index: js/src/xpconnect/src/XPCNativeWrapper.cpp
>+    if (!member->GetValue(ccx, iface, &toStringVal)) {
>+      return JS_FALSE;
>+    }
...
> 
>+    JSObject *funobj = xpc_CloneJSFunction(ccx, JSVAL_TO_OBJECT(toStringVal),
>+                                           wn_obj);

You need to keep toStringVal from dying here if the clone triggers GC.  Look at the other callers of xpc_CloneJSFunction in this file.

I really wish we could share this code with the GetOrSet method or something, but I guess the part about outputting our own thing makes it hard...

>-      ::JS_GetFunctionNative(cx, toStringFun) != XPC_WN_Shared_ToString;

Since you removed that, do you still need the extern decl of XPC_WN_Shared_ToString above?  I doubt you do.

> +  JSString* str = NULL;;

nsnull?

r=bzbarsky with those.

Comment 10

[Daniel Veditz [:dveditz]]

Doesn't appear to affect ff1.0.7/moz1.7.12 -- am I right in assuming this exploits changes in the wrappers made in 1.8?

Comment 11

[Boris Zbarsky [:bzbarsky]]

> am I right in assuming this exploits changes in the wrappers made in 1.8?

That's correct.

Comment 12

[Johnny Stenback (:jst)]

(In reply to comment #9)
> (From update of attachment 210935 [details] [diff] [review] [edit])
> >-      ::JS_GetFunctionNative(cx, toStringFun) != XPC_WN_Shared_ToString;
> 
> Since you removed that, do you still need the extern decl of
> XPC_WN_Shared_ToString above?  I doubt you do.

And if you remove the extern decl, you could make XPC_WN_Shared_ToString() in xpcwrappednativejsops.cpp static again too.

> 
> > +  JSString* str = NULL;;
> 
> nsnull?
> 
> r=bzbarsky with those.
> 

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 210935 [details] [diff] [review]
Potential fix

sr=jst

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: What I checked in

This is what I just checked in.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 212268 [details] [diff] [review]
What I checked in

approved for 1.8.0 branch, a=dveditz

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.8 branches.

Comment 18

[Bob Clary [:bc] (inactive)]

v 1.8.0.2/winxp/20060308

Comment 19

[Bob Clary [:bc] (inactive)]

ffb2 windows/linux verified fixed 1.8