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

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security



Sign in to add a comment
link

Issue 621362: Security: Universal XSS with Flash calling into JavaScript inside Node::removedFrom

Reported by serg.gla...@gmail.com, Jun 19 2016

Issue description

VULNERABILITY DETAILS
|Node::removedFrom()| is called while the DOM tree is in an inconsistent state and therefore is not supposed
to run user JavaScript code. However, HTMLPlugInElement overrides |removedFrom()| to call |Widget::dispose()|,
and  issue 546545  demonstrates how the Flash plugin can be used to call JavaScript code inside |dispose()|.

src/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp:85:
void HTMLPlugInElement::setPersistedPluginWidget(Widget* widget)
{
    if (m_persistedPluginWidget == widget)
        return;
    if (m_persistedPluginWidget) {
        if (m_persistedPluginWidget->isPluginView()) {
            m_persistedPluginWidget->hide();
            m_persistedPluginWidget->dispose();
        } else {
            ASSERT(m_persistedPluginWidget->isFrameView() || m_persistedPluginWidget->isRemoteFrameView());
        }
    }
    m_persistedPluginWidget = widget;
}

[...]

void HTMLPlugInElement::removedFrom(ContainerNode* insertionPoint)
{
    if (m_persistedPluginWidget) {
        HTMLFrameOwnerElement::UpdateSuspendScope suspendWidgetHierarchyUpdates;
        setPersistedPluginWidget(nullptr);
    }
    HTMLFrameOwnerElement::removedFrom(insertionPoint);
}

Note that |HTMLFrameOwnerElement::UpdateSuspendScope| doesn't defer the disposal of |m_persistedPluginWidget|.

Also, in this case, unlike  issue 546545 , it is not possible to use |ExternalInterface.call()| because
|ScriptForbiddenScope| would block the script execution:

src/third_party/WebKit/Source/core/dom/ContainerNode.cpp:728:
void ContainerNode::notifyNodeRemoved(Node& root)
{
    ScriptForbiddenScope forbidScript;
    EventDispatchForbiddenScope assertNoEventDispatch;

    for (Node& node : NodeTraversal::inclusiveDescendantsOf(root)) {
        if (!node.isContainerNode() && !node.isInTreeScope())
            continue;
        node.removedFrom(this);
        for (ShadowRoot* shadowRoot = node.youngestShadowRoot(); shadowRoot; shadowRoot = shadowRoot->olderShadowRoot())
            notifyNodeRemoved(*shadowRoot);
    }
}

Instead, the repro case defines an [object-element-name]_DoFSCommand getter on the global object in JavaScript
and calls |fscommand()| in Flash.

The code that turns a corrupted DOM tree into a UXSS bug is copied from issue 456518.

VERSION
Google Chrome 51.0.2704.103 (Official Build) m (64-bit)
Google Chrome 53.0.2772.0 (Official Build) canary (64-bit)

--

I would like to remain anonymous for this report.
 
repro.zip
2.0 KB Download

Comment 1 by est...@chromium.org, Jun 20 2016

Components: Blink>HTML
Labels: Security_Severity-High Security_Impact-Stable

Comment 2 by est...@chromium.org, Jun 20 2016

Owner: dcheng@chromium.org
dcheng, do you think you could suggest an owner for this? Thanks.

Comment 3 by ClusterFuzz, Jun 20 2016

Project Member
Status: Assigned (was: Unconfirmed)

Comment 4 by sheriffbot@chromium.org, Jun 21 2016

Project Member
Labels: M-51

Comment 5 by sheriffbot@chromium.org, Jun 21 2016

Project Member
Labels: Pri-1

Comment 6 by dcheng@chromium.org, Jun 21 2016

Cc: haraken@chromium.org sigbjo...@opera.com

Comment 7 by dcheng@chromium.org, Jun 21 2016

Cc: esprehn@chromium.org
Incidentally, this is yet another reason letting plugins synchronously script during teardown is turning out very very poorly.

I wonder if we can force the dispose earlier, like when ChildFrameDisconnector is tearing down frames attached to nodes that are being removed from the DOM…

Comment 8 by dcheng@chromium.org, Jun 21 2016

Cc: bbudge@chromium.org piman@chromium.org
+piman, +bbudge for a separate thread I'm going to start to see if we can get rid of this insanity altogether. We'll have to figure out another patch in the interim though.

Comment 9 by sheriffbot@chromium.org, Jul 6 2016

Project Member
dcheng: Uh oh! This issue still open and hasn't been updated in the last 14 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 10 by ta...@google.com, Jul 13 2016

Labels: OS-All

Comment 11 by bugdroid1@chromium.org, Jul 13 2016

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7bf106f2192e922c18f18cac4ae18d79ea90c1b0

commit 7bf106f2192e922c18f18cac4ae18d79ea90c1b0
Author: dcheng <dcheng@chromium.org>
Date: Wed Jul 13 05:15:53 2016

