1415291 - (CVE-2018-5093) Heap-buffer-overflow READ 8 · js::WasmTableObject::getImpl

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Alex Gaynor [:Alex_Gaynor]]

Attachment: clusterfuzz-testcase-6215596157960192.js

This bug was found by Google's OSS-Fuzz running their custom internal JS fuzzer. I am refiling it in our issue tracker.

Please note that they apply a 90-day disclose timeline to all bugs:

[Environment] ASAN_OPTIONS = redzone=256:strict_memcmp=0:allow_user_segv_handler=1:allocator_may_return_null=1:handle_sigfpe=1:handle_sigbus=1:detect_stack_use_after_return=0:alloc_dealloc_mismatch=0:print_scariness=1:max_uar_stack_size_log=16:detect_odr_violation=0:handle_sigill=1:coverage=0:use_sigaltstack=1:fast_unwind_on_fatal=1:detect_leaks=0:print_summary=1:handle_abort=1:check_malloc_usable_size=0:detect_container_overflow=1:symbolize=0:handle_segv=1
	
	=================================================================
	==13600==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x6110001b4c80 at pc 0x0000028d59ae bp 0x7ffdebc428b0 sp 0x7ffdebc428a8
	READ of size 8 at 0x6110001b4c80 thread T0
	SCARINESS: 23 (8-byte-read-heap-buffer-overflow)
	#0 0x28d59ad in js::WasmTableObject::getImpl(JSContext*, JS::CallArgs const&) mozilla-central/js/src/wasm/WasmJS.cpp:1719:15
	#1 0x28d5ea4 in bool JS::CallNonGenericMethod<&(IsTable(JS::Handle<JS::Value>)), &js::WasmTableObject::getImpl>(JSContext*, JS::CallArgs const&) mozilla-central/js/src/build_DBG.OBJ/dist/include/js/CallNonGenericMethod.h:100:16
	#2 0x28d5ea4 in js::WasmTableObject::get(JSContext*, unsigned int, JS::Value*) mozilla-central/js/src/wasm/WasmJS.cpp:1741
	#3 0x8bb754 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) mozilla-central/js/src/jscntxtinlines.h:291:15
	#4 0x8bb754 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) mozilla-central/js/src/vm/Interpreter.cpp:472
	#5 0x8a5edd in js::CallFromStack(JSContext*, JS::CallArgs const&) mozilla-central/js/src/vm/Interpreter.cpp:527:12
	#6 0x8a5edd in Interpret(JSContext*, js::RunState&) mozilla-central/js/src/vm/Interpreter.cpp:3061
	    #7 0x885088 in js::RunScript(JSContext*, js::RunState&) mozilla-central/js/src/vm/Interpreter.cpp:422:12
	    #8 0x8c10d4 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) mozilla-central/js/src/vm/Interpreter.cpp:705:15
	    #9 0x8c1dc6 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) mozilla-central/js/src/vm/Interpreter.cpp:737:12
	    #10 0x1894d40 in ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::Value*) mozilla-central/js/src/jsapi.cpp:4702:12
	    #11 0x1895267 in JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>) mozilla-central/js/src/jsapi.cpp:4735:12
	    #12 0x5d5d8d in RunFile(JSContext*, char const*, _IO_FILE*, bool) mozilla-central/js/src/shell/js.cpp:695:14
	    #13 0x5d5d8d in Process(JSContext*, char const*, bool, FileKind) mozilla-central/js/src/shell/js.cpp:1048
	    #14 0x56a13f in ProcessArgs(JSContext*, js::cli::OptionParser*) mozilla-central/js/src/shell/js.cpp:8175:14
	    #15 0x56a13f in Shell(JSContext*, js::cli::OptionParser*, char**) mozilla-central/js/src/shell/js.cpp:8547
	    #16 0x56a13f in main mozilla-central/js/src/shell/js.cpp:8960
	    #17 0x7fe5647af82f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/libc-start.c:291
	0x6110001b4c81 is located 0 bytes to the right of 1-byte region [0x6110001b4c80,0x6110001b4c81)
	allocated by thread T0 here:
	#0 0x51a0f0 in __interceptor_calloc _asan_rtl_
	#1 0x291ef2f in js_calloc(unsigned long) mozilla-central/js/src/build_DBG.OBJ/dist/include/js/Utility.h:376:12
	#2 0x291ef2f in js::wasm::ExternalTableElem* js_pod_calloc<js::wasm::ExternalTableElem>(unsigned long) mozilla-central/js/src/build_DBG.OBJ/dist/include/js/Utility.h:571
	#3 0x291ef2f in js::wasm::ExternalTableElem* js::MallocProvider<JSContext>::maybe_pod_calloc<js::wasm::ExternalTableElem>(unsigned long) mozilla-central/js/src/vm/MallocProvider.h:62
	#4 0x291ef2f in js::wasm::ExternalTableElem* js::MallocProvider<JSContext>::pod_calloc<js::wasm::ExternalTableElem>(unsigned long) mozilla-central/js/src/vm/MallocProvider.h:132
	#5 0x28d2f4f in js::wasm::Table::create(JSContext*, js::wasm::TableDesc const&, JS::Handle<js::WasmTableObject*>) mozilla-central/js/src/wasm/WasmTable.cpp:52:35
	#6 0x28d2354 in js::WasmTableObject::create(JSContext*, js::wasm::Limits const&) mozilla-central/js/src/wasm/WasmJS.cpp:1619:25
	#7 0x28d3b0e in js::WasmTableObject::construct(JSContext*, unsigned int, JS::Value*) mozilla-central/js/src/wasm/WasmJS.cpp:1674:37
	#8 0x8efb57 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) mozilla-central/js/src/jscntxtinlines.h:291:15
	#9 0x8efb57 in js::CallJSNativeConstructor(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) mozilla-central/js/src/jscntxtinlines.h:324
	#10 0x8bea61 in InternalConstruct(JSContext*, js::AnyConstructArgs const&) mozilla-central/js/src/vm/Interpreter.cpp:567:20
	#11 0x8a55c1 in Interpret(JSContext*, js::RunState&) mozilla-central/js/src/vm/Interpreter.cpp:3053:18
	    #12 0x885088 in js::RunScript(JSContext*, js::RunState&) mozilla-central/js/src/vm/Interpreter.cpp:422:12
	    #13 0x8c10d4 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) mozilla-central/js/src/vm/Interpreter.cpp:705:15
	    #14 0x8c1dc6 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) mozilla-central/js/src/vm/Interpreter.cpp:737:12
	    #15 0x1894d40 in ExecuteScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::Value*) mozilla-central/js/src/jsapi.cpp:4702:12
	    #16 0x1895267 in JS_ExecuteScript(JSContext*, JS::Handle<JSScript*>) mozilla-central/js/src/jsapi.cpp:4735:12
	    #17 0x5d5d8d in RunFile(JSContext*, char const*, _IO_FILE*, bool) mozilla-central/js/src/shell/js.cpp:695:14
	    #18 0x5d5d8d in Process(JSContext*, char const*, bool, FileKind) mozilla-central/js/src/shell/js.cpp:1048
	    #19 0x56a13f in ProcessArgs(JSContext*, js::cli::OptionParser*) mozilla-central/js/src/shell/js.cpp:8175:14
	    #20 0x56a13f in Shell(JSContext*, js::cli::OptionParser*, char**) mozilla-central/js/src/shell/js.cpp:8547
	    #21 0x56a13f in main mozilla-central/js/src/shell/js.cpp:8960
	    #22 0x7fe5647af82f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/libc-start.c:291
	
	SUMMARY: AddressSanitizer: heap-buffer-overflow (/mnt/scratch0/clusterfuzz/slave-bot/builds/clusterfuzz-builds-no-engine_spidermonkey_6aad6e0d14f81d36f48dbd887aa56b38e87859f7/revisions/js+0x28d59ad)
	Shadow bytes around the buggy address:
	0x0c228002e940: 00 00 00 00 00 00 00 00 fa fa fa fa fa fa fa fa
	0x0c228002e950: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
	0x0c228002e960: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
	0x0c228002e970: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
	0x0c228002e980: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
	=>0x0c228002e990:[01]fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
	0x0c228002e9a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
	0x0c228002e9b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
	0x0c228002e9c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
	0x0c228002e9d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
	0x0c228002e9e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
	Shadow byte legend (one shadow byte represents 8 application bytes):
	Addressable:           00
	Partially addressable: 01 02 03 04 05 06 07
	Heap left redzone:       fa
	Freed heap region:       fd
	Stack left redzone:      f1
	Stack mid redzone:       f2
	Stack right redzone:     f3
	Stack after return:      f5
	Stack use after scope:   f8
	Global redzone:          f9
	Global init order:       f6
	Poisoned by user:        f7
	Container overflow:      fc
	Array cookie:            ac
	Intra object redzone:    bb
	ASan internal:           fe
	Left alloca redzone:     ca
	Right alloca redzone:    cb
	==13600==ABORTING

