956404 - Audit uses of nsContentUtils::WrapNative

Status: RESOLVED DUPLICATE of bug 733636

(Core :: XPConnect, defect)

Description

[Andrew McCreight [:mccr8]]

I kind of dropped the ball on this, but I'm looking at it again.  I have some kind of taxonomy of all the places we call this which I'll put in this bug.

The fix for this will live in bug 733636, but please don't mark the dependence.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: change WrapNative

Comment 2

[Andrew McCreight [:mccr8]]

try run is green (I think slightly different than the attached patch):
  https://tbpl.mozilla.org/?tree=Try&rev=5cfa8f3f1177

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: de-defaultify nsContentUtils::WrapNative

Comment 4

[Andrew McCreight [:mccr8]]

First, here are the things that look like they should be for sure okay:

already pass in true
  - nsContentUtils.cpp
  - nsFrameMessageManager::ReceiveMessage
  - nsXMLHttpRequest::GetResponse
  - nsXBLPrototypeHandler::ExecuteHandler
  - Navigator::DoNewResolve,
  - nsTArrayHelpers

wrappee is newly created, so it can't have a security wrapper
 - CustomElementConstructor in nsDocument.cpp
 - nsGlobalWindow::PostMessageReadStructuredClone
 - IndexedDatabaseManager::InitWindowless
 - SmsManager::Send
 - MobileMessageManager::Send
 - CreateXMLHttpRequest in XPCComponents.cpp

wrappee isn't wrapper cached (mostly nsIDOMBlob or nsIDOMFile)
  - ArchiveRequest::GetFileResult
  - ArchiveRequest::GetFilesResult
  - GetFileHelper::GetSuccessResult
  - IDBObjectStore::StructuredCloneReadCallback
  - Read in StructuredCloneUtils.cpp
  - MmsMessage::GetAttachments (?)
  - WorkerPrivate structured clone thing
  - MmsAttachmentDataToJSObject
  - MobileMessageCallback::NotifySuccess (wraps SmsSegmentInfo)
  - MobileMessageCursorCallback::NotifyCursorResult (wraps SmsMessage)
  - InterfaceToJsval in nsDeviceStorage.cpp: two callers.
     nsIFileToJsval (wraps nsIDOMBlob), and
     DeviceStorageRequestChild::Recv__delete__ (wraps nsIDOMFile)
  - SmsRequest::SetSuccessInternal: argument is nsISupports*, but
  it is only called from two places, with classes that aren't
  wrappercached.
  - nsXULTemplateBuilder::InitHTMLTemplateRoot: last two uses
  (wraps nsIRDFCompositeDataSource and nsIXULTemplateBuilder)

Comment 5

[Andrew McCreight [:mccr8]]

This next set look okay to me as is, but could use some double checking:

wrapping into the same compartment so it is okay (?)
  - nsGlobalWindow::GetContent: gets content, wraps it (?)

these seem okay to use with false
  - nsIDocument::AdoptNode (explicitly passes false, so okay?)
  - nsIDocument::WrapObject (explicitly passes false, so okay?)
  - nsXBLProtoImplAnonymousMethod::Execute: calls JS_WrapObject on
  the result of WrapNative, so I guess that's okay?
  - nsJSContext::JSObjectFromInterface: "We don't wrap here because we
  trust the JS engine to wrap the target later."  The comment is a
  little odd.  The one caller is
  nsJSContext::BindCompiledEventHandler, which enters the compartment
  of the object, so maybe that's okay?
  - nsJSContext::ConvertSupportsTojsvals: seems to pull a random
  nsISupports out of an array and wrap it. The only place that calls
  it is nsJSContext::SetProperty, which goes through and calls
  JS_WrapValue on everything, so I guess we should pass in false?
  I haven't tested this one yet.

Comment 6

[Andrew McCreight [:mccr8]]

