Bug 1415883 (CVE-2018-5094)

Heap-buffer-overflow READ 8 with async generators

RESOLVED FIXED in Firefox 58

Status

()

Product:
Core
Component:
JavaScript Engine
Importance:
P1
normal
Status:
RESOLVED FIXED
Reported:
a year ago
Modified:
4 months ago

People

(Reporter: Alex_Gaynor, Assigned: jandem)

Tracking

({csectype-bounds, sec-high})

Version:
Trunk
Target:
mozilla59
Keywords:
csectype-bounds, sec-high
Points:
---
Duplicates:
1417898
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox56 wontfix, firefox57- wontfix, firefox58+ fixed, firefox59+ fixed)

Details

(Whiteboard: [adv-main58+][post-critsmash-triage])

Attachments

(2 attachments)

clusterfuzz-testcase-minimized-4564957816422400.js
1.70 KB, application/x-javascript
Details
Patch
1.46 KB, patch
arai
: review+
abillings
: approval-mozilla-beta+
abillings
: sec-approval+
Details | Diff | Splinter Review
(Reporter)

Description

•
a year ago 
Created attachment 8926854 [details]
clusterfuzz-testcase-minimized-4564957816422400.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
	
	=================================================================
	==29026==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x61100032bcc0 at pc 0x0000005ff8fc bp 0x7fff9dd8dbb0 sp 0x7fff9dd8dba8
	READ of size 8 at 0x61100032bcc0 thread T0
	SCARINESS: 23 (8-byte-read-heap-buffer-overflow)
	#0 0x5ff8fb in JS::Value::toTag() const mozilla-central/js/src/build_DBG.OBJ/dist/include/js/Value.h:437:32
	#1 0x5ff8fb in JS::Value::isString() const mozilla-central/js/src/build_DBG.OBJ/dist/include/js/Value.h:512
	#2 0x5ff8fb in _ZN2js13DispatchTypedINS_17PreBarrierFunctorIN2JS5ValueEEEJEEEDTclfp_scP8JSObjectLDnEspclsr7mozillaE7ForwardIT0_Efp1_EEET_RKS3_DpOS7_ mozilla-central/js/src/build_DBG.OBJ/dist/include/js/Value.h:1431
	#3 0x6ed1e9 in js::InternalBarrierMethods<JS::Value>::preBarrier(JS::Value const&) mozilla-central/js/src/gc/Barrier.h:283:9
	#4 0x6ed1e9 in js::WriteBarrieredBase<JS::Value>::pre() mozilla-central/js/src/gc/Barrier.h:374
	#5 0x6ed1e9 in js::HeapSlot::~HeapSlot() mozilla-central/js/src/gc/Barrier.h:678
	#6 0x6ed1e9 in js::NativeObject::prepareElementRangeForOverwrite(unsigned long, unsigned long) mozilla-central/js/src/vm/NativeObject.h:1007
	#7 0x6ed1e9 in js::NativeObject::setDenseInitializedLengthUnchecked(unsigned int) mozilla-central/js/src/vm/NativeObject.h:1179
	#8 0x1e243d0 in js::NativeObject::setDenseInitializedLength(unsigned int) mozilla-central/js/src/vm/NativeObject.h:1196:9
	#9 0x1e243d0 in js::AsyncGeneratorRequest* js::ShiftFromList<js::AsyncGeneratorRequest>(JSContext*, JS::Handle<js::NativeObject*>) mozilla-central/js/src/vm/List-inl.h:62
	    #10 0x1e10267 in js::AsyncGeneratorObject::dequeueRequest(JSContext*, JS::Handle<js::AsyncGeneratorObject*>) mozilla-central/js/src/vm/AsyncIteration.cpp:374:12
	    #11 0xa847c4 in js::AsyncGeneratorResolve(JSContext*, JS::Handle<js::AsyncGeneratorObject*>, JS::Handle<JS::Value>, bool) mozilla-central/js/src/builtin/Promise.cpp:2769:13
	    #12 0xa86073 in AsyncGeneratorResumeNext(JSContext*, JS::Handle<js::AsyncGeneratorObject*>) mozilla-central/js/src/builtin/Promise.cpp:2890:16
	    #13 0xa84d8b in js::AsyncGeneratorResolve(JSContext*, JS::Handle<js::AsyncGeneratorObject*>, JS::Handle<JS::Value>, bool) mozilla-central/js/src/builtin/Promise.cpp:2790:10
	    #14 0xa86073 in AsyncGeneratorResumeNext(JSContext*, JS::Handle<js::AsyncGeneratorObject*>) mozilla-central/js/src/builtin/Promise.cpp:2890:16
	    #15 0xa84d8b in js::AsyncGeneratorResolve(JSContext*, JS::Handle<js::AsyncGeneratorObject*>, JS::Handle<JS::Value>, bool) mozilla-central/js/src/builtin/Promise.cpp:2790:10
	    #16 0xa86073 in AsyncGeneratorResumeNext(JSContext*, JS::Handle<js::AsyncGeneratorObject*>) mozilla-central/js/src/builtin/Promise.cpp:2890:16
	    #17 0xa84d8b in js::AsyncGeneratorResolve(JSContext*, JS::Handle<js::AsyncGeneratorObject*>, JS::Handle<JS::Value>, bool) mozilla-central/js/src/builtin/Promise.cpp:2790:10
	    #18 0xa86073 in AsyncGeneratorResumeNext(JSContext*, JS::Handle<js::AsyncGeneratorObject*>) mozilla-central/js/src/builtin/Promise.cpp:2890:16
	    #19 0xa84d8b in js::AsyncGeneratorResolve(JSContext*, JS::Handle<js::AsyncGeneratorObject*>, JS::Handle<JS::Value>, bool) mozilla-central/js/src/builtin/Promise.cpp:2790:10
	    #20 0xa86073 in AsyncGeneratorResumeNext(JSContext*, JS::Handle<js::AsyncGeneratorObject*>) mozilla-central/js/src/builtin/Promise.cpp:2890:16
	    #21 0xa84d8b in js::AsyncGeneratorResolve(JSContext*, JS::Handle<js::AsyncGeneratorObject*>, JS::Handle<JS::Value>, bool) mozilla-central/js/src/builtin/Promise.cpp:2790:10
	    #22 0x1e0bfd7 in js::AsyncGeneratorResume(JSContext*, JS::Handle<js::AsyncGeneratorObject*>, js::CompletionKind, JS::Handle<JS::Value>) mozilla-central/js/src/vm/AsyncIteration.cpp:0
	    #23 0xa867e8 in AsyncGeneratorResumeNext(JSContext*, JS::Handle<js::AsyncGeneratorObject*>) mozilla-central/js/src/builtin/Promise.cpp:2918:12
	    #24 0xa84d8b in js::AsyncGeneratorResolve(JSContext*, JS::Handle<js::AsyncGeneratorObject*>, JS::Handle<JS::Value>, bool) mozilla-central/js/src/builtin/Promise.cpp:2790:10
	    #25 0x1e0bfd7 in js::AsyncGeneratorResume(JSContext*, JS::Handle<js::AsyncGeneratorObject*>, js::CompletionKind, JS::Handle<JS::Value>) mozilla-central/js/src/vm/AsyncIteration.cpp:0
	    #26 0xb405e3 in AsyncGeneratorPromiseReactionJob(JSContext*, JS::Handle<PromiseReactionRecord*>, JS::MutableHandle<JS::Value>) mozilla-central/js/src/builtin/Promise.cpp:1122:14
	    #27 0xb405e3 in PromiseReactionJob(JSContext*, unsigned int, JS::Value*) mozilla-central/js/src/builtin/Promise.cpp:1201
	    #28 0x8bc784 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) mozilla-central/js/src/jscntxtinlines.h:291:15
	    #29 0x8bc784 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) mozilla-central/js/src/vm/Interpreter.cpp:472
	    #30 0x8be967 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) mozilla-central/js/src/vm/Interpreter.cpp:540:10
	    #31 0x18777ce in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) mozilla-central/js/src/jsapi.cpp:3032:12
	    #32 0x18d8525 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JSObject*>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) mozilla-central/js/src/jsapi.h:3595:12
	    #33 0x18d8525 in js::RunJobs(JSContext*) mozilla-central/js/src/jscntxt.cpp:1213
	    #34 0x56a2f4 in Shell(JSContext*, js::cli::OptionParser*, char**) mozilla-central/js/src/shell/js.cpp:8561:9
	    #35 0x56a2f4 in main mozilla-central/js/src/shell/js.cpp:8967
	    #36 0x7fc3a22c282f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/libc-start.c:291
	0x61100032bcc0 is located 0 bytes to the right of 64-byte region [0x61100032bc80,0x61100032bcc0)
	allocated by thread T0 here:
	#0 0x51a300 in __interceptor_realloc _asan_rtl_
	#1 0x1913512 in js_realloc(void*, unsigned long) mozilla-central/js/src/build_DBG.OBJ/dist/include/js/Utility.h:406:12
	#2 0x1913512 in unsigned char* js_pod_realloc<unsigned char>(unsigned char*, unsigned long, unsigned long) mozilla-central/js/src/build_DBG.OBJ/dist/include/js/Utility.h:594
	#3 0x1913512 in unsigned char* js::MallocProvider<JS::Zone>::maybe_pod_realloc<unsigned char>(unsigned char*, unsigned long, unsigned long) mozilla-central/js/src/vm/MallocProvider.h:70
	#4 0x1913512 in unsigned char* js::MallocProvider<JS::Zone>::pod_realloc<unsigned char>(unsigned char*, unsigned long, unsigned long) mozilla-central/js/src/vm/MallocProvider.h:173
	#5 0x2a1f9d3 in js::Nursery::reallocateBuffer(JSObject*, void*, unsigned long, unsigned long) mozilla-central/js/src/gc/Nursery.cpp:403:29
	#6 0x20e78c9 in js::HeapSlot* js::ReallocateObjectBuffer<js::HeapSlot>(JSContext*, JSObject*, js::HeapSlot*, unsigned int, unsigned int) mozilla-central/js/src/gc/Nursery-inl.h:135:48
	#7 0x20e78c9 in js::NativeObject::shrinkElements(JSContext*, unsigned int) mozilla-central/js/src/vm/NativeObject.cpp:1028
	#8 0x1e24364 in js::AsyncGeneratorRequest* js::ShiftFromList<js::AsyncGeneratorRequest>(JSContext*, JS::Handle<js::NativeObject*>) mozilla-central/js/src/vm/List-inl.h:59:15
	    #9 0x1e10267 in js::AsyncGeneratorObject::dequeueRequest(JSContext*, JS::Handle<js::AsyncGeneratorObject*>) mozilla-central/js/src/vm/AsyncIteration.cpp:374:12
	    #10 0xa847c4 in js::AsyncGeneratorResolve(JSContext*, JS::Handle<js::AsyncGeneratorObject*>, JS::Handle<JS::Value>, bool) mozilla-central/js/src/builtin/Promise.cpp:2769:13
	    #11 0xa86073 in AsyncGeneratorResumeNext(JSContext*, JS::Handle<js::AsyncGeneratorObject*>) mozilla-central/js/src/builtin/Promise.cpp:2890:16
	    #12 0xa84d8b in js::AsyncGeneratorResolve(JSContext*, JS::Handle<js::AsyncGeneratorObject*>, JS::Handle<JS::Value>, bool) mozilla-central/js/src/builtin/Promise.cpp:2790:10
	    #13 0xa86073 in AsyncGeneratorResumeNext(JSContext*, JS::Handle<js::AsyncGeneratorObject*>) mozilla-central/js/src/builtin/Promise.cpp:2890:16
	    #14 0xa84d8b in js::AsyncGeneratorResolve(JSContext*, JS::Handle<js::AsyncGeneratorObject*>, JS::Handle<JS::Value>, bool) mozilla-central/js/src/builtin/Promise.cpp:2790:10
	    #15 0xa86073 in AsyncGeneratorResumeNext(JSContext*, JS::Handle<js::AsyncGeneratorObject*>) mozilla-central/js/src/builtin/Promise.cpp:2890:16
	    #16 0xa84d8b in js::AsyncGeneratorResolve(JSContext*, JS::Handle<js::AsyncGeneratorObject*>, JS::Handle<JS::Value>, bool) mozilla-central/js/src/builtin/Promise.cpp:2790:10
	    #17 0xa86073 in AsyncGeneratorResumeNext(JSContext*, JS::Handle<js::AsyncGeneratorObject*>) mozilla-central/js/src/builtin/Promise.cpp:2890:16
	    #18 0xa84d8b in js::AsyncGeneratorResolve(JSContext*, JS::Handle<js::AsyncGeneratorObject*>, JS::Handle<JS::Value>, bool) mozilla-central/js/src/builtin/Promise.cpp:2790:10
	    #19 0xa86073 in AsyncGeneratorResumeNext(JSContext*, JS::Handle<js::AsyncGeneratorObject*>) mozilla-central/js/src/builtin/Promise.cpp:2890:16
	    #20 0xa84d8b in js::AsyncGeneratorResolve(JSContext*, JS::Handle<js::AsyncGeneratorObject*>, JS::Handle<JS::Value>, bool) mozilla-central/js/src/builtin/Promise.cpp:2790:10
	    #21 0x1e0bfd7 in js::AsyncGeneratorResume(JSContext*, JS::Handle<js::AsyncGeneratorObject*>, js::CompletionKind, JS::Handle<JS::Value>) mozilla-central/js/src/vm/AsyncIteration.cpp:0
	    #22 0xa867e8 in AsyncGeneratorResumeNext(JSContext*, JS::Handle<js::AsyncGeneratorObject*>) mozilla-central/js/src/builtin/Promise.cpp:2918:12
	    #23 0xa84d8b in js::AsyncGeneratorResolve(JSContext*, JS::Handle<js::AsyncGeneratorObject*>, JS::Handle<JS::Value>, bool) mozilla-central/js/src/builtin/Promise.cpp:2790:10
	    #24 0x1e0bfd7 in js::AsyncGeneratorResume(JSContext*, JS::Handle<js::AsyncGeneratorObject*>, js::CompletionKind, JS::Handle<JS::Value>) mozilla-central/js/src/vm/AsyncIteration.cpp:0
	    #25 0xb405e3 in AsyncGeneratorPromiseReactionJob(JSContext*, JS::Handle<PromiseReactionRecord*>, JS::MutableHandle<JS::Value>) mozilla-central/js/src/builtin/Promise.cpp:1122:14
	    #26 0xb405e3 in PromiseReactionJob(JSContext*, unsigned int, JS::Value*) mozilla-central/js/src/builtin/Promise.cpp:1201
	    #27 0x8bc784 in js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) mozilla-central/js/src/jscntxtinlines.h:291:15
	    #28 0x8bc784 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) mozilla-central/js/src/vm/Interpreter.cpp:472
	    #29 0x8be967 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) mozilla-central/js/src/vm/Interpreter.cpp:540:10
	    #30 0x18777ce in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) mozilla-central/js/src/jsapi.cpp:3032:12
	    #31 0x18d8525 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JSObject*>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) mozilla-central/js/src/jsapi.h:3595:12
	    #32 0x18d8525 in js::RunJobs(JSContext*) mozilla-central/js/src/jscntxt.cpp:1213
	    #33 0x56a2f4 in Shell(JSContext*, js::cli::OptionParser*, char**) mozilla-central/js/src/shell/js.cpp:8561:9
	    #34 0x56a2f4 in main mozilla-central/js/src/shell/js.cpp:8967
	    #35 0x7fc3a22c282f 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+0x5ff8fb)
	Shadow bytes around the buggy address:
	0x0c228005d740: fd fd fd fd fa fa fa fa fa fa fa fa fa fa fa fa
	0x0c228005d750: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
	0x0c228005d760: fa fa fa fa fa fa fa fa 00 00 00 00 00 00 00 00
	0x0c228005d770: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
	0x0c228005d780: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
	=>0x0c228005d790: 00 00 00 00 00 00 00 00[fa]fa fa fa fa fa fa fa
	0x0c228005d7a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
	0x0c228005d7b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
	0x0c228005d7c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
	0x0c228005d7d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
	0x0c228005d7e0: 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
	==29026==ABORTING
