New issue
Advanced search Search tips

Issue 762944 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Pop-under in M61+

Project Member Reported by timloh@chromium.org, Sep 7 2017

Issue description

See attached for de-obfuscated PoC, works in Canary on Windows and probably also CrOS and Linux (well my PoC doesn't get the focus event on my Linux machine but e.g. setTimeout works).

- Relies on mouseup-in-new-tab
- I'm surprised the moveTo/resizeTo works and makes the popup completely offscreen.  Bug 681511  is filed for that, but it suggests we should always have some visibility.
- Something to work around the pop-under blocker, I didn't investigate.

Avi, do you want to have a look?
 
(oops, uploaded test page with a 5s timer interval instead of 20ms..)
PopUnder Demo.html
3.5 KB View Download

Comment 2 by a...@chromium.org, Sep 7 2017

Cc: sky@chromium.org
+sky

Another abuse of mouseup-in-new-tab, as in  bug 752824 .

At first glance this should be caught by the fix for that bug, but it's obviously not. Will investigate.
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 11 2017

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

commit 36465f338a331f439c7cfb8fe94fd0c7d3f95346
Author: Avi Drissman <avi@chromium.org>
Date: Mon Sep 11 20:49:39 2017

Strengthen the Popunder Preventer.

The Popunder Preventer uses the "original owner" of a popup
to determine malfeasance. It walks the original owner chain
of WebContentses to connect a WebContents to a popup down
the line.

Therefore, if a WebContents tries to be tricky, and sets up
an intermediate WebContents that it deletes before alerting,
maintain the "original owner" chain during the deletion.

BUG= 762944 
TEST=as in bug

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: Id5f1152cf24206ff97b70f65b11c540193e7e4cf
Reviewed-on: https://chromium-review.googlesource.com/658103
Commit-Queue: Avi Drissman <avi@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501030}
[modify] https://crrev.com/36465f338a331f439c7cfb8fe94fd0c7d3f95346/content/browser/frame_host/frame_tree_node.cc
[modify] https://crrev.com/36465f338a331f439c7cfb8fe94fd0c7d3f95346/content/browser/web_contents/web_contents_impl_browsertest.cc
[modify] https://crrev.com/36465f338a331f439c7cfb8fe94fd0c7d3f95346/content/public/browser/web_contents.h

Comment 4 by a...@chromium.org, Sep 11 2017

Status: Fixed (was: Assigned)
Project Member

Comment 5 by sheriffbot@chromium.org, Sep 12 2017

Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify

Comment 6 by timloh@chromium.org, Sep 19 2017

Should we merge to 62? Still a few weeks before stable.

Comment 7 by avi@google.com, Sep 19 2017

Labels: Merge-Request-62
I'll ask.


Project Member

Comment 8 by sheriffbot@chromium.org, Sep 19 2017

Labels: -Merge-Request-62 Merge-Review-62 Hotlist-Merge-Review
This bug requires manual review: M62 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-62 Merge-Approved-62
Thanks for the fix. Trying out the demo, seems like this is a necessary fix and it's also marked as RB-Stable. Approving this merge to M62. branch: 3202
Project Member

Comment 10 by bugdroid1@chromium.org, Sep 20 2017

Labels: -merge-approved-62 merge-merged-3202
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/55b5dd813b208548923dc41b468562a655a3d91a

commit 55b5dd813b208548923dc41b468562a655a3d91a
Author: Avi Drissman <avi@chromium.org>
Date: Wed Sep 20 05:15:56 2017

Strengthen the Popunder Preventer.

The Popunder Preventer uses the "original owner" of a popup
to determine malfeasance. It walks the original owner chain
of WebContentses to connect a WebContents to a popup down
the line.

Therefore, if a WebContents tries to be tricky, and sets up
an intermediate WebContents that it deletes before alerting,
maintain the "original owner" chain during the deletion.

BUG= 762944 
TEST=as in bug

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Change-Id: Id5f1152cf24206ff97b70f65b11c540193e7e4cf
Reviewed-on: https://chromium-review.googlesource.com/658103
Commit-Queue: Avi Drissman <avi@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#501030}(cherry picked from commit 36465f338a331f439c7cfb8fe94fd0c7d3f95346)
Reviewed-on: https://chromium-review.googlesource.com/674783
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/branch-heads/3202@{#346}
Cr-Branched-From: fa6a5d87adff761bc16afc5498c3f5944c1daa68-refs/heads/master@{#499098}
[modify] https://crrev.com/55b5dd813b208548923dc41b468562a655a3d91a/content/browser/frame_host/frame_tree_node.cc
[modify] https://crrev.com/55b5dd813b208548923dc41b468562a655a3d91a/content/browser/web_contents/web_contents_impl_browsertest.cc
[modify] https://crrev.com/55b5dd813b208548923dc41b468562a655a3d91a/content/public/browser/web_contents.h

Project Member

Comment 11 by sheriffbot@chromium.org, Dec 19 2017

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