Security: Security CHECK failed: !ScriptForbiddenScope::isScriptForbidden()
Reported by
cloudfuz...@gmail.com,
Jan 10 2017
|
|||||||||||
Issue description
VULNERABILITY DETAILS
The following testcase crashes the latest ASAN build of chromium as follows:
[15554:15554:0110/085546.604641:1876602239194:FATAL:V8PerIsolateData.cpp(49)] Security CHECK failed: !ScriptForbiddenScope::isScriptForbidden().
#0 0x00000048ff71 __interceptor_backtrace
#1 0x000005a84320 base::debug::StackTrace::StackTrace()
#2 0x000005acc03c logging::LogMessage::~LogMessage()
#3 0x00000a7c80d1 blink::beforeCallEnteredCallback()
#4 0x00000145fef6 v8::Function::NewInstance()
#5 0x00000a7ded41 blink::V8ScriptRunner::instantiateObject()
#6 0x00000a7c0495 blink::V8ObjectConstructor::newInstance()
#7 0x00000a7c18b6 blink::V8PerContextData::createWrapperFromCacheSlowCase()
#8 0x00000a7a04c8 blink::V8DOMWrapper::createWrapper()
#9 0x00000a75a897 blink::ScriptWrappable::wrap()
#10 0x00000b50f90d blink::ScriptPromiseResolver::resolveOrReject<>()
#11 0x00000c5111d8 blink::HTMLMediaElement::rejectPlayPromises()
#12 0x00000c50a1b2 blink::HTMLMediaElement::invokeLoadAlgorithm()
#13 0x00000c509325 blink::HTMLMediaElement::didMoveToNewDocument()
#14 0x00000bc2825a blink::TreeScopeAdopter::moveNodeToNewDocument()
#15 0x00000bc27849 blink::TreeScopeAdopter::moveTreeToNewScope()
#16 0x00000bc1d878 blink::TreeScope::adoptIfNeeded()
#17 0x00000b866d88 blink::ContainerNode::AdoptAndAppendChild::operator()()
#18 0x00000b862f03 blink::ContainerNode::insertNodeVector<>()
#19 0x00000b85e960 blink::ContainerNode::appendChild()
#20 0x00000bad8735 blink::Node::appendChild()
#21 0x00000a9b557b blink::NodeV8Internal::appendChildMethodCallbackForMainWorld()
#22 0x0000013ee19c v8::internal::FunctionCallbackArguments::Call()
#23 0x000001658cfd v8::internal::(anonymous namespace)::HandleApiCallHelper<>()
#24 0x0000016558b3 v8::internal::Builtin_Impl_HandleApiCall()
#25 0x7feb60c84427 <unknown>
Received signal 6
#0 0x00000048ff71 __interceptor_backtrace
#1 0x000005a8323b base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7fecd65d53e0 <unknown>
#3 0x7fecd5de2428 gsignal
#4 0x7fecd5de402a abort
#5 0x000005a7fd9a base::debug::BreakDebugger()
#6 0x000005acc914 logging::LogMessage::~LogMessage()
#7 0x00000a7c80d1 blink::beforeCallEnteredCallback()
#8 0x00000145fef6 v8::Function::NewInstance()
#9 0x00000a7ded41 blink::V8ScriptRunner::instantiateObject()
#10 0x00000a7c0495 blink::V8ObjectConstructor::newInstance()
#11 0x00000a7c18b6 blink::V8PerContextData::createWrapperFromCacheSlowCase()
#12 0x00000a7a04c8 blink::V8DOMWrapper::createWrapper()
#13 0x00000a75a897 blink::ScriptWrappable::wrap()
#14 0x00000b50f90d blink::ScriptPromiseResolver::resolveOrReject<>()
#15 0x00000c5111d8 blink::HTMLMediaElement::rejectPlayPromises()
#16 0x00000c50a1b2 blink::HTMLMediaElement::invokeLoadAlgorithm()
#17 0x00000c509325 blink::HTMLMediaElement::didMoveToNewDocument()
#18 0x00000bc2825a blink::TreeScopeAdopter::moveNodeToNewDocument()
#19 0x00000bc27849 blink::TreeScopeAdopter::moveTreeToNewScope()
#20 0x00000bc1d878 blink::TreeScope::adoptIfNeeded()
#21 0x00000b866d88 blink::ContainerNode::AdoptAndAppendChild::operator()()
#22 0x00000b862f03 blink::ContainerNode::insertNodeVector<>()
#23 0x00000b85e960 blink::ContainerNode::appendChild()
#24 0x00000bad8735 blink::Node::appendChild()
#25 0x00000a9b557b blink::NodeV8Internal::appendChildMethodCallbackForMainWorld()
#26 0x0000013ee19c v8::internal::FunctionCallbackArguments::Call()
#27 0x000001658cfd v8::internal::(anonymous namespace)::HandleApiCallHelper<>()
#28 0x0000016558b3 v8::internal::Builtin_Impl_HandleApiCall()
#29 0x7feb60c84427 <unknown>
r8: 000000008fff6fff r9: 0000000000000000 r10: 0000000000000008 r11: 0000000000000202
r12: 00007feb86c45ae0 r13: 0000000000000000 r14: 00007feb86c45800 r15: 00007feb86a2ea20
di: 0000000000003cc2 si: 0000000000003cc2 bp: 00007ffcf4a0baf0 bx: 00007ffcf4a0bb00
dx: 0000000000000006 ax: 0000000000000000 cx: 00007fecd5de2428 sp: 00007ffcf4a0b9b8
ip: 00007fecd5de2428 efl: 0000000000000202 cgf: 0000000000000033 erf: 0000000000000000
trp: 0000000000000000 msk: 0000000000000000 cr2: 0000000000000000
[end of stack trace]
VERSION
Chrome Version: asan-linux-release-434865
REPRODUCTION CASE
<script>
function start() {
o4=document.createElementNS('http://www.w3.org/1999/xhtml','table');
o9=document.createElementNS('http://www.w3.org/1999/xhtml','frameset');
try{while(window.top.document.removeChild(window.top.document.firstChild));}catch(e){}
o45=window.top.document.implementation.createHTMLDocument();
o45.body.appendChild(o4);
window.top.document.appendChild(o45.documentElement);
o4.appendChild(o9);
o53=document.createElementNS('http://www.w3.org/1999/xhtml','frame');
o86=document.createElementNS('http://www.w3.org/1999/xhtml','iframe');
o53.appendChild(o86);
o9.appendChild(o53);
o215=window.top.frames[1];
o216=o215.document;
o217=o216.createElementNS('http://www.w3.org/1999/xhtml','audio');
o217.play();
o233=document.createElementNS('http://www.w3.org/1999/xhtml','a');
o233.appendChild(o217);
}
</script>
<body onload="start()"></body>
FOR CRASHES, PLEASE INCLUDE THE FOLLOWING ADDITIONAL INFORMATION
Type of crash: tab
,
Jan 17 2017
CCing yangguo, from the git blame of the fatal check, in case the Clusterfuzz triage queue doesn't have read access to security bugs. I'm not sure what severity or impact of this is. Can someone from V8 help assess the severity of that check being triggered? Thanks.
,
Jan 17 2017
There have been some changes to the blink macros recently, but this is a RELEASE_ASSERT which should always lead to a safe crash in production builds. Some of our comments and documentation related to it are a bit off. We don't usually care about these from a security standpoint for that reason, though they are still crash bugs. ClusterFuzz historically hasn't generated reports for these to reduce noise, but we should probably fix that. That's why the above reports are marked as unreproducible. If you check the reports, you can see that we are reproducing the crash. Filed issue 681750 to track these problems, and flipping the labels on this one to a crash bug.
,
Jan 17 2017
Jochen, please guide us!
,
Jan 17 2017
Elliott, the ToV8(value) before the check for the script forbidden scope you added can also execute script :/
,
Jan 17 2017
How is that creating script in this example? Looks like it's creating a wrapper which should be marked as safe with an AllowUserAgentScript scope?
,
Jan 17 2017
,
Jan 17 2017
yes, it's calling "new DOMException()" which is safe
,
Jan 17 2017
esprehn/jochen, it sounds like AllowUserAgentScript in V8DOMWrapper::createWrapper or V8WrapperInstantiationScope would be safe (because in that path, we're technically "invoking the constructor" but nothing that can actually run author script should be running). Right? esprehn, is that something you're planning on doing (you're the assignee right now), or did you want Adithya to do this?
,
Jan 17 2017
If adithyas@ could take this that'd be great!
,
Jan 18 2017
> esprehn/jochen, it sounds like AllowUserAgentScript in V8DOMWrapper::createWrapper or V8WrapperInstantiationScope would be safe (because in that path, we're technically "invoking the constructor" but nothing that can actually run author script should be running). Right? I'm not a JS expert, but is there really no way to run author script in the constructor? In any case, I don't think it's helpful to insert AllowUserAgentScript to V8DOMWrapper::createWrapper because then we will pass toV8 but crash when we actually resolve/reject the promise. Maybe should we move this check (https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/v8/ScriptPromiseResolver.h?q=ScriptPromiseResolver::resolveOrReject&sq=package:chromium&dr=Ss&l=144) up to line 127?
,
Jan 18 2017
AFAIK these should only be the constructors that are created in our bindings (from the information in WrapperTypeInfo). Since ConstructorMode isn't on, it shouldn't even convert any of the arguments. If we _can_ run author script just while wrapping an existing DOM object, that's surprising. The alternative, I think, is to defer the conversion until the microtasks are running, but that seems harder to do while preserving correct ordering of things. The timer in that class, should eventually go away and be replaced with a CHECK.
,
Jan 18 2017
I do worry that putting an isMainThread() check inside every wrapper creation is going to be expensive. So perhaps moving the forbidden scope check higher is a better approach?
,
Jan 18 2017
Just moving the forbidden scope check higher wouldn't work on its own. resolveOrRejectImmediately (the function called when the timer fires) expects the wrapper to have already been created, so we would also need to move the toV8 call inside resolveOrRejectImmediately, which is more complicated.
,
Jan 19 2017
Why is it hard to move toV8 to onTimerFired()? Maybe can we create a helper method that does the following three things and call it from onTimerFired() and resolveOrReject(). - ScriptState::Scope - toV8 - resolveOrRejectImmediately
,
Jan 19 2017
The value the promise is being rejected/resolved with needs to be passed to onTimerFired() somehow. That's currently being done by creating the wrapper and storing it in m_value. We cannot store the value itself in another member variable because the value's type is not well defined (it's any type that ToV8 can be called with).
,
Jan 19 2017
I think you can hide the type inside the WTF::bind() parameters. Fwiw to fix this right now lets just put an AllowUserAgentScript scope on the stack in a block around the ToV8 call inside that one function. It avoids the crash and doesn't impact wrapper creation perf.
,
Jan 19 2017
#17: as far as bind goes, it's still non-trivial, since T may be, at least: - a pointer to a DOM object (which should be wrapped with wrapPersistent) - a primitive - a vector (which will end up being implicitly copied an extra time in that case)
,
Feb 7 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b81015e872d51319648e448129becd5a19799848 commit b81015e872d51319648e448129becd5a19799848 Author: adithyas <adithyas@chromium.org> Date: Tue Feb 07 20:07:21 2017 Add AllowUserAgentScript in ScriptPromiseResolver::resolveOrReject BUG= 679648 ,676004 Review-Url: https://codereview.chromium.org/2644343002 Cr-Commit-Position: refs/heads/master@{#448711} [modify] https://crrev.com/b81015e872d51319648e448129becd5a19799848/third_party/WebKit/Source/bindings/core/v8/ScriptPromiseResolver.h [modify] https://crrev.com/b81015e872d51319648e448129becd5a19799848/third_party/WebKit/Source/core/html/HTMLMediaElement.cpp
,
May 11 2017
,
May 12 2017
,
Aug 18 2017
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 |
|||||||||||
Comment 1 by ClusterFuzz
, Jan 11 2017