doesn't look okay, setting to true passes tests:
  - nsXMLHttpRequest::GetInterface: QIs the XHR to some interface, wraps it, returns it.
  - xpcJSWeakReference::Get: wraps and returns some random nsISupports.
  - nsJSContext::AddSupportsPrimitiveTojsval: Grabs random nsISupports and wraps
  and returns it. Maybe this is only used with nsVariant which isn't wrapper cached?
  - HelperBase::WrapNative in AsyncConnectionHelper.cpp is just a thin
  wrapper for WrapNative.  Used in various GetSuccessResult methods
  for IDB stuff (such as IPCOpenDatabaseHelper, IPCSetVersionHelper,
  and ContinueHelper). I'm not sure why this is okay.
  - nsXULTemplateBuilder::InitHTMLTemplateRoot: first use wraps an element,
  it looks like. I'm not sure why that is okay.


doesn't look okay, and fliping it to true causes test failures:
  - nsXBLProtoImpl::InitTargetObjects: almost certainly bad. we wrap a bound
   element, but it could end up in another compartment. There are actually many
   compartment failures like this in the wild: it is the most common cause of
   compartment mismatches on Nightly, going by the last week or so of crash stats.

   example of failures:
   https://crash-stats.mozilla.com/report/index/16405524-21fe-404b-a108-b33032140103
   https://crash-stats.mozilla.com/report/index/41e0e3c8-7fdc-4504-96a3-921f72140102
   https://crash-stats.mozilla.com/report/index/409c1247-6da4-44e0-839a-cf84b2140102
   https://crash-stats.mozilla.com/report/index/13c59231-09f5-4fc1-a53e-f96442140101
   https://crash-stats.mozilla.com/report/index/4a92f43d-655a-4c8f-bda3-a673f2140101
   https://crash-stats.mozilla.com/report/index/8fc17ee5-b58e-4c31-989e-e5abb2140103
   https://crash-stats.mozilla.com/report/index/a28f8216-8975-4dfe-8022-885ac2140103
   https://crash-stats.mozilla.com/report/index/ea8059e1-56c4-47c4-bb57-6857e2140102

   (some of these seem like they are kind of dupes, with similar sets of addons)

   bholley pointed out this issue in bug 851353, which was fixed another way.

   But, the fun thing here is that setting this to true causes various failures, in things like:
      content/events/test/test_focus_disabled.html

Comment 7

[Andrew McCreight [:mccr8]]

I haven't really investigated the failures with InitTargetObjects yet.  Anyways, if I could get some feedback on comment 5 and 6, I'd appreciate it, Bobby.

Comment 8

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #5)
> wrapping into the same compartment so it is okay (?)
>   - nsGlobalWindow::GetContent: gets content, wraps it (?)

I don't follow. This seems like it's only safe if |aCx| is currently in the compartment of |content|'s global, which doesn't seem likely.

Presumably the callers of GetContent() might be expecting an unwrapped return value, but it doesn't seem like it. In fact, GetScriptableContent looks pretty dangerous as-is...

> these seem okay to use with false
>   - nsIDocument::AdoptNode (explicitly passes false, so okay?)

Yes, it seems like it's trying to get a scope.

>   - nsIDocument::WrapObject (explicitly passes false, so okay?)

This is only safe insofar as the document should already be same-compartment with the window. But I think it's playing with fire. Let's remove the explicit |false| and replace it with an assertion that the result is not a cross-compartment wrapper?

>   - nsXBLProtoImplAnonymousMethod::Execute: calls JS_WrapObject on
>   the result of WrapNative, so I guess that's okay?

Yeah, but it wouldn't hurt to wrap it in the first place. Presumably we'll get that when we flip the defualt?

>   - nsJSContext::JSObjectFromInterface: "We don't wrap here because we
>   trust the JS engine to wrap the target later."  The comment is a
>   little odd.  The one caller is
>   nsJSContext::BindCompiledEventHandler, which enters the compartment
>   of the object, so maybe that's okay?

