906035 - Assertion failure: output.type() == MIRType_Int32, at jit/IonCaches.cpp:1024 or Crash [@ js::ion::GetPropertyIC::tryAttachArgumentsLength]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision 1ed5a88cd4d0 (threadsafe build, run with --fuzzing-safe --ion-eager --ion-parallel-compile=on --thread-count=2):


function getLength(arr) {
  return arr.length;
}
var arr2 = new Array(4294967295);
for (var i=0; i<10000000; ++i) {
  (function() function() {}) (getLength(arr2));
}

Comment 1

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 2

[Christian Holler (:decoder)]

This could be the same as bug 906024 but the opt-crash is different and it only occurs in threadsafe builds with parallel compilation:


Program received signal SIGSEGV, Segmentation fault.
js::ion::GetPropertyIC::tryAttachArgumentsLength (this=0x908e6c8, cx=0x906b380, ion=0x908e638, obj=(JSObject * const) 0xffffffff Cannot access memory at address 0xffffffff, name="length", emitted=0xffffc7ff) at js/src/jit/IonCaches.cpp:1546
1546        if (!IsOptimizableArgumentsObjectForLength(obj))
#0  js::ion::GetPropertyIC::tryAttachArgumentsLength (this=0x908e6c8, cx=0x906b380, ion=0x908e638, obj=(JSObject * const) 0xffffffff Cannot access memory at address 0xffffffff, name="length", emitted=0xffffc7ff) at js/src/jit/IonCaches.cpp:1546
#1  0x08361e9e in js::ion::GetPropertyIC::tryAttachStub (this=0x908e6c8, cx=0x906b380, ion=0x908e638, obj=(JSObject * const) 0xffffffff Cannot access memory at address 0xffffffff, name="length", returnAddr=0xf72ac472, emitted=0xffffc7ff) at js/src/jit/IonCaches.cpp:1618
#2  0x08362089 in js::ion::GetPropertyIC::update (cx=0x906b380, cacheIndex=0, obj=(JSObject * const) 0xffffffff Cannot access memory at address 0xffffffff, vp=$jsval(-nan(0xfff8200000000))) at js/src/jit/IonCaches.cpp:1657
#3  0xf64f1a05 in ?? ()
#4  0x0908c410 in ?? ()
#5  0xf64ef837 in ?? ()
eax     0xffffc868      -14232
edx     0xffffffff      -1
=> 0x83619b9 <js::ion::GetPropertyIC::tryAttachArgumentsLength(JSContext*, js::ion::IonScript*, JS::Handle<JSObject*>, JS::Handle<js::PropertyName*>, bool*)+185>:      mov    0x4(%edx),%eax


Marking s-s because bug 906024 is s-s.

Comment 3

[Jan de Mooij [:jandem]]

Eric this looks like a problem with Ion's GetProperty IC. Can you take a look? Thanks!

Comment 4

[Eric Faust [:efaust]]

Attachment: fixArrayLen.patch

This was really silly. Somehow, we had apparently never exercised the specialization path that carefully, since this assert is just dead simple.

The stub assumes that the output fits in an int. The length of (2 ** 32 - 1) doesn't fit. Kaboom.

Add a check before generation to ensure that we can actually handle the inputs we're given.

Comment 5

[Nicolas B. Pierron [:nbp]]

Comment on attachment 791476 [details] [diff] [review]
fixArrayLen.patch

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

::: js/src/jit/IonCaches.cpp
@@ +1086,5 @@
> +        return false;
> +
> +    if (output.type() != MIRType_Value && output.type() != MIRType_Int32) {
> +        // The next execution should cause an invalidation because the type
> +        // does not fit.

The type fit in this case, if our IC expect a MIRType Double.  But we do not handle it in the array length IC stub of GenerateArrayLength.  I think this is fine, we already do so with tryAttachArgumentsLength.  Based on the comment 4, I would have expected a check, but the check is already present in GenerateArrayLength.

The thing that I do not understand, is how this is related to the backtrace in comment 2.  How can a bad type cause a corrupted Object pointer to flow into the update function?

Comment 6

[Eric Faust [:efaust]]

So, this is /really/ bad. As long as TI says that the output register is some type that's not an int, we will use the float register code as a gpr code, which means taking some random register and smashing it with the length of the array.

In the backtrace above, you can see that the obj pointer has been badly corrupted. That's because the length of the array that we loaded (which was hardcoded!) 0xffffffff, got loaded into the randomly selected register, which aliased the object reg.

This means that an attacker has a write of an arbitrary pointer to an arbitrary register.

Luckily, the first bad revision is:
changeset:   142449:42776e928f7b
user:        Eric Faust <[e-mail]>
date:        Sat Aug 10 22:20:36 2013 -0700
summary:     Bug 902264 - Part 2: Expose Array.length optimization to idempotent GetPropertyICs. (r=jandem)

so the problem is inbound-only at the moment.

Comment 7

[Eric Faust [:efaust]]

Comment on attachment 791476 [details] [diff] [review]
fixArrayLen.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

It's fairly hard to understand the intricacies of all of TI, our MacroAssembler, and our Register encodings simultaneously. The contents of the patch looks like a straightforward assertion fix.

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

I think if we push the test in the bug as well, it's much easier. The test looks a lot like a potential attack vector as soon as you understand that changing the length of the array changes the pointer in the crash.

Which older supported branches are affected by this flaw?

None. Trunk only.

If not all supported branches, which bug introduced the flaw?

See comment above.

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

N/A

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

Little to no chance. If any, they will be perf.

Comment 8

[Christian Holler (:decoder)]

I assume this is the same problem? (64 bit, --ion-eager).


function Test262Error(message) {
    if (message) this.message = message;
};
function testFailed(message) {
    throw new Test262Error(message);
}
try {     testFailed("x"); } catch(e) {}
try { testFailed('x' + (3 >> 21)); } catch(e) {}
evaluate("testFailed((new Array(0x1337)).length);", { noScriptRval : true });


This crashes on the heap, trying to write to 0x1337.

Comment 9

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

Comment on attachment 791476 [details] [diff] [review]
fixArrayLen.patch

sec-approval+ for trunk.

Technically, if a bug is trunk only, you don't need sec-approval and can check in. We use sec-approval mainly as a gating function for exposure on branches.

Comment 11

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/88f744f7dc2c

Comment 12

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/88f744f7dc2c

Comment 13

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.