IonMonkey: Crash [@ js::GetObjectClass] with invalid read

VERIFIED FIXED in Firefox 20

Status

()

defect
--
critical
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: decoder, Assigned: dvander)

Tracking

(Blocks 1 bug, 4 keywords)

Trunk
mozilla22
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox19 wontfix, firefox20+ fixed, firefox21+ fixed, firefox22+ fixed, firefox-esr17 unaffected, b2g18 unaffected)

Details

(Whiteboard: [jsbugmon:update][adv-main20+], crash signature)

Attachments

(1 attachment)

Reporter

Description

6 years ago
The following testcase crashes on mozilla-central revision 0acbd06d48a9 (run with --ion-eager):


evaluate("\
  var x = function g() {\
    return g.toString();\
  };\
  x('y')\
", { noScriptRval : true });
evaluate("\
  x('y')\
", { noScriptRval : true });
Reporter

Comment 1

6 years ago
Debug trace:

Program received signal SIGSEGV, Segmentation fault.
0x00000000004050bb in js::GetObjectClass (obj=(js::RawObject) 0x7ffff602d381 Cannot access memory at address 0x5000007ffff603b6) at ../../jsfriendapi.h:366
366         return reinterpret_cast<const shadow::Object*>(obj)->type->clasp;
(gdb) bt 8
#0  0x00000000004050bb in js::GetObjectClass (obj=(js::RawObject) 0x7ffff602d381 Cannot access memory at address 0x5000007ffff603b6) at ../../jsfriendapi.h:366
#1  0x0000000000405337 in js::IsProxy (obj=(js::RawObject) 0x7ffff602d381 Cannot access memory at address 0x5000007ffff603b6) at ../../jsproxy.h:239
#2  0x0000000000409c4e in JSObject::isProxy (this=(const JSObject * const) 0x7ffff602d381 Cannot access memory at address 0x5000007ffff603b6) at ../../jsobjinlines.h:1296
#3  0x0000000000c69b7f in IsCacheableListBase (obj=(JSObject *) 0x7ffff602d381 Cannot access memory at address 0x5000007ffff603b6) at /srv/repos/mozilla-central/js/src/ion/IonCaches.cpp:89
#4  0x0000000000c6f029 in TryAttachNativeGetPropStub (cx=0x13bae90, ion=0x1449e10, cache=..., obj=(JSObject * const) 0x7ffff602d381 Cannot access memory at address 0x5000007ffff603b6, name="toString", safepointIndex=
    0x1449fd0, returnAddr=0x7ffff7fed873, isCacheable=0x7fffffff510f) at /srv/repos/mozilla-central/js/src/ion/IonCaches.cpp:855
#5  0x0000000000c6fbf7 in js::ion::GetPropertyCache (cx=0x13bae90, cacheIndex=0, obj=(JSObject * const) 0x7ffff602d381 Cannot access memory at address 0x5000007ffff603b6, vp=JSVAL_VOID)
    at /srv/repos/mozilla-central/js/src/ion/IonCaches.cpp:964
#6  0x00007ffff7fe9e84 in ?? ()
#7  0x00007fffffff51b0 in ?? ()
(More stack frames follow...)
(gdb) x /i $pc
=> 0x4050bb <js::GetObjectClass(JSObject*)+16>: mov    (%rax),%rax
(gdb) info reg rax
rax            0x13d76  81270
(gdb) l
363     inline js::Class *
364     GetObjectClass(RawObject obj)
365     {
366         return reinterpret_cast<const shadow::Object*>(obj)->type->clasp;
367     }


Corrupted obj pointer, marking sec-critical.
Blocks: IonFuzz
Keywords: sec-critical
Summary: Crash [@ js::GetObjectClass] with invalid read → IonMonkey: Crash [@ js::GetObjectClass] with invalid read
Whiteboard: [jsbugmon:update,bisect]
Reporter

Updated

6 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Reporter

Comment 2

6 years ago
JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   121830:507316db9c59
user:        David Anderson
date:        Wed Feb 13 17:24:50 2013 -0800
summary:     Implement JSOP_CALLEE in JM (bug 794427, r=bhackett).

This iteration took 123.461 seconds to run.
Reporter

Comment 3

6 years ago
Needinfo from dvander based on comment 2 :)
Flags: needinfo?(dvander)
Reporter

Comment 4

6 years ago
Simpler test for the testsuite:

var x = function g() {
  return g.toString();
};
for ( var i = 0; i < 3; i++ ) 
  x(i)
Assignee: general → dvander
Status: NEW → ASSIGNED
Flags: needinfo?(dvander)
This was actually a pre-existing bug in IonMonkey, exposed much easier now that JM can compile JSOP_CALLEE.
No longer blocks: 794427
Posted patch fixSplinter Review
Easy fix, when inlining make sure we read the innermost callee instead of the outermost.
Attachment #718684 - Flags: review?(nicolas.b.pierron)
Updating tracking flags based on comment 5 (that this is a preexisting Ionmonkey bug, so 17 and b2g are unaffected).
Would be nice to land in Beta too if the patch is low risk.
Comment on attachment 718684 [details] [diff] [review]
fix

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

sounds good to me.
Attachment #718684 - Flags: review?(nicolas.b.pierron) → review+
Comment on attachment 718684 [details] [diff] [review]
fix

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

Not easily.

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?

19 through 22.

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

This patch should apply.

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

Unlikely; tbpl+awfy is fine.
Attachment #718684 - Flags: sec-approval?
Attachment #718684 - Flags: sec-approval? → sec-approval+
This needs to land and then get nominated for uplift before Tues Mar 12th when we go to build on beta 5.
This doesn't apply cleanly to inbound. Please rebase.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4db85af71a2c
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Reporter

Updated

6 years ago
Status: RESOLVED → VERIFIED
Reporter

Comment 15

6 years ago
JSBugMon: This bug has been automatically verified fixed.
Comment on attachment 718684 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): IonMonkey
User impact if declined: potential sg:crit
Testing completed (on m-c, etc.): Yes
Risk to taking this patch (and alternatives if risky): None
String or UUID changes made by this patch: None
Attachment #718684 - Flags: approval-mozilla-beta?
Attachment #718684 - Flags: approval-mozilla-aurora?
Attachment #718684 - Flags: approval-mozilla-beta?
Attachment #718684 - Flags: approval-mozilla-beta+
Attachment #718684 - Flags: approval-mozilla-aurora?
Attachment #718684 - Flags: approval-mozilla-aurora+
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main20+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.