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 4 users
link

Issue 550017: Security: Modal dialogs overlaying Fullscreen permission dialog

Reported by luan.her...@hotmail.com, Oct 31 2015

Issue description

VULNERABILITY DETAILS
This vulnerability makes it possible to display a modal dialog in front of the Fullscreen permission dialog, making it "impossible" to the user to know he entered fullscreen mode, enabling the attacker to spoof the URL.

VERSION
Chrome Version: [46.0.2490.80 m] + [stable].

REPRODUCTION CASE
1. Click on the link: http://lherrera.16mb.com/fullscreen.html
2. Click on the link in the attacker's page to access "google.com".
3. An alert will show up asking you for credentials.

* Given I use an image to spoof all the page, because of different screen resolution the image maybe in a wrong position. But a dedicated attacker could easily fix this, as he knows the users screen resolution and operational system.

Here is a video simulating the attack:
https://www.youtube.com/watch?v=-y-XIbbj9gM
 
Showing comments 37 - 136 of 136 Older

Comment 37 by nparker@chromium.org, May 23 2016

I confirmed this still repro's on dev channel, with changes that were made to dialogs in the recent past.  I'd vote for disabling dialogs in full screen.

Comment 38 by mgiuca@chromium.org, May 24 2016

#37: Do we need security / UX / web standards approval before doing this? It sounds like a potentially controversial change.

Comment 39 by mea...@chromium.org, May 24 2016

You might consider sending a blink intent to deprecate/remove before proceeding.

Comment 40 by mgiuca@chromium.org, May 25 2016

Components: Blink>HTML>Dialog
I'm not sure I'm the right person to do this. I'm responsible for the fullscreen notification bubble (and that already isn't my main job). I don't have any experience with the modal dialogs. Adding Blink>HTML>Dialog; perhaps someone from there can take a look.

It also may be worth discussing this with Interventions working group (https://github.com/WICG/interventions). I will file a bug there and see if anyone is interested.

Comment 41 by mgiuca@chromium.org, May 25 2016

Cc: mea...@chromium.org
Actually, since that's public, if we want to have this discussion in public (which would also be the case with sending mail to blink-dev), we need to unrestrict this bug first. meacer@?

Comment 42 by mea...@chromium.org, May 25 2016

Cc: mbarbe...@chromium.org
I'd be okay with unrestricting this bug if it low severity, but not sure with medium.
mbarbella@: WDYT?

mgiuca@: Maybe you could avoid being specific while describing this case in the interventions bug, and just mention the general idea of disabling dialogs in fullscreen? (that is, if we decide to keep the bug restricted)

Comment 43 by mgiuca@chromium.org, May 25 2016

#42 I don't think there is a strong case for doing so without discussing the security implications of this.

Comment 44 by mgiuca@chromium.org, May 25 2016

The other thing we can do is just my suggestion in #30 to move it to the center instead of disabling it, and then open a new bug for removing it, and talk with Interventions at that time. (To reiterate, I don't want to be responsible for driving that effort.) I'm curious what avi@ thinks since his name has come up several times on this thread but hasn't commented.

Comment 45 by a...@chromium.org, May 25 2016

If we can remove alerts for fullscreen pages, that would be awesome. I don't know if we have the political will to do that. Until my dialog work happens, moving the dialog would help on the Views platform (they're already centered on the Mac).

I'd be cool with that, though I hope UX would be OK here.

Comment 46 by mea...@chromium.org, May 25 2016

+1 to moving the dialog to the center. It will also no more overlap browser chrome, which is extra good.

Comment 47 by mgiuca@chromium.org, May 26 2016

Dialog is centered on Firefox and Edge as well, so doing that seems fine. Not sure it fully solves the issue since a very tall alert could still cover the fullscreen bubble, but it's going to look a lot more suspicious.

avi@ would you be able to change the dialog to the center? I had a quick look and found:
UpdateModalDialogPosition in constrained_window_views.cc which is what's responsible for setting the position. But it gets the position from ModalDialogHost::GetDialogPosition and that's virtual and I have no idea which class it's using to get the position. (I'm too far out of my knowledge about this system.)

