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 671102 Security: Universal XSS through bypassing ScopedPageSuspender with closing windows
Starred by 1 user Reported by marius.mlynski@gmail.com, Dec 5 Back to list
Status: Fixed
Owner:
OOO through 2017-05-02
Closed: Dec 27
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 1
Type: Bug-Security

Blocked on:
issue 674203



Sign in to add a comment
VULNERABILITY DETAILS
ScopedPageSuspender works by taking pages from Page::ordinaryPages() and marking them as suspended. When window.close() is called, the following operations are performed:

From /third_party/WebKit/Source/web/ChromeClientImpl.cpp:
----------------
void ChromeClientImpl::closeWindowSoon() {
  // Make sure this Page can no longer be found by JS.
  m_webView->page()->willBeClosed();

  // Make sure that all loading is stopped.  Ensures that JS stops executing!
  m_webView->mainFrame()->stopLoading();

  if (m_webView->client())
    m_webView->client()->closeWidgetSoon();
}
----------------

|m_webView->page()->willBeClosed()| removes the associated page from the ordinaryPages set. Therefore, suspenders instantiated later, for example during |m_webView->mainFrame()->stopLoading()|, won't include the closing page. This allows an attacker to circumvent the suspender and perform synchronous loads in unexpected circumstances.

VERSION
Chrome 55.0.2883.75 (Stable)
Chrome 55.0.2883.75 (Beta)
Chrome 56.0.2924.14 (Dev)
Chromium 57.0.2943.0 + Pepper Flash (Release build compiled today)
 
exploit.zip
3.0 KB Download
Components: Blink
Labels: Security_Severity-High Security_Impact-Stable OS-All
Owner: dcheng@chromium.org
Status: Assigned
dcheng, would you mind helping with finding the right owner for this? Thanks.
Cc: haraken@chromium.org esprehn@chromium.org alex...@chromium.org
Status: Started
Meh. I was hoping to avoid maintaining two sets after https://codereview.chromium.org/2174263002, but it appears to be unavoidable.
Project Member Comment 4 by sheriffbot@chromium.org, Dec 6
Labels: M-55
Project Member Comment 5 by sheriffbot@chromium.org, Dec 6
Labels: Pri-1
Blockedon: 674203
Cc: mmoroz@chromium.org
Project Member Comment 8 by bugdroid1@chromium.org, Dec 22
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0879cb1c4d5e7a53d060bfb7cf7cf9ea05aced72

commit 0879cb1c4d5e7a53d060bfb7cf7cf9ea05aced72
Author: dcheng <dcheng@chromium.org>
Date: Thu Dec 22 10:36:49 2016

Make sure pages that are closing but not yet closed are still suspended.

BUG= 671102 

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

[modify] https://crrev.com/0879cb1c4d5e7a53d060bfb7cf7cf9ea05aced72/third_party/WebKit/Source/core/frame/DOMWindow.cpp
[modify] https://crrev.com/0879cb1c4d5e7a53d060bfb7cf7cf9ea05aced72/third_party/WebKit/Source/core/page/FrameTree.cpp
[modify] https://crrev.com/0879cb1c4d5e7a53d060bfb7cf7cf9ea05aced72/third_party/WebKit/Source/core/page/Page.cpp
[modify] https://crrev.com/0879cb1c4d5e7a53d060bfb7cf7cf9ea05aced72/third_party/WebKit/Source/core/page/Page.h
[modify] https://crrev.com/0879cb1c4d5e7a53d060bfb7cf7cf9ea05aced72/third_party/WebKit/Source/core/page/ScopedPageSuspender.cpp
[modify] https://crrev.com/0879cb1c4d5e7a53d060bfb7cf7cf9ea05aced72/third_party/WebKit/Source/core/page/ScopedPageSuspender.h
[modify] https://crrev.com/0879cb1c4d5e7a53d060bfb7cf7cf9ea05aced72/third_party/WebKit/Source/web/ChromeClientImpl.cpp
[modify] https://crrev.com/0879cb1c4d5e7a53d060bfb7cf7cf9ea05aced72/third_party/WebKit/Source/web/tests/ChromeClientImplTest.cpp
[modify] https://crrev.com/0879cb1c4d5e7a53d060bfb7cf7cf9ea05aced72/third_party/WebKit/Source/web/tests/WebViewTest.cpp

Status: Fixed
Labels: Merge-Request-56
Note that we already merged a generic fix for these class of UXSS in issue 674203, but this is still a potential correctness issue.
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 13 by sheriffbot@chromium.org, Dec 27
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Project Member Comment 14 by sheriffbot@chromium.org, Dec 30
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

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
Project Member Comment 15 by sheriffbot@chromium.org, Jan 2
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

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
Labels: reward-topanel
So there's a lot of conflicts; resolving them manually is tricky.

The simplest path forward is to merge https://codereview.chromium.org/2526163002, and then this CL, but I'm not sure if that's still considered OK.
That sounds fine to me, approving both changes for merge into M56
Project Member Comment 19 by bugdroid1@chromium.org, Jan 3
Labels: -merge-approved-56 merge-merged-2924
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fad76601729f6b957274071fb0f1d33072ab72c1

commit fad76601729f6b957274071fb0f1d33072ab72c1
Author: Daniel Cheng <dcheng@chromium.org>
Date: Tue Jan 03 21:35:21 2017

Rename blink::Page's load deferral to suspension

This CL renames:
 * ScopedPageLoadDeferrer to ScopedPageSuspender,
 * blink::Page::defersLoading() to suspended(), and
 * blink::Page::setDefersLoading() to setSuspended().

blink::setDefersLoading does not only defer loadings, but also suspends
all associated ExecutionContext and ActiveDOMObjects.