Group: core-security → javascript-core-security
 (Assignee)

Comment 1

•
a year ago 
Ni arai because async stuff :)
Flags: needinfo?(arai.unmht)

Comment 2

•
a year ago 
confirmed similar heap-buffer-overflow on macOS here.
I'll look into it
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED

Comment 3

•
a year ago 
here's reduced testcase.
looks like GC related (JS_GC_ZEAL=4 is necessary).

asan build
  revision: m-i 486620c2345a
  configure flag: --disable-jemalloc --enable-address-sanitizer --enable-debug --disable-optimize
  build env: JS_GC_ZEAL=IncrementalMultipleSlices LLVM_SYMBOLIZER={TOOLTOOL_CHECKOUT}/clang/bin/llvm-symbolizer
  run env: JS_GC_ZEAL=4

var f = async function* () {
      yield 1;
};
var g = f();
g.next();
g.next();
g.next();
g.next();
g.next();
g.next();
g.next();

Comment 4

•
a year ago 
here's custom variant for autospider, just in case
{"debug": true, "configure-args": " --disable-jemalloc --enable-address-sanitizer", "optimize": false, "env": {"JS_GC_ZEAL": "IncrementalMultipleSlices", "LLVM_SYMBOLIZER": "{TOOLTOOL_CHECKOUT}/clang/bin/llvm-symbolizer"}, "compiler": "clang"}

