Bug 1415883 (CVE-2018-5094)

Heap-buffer-overflow READ 8 with async generators

RESOLVED FIXED in Firefox 58

Status

()

defect
P1
normal
RESOLVED FIXED
2 years ago
11 months ago

People

(Reporter: Alex_Gaynor, Assigned: jandem)

Tracking

({csectype-bounds, sec-high})

Trunk
mozilla59
Points:
---
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)

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
Ni arai because async stuff :)
Flags: needinfo?(arai.unmht)
confirmed similar heap-buffer-overflow on macOS here.
I'll look into it
Assignee: nobody → arai.unmht
Status: NEW → ASSIGNED
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();
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"}
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)
Sure I can take a look at this tomorrow.
(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.
Posted patch PatchSplinter Review
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 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+
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?
(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.
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.
Whiteboard: [checkin on 11/28]
Attachment #8927290 - Flags: sec-approval? → sec-approval+
Duplicate of this bug: 1417898
Whiteboard: [checkin on 11/28]
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?
https://hg.mozilla.org/mozilla-central/rev/2467d71d0e0d
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Attachment #8927290 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Group: javascript-core-security → core-security-release
(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+]
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.