Crash [@ ToPrimitive] or [@ js::ToNumberSlow] or Assertion failure: v.isObject(), at jsnum.cpp

VERIFIED FIXED in Firefox 23

Status

()

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

People

(Reporter: gkw, Assigned: bhackett)

Tracking

(Blocks 1 bug, 5 keywords)

Trunk
mozilla25
x86
All
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox22 unaffected, firefox23+ fixed, firefox24+ fixed, firefox25+ fixed, firefox-esr17 unaffected, b2g18 unaffected)

Details

(Whiteboard: [adv-main23-] [jsbugmon:update,ignore], crash signature)

Attachments

(2 attachments)

x = ArrayBuffer(16)
y = Float32Array(x)
Int32Array(x)[0] = -3
x.valueOf = (function() {
    y[0] / x
});
+ x

asserts js (deterministic threadsafe 32-bit) debug shell on m-c changeset 7b2c82ae98db with --ion-eager at Assertion failure: v.isObject(), at jsnum.cpp and crashes js opt shell at ToPrimitive with js::ToNumberSlow on the stack.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   http://hg.mozilla.org/mozilla-central/rev/0670cdaf7e9c
user:        Brian Hackett
date:        Thu Jul 11 15:08:26 2013 -0600
summary:     Bug 891400 - Improve pattern matching on static typed array accesses, r=jandem.

Setting s-s because weird memory address 0xa0000000 seems to be being accessed.
Flags: needinfo?(bhackett1024)
I could reproduce on a Mac 32-bit threadsafe deterministic js shell on m-c tip rev b717a7945dfb.
OS: Linux → All
Posted patch patchSplinter Review
This is actually a regression from bug 864214.  When reading from static float/double typed arrays no double canonicalization was occurring --- asm.js canonicalizes doubles when passing into or out of asm.js code, but normal js always needs to do this canonicalization.
Assignee: general → bhackett1024
Attachment #781872 - Flags: review?(luke)
Flags: needinfo?(bhackett1024)
Comment on attachment 781872 [details] [diff] [review]
patch

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

With some difficulty, though this would be possible given knowledge of the need for double canonicalization in the js engine.

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?

aurora/beta

If not all supported branches, which bug introduced the flaw?

bug 864214

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

trivial

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

none
Attachment #781872 - Flags: sec-approval?
Comment on attachment 781872 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 864214
User impact if declined: security hole
Testing completed (on m-c, etc.): not on branches yet
Risk to taking this patch (and alternatives if risky): none
Attachment #781872 - Flags: approval-mozilla-beta?
Attachment #781872 - Flags: approval-mozilla-aurora?
Attachment #781872 - Flags: review?(luke) → review+
Blocks: 864214
No longer blocks: 891400
Can you suggest a security rating here? Either way, I think we should take this. The patch is miniscule.
I think this is critical, NaN forging can lead to reads or writes at easily controlled addresses.
Keywords: sec-critical
Final FF23 beta going to build tomorrow so if you are comfortable with sec-approval here, please also approve to branches so this gets landed asap.
Comment on attachment 781872 [details] [diff] [review]
patch

sec-approval+.
Attachment #781872 - Flags: sec-approval? → sec-approval+
Comment on attachment 781872 [details] [diff] [review]
patch

I approved for branches as well. Let's get this in.
Attachment #781872 - Flags: approval-mozilla-beta?
Attachment #781872 - Flags: approval-mozilla-beta+
Attachment #781872 - Flags: approval-mozilla-aurora?
Attachment #781872 - Flags: approval-mozilla-aurora+
Whiteboard: [jsbugmon:update] → [jsbugmon:update][adv-main23-]
Crash Signature: [@ ToPrimitive] [@ js::ToNumberSlow] → [@ ToPrimitive] [@ js::ToNumberSlow]
Whiteboard: [jsbugmon:update][adv-main23-] → [adv-main23-] [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 129ce98f4cb2).
https://hg.mozilla.org/mozilla-central/rev/f0ce0463bd65
Status: NEW → RESOLVED
Crash Signature: [@ ToPrimitive] [@ js::ToNumberSlow] → [@ ToPrimitive] [@ js::ToNumberSlow]
Closed: 6 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Status: RESOLVED → VERIFIED
Crash Signature: [@ ToPrimitive] [@ js::ToNumberSlow] → [@ ToPrimitive] [@ js::ToNumberSlow]
JSBugMon: This bug has been automatically verified fixed.
Crash Signature: [@ ToPrimitive] [@ js::ToNumberSlow] → [@ ToPrimitive] [@ js::ToNumberSlow]
Group: core-security
You need to log in before you can comment on or make changes to this bug.