Closed Bug 1268740 Opened 4 years ago Closed 4 years ago

Crash [@ js::TypedArrayMethods]

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla49
Tracking Status
firefox47 --- unaffected
firefox48 + verified
firefox49 + verified
firefox-esr45 --- unaffected

People

(Reporter: gkw, Assigned: arai)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:update])

Crash Data

Attachments

(2 files)

The following testcase crashes on mozilla-central revision 4292da9df16b (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --ion-eager):

new Int32Array(new Int8Array(~2877108499));

Backtrace:

0   js-dbg-64-dm-clang-darwin-4292da9df16b	0x0000000100939fe3 js::TypedArrayMethods<js::TypedArrayObject>::setFromTypedArray(JSContext*, JS::Handle<js::TypedArrayObject*>, JS::Handle<JSObject*>, unsigned int) + 90979 (TypedArrayCommon.h:232)
1   js-dbg-64-dm-clang-darwin-4292da9df16b	0x0000000100905113 (anonymous namespace)::TypedArrayObjectTemplate<int>::fromTypedArray(JSContext*, JS::Handle<JSObject*>, bool, JS::Handle<JSObject*>) + 1491 (TypedArrayObject.cpp:972)
2   js-dbg-64-dm-clang-darwin-4292da9df16b	0x00000001008edc62 (anonymous namespace)::TypedArrayObjectTemplate<int>::class_constructor(JSContext*, unsigned int, JS::Value*) + 690 (TypedArrayObject.cpp:482)
3   js-dbg-64-dm-clang-darwin-4292da9df16b	0x00000001007e568e js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 222 (jscntxtinlines.h:236)
4   js-dbg-64-dm-clang-darwin-4292da9df16b	0x00000001007ef876 js::CallJSNativeConstructor(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 118 (jscntxtinlines.h:268)
5   js-dbg-64-dm-clang-darwin-4292da9df16b	0x00000001007d6640 InternalConstruct(JSContext*, js::AnyConstructArgs const&) + 496 (Interpreter.cpp:569)
6   js-dbg-64-dm-clang-darwin-4292da9df16b	0x00000001007d6257 js::ConstructFromStack(JSContext*, JS::CallArgs const&) + 87 (Interpreter.cpp:608)
7   js-dbg-64-dm-clang-darwin-4292da9df16b	0x00000001001f20da js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) + 1482 (BaselineIC.cpp:5948)
8   ???                           	0x0000000102707dab 0 + 4335893931
9   ???                           	0x0000000102844b70 0 + 4337191792

For detailed crash information, see attachment.

TypedArrays are involved -> s-s by default.
=== Treeherder Build Bisection Results by autoBisect ===

The "good" changeset has the timestamp "20160415094550" and the hash "2f5f51545d0e466479ec6fdf8b89af72685dfc6d".
The "bad" changeset has the timestamp "20160415095344" and the hash "a338aa9ac43ff12190109267d2210d230db9286e".

Likely regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=2f5f51545d0e466479ec6fdf8b89af72685dfc6d&tochange=a338aa9ac43ff12190109267d2210d230db9286e

Arai-san, is bug 1263803 a likely regressor?
Blocks: 1263803
Flags: needinfo?(arai.unmht)
m-i rev 83b0db55bdc9 still seems to reproduce this issue.
Thanks, yes, it's from bug 1263803.

I should've passed byteLength separately with `count * unit` format, both to AllocateArrayBuffer and maybeCreateArrayBuffer.
It allows us to merge the code throwing JSMSG_NEED_DIET into single place in maybeCreateArrayBuffer.

in most case, `count=elementLength` and `unit=BYTES_PER_ELEMENT`, but in `CloneArrayBufferNoCopy`, `count=byteLength` and `unit=1`, as we're using `srcArray->byteLength()` value for the allocation.
Assignee: nobody → arai.unmht
Flags: needinfo?(arai.unmht)
Attachment #8746938 - Flags: review?(lhansen)
Attachment #8746938 - Flags: review?(lhansen) → review+
Comment on attachment 8746938 [details] [diff] [review]
Change AllocateArrayBuffer to receive byteLength with |count * unit| format.

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
This is buffer over flow, and it could be exploited based on the testcase. (I'm not sure how tho)

> Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The testcase describes how to hit the buffer over flow.

> Which older supported branches are affected by this flaw?
mozilla-aurora (mozilla48)

> If not all supported branches, which bug introduced the flaw?
bug 1263803

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Same patch is applicable to mozilla-aurora.

> How likely is this patch to cause regressions; how much testing does it need?
Less likely.  it adds range check for one missing case.
Attachment #8746938 - Flags: sec-approval?
sec-approval+ for trunk. We should get a patch nominated for aurora after it is on trunk as well.
Attachment #8746938 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/integration/mozilla-inbound/rev/3dfd039e8de2b2caaa6eb6e3be302a51f5313995
Bug 1268740 - Change AllocateArrayBuffer to receive byteLength with |count * unit| format. r=lth
Comment on attachment 8746938 [details] [diff] [review]
Change AllocateArrayBuffer to receive byteLength with |count * unit| format.

Asking approval in advance to land it to aurora quickly.

same patch is applicable to mozilla-aurora.

Approval Request Comment
> [Feature/regressing bug #]
bug 1263803

> [User impact if declined]
Possible exploit with buffer overflow.

> [Describe test coverage new/current, TreeHerder]
Tested in mozilla-inbound automated test. (passed on 14 pushes at this point)

> [Risks and why]
Low.  It adds range check for one missing case.

> [String/UUID change made/needed]
None
Attachment #8746938 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/3dfd039e8de2
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Group: javascript-core-security → core-security-release
Comment on attachment 8746938 [details] [diff] [review]
Change AllocateArrayBuffer to receive byteLength with |count * unit| format.

Sec-critical, has approval, let's land this on aurora
Attachment #8746938 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
JSBugMon: This bug has been automatically verified fixed on Fx48
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.