Bug 164853 – [GTK] Handle infinite popup dialogs exploit

• Component: WebKitGTK
• Status: NEW

[Que Quotion]

Originally reported against epiphany: https://bugzilla.gnome.org/show_bug.cgi?id=774384

Although it seems possible to handle the problem in epiphany, it would be more effective to implement a means to stop Javascript from opening infinite popups, which can effectively lock out webkit application's interfaces, in the webkit backend.

[Carlos Garcia Campos]

Is this GTK+ specific?

[Michael Catanzaro]

(In reply to comment #1)
> Is this GTK+ specific?

Well it has to have some handling in the GTK+ API layer: we need to add a checkbox to the default WebKitScriptDialog to prevent the site from displaying further dialogs, like all other browsers have done for many years.

[Michael Catanzaro]

And we need to add API to allow applications to make that same decision if they override the default WebKitScriptDialog, like Epiphany does.

[Que Quotion]

>Is this GTK+ specific?
>Well it has to have some handling in the GTK+ API layer

The two of you probably know a lot more about what you are talking about than I do, but I think the point is that while gtk would need to handle the interface aspect of "click here to prevent additional popups from this site", webkit itself should provide the means to recognize that javascript is runnining an infinte popups exploit.

[Michael Catanzaro]

*** Bug 165032 has been marked as a duplicate of this bug. ***

[Michael Catanzaro]

(In reply to comment #4)
> The two of you probably know a lot more about what you are talking about
> than I do, but I think the point is that while gtk would need to handle the
> interface aspect of "click here to prevent additional popups from this
> site", webkit itself should provide the means to recognize that javascript
> is runnining an infinte popups exploit.

GTK+ doesn't know anything about WebKit. This all has to be handled in WebKit.

[Carlos Alberto Lopez Perez]

It seems this issue affects also safari / the mac port.


Test case: https://people.igalia.com/clopez/wkbug/164853/

In their case is not grave as on webkitgtk+ because the javascript alert popup don't blocks the browser UI, so you can just close the tab.

So I think there may two issues here:

 1) Make our JavaScript dialogs not blocking the main UI interface: for example, that you can minimize the dialog or move it offscreen with the mouse and still have a working UI that allows you to close that tab without having to kill the browser.

 2) Allow the user to stop a given page for creating more javascript popup dialogs. Right now, this is also not working with the mac port (safari).

[Que Quotion]

>It seems this issue affects also safari / the mac port.

Back in 2008, Mark Rowe (bdash) thought apple should handle the problem with the interface being locked out on their own and seems to have reported it upstream: <rdar://problem/5767363>

The report is against webkit/safari and I don't think the bug was checked against epiphany at that time: https://bugs.webkit.org/show_bug.cgi?id=17560

I am unable to log in to apple's bugtracker and can't get any information about how they handled it. My guess is that they found a way to work around the interface lockout in their browser without considering fixing the problem in webkit.

[Que Quotion]

Note this duplicate of the above report agaisnt webkit/safari, also from 2008: https://bugs.webkit.org/show_bug.cgi?id=17859#c0

The description suggests much the same solutions as discussed here and in the report on epiphany's bugzilla.

[Carlos Alberto Lopez Perez]