Comment 1

[Christian Holler (:decoder)]

This one looks like an actual bug. I'm reducing the test right now.

Needinfo for Benjamin due to wasm being involved.

Comment 2

[Christian Holler (:decoder)]

Here's the reduced testcase:

Summary: AddressSanitizer: heap-buffer-overflow [@ js::WasmTableObject::getImpl] with READ of size 8
Build version: mozilla-central revision 4e6df5159df3
Build flags: --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --disable-tests --enable-stdcxx-compat --disable-profiling --disable-debug --enable-address-sanitizer --disable-jemalloc --enable-optimize=-O2
Runtime options: --fuzzing-safe

Testcase:

v = new WebAssembly.Table({
  element: "anyfunc"
});
v.get(1);

Backtrace:

==24676==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200001d4c0 at pc 0x000001e9d789 bp 0x7ffe061ef930 sp 0x7ffe061ef928
READ of size 8 at 0x60200001d4c0 thread T0
    #0 0x1e9d788 in js::WasmTableObject::getImpl(JSContext*, JS::CallArgs const&) js/src/wasm/WasmJS.cpp:1719:15
    #1 0x1e9d9d9 in bool JS::CallNonGenericMethod<&(IsTable(JS::Handle<JS::Value>)), &js::WasmTableObject::getImpl>(JSContext*, JS::CallArgs const&) dist/include/js/CallNonGenericMethod.h:100:16
    #2 0x1e9d9d9 in js::WasmTableObject::get(JSContext*, unsigned int, JS::Value*) js/src/wasm/WasmJS.cpp:1741
    #3 0x7fa333 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:291:15