Comment 48 by a...@chromium.org, May 26 2016

BTW, with the new scheme, the plan is to have centered dialog boxes all the time. Hope I can make that happen soon.

As for making this happen now for fullscreen, I'm a Mac person and don't know Views. Maybe sky@ or pkasting@?

Comment 49 by mgiuca@chromium.org, May 26 2016

I think it would be easier to make all dialog boxes always centered, than trying to make a special case for alert boxes in fullscreen. I can probably keep digging to find out how to do that if you think we can make that change globally now.

Comment 50 by a...@chromium.org, May 26 2016

I tried to get UX to approve a centered dialog style for OldSpice and got pushback, saying that we had enough UI styles already. Ugh.

Maybe this can help change their minds.

Comment 51 by mgiuca@chromium.org, May 26 2016

Cc: ainslie@chromium.org bbergher@chromium.org
+bbergher, ainslie: Would you be able to comment on this? Can we just make all the modal dialogs centered instead of in the top third of the screen?

(Avoid security problem with overlapping a modal dialog with the fullscreen bubble.)

Comment 52 by sheriffbot@chromium.org, May 26 2016

Project Member
Labels: -M-50 M-51

Comment 53 by bbergher@chromium.org, May 26 2016

I support making all modal dialogs centered. As far as a very long message still blocking the notification:

