Issue metadata
Sign in to add a comment
|
Potential execution of script inside forbidden scope in Animation |
||||||||||||||||||||||
Issue descriptionAfter adding CHECK(!ScriptForbiddenScope::isScriptForbidden()) inside ScriptPromiseProperty::resolve, a number of animation related layout tests fail due to the assert being triggered. This indicates that promises are being resolved inside a forbidden scope (which means script could potentially be executed inside a forbidden scope). Stack trace from web-animations-api/animation-ready-promise.html: STDERR: [1:1:0105/142031.404897:1223641017798:FATAL:ScriptPromiseProperty.h(108)] Check failed: !ScriptForbiddenScope::isScriptForbidden(). STDERR: #0 0x7f5920dda6be base::debug::StackTrace::StackTrace() STDERR: #1 0x7f5920e47cbf logging::LogMessage::~LogMessage() STDERR: #2 0x7f5917232970 blink::ScriptPromiseProperty<>::resolve<>() STDERR: #3 0x7f5917230d74 blink::Animation::resolveOrRejectReadyAndFinishedPromises() STDERR: #4 0x7f591723089f blink::Animation::PlayStateUpdateScope::~PlayStateUpdateScope() STDERR: #5 0x7f591722c3de blink::Animation::preCommit() STDERR: #6 0x7f59172cce03 blink::CompositorPendingAnimations::update() STDERR: #7 0x7f59172d599d blink::DocumentAnimations::updateAnimations() STDERR: #8 0x7f59181f58ef blink::PaintLayerCompositor::updateIfNeededRecursiveInternal() STDERR: #9 0x7f59181f55c8 blink::PaintLayerCompositor::updateIfNeededRecursive() STDERR: #10 0x7f5917acb07e blink::FrameView::updateLifecyclePhasesInternal() STDERR: #11 0x7f5917acaa72 blink::FrameView::updateAllLifecyclePhases() STDERR: #12 0x7f59183a692b blink::PageAnimator::updateAllLifecyclePhases() STDERR: #13 0x7f591a4ad695 blink::PageWidgetDelegate::updateAllLifecyclePhases() STDERR: #14 0x7f591a5bf9d4 blink::WebViewImpl::updateAllLifecyclePhases() STDERR: #15 0x7f5919fc0585 test_runner::WebWidgetTestClient::AnimateNow() STDERR: #16 0x7f5919f1d247 _ZN4base8internal13FunctorTraitsIMN11test_runner16MockColorChooserEFvvEvE6InvokeIRKNS_7WeakPtrIS3_EEJEEEvS5_OT_DpOT0_ STDERR: #17 0x7f5919fc0a4a _ZN4base8internal12InvokeHelperILb1EvE8MakeItSoIRKMN11test_runner19WebWidgetTestClientEFvvERKNS_7WeakPtrIS5_EEJEEEvOT_OT0_DpOT1_ STDERR: #18 0x7f5919fc09d2 _ZN4base8internal7InvokerINS0_9BindStateIMN11test_runner19WebWidgetTestClientEFvvEJNS_7WeakPtrIS4_EEEEEFvvEE7RunImplIRKS6_RKSt5tupleIJS8_EEJLm0EEEEvOT_OT0_NS_13IndexSequenceIJXspT1_EEEE STDERR: #19 0x7f5919fc091c _ZN4base8internal7InvokerINS0_9BindStateIMN11test_runner19WebWidgetTestClientEFvvEJNS_7WeakPtrIS4_EEEEEFvvEE3RunEPNS0_13BindStateBaseE STDERR: #20 0x7f5920de04b1 _ZNO4base8internal8RunMixinINS_8CallbackIFvvELNS0_8CopyModeE0ELNS0_10RepeatModeE0EEEE3RunEv STDERR: #21 0x7f5920ddfe82 base::debug::TaskAnnotator::RunTask() STDERR: #22 0x7f591ae9c6ea blink::scheduler::TaskQueueManager::ProcessTaskFromWorkQueue() STDERR: #23 0x7f591ae9a111 blink::scheduler::TaskQueueManager::DoWork() STDERR: #24 0x7f591aea2bac _ZN4base8internal13FunctorTraitsIMN5blink9scheduler16TaskQueueManagerEFvNS_9TimeTicksEbEvE6InvokeIRKNS_7WeakPtrIS4_EEJRKS5_RKbEEEvS7_OT_DpOT0_ STDERR: #25 0x7f591aea2a84 _ZN4base8internal12InvokeHelperILb1EvE8MakeItSoIRKMN5blink9scheduler16TaskQueueManagerEFvNS_9TimeTicksEbERKNS_7WeakPtrIS6_EEJRKS7_RKbEEEvOT_OT0_DpOT1_ STDERR: #26 0x7f591aea29e4 _ZN4base8internal7InvokerINS0_9BindStateIMN5blink9scheduler16TaskQueueManagerEFvNS_9TimeTicksEbEJNS_7WeakPtrIS5_EES6_bEEEFvvEE7RunImplIRKS8_RKSt5tupleIJSA_S6_bEEJLm0ELm1ELm2EEEEvOT_OT0_NS_13IndexSequenceIJXspT1_EEEE STDERR: #27 0x7f591aea28bc _ZN4base8internal7InvokerINS0_9BindStateIMN5blink9scheduler16TaskQueueManagerEFvNS_9TimeTicksEbEJNS_7WeakPtrIS5_EES6_bEEEFvvEE3RunEPNS0_13BindStateBaseE STDERR: #28 0x7f5920de04b1 _ZNO4base8internal8RunMixinINS_8CallbackIFvvELNS0_8CopyModeE0ELNS0_10RepeatModeE0EEEE3RunEv STDERR: #29 0x7f5920ddfe82 base::debug::TaskAnnotator::RunTask() STDERR: #30 0x7f5920e70dda base::MessageLoop::RunTask() STDERR: #31 0x7f5920e71064 base::MessageLoop::DeferOrRunPendingTask() STDERR: #32 0x7f5920e7134e base::MessageLoop::DoWork() STDERR: #33 0x7f5920e88ae3 base::MessagePumpDefault::Run() STDERR: #34 0x7f5920e7095a base::MessageLoop::RunHandler() STDERR: #35 0x7f5920f1e1b2 base::RunLoop::Run() STDERR: #36 0x7f5923c3d91c content::RendererMain() STDERR: #37 0x7f592404b13e content::RunZygote() STDERR: #38 0x7f592404b4f0 content::RunNamedProcessTypeMain() STDERR: #39 0x7f592404d8eb content::ContentMainRunnerImpl::Run() STDERR: #40 0x7f592404a7e2 content::ContentMain() STDERR: #41 0x000000492761 main STDERR: #42 0x7f5912d0bf45 __libc_start_main STDERR: #43 0x000000492635 <unknown>
,
Jan 5 2017
Yeah those need to be changed to resolveAsync() and someone should review the spec too. We already post a task to avoid the security problem: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/bindings/core/v8/ScriptPromiseResolver.h?q=ScriptPromiseResolver&sq=package:chromium&l=144
,
Jan 5 2017
#2: I don't think ScriptPromiseProperty routes through ScriptPromiseResolver::resolveOrReject (not sure why). IMHO once all the cases we can find are fixed both places should have a RELEASE_ASSERT or CHECK.
,
Jan 5 2017
Requesting daily updates on progress on this. Have you got this under control or do you need work from us in SYD?
,
Jan 6 2017
adithyas: Could you elaborate on the severity of this failure? Is it a security issue or more on the side of code health? I question whether Update-Daily is needed given this was filed as P3.
,
Jan 6 2017
This is a security bug, I'm not sure if it's exploitable yet, but it's P1.
,
Jan 6 2017
For the record any script running inside a ScriptForbiddenScope is always P1. :)
,
Jan 6 2017
alancutter: Sorry, it is a security bug. I forgot to change the priority from the default. suzyh: I will be looking at this tomorrow, I'll post an update.
,
Jan 6 2017
,
Jan 6 2017
I have a patch out for review: https://codereview.chromium.org/2615253002
,
Jan 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ffb284d85f2e725aefb2314d436addf9431622ad commit ffb284d85f2e725aefb2314d436addf9431622ad Author: adithyas <adithyas@chromium.org> Date: Mon Jan 09 17:04:27 2017 Resolve ready and finished promises asynchronously ~PlayStateUpdateScope() can be called inside a ScriptForbiddenScope, and resolving promises synchronously in ~PlayStateUpdateScope could result in script execution inside a forbidden scope. This patch makes promise resolution inside the destructor asynchronous. The DCHECK added crashes the following tests (when the promises are resolved synchronously): web-animations-api/animation-ready-promise.html animations/filter-responsive-neutral-keyframe.html web-animations-api/playState-changes.html imported/wpt/web-animations/interfaces/Animation/finished.html imported/wpt/web-animations/animation-model/keyframe-effects/effect-value-context.html web-animations-api/startTime.html imported/wpt/web-animations/interfaces/Animation/playState.html animations/viewport-unit-animation-responsive.html imported/wpt/web-animations/interfaces/Animation/cancel.html animations/zoom-responsive-transform-animation.html imported/wpt/web-animations/interfaces/Animation/play.html animations/transform-responsive-neutral-keyframe.html animations/opacity-responsive-neutral-keyframe.html imported/wpt/web-animations/interfaces/Animation/onfinish.html imported/wpt/web-animations/timing-model/animations/updating-the-finished-state.html transitions/opacity-transition-zindex.html transitions/opacity-transform-transitions-inside-iframe.html virtual/threaded/animations/transitions-retarget.html imported/wpt/web-animations/interfaces/Animation/startTime.html imported/wpt/web-animations/interfaces/Animation/playbackRate.html imported/wpt/web-animations/interfaces/Animation/pause.html web-animations-api/animation-set-timeline.html BUG= 678706 Review-Url: https://codereview.chromium.org/2615253002 Cr-Commit-Position: refs/heads/master@{#442272} [modify] https://crrev.com/ffb284d85f2e725aefb2314d436addf9431622ad/third_party/WebKit/Source/bindings/core/v8/ScriptPromiseProperty.h [modify] https://crrev.com/ffb284d85f2e725aefb2314d436addf9431622ad/third_party/WebKit/Source/core/animation/Animation.cpp [modify] https://crrev.com/ffb284d85f2e725aefb2314d436addf9431622ad/third_party/WebKit/Source/core/animation/Animation.h
,
Jan 9 2017
,
Jan 10 2017
,
Jan 10 2017
,
Feb 3 2017
,
Feb 3 2017
Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions. Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Feb 6 2017
,
Feb 6 2017
(Merge labels removed because the affected feature is disabled in M57.)
,
Mar 6 2017
,
Mar 6 2017
,
Apr 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 jbroman@chromium.org
, Jan 5 2017