Closed Bug 1219883 Opened 7 years ago Closed 7 years ago

Crash [@ js::jit::GetIndexFromString] or Assertion failure: !constant(), at js/src/jit/RegisterSets.h:254


(Core :: JavaScript Engine, defect)

Not set



Tracking Status
firefox42 --- unaffected
firefox43 --- unaffected
firefox44 + verified
firefox45 + verified
firefox-esr38 --- unaffected
b2g-v2.0 --- unaffected
b2g-v2.0M --- unaffected
b2g-v2.1 --- unaffected
b2g-v2.1S --- unaffected
b2g-v2.2 --- unaffected
b2g-v2.5 --- fixed
b2g-v2.2r --- unaffected
b2g-master --- fixed


(Reporter: rforbes, Assigned: jandem)



(5 keywords, Whiteboard: [jsbugmon:][b2g-adv-main2.5-])

Crash Data


(2 files, 1 obsolete file)

The following testcase crashes on mozilla-central revision 7b6a80ca1f0d (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --disable-debug, run with --fuzzing-safe --thread-count=2):

function test() {
    var arr = new Int8Array(400);
    var idx = "384";
    arr[idx] = 9;
    assertEq(arr[idx], 9);


Program received signal SIGSEGV, Segmentation fault.
js::jit::GetIndexFromString (str=0x0) at js/src/jit/VMFunctions.cpp:644
#0  js::jit::GetIndexFromString (str=0x0) at js/src/jit/VMFunctions.cpp:644
#1  0x00007ffff7ff2ee2 in ?? ()
#2  0x00007ffff7e7dbf0 in ?? ()
#3  0x00007ffffff99650 in ?? ()
#4  0x4022000000000000 in ?? ()
#5  0x0000000000000000 in ?? ()
rax	0x6379d0	6519248
rbx	0x7ffff7ff282d	140737354082349
rcx	0xfffc7ffff4624360	-985162613374112
rdx	0x0	0
rsi	0x7ffff4624360	140737293468512
rdi	0x0	0
rbp	0x7ffffff99818	140737487935512
rsp	0x7ffffff99620	140737487935008
r8	0x7ffffff99758	140737487935320
r9	0x7ffffff99660	140737487935072
r10	0x180	384
r11	0x7ffff7e7dbf0	140737352555504
r12	0x8	8
r13	0x7fffffffc6c0	140737488340672
r14	0x0	0
r15	0x7ffffff99818	140737487935512
rip	0x6379d4 <js::jit::GetIndexFromString(JSString*)+4>
=> 0x6379d4 <js::jit::GetIndexFromString(JSString*)+4>:	mov    (%rdi),%edx
   0x6379d6 <js::jit::GetIndexFromString(JSString*)+6>:	mov    $0xffffffff,%eax

Marked as security due to crash with random memory address.
Attached patch Patch (obsolete) — Splinter Review
The GetPropertyIC's TypedArray stub can accept string or int32 indexes. After bug 1209118, the string can be a constant so we need to handle that case. If it's int32 it must be in a register.
Assignee: nobody → jdemooij
Attachment #8680850 - Flags: review?(efaustbmo)
How bad of a security issue is this, Jan? Do we end up putting in random garbage in the case of a constant?
Comment on attachment 8680850 [details] [diff] [review]

Review of attachment 8680850 [details] [diff] [review]:

::: js/src/jit/IonCaches.cpp
@@ +3969,3 @@
>      } else {
>          MOZ_ASSERT(idval.isInt32());

prefer a MOZ_ASSERT(!index.constant()) explicitly here, despite that assert being entailed in the index.reg() calls. It will make explicit the link between integer ids and using registers.
Attachment #8680850 - Flags: review?(efaustbmo) → review+
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:bisect]
JSBugMon: Cannot process bug: Unable to automatically reproduce, please track manually.
Whiteboard: [jsbugmon:bisect] → [jsbugmon:]
Attached patch Part 1 - FixSplinter Review
[Security approval request comment]
* How easily could an exploit be constructed based on the patch?
Not sure, not trivial probably. Maybe with some effort.

* Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

* Which older supported branches are affected by this flaw?

* If not all supported branches, which bug introduced the flaw?
Bug 1209118.

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

* How likely is this patch to cause regressions; how much testing does it need?
Attachment #8680850 - Attachment is obsolete: true
Attachment #8687939 - Flags: sec-approval?
Attachment #8687939 - Flags: review+
Attached patch Part 2 - TestSplinter Review
Attachment #8687940 - Flags: review+
This needs a rating before we can determine if it needs sec-approval to land. Do you have any suggestions? Andrew asked about this in comment 2 as well.
Flags: needinfo?(jdemooij)
(In reply to Al Billings [:abillings] from comment #8)
> This needs a rating before we can determine if it needs sec-approval to
> land. Do you have any suggestions?

Hm it's hard to say but let's go with sec-high. We'll generate bogus JIT code and that's always scary.
Flags: needinfo?(jdemooij)
Keywords: sec-high
Comment on attachment 8687939 [details] [diff] [review]
Part 1 - Fix

sec-approval+. We'll want it nominated for 44 as well.
Attachment #8687939 - Flags: sec-approval? → sec-approval+
Comment on attachment 8687939 [details] [diff] [review]
Part 1 - Fix

Approval Request Comment
[Feature/regressing bug #]: Bug 1209118.
[User impact if declined]: Crashes, correctness/security bugs.
[Describe test coverage new/current, TreeHerder]: Fixes the testcase.
[Risks and why]: Very low risk.
[String/UUID change made/needed]: None.
Attachment #8687939 - Flags: approval-mozilla-aurora?
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment on attachment 8687939 [details] [diff] [review]
Part 1 - Fix

Taking it because it's a sec fix. Aurora44+
Attachment #8687939 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
JSBugMon: This bug has been automatically verified fixed on Fx44
Group: javascript-core-security → core-security-release
JSBugMon: This bug has been automatically verified fixed on Fx45
Whiteboard: [jsbugmon:] → [jsbugmon:][b2g-adv-main2.5-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.