Review-Url: https://codereview.chromium.org/2526163002
Cr-Commit-Position: refs/heads/master@{#434378}
(cherry picked from commit 5ad6641ca96fc131e8268f649bc6db71170ad736)

BUG= 671102 

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

[modify] https://crrev.com/fad76601729f6b957274071fb0f1d33072ab72c1/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/fad76601729f6b957274071fb0f1d33072ab72c1/third_party/WebKit/Source/core/loader/DocumentLoader.cpp
[modify] https://crrev.com/fad76601729f6b957274071fb0f1d33072ab72c1/third_party/WebKit/Source/core/loader/FrameFetchContext.cpp
[modify] https://crrev.com/fad76601729f6b957274071fb0f1d33072ab72c1/third_party/WebKit/Source/core/loader/FrameLoader.cpp
[modify] https://crrev.com/fad76601729f6b957274071fb0f1d33072ab72c1/third_party/WebKit/Source/core/loader/NavigationScheduler.cpp
[modify] https://crrev.com/fad76601729f6b957274071fb0f1d33072ab72c1/third_party/WebKit/Source/core/page/BUILD.gn
[modify] https://crrev.com/fad76601729f6b957274071fb0f1d33072ab72c1/third_party/WebKit/Source/core/page/ChromeClient.cpp
[modify] https://crrev.com/fad76601729f6b957274071fb0f1d33072ab72c1/third_party/WebKit/Source/core/page/FocusController.cpp
[modify] https://crrev.com/fad76601729f6b957274071fb0f1d33072ab72c1/third_party/WebKit/Source/core/page/Page.cpp
[modify] https://crrev.com/fad76601729f6b957274071fb0f1d33072ab72c1/third_party/WebKit/Source/core/page/Page.h
[rename] https://crrev.com/fad76601729f6b957274071fb0f1d33072ab72c1/third_party/WebKit/Source/core/page/ScopedPageSuspender.cpp
[rename] https://crrev.com/fad76601729f6b957274071fb0f1d33072ab72c1/third_party/WebKit/Source/core/page/ScopedPageSuspender.h
[modify] https://crrev.com/fad76601729f6b957274071fb0f1d33072ab72c1/third_party/WebKit/Source/web/ChromeClientImpl.cpp
[modify] https://crrev.com/fad76601729f6b957274071fb0f1d33072ab72c1/third_party/WebKit/Source/web/WebViewImpl.cpp
[modify] https://crrev.com/fad76601729f6b957274071fb0f1d33072ab72c1/third_party/WebKit/Source/web/tests/ChromeClientImplTest.cpp
[modify] https://crrev.com/fad76601729f6b957274071fb0f1d33072ab72c1/third_party/WebKit/Source/web/tests/WebFrameTest.cpp
[modify] https://crrev.com/fad76601729f6b957274071fb0f1d33072ab72c1/third_party/WebKit/Source/web/tests/WebViewTest.cpp

Project Member Comment 20 by bugdroid1@chromium.org, Jan 3
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dc3f2823c1485ab302a2144ec1fcfe37f0e5af50

commit dc3f2823c1485ab302a2144ec1fcfe37f0e5af50
Author: Daniel Cheng <dcheng@chromium.org>
Date: Tue Jan 03 21:44:44 2017

Make sure pages that are closing but not yet closed are still suspended.

BUG= 671102 

Review-Url: https://codereview.chromium.org/2580703003
Cr-Commit-Position: refs/heads/master@{#440376}
(cherry picked from commit 0879cb1c4d5e7a53d060bfb7cf7cf9ea05aced72)

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

[modify] https://crrev.com/dc3f2823c1485ab302a2144ec1fcfe37f0e5af50/third_party/WebKit/Source/core/frame/DOMWindow.cpp
[modify] https://crrev.com/dc3f2823c1485ab302a2144ec1fcfe37f0e5af50/third_party/WebKit/Source/core/page/FrameTree.cpp
[modify] https://crrev.com/dc3f2823c1485ab302a2144ec1fcfe37f0e5af50/third_party/WebKit/Source/core/page/Page.cpp
[modify] https://crrev.com/dc3f2823c1485ab302a2144ec1fcfe37f0e5af50/third_party/WebKit/Source/core/page/Page.h
[modify] https://crrev.com/dc3f2823c1485ab302a2144ec1fcfe37f0e5af50/third_party/WebKit/Source/core/page/ScopedPageSuspender.cpp
[modify] https://crrev.com/dc3f2823c1485ab302a2144ec1fcfe37f0e5af50/third_party/WebKit/Source/core/page/ScopedPageSuspender.h
[modify] https://crrev.com/dc3f2823c1485ab302a2144ec1fcfe37f0e5af50/third_party/WebKit/Source/web/ChromeClientImpl.cpp
[modify] https://crrev.com/dc3f2823c1485ab302a2144ec1fcfe37f0e5af50/third_party/WebKit/Source/web/tests/ChromeClientImplTest.cpp
[modify] https://crrev.com/dc3f2823c1485ab302a2144ec1fcfe37f0e5af50/third_party/WebKit/Source/web/tests/WebViewTest.cpp

Labels: -reward-topanel reward-unpaid reward-8837
Hi!  The panel decided to reward $7,500 for this bug, and a $1337 bonus for the fix in https://chromium.googlesource.com/chromium/src/+/783e19486c
Labels: -Hotlist-Merge-Approved
Labels: -M-55 Release-0-M56 M-56
Labels: CVE-2017-5007
Labels: -reward-unpaid reward-inprocess
Project Member Comment 27 by sheriffbot@chromium.org, Apr 4
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