Closed
Bug 584158
Opened 14 years ago
Closed 14 years ago
ensure that typed arrays cannot produce non-canonical nans
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | beta3+ |
People
(Reporter: luke, Assigned: luke)
Details
(Whiteboard: [sg:critical][critsmash:investigating] fixed-in-tracemonkey)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
gal
:
review+
|
Details | Diff | Splinter Review |
Typed arrays let users construct doubles from bits by punning between int8[]s and doubles. With fatvals, we use non-canonical nans to encode all the other types a jsval may contain. Thus, we must guard, whenever reading a double from a typed array, that, if it holds a nan, it holds a canonical nan. Otherwise, things will go poorly.
Talking to vlad, it seems there are there are at least two access paths we need to protect: reading a double slot on trace and reading a double slot through the typed array's getter.
Assignee | ||
Comment 1•14 years ago
|
||
This fix should also block fatvals release into a beta.
Comment 2•14 years ago
|
||
Typed arrays were meant to solve a problem mapping C structs and arrays of structs (arrays of structs containing array members, etc.) into JS, but the aliasing ability was not meant to be used to pun storage. Argh!
/be
Comment 3•14 years ago
|
||
Is this a security bug? Can we forge values through this path? If not, please re-open.
Group: core-security
Updated•14 years ago
|
Whiteboard: [sg:investigate?]
Sure can, you can write whatever bits you want in and then read it as a double. This wasn't a (big) issue before, but isn't great now. Luke and I think that just adding some canonicalization on read wouldn't be too expensive.
Updated•14 years ago
|
blocking2.0: --- → ?
Whiteboard: [sg:investigate?] → [sg:critical]
Updated•14 years ago
|
blocking2.0: ? → beta3+
Comment 5•14 years ago
|
||
let's try to get this fixed ASAP, and get it in the beta
Updated•14 years ago
|
Whiteboard: [sg:critical] → [sg:critical][critsmash:investigating]
Assignee | ||
Comment 6•14 years ago
|
||
Here's the basic fix (which requires LIR_cmovd in bug 584214). Also need to write some tests that exercise this path.
Assignee | ||
Comment 7•14 years ago
|
||
While I'm waiting on cmovd, this version uses a silly insAlloc'd phi node. The attached test is confirmed to crash without the changes and is fixed with the changes. The question is whether I have caught all the ways that a punned double can be read from a typed array. vlad could you double check me and just ignore TraceRecorder::canonicalizeNaNs?
Attachment #462547 -
Attachment is obsolete: true
Attachment #462629 -
Flags: review?(vladimir)
Yeah, pretty sure you got them all -- but please please don't check this in without cmovd, it will likely be brutal for performance. I'll get some benchmarks up to see what happens.
Assignee | ||
Comment 9•14 years ago
|
||
cmovd just went in, so I'm about to post a patch with that.
Assignee | ||
Comment 10•14 years ago
|
||
Attachment #462667 -
Flags: review?(gal)
Assignee | ||
Updated•14 years ago
|
Attachment #462629 -
Attachment is obsolete: true
Attachment #462629 -
Flags: review?(vladimir)
Comment 11•14 years ago
|
||
Comment on attachment 462667 [details] [diff] [review]
fix with cmovd
Always set cmov flag to true for lircmovd. Please test in fpu mode.
Attachment #462667 -
Flags: review?(gal) → review+
Assignee | ||
Comment 12•14 years ago
|
||
Whiteboard: [sg:critical][critsmash:investigating] → [sg:critical][critsmash:investigating] fixed-in-tracemonkey
Comment 13•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Updated•13 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•