760887 - "ASSERTION: function object has parent of unknown class!"

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: function object has parent of unknown class!: 'Error', file js/xpconnect/src/XPCWrappedNative.cpp, line 1779

Smells cpg-related.

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Andrew McCreight [:mccr8]]

bholley thinks this sounds a lot like bug 752764, and I agree, so I'll look at it to see what is going on.

Comment 3

[Andrew McCreight [:mccr8]]

WFM on a local build of Fx16 with bug 751995 and bug 758415 applied.  So 16 is probably fixed.  Hurray!

Comment 4

[Andrew McCreight [:mccr8]]

Oops, never mind.  I must have been doing something clever like running a 14 debug build my local 16 build directory or something awesome like that.  Still affected.

Comment 5

[Andrew McCreight [:mccr8]]

This looks different than bug 752764 to me.  What is happening is that the parent of the function hasChildNodes is another function, btoa (well, the actual parent is a proxy, but the call and private slots of the proxy are the btoa function).  The parent of btoa is XPC_WN_ModsAllowed_NoCall_Proto_JSClass, which looks like it would be okay.  Maybe the problem here is that GetWrappedNativeOfJSObject needs to chase further up the parent chain?

Comment 6

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #5)
> This looks different than bug 752764 to me.  What is happening is that the
> parent of the function hasChildNodes is another function, btoa (well, the
> actual parent is a proxy, but the call and private slots of the proxy are
> the btoa function).

Hm, if the parent of the function is actually btoa, that's a bug. But it's not clear to me how that would happen. btoa appears in this testcase as a mutable proto for |document|, and there are very few uses of js::GetObjectProto in xpconnect. One of those uses is in XPCWrappedNative::GetWrappedNativeForJSObject, where it crawls the proto chain. Is the function actually parented to btoa, or did we just get there by crawling the proto chain?

The current bizarre implementation/behavior of GetWrappedNativeOfJSObject really needs to go away anyway. I already ripped it out for new dom bindings and quickstubs. Last I recall there was some reason why we still needed it. Something to do with bug 658909...

Comment 7

[Andrew McCreight [:mccr8]]

The actual parent of hasChildNodes is this object:

0x11871c220 W Proxy <no private>
> 0x118716790 W type
> 0x1280267e0 W shape
> 0x12801e1c0 W call
> 0x12801e1c0 W private

the call and shape fields point to btoa:

0x12801e1c0 W Function btoa
> 0x11870d5e0 W type
> 0x128010ee8 W shape
> 0x1185257a0 B atom

js::GetObjectParent of hasChildNodes is 0x11871c220.
js::UnwrapObject of 0x11871c220 is 0x12801e1c0 (btoa)
Then it computes JS_ObjectToInnerObject of that, which is just the same thing, the btoa function.

Comment 8

[Andrew McCreight [:mccr8]]

Notice that the object header thing says <no private> while the actual object says there is a private. ;)  That could just be a bug in the printing code that decides what the header should show.

Comment 9

[Andrew McCreight [:mccr8]]

Not fixed by my patch in bug 752340.

Comment 10

[Andrew McCreight [:mccr8]]

Here's the sequence of events.

1. We grab btoa, then set its proto to the text node, which changes its shape and type.

2. Then, we pass a proxy into nsXPCComponents_Utils::LookupMethod, which becomes obj.  The private of the proxy is btoa.  The parent of the proxy is the mJSProtoObject of a window.

3. The first thing LookupMethod does is go up the proto-chain, but js::IsWrapper(obj) is true, so obj remains unchanged.

4. Then we call GetJSObjectOfWrapper on obj, which returns the text node.

5. JS_ObjectToInnerObject is called, but that doesn't change unwrappedObject.

6. Then there's some XPCCallContext stuff I don't really understand that digs around to get the method, which succeeds.

7. Then we call NewFunctionObject, which creates the new function object.  The parent of this new function object is obj, which as I said above is a proxy for btoa.

8. Then later we're in GetWrappedNativeOfJSObject for some reason.  We unwrap the parent of hasChildNodes, which gets us btoa.  This isn't a proto, wrapper or tearoff, so we hit the assertion.

My guess would be that something is going weird in step 3, but I'm not really sure what that code (or most of this code, really) should be doing.

Comment 11

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #10)
> Here's the sequence of events.
> 
> 1. We grab btoa, then set its proto to the text node, which changes its
> shape and type.

More specifically, we set its proto to a CCW to the text node, since it's in the outer document's compartment.

> 
> 2. Then, we pass a proxy into nsXPCComponents_Utils::LookupMethod, which
> becomes obj.  The private of the proxy is btoa.  The parent of the proxy is
> the mJSProtoObject of a window.
> 
> 3. The first thing LookupMethod does is go up the proto-chain, but
> js::IsWrapper(obj) is true, so obj remains unchanged.
> 
> 4. Then we call GetJSObjectOfWrapper on obj, which returns the text node.

Wow, nsXPConnect::GetJSObjectOfWrapper is some bizarre stuff. I didn't even know it existed. It seems to me like its primary purpose here is to bust through security wrappers. I'm sort of surprised we're not hitting cross-compartment assertions, here...

> 
> 5. JS_ObjectToInnerObject is called, but that doesn't change unwrappedObject.
> 
> 6. Then there's some XPCCallContext stuff I don't really understand that
> digs around to get the method, which succeeds.

So this seems like the bug, IMO. I don't think we should be passing arbitrary js objects to XPCCallContext and expect to get something sane back. XrayWrapper does this, but it does it with very controlled objects that don't have anything funky like this going on.

