Closed Bug 370127 Opened 17 years ago Closed 17 years ago

XPCNativeWrapper function wrapper's __proto__ comes from content

Categories

(Core :: XPConnect, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: moz_bug_r_a4, Assigned: mrbkap)

Details

(Keywords: verified1.8.0.13, verified1.8.1.5, Whiteboard: [sg:moderate?] keep private for bug 344495. booby-trap for chrome, don't know specific vulnerable chrome)

Attachments

(1 file, 2 obsolete files)

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?
Flags: blocking1.9?
Attached patch Like so (obsolete) — Splinter Review
Unfortunately, this doesn't seem to fix the bug...
Blake, you know you love these wrappers! Wanna have a look?
Assignee: nobody → mrbkap
Flags: blocking1.9? → blocking1.9+
(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).
Status: NEW → ASSIGNED
Attached patch Fixed fix (obsolete) — Splinter Review
Attachment #260211 - Attachment is obsolete: true
Attachment #265851 - Flags: superreview?(jst)
Attachment #265851 - Flags: review?(jst)
Attachment #265851 - Flags: review?(bzbarsky)
Comment on attachment 265851 [details] [diff] [review]
Fixed fix

r+sr=jst
Attachment #265851 - Flags: superreview?(jst)
Attachment #265851 - Flags: superreview+
Attachment #265851 - Flags: review?(jst)
Attachment #265851 - Flags: review+
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
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.
Attachment #265851 - Flags: review?(bzbarsky) → review-
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
> 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.
I backed the patch out.
Status: REOPENED → ASSIGNED
(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?
> 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?
(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.
Makes sense.  So let's make this code fail out in that case.
If this turns out to break things, we could probably use the safe context, which would let us inherit its Function.prototype.
Attachment #265851 - Attachment is obsolete: true
Attachment #266125 - Flags: superreview?(bzbarsky)
Attachment #266125 - Flags: review?(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.
Attachment #266125 - Flags: superreview?(bzbarsky)
Attachment #266125 - Flags: superreview+
Attachment #266125 - Flags: review?(bzbarsky)
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.
Attachment #266125 - Flags: review?(brendan)
I'll review in the morning.

/be
Attachment #266125 - Flags: review?(brendan) → review+
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
Flags: blocking1.8.1.5?
Flags: blocking1.8.0.13?
Whiteboard: [sg:critical?] booby-trap for chrome, don't know specific vulnerable chrome
Flags: blocking1.8.1.5?
Flags: blocking1.8.1.5+
Flags: blocking1.8.0.13?
Flags: blocking1.8.0.13+
Comment on attachment 266125 [details] [diff] [review]
Updated to comments

This applies to the 1.8 and 1.8.0 branches as-is.
Attachment #266125 - Flags: approval1.8.1.5?
Attachment #266125 - Flags: approval1.8.0.13?
Whiteboard: [sg:critical?] booby-trap for chrome, don't know specific vulnerable chrome → [sg:moderate?] booby-trap for chrome, don't know specific vulnerable chrome
Comment on attachment 266125 [details] [diff] [review]
Updated to comments

approved for 1.8.1.5 and 1.8.0.13, a=dveditz
Attachment #266125 - Flags: approval1.8.1.5?
Attachment #266125 - Flags: approval1.8.1.5+
Attachment #266125 - Flags: approval1.8.0.13?
Attachment #266125 - Flags: approval1.8.0.13+
Fixed on the 1.8 branches.
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?
(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.
v.fixed with trunk nightly and 2006
Status: RESOLVED → VERIFIED
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.
Whiteboard: [sg:moderate?] booby-trap for chrome, don't know specific vulnerable chrome → [sg:moderate?] keep private for bug 344495. booby-trap for chrome, don't know specific vulnerable chrome
Group: core-security
You need to log in before you can comment on or make changes to this bug.