Crash [@ ??] or Assertion failure: MIR instruction returned value with unexpected type, at js/src/jit/MacroAssembler.cpp:1454

VERIFIED FIXED in Firefox 48

Status

()

defect
--
critical
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: gkw, Assigned: jandem)

Tracking

(Blocks 2 bugs, 4 keywords)

Trunk
mozilla50
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 wontfix, firefox48+ verified, firefox49+ verified, firefox-esr4548+ fixed, firefox50+ verified)

Details

(Whiteboard: [jsbugmon:update][adv-main48+][adv-esr45.3+])

Attachments

(3 attachments)

The following testcase crashes on mozilla-central revision 4292da9df16b (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --ion-eager):

x = y = [];
x.shift(y);
x.push(y);
x.splice(14, ({
    valueOf: function() {
        x.shift(y);
    }
}), y);
x.forEach(function() {});
x.forEach(function() {});

Backtrace:

0   ???                           	0x00000001026a4888 0 + 4335487112

For detailed crash information, see attachment.

Variant:

x = y = [];
x.unshift(y);
x.push(y);
x.splice(14, ({
    valueOf: function() {
        x.shift(y);
    }
}), y);
x.forEach(function() {});
x.forEach(function() {});


Process 34906 stopped
* thread #1: tid = 0x501ff8, 0x00000001027b1ed8, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
    frame #0: 0x00000001027b1ed8
->  0x1027b1ed8: movq   (%rcx), %rax
    0x1027b1edb: movabsq $0x102670820, %r11        ; imm = 0x102670820
    0x1027b1ee5: cmpq   %r11, %rax
    0x1027b1ee8: jne    0x1027b1ef3
(lldb) bt
* thread #1: tid = 0x501ff8, 0x00000001027b1ed8, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x0)
  * frame #0: 0x00000001027b1ed8
(lldb)

Assertion failure message involves MIR -> s-s by default. Bisection seems to show that it goes way back prior to m-c rev dc4b163f7db7 (early Nov 2014)
Setting needinfo? from Jan as a start.
Flags: needinfo?(jdemooij)
Summary: Crash [??] or Assertion failure: MIR instruction returned value with unexpected type, at js/src/jit/MacroAssembler.cpp:1454 → Crash [@ ??] or Assertion failure: MIR instruction returned value with unexpected type, at js/src/jit/MacroAssembler.cpp:1454
Hrm, this is a bug in array_splice.

  x.splice(14, {valueOf() { x.shift(); }}, x);

The deleteCount valueOf function changes the array length from 1 to 0, but splice already got array length 1, so x becomes [<hole>, x]

That's all fine, except we forget to mark the array as non-packed :/
Posted patch Part 1 - FixSplinter Review
We need to pass |len| instead of |array->length()| to ensureDenseElements.

I think the only thing that can happen in opt builds is that we don't mark the array as non-packed, and then the JIT can interpret the hole MagicValue as a different Value.

Not sure if this is actually exploitable - let's assume it is though.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Attachment #8746971 - Flags: review?(jwalden+bmo)
Posted patch Part 2 - TestSplinter Review
Attachment #8746973 - Flags: review?(jwalden+bmo)
Keywords: sec-high
Comment on attachment 8746971 [details] [diff] [review]
Part 1 - Fix

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

> I think the only thing that can happen in opt builds is that we don't mark the array as
> non-packed, and then the JIT can interpret the hole MagicValue as a different Value.

I...was going to agree, then I found a contradiction, then I reread CanOptimizeForDenseStorage and convinced myself I was mistaken, and now I'm just convinced the whole setup is too intricate to be confident without way too much close reading (probably by multiple people, to have any confidence in that analysis -- and ideally by people not aware what the claimed consequence is, already).

> Not sure if this is actually exploitable - let's assume it is though.

Yes.  Please.  Super-duper-please.  There is less than zero justification for avoiding the "risk" of a fix by ass-u-ming we can correctly read through the code and identify the only consequences here.
Attachment #8746971 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8746973 [details] [diff] [review]
Part 2 - Test

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

::: js/src/jit-test/tests/basic/array-splice-valueOf-deleteCount.js
@@ +1,3 @@
> +var x = [];
> +x.push(x);
> +x.splice(14, {valueOf() { x.shift(); }}, x);

Add a blank line after these, and push some value other than |x| into the array (it doesn't seem necessary to push that, right?).

s/14/x.length/ so it's clear we're splicing at the end, without having to massage 14 into 1 by implicit funny business.

Shifting is a fairly intensive operation -- could that be replaced with just |x.length = 0;|?  I would prefer that, for its narrower and easier-to-understand effect on the array's length/capacity/dense initialized length/etc.  Have the valueOf method return an explicit 0, rather than going through obscurer conversion.  Splice in a different value than |x|, too.

@@ +4,5 @@
> +function f(arr) {
> +    assertEq(arr.length, 2);
> +    for (var i = 0; i < 2; i++) {
> +	var v = arr[i];
> +	assertEq(v, (i == 0) ? undefined : x);

s/x/whatever different value you splice in, above/

|i === 0| rather than double-equals.
Attachment #8746973 - Flags: review?(jwalden+bmo) → review+
Duplicate of this bug: 1266242
Can you request sec approval on this since the bug is rated sec-high and it looks like you have a fix? 
It's pretty late in beta to land this for 47, though. (RC1 will go to build next Tuesday)
Flags: needinfo?(jdemooij)
Comment on attachment 8746971 [details] [diff] [review]
Part 1 - Fix

Sorry for the delay.

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
It's hard. We're not convinced it's exploitable, but since it's hard to prove let's assume it is.

> 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?
Goes back a while. I think all supported branches.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Trivial, patch is a one-liner.

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely, a Nightly or two + fuzz testing should be sufficient.
Flags: needinfo?(jdemooij)
Attachment #8746971 - Flags: sec-approval?
sec-approval for checkin on June 21. We're making 47 release builds within the next day so this is too late for 47. We'll want it on all branches after it goes onto trunk.
Whiteboard: [jsbugmon:update] → [jsbugmon:update][checkin on 6/21]
Attachment #8746971 - Flags: sec-approval? → sec-approval+
Comment on attachment 8746971 [details] [diff] [review]
Part 1 - Fix

Approval Request Comment
[Feature/regressing bug #]: Old bug.
[User impact if declined]: Potential security/crash/correctness bugs.
[Describe test coverage new/current, TreeHerder]: This code path is exercised by various tests. Patch just landed on inbound.
[Risks and why]: One-liner, low risk.
[String/UUID change made/needed]: None.
Attachment #8746971 - Flags: approval-mozilla-esr45?
Attachment #8746971 - Flags: approval-mozilla-beta?
Attachment #8746971 - Flags: approval-mozilla-aurora?
Whiteboard: [jsbugmon:update][checkin on 6/21] → [jsbugmon:update]
Comment on attachment 8746971 [details] [diff] [review]
Part 1 - Fix

Let's take it on all branches!
Should be in 48 beta 3 and 45.3.0!
Attachment #8746971 - Flags: approval-mozilla-esr45?
Attachment #8746971 - Flags: approval-mozilla-esr45+
Attachment #8746971 - Flags: approval-mozilla-beta?
Attachment #8746971 - Flags: approval-mozilla-beta+
Attachment #8746971 - Flags: approval-mozilla-aurora?
Attachment #8746971 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/fdd1161f4456
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx48
JSBugMon: This bug has been automatically verified fixed on Fx49
Group: javascript-core-security → core-security-release
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main48+][adv-esr45.3+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.