1. We seem to truncate the prompt string after a certain length, still leaving the notification visible
2. Such a long string would probably seem suspicious (though that's obviously not enough to be trusted).

ainslie@ is out, back on Tuesday, so it might be worth waiting for him before taking action.

Comment 54 by mea...@chromium.org, May 26 2016

Just to clarify, modal dialogs here refer to content-initiated modal dialogs, right? Because we still want trusted dialogs to overlap browser chrome.

Comment 55 by a...@chromium.org, May 26 2016

Absolutely. We need to build a distinction between trusted dialogs that are unspoofable, and untrusted dialogs.

Comment 56 by ainslie@chromium.org, Jun 6 2016

Cc: emilyschechter@chromium.org
re: #50 
"I tried to get UX to approve a centered dialog style for OldSpice and got pushback, saying that we had enough UI styles already. Ugh."

To make that anxiety more precise, here's what we are shipping in OSX canary today (attached). We have so much work to do to make Chrome UIs feel deliberate and coherent - and we plan to do that work as a continuation of the MD-Top-Chrome work (which is just focused on tabs and toolbar). 

Instead of creating a new position or style for untrusted dialogs, I'd prefer to explore using the normal trusted position and button style while also doing something extra to make it clear that certain strings are coming from the developer. There are a range of options I can imagine.

Maybe +emilyschechter@ can help us coordinate a time to chat.
osx-canary-button-styles.png
683 KB View Download

Comment 57 by mea...@chromium.org, Jun 6 2016

> Instead of creating a new position or style for untrusted dialogs, I'd prefer to explore using the normal trusted position and button style while also doing something extra to make it clear that certain strings are coming from the developer.

I'm not sure how this will be possible without adding even more text and styling. I also disagree that moving untrusted dialogs inside the content area is adding another style. Rather, it's changing the existing style to be more consistent with what a user would expect, that is, page generated content only shows up in page controlled area. From the user's and our perspective, an alert() shouldn't be any different than an HTML generated dialog.

Comment 58 by palmer@chromium.org, Jun 30 2016

Have we had this discussion yet? I don't think we should gate fixing this bug on the UX design team; we should try to at least get some kind of mitigation happening that doesn't require new design work. Random ideas:

* Can the full-screen notification always have the highest Z-order?

* We would like to kill window.{alert,prompt,confirm} anyway; maybe block them in fullscreen mode as a first step toward deprecation?

* Ensure that modal dialogs always show the real origin? What's the PoC code for this one, by the way? The obvious thing of just calling window.prompt("foo") and window.location = "javascript:window.prompt('foo')" always causes the real origin to show, so maybe we're already doing that? Would that be mitigation enough for this bug?

Comment 59 by mgiuca@chromium.org, Jul 1 2016

I think removing alerts in fullscreen would be good (but I'm not part of those discussions). If we're going to do that soon, we don't need another mitigation for this bug.

> * Can the full-screen notification always have the highest Z-order?

I've spent most of this week enragedly trying to do this for another bug ( Issue 623862 ), pretty much the same issue: a modal dialog covering up the fullscreen. Our widget system is amazingly bad for manually setting z-orders. There are procedural StackBelow / StackAbove commands, but they don't let you set a permanent Z order (so as soon as a new window is created it goes in front). And they aren't implemented on Mac. So there's really no good solution for making sure a particular dialog is on top, on all platforms. :( It's been a sad, sad week.

I'd rather kill alerts in fullscreen.

Comment 60 by palmer@chromium.org, Jul 1 2016

avi: This may be relevant to your interests.

Comment 61 by a...@chromium.org, Jul 1 2016

mgiuca:
> I think removing alerts in fullscreen would be good (but I'm not part of
> those discussions).

There are discussions? I've seen grumblings about it, but haven't personally seen more. If you think this should happen, you totally should write an intent and push it. I will support you in whatever way I can.

(And if you don't want to be the public face of this, and want me to do it, I'm out next week, so talk to me when I get back on the 11th.)

Comment 62 by mgiuca@chromium.org, Jul 1 2016

#61:

> There are discussions?

I mean the discussions about removing alert() entirely which you seem to have been having for some time.

Comment 63 by a...@chromium.org, Jul 1 2016

> I mean the discussions about removing alert() entirely which you seem to have been having for some time.

Oh, right. Yes, but that's not going to happen for a while. If this bug is a good reason to take a small step of killing them in a specific circumstance, we should grab it independently.

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

Labels: OS-Windows

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

Labels: -OS-Windows OS-All

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

Project Member
Labels: -M-51 M-52

Comment 67 by raymes@chromium.org, Aug 1 2016

ping - any updates here?

Comment 68 by mgiuca@chromium.org, Aug 1 2016

Cc: -a...@chromium.org -egm@chromium.org mgiuca@chromium.org
Owner: a...@chromium.org
Hi, (getting harrassed by sheriff) :)

It looks like the decision that was made was to drop alert() boxes when in fullscreen mode. If we're going to do that, then it doesn't really make sense for me to do it since I have no knowledge of that area of the code (nor the time to learn it). Therefore, assigning to avi@.

I can point you in the right direction for "is the browser in fullscreen" checks. Specifically, if you have a Browser* browser, it is:

browser->exclusive_access_manager()->fullscreen_controller()->IsControllerInitiatedFullscreen()

See https://cs.chromium.org/chromium/src/chrome/browser/renderer_context_menu/render_view_context_menu.cc?l=1210 for an example.

Comment 69 by a...@chromium.org, Aug 1 2016

Was that the decision that was made? If so, doing that is trivial and I'll get that done by the end of the day.

All alert() boxes? How about prompt() ones, or confirm() ones?

Comment 70 by a...@chromium.org, Aug 1 2016

I'm proceeding with the plan of just preventing all JavaScript dialogs. I hope we don't have to suppress onbeforeunload ones too.

Is there still a working repro? It seems like the repros have died of linkrot.

Comment 71 by luan.her...@hotmail.com, Aug 1 2016

#70:

data:text/html,<html><button id="btn">Enter fullscreen</button><script>document.getElementById("btn").onclick = function() { document.body.webkitRequestFullScreen(function(){});setTimeout(function() { alert(0); }, 100);}</script></html>

Comment 72 by a...@chromium.org, Aug 1 2016

Luan: Thanks!

Fix is at https://codereview.chromium.org/2194243002 . Fixing compilation, verifying tests, will send for review soon.

Comment 73 by mea...@chromium.org, Aug 1 2016

Looks like there hasn't been any discussion outside this bug about the change to remove alert on fullscreen.

I'm going to drop view restrictions if no one objects, so that we can refer people to the bug.

Comment 74 by a...@chromium.org, Aug 2 2016

Cc: jam@chromium.org

Comment 75 by mgiuca@chromium.org, Aug 2 2016

I think it makes sense to do as many modal dialogs as are tied together in the same system. I would imagine alert(), prompt() and confirm() are all implemented the same way so hopefully can all be done at the same time. onbeforeunload would be separate so we can think about that later.

Comment 76 by mgiuca@chromium.org, Aug 2 2016

Oh, I see from the CL that it's actually easier to suppress them all at the same time, but you avoid the onbeforeunload because we actually still need it. SGTM. I'll write more on the CL.

Comment 77 by a...@chromium.org, Aug 2 2016

Yes. Suppressing the "onbeforeunload" dialog scares me, because it's used to save data. But treating all the others as the same feels good to me.

Comment 78 by nparker@chromium.org, Aug 25 2016

Discussion of an actual social engineering page that covers the full-screen message with an alert: https://groups.google.com/a/google.com/d/msg/chrome-karma-team/FhCQGe2duC0/D-11O45BBgAJ

So this is being used in the wild now.

Comment 79 by luan.her...@hotmail.com, Aug 25 2016

Would be possible for me to get access to the discussion? It says my account has no authorization to view it.

Comment 80 by a...@chromium.org, Aug 25 2016

Should we put this up as an intent?

Comment 81 by mea...@chromium.org, Aug 25 2016

I feel like we need a stronger case for the intent at http://go/fullscreen-alert-intent. I can't seem to find another security bug for fullscreen+alerts, and we don't have metrics to cite for the current usage. We can still send it, but I'd expect pushback without those.

mgiuca and I was talking about this yesterday. The obvious and simpler alternative is to simply change the placement of the dialogs, so should we do that instead?

Comment 82 by a...@chromium.org, Aug 26 2016

We'd need to talk to UX, and good luck. When I was talking to UX about new UI for OldSpice, I got severe pushback, and that's for a redesign and a rework of functionality.

Comment 83 by mea...@chromium.org, Aug 26 2016

Do you mean we need to talk to UX for changing the positioning of the alert, or the whole deprecation?

Comment 84 by mgiuca@chromium.org, Aug 26 2016

I can't give you access to that discussion, but the exploit site is:
http://www.mswindowsalert.online

Comment 85 by a...@chromium.org, Aug 26 2016

Yes. If we want to move alerts to address this issue, we need to talk to UX.

Comment 86 by sheriffbot@chromium.org, Sep 1 2016

Project Member
Labels: -M-52 M-53

Comment 87 by mea...@chromium.org, Oct 7 2016

It seems we are in a deadlock on this medium severity security bug. Displaying browser chrome when an alert dialog is shown ( bug 642568 ) was considered because we thought it would help with this bug. But per Matt's comments in that bug, doing so still doesn't help unless we move alert dialogs inside content area.

@ainslie: Given that gating on site engagement isn't happening soon either, what is the proposal from the UI team to go forward? Is consistency the main argument against moving alerts inside content area? If so, I'd like to understand it better, because current display style isn't consistent with anything else either. The dialogs are fully controlled by the page, yet they overlap browser chrome. These dialogs are a class of their own, so I don't think we'll be introducing yet another dialog style by fixing their positioning.

Comment 88 Deleted

Comment 89 by ainslie@chromium.org, Oct 10 2016

I think it'd make more sense to chat in person (with hwi@, rpop@, and emilyschechter@) than to continue the discussion here.

Comment 90 by emilyschechter@chromium.org, Oct 10 2016

Thanks, setting up some time now.

Comment 91 by palmer@chromium.org, Oct 11 2016

Cc: -palmer@chromium.org

Comment 92 by sheriffbot@chromium.org, Oct 13 2016

Project Member
Labels: -M-53 M-54

Comment 93 by emilyschechter@chromium.org, Oct 24 2016

We discussed and came up with two solutions to investigate:

(1)  Issue 658900  - Fullscreen bubble should be ordered on top of other dialogs/alerts

(2)  Issue 642568  - Make all popups in fullscreen temporarily show the browser UI

Comment 94 by mgiuca@chromium.org, Nov 27 2016

Blocking: 658900

Comment 95 by mgiuca@chromium.org, Nov 27 2016

Blocking: 642568

Comment 96 by raymes@chromium.org, Nov 30 2016

Components: -Security>UX Blink>HTML>JavaScriptDialog
Labels: Team-Security-UX

Comment 97 by raymes@chromium.org, Nov 30 2016

Components: -Blink>HTML>JavaScriptDialog Blink>WindowDialog

Comment 98 by sheriffbot@chromium.org, Dec 2 2016

Project Member
Labels: -M-54 M-55

Comment 99 by nparker@chromium.org, Jan 20 2017

emilyschechter / mguica -- which of those two bugs is likely to happen first?  Both are P3s, but this is P1, so we should raise at least one of them.

Comment 100 by mgiuca@chromium.org, Jan 23 2017

I think we decided  Issue 658900  should happen. I bumped it to P1.

Comment 101 by sheriffbot@chromium.org, Jan 26 2017

Project Member
Labels: -M-55 M-56

Comment 102 by mgiuca@chromium.org, Feb 7 2017

Blocking: 670135
Now I've gotten confused after meacer@ pinged  Issue 670135 . It looks like we came up with 3 different solutions:

(1)  Issue 658900  - Fullscreen bubble should be ordered on top of other dialogs/alerts

(2)  Issue 642568  - Make all modal dialogs in fullscreen temporarily show the browser UI

(3)  Issue 670135  - Make all modal dialogs in fullscreen just exit fullscreen entirely

I think 3 is much more feasible (and given that we basically want to kill alerts anyway, I don't think a big deal). I will try to do 3.

Comment 103 by sheriffbot@chromium.org, Mar 10 2017

Project Member
Labels: -M-56 M-57

Comment 104 by jialiul@chromium.org, Mar 21 2017

Labels: -M-57 M-58

Comment 105 by ainslie@chromium.org, Mar 21 2017

Cc: hwi@chromium.org

Comment 106 by ainslie@chromium.org, Mar 21 2017

Cc: -ainslie@chromium.org

Comment 107 by mea...@chromium.org, Apr 19 2017

Have we determined which of the three options in Comment #102 we'll implement? Seems like we are leaning towards (3).

Comment 108 by mgiuca@chromium.org, May 2 2017

#107: I've decided to go ahead with (3), and closed the other bugs. Let's continue the discussion on  Issue 670135 .

Comment 109 by mgiuca@chromium.org, May 2 2017

Blockedon: 642568 658900 670135
Blocking: -642568 -658900 -670135

Comment 110 by a...@chromium.org, May 2 2017

Matt, can I give this bug to you then? Am willing to help with anything you need.

Comment 111 by mgiuca@chromium.org, May 3 2017

Owner: mgiuca@chromium.org
Yeah I'll take this (which is now effectively a meta-bug for  Issue 670135 ).

Not clear who will do engineering on it but it makes sense for me to own both.

Comment 112 by mmoroz@google.com, May 4 2017

Matt, do you have any estimate on when you can get around it?

It's a Security FixIt week, and we are tying to get rid of all stale security issues. I'd be happy to help with this, if I can :)

Comment 113 by mgiuca@chromium.org, May 4 2017

mmoroz: See https://bugs.chromium.org/p/chromium/issues/detail?id=670135#c9
(I'm considering this bug a tracking bug for that one; they would both be marked fixed at the same time.)

This is 2nd in my queue (behind another security bug). If you want to take it, go ahead: it isn't actually going to be working in any code I'm familiar with (so I have no state on it aside from what I wrote in Comment #9 there). Otherwise I'll try to get to it tomorrow (but unlikely).

Comment 114 by mmoroz@google.com, May 8 2017

Cc: mmoroz@chromium.org

Comment 115 by sheriffbot@chromium.org, Jun 6 2017

Project Member
Labels: -M-58 M-59

Comment 117 by a...@chromium.org, Jun 19 2017

Status: Fixed (was: Assigned)

Comment 118 by awhalley@chromium.org, Jun 20 2017

Labels: reward-topanel

Comment 119 by sheriffbot@chromium.org, Jun 20 2017

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

Comment 120 by sheriffbot@chromium.org, Jun 22 2017

Project Member
Labels: Merge-Request-60

Comment 121 by sheriffbot@chromium.org, Jun 22 2017

Project Member
Labels: -Merge-Request-60 Hotlist-Merge-Review Merge-Review-60
This bug requires manual review: M60 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), josafat@(ChromeOS), bustamante@(Desktop)

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

Comment 122 by abdulsyed@chromium.org, Jun 23 2017

Labels: -Merge-Review-60 Merge-Approved-60
Approving merge to M60.

Comment 123 by awhalley@chromium.org, Jun 27 2017

Labels: -reward-topanel reward-unpaid reward-3000

Comment 124 by awhalley@chromium.org, Jun 27 2017

Congratulations luan.herrera@! The VRP panel decided to award you $3,000 for this report. Nice one!

Comment 125 by awhalley@chromium.org, Jun 27 2017

Labels: -Security_Severity-Medium Security_Severity-High

Comment 126 by awhalley@chromium.org, Jun 27 2017

Labels: -reward-unpaid reward-inprocess

Comment 127 by abdulsyed@chromium.org, Jul 10 2017

Please merge this to M60 ASAP. branch:3112

Comment 128 by abdulsyed@chromium.org, Jul 18 2017

Cc: a...@chromium.org

Comment 129 by bugdroid1@chromium.org, Jul 18 2017

Project Member
Labels: -merge-approved-60 merge-merged-3112
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f36b11b74a9d97621a65d466862948b0b8650889

commit f36b11b74a9d97621a65d466862948b0b8650889
Author: Avi Drissman <avi@chromium.org>
Date: Tue Jul 18 23:38:13 2017

If JavaScript shows a dialog, cause the page to lose fullscreen.

BUG= 670135 ,  550017 ,  726761 ,  728276 
TBR=avi@chromium.org

(cherry picked from commit 0720b02e4f303ea6b114d4ae9453e3a7ff55f8dc)

Review-Url: https://codereview.chromium.org/2906133004
Cr-Original-Commit-Position: refs/heads/master@{#478884}
Change-Id: Id833bfcc88e7faf9129ceb3184e11d37a71c61cc
Reviewed-on: https://chromium-review.googlesource.com/576402
Reviewed-by: Avi Drissman <avi@chromium.org>
Cr-Commit-Position: refs/branch-heads/3112@{#644}
Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897}
[modify] https://crrev.com/f36b11b74a9d97621a65d466862948b0b8650889/chrome/browser/printing/print_job_worker.cc
[modify] https://crrev.com/f36b11b74a9d97621a65d466862948b0b8650889/chrome/browser/printing/print_view_manager.cc
[modify] https://crrev.com/f36b11b74a9d97621a65d466862948b0b8650889/content/browser/web_contents/web_contents_impl.cc
[modify] https://crrev.com/f36b11b74a9d97621a65d466862948b0b8650889/content/browser/web_contents/web_contents_impl.h
[modify] https://crrev.com/f36b11b74a9d97621a65d466862948b0b8650889/content/browser/web_contents/web_contents_impl_browsertest.cc
[modify] https://crrev.com/f36b11b74a9d97621a65d466862948b0b8650889/content/public/browser/web_contents.h

Comment 130 by mgiuca@chromium.org, Jul 19 2017

Owner: a...@chromium.org

Comment 131 by bbergher@chromium.org, Jul 19 2017

Cc: -bbergher@chromium.org

Comment 132 by awhalley@chromium.org, Jul 24 2017

Labels: -M-59 M-60 M-61

Comment 133 by awhalley@chromium.org, Jul 24 2017

Labels: Release-0-M60

Comment 134 by awhalley@chromium.org, Jul 25 2017

Labels: CVE-2017-5093

Comment 135 by sheriffbot@chromium.org, Sep 26 2017

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 136 by awhalley@chromium.org, Apr 25 2018

Labels: CVE_description-submitted
Showing comments 37 - 136 of 136 Older

Sign in to add a comment