Comment 5

•
a year ago 
apparently elements_ points out-of-bound after shrink from length=1 to 0.

https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/js/src/vm/List-inl.h#59-62
> template<class T>
> inline MOZ_MUST_USE T*
> ShiftFromList(JSContext* cx, HandleNativeObject list)
> {
>     uint32_t length = list->getDenseInitializedLength();
>     MOZ_ASSERT(length > 0);
> 
>     Rooted<T*> entry(cx, &list->getDenseElement(0).toObject().as<T>());
>     if (!list->tryShiftDenseElements(1)) {
>         list->moveDenseElements(0, 1, length - 1);
>         list->shrinkElements(cx, length - 1);
>     }
> 
>     list->setDenseInitializedLength(length - 1);
> 
>     return entry;
> }

https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/js/src/vm/NativeObject.cpp#1038
> void
> NativeObject::shrinkElements(JSContext* cx, uint32_t reqCapacity)
> {
> ...
>     HeapSlot* oldHeaderSlots = reinterpret_cast<HeapSlot*>(getUnshiftedElementsHeader());
>     HeapSlot* newHeaderSlots = ReallocateObjectBuffer<HeapSlot>(cx, this, oldHeaderSlots,
>                                                                 oldAllocated, newAllocated);
>     if (!newHeaderSlots) {
>         cx->recoverFromOutOfMemory();
>         return;  // Leave elements at its old size.
>     }
> 
>     ObjectElements* newheader = reinterpret_cast<ObjectElements*>(newHeaderSlots);
>     elements_ = newheader->elements() + numShifted;
>     getElementsHeader()->capacity = newCapacity;

there:
  oldAllocated = 16
  oldCapacity = 8
  newAllocated = 8
  newCapacity = 0
  numShifted = 6

so, elements_ points the element after the last allocated element.

then, inside setDenseInitializedLength we try to pre-barrier on the 0-th elements, that is out-of-bound.

https://dxr.mozilla.org/mozilla-central/rev/2535bad09d720e71a982f3f70dd6925f66ab8ec7/js/src/vm/NativeObject.h#1004
>     void prepareElementRangeForOverwrite(size_t start, size_t end) {
>         MOZ_ASSERT(end <= getDenseInitializedLength());
>         MOZ_ASSERT(!denseElementsAreCopyOnWrite());
>         for (size_t i = start; i < end; i++)
>             elements_[i].HeapSlot::~HeapSlot();
>     }

so, I think we should do pre-barrier and resetting initializedLength inside shrinkElements, maybe?

jandem, I think you know better about the shift optimization.
what's the expected behavior for this case?
Flags: needinfo?(arai.unmht) → needinfo?(jdemooij)
 (Assignee)

Comment 6

•
a year ago 
Sure I can take a look at this tomorrow.

Comment 7

•
a year ago 
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #0)
> Created attachment 8926854 [details]
> clusterfuzz-testcase-minimized-4564957816422400.js
> 
> This bug was found by Google's OSS-Fuzz running their custom internal JS
> fuzzer. I am refiling it in our issue tracker.

Did they notify you of it? I'm wondering if we're going to be funneled more bugs this way now.
Keywords: csectype-bounds, sec-high
 (Assignee)

Comment 8

•
a year ago 
Created attachment 8927290 [details] [diff] [review]
Patch

This is unrelated to the shift optimization. The problem is that we do:

  list->shrinkElements(cx, length - 1);
  ..
  list->setDenseInitializedLength(length - 1);

shrinkElements reallocates the elements in memory and sets capacity to |length - 1|. Then setDenseInitializedLength may invoke a GC pre-barrier on the last element but that's now uninitialized memory because it's outside our allocated capacity.

To catch similar issues in the future I added this assert to shrinkElements:

  MOZ_ASSERT(reqCapacity >= getDenseInitializedLength());

That assert fails on many async-generator shell tests without the fix.

Also, tryShiftDenseElements updates the initialized length, so I moved setDenseInitializedLength inside the if-statement and added an assert at the end of the function.
Assignee: arai.unmht → jdemooij
Flags: needinfo?(jdemooij)
Attachment #8927290 - Flags: review?(arai.unmht)

Comment 9

•
a year ago 
Comment on attachment 8927290 [details] [diff] [review]
Patch

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

ah, thanks, makes sense :)
Attachment #8927290 - Flags: review?(arai.unmht) → review+
 (Assignee)

