996715 - Crash [@ compartment] with invalid object pointer

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 1f932e462b84 (x86 ARM simulator build, run with --fuzzing-safe):


function g(e,a) {
    return Math.abs(a - e) <= 1E-10;
}
function w(s) {}
eval("(function() { " + "\
var FOUR_HUNDRED_YEARS = (0);\
var SECTION         =  (0);\
w(\"15.9.1.1 Time Range\");\
for ( var M_SECS = 0, CURRENT_YEAR = 1970; M_SECS < 8640000000000000; M_SECS += FOUR_HUNDRED_YEARS, CURRENT_YEAR += 400) {\
  g(SECTION,  '%' + (new String('1') % undefined), (new Date(M_SECS)).getUTCFullYear());\
}\
test();\
" + " })();");

Comment 1

[Christian Holler (:decoder)]

Crash trace:


Program received signal SIGSEGV, Segmentation fault.
compartment (this=0xf6926980) at js/src/jit/shared/CodeGenerator-shared.cpp:1069
1069    } // namespace js
(gdb) bt 8
#0  compartment (this=0xf6926980) at js/src/jit/shared/CodeGenerator-shared.cpp:1069
#1  js::jit::AssertValidObjectPtr (cx=0x94d1f68, obj=(JSObject *) 0xf6926980 Cannot access memory at address 0x0) at js/src/jit/VMFunctions.cpp:1046
#2  0x083ec928 in js::jit::Simulator::softwareInterrupt (this=0x94d15e8, instr=0x958874c) at js/src/jit/arm/Simulator-arm.cpp:2145
#3  0x083e9905 in decodeType7 (instr=0x958874c, this=0x94d15e8) at js/src/jit/arm/Simulator-arm.cpp:3179
#4  js::jit::Simulator::instructionDecode (this=0x94d15e8, instr=0x958874c) at js/src/jit/arm/Simulator-arm.cpp:4036
#5  0x083ed018 in execute<false> (this=0x94d15e8) at js/src/jit/arm/Simulator-arm.cpp:4068
#6  js::jit::Simulator::callInternal (this=0x94d15e8, entry=0xf64af7d8 "\360O-\351\r\200\240\341\304\314\005\343L\311", <incomplete sequence \343>) at js/src/jit/arm/Simulator-arm.cpp:4150
#7  0x083ed28c in js::jit::Simulator::call (this=0x94d15e8, entry=0xf64af7d8 "\360O-\351\r\200\240\341\304\314\005\343L\311", <incomplete sequence \343>, argument_count=8)
    at js/src/jit/arm/Simulator-arm.cpp:4230
(More stack frames follow...)
(gdb) x /i $pc
=> 0x83a3e14 <js::jit::AssertValidObjectPtr(JSContext*, JSObject*)+36>: mov    (%eax),%eax
(gdb) info reg eax
eax            0xffffff82       -126

Comment 2

[Douglas Crosher [:dougc]]

Tracing the code shows a suspect jump, at 0xf77afddc.  The destination does not look plausible.  So it looks like a bug in the assembler buffer code.  These bugs can be very sensitive to the instruction layout and using the backtracking register allocator chances the instructions enough to avoid tickling this bug.  The assembler buffer code is being rewritten in bug 972710 and it does not reproduce there.

