Closed Bug 1505181 Opened 10 months ago Closed 9 months ago

Crash [@ js::jit::IonSetPropertyIC::update] or Crash [@ ??] or Crash [@ JSObject::getClass] with invalid address and TypedArray

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla65
Tracking Status
firefox-esr60 64+ fixed
firefox63 --- wontfix
firefox64 + fixed
firefox65 + fixed

People

(Reporter: decoder, Assigned: jandem)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [jsbugmon:update][adv-main64+][adv-esr60.4+])

Crash Data

Attachments

(4 files, 1 obsolete file)

The following testcase crashes on mozilla-central revision d2963b5a2897 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-profiling --disable-debug --enable-optimize, run with --fuzzing-safe --cpu-count=2 --ion-offthread-compile=off --disable-oom-functions --ion-eager --baseline-eager):

See attachment.


Backtrace:

received signal SIGSEGV, Segmentation fault.
0x0000555555ec0750 in js::jit::IonSetPropertyIC::update (cx=0x7ffff5f28800, outerScript=..., ic=0x7ffff580c8b0, obj=..., idVal=..., rhs=...) at js/src/jit/IonIC.cpp:265
#0  0x0000555555ec0750 in js::jit::IonSetPropertyIC::update (cx=0x7ffff5f28800, outerScript=..., ic=0x7ffff580c8b0, obj=..., idVal=..., rhs=...) at js/src/jit/IonIC.cpp:265
#1  0x000032ab7bbb658a in ?? ()
#2  0x00007fffffffa5a0 in ?? ()
#3  0x00007fffffffa4c8 in ?? ()
#4  0x00007fffffffa5a8 in ?? ()
#5  0x000055555739b860 in ?? ()
#6  0x000032ab7bcf8363 in ?? ()
#7  0x0000000000008020 in ?? ()
#8  0x00007ffff59d3a60 in ?? ()
#9  0x00007ffff580c8b0 in ?? ()
#10 0x0007800000000000 in ?? ()
#11 0xfffb7ffff599db28 in ?? ()
#12 0xfffe7ffff59d2ac0 in ?? ()
#13 0xe5e5e5e5e5e5e5e5 in ?? ()
#14 0x0007800000000000 in ?? ()
#15 0x00007ffff5abcd40 in ?? ()
#16 0x00007ffff59d2ac0 in ?? ()
#17 0x00007ffff580c8b0 in ?? ()
#18 0x00007ffff59d2b40 in ?? ()
#19 0x0000000000000000 in ?? ()
rax	0x7800000000000	2111062325329920
rbx	0x0	0
rcx	0x0	0
rdx	0x0	0
rsi	0x7fffffffa4e0	140737488332000
rdi	0x7ffff5f28800	140737319700480
rbp	0x7fffffffa4f8	140737488332024
rsp	0x7fffffffa1c0	140737488331200
r8	0x7fffffffa4f8	140737488332024
r9	0x7fffffffa500	140737488332032
r10	0x7fffffffa4e0	140737488332000
r11	0x55555739b860	93825023981664
r12	0x7fffffffa4f0	140737488332016
r13	0x7ffff580c8b0	140737312245936
r14	0x7fffffffa220	140737488331296
r15	0x7ffff5f28800	140737319700480
rip	0x555555ec0750 <js::jit::IonSetPropertyIC::update(JSContext*, JS::Handle<JSScript*>, js::jit::IonSetPropertyIC*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::Value>)+272>
=> 0x555555ec0750 <js::jit::IonSetPropertyIC::update(JSContext*, JS::Handle<JSScript*>, js::jit::IonSetPropertyIC*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::Value>)+272>:	mov    (%rax),%rdx
   0x555555ec0753 <js::jit::IonSetPropertyIC::update(JSContext*, JS::Handle<JSScript*>, js::jit::IonSetPropertyIC*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::Value>)+275>:	mov    (%rdx),%rdx