I guess so.

>   - nsJSContext::ConvertSupportsTojsvals: seems to pull a random
>   nsISupports out of an array and wrap it. The only place that calls
>   it is nsJSContext::SetProperty, which goes through and calls
>   JS_WrapValue on everything, so I guess we should pass in false?
>   I haven't tested this one yet.

Is there any reason to pass false? There's no harm in wrapping things twice, and it makes it less of a footgun.

Comment 9

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #6)
> doesn't look okay, and fliping it to true causes test failures:
>   - nsXBLProtoImpl::InitTargetObjects: almost certainly bad. we wrap a bound
>    element, but it could end up in another compartment. There are actually
> many
>    compartment failures like this in the wild: it is the most common cause of
>    compartment mismatches on Nightly, going by the last week or so of crash
> stats.

>    But, the fun thing here is that setting this to true causes various
> failures, in things like:
>       content/events/test/test_focus_disabled.html

Hm.

So, the bound element's reflector _should_ be same-compartment with its OwnerDoc's reflector, which in this case should be the compartment of |global|.

Of course, in this case, |cx| is in no compartment in particular (which should definitely be fixed - we should unscope the enter of |global|'s compartment).

But the only thing that ends after the WrapNative() call is a call to nsXBLPrototypeBinding::InitClass(), which bounces directly to nsXBLBinding::DoInitJSClass(), whose first order of business is to enter the compartment of |global|. So at first glance, this should be ok (though fragile without the change I proposed above).

Nevertheless, the fact that you get gets failures when you pass in |true| presumably must mean that there are cases when the element's reflector is not same-compartment with |global|. Can you reproduce it in a debugger? If so, we should be able to figure out what's going wrong.

Those JS_GetObjectId crashes are concerning. It looks like JS_GetPrototype doesn't assertSameCompartment, so either |obj| isn't same-compartment with |cx|, or isn't same-compartment with its proto. Given the other enforcement in the engine with respect to proto sets, I'd bet my money on the former. And this, along with your evidence of test failure (and thus behavior change), suggests that we have element that isn't same-compartment with its document, or a document that isn't same-compartment with its GetScopeObject.

GetScopeObject should return an inner, right? If it's returning an outer, and there's navigation involved, that would certainly explain it.

Comment 10

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #9)
> GetScopeObject should return an inner, right? If it's returning an outer,
> and there's navigation involved, that would certainly explain it.

I wish I knew more about XBL... Yes, it should return the inner, although by default it is set to the JunkScope, and then it has the be set manually to point to the inner (if it's a window). So I can imagine that somewhere it is not set (http://mxr.mozilla.org/mozilla-central/source/content/xbl/src/nsXBLService.cpp#1023 like here... is this OK btw?) or that something calls GetScopeObject before it were set to the right thing and gets the JunkScope accidentally.

Comment 11

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #10)
> I wish I knew more about XBL...

I'm happy to tell you more about it in SF in a few weeks. Just remind me. :P

> Yes, it should return the inner, although by
> default it is set to the JunkScope, and then it has the be set manually to
> point to the inner (if it's a window). So I can imagine that somewhere it is
> not set
> (http://mxr.mozilla.org/mozilla-central/source/content/xbl/src/nsXBLService.
> cpp#1023 like here... is this OK btw?)

Yeah, it's ok. The XBL binding document never gets any sort of window or global associated with it (other than an XBL compilation scope, which is specially-managed).

> or that something calls
> GetScopeObject before it were set to the right thing and gets the JunkScope
> accidentally.

This is a possibility. Andrew can verify.

Comment 12

[Andrew McCreight [:mccr8]]

> Nevertheless, the fact that you get gets failures when you pass in |true| presumably must mean that there are cases when the element's reflector is not same-compartment with |global|.

Looking into this a bit, when we pass "false", the resulting return value is still same-compartment with |global|.  When we pass in "true", we're failing somewhere inside Proxy::setPrototypeOf() in a way that ends up setting an exception, which we never handle, which causes an assertion later.  I'm currently trying to figure out what the proxy is that we're returning.

Comment 13

[Andrew McCreight [:mccr8]]

The good news is that a try run is green with everything flipped to true except for nsIDocument::AdoptNode and nsXBLProtoImpl::InitTargetObjects.

https://tbpl.mozilla.org/?tree=Try&rev=58f0b8771abe

I should also audit the other places that pass in false.  For instance, there are a number of calls to nsXPConnect::WrapNative, and it always passes in false, which seems questionable, given that the callers are various innocents outside XPConnect.  I'll file a followup for that at some point.

Comment 14

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #12)
> > Nevertheless, the fact that you get gets failures when you pass in |true| presumably must mean that there are cases when the element's reflector is not same-compartment with |global|.
> 
> Looking into this a bit, when we pass "false", the resulting return value is
> still same-compartment with |global|.

Ok. Presumably it's not a proxy?

> When we pass in "true", we're failing
> somewhere inside Proxy::setPrototypeOf()

During wrapping? If so, that's quite odd - if the underlying object is same-compartment with |global|, then the wrap call shouldn't be doing anything complicated.

> in a way that ends up setting an
> exception, which we never handle, which causes an assertion later.

Whatever we end up doing here, we should fix whatever path is failing to propagate errors.

> I'm
> currently trying to figure out what the proxy is that we're returning.

Sounds good.

Comment 15

[Andrew McCreight [:mccr8]]

When passing |true| in, the call to JS_SetPrototype in nsXBLBinding::DoInitJSClass ends up hitting SecurityWrapper<Base>::setPrototypeOf, which sets an exception and fails.  So that's where the exception comes from.  We don't hit that code when passing in |false|.

Putting this block right after the call to InitTargetObjects in nsXBLProtoImpl::InstallImplementation makes content/events/test/test_focus_disabled.html pass with |true| passed in to WrapNative:
  if (JS_IsExceptionPending(cx)) {
    JS_ClearPendingException(cx);
  }

...but various other tests like content/events/test/test_bug238987.html end up failing.  They hit the same setPrototypeOf, and look like they are failing in a similar way, but for some reason there's no exception pending at that point.  Then I get this error:
 0:05.26 TEST-UNEXPECTED-FAIL | unknown test url | uncaught exception - Error: permission denied to unwrap object at :0
 0:05.26 TEST-UNEXPECTED-FAIL | unknown test url | uncaught exception - Error: permission denied to unwrap object at :0

and then the test just falls apart.  So my guess is that test_focus_disabled.html for some reason doesn't care that nsXBLService::LoadBindings fails, but most tests do.  (The uncaught exception looks like the one that setPrototypeOf creates, so maybe that somehow magically gets handled higher up in the stack.)

Comment 16

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #15)
> When passing |true| in, the call to JS_SetPrototype in
> nsXBLBinding::DoInitJSClass ends up hitting
> SecurityWrapper<Base>::setPrototypeOf, which sets an exception and fails. 
> So that's where the exception comes from.  We don't hit that code when
> passing in |false|.

This is what I'm confused about - you said that the object returned when passing |false| was same-compartment with global, right? So how is it ending up with any sort of Security Wrapper when we pass |true|? Is it a SOW or something? That could explain it.

Comment 17

[Andrew McCreight [:mccr8]]

In the weird failing case, in XPCConvert::NativeInterface2JSObject is going into the wrapper cached DOM binding case, and flat is null.  If we then only call that JS_WrapObject when js::GetContextCompartment(cx) != js::GetObjectCompartment(flat), then at least some of the tests start passing.  So the question is, why do we wrap something into the same compartment, and then end up with it being wrapped in a SecurityWrapper<Base>.  I'll look at that tomorrow, as I'm not very familiar with the wrapping infrastructure.

Comment 18

[Andrew McCreight [:mccr8]]

> So how is it ending up with any sort of Security Wrapper when we pass |true|? Is it a SOW or something? That could explain it.

I'm not sure.  How do I figure that out?

Comment 19

[Bobby Holley (:bholley)]

(In reply to Andrew McCreight [:mccr8] from comment #18)
> > So how is it ending up with any sort of Security Wrapper when we pass |true|? Is it a SOW or something? That could explain it.
> 
> I'm not sure.  How do I figure that out?

obj->dump() in a debugger will tell you if the thing is a proxy. Once you've done that, you can do js::GetProxyHandler(obj), which should hopefully tell you which handler singleton you're dealing with.

(In reply to Andrew McCreight [:mccr8] from comment #17)
> If we then only
> call that JS_WrapObject when js::GetContextCompartment(cx) !=
> js::GetObjectCompartment(flat), then at least some of the tests start
> passing.

Ok, then we're almost certainly dealing with a same-compartment SOW then.

So this means that a piece of anonymous content is itself getting bound (in untrusted content). This is kinda wacky, and it's worth considering what we actually want to do in that case. We probably just shouldn't set up the class object at all, since the content scope shouldn't be scripting the NAC anyway (and anything over Xrays from the XBL scope should still resolve the appropriate stuff).

How about this: We pass |true|, and then detect if the result passes js::IsWrapper. If it is, we just early-return and don't try to set up the prototype object.

Does that work?

Comment 20

[Andrew McCreight [:mccr8]]

I filed bug 960320 for the issue of the exception not being handled properly sometimes.

Comment 21

[Andrew McCreight [:mccr8]]

> How about this: We pass |true|, and then detect if the result passes js::IsWrapper. If it is, we just early-return and don't try to set up the prototype object.

Nice!  That seems to do the trick.  With that, the content/events directory passes, except for a few tests that fail even without any patches.

Comment 22

[Andrew McCreight [:mccr8]]

https://tbpl.mozilla.org/?tree=Try&rev=bb179e455d65

Comment 23

[Andrew McCreight [:mccr8]]

There are still a number of failures.

layout/forms/test/test_textarea_resize.html
toolkit/content/tests/widgets/test_audiocontrols_dimensions.html
toolkit/content/tests/widgets/test_bug898940.html
toolkit/content/tests/widgets/test_videocontrols_audio.html
chrome/layout/base/tests/chrome/test_printpreview.xul
reftest/tests/layout/reftests/bugs/449149-1a.html

I'm not sure what is going on.  I looked at one of the things, and it seems like another instance of a SOW, as you end up with "JavaScript error: , line 0: permission denied to unwrap object" if you don't bail out if WrapNative returns a wrapper.  I tried returning NS_ERROR_XBL_BLOCKED on failure, as the CSS code that is initializing the XBL behaves differently with that error, but it didn't make a difference.  I'll try to dig some more.

Comment 24

[Bobby Holley (:bholley)]

It looks like your patch returns NS_ERROR_UNEXPECTED. I meant (though didn't specify) in comment 19 to just return NS_OK.

Comment 25

[Andrew McCreight [:mccr8]]

Attachment: Something a little like this. WIP.

Hmm. Ok. Once I did that, I had to add a null-check on targetClassObject, returning
early in that case. Once that's done, then I get this:

 0:05.54 [87149] WARNING: value is wrapper, giving up: file /Users/amccreight/mc/dom/xbl/nsXBLProtoImpl.cpp, line 192
    [...the "value is wrapper" warning appears dozens of times...]
 0:05.54 [87149] WARNING: Not same origin error!: file /Users/amccreight/mc/dom/base/nsJSEnvironment.cpp, line 483
 0:05.54 TEST-UNEXPECTED-FAIL | unknown test url | uncaught exception - Script error. at chrome://global/content/bindings/videocontrols.xml:0
 0:05.54 JavaScript error: chrome://global/content/bindings/videocontrols.xml, line 969: this.scrubber.valueChanged is not a function

Which kind of looks like it is depending on something actually working.

Comment 26

[Bobby Holley (:bholley)]

Heh, so it looks like cody is fighting this exact same issue in bug 825392, where he's trying to hoist NAC reflectors into the XBL scope.

We already have XBL prototypes for both the content and the XBL scope, and the functions on the content scope are generally just cross-compartment wrappers to the functions in the XBL scope. So I think we're going to need a way to support prototypes that only exist in the XBL scope. I'm going to dig into this for a bit.

Comment 27

[Cody Crews]

I just wanted to let you guys know that I'll be looking this over and getting back to work on bug 825392 here shortly.  Last night my net died on me, I have a new ISP and quite a few issues, waiting on a tech but I'm not very hopeful about it.   Had this same ISP over 5 years ago and tons of issues then too.

Comment 28

[Bobby Holley (:bholley)]

Ok, looking at this more, I think this is something that's going to need to be fixed in cody's patch in bug 825392. I'll comment over there.

Andrew, I'd just keep the current behavior here for now and unblock yourself. We should check in on this issue in a month or so to make sure it's sorted out though - can you add that to your sec pokelist?

Comment 29

[Andrew McCreight [:mccr8]]

Sure.  I may file a new sec-high bug or something.

Comment 30

[Andrew McCreight [:mccr8]]

Attachment: Change nsContentUtils::WrapNative's default to true.

Comment 31

[Andrew McCreight [:mccr8]]

This passes in true everywhere except for
- nsIDocument::AdoptNode.  Eventually, this could become an UncheckedUnwrap or something.
- nsXBLProtoImpl::InitTargetObjects.  Hopefully this will be fixed by cody's patch.

A L64 debug try run for this is green:
  https://tbpl.mozilla.org/?tree=Try&rev=143a886e539f

I'm not sure how exactly to go about landing it.  There's some badness this patch is probably fixing, but we've had the public bug sitting around for over a year, and there's nothing the patch reveals in particular beyond that, so I'm tempted to just upload the patch to the public bug, then put it up for review and land on trunk as usual.  Then, later in the cycle, I'll nominate for branches, which will expose more that there are probably actual problems.

Comment 32

[Cody Crews]

(In reply to Andrew McCreight [:mccr8] from comment #31)
> This passes in true everywhere except for
> - nsIDocument::AdoptNode.  Eventually, this could become an UncheckedUnwrap
> or something.
> - nsXBLProtoImpl::InitTargetObjects.  Hopefully this will be fixed by cody's
> patch.

I have definitely taken care of the compartment mismatch, but its a strange situation.  Ill be cleaning up some code and trying to post what I have on bug 825392 soon.  Somehow with the fix I have right after a comparison like 

xpc::IsXBLScope(js::GetObjectCompartment(globalObject)) ? js::GetGlobalForObjectCrossCompartment(globalObject) : xpc::GetXBLScope(cx, js::GetGlobalForObjectCrossCompartment(globalObject))); 

IsXBLScope returns false, but an assertion is hit following the call to GetXBLScope saying that that the scope already is the XBL scope essentially.  Ill have a cleaned up version up later today probably, Ive been trying to figure it out and have massive code in place for debugging that has be stripped.  

My point is we should be good on my end soon.

Comment 33

[Andrew McCreight [:mccr8]]

So, there's basically already a bug on file for this, bug 733636.  We probably want to backport it many places, as there's a bunch of gross code in here that this fixes.  I was thinking I'd put the patch up for review in the public bug, get it reviewed and landed there, as the patch itself doesn't really reveal anything in particular.  Then, later in the cycle, I'd put it up for approval for branches, to reduce the window where it is clear that we think something bad may be going on.  Does that sound reasonable, Al?

Comment 34

[Al Billings [:abillings - ex-MoCo]]

That works for me.