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

Issue 840320 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: May 14
Cc:
Components:
EstimatedDays: ----
NextAction: 2018-06-20
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug-Security



Sign in to add a comment

Security: type confusion trigger DCHECK fail in ReadableStreamBytesConsumer::OnFulfilled::Call

Reported by higonggu...@gmail.com, May 7

Issue description


VULNERABILITY DETAILS
the argument v passed to the  Function ReadableStreamBytesConsumer::OnFulfilled::Call May be not a js object.
  ScriptValue Call(ScriptValue v) override {
    bool done;
    v8::Local<v8::Value> item = v.V8Value();
    DCHECK(item->IsObject()); ------>this check will be fail under certain circumstances
    v8::Local<v8::Value> value =
        V8UnpackIteratorResult(v.GetScriptState(), item.As<v8::Object>(), &done)------------> this line will cause type confusion when convert item to an object.
            .ToLocalChecked();
    if (done) {
      consumer_->OnReadDone();
      return v;
    }
    if (!value->IsUint8Array()) {
      consumer_->OnRejected();
      return ScriptValue();
    }
    consumer_->OnRead(V8Uint8Array::ToImpl(value.As<v8::Object>()));
    return v;
  }

VERSION
Chrome Version: [66.0.3359.139] 
Operating System: [All]

REPRODUCTION CASE
load the attached poc.html in a debugger version chrome and you'll see the crash as follows.

[32192:32225:0507/182630.740682:FATAL:readable_stream_bytes_consumer.cc(36)] Check failed: item->IsObject(). 

Program received signal SIGTRAP, Trace/breakpoint trap.
[Switching to Thread 0x7fffb91cd700 (LWP 32225)]
base::debug::(anonymous namespace)::DebugBreak () at ../../base/debug/debugger_posix.cc:239
239	}
(gdb) bt
#0  base::debug::(anonymous namespace)::DebugBreak () at ../../base/debug/debugger_posix.cc:239
#1  0x00007ffff7ad7a88 in base::debug::BreakDebugger () at ../../base/debug/debugger_posix.cc:258
#2  0x00007ffff7875d34 in logging::LogMessage::~LogMessage (this=0x7fffb91c96a0) at ../../base/logging.cc:855
#3  0x00007fffe7fac6a6 in blink::ReadableStreamBytesConsumer::OnFulfilled::Call (this=0x19fd50194140, v=...)
    at ../../third_party/blink/renderer/core/fetch/readable_stream_bytes_consumer.cc:36
#4  0x00007fffe75d2e54 in blink::ScriptFunction::CallCallback (args=...) at ../../third_party/blink/renderer/bindings/core/v8/script_function.cc:32
#5  0x00007fffea009953 in v8::internal::FunctionCallbackArguments::Call (this=<optimized out>, handler=<optimized out>) at ../../v8/src/api-arguments-inl.h:94
#6  0x00007fffea007c4e in v8::internal::(anonymous namespace)::HandleApiCallHelper<false> (isolate=<optimized out>, function=..., new_target=..., fun_data=..., receiver=..., args=...)
    at ../../v8/src/builtins/builtins-api.cc:108
#7  0x00007fffea005f39 in v8::internal::Builtin_Impl_HandleApiCall (args=..., isolate=<optimized out>) at ../../v8/src/builtins/builtins-api.cc:138
#8  0x00007fffea00597d in v8::internal::Builtin_HandleApiCall (args_length=<optimized out>, args_object=<optimized out>, isolate=<optimized out>)
    at ../../v8/src/builtins/builtins-api.cc:126
#9  0x000022e397d10f19 in ?? ()
#10 0x000022e397d10e81 in ?? ()
#11 0x00007fffb91c9e10 in ?? ()
#12 0x0000000000000006 in ?? ()
#13 0x00007fffb91c9e80 in ?? ()
#14 0x00007fffeaadf12b in v8_Default_embedded_blob_ () from /home/ggong/ssd1/chromium/src/out/Debug/./libv8.so
#15 0x0000136c3ce826c1 in ?? ()
#16 0x000028e057edd909 in ?? ()
#17 0x0000000600000000 in ?? ()
#18 0x0000136c3ce827d1 in ?? ()
#19 0x0000000100000000 in ?? ()
#20 0x00000dc66d807491 in ?? ()
#21 0x00000dc66d81a941 in ?? ()
#22 0x00000dc66d81a479 in ?? ()
#23 0x0000000000000016 in ?? ()
#24 0x00007fffb91c9ed8 in ?? ()
#25 0x00007fffeaa563f0 in v8_Default_embedded_blob_ () from /home/ggong/ssd1/chromium/src/out/Debug/./libv8.so
#26 0x00000dc66d81ad99 in ?? ()
#27 0x00007ffff7b9cb38 in base::trace_event::TraceLog::GetInstance()::instance () from /home/ggong/ssd1/chromium/src/out/Debug/./libbase.so
#28 0x0000000000000002 in ?? ()
#29 0x00000dc66d81a941 in ?? ()
#30 0x00000dc66d81a479 in ?? ()
#31 0x0000000000000000 in ?? ()

