1354843 - Don't accept dead wrappers in JS_FindCompilationScope

Status: NEW

(Core :: XPConnect, enhancement, P3)

Description

[Kris Maglione [:kmag]]

The subscript loader and precompiled script loader both use JS_FindCompilationScope to find the target global for script execution. Neither of these currently check for dead wrappers, which means that if we pass a dead wrapper that previously pointed to a different compartment, we're suddenly executing scripts in our own, chrome-privileged compartments instead.

The same probably goes for other export helpers that take an object scope.

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1354843: Don't accept dead wrappers in JS_FindCompilationScope.

Review commit: https://reviewboard.mozilla.org/r/128100/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128100/

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1354843: Don't accept dead wrappers in export helpers and other XPC utils.

Review commit: https://reviewboard.mozilla.org/r/128102/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128102/

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 8856171 [details]
Bug 1354843: Don't accept dead wrappers in JS_FindCompilationScope.

https://reviewboard.mozilla.org/r/128100/#review130618

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8856172 [details]
Bug 1354843: Don't accept dead wrappers in export helpers and other XPC utils.

https://reviewboard.mozilla.org/r/128102/#review130620

I'm pretty warry of introducing a general function with these semantics. There are tons of places all over the codebase that use the result of CheckedUnwrap to determine the object for new compartments, and with this patch it's not at all obvious which ones of those should be using JS_FindObjectScope.

There are two paths that seem workable to me:
(1) Generally allow DeadObjectProxies, like we have been, and specifically check for them at problematic callsites.
(2) Make CheckedUnwrap return nullptr for DeadObjectProxies.

Option (1) is low risk. Option (2) is more consistent, but a lot higher risk.

Comment 5

[Kris Maglione [:kmag]]

I'd definitely like for CheckedUnwrap to return a nullptr for dead wrappers, but after bug 1338563, I'm worried that it would cause crashes. And returning null from UncheckedUnwrap would *definitely* cause crashes, so there are probably places where we use that that need to be changed, anyway.

But either way, I think that a separate helper for these use cases makes sense. I'm particularly worried about the cases where we pass scope objects to export helpers from JS. Most of them probably don't matter much, but I'd be willing to bet there are ways to exploit getObjectPrincipal returning the system principal for a dead wrapper.

If we changed CheckedUnwrap to check for dead wrappers, we could just rely on it for all of those cases, but having a separate helper at least gives us consistent error messages, rather than falling back to an ordinary "access denied" error, or duplicating the same logic in a half dozen places. And I wouldn't be surprised if we want to add more sanity checks in the future.

I'm less concerned about most of the other places where we do this. There are probably other places where it matters, and they should probably be changed to use the same helper. But I don't think there are many places where we take scope objects from JS like this, and that's what really concerns me. In native code, we usually don't have CCWs in places where we need to run things in a specific scope, and/or we store a separate incumbent global, anyway. The weird cases like CallbackObjects and Promise handlers already take special care not to screw this up.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Kris Maglione [:kmag] from comment #5)
> I'd definitely like for CheckedUnwrap to return a nullptr for dead wrappers,
> but after bug 1338563, I'm worried that it would cause crashes.

Possibly, but any caller of CheckedUnwrap is supposed to null-check, so I wouldn't be surprised if it didn't cause any crashes (though it still might break in other ways).

> And
> returning null from UncheckedUnwrap would *definitely* cause crashes, so
> there are probably places where we use that that need to be changed, anyway.

I would suggest leaving UncheckedUnwrap alone.

> 
> But either way, I think that a separate helper for these use cases makes
> sense.

Yes, but point is that it's not at all clear to me what "these use cases" are. For example, does it include GlobalObject:::GlobalObject and all its callers?

http://searchfox.org/mozilla-central/rev/624d25b2980cf5f83452b040e6d664460f9b7ec5/dom/bindings/BindingUtils.cpp#2265

I know it's tempting to add another function that improves the situation at some callsites, but the cost of that is the proliferation of helpers and a lack of clarity on what the right thing to use is.

Most of the callers of CheckedUnwrap already check the JSClass of the object after unwrapping it, or perform some operation which would cause the DeadObjectProxy to throw, returning an error. Those that don't would be probably be better-served by having CheckedUnwrap return null for DeadObjectProxies (since they already need to be fallible).

So I think we should do that instead.

Comment 7

