1221421 - "Assertion failure: this->is<T>()" in array_length_getter after changing __proto__ on DOM style object

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jesse Ruderman]

Attachment: browser testcase

Assertion failure: this->is<T>(), at js/src/jsobj.h:547

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Boris Zbarsky [:bzbarsky]]

So we're landing in the self-hosted ArrayMap.  This gets .length on the object in question, which goes through jitcode and ends up calling array_length_getter, but with the CSS2Properties object as obj.

The normal .length codepath would go through CallGetter which for JSPropertyOps (which this is) uses the holder, not the receiver, as the object to pass to the getter.

I don't see anything in BaselineIC that does anything with JSPropertyOp.  But Ion does have it, and seems to be passing "obj", not "receiver" to the getter at first glance...

Comment 3

[Boris Zbarsky [:bzbarsky]]

And in particular, it's not clear to me why the DOM thing matters, but it does: if I use a random object with [] as proto, array_length_getter is passed the array, even from jitcode.

Comment 4

[Jan de Mooij [:jandem]]

In GetPropertyIC::tryAttachDOMProxyUnshadowed we do:

    RootedObject checkObj(cx, obj->getTaggedProto().toObjectOrNull());

Then we pass checkObj instead of obj to EmitGetterCall, with this comment:

    // EmitGetterCall() expects |obj| to be the object the property is
    // on to do some checks. Since we actually looked at checkObj, and
    // no extra guards will be generated, we can just pass that instead.

But EmitGetterCall has the following logic in the JSPropertyOp case:

    if (obj == holder) {
        // When the holder is also the current receiver, we just have a shape guard,
        // so we might end up with a random object which is also guaranteed to have
        // this JSGetterOp.
        masm.Push(object);
    } else {
        // If the holder is on the prototype chain, the prototype-guarding
        // only allows objects with the same holder.
        masm.movePtr(ImmGCPtr(holder), scratchReg);
        masm.Push(scratchReg);
    }

We take the if-branch here but that's wrong, we should take the else branch.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Fix

This adds a holderIsReceiver argument to EmitGetterCall.

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Test

Adds a Mochitest. Crashes without the patch.

Comment 7

[Jan de Mooij [:jandem]]

Calling array_length_getter on a random DOM proxy can crash and maybe allow an attacker to read some memory locations (not sure, it depends on the proxy layout, it could well be impossible).

The (few) other JSPropertyOp getters may do more harm though, so I think it's best to treat this as a sec-high just to be safe.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8686547 [details] [diff] [review]
Part 1 - Fix

r=me.  This is probably my fault, isn't it?  :(

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8686548 [details] [diff] [review]
Part 2 - Test

r=me

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bz] from comment #8)
> r=me.  This is probably my fault, isn't it?  :(

No it's a regression from bug 895223 (Firefox 40). Before that we always passed the receiver to JSPropertyOp getters.

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8686547 [details] [diff] [review]
Part 1 - Fix

[Security approval request comment]
* How easily could an exploit be constructed based on the patch?
Not very easily. I'm not even 100% sure this is exploitable, see comment 7, but maybe with some work.

* Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

* Which older supported branches are affected by this flaw?
Firefox 40+.

* Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be easy to backport.

* How likely is this patch to cause regressions; how much testing does it need?
Unlikely.

Comment 12

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

sec-approval+ for trunk. Please nominate a patch for Aurora and Beta.

Comment 13

[Jan de Mooij [:jandem]]

Part 1:

https://hg.mozilla.org/integration/mozilla-inbound/rev/d434abe6e251

Comment 14

[Jan de Mooij [:jandem]]

Comment on attachment 8686547 [details] [diff] [review]
Part 1 - Fix

Approval Request Comment
[Feature/regressing bug #]: Bug 895223.
[User impact if declined]: Crashes, maybe security bugs.
[Describe test coverage new/current, TreeHerder]: Fixes the test.
[Risks and why]: Low risk.
[String/UUID change made/needed]: None.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d434abe6e251

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8686547 [details] [diff] [review]
Part 1 - Fix

Crash fix for regression from 40, please uplift to aurora and beta.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5d43918469c6

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/7a0601b27b5b

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/5d43918469c6

Comment 20

[Jan de Mooij [:jandem]]

Pushed the test:

https://hg.mozilla.org/integration/mozilla-inbound/rev/0a48f8ebe02f

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0a48f8ebe02f