1514682 - (CVE-2019-9795) Assertion failure: obj->is<PlainObject>(), at /builds/worker/workspace/build/src/js/src/jit/CacheIR.cpp:4626

Status: RESOLVED FIXED

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

Description

The following testcase crashes the latest debug build of the jsshell. It requires the --ion-eager flag.

Testcase:


o0=(class Cl26349 extends ReferenceError{ set stack (x) {super.stack=fun0()}});
o1=new o0(o0,null);
o0.__proto__=o1.__proto__;
o26=[1.1,2.2,3.3];
o26.__proto__=o0.__proto__;
o39=[1.1,2.2,3.3];
o26['stack']={};
function fun0() {
        o26.__proto__=o39.__proto__;
        return 0xdead;
}


Debugger output:
lldb-4.0 -- ./build/js --ion-eager bug.js
(lldb) target create "./build/js"
Current executable set to './build/js' (x86_64).
(lldb) settings set -- target.run-args  "--ion-eager" "bug.js"
(lldb) r
Process 18223 launched: './build/js' (x86_64)
Assertion failure: obj->is<PlainObject>(), at /builds/worker/workspace/build/src/js/src/jit/CacheIR.cpp:4626
Process 18223 stopped
* thread #1, name = 'js', stop reason = signal SIGSEGV: invalid address (fault address: 0x0)
    frame #0: 0x00005555561bd340 js`js::jit::SetPropIRGenerator::tryAttachAddSlotStub(JS::Handle<js::ObjectGroup*>, JS::Handle<js::Shape*>) + 4800
js`js::jit::SetPropIRGenerator::tryAttachAddSlotStub:
->  0x5555561bd340 <+4800>: movl   $0x1212, 0x0              ; imm = 0x1212
    0x5555561bd34b <+4811>: callq  0x5555557655f2            ; abort
    0x5555561bd350 <+4816>: leaq   0x7f4676(%rip), %rdi      ;  + 10701
    0x5555561bd357 <+4823>: leaq   0x7f467d(%rip), %rsi      ;  + 10715
(lldb) bt 16
* thread #1, name = 'js', stop reason = signal SIGSEGV: invalid address (fault address: 0x0)
  * frame #0: 0x00005555561bd340 js`js::jit::SetPropIRGenerator::tryAttachAddSlotStub(JS::Handle<js::ObjectGroup*>, JS::Handle<js::Shape*>) + 4800
    frame #1: 0x000055555609e9a2 js`js::jit::DoSetPropFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICSetProp_Fallback*, JS::Value*, JS::Handle<JS::Value>, J                                                                                        S::Handle<JS::Value>) + 2738
    frame #2: 0x000025b6efb8e9f5
    frame #3: 0x000025b6efb7cac4
    frame #4: 0x000055555632c914 js`js::jit::MaybeEnterJit(JSContext*, js::RunState&) + 1636
    frame #5: 0x00005555557e2ac7 js`js::RunScript(JSContext*, js::RunState&) + 535
    frame #6: 0x00005555557f7d75 js`js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) + 1077
    frame #7: 0x00005555557f837d js`js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) + 621
    frame #8: 0x000055555591fa96 js`ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::Value*) + 294
    frame #9: 0x000055555591fc06 js`JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>) + 150
    frame #10: 0x00005555557422d9 js`RunFile(JSContext*, char const*, _IO_FILE*, CompileUtf8, bool) + 521
    frame #11: 0x000055555574165f js`Process(JSContext*, char const*, bool, FileKind) + 2463
    frame #12: 0x00005555556f9ed0 js`main + 17552
    frame #13: 0x00007ffff6af81c1 libc.so.6`__libc_start_main + 241
    frame #14: 0x00005555556f0f55 js`_start + 41

Comment 1

[Christian Holler (:decoder)]

Slightly simplified:


o0 = class Cl26349 { 
  set m(x) {
    super.m = fun0()
  }
}
o1 = new o0();
o0.__proto__ = o1.__proto__;
o26 = [];
o26.__proto__ = o0.__proto__;
o39 = [];
o26['m'] = {};
function fun0() {
  o26.__proto__ = o39.__proto__;
}

