Assertion failure: nbytes > 0, at js/src/gc/Nursery.cpp:357

VERIFIED FIXED in Firefox 51

Status

()

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

People

(Reporter: gkw, Assigned: sandervv)

Tracking

(Blocks 1 bug, 4 keywords)

Trunk
mozilla52
x86_64
macOS
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox48 unaffected, firefox49 unaffected, firefox-esr45 unaffected, firefox50 unaffected, firefox51+ verified, firefox52+ verified)

Details

(Whiteboard: [jsbugmon:update])

Attachments

(2 attachments, 3 obsolete attachments)

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

function f(x) {
    new Int32Array(x);
}
f(0);
f(2147483647);


Backtrace:

0   js-dbg-64-dm-clang-darwin-97a52326b06a	0x0000000110610220 js::Nursery::allocateBuffer(JSObject*, unsigned int) + 256 (Nursery.cpp:357)
1   js-dbg-64-dm-clang-darwin-97a52326b06a	0x000000010fef08f7 AllocateObjectBufferWithInit(JSContext*, js::TypedArrayObject*, int) + 359 (MacroAssembler.cpp:1063)
2   ???                           	0x0000000112163ebb 0 + 4598415035
3   js-dbg-64-dm-clang-darwin-97a52326b06a	0x000000010fda76ab js::jit::IonCannon(JSContext*, js::RunState&) + 715 (Ion.cpp:2822)
4   js-dbg-64-dm-clang-darwin-97a52326b06a	0x000000011030fecf js::RunScript(JSContext*, js::RunState&) + 351 (Interpreter.cpp:379)
5   js-dbg-64-dm-clang-darwin-97a52326b06a	0x0000000110321a3e js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) + 750 (Interpreter.cpp:471)
6   js-dbg-64-dm-clang-darwin-97a52326b06a	0x000000010fcd0cde js::jit::DoCallFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICCall_Fallback*, unsigned int, JS::Value*, JS::MutableHandle<JS::Value>) + 1918 (BaselineIC.cpp:5989)
/snip

For detailed crash information, see attachment.

Setting s-s because gc stuff is on the stack, and this seems to involve TypedArrays.
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/810a43be36de
user:        Sander Mathijs van Veen
date:        Fri Aug 12 21:38:45 2016 -0400
summary:     Bug 1293258 - AddressSanitizer: attempting double-free on 0x614000342640 [@ __interceptor_free] or Crash [@ jemalloc_crash] with TypedArray. r=jandem

Sander, is bug 1293258 a likely regressor?
Blocks: 1293258
Flags: needinfo?(sandervv)
Prevent an overflow caused by the JS_ROUNDUP since |allocateBuffer| converts |nbytes| of type size_t to uint32_t. The value for |nbytes| will truncate to zero when |new Int32Array(2147483647)| is used.
Assignee: nobody → sandervv
Flags: needinfo?(sandervv)
Attachment #8783469 - Flags: review?(jdemooij)
Comment on attachment 8783469 [details] [diff] [review]
bug1296249-nbytes-overflow.patch

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

::: js/src/jit/MacroAssembler.cpp
@@ +1059,5 @@
>  
> +    // Prevent an overflow caused by the JS_ROUNDUP since |allocateBuffer|
> +    // converts |nbytes| of type size_t to uint32_t. The value for |nbytes| will
> +    // truncate to zero when |new Int32Array(2147483647)| is used.
> +    if (nbytes > TypedArrayObject::SINGLETON_BYTE_LENGTH)

