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

Issue 616907 link

Starred by 5 users

Issue metadata

Status: Fixed
Owner:
Closed: Jul 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug-Security



Sign in to add a comment

Security: Universal XSS using a ScopedPageLoadDeferrer bypass

Reported by marius.mlynski@gmail.com, Jun 2 2016

Issue description

VULNERABILITY DETAILS
This is an architectural problem with the implementation of ScopedPageLoadDeferrer. Basically, it works by marking pages as deferred at the time a deferrer is instantiated. The issue is that pages created past this point don't defer loads by default. An attacker can move an iframe across the deferral boundary, which allows synchronous cross-origin navigations in unexpected circumstances.

VERSION
Chrome 51.0.2704.79 (Stable)
Chrome 51.0.2704.63 (Beta)
Chrome 52.0.2743.19 (Dev)
Chromium 53.0.2757.0 (Release build compiled today)
 
exploit.zip
2.5 KB Download
I was thinking about 2 alternative solutions:

1. Make new ordinary pages inherit the deferral state from the opener frame's page. Unfortunately, that'd also require updating |m_deferredFrames| on a ScopedPageLoadDeferrer to ensure that new pages are marked as non-deferred later when the deferrer is destroyed. That'd probably require making the class more static, and I'm not sure how to reconcile that with multiple deferrers on the stack. Meh.
2. Ban creating new windows from frames belonging to deferred pages. Popping new pages from behind a modal dialog is a somewhat shady concept, and this is a very simple solution, so if it doesn't break anything, I'd go for it.

I've made an attempt at implementing #2 in https://codereview.chromium.org/2035973002.

Comment 2 by f...@chromium.org, Jun 3 2016

Labels: Security_Severity-High Security_Impact-Stable Pri-1
Status: Available (was: Unconfirmed)
Thanks for the report, and the start on a patch!

Comment 3 by f...@chromium.org, Jun 3 2016

Labels: M-51
Owner: dcheng@chromium.org
dcheng@, would you be a good owner for this? If so, PTAL?

Comment 4 by f...@chromium.org, Jun 3 2016

Cc: tasak@chromium.org
Components: Blink
or perhaps tasak@, i see you have recently done work in core/page?
Cc: japhet@chromium.org dcheng@chromium.org
Owner: kinuko@chromium.org
Status: Assigned (was: Available)
Nooooooooo. kinuko@, can someone from the Tokyo loading team investigate this so we can distribute the load for these bugs?

