1536768 - (CVE-2019-9816) IonMonkey: unexpected ObjectGroup in ObjectGroupDispatch operation might lead to potentially unsafe code being executed

Status: RESOLVED FIXED

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

Description

[Samuel Groß]

While fuzzing Spidermonkey, I encountered the following (commented and modified) JavaScript program which crashes debug builds of the latest release version of Spidermonkey (from commit 3ecf89da497cf1abe2a89d1b3c282b48e5dfac8c):

function O1() {
    this.s = 'foobar';
    this.a = 42;

    // Avoid unboxed layout for O1 instances as this will cause the JIT
    // compiler to behave differently and not emit the ObjectGroupDispatch
    // operation (see below).
    delete this.a;
}

// This function will be inlined below in v4, together with the default
// Object.prototype.toString implementation.
// This just demonstrates that a custom function can be inlined which
// will make assumptions about the input ObjectGroup.
O1.prototype.toString = function() {
    return this.s;
};

function v4(v5) {
    // Once v22 is allocated as unboxed object, this will convert it to a
    // native object, which will cause its ObjectGroup to change.
    delete v5.nonExistent;

    // The call to .toString here will be implemented as a switch
    // (ObjectGroupDispatch operation) on the ObjectGroup with two cases
    // (ObjectGroup of v22 and v17). Depending on the input, the dispatch will
    // jump to one of the two inlined implementations of toString. However,
    // after v22 is allocated as UnboxedObject (still with the same
    // ObjectGroup as before), the delete operation above will convert it back
    // to a NativeObject, now changing the ObjectGroup. Afterwards, this
    // ObjectGroupDispatch operation will see an unexpected ObjectGroup and,
    // in debug builds, crash with an assertion failure. In release builds
    // it will just fallthrough to whichever branch was emitted right after
    // the dispatch operation.
    return v5.toString();
}

function v11() {
    const v22 = {p: 1337};
    v4(v22);

    let v26 = 0;
    do {
        const v17 = new O1;
        v4(v17);
        v26++;
    } while (v26 < 100);
}
for (let v33 = 0; v33 < 100; v33++) {
    const v37 = v11();
}

The program will crash with an assertion similar to:

> ../build_DBG.OBJ/dist/bin/js crash.js
Assertion failure: Unexpected ObjectGroup, at js/src/jit/MacroAssembler.cpp:2014
[1]    54116 trace trap  ../build_DBG.OBJ/dist/bin/js crash.js

It appears that roughly the following is happening here:

• Initially, the objects v22 and v17 will be allocated as native objects with two different ObjectGroups (an ObjectGroup [1] stores type information such as the prototype object and property/method types for an object): OG1 and OG2.
• Function v4 will be called repeatedly with objects of both ObjectGroups and will eventually be JIT compiled by IonMonkey. At that point, it will be compiled to expect an object with ObjectGroup OG1 or OG2 based on type feedback from the interpreter/baseline JIT and will be deoptimized if it is ever called with an object of a different group.
• The call to .toString in v4 will be optimized by inlining [2] the two possible implementations (O1.prototype.toString and Object.prototype.toString) and then performing a switch on the ObjectGroup of the input to determine which implementation to jump to. The switch is implemented by the ObjectGroupDispatch operation. Since both input ObjectGroups are covered, the instruction does not have a default (fallback) path [3].
• At a later point, the allocation site of v22 is modified to create an object with unboxed layout [4] which will store its properties inline in an unboxed form but still use ObjectGroup OG1.
• Afterwards, in the JIT code for v4, the delete operation converts the UnboxedObject back to a NativeObject [5], this time changing the ObjectGroup to a new group OG3 [6].
• Finally, when executing the machine code for the ObjectGroupDispatch operation, the new ObjectGroup matches none of the expected ones. At this point the program will crash with an assertion in debug builds. In release builds, it would now simply fall through to whichever one of the inlined implementations was directly following the ObjectGroupDispatch operation.

At least this way of triggering the bug is related to UnboxedObjects, which have recently been disabled by default. However, I am not sure if the conversion from unboxed to native objects due to the property deletion is the only reason that an object's ObjectGroup can change unexpectedly (in this situation).

