1157456 - Compartment mismatch assertion failures with SavedFrame accessors

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Comment 1

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Re-wrap results in SavedFrame accessors

Comment 2

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=046ddf977563

Comment 3

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Note that the testcase in the patch crashes without the changes made to the SavedFrame accessors. With those changes, everything is A-OK.

Comment 4

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Try push is green.

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 8596199 [details] [diff] [review]
Re-wrap results in SavedFrame accessors

Review of attachment 8596199 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/SavedStacks.cpp
@@ +694,5 @@
>      THIS_SAVEDFRAME(cx, argc, vp, "(get source)", args, frame);
>      RootedString source(cx);
> +    if (JS::GetSavedFrameSource(cx, frame, &source) == JS::SavedFrameResult::Ok) {
> +        if (!cx->compartment()->wrap(cx, &source))
> +            return false;

Unless I'm missing something, we need to do this inside JS::GetSavedFrameSource and change the API to allow for rewrapping errors (I think just OOM). You can just change all the related APIs at the same time and it won't be so bad.

The way that API is designed right now is surprising to me. I  might have r-'d that. We have a standard way of reporting errors: set an exception and return false. Errors are hard enough to deal with when people use the standard. Why is that API any different?

Compartment correctness, too, is hard enough without making it harder on ourselves. Rewrapping must happen as close to the source as possible, to prevent this kind of error (and proliferation of this kind of code).

Comment 6

[Boris Zbarsky [:bzbarsky]]

So the problem is that the current setup is kinda messed up.

We have the following things in play:

1)  Each SavedFrame has an associated principal for the stackframe itself.
2)  Each SavedFrame has an associated compartment which may not match the principal of
    the stackframe.  This compartment represents whatever code happened to be on the
    stack when the stack was captured.

Currently, we use #1 for deciding whether a given caller is allowed to see info about a given stackframe.  We use #2 for deciding whether a given caller can touch the stack at all, in the usual way.

The current consumers of the C++ API really mostly want to use the lower of the two privilege sets "where the stack was captured" and "caller".  This is implemented via the AutoMaybeEnterFrameCompartment thing in C++ accessors.  From that point of view, ensuring that GetSavedFrameParent returns something in the same compartment as the SavedFrame whose parent you're getting is important: it preserves the "where the stack was captured" information.

Now we _could_ return a cross-compartment wrapper there and then just make sure that operations on saved frames always unwrap to the underlying object when trying to do the "where the stack was captured" bits...  or something.

Also of note: it doesn't help that the security models are different here for access from JS and access from JS via C++...

One thing that we could consider doing is making the C++ JSAPI use some opaque type (a la JSFunction), not a JSObject.  Then there would be no danger of compartment mismatches by the API consumers and the API implementation could make sure it does the right things depending on which compartments things are in.  The hard part here is rooting...

Comment 7

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Jason Orendorff [:jorendorff] from comment #5)
> Comment on attachment 8596199 [details] [diff] [review]
> Re-wrap results in SavedFrame accessors
> 
> Review of attachment 8596199 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/vm/SavedStacks.cpp
> @@ +694,5 @@
> >      THIS_SAVEDFRAME(cx, argc, vp, "(get source)", args, frame);
> >      RootedString source(cx);
> > +    if (JS::GetSavedFrameSource(cx, frame, &source) == JS::SavedFrameResult::Ok) {
> > +        if (!cx->compartment()->wrap(cx, &source))
> > +            return false;
> 
> Unless I'm missing something, we need to do this inside
> JS::GetSavedFrameSource and change the API to allow for rewrapping errors (I
> think just OOM). You can just change all the related APIs at the same time
> and it won't be so bad.

IIRC, this will break DOM use of the SavedFrame JSAPI.

> The way that API is designed right now is surprising to me. I  might have
> r-'d that. We have a standard way of reporting errors: set an exception and
> return false. Errors are hard enough to deal with when people use the
> standard. Why is that API any different?

The thing is, access denied isn't an error and you still get the default value.

> Compartment correctness, too, is hard enough without making it harder on
> ourselves. Rewrapping must happen as close to the source as possible, to
> prevent this kind of error (and proliferation of this kind of code).

Agreed, but as mentioned above, IIRC this would break DOM usage of SavedFrames.

Comment 8

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

(In reply to Not doing reviews right now from comment #6)
> One thing that we could consider doing is making the C++ JSAPI use some
> opaque type (a la JSFunction), not a JSObject.  Then there would be no
> danger of compartment mismatches by the API consumers and the API
> implementation could make sure it does the right things depending on which
> compartments things are in.  The hard part here is rooting...

What if we split SavedFrame into SavedFrame (rootable C++/JSAPI thing) and SavedFrameObject (JSObject subclass with a private pointer to a SavedFrame)? Additionally, what if wrapping a SavedFrameObject just created a new SavedFrameObject in the target compartment and copied the SavedFrame private pointer?

Comment 9

[Boris Zbarsky [:bzbarsky]]

That seems reasonable, conceptually.

Comment 10

[Tom Tromey :tromey]

I think this is also blocking bug 1160307.

Comment 11

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Jason, I don't have the cycles for a proper re-architecting of the SavedFrame internals right now. However, since this issue has come up twice now, would you be alright with going ahead with the original patch so we can stop getting the assertion failures, and file a follow up for the Proper Fix?

Comment 12

[Jason Orendorff [:jorendorff]]

Comment on attachment 8596199 [details] [diff] [review]
Re-wrap results in SavedFrame accessors

Review of attachment 8596199 [details] [diff] [review]:
-----------------------------------------------------------------

r=me as there's no real alternative at present.

Comment 13

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Attachment: Re-wrap results in SavedFrame accessors

Comment 14

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Rebased and here is a new try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=89a3c95fff43

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a5c55b45c9e

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6a5c55b45c9e