Comment 2

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

function f() {
    z.__proto__ = [];
}
x = class {
    set g(x) {
        super.g = f();
    }
}
z = [];
z.__proto__ = (new x);
z["g"] = 0;

asserts js shell compiled with "--enable-debug --enable-more-deterministic" on m-c rev edf1f05e9d00 with --fuzzing-safe --no-threads --ion-eager at Assertion failure: obj->is<PlainObject>(), at /home/ubuntu/trees/mozilla-central/js/src/jit/CacheIR.cpp:4626

(even further simplified)

Comment 3

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/24191914f43b
user:        Ted Campbell
date:        Wed Jun 07 14:04:13 2017 -0400
summary:     Bug 1169743 - Implement class decls with extends in Baseline (cont.) r=jandem

Ted, is bug 1169743 a likely regressor?

(not setting dependency on "Blocks" yet until this lands)

Comment 4

[Ted Campbell [:tcampbell]]

I'll take this. Yes, that seems related.

Comment 5

[Ted Campbell [:tcampbell]]

This is a bug in CacheIR SetProp / AddSlot logic.

Simplified testcase without any class stuff or setter/getter notation:

> // Setter implementation
> var s = function(_) {
>     // Define data-property |g| on z
>     Object.defineProperty(this, "g", {value: 1,
>                                       writable: true,
>                                       configurable: true,
>                                       enumerable: true});
> 
>     // Change proto of z to change its group
>     Object.setPrototypeOf(this, null);
> };
> 
> // Prototype with setter |g|
> var y = {};
> Object.defineProperty(y, "g", {set: s,
>                                configurable: true,
>                                enumerable: true});
> 
> // |z| is an Array exotic object with prototype |y|
> var z = [];
> Object.setPrototypeOf(z, y);
> 
> // Trigger setter |y.g| via prototype chain
> z.g = 0;

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I have made some tweaks to jsfunfuzz to try and generate the patterns shown in comment 5.

Comment 7

[Steven DeTar [:sdetar]]

Ted would it make sense to have Matthew look at this now, since he is back from his leave?

Comment 8

[Ted Campbell [:tcampbell]]

I will post a patch tomorrow. I've ended up going with a quick fix for the purpose of closing out this bug. This can all be reworked to be cleaner on master once unboxed-objects are removed.

Comment 9

[Ted Campbell [:tcampbell]]

Attachment: Bug 1514682 - Split up AddSlot IC logic

Comment 10

[Ted Campbell [:tcampbell]]

Attachment: RELEASE-BETA-CENTRAL-Bug-1514682-Split-up-AddSlot-IC-logic.patch

hg-base: 426ca85d23038f08ebe29ce16db8fd484062f1f9
phabricator: https://phabricator.services.mozilla.com/D18631?id=62799

Gary + decoder, can you do a little bit of fuzzing on this patch? It's a sec-high that will probably need uplifts but is tricky perf-sensitive jitcode. I'm very interested on if any of the MOZ_RELEASE_ASSERTs trip.

Comment 11

[Ted Campbell [:tcampbell]]

Attachment: ESR60-Bug-1514682-Split-up-AddSlot-IC-logic.patch

hg-base: db09f3a62a37bbd3c34d3218459ff5304705f230

This is the ESR60 version of the patch. The only issues were if-statements without {}s in the ESR branch and the effect of the clang-format changes around that.

Comment 12

[Ted Campbell [:tcampbell]]

Comment on attachment 9041348 [details]
Bug 1514682 - Split up AddSlot IC logic

Security Approval Request

How easily could an exploit be constructed based on the patch?

The patch has a number of changes going on due to performance considerations. It would take detailed inspection by someone familiar with the code to determine which of the changes was the actual bug. Further, it isn't entirely clear (even to me) how an exploit with be derived.
NOTE: The included test-case is unrelated to the security issue and came up making sure I didn't introduce new bugs. It passes safely on all branches.

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?

