Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Issue 673170 Security: Universal XSS using late widget updates
Starred by 1 user Reported by marius.mlynski@gmail.com, Dec 11 2016 Back to list
Status: Fixed
Owner:
Closed: Dec 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security

Blocked on:
issue 674203



Sign in to add a comment
VULNERABILITY DETAILS
Among the things that Document::shutdown() does, |view()->dispose()| is called:

From /third_party/WebKit/Source/core/frame/FrameView.cpp:
----------------
void FrameView::dispose() {
(...)
  // FIXME: Do we need to do something here for OOPI?
  HTMLFrameOwnerElement* ownerElement = m_frame->deprecatedLocalOwner();
  // TODO(dcheng): It seems buggy that we can have an owner element that
  // points to another Widget.
  if (ownerElement && ownerElement->ownedWidget() == this)
    ownerElement->setWidget(nullptr);
(...)
}
----------------

In addition to what Daniel said, not clearing the owner's widget when it doesn't match the document's view is dangerous -- this logic means there are 2 cases:

1. ownerElement->ownedWidget() == document_in_shutdown->view()
The frame element's widget is cleared during Document::shutdown(). The widget update is suspended and executed in a safe state.

2. ownerElement->ownedWidget() != document_in_shutdown->view()
The frame element's widget is left intact during Document::shutdown(). Instead, it's cleared in LocalFrame::createView when a new view is created for the navigated frame shortly thereafter. This is unsafe because clearing the widget may destroy a plugin and run script, allowing an attacker to violate invariants.

VERSION
The attached exploit works in:
Chrome 56.0.2924.21 (Beta)
Chrome 56.0.2924.18 (Dev)
Chromium 57.0.2948.0 + Pepper Flash (Release build compiled today)
 
exploit.zip
2.5 KB Download
Here's a patch ensuring that the widget is always cleared soon enough: https://codereview.chromium.org/2563313002

Please note that although the exploit doesn't work in the stable channel, the bug seems to affect that branch as well. The plugin element loading logic is somehow different in stable and I have yet to see if I can adjust the PoC to it.
Comment 2 by wrengr@chromium.org, Dec 12 2016
Cc: dcheng@chromium.org
Labels: Security_Severity-High M-56 Security_Impact-Beta OS-All Pri-1
Status: Available
Comment 3 by dcheng@chromium.org, Dec 12 2016
Cc: esprehn@chromium.org
Okay, so it turns out embed elements are a tiny bit broken in stable, ie. setting src works only once. Please find attached an example that should work in Chrome 55.0.2883.87 as well, the Security_Impact flag can be updated accordingly.
exploit-55.zip
2.6 KB Download
Comment 5 by dcheng@chromium.org, Dec 13 2016
Cc: haraken@chromium.org sigbjo...@opera.com
Comment 6 by dcheng@chromium.org, Dec 13 2016
Unmergable, but I'm going to attempt (again) to just remove deferred widget updates altogether (issue 561683)

In the meanwhile, we'll need a short-term fix for this to stop the bleeding though.
Project Member Comment 7 by sheriffbot@chromium.org, Dec 13 2016
Labels: ReleaseBlock-Stable
Comment 8 by dcheng@chromium.org, Dec 14 2016
Blockedon: 674203
Cc: -dcheng@chromium.org
Labels: -Security_Impact-Beta Security_Impact-Stable reward-topanel
Owner: dcheng@chromium.org
Looks like the patch from c#1 is in the CQ now, but setting an owner just in case any follow-up is needed. Feel free to reassign if someone else makes more sense.

Thanks for the report and fix, Marius!
Project Member Comment 10 by bugdroid1@chromium.org, Dec 16 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/71b91a424a00dce5ff5ff4cf2e7b52fc70136f36

commit 71b91a424a00dce5ff5ff4cf2e7b52fc70136f36
Author: marius.mlynski <marius.mlynski@gmail.com>
Date: Fri Dec 16 01:48:01 2016

Clear the owner element's widget in Document::shutdown().

FrameView::dispose() doesn't guarantee that the owner's widget is cleared
and this could be problematic when it's overwritten in
LocalFrame::createView() a short time later.

BUG= 673170 

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

[modify] https://crrev.com/71b91a424a00dce5ff5ff4cf2e7b52fc70136f36/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/71b91a424a00dce5ff5ff4cf2e7b52fc70136f36/third_party/WebKit/Source/core/frame/FrameView.cpp

Project Member Comment 11 by sheriffbot@chromium.org, Dec 16 2016
Status: Assigned
Labels: Merge-Request-56
Comment 13 by dimu@chromium.org, Dec 19 2016
Labels: -Merge-Request-56 Merge-Approved-56 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M56 (branch: 2924)
Project Member Comment 14 by bugdroid1@chromium.org, Dec 19 2016
Labels: -merge-approved-56 merge-merged-2924
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a7a51f270ce442c7292abb84e2e4941f1c9c6199

commit a7a51f270ce442c7292abb84e2e4941f1c9c6199
Author: Daniel Cheng <dcheng@chromium.org>
Date: Mon Dec 19 18:25:33 2016

Clear the owner element's widget in Document::shutdown().

FrameView::dispose() doesn't guarantee that the owner's widget is cleared
and this could be problematic when it's overwritten in
LocalFrame::createView() a short time later.

BUG= 673170 

Review-Url: https://codereview.chromium.org/2563313002
Cr-Commit-Position: refs/heads/master@{#438977}
(cherry picked from commit 71b91a424a00dce5ff5ff4cf2e7b52fc70136f36)

Review-Url: https://codereview.chromium.org/2589913002 .
Cr-Commit-Position: refs/branch-heads/2924@{#551}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[modify] https://crrev.com/a7a51f270ce442c7292abb84e2e4941f1c9c6199/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/a7a51f270ce442c7292abb84e2e4941f1c9c6199/third_party/WebKit/Source/core/frame/FrameView.cpp

Components: Blink>Internals
Project Member Comment 16 by sheriffbot@chromium.org, Dec 20 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
Labels: -Hotlist-Merge-Approved -ReleaseBlock-Stable
Project Member Comment 18 by sheriffbot@chromium.org, Dec 21 2016
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: -reward-topanel reward-unpaid reward-8000
Many thanks as ever - $8,000 for this one!
Labels: Release-0-M56
Labels: CVE-2017-5006
Labels: -reward-unpaid reward-inprocess
Cc: joelhockey@chromium.org
Project Member Comment 25 by sheriffbot@chromium.org, Mar 29
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