Updated

•
a year ago
status-firefox56: --- → unaffected
status-firefox57: --- → affected
status-firefox-esr52: --- → unaffected
tracking-firefox57: --- → ?
tracking-firefox58: --- → ?
Priority: -- → P1
 (Assignee)

Updated

•
a year ago
status-firefox56: unaffected → affected
 (Assignee)

Comment 10

•
a year ago 
Comment on attachment 8927290 [details] [diff] [review]
Patch

[Security approval request comment]
> How easily could an exploit be constructed based on the patch?
An attacker can invoke a GC pre-barrier on uninitialized memory. Not easy to exploit but it's possible.

> 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?
56+.

> If not all supported branches, which bug introduced the flaw?
Bug 1272697 added the code (for ReadableStream). Since bug 1390082 we use this code for async generators as well.

> Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
Should apply or be easy to backport.

> How likely is this patch to cause regressions; how much testing does it need?
Unlikely.
Attachment #8927290 - Flags: sec-approval?
 (Assignee)

Comment 11

•
a year ago 
(In reply to Jan de Mooij [:jandem] from comment #10)
> An attacker can invoke a GC pre-barrier on uninitialized memory.

Hm I just realized there's another way to attack this: because we invoke the pre-barrier on uninitialized memory, it means we *don't* barrier/trace the object we *should have* barriered. So this is also a GC hazard.

Comment 12

•
a year ago 
Too late for 57.
status-firefox56: affected → wontfix
status-firefox57: affected → wontfix
tracking-firefox57: ? → -
tracking-firefox58: ? → +

Comment 13

•
a year ago 
Sec-approval+ for checkin on November 28, two weeks into the new cycle. Please do not check in before that date.

We'll want this on Beta so a Beta patch should be made and nominated at that time as well.
status-firefox59: --- → affected
tracking-firefox59: --- → +
Whiteboard: [checkin on 11/28]
Attachment #8927290 - Flags: sec-approval? → sec-approval+
 (Assignee)

Updated

•
a year ago
Duplicate of this bug: 1417898
 (Assignee)

Updated

•
a year ago
Whiteboard: [checkin on 11/28]
 (Assignee)

Comment 16

•
a year ago 
Comment on attachment 8927290 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1272697 added the code (for ReadableStream). Since bug 1390082 we use this code for async generators as well.
[User impact if declined]: Security issues.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Not yet.
[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?]: Local/minor fix.
[String changes made/needed]: None.
Attachment #8927290 - Flags: approval-mozilla-beta?

Comment 17

•
a year ago 
https://hg.mozilla.org/mozilla-central/rev/2467d71d0e0d
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Attachment #8927290 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: javascript-core-security → core-security-release

Comment 19

•
a year ago 
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #0)
> Created attachment 8926854 [details]
> clusterfuzz-testcase-minimized-4564957816422400.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?
Flags: needinfo?(agaynor)
Whiteboard: [adv-main58+]
 (Reporter)

Comment 20

•
a year ago 
Per https://github.com/google/oss-fuzz#process-overview these finds should be credited to OSS-Fuzz.
Flags: needinfo?(agaynor)
Alias: CVE-2018-5094
Flags: qe-verify-
Whiteboard: [adv-main58+] → [adv-main58+][post-critsmash-triage]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.