Last Comment Bug 370127 - XPCNativeWrapper function wrapper's __proto__ comes from content
: XPCNativeWrapper function wrapper's __proto__ comes from content
Status: VERIFIED FIXED
[sg:moderate?] keep private for bug 3...
: verified1.8.0.13, verified1.8.1.5
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: x86 Windows XP
: -- normal (vote)
: ---
Assigned To: Blake Kaplan (:mrbkap)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2007-02-12 01:23 PST by moz_bug_r_a4
Modified: 2008-10-10 14:24 PDT (History)
12 users (show)
jst: blocking1.9+
dveditz: blocking1.8.1.5+
dveditz: wanted1.8.1.x+
dveditz: blocking1.8.0.13+
dveditz: wanted1.8.0.x+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Like so (3.14 KB, patch)
2007-03-30 22:18 PDT, Boris Zbarsky [:bz] (TPAC)
no flags Details | Diff | Splinter Review
Fixed fix (1.38 KB, patch)
2007-05-23 15:05 PDT, Blake Kaplan (:mrbkap)
bzbarsky: review-
jst: review+
jst: superreview+
Details | Diff | Splinter Review
Updated to comments (2.04 KB, patch)
2007-05-25 15:14 PDT, Blake Kaplan (:mrbkap)
brendan: review+
bzbarsky: superreview+
dveditz: approval1.8.1.5+
dveditz: approval1.8.0.13+
Details | Diff | Splinter Review

Comment 1 Boris Zbarsky [:bz] (TPAC) 2007-03-30 22:05:25 PDT
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 [:bz] (TPAC) 2007-03-30 22:18:58 PDT
Created attachment 260211 [details] [diff] [review]
Like so

Unfortunately, this doesn't seem to fix the bug...
Comment 3 Johnny Stenback (:jst, jst@mozilla.com) 2007-05-22 16:27:19 PDT
Blake, you know you love these wrappers! Wanna have a look?
Comment 4 Blake Kaplan (:mrbkap) 2007-05-23 15:04:20 PDT
(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) 2007-05-23 15:05:07 PDT
Created attachment 265851 [details] [diff] [review]
Fixed fix
Comment 6 Johnny Stenback (:jst, jst@mozilla.com) 2007-05-23 17:07:06 PDT
Comment on attachment 265851 [details] [diff] [review]
Fixed fix

r+sr=jst
Comment 7 Blake Kaplan (:mrbkap) 2007-05-23 17:18:18 PDT
Fix checked into trunk.
Comment 8 Boris Zbarsky [:bz] (TPAC) 2007-05-23 17:32:17 PDT
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) 2007-05-23 17:47:12 PDT
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 [:bz] (TPAC) 2007-05-23 17:50:46 PDT
> 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) 2007-05-23 17:52:04 PDT
I backed the patch out.
Comment 12 Blake Kaplan (:mrbkap) 2007-05-23 18:07:20 PDT
(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 [:bz] (TPAC) 2007-05-23 18:19:28 PDT
> 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) 2007-05-23 18:54:24 PDT
(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 [:bz] (TPAC) 2007-05-23 20:25:17 PDT
Makes sense.  So let's make this code fail out in that case.
Comment 16 Blake Kaplan (:mrbkap) 2007-05-25 15:14:24 PDT
Created attachment 266125 [details] [diff] [review]
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 [:bz] (TPAC) 2007-05-25 22:41:05 PDT
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) 2007-05-26 01:42:47 PDT
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] 2007-05-29 23:46:07 PDT
I'll review in the morning.

/be
Comment 20 Blake Kaplan (:mrbkap) 2007-06-04 14:44:07 PDT
Fix checked into trunk.
Comment 21 Blake Kaplan (:mrbkap) 2007-07-10 17:54:05 PDT
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] 2007-07-10 18:10:40 PDT
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) 2007-07-10 18:30:49 PDT
Fixed on the 1.8 branches.
Comment 24 juan becerra [:juanb] 2007-08-22 17:56:44 PDT
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) 2007-08-22 19:59:37 PDT
(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] 2007-08-22 20:11:00 PDT
v.fixed with trunk nightly and 2006
Comment 27 Carsten Book [:Tomcat] 2007-08-23 07:52:33 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.