(In reply to comment #8)
> >It seems this issue affects also safari / the mac port.
> 
> Back in 2008, Mark Rowe (bdash) thought apple should handle the problem with
> the interface being locked out on their own and seems to have reported it
> upstream: <rdar://problem/5767363>
> 
> The report is against webkit/safari and I don't think the bug was checked
> against epiphany at that time: https://bugs.webkit.org/show_bug.cgi?id=17560
> 
> I am unable to log in to apple's bugtracker and can't get any information
> about how they handled it. My guess is that they found a way to work around
> the interface lockout in their browser without considering fixing the
> problem in webkit.

The safari solution is a half-backed one.

The safari UI is not blocked by the pop-up, but the webview of that tab is.

You have to close the tab, you can't just stop the popup without closing it.

I find chromium/firefox solution of adding a check-box with a "prevent this page from creating additional dialogs" option much better.

[Que Quotion]

>I find chromium/firefox solution of adding a check-box with a "prevent this page from creating additional dialogs" option much better.

I agree. There's the possibilty that this expliot could come out of an advertisement or hack on an otherwise sane and legitimate webpage. A means to stop the popups themselves is needed.

[Michael Catanzaro]

*** Bug 169859 has been marked as a duplicate of this bug. ***

[ManDay]

I agree with Carlos in comment #7 and think that not making the alert a modal window is the first thing to do. In fact, I don't see any implication why it should ever been modal, so why is it?

[ManDay]

I agree with Carlos in comment #7 and think that not making the alert a modal window is the first thing to do. In fact, I don't see any implication why it should ever been modal, so why is it?

[ManDay]

Apologies for the double (now triple) comment. I would like to add, though, that the "Are you sure you want to leave this page" dialog should equally not be modal.

[Claudio Saavedra]

(In reply to Carlos Alberto Lopez Perez from comment #7)

>  2) Allow the user to stop a given page for creating more javascript popup
> dialogs. Right now, this is also not working with the mac port (safari).

The specs indicate that the dialog should be modal unless the sandboxed modals flag is set: https://w3c.github.io/html/webappapis.html#simple-dialogs

[Claudio Saavedra]

I replied to the wrong part of the comment. I meant to say that dialog needs to be modal. Also, the specs explicitly mention that user agents might give the user the possibility to ignore the messages, so that's what we should be doing.

[ManDay]

(In reply to Claudio Saavedra from comment #17)
> I replied to the wrong part of the comment. I meant to say that dialog needs
> to be modal.

Is it sure that "modal" in this context means that the dialog should be modal w.r.t. the browser? Absent evidence to the contrary I'd say no, because the spec is by nature limited to what _the website_ should do. So anything that is said goes within the realm of each individual website, i.e. the dialog should be modal w.r.t. the website.

That would also agree with my natural feeling about any website being able (or rather, needing not) to have any effect outside of its limited domain (other than perhaps a file chooser dialog).

[ManDay]

The latter of which, by the way, may be subject to the same exploit (or can they only be triggered explicitly)?

[Claudio Saavedra]

You are right in that it's most likely that modality refers uniquely to the page.

I have been checking today how this could be fixed and it seems to me that we need to go all the way down to WebCore. Fixing this in the GTK+ side only (to avoid showing the dialog) doesn't prevent the endless communication between ui and web processes coming from the endless amount of alerts that renders the browser unusable, so it's there where the alerts need to be ignored.

There's currently in WebCore::Page a forbidPrompts()/allowPrompts() that is used to disable them when the page is unloading. I think we probably need to do something similar (this is not useful as is because it blocks all kind of prompts, including window.print()).

[ManDay]

I think once the window is no longer modal to the browser, there is no reason to prevent it. If a website wants to block itself by spawing an infinity of alerts, then it should be free to do so. The only important thing is that it may not block anything "outside" of itsself (other websites or worse, the whole browser).

[ManDay]

For what it's worth, I find the whole alert() business as it is stated in the spec an unfit relict anyway. The proper thing to do what have the website deal with its presentation itself (render a position:absolute block and gray out the background - much like most modern websites already do it). If the goal were a modern, feasible webengine first and fulfilling the spec second, the alert() thing should be gone. But sure, if it's for the label "100% conformant"....

[ManDay]

(I'm not suggesting to remove alert(), of course, since still a fair number of non-malicious websites rely on it)

[Michael Catanzaro]

(In reply to Claudio Saavedra from comment #20)
> You are right in that it's most likely that modality refers uniquely to the
> page.
> 
> I have been checking today how this could be fixed and it seems to me that
> we need to go all the way down to WebCore. Fixing this in the GTK+ side only
> (to avoid showing the dialog) doesn't prevent the endless communication
> between ui and web processes coming from the endless amount of alerts that
> renders the browser unusable, so it's there where the alerts need to be
> ignored.
> 
> There's currently in WebCore::Page a forbidPrompts()/allowPrompts() that is
> used to disable them when the page is unloading. I think we probably need to
> do something similar (this is not useful as is because it blocks all kind of
> prompts, including window.print()).

How does Apple handle this? Surely they have a way to prevent Safari users from being hit by infinite popups?

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #24)
> How does Apple handle this? Surely they have a way to prevent Safari users
> from being hit by infinite popups?

Could someone with Safari please try Carlos Lopez's DOS test page https://people.igalia.com/clopez/wkbug/164853/ and say what happens in Safari? Do you get an option to prevent the page from showing further popups?

[Claudio Saavedra]

I have checked Safari, and the main difference is that the dialogs are only modal to the webpage so they don't block the whole browser. You can just close the tab and move on. Even if we disallow popups, the user will still have to do this, as we're talking of a while(true).

Also there is no checkbox to prevent web pages from showing popups though in Safari.

[Adrian Perez]

(In reply to Claudio Saavedra from comment #26)
> I have checked Safari, and the main difference is that the dialogs are only
> modal to the webpage so they don't block the whole browser. You can just
> close the tab and move on. Even if we disallow popups, the user will still
> have to do this, as we're talking of a while(true).
> 
> Also there is no checkbox to prevent web pages from showing popups though in
> Safari.

We could make the alert dialogs webview-modal in the same way that
the prompt dialogs for HTTP authentication are made. That would allow
to close the tab/browser even in the face of alert()-spamming, and
wouldn't need changes deep down into WebCore. WDYT?

[Claudio Saavedra]

What would make sense to solve the modality issue is to use the same base dialog that is used for authentication dialogs. So basically split WebKitAuthenticationDialog to a base class that can be reused for this.

I still think we want to allow users to block alerts, as they can be pretty annoying.

[Adrian Perez]

(In reply to Claudio Saavedra from comment #28)
> What would make sense to solve the modality issue is to use the same base
> dialog that is used for authentication dialogs. So basically split
> WebKitAuthenticationDialog to a base class that can be reused for this.

Bingo. That's exactly what I meant — maybe my previous comment could have
been clearer O:-)

> I still think we want to allow users to block alerts, as they can be pretty
> annoying.

I see value in having a stopgap measure implemented first, and then improving
on that. Also, if the widget used for alerts looks and behaves in the same as
the authentication dialog, we would be providing a more coherent user interface
(and that's a nice side effect!)

[Claudio Saavedra]

Created attachment 308509 [details]
Patch

[Build Bot]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Build Bot]

Attachment 308509 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/WebPageProxy.cpp:350:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Claudio Saavedra]

Created attachment 308513 [details]
Patch

[Build Bot]

Attachment 308513 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/WebPageProxy.cpp:350:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Carlos Garcia Campos]

Comment on attachment 308513 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308513&action=review

I think we could add a layout test for this, adding private API to WKPage, or using WebCore testing to disable prompts

> Source/WebCore/ChangeLog:4
> +        Make it possible for users to disable javascript prompts
> +        https://bugs.webkit.org/show_bug.cgi?id=164853

I think this patch should be split, the fact that the title here doesn't match the bug title just confirms it. The cross-platform part first and then the GTK+ part that should include a test.

> Source/WebCore/page/DOMWindow.cpp:1156
> +    if (page->userDisabledSimplePrompts())
> +        return;
> +
>      if (!page->arePromptsAllowed()) {

Do we really want to give preference to prompts disabled by user?

> Source/WebCore/page/DOMWindow.cpp:1206
> +    if (page->userDisabledSimplePrompts())
> +        return String();
> +
>      if (!page->arePromptsAllowed()) {

This pattern is repeated everywhere, I wonder if we could merge both userDisabledSimplePrompts and arePromptsAllowed. We could add an enum with the reason why the prompts are disabled, for example. Allowed, DisabledByUser, DisabledByPageUnload. The we could also use forbidPrompts() for user disabled ones, by adding the enum as parameter. In case m_forbidPromptsDepth > 0 && m_userDisabledSimplePrompts I would probably return DisabledByPageUnload to ensure we log the error message.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:350
> +    , m_userBlocksPopups(false)

This should be initialized in the header

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1407
> +void WebPageProxy::userBlocksPopups()

So, we are mixing popups and prompts, I would use prompts since that's what we already use in WebCore. We could assume that this is called by API and then it's isabled by user, so it can simply be diablePrompts() or even forbidPrompts() for consistency with WebCore. The WebProcess would take care of calling forbidPrompts with the DisabledByUser enum vale when handling the message.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1412
> +    m_userBlocksPopups = true;

So, once disabled they can't be enabled again?

[Claudio Saavedra]

Comment on attachment 308513 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308513&action=review

>> Source/WebCore/ChangeLog:4
>> +        https://bugs.webkit.org/show_bug.cgi?id=164853
> 
> I think this patch should be split, the fact that the title here doesn't match the bug title just confirms it. The cross-platform part first and then the GTK+ part that should include a test.

We can do that. I'll move the cross-platform part to a separate bug and leave the GTK+ part here, depending on the new bug.

>> Source/WebCore/page/DOMWindow.cpp:1156
>>      if (!page->arePromptsAllowed()) {
> 
> Do we really want to give preference to prompts disabled by user?

You are right, the other one should have preference in order to log the error.

>> Source/WebCore/page/DOMWindow.cpp:1206
>>      if (!page->arePromptsAllowed()) {
> 
> This pattern is repeated everywhere, I wonder if we could merge both userDisabledSimplePrompts and arePromptsAllowed. We could add an enum with the reason why the prompts are disabled, for example. Allowed, DisabledByUser, DisabledByPageUnload. The we could also use forbidPrompts() for user disabled ones, by adding the enum as parameter. In case m_forbidPromptsDepth > 0 && m_userDisabledSimplePrompts I would probably return DisabledByPageUnload to ensure we log the error message.

Sounds good to me.

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:1407
>> +void WebPageProxy::userBlocksPopups()
> 
> So, we are mixing popups and prompts, I would use prompts since that's what we already use in WebCore. We could assume that this is called by API and then it's isabled by user, so it can simply be diablePrompts() or even forbidPrompts() for consistency with WebCore. The WebProcess would take care of calling forbidPrompts with the DisabledByUser enum vale when handling the message.

OK.

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:1412
>> +    m_userBlocksPopups = true;
> 
> So, once disabled they can't be enabled again?

As the patch stands, no, you can't. I checked chromium's behavior and it seems to me that once you disable popups, they remain disabled in that webview forever and you can't enable them there again. I hesitated between mocking that behavior or re-enabling them again when a page is loaded in the same webview. We would need extra WebProxy API (or basically the same with a boolean parameter) for that, but we can do it if you think that would be better.

[Claudio Saavedra]

Created attachment 308906 [details]
Patch

[Claudio Saavedra]

D'oh, of course this won't build without the patch in 171551. Any way to stop the bots?

[Michael Catanzaro]

Just let the bots fail.

[Michael Catanzaro]

Comment on attachment 308906 [details]
Patch

This one's not ready for review because the patch is bug #171551 got r-.