Closed Bug 397855 Opened 15 years ago Closed 15 years ago

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

Categories

(Core :: XPConnect, defect, P2)

x86
Windows XP
defect

Tracking

()

VERIFIED FIXED

People

(Reporter: moz_bug_r_a4, Assigned: mrbkap)

References

Details

(Keywords: crash, testcase)

Attachments

(2 files, 2 obsolete files)

Variant of bug 394815.
it is not just window:
executing 2 times causes crash
javascript:document.__proto__.__proto__=document;alert(document)
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
(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?
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.
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>
(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.
Severity: normal → critical
Keywords: crash
This is a fairly common crash for me when I fuzz.
Flags: blocking1.9?
Assignee: nobody → mrbkap
Flags: blocking1.9? → blocking1.9+
Keywords: testcase
Blocks: 402915
I'm disagreeing with Jonas here. This is symptomatic of a larger problem where wrappers can easily form __proto__ cycles and is biting Jesse.
Priority: P4 → P2
Attached patch Fix v1 (obsolete) — Splinter Review
This is something like what I had in mind. This fixes bug 403005 as well.
Attachment #293980 - Flags: review?(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
Attachment #293980 - Flags: review?(brendan)
Attachment #293980 - Flags: review+
Attachment #293980 - Flags: approval1.9+
Attachment #293980 - Flags: superreview?(dveditz)
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 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 ;-)
Attachment #293980 - Flags: superreview?(dveditz) → superreview-
Attached patch Interdiff showing changes (obsolete) — Splinter Review
Attachment #293980 - Attachment is obsolete: true
Attachment #293992 - Flags: superreview?(dveditz)
Attachment #293992 - Flags: review?(brendan)
Comment on attachment 293992 [details] [diff] [review]
Interdiff showing changes

sr=dveditz on the other patch plus this diff
Attachment #293992 - Flags: superreview?(dveditz) → superreview+
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
Attachment #293992 - Flags: review?(brendan)
Attachment #293992 - Flags: review+
Attachment #293992 - Flags: approval1.9+
When the tree is green again, I'll check this in.
Attachment #293992 - Attachment is obsolete: true
Fix checked into trunk.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
mrbkap, I think this hurt perf at least on Linux. :(
Flags: in-testsuite?
Blocks: 405159
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.