397855 - window.__proto__.__proto__ = window creates __proto__ cycles with XOW

Status: VERIFIED FIXED

(Core :: XPConnect, defect, P2)

Description

[moz_bug_r_a4]

Attachment: testcase (crashes Firefox when loaded)

Variant of bug 394815.

Comment 1

[georgi - hopefully not receiving bugspam]

it is not just window:
executing 2 times causes crash
javascript:document.__proto__.__proto__=document;alert(document)

Comment 2

[moz_bug_r_a4]

The testcase in comment #0 no longer crashes. (Due to the fix for bug 390947?)

(In reply to comment #1)
> it is not just window:
> executing 2 times causes crash
> javascript:document.__proto__.__proto__=document;alert(document)
> 

Yeah, location, iframe and frame object can be used, too.

location.__proto__.__proto__ = location;
location.__proto__; // -> crash

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/js/src/xpconnect/src/XPCWrapper.h&rev=1.3&mark=84-88#81

Comment 3

[georgi - hopefully not receiving bugspam]

(In reply to comment #2)
>http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/js/src/xpconnect/src/XPCWrapper.h&rev=1.3&mark=84-88#81
> 

 84              return !strcmp(name, "Window")            ||
 85 mrbkap 1.1          !strcmp(name, "Location")          ||
 86                     !strcmp(name, "HTMLDocument")      ||
 87                     !strcmp(name, "HTMLIFrameElement") ||
 88                     !strcmp(name, "HTMLFrameElement");

what makes mrbkap think this is a complete list - not counting the crash?

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

That's definitely a problem, it probably isn't a complete list. Unfortunately, there isn't really a better way of knowing whether an object needs a same origin XOW that doesn't involve checking its class. The list was culled (perhaps incorrectly) from places in nsDOMClassInfo.cpp where we used to do security checks.

Comment 5

[georgi - hopefully not receiving bugspam]

since bug 367911 is private i assume the XOW stuff is security related.

suggest someone with an exploit try to exploit these object:
ImageDocument (when you open an image in new window)
<object>
<embed>
<frameset>

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #5)
> ImageDocument (when you open an image in new window)

This creates a faux HTML document.

> <object>
> <embed>

These don't really offer a way to access whatever content they're displaying. <object> provides the contentDocument, but that'll get wrapped in a XOW before coming out.

> <frameset>

This creates an HTMLFrameElement.

Comment 7

[Jesse Ruderman]

This is a fairly common crash for me when I fuzz.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

I'm disagreeing with Jonas here. This is symptomatic of a larger problem where wrappers can easily form __proto__ cycles and is biting Jesse.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix v1

This is something like what I had in mind. This fixes bug 403005 as well.

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 293980 [details] [diff] [review]
Fix v1

>+            if ((lclasp->flags & JSCLASS_IS_EXTENDED) &&
>+                (rclasp->flags & JSCLASS_IS_EXTENDED)) {

Could use (lclasp->flags & rclasp->flags & JSCLASS_IS_EXTENDED) to avoid a branch, at least in my head if not in optimized code.

>+            JSObject *wrappedObj = xclasp->wrappedObject(cx, obj);
>+            if (wrappedObj)
>+                scopeobj = OBJ_GET_PARENT(cx, wrappedObj);

Your camelCaps is in my runonunixstyle! How about "wrappedobj" or just "wrapped"?

Looks great otherwise, r/a=me with these nits picked.

/be

Comment 11

[Brendan Eich [:brendan]]

Comment on attachment 293980 [details] [diff] [review]
Fix v1

>+    JSObjectOp          wrappedObject;

Mentioned to mrbkap in person: comment (inline on right should fit) about this hook being infallible.

/be

Comment 12

[Daniel Veditz [:dveditz]]

Comment on attachment 293980 [details] [diff] [review]
Fix v1

>+            if ((lclasp->flags & JSCLASS_IS_EXTENDED) &&
>+                (rclasp->flags & JSCLASS_IS_EXTENDED)) {

While explaining this to me mrbkap noticed that a wrapper will not be === with the object it wraps, an incorrect result if we want the wrappers to be invisible. This could happen if some code gets the same object from different paths, one of which wraps while the other does not.

>   // JSExtendedClass initialization
>-  XPC_SJOW_Equality
>+  XPC_SJOW_Equality,
>+  nsnull, // iteratorObject
>+  XPC_SJOW_WrappedObject,

Do you need outer and inner between equality and the interator?

sr- due to the problems you noticed yourself ;-)

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Interdiff showing changes

Comment 14

[Daniel Veditz [:dveditz]]

Comment on attachment 293992 [details] [diff] [review]
Interdiff showing changes

sr=dveditz on the other patch plus this diff

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 293992 [details] [diff] [review]
Interdiff showing changes

Good thing someone was paying attention! I thought it wasn't supposed to be possible to have an unwrapped object and its wrapper in the same execution -- either that's a security hole, or an avoidable inefficiency if the subject principal has proper access to the unwrapped object (and so does not need the wrapper).

Is there hope (and a followup bug) for unwrapping as appropriate when values flow across trust domain boundaries?

/be

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: What I'm about to check in

When the tree is green again, I'll check this in.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 18

[Reed Loden [:reed]]

mrbkap, I think this hurt perf at least on Linux. :(

Comment 19

[Carsten Book [:Tomcat]]

no crash using the testcase and Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9pre) Gecko/2008050606 Minefield/3.0pre ID:2008050606 -> verified fixed