Use ChildFrameDisconnector when detaching child frames of a LocalFrame.

Currently, UpdateSuspendScope is used to defer widget updates when the
DOM hierarchy is mutated. This is used to prevent script from running
in the middle of DOM mutations, since plugins can run script when
destroyed.

This is part 1 of 2 CLs to remove the need for UpdateSuspendScope. Part
1 changes LocalFrame detach to always use ChildFrameDisconnector to
detach child frames. Part 2 will rework ChildFrameDisconnector to also
detach plugin elements. This should eliminate the need to defer widget
updates, since script will never have to run during the actual mutation
of internal state.

BUG=524113,528867,561683, 621362 

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

[modify] https://crrev.com/7bf106f2192e922c18f18cac4ae18d79ea90c1b0/third_party/WebKit/Source/core/frame/Frame.cpp
[modify] https://crrev.com/7bf106f2192e922c18f18cac4ae18d79ea90c1b0/third_party/WebKit/Source/core/frame/Frame.h
[modify] https://crrev.com/7bf106f2192e922c18f18cac4ae18d79ea90c1b0/third_party/WebKit/Source/core/frame/LocalFrame.cpp
[modify] https://crrev.com/7bf106f2192e922c18f18cac4ae18d79ea90c1b0/third_party/WebKit/Source/core/frame/LocalFrame.h
[modify] https://crrev.com/7bf106f2192e922c18f18cac4ae18d79ea90c1b0/third_party/WebKit/Source/core/frame/RemoteFrame.cpp
[modify] https://crrev.com/7bf106f2192e922c18f18cac4ae18d79ea90c1b0/third_party/WebKit/Source/core/frame/RemoteFrame.h

Comment 12 by bugdroid1@chromium.org, Jul 13 2016

Project Member
Labels: merge-merged-2795
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/7bf106f2192e922c18f18cac4ae18d79ea90c1b0

commit 7bf106f2192e922c18f18cac4ae18d79ea90c1b0
Author: dcheng <dcheng@chromium.org>
Date: Wed Jul 13 05:15:53 2016

Use ChildFrameDisconnector when detaching child frames of a LocalFrame.

Currently, UpdateSuspendScope is used to defer widget updates when the
DOM hierarchy is mutated. This is used to prevent script from running
in the middle of DOM mutations, since plugins can run script when
destroyed.

This is part 1 of 2 CLs to remove the need for UpdateSuspendScope. Part
1 changes LocalFrame detach to always use ChildFrameDisconnector to
detach child frames. Part 2 will rework ChildFrameDisconnector to also
detach plugin elements. This should eliminate the need to defer widget
updates, since script will never have to run during the actual mutation
of internal state.

BUG=524113,528867,561683, 621362 

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

[modify] https://crrev.com/7bf106f2192e922c18f18cac4ae18d79ea90c1b0/third_party/WebKit/Source/core/frame/Frame.cpp
[modify] https://crrev.com/7bf106f2192e922c18f18cac4ae18d79ea90c1b0/third_party/WebKit/Source/core/frame/Frame.h
[modify] https://crrev.com/7bf106f2192e922c18f18cac4ae18d79ea90c1b0/third_party/WebKit/Source/core/frame/LocalFrame.cpp
[modify] https://crrev.com/7bf106f2192e922c18f18cac4ae18d79ea90c1b0/third_party/WebKit/Source/core/frame/LocalFrame.h
[modify] https://crrev.com/7bf106f2192e922c18f18cac4ae18d79ea90c1b0/third_party/WebKit/Source/core/frame/RemoteFrame.cpp
[modify] https://crrev.com/7bf106f2192e922c18f18cac4ae18d79ea90c1b0/third_party/WebKit/Source/core/frame/RemoteFrame.h

Comment 13 by sheriffbot@chromium.org, Jul 14 2016

Project Member
Status: Fixed (was: Assigned)
Please mark security bugs as fixed as soon as the fix lands, and before requesting merges.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 14 by sheriffbot@chromium.org, Jul 15 2016

Project Member
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify

Comment 15 by dcheng@chromium.org, Jul 20 2016

Status: Started (was: Fixed)
This isn't fixed yet, I'm still working out the details of widget cleanup, which is proving to be surprisingly complicated.

For now, I'll likely check in a horrible hack. Blah.

Comment 16 by och...@chromium.org, Jul 20 2016

Labels: Restrict-View-SecurityEmbargo

Comment 17 by dcheng@chromium.org, Jul 21 2016

I'm having trouble reproing this on dev channel chrome: are there any special steps I need to take before running this repro?

Comment 18 by serg.gla...@gmail.com, Jul 21 2016

The issue still reproduces for me on chrome dev win64, the only requirement is that the repro should be hosted on a web server.

Comment 19 by dcheng@chromium.org, Jul 21 2016

