1360852 - Crash [@ js::jit::IonSetPropertyIC::update] with invalid write

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 308cdb913d71 (build with --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --enable-debug --enable-optimize, run with --fuzzing-safe --no-threads --ion-eager):

function f() {
  var _76 = {};
  for (var i = 0; i < arguments.length; i++) {
    var typ = arguments[i];
    _76[typ] = typ;
  }
}
g = f("number", "boolean", "object");
f(0, -0)
f(0, 2147483649)



Backtrace:

 received signal SIGSEGV, Segmentation fault.
0x000000000073711a in js::jit::IonSetPropertyIC::update (cx=0x7ffff694c000, outerScript=..., ic=0x7ffff5b48518, obj=..., idVal=..., rhs=...) at js/src/jit/IonIC.cpp:210
#0  0x000000000073711a in js::jit::IonSetPropertyIC::update (cx=0x7ffff694c000, outerScript=..., ic=0x7ffff5b48518, obj=..., idVal=..., rhs=...) at js/src/jit/IonIC.cpp:210
#1  0x000012e863a1d2e9 in ?? ()
#2  0x0000000000000000 in ?? ()
rax	0x7fffffffc2c8	140737488339656
rbx	0x7fffffffbf90	140737488338832
rcx	0x0	0
rdx	0x0	0
rsi	0x41e0000000200000	4746794007250599936
rdi	0x7ffff5b4855a	140737315636570
rbp	0x7fffffffc270	140737488339568
rsp	0x7fffffffbf20	140737488338720
r8	0x7fffffffc2d0	140737488339664
r9	0x7fffffffc2d8	140737488339672
r10	0x7fffffffc2b8	140737488339640
r11	0x7ffff5e7cc40	140737318997056
r12	0x7fffffffbf70	140737488338800
r13	0x7ffff5b48518	140737315636504
r14	0x7ffff694c000	140737330331648
r15	0x7ffff694c000	140737330331648
rip	0x73711a <js::jit::IonSetPropertyIC::update(JSContext*, JS::Handle<JSScript*>, js::jit::IonSetPropertyIC*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::Value>)+618>
=> 0x73711a <js::jit::IonSetPropertyIC::update(JSContext*, JS::Handle<JSScript*>, js::jit::IonSetPropertyIC*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::Value>)+618>:	mov    (%rsi),%rax
   0x73711d <js::jit::IonSetPropertyIC::update(JSContext*, JS::Handle<JSScript*>, js::jit::IonSetPropertyIC*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::Value>)+621>:	mov    (%rax),%rax


Marking this sec-critical because this looks like an invalid write to a weird address (which might even be controllable given the values in the address).

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - Fix

GuardIsInt32Index needs to allocate the input register before calling addFailurePath.

This is a regression from bug 1330793, although it's possible this wasn't a problem until bug 1341067 or bug 1357024.

We should probably add some more assertions to the compiler/regalloc to check these things in debug builds.

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Test

Comment 3

[Jan de Mooij [:jandem]]

[Tracking Requested - why for this release]:

Comment 5

[Marcia Knous [:marcia]]

Tracking 54/55+. I see Android crashes in this signature in the duped Bug in Comment 4.

Comment 6

[Jan de Mooij [:jandem]]

Comment on attachment 8863333 [details] [diff] [review]
Part 1 - Fix

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
Pretty difficult or impossible, at least on current release and beta.

> 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?
The code was in 53 but it's likely only a problem on 54.

> If not all supported branches, which bug introduced the flaw?
Bug 1330793.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should be trivial.

> How likely is this patch to cause regressions; how much testing does it need?
Very unlikely to cause regressions.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1330793 but as mentioned above it's likely not a problem in 53.
[User impact if declined]: Crashes, security bugs.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not yet.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Very simple one-line fix.
[String changes made/needed]: None.

Comment 7

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

Comment on attachment 8863333 [details] [diff] [review]
Part 1 - Fix

Giving sec-approval+ and Beta approval.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/70bcf05f0210a49d3e5aae0f204fd0249be3af9c

Will get it uplifted to Beta later today in time for b4.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4c630b9a7d2f

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/70bcf05f0210

Comment 11

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 12

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed on Fx54