Crash [@ compartment] with invalid object pointer

VERIFIED FIXED in Firefox 30

Status

()

defect
--
major
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: decoder, Assigned: mjrosenb)

Tracking

(Blocks 1 bug, {crash, sec-critical, testcase})

Trunk
mozilla32
ARM
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox29 wontfix, firefox30+ verified, firefox31+ verified, firefox32+ verified, firefox-esr2430+ fixed, b2g-v1.2 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed, b2g-v2.0 fixed, seamonkey2.26 fixed)

Details

(Whiteboard: [adv-main30+][adv-esr24.6+], crash signature)

Attachments

(2 attachments)

Reporter

Description

5 years ago
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();\
" + " })();");
Reporter

Comment 1

5 years ago
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
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
Depends on: 972710
Forgot to add that errant branch appears to be a patched branch.
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"?
Keywords: sec-critical
Adding Marty to answer comment 4.
Flags: needinfo?(mrosenberg)
Group: javascript-core-security
Marty, ping? Still curious about Dan's paranoia (comment 4)...
Assigning to Marty, we need to fix this now before the new assembler buffer is ready.
Assignee: nobody → mrosenberg
Status: NEW → ASSIGNED
Assignee

Comment 8

5 years ago
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.
Attachment #8425755 - Flags: review?(dtc-moz)
Flags: needinfo?(mrosenberg)
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.
Attachment #8425755 - Flags: review?(dtc-moz) → review+
Assignee

Comment 10

5 years ago
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.
Attachment #8425755 - Flags: sec-approval?
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.
Attachment #8425755 - Flags: sec-approval? → sec-approval+
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.
Flags: needinfo?(mrosenberg)
Assignee

Comment 13

5 years ago
landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/9d8a5c8d8328 I'll request uplift approval this evening.
https://hg.mozilla.org/mozilla-central/rev/9d8a5c8d8328
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Assignee

Comment 15

5 years ago
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
Attachment #8425755 - Flags: approval-mozilla-release?
Attachment #8425755 - Flags: approval-mozilla-beta?
Attachment #8425755 - Flags: approval-mozilla-aurora?
Assignee

Comment 16

5 years ago
[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.
Attachment #8429880 - Flags: approval-mozilla-esr24?
Flags: needinfo?(mrosenberg)
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.
Attachment #8429880 - Flags: approval-mozilla-esr24? → approval-mozilla-esr24+
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.
Attachment #8425755 - Flags: approval-mozilla-release?
Attachment #8425755 - Flags: approval-mozilla-release-
Attachment #8425755 - Flags: approval-mozilla-beta?
Attachment #8425755 - Flags: approval-mozilla-beta+
Attachment #8425755 - Flags: approval-mozilla-aurora?
Attachment #8425755 - Flags: approval-mozilla-aurora+
Whiteboard: [adv-main30+][adv-esr24.6+]
Group: javascript-core-security
Hi decoder - can you verify fixed in Fx30 and/or Fx24esr? Thanks!
Flags: needinfo?(choller)
Reporter

Comment 24

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

Updated

5 years ago
Status: RESOLVED → VERIFIED
Reporter

Comment 25

5 years ago
JSBugMon: This bug has been automatically verified fixed.
Reporter

Comment 26

5 years ago
Verified manually on Fx30 and Fx31, Fx24esr can't be verified as I mentioned before.
Flags: needinfo?(choller)
Reporter

Comment 28

5 years ago
JSBugMon: This bug has been automatically verified fixed on Fx32
Duplicate of this bug: 988791
Group: core-security
You need to log in before you can comment on or make changes to this bug.