Closed Bug 1514682 (CVE-2019-9795) Opened 2 years ago Closed 2 years ago

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

Categories

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

66 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla67
Tracking Status
firefox-esr60 66+ fixed
firefox65 --- wontfix
firefox66 + fixed
firefox67 + fixed

People

(Reporter: nils, Assigned: tcampbell)

Details

(Keywords: sec-high, Whiteboard: [jsbugmon:update,origRev=edf1f05e9d00,testComment=2][post-critsmash-triage][adv-main66+][adv-esr60.6+])

Attachments

(3 files)

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
Whiteboard: [jsbugmon:update,bisect,origRev=5c892a6147ae]
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__;
}
Group: javascript-core-security
Flags: needinfo?(jdemooij)
Group: core-security
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)
Whiteboard: [jsbugmon:update,bisect,origRev=5c892a6147ae] → [jsbugmon:update,origRev=edf1f05e9d00,testComment=2]
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)
Flags: needinfo?(tcampbell)
I'll take this. Yes, that seems related.
Assignee: nobody → tcampbell
Flags: needinfo?(jdemooij)
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;
I have made some tweaks to jsfunfuzz to try and generate the patterns shown in comment 5.
Keywords: sec-high

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

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.

Flags: needinfo?(tcampbell)
Priority: -- → P1

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.

Flags: needinfo?(nth10sd)
Flags: needinfo?(choller)

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 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.

Attachment #9041348 - Flags: sec-approval?

(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.

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

Attachment #9041348 - Flags: sec-approval? → sec-approval+

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

Attachment #9041348 - Flags: approval-mozilla-beta?

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

Attachment #9044248 - Flags: approval-mozilla-esr60?

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

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

Attachment #9041348 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

(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 :)

Flags: needinfo?(choller)

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

Flags: needinfo?(nth10sd)
Group: javascript-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla67
Flags: qe-verify-
Whiteboard: [jsbugmon:update,origRev=edf1f05e9d00,testComment=2] → [jsbugmon:update,origRev=edf1f05e9d00,testComment=2][post-critsmash-triage]
Flags: sec-bounty?
Flags: sec-bounty? → sec-bounty+
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.
Attachment #9044248 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Flags: in-testsuite+
Whiteboard: [jsbugmon:update,origRev=edf1f05e9d00,testComment=2][post-critsmash-triage] → [jsbugmon:update,origRev=edf1f05e9d00,testComment=2][post-critsmash-triage][adv-main66+][adv-esr60.6+]
Alias: CVE-2019-9795
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.