Anyway, reading through the implementation of LookupMethod here gives me the creeps. I've never even looked through this codepath before, and it sure looks to me like there are lots of little dark corners to exploit.

The thing is, XrayWrapper does the same thing, in a saner way. So as long as this method is exposed to content JS (which isn't forever, but is the case for the immediate future), I think we should re-implement it in terms of XrayWrapper. Here are the steps, as far as I see:

When the object comes in, do a CheckedUnwrap (which does a PUNCTURE at every level of security wrappers, throwing if we're leave same-origin boundaries). Then enter the compartment of the underlying object.

Then, we check CanXray on the underlying object. If we can't, we throw. If we can, we construct an ephemeral same-compartment Xray wrapper around the object. We then call getPropertyDescriptor on the Xray wrapper. If it comes back as a callable value attribute, then we've found what we're looking for. We leave the object's compartment, wrap the method, and return.

What do you think, mccr8? Does this sound like a sane plan? I can take a crack at it next week if you'd prefer to skip it. ;-)

Comment 12

[Andrew McCreight [:mccr8]]

I know very little about XrayWrappers, but it does sound like a good idea to use them implement a method that is supposed to imitate them.  I'm fine with you taking this.  I should try to learn about XrayWrappers at some point, but I'm okay with it if it isn't in the near future.

(Bug 693733 talks about making lookupMethod not available to content.)

Comment 13

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #12)
> (Bug 693733 talks about making lookupMethod not available to content.)

Indeed, but we can't do that until we find another way to make it available to XBL, since Flashblock needs it. This will all be fixed proper when we make |Components| available only to XBL.

Comment 14

[Bobby Holley (:bholley)]

I'm reimplementing lookupMethod over in bug 774245. I'm conservatively not marking the dep to avoid indicating that that bug fixes a known security issue, though I think it'd probably be fine to mark it as well.

Comment 15

[Jesse Ruderman]

> Indeed, but we can't do that until we find another way to make it available to XBL, 
> since Flashblock needs it.

Or we could give Flashblock a more direct way to do what it wants to do.

> This will all be fixed proper when we make |Components| available only to XBL.

Will XBL scripts be shielded from page scripts if that happens?

Comment 16

[Bobby Holley (:bholley)]

(In reply to Jesse Ruderman from comment #15)
> Or we could give Flashblock a more direct way to do what it wants to do.

Do you have any suggestions?

> > This will all be fixed proper when we make |Components| available only to XBL.
> Will XBL scripts be shielded from page scripts if that happens?

Somewhat. The idea is that |Components| will live on a special enclosing scope for XBL code that isn't otherwise accessible to content. It's still totally possible for XBL to leak Components to content, though.

Comment 17

[Jesse Ruderman]

We should ensure Flashblock can hook into Firefox's own plugin blocking and click-to-play mechanisms.  That would be better than adding complexity to keep the XBL Flashblock alive.

Comment 18

[Bobby Holley (:bholley)]

(In reply to Jesse Ruderman from comment #17)
> We should ensure Flashblock can hook into Firefox's own plugin blocking and
> click-to-play mechanisms.  That would be better than adding complexity to
> keep the XBL Flashblock alive.

The Components-for-XBL hack is needed for more than just flashblock, so that's going to happen anyway.

As for LookupMethod, we could also just kill it, but that's a pretty high-risk move in response to a security bug. Specifically, if we find out that it breaks the web, the backout will put us in a bad place. So I'm going to land the reimplementation for now, which is a step in the right direction.

That being said, if flashblock can wean itself off lookupMethod, I'd like to try killing it for content and see what happens. If we kill it for content, we can probably kill it for chrome too, since chrome gets this behavior automatically.

Comment 19

[Jesse Ruderman]

Bug 693733 is another (weird) reason to remove lookupMethod from the web.  But sure, avoiding a backout while fixing this more severe bug makes sense.

Comment 20

[Andrew McCreight [:mccr8]]

This test case now produces the following:

WARNING: Components.lookupMethod deprecated, use Components.utils.lookupMethod: file /Users/amccreight/mz/cent4/js/xpconnect/src/XPCComponents.cpp, line 4754
JavaScript error: file:///Users/amccreight/mz/tests/760887.html, line 18: NS_ERROR_XPC_BAD_CONVERT_JS: Component returned failure code: 0x80570009 (NS_ERROR_XPC_BAD_CONVERT_JS) [nsIXPCComponents.lookupMethod]

Comment 21

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #20)
> This test case now produces the following:
> 
> WARNING: Components.lookupMethod deprecated, use
> Components.utils.lookupMethod: file
> /Users/amccreight/mz/cent4/js/xpconnect/src/XPCComponents.cpp, line 4754
> JavaScript error: file:///Users/amccreight/mz/tests/760887.html, line 18:
> NS_ERROR_XPC_BAD_CONVERT_JS: Component returned failure code: 0x80570009
> (NS_ERROR_XPC_BAD_CONVERT_JS) [nsIXPCComponents.lookupMethod]

SGTM :-)

Comment 22

[Andrew McCreight [:mccr8]]

I guess we can call this fixed in 17.  I don't know how scary it is to backport bug 774245.

Comment 23

[Andrew McCreight [:mccr8]]

At this point, I imagine bug 774245 isn't going to be backported to beta, so I'm just going to mark this wontfix.

Comment 24

[Daniel Veditz [:dveditz]]

I don't know how to rate this from a security standpoint, and it may not be worth a lot of effort getting it right since we've fixed it now. Would the same errors crop up with more interesting objects than btoa? Going with sec-high just because wrapper bugs give me the willies.

Comment 25

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old