the patch is simple, attached as patch.diff

diff --git a/third_party/blink/renderer/core/fetch/readable_stream_bytes_consumer.cc b/third_party/blink/renderer/core/fetch/readable_stream_bytes_consumer.cc
index abf67b3..ec7eaec 100644
--- a/third_party/blink/renderer/core/fetch/readable_stream_bytes_consumer.cc
+++ b/third_party/blink/renderer/core/fetch/readable_stream_bytes_consumer.cc
@@ -33,7 +33,10 @@ class ReadableStreamBytesConsumer::OnFulfilled final : public ScriptFunction {
   ScriptValue Call(ScriptValue v) override {
     bool done;
     v8::Local<v8::Value> item = v.V8Value();
-    DCHECK(item->IsObject());
+    if (!item->IsObject()) {
+      consumer_->OnRejected();
+      return ScriptValue();
+    }
     v8::Local<v8::Value> value =
         V8UnpackIteratorResult(v.GetScriptState(), item.As<v8::Object>(), &done)
             .ToLocalChecked();



 
poc.html
954 bytes View Download
patch.diff
828 bytes Download
Project Member

Comment 1 by ClusterFuzz, May 7

ClusterFuzz is analyzing your testcase. Developers can follow the progress at https://clusterfuzz.com/testcase?key=6488298034561024.
Components: Blink>JavaScript Blink>Network>FetchAPI
Labels: Security_Severity-High M-67 Security_Impact-Stable Pri-1
Owner: yhirano@chromium.org
Status: Assigned (was: Unconfirmed)
Assigning per OWNERs file.
Labels: OS-Android OS-Chrome OS-Fuchsia OS-Linux OS-Mac OS-Windows
Components: -Blink>JavaScript Blink>Network>StreamsAPI
Cc: ricea@chromium.org
Cc: -ricea@chromium.org yhirano@chromium.org
Owner: ricea@chromium.org

Comment 8 Deleted

wpt-style poc:

promise_test(async () => {
  const hello = new TextEncoder('utf-8').encode('hello');
  const bye = new TextEncoder('utf-8').encode('bye');
  const rs = new ReadableStream({
    start(controller) {
      controller.enqueue(hello);
      controller.close();
    }
  });
  const resp = new Response(rs);
  Object.prototype.then = (f1, f2) => {
    delete Object.prototype.then;
    f1({done: false, value: bye});
  };
  const text = await resp.text();
  assert_equals(text, 'hello');
}, '...');

Cc: domenic@chromium.org
Unfortunately, our ReadableStream implementation is behaving according to the standard. CreateIterResultObject() in the ECMAScript standard explicitly creates an object using Object.prototype: https://tc39.github.io/ecma262/#sec-createiterresultobject

I'd like to change this, because I think it breaks the guarantee that the internals of pipeTo() are invisible to Javascript. I will file an issue against the Streams Standard once we have resolved this issue.

This means the DCHECK() is wrong: there's no guarantee that we get an object here.

I am going to check the Fetch Standard to see whether our behaviour is standard there.
The Fetch Standard is fine (our behaviour is wrong). The relevant step is:

> * When read is fulfilled with a value that matches with neither of the above patterns, reject promise with a TypeError.
Cc: -yhirano@chromium.org ricea@chromium.org
Owner: yhirano@chromium.org
Ok, then I will fix that in core/fetch. Thanks!
tsepez@, is this really Security_Severity-High? Do we need to merge the fix to stable?
Project Member

Comment 14 by bugdroid1@chromium.org, May 8

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7712d138374a92c4d2f3b05cdc86d1a7a523702b

commit 7712d138374a92c4d2f3b05cdc86d1a7a523702b
Author: Yutaka Hirano <yhirano@chromium.org>
Date: Tue May 08 10:55:08 2018

ReadableStreamBytesConsumer should check read results

ReadableStreamBytesConsumer expected that the results from
ReadableStreamReaderDefaultRead should be Promise<Object> because that
is provided from ReadableStream provided by blink, but it's possible to
inject arbitrary values with the promise assimilation.

This CL adds additional checks for such injection.

Bug:  840320 
Change-Id: I7b3c6a8bfcf563dd860b133ff0295dd7a5d5fea5
Reviewed-on: https://chromium-review.googlesource.com/1049413
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556751}
[add] https://crrev.com/7712d138374a92c4d2f3b05cdc86d1a7a523702b/third_party/WebKit/LayoutTests/external/wpt/fetch/api/response/response-stream-with-broken-then.any.js
[modify] https://crrev.com/7712d138374a92c4d2f3b05cdc86d1a7a523702b/third_party/blink/renderer/core/fetch/readable_stream_bytes_consumer.cc

Status: Fixed (was: Assigned)
Project Member

Comment 16 by sheriffbot@chromium.org, May 14

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: reward-topanel
Cc: tsepez@chromium.org
+tsepez@ for #13
Labels: Merge-Request-67
Project Member

Comment 20 by sheriffbot@chromium.org, May 16

Labels: -Merge-Request-67 Merge-Review-67 Hotlist-Merge-Review
This bug requires manual review: We are only 12 days from stable.
Please contact the milestone owner if you have questions.
Owners: cmasso@(Android), cmasso@(iOS), kbleicher@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: awhalley@chromium.org
+awhalley@ for M67 merge review (CL listed at #14 landed in trunk on May 8th)
govind@ - good for 67
Labels: -Merge-Review-67 Merge-Approved-67
Approving merge to M67 branch 3396 based on comment #22. Pls merge ASAP. Thank you.
Project Member

Comment 24 by bugdroid1@chromium.org, May 17

Labels: -merge-approved-67 merge-merged-3396
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e20fa6ea657a39d9697c167c3b4c6809b5dbc2e3

commit e20fa6ea657a39d9697c167c3b4c6809b5dbc2e3
Author: Yutaka Hirano <yhirano@chromium.org>
Date: Thu May 17 04:08:39 2018

ReadableStreamBytesConsumer should check read results

ReadableStreamBytesConsumer expected that the results from
ReadableStreamReaderDefaultRead should be Promise<Object> because that
is provided from ReadableStream provided by blink, but it's possible to
inject arbitrary values with the promise assimilation.

This CL adds additional checks for such injection.

TBR=yhirano@chromium.org

(cherry picked from commit 7712d138374a92c4d2f3b05cdc86d1a7a523702b)

Bug:  840320 
Change-Id: I7b3c6a8bfcf563dd860b133ff0295dd7a5d5fea5
Reviewed-on: https://chromium-review.googlesource.com/1049413
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Reviewed-by: Adam Rice <ricea@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#556751}
Reviewed-on: https://chromium-review.googlesource.com/1062992
Reviewed-by: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/branch-heads/3396@{#620}
Cr-Branched-From: 9ef2aa869bc7bc0c089e255d698cca6e47d6b038-refs/heads/master@{#550428}
[add] https://crrev.com/e20fa6ea657a39d9697c167c3b4c6809b5dbc2e3/third_party/WebKit/LayoutTests/external/wpt/fetch/api/response/response-stream-with-broken-then.any.js
[modify] https://crrev.com/e20fa6ea657a39d9697c167c3b4c6809b5dbc2e3/third_party/blink/renderer/core/fetch/readable_stream_bytes_consumer.cc

Labels: -reward-topanel reward-unpaid reward-5000
*** Boilerplate reminders! ***
Please do NOT publicly disclose details until a fix has been released to all our users. Early public disclosure may cancel the provisional reward. Also, please be considerate about disclosure when the bug affects a core library that may be used by other products. Please do NOT share this information with third parties who are not directly involved in fixing the bug. Doing so may cancel the provisional reward. Please be honest if you have already disclosed anything publicly or to third parties. Lastly, we understand that some of you are not interested in money. We offer the option to donate your reward to an eligible charity. If you prefer this option, let us know and we will also match your donation - subject to our discretion. Any rewards that are unclaimed after 12 months will be donated to a charity of our choosing.
*********************************
Thanks higongguang@ - the VRP panel decided to award $5,000 for this report :-)
Labels: -reward-unpaid reward-inprocess
Labels: Release-0-M67
Labels: CVE-2018-6124 CVE_description-missing
Can you let me know when this becomes public? I want to address the issue in the streams standard.
NextAction: 2018-06-06
Hi ricea@ - since it's fixed in 67 which went stable yesterday we could open it up early, as getting it looked at in the standard would be great. We can open it up next week once 67 has gone to 100% and folk have had a chance to upgrade.
The NextAction date has arrived: 2018-06-06
NextAction: 2018-06-13
NextAction: 2018-06-20
The NextAction date has arrived: 2018-06-20
Project Member

Comment 36 by sheriffbot@chromium.org, Aug 20

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