0xf77afd84	ldr	sp, [sp]
0xf77afd88	vpop	{d0, d1, d2, d3, d4, d5, d6, d7}
0xf77afd8c	pop	{r0, r1, r2, r3}
0xf77afd90	ldr	r0, [sp], #4
0xf77afd94	b	#64               ; to f77afddc ok
<constant pool>
0xf77afddc	b	#4080             ; from f77afd94 ok, to 0xf77b0dd4 XX
0xf77afde0	str	r0, [sp, #-4]!
0xf77afde4	str	r1, [sp, #-4]!
0xf77afde8	movw	r0, #32768
0xf77afdec	movt	r0, #63082
0xf77afdf0	ldr	r12, [r0, #144]
0xf77afdf4	cmp	r12, #0
...
0xf77b0dc0	ldr	r10, [sp, #36]
0xf77b0dc4	ldr	r9, [sp, #32]
0xf77b0dc8	ldr	r7, [sp, #28]
0xf77b0dcc	ldr	r6, [sp, #24]
0xf77b0dd0	ldr	r5, [sp, #20]
0xf77b0dd4	ldr	r4, [sp, #16]     ; from f77afddc XXX
0xf77b0dd8	ldr	r3, [sp, #12]
0xf77b0ddc	ldr	r2, [sp, #8]
0xf77b0de0	ldr	r1, [sp, #4]
0xf77b0de4	ldr	r0, [sp]
0xf77b0de8	add	sp, sp, #40
0xf77b0dec	b	#-4296            ; to 0xf77afd2c

Comment 3

[Douglas Crosher [:dougc]]

Forgot to add that errant branch appears to be a patched branch.

Comment 4

[Daniel Veditz [:dveditz]]

rewrites like bug 972710 are unlikely to be back-ported to old branches. What can we do to fix this on old branches to fix our shipping devices? Or is my paranoia reading too much into the implications of "suspect jump"?

Comment 5

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

Adding Marty to answer comment 4.

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Marty, ping? Still curious about Dan's paranoia (comment 4)...

Comment 7

[Jan de Mooij [:jandem]]

Assigning to Marty, we need to fix this now before the new assembler buffer is ready.

Comment 8

[Marty Rosenberg [:mjrosenb]]

Attachment: ActuallyCheckBranch-r0.patch

Ok, I'm sure there was some reason I did this, My best guess is that I used the wrong variable? We pass all tests with the patch applied, and the testcase in this bug no longer crashes.

Comment 9

[Douglas Crosher [:dougc]]

Comment on attachment 8425755 [details] [diff] [review]
ActuallyCheckBranch-r0.patch

Review of attachment 8425755 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. I can't find any reason why the first branch in a slice should not have been patched. Thank you.

::: js/src/jit/shared/IonAssemblerBufferWithConstantPools.h
@@ +220,5 @@
>          isBranch[idx >> 3] |= 1 << (idx & 0x7);
>      }
>      bool isNextBranch() {
>          unsigned int size = this->nodeSize;
> +        if (size >= SliceSize) {

Perhaps the brace should not be included for a one line statement.

Comment 10

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 8425755 [details] [diff] [review]
ActuallyCheckBranch-r0.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Incredibly difficult
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
not even close
Which older supported branches are affected by this flaw?
everything since IonMonkey landed (18 or 19)
If not all supported branches, which bug introduced the flaw?

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
yes, this code hasn't changed.
How likely is this patch to cause regressions; how much testing does it need?
Unlikely to cause regressions, we should still fuzz it to death.

Comment 11

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

Comment on attachment 8425755 [details] [diff] [review]
ActuallyCheckBranch-r0.patch

sec-approval+ for trunk.

We should get Aurora, Beta, and ESR24 patches for this made and nominated so we can check them in after trunk goes green.

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Ni? on the assignee to ensure that the request in comment 11 is on their radar.  Next week is the last for FF 30 Beta builds so we'll want to get uplifts asap.

Comment 13

[Marty Rosenberg [:mjrosenb]]

landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/9d8a5c8d8328 I'll request uplift approval this evening.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9d8a5c8d8328

Comment 15

[Marty Rosenberg [:mjrosenb]]

Comment on attachment 8425755 [details] [diff] [review]
ActuallyCheckBranch-r0.patch

Review of attachment 8425755 [details] [diff] [review]:
-----------------------------------------------------------------

This patch should apply cleanly to all currently

Comment 16

[Marty Rosenberg [:mjrosenb]]

Attachment: 996715-esr24-r0.patch

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: 
possible crashes, possibly exploitable
Fix Landed on Version:
mozilla-central
Risk to taking this patch (and alternatives if risky): 
basically risk free.
String or UUID changes made by this patch: 

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 17

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8429880 [details] [diff] [review]
996715-esr24-r0.patch

Review of attachment 8429880 [details] [diff] [review]:
-----------------------------------------------------------------

ESR approval granted. Please land this week.

Comment 18

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8425755 [details] [diff] [review]
ActuallyCheckBranch-r0.patch

Aurora and Beta approval granted. Please land ASAP.

Release approval denied based on Al's won't fix for 29.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr24/rev/995acd2ae9b9

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/a6eac29e49ec
https://hg.mozilla.org/releases/mozilla-b2g26_v1_2/rev/d2b20f54d7e5

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/44bc3af856a4
https://hg.mozilla.org/releases/mozilla-beta/rev/23cc851c112b

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/23cc851c112b

Comment 23

[Matt Wobensmith [:mwobensmith][:matt:]]

Hi decoder - can you verify fixed in Fx30 and/or Fx24esr? Thanks!

Comment 24

[Christian Holler (:decoder)]

I can try to Fx30, but Fx24esr doesn't have the ARM simulator, so we can't verify it there.

Comment 25

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 26

[Christian Holler (:decoder)]

Verified manually on Fx30 and Fx31, Fx24esr can't be verified as I mentioned before.

Comment 27

[Justin Wood (:Callek)]

https://hg.mozilla.org/releases/mozilla-release/rev/af7d521d4fb3

Comment 28

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed on Fx32