Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Issue 655904 Security: Universal XSS via fullscreen element updates
Starred by 3 users Reported by marius.mlynski@gmail.com, Oct 14 2016 Back to list
Status: Fixed
Owner:
Closed: Oct 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security



Sign in to add a comment
VULNERABILITY DETAILS
From /third_party/WebKit/Source/core/dom/Fullscreen.cpp:
----------------
void Fullscreen::didEnterFullscreenForElement(Element* element) {
(...)
  // FIXME: This should not call updateStyleAndLayoutTree.
  document()->updateStyleAndLayoutTree();
(...)
}
----------------

Indeed. |didEnterFullscreenForElement| may be called in the middle of DOM node removal if the node being removed is the active fullscreen element and there are other fullscreen elements on the Fullscreen::m_fullscreenElementStack (see Fullscreen::exitFullscreen()). In specific circumstances, when the document's focused node is in a <use> shadow tree with a scheduled update, this synchronous layout update may result in events being dispatched at a wrong time, which allows an attacker to corrupt the DOM tree.

VERSION
Chrome 54.0.2840.59 (Stable)
Chrome 54.0.2840.59 (Beta)
Chrome 55.0.2883.11 (Dev)
Chromium 56.0.2890.0 (Release build compiled today)
 
exploit.zip
4.5 KB Download
Comment 1 by mmoroz@chromium.org, Oct 14 2016
Cc: mmoroz@chromium.org
Components: Blink>DOM
Labels: Security_Severity-High Security_Impact-Stable OS-All Pri-1
Owner: jochen@chromium.org
Status: Available
Thanks for your report! That's cool.
Project Member Comment 2 by sheriffbot@chromium.org, Oct 14 2016
Labels: M-54
Project Member Comment 3 by sheriffbot@chromium.org, Oct 14 2016
Status: Assigned
Comment 4 by jochen@chromium.org, Oct 14 2016
Cc: jochen@chromium.org
Owner: esprehn@chromium.org
Blockedon: 656160
Cc: haraken@chromium.org pdr@chromium.org foolip@chromium.org
Components: -Blink>DOM Blink>SVG
If you reduce the test case a bunch you correctly hit the ScriptForbiddenScope which should mitigate these attacks (the crash is still a bug, but it's not a security bug). 

What seems so serious here is that somehow the page is getting JS to execute and *not* hit the ScriptForbiddenScope? :(

I don't understand how the full screen API is related, what's happening inside the full screen code that creates the UXSS?
Owner: foolip@chromium.org
Comment 7 by aarya@google.com, Oct 14 2016
Cc: esprehn@chromium.org
The fullscreen code triggers a layout update, and that can fire events in the middle of child node removal -- if you run the exploit in a debug build, it should hit an EventDispatchForbiddenScope assertion with a stack like this:

[1:1:1012/225739:FATAL:EventDispatcher.cpp(56)] Check failed: !EventDispatchForbiddenScope::isEventDispatchForbidden(). 
#0 0x7f2901a8ee4e base::debug::StackTrace::StackTrace()
#1 0x7f2901afcc8f logging::LogMessage::~LogMessage()
#2 0x7f28e943ac36 blink::EventDispatcher::dispatchEvent()
#3 0x7f28e91f4a24 blink::Node::dispatchEventInternal()
#4 0x7f28e945489c blink::EventTarget::dispatchEvent()
#5 0x7f28e91758ef blink::Element::dispatchBlurEvent()
#6 0x7f28e9e0f16b blink::SVGAElement::dispatchBlurEvent()
#7 0x7f28e9105224 blink::Document::setFocusedElement()
#8 0x7f28e9104aea blink::Document::clearFocusedElement()
#9 0x7f28e9104aa5 blink::Document::removeFocusedElementOfSubtree()
#10 0x7f28e90bce65 blink::ContainerNode::removeChildren()
#11 0x7f28e9ebb0f2 blink::SVGUseElement::clearShadowTree()
#12 0x7f28e9ebc1ca blink::SVGUseElement::buildPendingResource()
#13 0x7f28e90fa214 blink::Document::updateUseShadowTreesIfNeeded()
#14 0x7f28e90f686c blink::Document::updateStyleAndLayoutTree()
#15 0x7f28e91a68d8 blink::Fullscreen::didEnterFullscreenForElement()
#16 0x7f28f2d69a72 blink::FullscreenController::didEnterFullscreen()
#17 0x7f28f2d69f0f blink::FullscreenController::enterFullscreenForElement()
#18 0x7f28f2e7feef blink::WebViewImpl::enterFullscreenForElement()
#19 0x7f28f2d2b681 blink::ChromeClientImpl::enterFullscreenForElement()
#20 0x7f28e91a61a8 blink::Fullscreen::exitFullscreen()
#21 0x7f28e91a710e blink::Fullscreen::elementRemoved()
#22 0x7f28e916ea54 blink::Element::removedFrom()
#23 0x7f28e90bcd60 blink::ContainerNode::notifyNodeRemoved()
#24 0x7f28e90bcf14 blink::ContainerNode::removeChildren()
#25 0x7f28e91f055f blink::Node::setTextContent()
#26 0x7f28ea150609 blink::NodeV8Internal::textContentAttributeSetter()
#27 0x7f28ea150510 blink::NodeV8Internal::textContentAttributeSetterCallback()
#28 0x7f28f733615d v8::internal::FunctionCallbackArguments::Call()

Events are undesirable in ScriptForbiddenScopes -- the dispatch can call into V8 in the process, for example to instantiate a JS event wrapper (this is probably the crash you're seeing) or to fetch a function from an event listener object (this is where you see JS execute).
Here's a fix for the ScriptForbiddenScope bypass: https://codereview.chromium.org/2423623002

document.addEventListener("blur", {get handleEvent() { 
  /* this getter will run */
  return function() { /* this won't run */ };
}});

We won't run the event listener because of the early return in V8ScriptRunner::callFunction, but we will still call the getter.

I do wonder if v8 should be running the beforeCallEnteredCallback in this case? We run it when doing micro tasks but not around getters/setters which can run arbitrary code too.
I would say that the core problem is that both Fullscreen::requestFullscreen() and exitFullscreen() take a synchronous shortcut for nested fullscreen cases, in this case the Fullscreen::didEnterFullscreenForElement().

Per spec, calls to requestFullscreen() and exitFullscreen() should synchronously merely determine whether to resize the window or not. If we shouldn't which is the case here, we should wait until the next animation frame to actually do the work of changing the fullscreen element stack, firing events and fiddling with the layout. I'm treating  issue 402421  as tracking that, because to fix those timing issues is precisely what it would take to make webkitCurrentFullScreenElement an alias.

esprehn@, do you think that https://codereview.chromium.org/2423623002 is a sufficient mitigation for this that could be backported? The work that I still have on a branch for  issue 402421  might also fix this, but it comes with compat risk and not a great idea to backport I think.
Project Member Comment 11 by bugdroid1@chromium.org, Oct 17 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/610b88604db99184334982ab982d758296718879

commit 610b88604db99184334982ab982d758296718879
Author: esprehn <esprehn@chromium.org>
Date: Mon Oct 17 20:10:35 2016

Don't run handleEvent getter in V8EventListener::getListenerFunction if script is forbidden.

It results in arbitrary code execution under ScriptForbiddenScopes. :(

BUG= 655904 

Review-Url: https://codereview.chromium.org/2423623002
Cr-Commit-Position: refs/heads/master@{#425763}

[modify] https://crrev.com/610b88604db99184334982ab982d758296718879/third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp

Labels: Merge-Request-55
Project Member Comment 13 by sheriffbot@chromium.org, Oct 22 2016
Status: Fixed
Please mark security bugs as fixed as soon as the fix lands, and before requesting merges. This update is based on the merge- labels applied to this issue. Please reopen if this update was incorrect.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member Comment 14 by sheriffbot@chromium.org, Oct 23 2016
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: reward-topanel
Comment 16 by dimu@google.com, Oct 24 2016
Labels: -Merge-Request-55 Merge-Review-55 Hotlist-Merge-Review
[Automated comment] There appears to be on-going work (i.e. bugroid changes), needs manual review.
Good for M55.
Labels: -Merge-Review-55 Merge-Approved-55
Approving merge to M55 branch 2883 per comment #17. Please merge ASAP. Thank you.
Project Member Comment 19 by bugdroid1@chromium.org, Oct 24 2016
Labels: -merge-approved-55 merge-merged-2883
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e4aa8931b775f144de76b4ae3f335348f8e1e0ff

commit e4aa8931b775f144de76b4ae3f335348f8e1e0ff
Author: Philip J├Ągenstedt <foolip@chromium.org>
Date: Mon Oct 24 20:47:43 2016

Don't run handleEvent getter in V8EventListener::getListenerFunction if script is forbidden.

It results in arbitrary code execution under ScriptForbiddenScopes. :(

BUG= 655904 

Review-Url: https://codereview.chromium.org/2423623002
Cr-Commit-Position: refs/heads/master@{#425763}
(cherry picked from commit 610b88604db99184334982ab982d758296718879)

Review URL: https://codereview.chromium.org/2449623002 .

Cr-Commit-Position: refs/branch-heads/2883@{#259}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/e4aa8931b775f144de76b4ae3f335348f8e1e0ff/third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp

Labels: -reward-topanel reward-unpaid reward-7500
Very nice, $7,500 for this report :-)
Project Member Comment 22 by bugdroid1@chromium.org, Oct 27 2016
Labels: merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e4aa8931b775f144de76b4ae3f335348f8e1e0ff

commit e4aa8931b775f144de76b4ae3f335348f8e1e0ff
Author: Philip J├Ągenstedt <foolip@chromium.org>
Date: Mon Oct 24 20:47:43 2016

Don't run handleEvent getter in V8EventListener::getListenerFunction if script is forbidden.

It results in arbitrary code execution under ScriptForbiddenScopes. :(

BUG= 655904 

Review-Url: https://codereview.chromium.org/2423623002
Cr-Commit-Position: refs/heads/master@{#425763}
(cherry picked from commit 610b88604db99184334982ab982d758296718879)

Review URL: https://codereview.chromium.org/2449623002 .

Cr-Commit-Position: refs/branch-heads/2883@{#259}
Cr-Branched-From: 614d31daee2f61b0180df403a8ad43f20b9f6dd7-refs/heads/master@{#423768}

[modify] https://crrev.com/e4aa8931b775f144de76b4ae3f335348f8e1e0ff/third_party/WebKit/Source/bindings/core/v8/V8EventListener.cpp

Labels: -reward-unpaid reward-inprocess
Cc: adamk@chromium.org tkent@chromium.org dcheng@chromium.org
tkent@ adamk@ dcheng@ I think there's still a bug here, how was running script in the middle of the full screen code causing a UXSS like this? The ScriptForbiddenScope plugs the hole, but I'm not sure I understand how full screen was related here.
The problem is the fullscreen code is getting invoked in the middle of DOM mutation. Running JS in the middle of DOM tree mutation is a primitive that's been leveraged to achieve UXSS several times in the past by corrupting the DOM tree...

I'll try to find some other UXSS bugs that abused the same primitive; I think one of them might have an explanation of how DOM tree corruption = UXSS.
[Automated comment] removing mislabelled merge-merged-2840
Labels: -merge-merged-2840
[Automated comment] removing mislabelled merge-merged-2840
I recently fixed a similar security issue,  Issue 658535 .
I think we can make more improvement.

* ScriptForbiddenScope actually forbids script in release build, but EventDispatchForbiddenScope doesn't affect release build.  EventDispatchForbiddenScope should imply ScriptForbiddenScope.

* Updating <use> Shadow DOM in updateStyleAndLayoutTree sounds very bad.

* Both of this issue and  Issue 658535  load content of a disconnected iframe, and bypass cross-origin check.  We should investigate why the bypass is possible, and should make the code defensive.

> tkent@ said:
> * Updating <use> Shadow DOM in updateStyleAndLayoutTree sounds very bad.

That was my design, it's a step right before we update style. Unfortunately the web requires it because it assumes that use trees are always synchronously updated, so we either need to update all use trees inside every DOM mutation (slow), or we need to do them async whenever we update style so getBoundingClientRect() and getComputedStyle() return the right answer. 
Labels: M-55
Labels: Release-0-M55
Labels: CVE-2016-5207
Project Member Comment 33 by sheriffbot@chromium.org, Jan 28
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