Assertion failure: cloneLength % BYTES_PER_ELEMENT == 0, at js/src/vm/TypedArrayObject.cpp:850

VERIFIED FIXED in Firefox 48

Status

()

defect
--
critical
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: gkw, Assigned: arai)

Tracking

(Blocks 1 bug, {assertion, testcase})

Trunk
mozilla48
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 unaffected, firefox48 verified)

Details

(Whiteboard: [jsbugmon:update])

Attachments

(2 attachments)

Reporter

Description

3 years ago
The following testcase crashes on mozilla-central revision 29d5a4175c8b (build with --enable-debug --enable-more-deterministic, run with --fuzzing-safe --no-threads --no-baseline --no-ion):

x = new ArrayBuffer(38);
y = new Float64Array(x, 24, x);
new Float64Array(y);

Backtrace:

0   js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x00000001008f8761 (anonymous namespace)::TypedArrayObjectTemplate<double>::fromTypedArray(JSContext*, JS::Handle<JSObject*>, bool, JS::Handle<JSObject*>) + 2337 (TypedArrayObject.cpp:850)
1   js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x00000001008df472 (anonymous namespace)::TypedArrayObjectTemplate<double>::class_constructor(JSContext*, unsigned int, JS::Value*) + 690 (TypedArrayObject.cpp:482)
2   js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x00000001007d617e js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 222 (jscntxtinlines.h:236)
3   js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x00000001007e03c6 js::CallJSNativeConstructor(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) + 118 (jscntxtinlines.h:268)
4   js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x00000001007c74d7 InternalConstruct(JSContext*, js::AnyConstructArgs const&) + 487 (Interpreter.cpp:553)
5   js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x00000001007c70f7 js::ConstructFromStack(JSContext*, JS::CallArgs const&) + 87 (Interpreter.cpp:592)
6   js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x00000001007bbdc4 Interpret(JSContext*, js::RunState&) + 48212 (Interpreter.cpp:2799)
7   js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x00000001007b00a7 js::RunScript(JSContext*, js::RunState&) + 519 (Interpreter.cpp:426)
8   js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x00000001007c8044 js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) + 1124 (Interpreter.cpp:682)
9   js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x00000001007c83c5 js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) + 469 (RootingAPI.h:667)
10  js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x0000000100595241 ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::Value*) + 417 (jsapi.cpp:4372)
11  js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x00000001005954b2 JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>) + 82 (RootingAPI.h:667)
12  js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x0000000100020aa9 Process(JSContext*, char const*, bool, FileKind) + 3609 (js.cpp:530)
13  js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x000000010000610b main + 11739 (js.cpp:6732)
14  js-dbg-64-dm-clang-darwin-29d5a4175c8b	0x0000000100001374 start + 52

Setting s-s because this involves TypedArrays.
Reporter

Comment 1

3 years ago
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/1a92f3cced1b
user:        Tooru Fujisawa
date:        Sat Mar 05 18:57:51 2016 +0900
summary:     Bug 1165053 - Part 7: Call SpeciesConstructor in TypedArray ctors. r=lth

arai-san, is bug 1165053 a likely regressor? (This landed on April 7, 2016 fwiw)
Blocks: 1165053
Flags: needinfo?(arai.unmht)
Assignee

Comment 2

3 years ago
Looks like, the issue is that we always allocate ArrayBuffer with |BYTES_PER_ELEMENT * typedArray.length|, sometimes lazily,
but in CloneArrayBuffer, it should allocate ArrayBuffer that doesn't match to |BYTES_PER_ELEMENT * n| length (in the example's case, it should allocate 14 bytes buffer)

Also, we cannot allocate ArrayBuffer lazily when the length of the ArrayBuffer is not |BYTES_PER_ELEMENT * typedArray.length|.

In the example's case, typedArray.length is 0, but the ArrayBuffer's byteLength should be 14.
Flags: needinfo?(arai.unmht)
Assignee

Comment 3

3 years ago
Sorry, I always forgot to say, yes, it's a regression from bug 1165053.

Then, the issue related to the assertion could be fixed by passing byteLength instead of elementLength to AllocateArrayBuffer,
but it won't fix the issue related to lazily allocated ArrayBuffer.  maybe we should fix it in separated bug tho.
Assignee

Comment 4

3 years ago
Changing AllocateArrayBuffer and maybeCreateArrayBuffer to receive byteLength fixes the issue related to byte length that is not |BYTES_PER_ELEMENT*n|.

Then in the testcase |a2.buffer.byteLength| becomes 0 because the ArrayBuffer gets created lazily with |a2.byteLength| that is 0.
The issue doesn't happen when I use ArrayBuffer subclass, so I added subclassed case as a testcase for this bug.

Is there any bug filed for lazily allocated ArrayBuffer's length?
Assignee: nobody → arai.unmht
Attachment #8740260 - Flags: review?(lhansen)
Triage is looking at this from a security point of view. What are the implications of this? A 14 byte write?

We're trying to come up with a security rating for this issue.
Flags: needinfo?(arai.unmht)
Assignee

Comment 6

