Closed Bug 1360852 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla55
Tracking Status
firefox-esr45 --- unaffected
firefox-esr52 --- unaffected
firefox53 --- wontfix
firefox54 + verified
firefox55 + verified

People

(Reporter: decoder, Assigned: jandem)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [jsbugmon:update,bisect][adv-main54+])

Crash Data

Attachments

(2 files)

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).
Flags: needinfo?(jdemooij)
Flags: needinfo?(jdemooij)
Attached patch Part 1 - FixSplinter Review
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.
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8863333 - Flags: review?(evilpies)
Attached patch Part 2 - TestSplinter Review
Attachment #8863334 - Flags: review?(evilpies)
[Tracking Requested - why for this release]:
Blocks: 1357024
Duplicate of this bug: 1361050
Tracking 54/55+. I see Android crashes in this signature in the duped Bug in Comment 4.
Attachment #8863333 - Flags: review?(evilpies) → review+
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.
Attachment #8863333 - Flags: sec-approval?
Attachment #8863333 - Flags: approval-mozilla-beta?
Comment on attachment 8863333 [details] [diff] [review]
Part 1 - Fix

Giving sec-approval+ and Beta approval.
Attachment #8863333 - Flags: sec-approval?
Attachment #8863333 - Flags: sec-approval+
Attachment #8863333 - Flags: approval-mozilla-beta?
Attachment #8863333 - Flags: approval-mozilla-beta+
Attachment #8863334 - Flags: review?(evilpies) → review+
https://hg.mozilla.org/mozilla-central/rev/70bcf05f0210
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Group: javascript-core-security → core-security-release
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update,bisect][adv-main54+]
JSBugMon: This bug has been automatically verified fixed on Fx54
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.