XPCNativeWrapper function wrapper's __proto__ comes from content

VERIFIED FIXED

Status

()

Core
XPConnect
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: moz_bug_r_a4, Assigned: mrbkap)

Tracking

({verified1.8.0.13, verified1.8.1.5})

Trunk
x86
Windows XP
verified1.8.0.13, verified1.8.1.5
Points:
---
Bug Flags:
blocking1.9 +
blocking1.8.1.5 +
wanted1.8.1.x +
blocking1.8.0.13 +
wanted1.8.0.x +

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 2 obsolete attachments)

Comment 1

11 years ago
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?

Comment 2

11 years ago
Created attachment 260211 [details] [diff] [review]
Like so

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+
(Assignee)

Comment 4

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

Updated

10 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 5

10 years ago
Created attachment 265851 [details] [diff] [review]
Fixed fix
Attachment #260211 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
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+
(Assignee)

Comment 7

10 years ago
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 8

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

Comment 9

10 years ago
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 → ---

Comment 10

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

Comment 11

10 years ago
I backed the patch out.
Status: REOPENED → ASSIGNED
(Assignee)

Comment 12

10 years ago
(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

10 years ago
> 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?
(Assignee)

Comment 14

10 years ago
(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

10 years ago
Makes sense.  So let's make this code fail out in that case.
(Assignee)

Comment 16

10 years ago
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.
Attachment #265851 - Attachment is obsolete: true
Attachment #266125 - Flags: superreview?(bzbarsky)
Attachment #266125 - Flags: review?(bzbarsky)

Comment 17

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

Comment 18

10 years ago
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

Updated

10 years ago
Attachment #266125 - Flags: review?(brendan) → review+
(Assignee)

Comment 20

10 years ago
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago10 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+
(Assignee)

Comment 21

10 years ago
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+
(Assignee)

Comment 23

10 years ago
Fixed on the 1.8 branches.
Keywords: fixed1.8.0.13, fixed1.8.1.5
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?
(Assignee)

Comment 25

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

Updated

10 years ago
Keywords: fixed1.8.1.5 → verified1.8.1.5

Comment 26

10 years ago
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.
Keywords: fixed1.8.0.13 → verified1.8.0.13
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.