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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: debug and opt stacks

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.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

I could reproduce on a Mac 32-bit threadsafe deterministic js shell on m-c tip rev b717a7945dfb.

Comment 2

[Brian Hackett [Laid off!]]

Attachment: patch

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.

Comment 3

[Brian Hackett [Laid off!]]

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

Comment 4

[Brian Hackett [Laid off!]]

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

Comment 5

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

Can you suggest a security rating here? Either way, I think we should take this. The patch is miniscule.

Comment 6

[Brian Hackett [Laid off!]]

I think this is critical, NaN forging can lead to reads or writes at easily controlled addresses.

Comment 7

[Lukas Blakk [:lsblakk] use ?needinfo]

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 8

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

Comment on attachment 781872 [details] [diff] [review]
patch

sec-approval+.

Comment 9

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

Comment on attachment 781872 [details] [diff] [review]
patch

I approved for branches as well. Let's get this in.

Comment 10

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0ce0463bd65
https://hg.mozilla.org/releases/mozilla-aurora/rev/8682482c7e23
https://hg.mozilla.org/releases/mozilla-beta/rev/683623ead9ac

Comment 11

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 129ce98f4cb2).

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f0ce0463bd65

Comment 13

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.