413902 - XPCSafeJSObjectWrapper's equality hook returns true too much

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P2)

Description

[Jeremy Maitin-Shepard]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2007122108
Build Identifier: xulrunner 1.9b3pre

The following simple XUL document demonstrates the problem:

<?xml version="1.0"?>
<?xml-stylesheet href="chrome://global/skin" type="text/css"?>
<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
<script language="JavaScript" type="application/javascript;version=1.8"><![CDATA[
var w = new XPCNativeWrapper(window).wrappedJSObject;
w.foo = function() {yield;};
alert("Result: " + (w.foo() == {}));
]]></script>
</window>


Reproducible: Always

Comment 1

[Jason Orendorff [:jorendorff]]

Maybe wrappedJSObject is returning a wrapper that has a busted JSExtendedClass.equality hook?

jbms also reports that
  typeof(w.foo().send) == 'object',
whereas
  typeof(function() {yield;}().send) == 'function'

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

I think I see what's going on here. I'll attach a patch in a little bit.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #1)
>   typeof(w.foo().send) == 'object',

This is an unfortunate side-effect of how SJOWs work -- wrapped functions are actually SJOWs that know how to call an underlying function. The JS engine would need some additional work to fix this and I don't think anybody's thought that it's worth it.

Requesting blocking based on the relationship to bug 344494.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

The problem is that XPC_GetIdentityObject was returning NULL for both sides of the equality test. I restructured things a little to allow myself to stay sane.

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: With mochitest

Sorry for the bugspam -- I realized that I'd forgotten to add mochitests.

Comment 6

[Jeremy Maitin-Shepard]

This problem came up for me when I had to obtain a reference to the top-level chrome window given a reference to an XPCNativeWrapper-wrapped DOMWindow in an xul:browser within the chrome window.  (This was for implementing nsIHelperAppLauncherDialog.)  Using the standard trick of walking up the doc shell tree to get at the top-level chrome window, the result is an XPCNativeWrapper-wrapped reference to the chrome window.  Using wrappedJSObject results in an XPCSafeJSObjectWrapper-wrapped reference, when what is really desired is the unwrapped reference.

Although it seems this particular bug should be fixed, the real solution would seem to be a method for fulling unwrapping an XPCSafeJSObjectWrapper, as there currently does not seem to be any way to do that from JavaScript.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

That would be patently unsafe and cause extensions and chrome code to get hacked. Why, exactly, do you need an unwrapped content window? Ideally, we should be unwrapping when you pass your reference into C++ code that can safely deal with content windows.

Comment 8

[Jeremy Maitin-Shepard]

It seems it would certainly be unsafe if used improperly, but I don't see why it couldn't be used properly, such as in this situation.  I don't want an unwrapped content window; I want an unwrapped reference to the top-level chrome window that contains the content window.  This is the code I am using to obtain a wrapped reference to the chrome window:  (please let me know if it could be exploited)

function get_window_from_frame(frame) {
    try {
        var window = frame.QueryInterface(Ci.nsIInterfaceRequestor)
            .getInterface(Ci.nsIWebNavigation)
            .QueryInterface(Ci.nsIDocShellTreeItem)
            .rootTreeItem
            .QueryInterface(Ci.nsIInterfaceRequestor)
            .getInterface(Ci.nsIDOMWindow).wrappedJSObject;
        /* window is now an XPCSafeJSObjectWrapper */
        return window;
    } catch (e) {
        return null;
    }
}

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

In this case, |frame| is a content frame? If so, it sounds like XPCNativeWrapper could auto-unwrap if you go from a content thing -> chrome thing, avoiding the need for XPCSafeJSObjectWrapper.

In general, without a wrapper between you and a content object, you cannot do *anything* with that object without becoming exploitable in some way. At this point, wrapper automation is really the only way to go.

Comment 10

[Jeremy Maitin-Shepard]

Yes, |frame| is a content frame.  I don't understand what you mean by "could auto-unwrap".  That certainly does not seem to be what happens currently, but indeed it could perhaps be useful.  That would break existing code, though, unless you mean only the XPCSafeJSObjectWrapper would automatically not be used when accessing wrappedJSObject on a chrome object, but the XPCNativeWrapper would remain.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

I mean an XPCNativeWrapper on a chrome object is mostly redundant and we wouldn't bother wrapping the chrome object as you get it. That would mean that you wouldn't need to use .wrappedJSObject at all at the end of your big expression because it would return the actual object (and not a wrapper wrapper). Am I missing something else?

Comment 12

[Jeremy Maitin-Shepard]

I suppose that would be fine, but it seems like it could possibly be confusing if XPCNativeWrappers can go away implicitly.  It would also break some existing code.

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 299647 [details] [diff] [review]
With mochitest

- In js/src/xpconnect/tests/mochitest/test_wrappers.html:

+    ok(!(new XPCSafeJSObjectWrapper({}) == new XPCSafeJSObjectWrapper({})),
+       'SJOWs equality hook returns true correctly');
+
+    ok(!(new XPCSafeJSObjectWrapper({}) == new XPCSafeJSObjectWrapper({})),
+       'SJOWs equality hook returns false correctly');

Don't these two tests test the exact same thing, with different explanation?

r+sr=jst with that figured out.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

I filed bug 414296 on avoiding SJOWs for chrome objects retrieved out of XPCNativeWrappers.

The duplicated test was a bad copy/paste.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated to comments

This will be needed to further the work for a blocking-1.9+ bug.

Comment 16

[Damon Sicore (:damons)]

Comment on attachment 299673 [details] [diff] [review]
Updated to comments

a1.9+=damons

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in.