Nuke XPCNativeWrapper.cpp:MirrorWrappedNativeParent

RESOLVED FIXED

Status

()

Core
XPConnect
RESOLVED FIXED
8 years ago
7 years ago

People

(Reporter: mrbkap, Assigned: mrbkap)

Tracking

Trunk
x86
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [compartments] fixed-in-tracemonkey)

Attachments

(2 attachments)

(Assignee)

Description

8 years ago
Nobody remembers why this was done (other than the fact that it's semantically more correct to wrap the parent chain) and as far as I can tell, it is impossible to use an XPCNativeWrapped node as the scope chain (with doesn't count).

In <button onclick="foo"> the function compiled around "foo" is parented by the button node, but this is lexically limited to the untrusted node.

The other methods of setting event listeners only take functions whose scope chains are set at compile time, lexcially.
Other than purity of bodily fluids, the parent chain is what we walk to get the object principal.   I seem to recall this mattering at the time.  I also seem to recall us not wanting .__parent__ on an XPCWrappedNative to accidentally toss an unwrapped node at the chrome and screw it over and other such worries.
(Assignee)

Comment 2

8 years ago
(In reply to comment #1)
> Other than purity of bodily fluids, the parent chain is what we walk to get the
> object principal.   I seem to recall this mattering at the time.

I don't think this matters anymore -- we've moved the security checks inside the wrappers and if we create a wrapper for an object, the enclosing scope had better be able to access the wrapper itself.

> I also seem
> to recall us not wanting .__parent__ on an XPCWrappedNative to accidentally
> toss an unwrapped node at the chrome and screw it over and other such worries.

We don't have to worry about __parent__ because it won't resolve in the idl (unless someone actually has an idl property called __parent__) and we null out the prototype, so the magical obj_getSlot getter will never be called.
(Assignee)

Comment 3

7 years ago
Created attachment 464668 [details] [diff] [review]
Fix
Assignee: nobody → mrbkap
Status: NEW → ASSIGNED
Attachment #464668 - Flags: review?(jorendorff)
(Assignee)

Comment 4

7 years ago
Created attachment 464670 [details] [diff] [review]
Reparent XPCNWs when their wrapped object is reparented

This is needed for making the outer window not a global object. Otherwise, we end up asserting when using the XPCNativeWrappers, whose scope was still the old inner window.
Attachment #464670 - Flags: review?(jorendorff)
(Assignee)

Updated

7 years ago
Blocks: 586083
(In reply to comment #2)
> (In reply to comment #1)
> > I also seem
> > to recall us not wanting .__parent__ on an XPCWrappedNative to accidentally
> > toss an unwrapped node at the chrome and screw it over and other such worries.
> 
> We don't have to worry about __parent__ because it won't resolve in the idl
> (unless someone actually has an idl property called __parent__) and we null out
> the prototype, so the magical obj_getSlot getter will never be called.

__parent__ is toast. Waldo killed it.
(In reply to comment #2)
> (In reply to comment #1)
> > Other than purity of bodily fluids, the parent chain is what we walk to get the
> > object principal.   I seem to recall this mattering at the time.
> 
> I don't think this matters anymore -- we've moved the security checks inside
> the wrappers and if we create a wrapper for an object, the enclosing scope had
> better be able to access the wrapper itself.

Let me echo this back. I only partly understand.

1. The first patch here changes the principals of XPCNativeWrappers. With the patch, they'll be parented to the wrapped object's global, so they'll have the same principals as the wrapped object.  (...But what were they before? NULL?)

2. This does not change the subject principals of any code, because as comment 0 explains, XPCNWs are never on the scope chain.

3. This does change the object principals of some objects, but that's OK.

4a. This does not change the result of Components.utils.getGlobalForObject(xpcnw) when xpcnw wraps a WN whose global is an inner window. It still returns the outer window.

4b. What about when xpcnw wraps something whose global is a sandbox? We should return a wrapper of the sandbox, right? Do we? Will we continue to do so after this change?
(Assignee)

Comment 7

7 years ago
(In reply to comment #6)
> 1. The first patch here changes the principals of XPCNativeWrappers. With the
> patch, they'll be parented to the wrapped object's global, so they'll have the
> same principals as the wrapped object.  (...But what were they before? NULL?)

They haven't changed. When MirrorWrappedNativeParent when creating an XPCNativeWrapper around a global object, we parent that native wrapper to the global for that wrapped native scope. So the parent chain would look like XPCNW(obj1)->XPCNW(obj1.parent)->XPCNW(obj1.parent.parent)->obj1.parent.parent. This patch just gets rid of the intermediate XPCNativeWrappers.

> 3. This does change the object principals of some objects, but that's OK.

Actually, given the above, I don't even think there should be a change here.

> 4a. This does not change the result of
> Components.utils.getGlobalForObject(xpcnw) when xpcnw wraps a WN whose global
> is an inner window. It still returns the outer window.

Yes, but...

> 4b. What about when xpcnw wraps something whose global is a sandbox? We should
> return a wrapper of the sandbox, right? Do we? Will we continue to do so after
> this change?

This is an existing problem. We need to make sure that the return value of getGlobalForObject is wrapped in the right security/cross compartment wrapper.

Updated

7 years ago
Whiteboard: [compartments]
Intuitive argument from the old days: NW lineage should mirror WN in case some crazy DOM event handler compatibility thing requires it.

Is 4b on file separately?

/be
(Assignee)

Comment 9

7 years ago
(In reply to comment #8)
> Intuitive argument from the old days: NW lineage should mirror WN in case some
> crazy DOM event handler compatibility thing requires it.

Yeah. I'm putting my foot down here and saying we don't have to worry about that. 4b is bug 586823.
Comment on attachment 464668 [details] [diff] [review]
Fix

OK. Looks good.
Attachment #464668 - Flags: review?(jorendorff) → review+
Attachment #464670 - Flags: review?(jorendorff) → review+
(Assignee)

Comment 11

7 years ago
http://hg.mozilla.org/tracemonkey/rev/601d80023858
http://hg.mozilla.org/tracemonkey/rev/e5098ff8e0ce
Whiteboard: [compartments] → [compartments] fixed-in-tracemonkey

Comment 12

7 years ago
http://hg.mozilla.org/mozilla-central/rev/601d80023858
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.