As for exploitation, it might be possible to cause a type confusion by causing a fallthrough to the inlined code for the other ObjectGroup. In that case, the inlined code would expect to receive an object of a specific ObjectGroup and might omit further security checks based on that. For this specific sample it seems that the fallthrough path always happens to be the correct one (i.e. the one for Object.prototype.toString), but I assume it could also be the other way around in a different context. Furthermore, it might be possible to cause other code constructs (apart form the ObjectGroupDispatch) that rely on ObjectGroup information to misbehave in this situation.

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/3ecf89da497cf1abe2a89d1b3c282b48e5dfac8c/js/src/vm/ObjectGroup.h#L87
[2] https://github.com/mozilla/gecko-dev/blob/3ecf89da497cf1abe2a89d1b3c282b48e5dfac8c/js/src/jit/IonBuilder.cpp#L4517
[3] https://github.com/mozilla/gecko-dev/blob/3ecf89da497cf1abe2a89d1b3c282b48e5dfac8c/js/src/jit/IonBuilder.cpp#L4923
[4] https://github.com/mozilla/gecko-dev/blob/3ecf89da497cf1abe2a89d1b3c282b48e5dfac8c/js/src/vm/UnboxedObject.cpp#L1416
[5] https://github.com/mozilla/gecko-dev/blob/3ecf89da497cf1abe2a89d1b3c282b48e5dfac8c/js/src/vm/UnboxedObject.cpp#L1150
[6] https://github.com/mozilla/gecko-dev/blob/3ecf89da497cf1abe2a89d1b3c282b48e5dfac8c/js/src/vm/UnboxedObject.cpp#L756

Comment 1

[Jan de Mooij [:jandem]]

Thanks for the report. We have to check if there's a way to trigger this without unboxed objects. Maybe __proto__ mutation.

Regardless, ObjectGroupDispatch is complicated enough that we should probably have it crash in release builds too when it sees an unexpected group.

Comment 2

[Nicolas B. Pierron [:nbp]]

Matthew, you might have the fresh-est knowledge about this part of the code. Mind to have a look and forwarding to the right person or taking it?

Comment 3

[Matthew Gaudet (he/him) [:mgaudet]]

Ok, I'll spend today looking at this. At the bare minimum I'll produce a patch to crash on an unexpected group in ObjectGroupDispatch.

Comment 4

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1536768 - Check completeness of ObjectGroupDispatch in opt builds r?jandem

Comment 5

[Matthew Gaudet (he/him) [:mgaudet]]

Investigation into other group change mechanisms

tldr; I can't prove this is unexploitable outside of UnboxedObjects, due to my unfamiliarity with edge cases of group changes. It seems generally to be an excellent idea to assert in ObjectGroupDispatch. However, I was unable to generate an exploitable test case from attempting to trigger group changes through other mechanisms.

In order for this to be exploitable outside UnboxedObjects we would need to find a way to change the group of an object in such a way that we don't trigger bailout or invalidation, as was the case of the UnboxedLayout bug described in this bug.

First, let's look at the callers of setGroup(). These callers can be divided into roughly three categories:

1. Allocation: These calls to setGroup apply only to newly created objects; e.g. js::MapIteratorObject::createResultPair, js::NewArrayOperationWithTemplate.

2. Partially Initialized -> Fully initialized: This would be the transition that occurs in UpdateShapeTypeAndValue. This happens when an objects transitions into its final 'initialized' shape, and the group is changed from a partially-initalized group to a final group.

   I was unable to determine how the boundaries of the "new script" range are determined, so I was unable to experiment with these group transitions.

3. Singleton-ification: JSObject::setSingleton and JSObject::changeToSingleton modify the group. This seems relatively controlled.

4. Prototype Modification: or SetProto.

The most promising in terms of exploitability seems to be prototype mutation. However, in my experiments, I was unable to trigger a situation where prototype mutation did not in turn invalidate or bailout from Ion.

Caveats:

There are aspects I haven't investigated that might be cause for concern: While I think that this is most likely to be exploitable by trying to change the group of an object in an Ion frame without triggering bailout, it is possible that there are other ways that type barriers can be missed leading to an ObjectGroupDispatch. I didn't really investigate them.

Final Comment

This was not a comprehensive audit: I don't feel like I am qualified to give an overarching recommendation about potential exploitability of this bug, as it sits at the corner of a number of things I have a weak knowledge of: Type Barriers, group mutation, and Ion's guard invariants.

Comment 6

[Matthew Gaudet (he/him) [:mgaudet]]

Attachment: Bug 1536768 - Check completeness of FunctionDispatch without fallbacks r?jandem

Depends on D25304

Comment 7

[Matthew Gaudet (he/him) [:mgaudet]]

Jan requested I also check the completeness of FunctionDispatch; see above patch.

This change has the potential to cause crashes where we formerly wouldn't have: I'm not sure if we want to subject this patch to some fuzzing before we land it?

Comment 8

[Matthew Gaudet (he/him) [:mgaudet]]

Comment on attachment 9054337 [details]
Bug 1536768 - Check completeness of ObjectGroupDispatch in opt builds r?jandem

Security Approval Request

• How easily could an exploit be constructed based on the patch?: It would be difficult.
• 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 UnboxedObjects were enabled, however, we disabled them by default on all supported branches already
• If not all supported branches, which bug introduced the flaw?: Bug 1116855
• Do you have backports for the affected branches?: No
• If not, how different, hard to create, and risky will they be?: They should be easy to do.
• How likely is this patch to cause regressions; how much testing does it need?: It's possible that this could create crashes where we were silently experiencing a vulnerability (and potentially incorrect execution).

Comment 9

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

Comment on attachment 9054337 [details]
Bug 1536768 - Check completeness of ObjectGroupDispatch in opt builds r?jandem

Sec-approval+ for mozilla-central. Please nominate ESR60 and Beta patches as well.

Comment 10

[Matthew Gaudet (he/him) [:mgaudet]]

Comment on attachment 9054337 [details]
Bug 1536768 - Check completeness of ObjectGroupDispatch in opt builds r?jandem

Beta/Release Uplift Approval Request

• Feature/Bug causing the regression: Bug 1116855
• User impact if declined: Potential security issue
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• 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): It's possible that this could create crashes where we were silently experiencing a vulnerability (and potentially incorrect execution).
• String changes made/needed: None

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration:
• User impact if declined: Potential security issue
• Fix Landed on Version:
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It's possible that this could create crashes where we were silently experiencing a vulnerability (and potentially incorrect execution).
• String or UUID changes made by this patch: None

Comment 11

[Matthew Gaudet (he/him) [:mgaudet]]

Landed on autoland:

https://hg.mozilla.org/integration/autoland/rev/8f07c3939286 - Bug 1536768 - Check completeness of ObjectGroupDispatch in opt builds r=jandem
https://hg.mozilla.org/integration/autoland/rev/c22855eae212 - Bug 1536768 - Check completeness of FunctionDispatch without fallbacks r=jandem

Comment 12

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

https://hg.mozilla.org/mozilla-central/rev/8f07c3939286
https://hg.mozilla.org/mozilla-central/rev/c22855eae212

Comment 13

[Pascal Chevrel:pascalc]

Comment on attachment 9054337 [details]
Bug 1536768 - Check completeness of ObjectGroupDispatch in opt builds r?jandem

Approved for 67 beta 10, thanks.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b4fc373023df
https://hg.mozilla.org/releases/mozilla-beta/rev/8f151985cb7f

Comment 15

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

Comment on attachment 9054337 [details]
Bug 1536768 - Check completeness of ObjectGroupDispatch in opt builds r?jandem

Fix for sec-high issue, let's take it for ESR 60.7.

Comment 16

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

https://hg.mozilla.org/releases/mozilla-esr60/rev/303fa85d8f89
https://hg.mozilla.org/releases/mozilla-esr60/rev/c9328455c9c1

Comment 17

[Andrew McCreight [:mccr8]]

The Project Zero report on this (which looks to be the same as comment 0) is public now.