None

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?

Moderate risk. This is tricky JIT code, but this patch was iterated a number of times to reduce the scope of changes. Local testing and additional asserts have identified (hopefully all of) the tricky cases and they have been validated (see Phabricator). I'm also requesting jsshell fuzzing of the patch.
I believe this patch to be strictly more correct than the original code and easier to do further patching.

Comment 13

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(In reply to Ted Campbell [:tcampbell] from comment #10)

Created attachment 9044246 [details] [diff] [review]
RELEASE-BETA-CENTRAL-Bug-1514682-Split-up-AddSlot-IC-logic.patch

hg-base: 426ca85d23038f08ebe29ce16db8fd484062f1f9
phabricator: https://phabricator.services.mozilla.com/D18631?id=62799

Now testing this on a Linux box.

Comment 14

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

Sec-approval+ for trunk. Can you please nominate beta and ESR60 patches as well?

Comment 15

[Ted Campbell [:tcampbell]]

Comment on attachment 9041348 [details]
Bug 1514682 - Split up AddSlot IC logic

Beta/Release Uplift Approval Request

Feature/Bug causing the regression

None

User impact if declined

sec-high bug

Is this code covered by automated tests?

Yes

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

Medium

Why is the change risky/not risky? (and alternatives if risky)

Moderate risk. This is tricky JIT code, but this patch was iterated a number of times to reduce the scope of changes. Local testing and additional asserts have identified (hopefully all of) the tricky cases and they have been validated (see Phabricator). I'm also requesting jsshell fuzzing of the patch.
I believe this patch to be strictly more correct than the original code and easier to do further patching.

String changes made/needed

Comment 16

[Ted Campbell [:tcampbell]]

Comment on attachment 9044248 [details] [diff] [review]
ESR60-Bug-1514682-Split-up-AddSlot-IC-logic.patch

ESR Uplift Approval Request

If this is not a sec:{high,crit} bug, please state case for ESR consideration

sec-high

User impact if declined

sec-high bug that will have patch published on nightly

Fix Landed on Version

Planned 67

Risk to taking this patch

Medium

Why is the change risky/not risky? (and alternatives if risky)

Moderate risk. This is tricky JIT code, but this patch was iterated a number of times to reduce the scope of changes. Local testing and additional asserts have identified (hopefully all of) the tricky cases and they have been validated (see Phabricator). I'm also requesting jsshell fuzzing of the patch.
I believe this patch to be strictly more correct than the original code and easier to do further patching.

String or UUID changes made by this patch

Comment 17

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

Comment on attachment 9041348 [details]
Bug 1514682 - Split up AddSlot IC logic

Fix for sec-high issue, let's land it for beta 9.

Comment 18

[Christian Holler (:decoder)]

(In reply to Ted Campbell [:tcampbell] from comment #10)

Created attachment 9044246 [details] [diff] [review]
RELEASE-BETA-CENTRAL-Bug-1514682-Split-up-AddSlot-IC-logic.patch

hg-base: 426ca85d23038f08ebe29ce16db8fd484062f1f9
phabricator: https://phabricator.services.mozilla.com/D18631?id=62799

Gary + decoder, can you do a little bit of fuzzing on this patch? It's a sec-high that will probably need uplifts but is tricky perf-sensitive jitcode. I'm very interested on if any of the MOZ_RELEASE_ASSERTs trip.

Fuzzed this for at least 2 days, no issues found :)

Comment 19

[Ted Campbell [:tcampbell]]

Per Comment 14, I've pushed to Autoland.

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

Comment 20

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I didn't find anything from the comment 10 patch over the weekend either.

Comment 21

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

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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/acf56427a738

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9044248 [details] [diff] [review]
ESR60-Bug-1514682-Split-up-AddSlot-IC-logic.patch

Fixes a JS sec-high. Approved for 60.6esr.

Comment 24

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

https://hg.mozilla.org/releases/mozilla-esr60/rev/22d30ce1468e