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

Issue 678706 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug-Security



Sign in to add a comment

Potential execution of script inside forbidden scope in Animation

Project Member Reported by adithyas@chromium.org, Jan 5 2017

Issue description

After 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>
 
Cc: alancutter@chromium.org
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


#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.

Comment 4 by suzyh@chromium.org, Jan 5 2017

Cc: suzyh@chromium.org
Labels: Update-Daily
Requesting daily updates on progress on this. Have you got this under control or do you need work from us in SYD?
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.
Labels: -Pri-3 Pri-1
This is a security bug, I'm not sure if it's exploitable yet, but it's P1.
For the record any script running inside a ScriptForbiddenScope is always P1. :)
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.
Labels: -Type-Bug Type-Bug-Security
I have a patch out for review: https://codereview.chromium.org/2615253002
Project Member

Comment 11 by bugdroid1@chromium.org, 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

Status: Fixed (was: Assigned)
Labels: Security_Severity-High M-57 Security_Impact-Stable
Project Member

Comment 14 by sheriffbot@chromium.org, Jan 10 2017

Labels: Restrict-View-SecurityNotify
Project Member

Comment 15 by sheriffbot@chromium.org, Feb 3 2017

Labels: Merge-Request-57
Project Member

Comment 16 by sheriffbot@chromium.org, Feb 3 2017

Labels: -Merge-Request-57 Hotlist-Merge-Approved Merge-Approved-57
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
Labels: -Hotlist-Merge-Approved -Merge-Approved-57
(Merge labels removed because the affected feature is disabled in M57.)
Labels: Release-0-57
Labels: -Release-0-57 Release-0-M57
Project Member

Comment 21 by sheriffbot@chromium.org, Apr 18 2017

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