japhet@ or I will be happy to review any changes.
As a matter of cleanup, we probably should make the deferred loading flag a per-process flag instead of a per-page flag. That wouldn't stop about:blank navigations during load deferral, though I think it would stop data: urls?
I was thinking about this, but then I noticed the constructor takes an excluded page as a parameter and I concluded a more fine-grained control over the deferral state might be desirable for some reason(s). data: URIs are loaded asynchronously, so they're subject to deferral, yes.
I don't think we actually ever set an exclusion page (probably vestigial from the WebKit era), so that hopefully isn't a limiting factor.
Cc: hirosh...@chromium.org yhirano@chromium.org
 :( cc-ing some folks who might be able to investigate it a little further
I feel we could probably land both? Marius's patch seems to work but also feels very specific so I'm afraid there could potentially be other similar issues, while changing like Nate suggests might need to touch a lot more files (as well as might be a bit controversial) and may take a little more time.
Yeah, I think it makes sense to land Marius's patch, then do my idea separately.
Project Member

Comment 12 by sheriffbot@chromium.org, Jun 23 2016

kinuko: 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
Cc: palmer@chromium.org
Labels: -M-51 M-52 OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows
We have a fix and it is LGTM'd. This is a High severity bug; is there any reason not to land the fix?

I suggest we land it ASAP and merge it back into 52. It's a simple patch so it should be a clean merge.
The band-aid fix by marius is LGTM'ed about 2~3 weeks ago but looks like it's not landed yet-- let me just try CQ now.
Project Member

Comment 15 by bugdroid1@chromium.org, Jul 4 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/25a8ae78165cd8725465d26e02a342d7121c42f4

commit 25a8ae78165cd8725465d26e02a342d7121c42f4
Author: marius.mlynski <marius.mlynski@gmail.com>
Date: Mon Jul 04 06:24:56 2016

Don't allow deferred frames to create new windows.

New pages never defer loads, which makes it difficult for ScopedPageLoadDeferrer
to protect against synchronous loads.

This patch adds a check in ChromeClientImpl::createWindow.

BUG= 616907 

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

[modify] https://crrev.com/25a8ae78165cd8725465d26e02a342d7121c42f4/third_party/WebKit/Source/web/ChromeClientImpl.cpp
[modify] https://crrev.com/25a8ae78165cd8725465d26e02a342d7121c42f4/third_party/WebKit/Source/web/tests/ChromeClientImplTest.cpp

Labels: Merge-Request-52
Project Member

Comment 17 by sheriffbot@chromium.org, Jul 4 2016

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

Comment 18 by sheriffbot@chromium.org, Jul 5 2016

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: reward-topanel
Before we approve merge to M52, Could you please confirm whether this change is baked/verified in Canary and safe to merge?

Comment 21 Deleted

+awhalley@, can we take this in for M52 release next week?
OK for M52.
Labels: -Merge-Request-52 Merge-Approved-52
Approving merge to M52 branch 2743 based on comment #23. Please merge ASAP or latest by 4:00 PM PST on Monday, 07/18 so we can take it for next week M52 Stable promotion release.

Also does this require a merge to M53 as well?
Labels: Merge-Request-53
+govind@ yes, also needed for M53
Labels: -Merge-Request-53 Merge-Approved-53
Approving merge to M53 branch 2785. Please merge ASAP or latest by 4:00 PM PST on Monday, 07/18. Thank you.
Project Member

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

Labels: -merge-approved-52 merge-merged-2743
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cbe0b97d5cf56f8eaab3b2fc57b925c69d8f7aa0

commit cbe0b97d5cf56f8eaab3b2fc57b925c69d8f7aa0
Author: Kinuko Yasuda <kinuko@chromium.org>
Date: Sun Jul 17 10:12:01 2016

Don't allow deferred frames to create new windows.

New pages never defer loads, which makes it difficult for ScopedPageLoadDeferrer
to protect against synchronous loads.

This patch adds a check in ChromeClientImpl::createWindow.

BUG= 616907 

Review-Url: https://codereview.chromium.org/2035973002
Cr-Commit-Position: refs/heads/master@{#403640}
(cherry picked from commit 25a8ae78165cd8725465d26e02a342d7121c42f4)

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

Cr-Commit-Position: refs/branch-heads/2743@{#658}
Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939}

[modify] https://crrev.com/cbe0b97d5cf56f8eaab3b2fc57b925c69d8f7aa0/third_party/WebKit/Source/web/ChromeClientImpl.cpp
[modify] https://crrev.com/cbe0b97d5cf56f8eaab3b2fc57b925c69d8f7aa0/third_party/WebKit/Source/web/tests/ChromeClientImplTest.cpp

Project Member

Comment 28 by bugdroid1@chromium.org, Jul 17 2016

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

commit 65422dba656b2dc51c4a36e6398d5391ae9ef120
Author: Kinuko Yasuda <kinuko@chromium.org>
Date: Sun Jul 17 10:24:16 2016

Don't allow deferred frames to create new windows.

New pages never defer loads, which makes it difficult for ScopedPageLoadDeferrer
to protect against synchronous loads.

This patch adds a check in ChromeClientImpl::createWindow.

BUG= 616907 

Review-Url: https://codereview.chromium.org/2035973002
Cr-Commit-Position: refs/heads/master@{#403640}
(cherry picked from commit 25a8ae78165cd8725465d26e02a342d7121c42f4)

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

Cr-Commit-Position: refs/branch-heads/2785@{#172}
Cr-Branched-From: 68623971be0cfc492a2cb0427d7f478e7b214c24-refs/heads/master@{#403382}

[modify] https://crrev.com/65422dba656b2dc51c4a36e6398d5391ae9ef120/third_party/WebKit/Source/web/ChromeClientImpl.cpp
[modify] https://crrev.com/65422dba656b2dc51c4a36e6398d5391ae9ef120/third_party/WebKit/Source/web/tests/ChromeClientImplTest.cpp

Merged to M52 and M53.
Not sure if tests are compiled for beta/stable, but ChromeClientImplTest.cpp may fail due to a mismatch of arguments provided to WebView::create:

https://chromium.googlesource.com/chromium/src.git/+/52.0.2743.79/third_party/WebKit/Source/web/WebViewImpl.cpp#332
Thanks for the heads-up. We don't seem building webkit_unit_tests for Beta/Stable, but will be watching alerts.
Project Member

Comment 32 by bugdroid1@chromium.org, Jul 18 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fdaa78ddc7867123001e1c91e79f1f0e830b7e69

commit fdaa78ddc7867123001e1c91e79f1f0e830b7e69
Author: Kinuko Yasuda <kinuko@chromium.org>
Date: Mon Jul 18 15:12:13 2016

Fix build failure for the broken merge on M52/M53 tree

BUG=628975,  616907 
TBR=govind@chromium.org

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

Cr-Commit-Position: refs/branch-heads/2743@{#661}
Cr-Branched-From: 2b3ae3b8090361f8af5a611712fc1a5ab2de53cb-refs/heads/master@{#394939}

[modify] https://crrev.com/fdaa78ddc7867123001e1c91e79f1f0e830b7e69/third_party/WebKit/Source/web/tests/ChromeClientImplTest.cpp

Labels: Release-0-M52
Labels: CVE-2016-1710
Labels: reward-8000 reward-unpaid
Nice - $8,000 for this one, Marius.
Labels: -reward-topanel
Labels: -reward-unpaid reward-inprocess
Project Member

Comment 39 by sheriffbot@chromium.org, Oct 10 2016

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
Labels: CVE_description-submitted

Sign in to add a comment