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

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: jruderman, Assigned: bholley)

Tracking

(Blocks 2 bugs, {assertion, sec-high, testcase})

Trunk
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox14 unaffected, firefox15 wontfix, firefox16 wontfix, firefox17 fixed, firefox-esr10 unaffected)

Details

(Whiteboard: [adv-track-main17+] fixed by bug 774245)

Attachments

(2 attachments)

Reporter

Description

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

Smells cpg-related.
Reporter

Comment 1

7 years ago
Posted file stack trace
bholley thinks this sounds a lot like bug 752764, and I agree, so I'll look at it to see what is going on.
Assignee: nobody → continuation
WFM on a local build of Fx16 with bug 751995 and bug 758415 applied.  So 16 is probably fixed.  Hurray!
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.
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?
Assignee

Comment 6

7 years ago
(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...
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.
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.
Not fixed by my patch in bug 752340.
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.
Assignee

Comment 11

7 years ago
(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. ;-)
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.)
Assignee: continuation → bobbyholley+bmo
Assignee

Comment 13

7 years ago
(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.
Assignee

Comment 14

7 years ago
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.
Reporter

Comment 15

7 years ago
> 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?
Whiteboard: to be fixed by bug 774245
Assignee

Comment 16

7 years ago
(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.
Reporter

Comment 17

7 years ago
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.
Assignee

Comment 18

7 years ago
(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.
Reporter

Comment 19

7 years ago
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.
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]
Assignee

Comment 21

7 years ago
(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 :-)
I guess we can call this fixed in 17.  I don't know how scary it is to backport bug 774245.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Whiteboard: to be fixed by bug 774245 → fixed by bug 774245
At this point, I imagine bug 774245 isn't going to be backported to beta, so I'm just going to mark this wontfix.
Whiteboard: fixed by bug 774245 → [adv-track-main17+] fixed by bug 774245
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.
Depends on: 774245
Keywords: sec-high
Group: core-security
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.