Can you make this >= to be consistent with other places where we check this?
Attachment #8783469 - Flags: review?(jdemooij) → review+
Used ">=" for consistency.
Attachment #8783469 - Attachment is obsolete: true
Keywords: checkin-needed
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/afd6ad990dd4 for the test failing like https://treeherder.mozilla.org/logviewer.html#?job_id=35787106&repo=mozilla-inbound in, so far, the ARM shell build we run on Linux32, the ARM shell build we run really weirdly, displaying on the Linux64 row but being the SM(arm) right next to the SM(arm64) so I bet it's actually 32-bit, and the SM(p) on the WinXP row (which is a lie, it's Server 2008, but probably means 32-bit as well).
And 64-bit didn't save you from everything, in the browser jittests you got OOMs like https://treeherder.mozilla.org/logviewer.html#?job_id=35790646&repo=mozilla-inbound
Sander, your patch got backed out, any chance of working it through? (Else we'll just pass the ni? along)
Flags: needinfo?(sandervv)
Flags: needinfo?(jdemooij)
I've verified locally that the OOM exception doesn't fail the test case when running the test in a shell with |ulimit -m 4096000; ulimit -v 4096000| set. The try job results are pending (https://treeherder.mozilla.org/#/jobs?repo=try&revision=683968dd3ed3).

I'm using |./mach try -b do -p linux,linux64,linux64-asan,linux64-st-an,linux64-valgrind,linux64-shell-haz -u jsreftest,jittests,jittest-1,jittest-2,mochitests,mochitests-bc -t none| for the try build. Is this sufficient?
Attachment #8790735 - Attachment is obsolete: true
Flags: needinfo?(sandervv)
Attachment #8794776 - Flags: review?(jdemooij)
Comment on attachment 8794776 [details] [diff] [review]
bug1296249-nbytes-overflow.patch

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

I see you still got some jit-test failures on Try, let me know if you need help with that.

::: js/src/jit/MacroAssembler.cpp
@@ +1063,5 @@
>      }
>  
> +    // Prevent an overflow caused by the JS_ROUNDUP since |allocateBuffer|
> +    // converts |nbytes| of type size_t to uint32_t. The value for |nbytes| will
> +    // truncate to zero when |new Int32Array(2147483647)| is used.

Please change allocateBuffer to just take size_t instead of uint32_t, to fix this footgun. If we do that, can we remove this change?
Attachment #8794776 - Flags: review?(jdemooij)
[Tracking Requested - why for this release]: Sec bug.
Flags: needinfo?(jdemooij)
I fixed the comments in the updated patch.

Steps to reproduce the test failure for arm-sim running on x86-64:

$ cd js/src/;
$ ./devtools/automation/autospider.py --build-only --dep arm-sim;
$ python jit-test/jit_test.py --jitflags=all -j8 ../../obj-spider/js/src/js basic/bug1296249.js
Assertion failure: cx->isExceptionPending() (Thunk execution failed but no exception was raised - missing call to js::ReportOutOfMemory()?), at /home/smvv/work/mozilla-central/js/src/builtin/TestingFunctions.cpp:1373
Exit code: -11
FAIL - basic/bug1296249.js

I'm starting a new try job to make sure that the uint32_t -> size_t change doesn't break any builds. Results will be listed here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=efbca55c4921
Attachment #8794776 - Attachment is obsolete: true
Attachment #8794828 - Flags: review?(jdemooij)
Comment on attachment 8794828 [details] [diff] [review]
bug1296249-nbytes-overflow.patch

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

Thanks. Don't forget to request sec-approval before landing, as this affects Aurora and is probably exploitable.
Attachment #8794828 - Flags: review?(jdemooij) → review+
The assertion fails on arm 32-bit (using the arm-sim running on a host x86-64 bit).
Flags: needinfo?(jdemooij)
Depends on: 1305948
(In reply to Sander Mathijs van Veen from comment #15)
> The assertion fails on arm 32-bit (using the arm-sim running on a host
> x86-64 bit).

I filed bug 1305948 with a patch for this OOM bug.
Flags: needinfo?(jdemooij)
Comment on attachment 8794828 [details] [diff] [review]
bug1296249-nbytes-overflow.patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
I think it would be easy since the test case that illustrates the assertion failure is easy to construct.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
The test gives a clear hint about the security problem.

Which older supported branches are affected by this flaw?
The patch landed in Firefox 51.

If not all supported branches, which bug introduced the flaw?
https://bugzilla.mozilla.org/show_bug.cgi?id=1279992

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
I do not have backports, but the patch should be easy to backport to the affected branches. They are not risky and not hard to create.

How likely is this patch to cause regressions; how much testing does it need?
It is not likely to cause regressions.
Attachment #8794828 - Flags: sec-approval?
Tracking 52+ for this assertion failure.
Keywords: sec-high
sec-approval+ for trunk.
We'll want an aurora patch made and nominated for this as well so we don't ship the bug.
Attachment #8794828 - Flags: sec-approval? → sec-approval+
Comment on attachment 8794828 [details] [diff] [review]
bug1296249-nbytes-overflow.patch

Approval Request Comment
[Feature/regressing bug #]: 1296249
[User impact if declined]: it could allocate zero bytes in the nursery for the typed array elements buffer. Writing to that typed array's buffer is dangerous.
[Describe test coverage new/current, TreeHerder]: The added test verifies that an OOM error is thrown instead of an assertion failure.
[Risks and why]: Not sure what risks this could have. The size parameter is changed from uint32_t to size_t so it would only affect platforms where size_t > uint32_t.
[String/UUID change made/needed]:
Attachment #8794828 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/bb162dfbe7f2
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Depends on: 1308647
Group: javascript-core-security → core-security-release
Comment on attachment 8794828 [details] [diff] [review]
bug1296249-nbytes-overflow.patch

Sec-high. Take it in 51 aurora.
Attachment #8794828 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
JSBugMon: This bug has been automatically verified fixed on Fx51
Group: core-security-release
Keywords: regression
You need to log in before you can comment on or make changes to this bug.