Thanks, I was missing that step: sorry, I should have tried that first!

Comment 20 by sheriffbot@chromium.org, Jul 21 2016

Project Member
Labels: -M-51 M-52

Comment 21 by sheriffbot@chromium.org, Jul 21 2016

Project Member
Status: Fixed (was: Started)
Please mark security bugs as fixed as soon as the fix lands, and before requesting merges.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Comment 22 by dcheng@chromium.org, Jul 21 2016

Note: https://chromium.googlesource.com/chromium/src/+/cade7295256e7adabf84272fe5e269834eb44dde is the actual fix for this. I plan on revisiting this in a few weeks still, to rework widget detach.

Comment 23 by serg.gla...@gmail.com, Jul 22 2016

#19: Sorry, I should have noted that in the report. Hopefully, it won't happen again since I add a protocol check to more recent repro cases.

Comment 24 by dcheng@chromium.org, Jul 27 2016

Labels: -merge-merged-2795 Merge-Request-52
Removing the bogus merge label and requesting a merge to M52.

Comment 25 by dimu@chromium.org, Jul 27 2016

Labels: -Merge-Request-52 Merge-Review-52 Hotlist-Merge-Review
[Automated comment] Request affecting a post-stable build (M52), manual review required.

Comment 26 by gov...@chromium.org, Jul 27 2016

Cc: awhalley@chromium.org
+awhalley@, seems like this also require a merge to M53 branch 2785, correct?

Comment 27 by awhalley@chromium.org, Jul 27 2016

Labels: reward-topanel

Comment 28 by awhalley@chromium.org, Jul 27 2016

Labels: Merge-Request-53
Yep!

Comment 29 by dimu@chromium.org, Jul 27 2016

Labels: -Merge-Request-53 Merge-Approved-53 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M53 (branch: 2785)

Comment 30 by gov...@chromium.org, Jul 27 2016

Please try to merge you change to M53 branch 2785 ASAP latest by 5:00 PM PDT today (sooner the better to avoid compile failure and merge conflicts) so we can take it for tomorrow's M53 beta promotion. Thank you.

Comment 31 by dcheng@chromium.org, Jul 27 2016

I'm in Tokyo time atm and about to go to sleep. I don't feel comfortable merging without being around for potential failures; is anyone else willing to do the merge for me? Thanks!

Comment 32 by awhalley@chromium.org, Jul 27 2016

Ok, I'll take a shot at it.

Comment 33 by bugdroid1@chromium.org, Jul 27 2016

Project Member
Labels: -merge-approved-53 merge-merged-2785
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4d1400b0018d9e7e301a49d9ee1a6c38197e35ce

commit 4d1400b0018d9e7e301a49d9ee1a6c38197e35ce
Author: awhalley <awhalley@chromium.org>
Date: Wed Jul 27 17:50:22 2016

[merge to M53] Make sure Widget::dispose() respects UpdateSuspendScope.

BUG= 621362 

Review-Url: https://codereview.chromium.org/2171683002
Cr-Commit-Position: refs/heads/master@{#406802}
(cherry picked from commit cade7295256e7adabf84272fe5e269834eb44dde)

TBR=dcheng
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true

Review-Url: https://codereview.chromium.org/2190693002
Cr-Commit-Position: refs/branch-heads/2785@{#371}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/4d1400b0018d9e7e301a49d9ee1a6c38197e35ce/third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.cpp
[modify] https://crrev.com/4d1400b0018d9e7e301a49d9ee1a6c38197e35ce/third_party/WebKit/Source/core/html/HTMLFrameOwnerElement.h
[modify] https://crrev.com/4d1400b0018d9e7e301a49d9ee1a6c38197e35ce/third_party/WebKit/Source/core/html/HTMLPlugInElement.cpp

Comment 34 Deleted

Comment 35 Deleted

Comment 36 by awhalley@chromium.org, Aug 2 2016

Labels: -reward-topanel reward-unpaid reward-7500

Comment 37 by awhalley@chromium.org, Aug 2 2016

Another great bug, many thanks.  $7,500 from the panel.

Comment 38 by awhalley@chromium.org, Aug 4 2016

Labels: -reward-unpaid reward-inprocess

Comment 39 by awhalley@chromium.org, Aug 10 2016

Labels: -Hotlist-Merge-Approved

Comment 40 by awhalley@chromium.org, Aug 31 2016

Labels: -M-52 Release-0-M53 M-53

Comment 41 by awhalley@chromium.org, Sep 14 2016

Labels: CVE-2016-5148

Comment 42 by sheriffbot@chromium.org, Oct 28 2016

Project Member
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

Comment 43 by awhalley@chromium.org, Apr 25 2018

Labels: CVE_description-submitted

Comment 44 by awhalley@google.com, Today (14 hours ago)

Labels: -Restrict-View-SecurityEmbargo

Sign in to add a comment