370127 - XPCNativeWrapper function wrapper's __proto__ comes from content

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Comment 1

[Boris Zbarsky [:bzbarsky]]

So the issue is basically that we pass the unsafe function as the parent to JS_NewFunction.

Could we perhaps pass in the XPCNativeWrapper object itself, and then reset the parent on the function object manually once the function is all set up?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Like so

Unfortunately, this doesn't seem to fix the bug...

Comment 3

[Johnny Stenback (:jst)]

Blake, you know you love these wrappers! Wanna have a look?

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #1)
> Could we perhaps pass in the XPCNativeWrapper object itself, and then reset the
> parent on the function object manually once the function is all set up?

Almost, but the XPCNativeWrapper mirrors its XPCWrappedNative's parent, meaning that it comes from content as well. I think that it's sufficient to pass in nsnull for the parent, since we'll then use the top of the JS stack for the parent, which should be chrome (or, if content is doing this, then we don't really care where the proto comes from).

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fixed fix

Comment 6

[Johnny Stenback (:jst)]

Comment on attachment 265851 [details] [diff] [review]
Fixed fix

r+sr=jst

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 265851 [details] [diff] [review]
Fixed fix

I have to admit I'm not completely happy with this fix.

Are we sure this code always gets hit from JS code?  That is, that it never gets hit from C++?  In particular, note the fix for bug 337095 (even if we ignore the deep wrapping that might happen under JSAPI calls on XPCNativeWrapper objects from C++).

If cx->fp is null, we'll use the global object of cx as the proto, which doesn't seem like the thing we really want, does it?

Maybe it doesn't matter what the proto is in the C++ cases?  That seems possible, I guess...  But maybe we should also just make XPCNativeWrapper::GetNewOrUsed take a scope object?  That seems a lot safer, no?

In any case, I think we want some comments here that the parent in the JS_NewFunction call is very important, maybe even referencing this bug directly.

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

What would happen when we got two different scope objects for the same wrapped native? Which one would we use?

Would it work if we could create a "pristine" Function.prototype to use that had elevated privileges for all wrappers?

Comment 10

[Boris Zbarsky [:bzbarsky]]

> What would happen when we got two different scope objects for the same wrapped
> native?

Hmm...  That's a good question.  Ideally we wouldn't share XPCNativeWrappers in that case, right?

But also, given that we're not talking about creating new XPCNativeWrappers (I think I somehow missed that) but about their function-wrapping stuff, _can_ it be hit from C++ usefully?  I'm guessing yes?

Not sure about the other question; someone who understands this stuff better (esp. the security checks jseng does) should cover it.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

I backed the patch out.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #10)
> Hmm...  That's a good question.  Ideally we wouldn't share XPCNativeWrappers in
> that case, right?

I'm not sure what you mean. It seems that we can only have one native wrapper per wrapped native, but a wrapped native can be shared amongst many different scopes (although we're now talking about component and other chrome scopes since we're talking about GetNewOrUsed, so maybe it doesn't really matter).

> But also, given that we're not talking about creating new XPCNativeWrappers (I
> think I somehow missed that) but about their function-wrapping stuff, _can_ it
> be hit from C++ usefully?  I'm guessing yes?

Well, sure, through the JS API. Of course, anybody using an XPCNativeWrapper via the JS API in C++ might as well just get the underlying native out of it to avoid weirdness like this. Although, if that were the case, I'd hope that the C++ code would be passing in a context whose global object was privileged and untouched by untrusted content.

Who would do this anyway?

Comment 13

[Boris Zbarsky [:bzbarsky]]

> I'm not sure what you mean.

That if the scopes don't match with an existing automatic deep wrapper, we should just create a new one.

> Who would do this anyway?

I guess they'd need to JS_GetProperty the function and then pass the result over to unprivileged code for it to be a problem?

If that's the case, I agree that it seems like a long shot.  Perhaps we could even fail in the relevant code if cx->fp is null or something?

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #13)
> I guess they'd need to JS_GetProperty the function and then pass the result
> over to unprivileged code for it to be a problem?

Well, they'd need to look up the function, passing in a context whose global object has been exposed to untrusted content, and then get a property that's on either Function.prototype or Object.prototype.

> If that's the case, I agree that it seems like a long shot.  Perhaps we could
> even fail in the relevant code if cx->fp is null or something?

I kind of want to do that anyway. I don't think this is common enough to be a problem; at least in Mozilla, I don't think anybody deals with XPCNativeWrappers in C++ in JSObject * form outside of XPConnect.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Makes sense.  So let's make this code fail out in that case.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated to comments

If this turns out to break things, we could probably use the safe context, which would let us inherit its Function.prototype.

Comment 17

[Boris Zbarsky [:bzbarsky]]

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

>Index: js/src/xpconnect/src/XPCNativeWrapper.cpp
>+    ::JS_ReportError(cx, "XPCNativeWrappers must be used from script");

Why is this playing nice with XPConnect (e.g., why no need for ThrowException)?  I'm willing to believe it's fine, but someone familiar with the JSAPI should review this.

>+    return nsnull;

You mean JS_FALSE, right?

sr=bzbarsky with the above addressed; I guess brendan is a reasonable reviewer....

Not switching review myself because you should get the review mail when it happens, and if I request it I'll get the mail.

Comment 18

[Blake Kaplan (:mrbkap) (inactive)]

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

I considered using XPCThrower, but I'm worried that somehow someone's going to end up hitting this case and getting confused about exactly why they're getting denied.

Comment 19

[Brendan Eich [:brendan]]

I'll review in the morning.

/be

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 21

[Blake Kaplan (:mrbkap) (inactive)]

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

This applies to the 1.8 and 1.8.0 branches as-is.

Comment 22

[Daniel Veditz [:dveditz]]

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

approved for 1.8.1.5 and 1.8.0.13, a=dveditz

Comment 23

[Blake Kaplan (:mrbkap) (inactive)]

Fixed on the 1.8 branches.

Comment 24

[juan becerra [:juanb]]

In Fx2004 when I evaluate any of the three lines of code in the testcase in comment #0, I get a dialog, but in Fx2005 I get a message in the Error Console: 

Error: uncaught exception: [Exception... "Unexpected error"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: javascript: (new XPCNativeWrapper(top.opener.content)).focus.call();1; :: <TOP_LEVEL> :: line 1"  data: no]

Blake, is this the expected result after the fix?

Comment 25

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to comment #24)
> Blake, is this the expected result after the fix?

Yes, it is. In particular, calling the XPCNativeWrapper's function wrapper with a window object as 'this' will throw an exception. XPCNativeWrapper is not a generic wrapper as XPCSafeJSObjectWrapper and XPCCrossOriginWrapper are.

Comment 26

[Jay Patel [:jay]]

v.fixed with trunk nightly and 2006

Comment 27

[Carsten Book [:Tomcat]]

verified fixed 1.8.0.13 with Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.8.0.13pre) Gecko/20070822 Firefox/1.5.0.13pre and the testcase in this bug.

Adding verified keyword.