[...]

0x60200001d4c0 is located 15 bytes to the right of 1-byte region [0x60200001d4b0,0x60200001d4b1)
allocated by thread T0 here:
    #0 0x5042ac in __interceptor_calloc compiler-rt/lib/asan/asan_malloc_linux.cc:66
    #1 0x1edeff9 in js_calloc(unsigned long) dist/include/js/Utility.h:376:12
    #2 0x1edeff9 in js::wasm::ExternalTableElem* js_pod_calloc<js::wasm::ExternalTableElem>(unsigned long) dist/include/js/Utility.h:571
    #3 0x1edeff9 in js::wasm::ExternalTableElem* js::MallocProvider<JSContext>::maybe_pod_calloc<js::wasm::ExternalTableElem>(unsigned long) js/src/vm/MallocProvider.h:62
    #4 0x1edeff9 in js::wasm::ExternalTableElem* js::MallocProvider<JSContext>::pod_calloc<js::wasm::ExternalTableElem>(unsigned long) js/src/vm/MallocProvider.h:132
    #5 0x1e9b5ac in js::wasm::Table::create(JSContext*, js::wasm::TableDesc const&, JS::Handle<js::WasmTableObject*>) js/src/wasm/WasmTable.cpp:52:31
    #6 0x1e9afea in js::WasmTableObject::create(JSContext*, js::wasm::Limits const&) js/src/wasm/WasmJS.cpp:1619:25
    #7 0x1e9c4d9 in js::WasmTableObject::construct(JSContext*, unsigned int, JS::Value*) js/src/wasm/WasmJS.cpp:1674:37
    #8 0x7fbb79 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) js/src/jscntxtinlines.h:291:15
[...]

SUMMARY: AddressSanitizer: heap-buffer-overflow js/src/wasm/WasmJS.cpp:1719:15 in js::WasmTableObject::getImpl(JSContext*, JS::CallArgs const&)
Shadow bytes around the buggy address:
  0x0c047fffba80: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c047fffba90: fa fa fa fa fa fa 01 fa[fa]fa fd fd fa fa fd fa
  0x0c047fffbaa0: fa fa 07 fa fa fa fd fd fa fa fd fa fa fa 00 00
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd

Comment 3

[Alex Gaynor [:Alex_Gaynor]]