[Kris Maglione [:kmag]]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #6)
> (In reply to Kris Maglione [:kmag] from comment #5)
> > I'd definitely like for CheckedUnwrap to return a nullptr for dead wrappers,
> > but after bug 1338563, I'm worried that it would cause crashes.
>
> Possibly, but any caller of CheckedUnwrap is supposed to null-check, so I
> wouldn't be surprised if it didn't cause any crashes (though it still might
> break in other ways).

Well, bug 1338563 was caused by a missing null check on a CheckedUnwrap return
value. In that case, we were returning null because we were creating an opaque
wrapper in a place where we now create a DeadObjectProxy. So without some
other changes, I suspect we'd get crashes there again.

> > And returning null from UncheckedUnwrap would *definitely* cause crashes,
> > so there are probably places where we use that that need to be changed,
> > anyway.
>
> I would suggest leaving UncheckedUnwrap alone.

Yeah, I agree UncheckedUnwrap should definitely not return null. But I suspect
there are places where we use UncheckedUnwrap because we know we have a chrome
caller where we should really be doing this kind of check.

> > But either way, I think that a separate helper for these use cases makes
> > sense.
> 
> Yes, but point is that it's not at all clear to me what "these use cases"
> are. For example, does it include GlobalObject:::GlobalObject and all its
> callers?
> 
> http://searchfox.org/mozilla-central/rev/
> 624d25b2980cf5f83452b040e6d664460f9b7ec5/dom/bindings/BindingUtils.cpp#2265

Arguably, yes, we should fail if we get a dead wrapper there. But GlobalObject
is a helper with a similar purpose to this one, so adding a direct check there
might make just as much sense.

It might even make sense to just do that and then use GlobalObject in the
export helpers.

> I know it's tempting to add another function that improves the situation at
> some callsites, but the cost of that is the proliferation of helpers and a
> lack of clarity on what the right thing to use is.
>
> Most of the callers of CheckedUnwrap already check the JSClass of the object
> after unwrapping it, or perform some operation which would cause the
> DeadObjectProxy to throw, returning an error. Those that don't would be
> probably be better-served by having CheckedUnwrap return null for
> DeadObjectProxies (since they already need to be fallible).
>
> So I think we should do that instead.

I think having CheckedUnwrap return null for dead wrappers does make sense,
but I still think it makes sense to have a helper for checking and unwrapping
scope objects in export utils.

This patch changes that behavior in five different places. Ideally, in all of
those places, we'd want to report a different error for trying to use a dead
wrapper than for a failed permission check. These kinds of errors are likely
to be timing sensitive, and a generic permission check error would be almost
impossible to make sense of, and hard to reproduce. And even in the normal
failure case, it would be nice to have consistent errors when the permission
check fails.

If we don't have some sort of helper, we're going to have to duplicate a lot
of logic to determine why CheckedUnwrap failed, and how to report it.

Comment 8

[Kris Maglione [:kmag]]

I suppose another possibility is to add a variant of CheckedUnwrap that takes a JSContext and sets a pending exception on failure, based on why it failed.

Comment 9

[Bobby Holley (:bholley)]

We used to have a stateful unwrap that set exceptions on the cx, and it didn't work well. There are just too many different needs at the callsites.

I agree that making the errors more visible for intermittent CI failures and whatnot would be good. But what about just logging to the console? It should be a rare enough case that we could even do it in release builds if we wanted.

If you prefer to do a targeted fix, then let's make it CheckedUnwrapAndCheckForDeadObject(cx, obj), and put it in XPConnect rather than SM.

Comment 10

[Kris Maglione [:kmag]]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #9)
> I agree that making the errors more visible for intermittent CI failures and
> whatnot would be good. But what about just logging to the console? It should
> be a rare enough case that we could even do it in release builds if we
> wanted.

I hadn't considered that. It might be a good option. But as far as I know, we can't directly log to the console from SM, so we'd need new helper code either way...

> If you prefer to do a targeted fix, then let's make it
> CheckedUnwrapAndCheckForDeadObject(cx, obj), and put it in XPConnect rather
> than SM.

That sounds like a good idea.

Comment 11

[Bobby Holley (:bholley)]

(In reply to Kris Maglione [:kmag] from comment #10)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #9)
> > I agree that making the errors more visible for intermittent CI failures and
> > whatnot would be good. But what about just logging to the console? It should
> > be a rare enough case that we could even do it in release builds if we
> > wanted.
> 
> I hadn't considered that. It might be a good option. But as far as I know,
> we can't directly log to the console from SM, so we'd need new helper code
> either way...

We already dump stuff to the console in e.g. RegExpShared::dumpBytecode.

Comment 12

[Kris Maglione [:kmag]]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #11)
> We already dump stuff to the console in e.g. RegExpShared::dumpBytecode.

Ah. I thought you meant the error console. Hm... I'm worried that just dumping a message to stderr wouldn't be enough. I think we should go for having CheckedUnwrap return null for dead wrappers, and an XPC helper to raise a better error in the cases I'm dealing with in this patch.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Kris Maglione [:kmag] from comment #12)
> (In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #11)
> > We already dump stuff to the console in e.g. RegExpShared::dumpBytecode.
> 
> Ah. I thought you meant the error console. Hm... I'm worried that just
> dumping a message to stderr wouldn't be enough.

Well, it depends on the use-case. I think hard-to-reproduce nuking issues are most likely to occur during CI or while hacking on the frontend, when stderr is usually visible.

> I think we should go for
> having CheckedUnwrap return null for dead wrappers, and an XPC helper to
> raise a better error in the cases I'm dealing with in this patch.

That seems fine.

Comment 14

[Andrew Overholt [:overholt]]

This feels somewhat more urgent than "someday" so I'm setting P2 here. Feel free to disagree (obviously!).

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information

Comment 16

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.