This looks like a write with a broken address and the address has changed multiple times during reduction, from looking totally random to NULL to the one in this trace now. Marking as sec-critical based on that. The testcase is not fully reduced yet (the crash seems to require some of the logic of the new driver I wrote, just running the finalized code doesn't work), so I'm filing it as it is so we get to work on it as soon as possible.
Attached file Testcase
Attached file More reduced testcase (obsolete) —
This repros with --ion-offthread-compile=off --ion-eager
Attached file A bit nicer
Attachment #9023209 - Attachment is obsolete: true
I'm looking into this. Seems to be TI related probably.
Flags: needinfo?(jdemooij)
Old bug. Took me a while to track down.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Flags: needinfo?(jdemooij)
Just to make sure: the worst that can happen here is that we reinterpret UndefinedValue as something else, right? Can you think of worse problems when we don't rollbackPartiallyInitializedObjects?
Flags: needinfo?(bhackett1024)
Or there's also the UnboxedPlainObject::convertToNative call that we shouldn't be skipping probably. Enough complexity to keep this sec-crit I guess.
During reduction, I have 100% seen the crash address jump around, it wasn't just 0x7800000000000 but I also saw addresses starting with 0x33... etc. so I assume this can have multiple side-effects.
(In reply to Jan de Mooij [:jandem] from comment #7)
> Just to make sure: the worst that can happen here is that we reinterpret
> UndefinedValue as something else, right? Can you think of worse problems
> when we don't rollbackPartiallyInitializedObjects?

This seems like it should be the case, and not calling convertToNative shouldn't cause downstream problems, but I can't be sure and comment 9 suggests otherwise.
Flags: needinfo?(bhackett1024)
autobisectjs shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/7820fd141998
user:        Brian Hackett
date:        Tue Jan 27 02:47:25 2015 -0700
summary:     Bug 1116855 - Add default-disabled unboxed objects for use by interpreted constructors, r=jandem.

Would bug 1116855 be a likely regressor?
Blocks: 1116855
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Comment on attachment 9023414 [details]
Bug 1505181 - Use canonical function in TypeNewScript::rollbackPartiallyInitializedObjects. r?bhackett

[Security Approval Request]

How easily could an exploit be constructed based on the patch?: It's 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 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?: Should either apply or be easy to backport.

How likely is this patch to cause regressions; how much testing does it need?: Should be unlikely. Fuzzing + Nightly testing will be sufficient I think.
Attachment #9023414 - Flags: sec-approval?
sec-approval+ for trunk.
Please nominate beta and ESR60 patches as well.
Attachment #9023414 - Flags: sec-approval? → sec-approval+
Comment on attachment 9023414 [details]
Bug 1505181 - Use canonical function in TypeNewScript::rollbackPartiallyInitializedObjects. r?bhackett

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Unknown

User impact if declined: Security issues, crashes.

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): Patch is relatively small and well understood but it's tricky code.

String changes made/needed: None
Attachment #9023414 - Flags: approval-mozilla-esr60?
Attachment #9023414 - Flags: approval-mozilla-beta?
Attached patch Patch for ESR60Splinter Review
Attachment #9024711 - Flags: approval-mozilla-esr60?
Attachment #9023414 - Flags: approval-mozilla-esr60?
https://hg.mozilla.org/mozilla-central/rev/2599b449d317
Group: javascript-core-security → core-security-release
Status: ASSIGNED → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Comment on attachment 9023414 [details]
Bug 1505181 - Use canonical function in TypeNewScript::rollbackPartiallyInitializedObjects. r?bhackett

[Triage Comment]
Fixes a sec-crit. Approved for 64.0b10 and 60.4.0esr.
Attachment #9023414 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9024711 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
I have tried to reproduce this issue with the following build : https://tools.taskcluster.net/index/gecko.v2.mozilla-central.pushdate.2018.11.06.20181106095323.firefox but without any success. 

I see that this is covered by automated tests so I will remove the qe-verify+ flag.
Flags: qe-verify+ → qe-verify-
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main64+][adv-esr60.4+]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.