611485 - Passing a Location object as a string argument of a cross-origin XPConnect call fails

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[James Kay]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_4; en-us) AppleWebKit/533.18.1 (KHTML, like Gecko) Version/5.0.2 Safari/533.18.5
Build Identifier: 4.0b7

Some fairly standard JavaScript to break out of a frameset does not work as intended.

 if (parent.frames.length > 0) {
        top.location.replace(document.location);
  }



Reproducible: Always

Steps to Reproduce:
1. Navigate to https://www.workbooks.com/login
2. Login with username: [e-mail]   and password:  demo
3. Observe that the Desktop you see is framed; it should not be and is not in any other browser including in previous versions of Firefox.
Actual Results:  
The desktop is in a frame.

Expected Results:  
The desktop should occupy the whole browser window; the URL becomes https://secure.workbooks.com/ui/desktop

The website executes this JavaScript which appears to be ineffective:

 if (parent.frames.length > 0) {
        top.location.replace(document.location);
 }

Comment 1

[j.j.]

Attachment: testcase

Mozilla/5.0 (Windows NT 5.1; rv:2.0b8pre) Gecko/20101108 Firefox/4.0b8pre

Testcase works fine on winXP, please test on Mac.

Comment 2

[Boris Zbarsky [:bzbarsky]]

The testcase works for me too.

The steps to reproduce in comment 0 reproduce a problem for me, though.

The error console has this to say:

  Error: uncaught exception: [Exception... "Could not convert JavaScript
  argument arg 0 [nsIDOMLocation.replace]"  nsresult: "0x80570009
  (NS_ERROR_XPC_BAD_CONVERT_JS)"  location: "JS frame ::
  https://secure.workbooks.com/ui/desktop :: <TOP_LEVEL> :: line 16"  data: no]

The issue is that the argument to replace() needs to be a string, but document.location is an object...  Normally this would work by converting the object to a string, but in this case we land in XPCConvert::JSData2Native with the incoming jsval being a proxy.

So we call JS_ValueToString on this proxy.  This calls down into js_ValueToString, which calls js::DefaultValue, which calls js_TryMethod, which calls js_GetMethod.  This calls js::proxy_GetProperty with "toString" as the id.  

This ends up calling xpc::XrayWrapper<JSCrossCompartmentWrapper, xpc::CrossCompartmentXray>::get and passing it wrapper === receiver, which calls js::JSProxyHandler::get, passing it proxy === receiver.

Now we this last get() calls xpc::XrayWrapper<JSCrossCompartmentWrapper, xpc::CrossCompartmentXray>::getPropertyDescriptor.  We end up with desc->obj being null, call this->enter, get a false return, and return false all the way up the chain.  So the conversion fails.

The reason enter() fails is that xpc::CheckAndReport<xpc::SameOriginOrCrossOriginAccessiblePropertiesOnly> returns false (though it'd be nice if this were reported somewhere useful).

Blake, this sounds like your kettle of fish.  If you just breakpoint on the DOMString case in JSData2Native and condition the breakpoint on the incoming jsval_layout tag being OBJECT, then follow the steps to reproduce, you'll hit that breakpoint exactly for the replace() call in question.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Ideally, this would just call the toString from nsIDOMLocation....

Even more ideally, it wouldn't look like the callee (window.top here) is the one trying to do the toString() call, since that's in fact something that shouldn't be allowed.

Comment 4

[Johnny Stenback (:jst)]

Andreas, is this related to the changes we took to refuse to pass certain types of C++ objects across origins, or is this something different?

Either way, assigning to mrbkap, and blocking.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: WIP (DOES NOT WORK)

This is a work in progress. It doesn't actually succeed in calling functions through Xrays yet (we get a compartment mismatch trying to do so). I think it's pretty close, but the end result is going to be a least a little fragile. I think there might be a better way to do this by teaching XPConnect internals more about compartments. But for now, this approach *should* work.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: WIP (DOES NOT WORK)

This patch mostly works, but is too incompatible to land. I'm working on an alternative that will be more compatible.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

Comment 8

[Andreas Gal :gal]

Comment on attachment 506562 [details] [diff] [review]
Fix

We still have to rename CrossOriginWrappers. Nice to get rid of the Same/Cross compartment Xray business. This patch is definitely a step forward.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/tracemonkey/rev/a2825fbe23e3

Comment 10

[Johnny Stenback (:jst)]

Fixed by tracemonkey merge.

http://hg.mozilla.org/mozilla-central/rev/a2825fbe23e3

Comment 11

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/a2825fbe23e3
http://hg.mozilla.org/mozilla-central/rev/8474f8f13bb1

Comment 12

[George Carstoiu]

Build identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110206 Firefox/4.0b12pre

Confirming issue as being fixed.