Closed Bug 1221421 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: JavaScript Engine: JIT, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox42 --- wontfix
firefox43 + fixed
firefox44 + fixed
firefox45 + fixed
firefox-esr38 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-v2.5 --- fixed
b2g-v2.2r --- unaffected
b2g-master --- fixed

People

(Reporter: jruderman, Assigned: jandem)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, sec-high, testcase, Whiteboard: [adv-main43+][post-critsmash-triage])

Attachments

(4 files)

Attached file browser testcase
Assertion failure: this->is<T>(), at js/src/jsobj.h:547
Attached file stack
Group: core-security → javascript-core-security
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...
Component: JavaScript Engine → JavaScript Engine: JIT
Flags: needinfo?(jdemooij)
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.
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.
Attached patch Part 1 - FixSplinter Review
This adds a holderIsReceiver argument to EmitGetterCall.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8686547 - Flags: review?(bzbarsky)
Attached patch Part 2 - TestSplinter Review
Adds a Mochitest. Crashes without the patch.
Attachment #8686548 - Flags: review?(bzbarsky)
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.
Keywords: sec-high
Comment on attachment 8686547 [details] [diff] [review]
Part 1 - Fix

r=me.  This is probably my fault, isn't it?  :(
Attachment #8686547 - Flags: review?(bzbarsky) → review+
Comment on attachment 8686548 [details] [diff] [review]
Part 2 - Test

r=me
Attachment #8686548 - Flags: review?(bzbarsky) → review+
(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 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.
Attachment #8686547 - Flags: sec-approval?
sec-approval+ for trunk. Please nominate a patch for Aurora and Beta.
Attachment #8686547 - Flags: sec-approval? → sec-approval+
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.
Flags: needinfo?(jdemooij)
Attachment #8686547 - Flags: approval-mozilla-beta?
Attachment #8686547 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/d434abe6e251
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment on attachment 8686547 [details] [diff] [review]
Part 1 - Fix

Crash fix for regression from 40, please uplift to aurora and beta.
Attachment #8686547 - Flags: approval-mozilla-beta?
Attachment #8686547 - Flags: approval-mozilla-beta+
Attachment #8686547 - Flags: approval-mozilla-aurora?
Attachment #8686547 - Flags: approval-mozilla-aurora+
Group: javascript-core-security → core-security-release
Whiteboard: [adv-main43+]
Whiteboard: [adv-main43+] → [adv-main43+][post-critsmash-triage]
Flags: in-testsuite+
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.