Closed Bug 584158 Opened 12 years ago Closed 12 years ago

ensure that typed arrays cannot produce non-canonical nans

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

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)

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.
This fix should also block fatvals release into a beta.
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
Is this a security bug? Can we forge values through this path? If not, please re-open.
Group: core-security
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.
blocking2.0: --- → ?
Whiteboard: [sg:investigate?] → [sg:critical]
blocking2.0: ? → beta3+
let's try to get this fixed ASAP, and get it in the beta
Whiteboard: [sg:critical] → [sg:critical][critsmash:investigating]
Attached patch patch (obsolete) — Splinter Review
Here's the basic fix (which requires LIR_cmovd in bug 584214).  Also need to write some tests that exercise this path.
Attached patch fix sans cmovd (obsolete) — Splinter Review
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.
cmovd just went in, so I'm about to post a patch with that.
Attached patch fix with cmovdSplinter Review
Attachment #462667 - Flags: review?(gal)
Attachment #462629 - Attachment is obsolete: true
Attachment #462629 - Flags: review?(vladimir)
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+
http://hg.mozilla.org/tracemonkey/rev/e866db9165ef
Whiteboard: [sg:critical][critsmash:investigating] → [sg:critical][critsmash:investigating] fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/e866db9165ef
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Group: core-security
You need to log in before you can comment on or make changes to this bug.