Basically the same issue exists in |js::wasm::Table::set|, the test case is a bit longer, let me know if you'd like a separate bug filed for it.

Comment 4

[Luke Wagner [:luke]]

Oh dear, when ToNonWrappingUint32() was introduced, WasmTableObject::getImpl() started passing table.length()-1 which of course underflows when table.length()=0.  This is pretty bad; we should definitely get this fixed ASAP.

Comment 5

[Luke Wagner [:luke]]

Attachment: fix-table-len-zero

Comment 6

[Luke Wagner [:luke]]

Attachment: fix-table-len-zero

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8926252 [details] [diff] [review]
fix-table-len-zero

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

Aouch.

::: js/src/wasm/WasmJS.cpp
@@ +1711,5 @@
> +    if (!ToNonWrappingUint32(cx, v, UINT32_MAX, "Table", noun, index))
> +        return false;
> +
> +    if (*index >= table.length()) {
> +        JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr, JSMSG_WASM_BAD_UINT32, "Table", noun);

(I'd have suggested to use JSMSG_WASM_OUT_OF_BOUNDS, but this has to be a RangeError, so BAD_UINT32 is more correct)

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Regression from bug 1284156 I guess. Wasm is disabled on ESR52, so setting the remaining flags accordingly.

Comment 9

[Luke Wagner [:luke]]

Comment on attachment 8926252 [details] [diff] [review]
fix-table-len-zero

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Moderately easy.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No

Which older supported branches are affected by this flaw?
release, beta, but not ESR52

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

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Easy

How likely is this patch to cause regressions; how much testing does it need?
Unlikely, pretty simple

Comment 10

[Luke Wagner [:luke]]

Comment on attachment 8926252 [details] [diff] [review]
fix-table-len-zero

Timing is tight with upcoming release, so I thought maybe I'd parallelize these requests.

Approval Request Comment
[Feature/Bug causing the regression]: 1284156
[User impact if declined]: exploitable security bug
[Is this code covered by automated tests?]: privately, but not landed (to avoid painting a target)
[Has the fix been verified in Nightly?]: not yet (due to imminent release)
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: simple patch, only executed by wasm (not even asm.js)
[String changes made/needed]:

Comment 11

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for checkin on 11/28 or immediately after. Do not check this in before then as that is two weeks into the 58 cycle. 

This won't make the 57 release as it is too late for that. We are about to ship.

Comment 12

[Alex Gaynor [:Alex_Gaynor]]

Looks like this may have been (accidentally?) resolved by bug 1418195.

Comment 13

[Luke Wagner [:luke]]

Bug 1418195 conflicts with this patch by renaming ToNonWrappingUint32() but it doesn't fix the underlying issue (the `table.length() - 1` is still there).  I'll rebase before landing and post a branch patch.

Comment 14

[Luke Wagner [:luke]]

I rebased and landed on m-i (59):
  https://hg.mozilla.org/integration/mozilla-inbound/rev/622c05a8e7a1
The beta branch patch is actually exactly what was reviewed (no rebase necessary).  Is any further approval required to land on 58?

Comment 15

[Ryan VanderMeulen [:RyanVM]]

You still need to do the regular beta approval request, yes.

Comment 16

[Luke Wagner [:luke]]

Comment on attachment 8926252 [details] [diff] [review]
fix-table-len-zero

Approval Request Comment
See comment 10.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/622c05a8e7a1

Comment 18

[Gerry Chang [:gchang]]

Comment on attachment 8926252 [details] [diff] [review]
fix-table-len-zero

Fix a sec-high. Beta58+.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/70d5016333a4

Comment 21

[Al Billings [:abillings - ex-MoCo]]

(In reply to Alex Gaynor [:Alex_Gaynor] from comment #0)
> Created attachment 8926053 [details]
> clusterfuzz-testcase-6215596157960192.js
> 
> This bug was found by Google's OSS-Fuzz running their custom internal JS
> fuzzer. I am refiling it in our issue tracker.
> 
> Please note that they apply a 90-day disclose timeline to all bugs:

I need the name of the reporter and/or who to credit for advisory since it isn't you and this is a third party report. Can you supply this?

Comment 22

[Alex Gaynor [:Alex_Gaynor]]

Per https://github.com/google/oss-fuzz#process-overview these finds should be credited to OSS-Fuzz.