1589545 - AutoRealm passed a cross-compartment wrapper in handling of readable streams' pull/cancel methods, leading to an assertion

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect, P1)

Description

[Jeff Walden [:Waldo]]

Whilst implementing writable stream handling of write/close/abort algorithms and copying from readable streams implementations, I saw some code that...did not look right. Turns out, it isn't right.

var og = newGlobal({newCompartment:true});
var ccwPullMethod = new og.Function("return 42;");
var ccwCancelMethod = new og.Function("return 17;");

switch (scriptArgs[0]) {
  case "pull":
    new ReadableStream({ pull: ccwPullMethod });
    break;

  case "cancel":
    new ReadableStream({ cancel: ccwCancelMethod }).cancel("bye");
    break;

  default:
    throw new Error("bad action, must be pull or cancel: " + scriptArgs[0]);
}

drainJobQueue();

Run that as dbg/js/src/js -f /tmp/stream-test.js -- pull or dbg/js/src/js -f /tmp/stream-test.js -- cancel and either way you'll get a compartment-mismatch assertion, in both cases from code that looks basically like this:

    // The algorithm created in
    // SetUpReadableByteStreamControllerFromUnderlyingSource step 5.
    Rooted<Value> unwrappedCancelMethod(cx,
                                        unwrappedController->cancelMethod());
    if (unwrappedCancelMethod.isUndefined()) {
      // CreateAlgorithmFromUnderlyingMethod step 7.
      result = PromiseObject::unforgeableResolve(cx, UndefinedHandleValue);
    } else {
      // CreateAlgorithmFromUnderlyingMethod steps 6.c.i-ii.
      {
        AutoRealm ar(cx, &unwrappedCancelMethod.toObject());

and that final line can be handed a cross-compartment wrapper and therefore can assert.

I suppose this is a good sign, for my understanding of the code I am writing; flip side is it's a bad sign, for the code that I was roughly cribbing from. :-|

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Bug 1589545 - Add a test. r=arai, r=jorendorff

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Bug 1589545 - Don't pass a CCW to AutoRealm in streams code, when processing pull/cancel functionality. r=arai, r=jorendorff

Depends on D49693

Comment 3

[Jeff Walden [:Waldo]]

FWIW the reason for two blocking reviews here is that this dates to the fix for bug 1507943, and one of you wrote the patch, one of you reviewed it. :-) Seems like you both should be aware of the failure here, in addition to perhaps being the people with the most understanding of this code.

Comment 4

[Daniel Veditz [:dveditz]]

Is the compartment mismatch assertion fatal in runtime? Assuming not, and that this therefore might be a sec-high bug.

Comment 5

[Jeff Walden [:Waldo]]

The precise problem is the AutoRealm constructor takes an object and enters its compartment. The object passed in must not be a cross-compartment wrapper -- not for any important reason, but because if you have a CCW around an object and you're not careful where you got the CCW from, you could get it from some arbitrary realm that isn't the realm you might have meant. At least in theory. (I'm not 100% certain this theory truly checks out, but it's not a crazy thought, and there's little hard and potentially benefit to adopting extra rigor when it's not all that difficult, as here.)

In practice, and here, if you have a CCW around an object, you probably got it from a code path that consistently returns objects from a single compartment. Here, the compartment of the CCW being used in both cases is that of the object that has the slot the CCW was stored in. And that's the same compartment as unwrappedController in these paths -- the object the patch passes in.

So what happens if you pass in a CCW from a specific realm, as opposed to an object from that realm?

  JS::Realm* nonCCWRealm() const {
    MOZ_ASSERT(!js::UninlinedIsCrossCompartmentWrapper(this));
    return group_->realm();
  }

  // Returns the object's realm even if the object is a CCW (be careful, in
  // this case the realm is not very meaningful because wrappers are shared by
  // all realms in the compartment).
  JS::Realm* maybeCCWRealm() const { return group_->realm(); }

Basically, the realm of an object and a CCW from the same realm is the same -- just if you go through that first function, there's an assertion of not-a-CCW.

So end of day, I think this is going to manifest in reality as just an assertion failure. If you don't perform that assertion, as shipping builds will not, you'll just return the realm of the CCW, which is the realm we want to enter. (I didn't bother diving into this earlier because compartments+assertions is ample reason to be cautious, and assertions are IMO best considered to be things you don't think about the consequences of them failing -- they're knowledge you can use to not think about possibilities.)

And as a consequence, unhiding. (And retagging as sec-other, which seems vaguely the closest keyword to use for "bugs with security implications that turn out not to be a security error on careful inspection").

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bcec42277f52
Don't pass a CCW to AutoRealm in streams code, when processing pull/cancel functionality.  r=arai,jorendorff
https://hg.mozilla.org/integration/autoland/rev/d32c29eed6a5
Add a test.  r=arai,jorendorff

Comment 7

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/bcec42277f52
https://hg.mozilla.org/mozilla-central/rev/d32c29eed6a5