3 years ago
With this bug, ArrayBuffer object can be created with less capacity than requested length, if the passed buffer is a subclass of ArrayBuffer.  This has no effect when the ArrayBuffer is not subclass, as the wrong value calculated there is discarded

>     MOZ_ASSERT(cloneLength % BYTES_PER_ELEMENT == 0);
>     if (!AllocateArrayBuffer(cx, cloneCtor, cloneLength / BYTES_PER_ELEMENT, buffer))
>         return false;

>         if (!nonDefaultProto && nelements <= INLINE_BUFFER_LIMIT / sizeof(NativeType)) {
>             // The array's data can be inline, and the buffer created lazily.
>             return true;
>         }
> ...
>         ArrayBufferObject* buf = ArrayBufferObject::create(cx, nelements * sizeof(NativeType),
>                                                            nonDefaultProto);
>         if (!buf)
>             return false;

That area cannot be accessed from the TypedArray instance, so I cannot think of any way to exploit this.


For example, in a similar code to the comment #0:

  x = new ArrayBuffer(38);
  y = new Float64Array(x, 24, 2);
  z = new Float64Array(y);

|y| is a 2 element array, and can see following range:

                       y.byteOffset     y.byteOffset+y.byteLength
                       |                |
                       v                v
  x: 01234567 89012345 67890123 45678901 234567

|z| is also a 2 element array, and can see following range:

            z.byteOffset     z.byteOffset+z.byteLength
            |                |
            v                v
  z.buffer: 67890123 45678901 234567
                              ~~~~~~

The trailing "234567" there is the area that is not created because of this bug.
The area cannot be accessed, as it needs to create |y| with 3 elements, but it's out of bound and throws TypeError.

>         if (arrayByteLength + byteOffset > buffer->byteLength()) {
>             JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_TYPED_ARRAY_BAD_ARGS);
>             return nullptr; // byteOffset + len is too big for the arraybuffer
>         }

So, I think this matches to sec-other (Bugs that may not be exploitable security issues).
Flags: needinfo?(arai.unmht)
Less mysterious test case that provokes the same crash:

x = new ArrayBuffer(38);
y = new Float64Array(x, 24, 0);
z = new Float64Array(y);
Comment on attachment 8740260 [details] [diff] [review]
Change AllocateArrayBuffer to receive byteLength instead of nelements.

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

::: js/src/tests/ecma_6/ArrayBuffer/indivisible-byteLength.js
@@ +11,5 @@
> +assertEq(a1.byteLength, 0);
> +assertEq(a1.byteOffset, 24);
> +
> +var a2 = new Float64Array(a1);
> +// FIXME bug XXX: ArrayBuffer is created lazily with wrong byteLength.

Would be good to replace XXX by an actual bug number here.  Unless that bug is more exploitable than this one.
Attachment #8740260 - Flags: review?(lhansen) → review+
Assignee

Comment 9

3 years ago
Thank you for reviewing :D

(In reply to Lars T Hansen [:lth] from comment #8)
> > +// FIXME bug XXX: ArrayBuffer is created lazily with wrong byteLength.
> 
> Would be good to replace XXX by an actual bug number here.  Unless that bug
> is more exploitable than this one.

Sorry, I forgot to note about that.
Do you know any existing bug related to that behavior?
otherwise I'll file it.
Flags: needinfo?(lhansen)
Not aware of a bug regarding that.
Flags: needinfo?(lhansen)
Assignee

Comment 11

3 years ago
Thanks!

Filed bug 1264941, and it turned out that current behavior is better.
So I'll change the comment in the testcase, and enable the assertion for the buffer.byteLength==0,
so that the behavior change in bug 1264941 will hit either assertion.
Assignee

Comment 12

3 years ago
Sorry, it hits following error:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=10e313995e17&selectedJob=19539976
> 740 INFO TEST-UNEXPECTED-FAIL | dom/canvas/test/webgl-conformance/_wrappers/test_conformance__typedarrays__array-unit-tests.html | Construction of huge Int16Array should throw exception

it's because of uint32_t overflow.
I should check the range *before* multiply BYTES_PER_ELEMENT.

will fix the patch.
Assignee

Comment 13

3 years ago
Moved the INT32_MAX check before `* BYTES_PER_ELEMENT`.
Attachment #8741810 - Flags: review?(lhansen)
Comment on attachment 8741810 [details] [diff] [review]
followup - Check byteLength before multiplication.

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

OK, these are clearly correct in the sense that they check invariants that must hold.  I can't quite tell if they're redundant but I'll trust you that they are not, and in any case, it does not matter if they are.
Attachment #8741810 - Flags: review?(lhansen) → review+
Assignee

Comment 15

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a338aa9ac43ff12190109267d2210d230db9286e
Bug 1263803 - Change AllocateArrayBuffer to receive byteLength instead of nelements. r=lth
https://hg.mozilla.org/mozilla-central/rev/a338aa9ac43f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48

Updated

3 years ago
Status: RESOLVED → VERIFIED

Comment 17

3 years ago
JSBugMon: This bug has been automatically verified fixed.
Group: javascript-core-security → core-security-release
Reporter

Updated

3 years ago
Depends on: 1268740
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.