Bug 171551

Summary: Allow forbidding javascript prompts
Product: WebKit Reporter: Claudio Saavedra <csaavedra>
Component: New BugsAssignee: Claudio Saavedra <csaavedra>
Status: NEW ---    
Severity: Normal CC: aperez, beidson, buildbot, cdumez, cgarcia, dbates, ggaren, japhet, mcatanzaro, rniwa
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 164853    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews103 for mac-elcapitan
none
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews123 for ios-simulator-wk2
none
Patch
none
Archive of layout-test-results from ews102 for mac-elcapitan
none
Archive of layout-test-results from ews107 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews114 for mac-elcapitan
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Patch ggaren: review-

Description Claudio Saavedra 2017-05-02 07:23:56 PDT 
Allow forbidding javascript prompts
Comment 1 Claudio Saavedra 2017-05-02 07:24:40 PDT 
Created attachment 308821 [details]
Patch
Comment 2 Claudio Saavedra 2017-05-02 09:20:27 PDT 
Created attachment 308825 [details]
Patch
Comment 3 Geoffrey Garen 2017-05-02 09:47:53 PDT 
Comment on attachment 308825 [details]
Patch

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

Prompting is not built into WebKit by default. Instead, WebKit sends a delegate message to the client, and then the client has the option to prompt. 

If a client doesn't want any prompting ever, isn't the best way to get that behavior simply not to implement the delegate messages?

Or maybe you envision a client disabling prompts but only temporarily? If so, under what conditions (other than page unload) do you expect clients to disable prompts? And why do a client need WebKit to track those conditions (instead of tracking them itself)?

> Source/WebCore/loader/FrameLoader.cpp:208
> +        m_page->forbidPrompts(Page::PromptPolicy::DisabledByPageUnload);

I would rename this class to DisablePromptsForPageUnload and rename the enum value DisabledForPageUnload.

It seems wrong for an abstract ForbidPromptsScope to assume that it is used only for page unload.

> Source/WebCore/page/DOMWindow.cpp:1121
> +    if (page->arePromptsAllowed() == Page::PromptPolicy::DisabledByPageUnload) {

I would rename arePromptsAllowed to promptPolicy, since you've changed it to an enum.

I think this comparison has a bug in it. In the DisabledByUser case, the equality comparison will return false, and you'll still print. I believe you could have caught this bug with a simple regression test. Can you add regression tests? We usually require a regression test for every patch.

> Source/WebCore/page/DOMWindow.cpp:1155
> +    case Page::PromptPolicy::DisabledByUser:
>          return;

I would name this case just "Disabled". There's nothing about the setting or its behavior that ties to a user action.

> Source/WebCore/page/DOMWindow.cpp:1160
> +    default:
> +        break;

This code is meaningless, since the default behavior of an unmatched switch is to do nothing.

The net effect of writing this code is that you disable the compiler's ability to verify that you've handled all possible enum values. That's bad. You should add a case for Enabled and remove default.

> Source/WebCore/page/DOMWindow.cpp:1187
> +    default:
> +        break;

Ditto.

> Source/WebCore/page/DOMWindow.cpp:1214
> +    default:
> +        break;

Ditto.

> Source/WebCore/page/DOMWindow.cpp:2327
> +    if (page->arePromptsAllowed() == Page::PromptPolicy::DisabledByPageUnload) {

Same bug here as above.

> Source/WebCore/page/Page.cpp:2067
> +    default:
> +        ASSERT_NOT_REACHED();

Same comment about default.

> Source/WebCore/page/Page.h:771
>      unsigned m_lastSpatialNavigationCandidatesCount;
>      unsigned m_forbidPromptsDepth;
> +    bool m_userDisabledSimplePrompts;

I would call m_forbidPromptsDepth "m_disablePromptsForPageUnloadDepth" and m_userDisabledSimplePrompts "m_disablePrompts".

> Source/WebKit2/UIProcess/WebPageProxy.h:1668
> +    bool m_promptsForbidden { false };

Why do we need this extra state? Do we expect clients to needlessly call this API repeatedly?
Comment 4 Claudio Saavedra 2017-05-03 01:52:16 PDT 
(In reply to Geoffrey Garen from comment #3)
> Comment on attachment 308825 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=308825&action=review
> 
> Prompting is not built into WebKit by default. Instead, WebKit sends a
> delegate message to the client, and then the client has the option to
> prompt. 
> 
> If a client doesn't want any prompting ever, isn't the best way to get that
> behavior simply not to implement the delegate messages?
> 
> Or maybe you envision a client disabling prompts but only temporarily? If
> so, under what conditions (other than page unload) do you expect clients to
> disable prompts? And why do a client need WebKit to track those conditions
> (instead of tracking them itself)?

I had explained this in the bug that depends on this, which is about handling web pages sending endless amount of prompts (via while(1) alert(), for example). The problem with dealing with this on the client side (by just not handling the prompt requests) is that this doesn't stop the web process from sending the prompt requests to the UI process, which in turn locks the UI process.

We need a way to stop these requests from reaching the UI process in the first place, so that only the WebProcess is in infinite loop and users can just close the offending tab without the browser UI having to be killed.
Comment 5 Claudio Saavedra 2017-05-03 02:26:59 PDT 
Comment on attachment 308825 [details]
Patch

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

>> Source/WebCore/page/DOMWindow.cpp:1121
>> +    if (page->arePromptsAllowed() == Page::PromptPolicy::DisabledByPageUnload) {
> 
> I would rename arePromptsAllowed to promptPolicy, since you've changed it to an enum.
> 
> I think this comparison has a bug in it. In the DisabledByUser case, the equality comparison will return false, and you'll still print. I believe you could have caught this bug with a simple regression test. Can you add regression tests? We usually require a regression test for every patch.

This is not a bug; window.print() is not considered a "simple prompt" of the kind that can be user disabled (those are alert, confirm, and prompt).

>> Source/WebCore/page/DOMWindow.cpp:1155
>>          return;
> 
> I would name this case just "Disabled". There's nothing about the setting or its behavior that ties to a user action.

It is tied to a user action in the sense that we're making it possible for users to disable simple prompts. Alternatively, we could call it SimplePromptsDisabled to make it explicit that it's about simple prompts and not tie it directly to user actions.

>> Source/WebCore/page/DOMWindow.cpp:2327
>> +    if (page->arePromptsAllowed() == Page::PromptPolicy::DisabledByPageUnload) {
> 
> Same bug here as above.

Same as I explained above.

>> Source/WebCore/page/Page.h:771
>> +    bool m_userDisabledSimplePrompts;
> 
> I would call m_forbidPromptsDepth "m_disablePromptsForPageUnloadDepth" and m_userDisabledSimplePrompts "m_disablePrompts".

I'll call the latter m_disableSimplePrompts.

>> Source/WebKit2/UIProcess/WebPageProxy.h:1668
>> +    bool m_promptsForbidden { false };
> 
> Why do we need this extra state? Do we expect clients to needlessly call this API repeatedly?

Not really. Clients would only call this on response to a prompt, so you're right that we don't need this.
Comment 6 Claudio Saavedra 2017-05-03 06:32:12 PDT 
Created attachment 308900 [details]
Patch
Comment 7 Build Bot 2017-05-03 07:24:49 PDT Comment hidden (obsolete)
Comment 8 Build Bot 2017-05-03 07:24:51 PDT Comment hidden (obsolete)
Comment 9 Build Bot 2017-05-03 07:28:50 PDT Comment hidden (obsolete)
Comment 10 Build Bot 2017-05-03 07:28:52 PDT Comment hidden (obsolete)
Comment 11 Build Bot 2017-05-03 07:57:15 PDT Comment hidden (obsolete)
Comment 12 Build Bot 2017-05-03 07:57:16 PDT Comment hidden (obsolete)
Comment 13 Claudio Saavedra 2017-05-03 08:13:58 PDT 
Created attachment 308905 [details]
Patch
Comment 14 Build Bot 2017-05-03 09:26:04 PDT Comment hidden (obsolete)
Comment 15 Build Bot 2017-05-03 09:26:05 PDT Comment hidden (obsolete)
Comment 16 Build Bot 2017-05-03 09:31:57 PDT Comment hidden (obsolete)
Comment 17 Build Bot 2017-05-03 09:31:58 PDT Comment hidden (obsolete)
Comment 18 Build Bot 2017-05-03 09:40:59 PDT Comment hidden (obsolete)
Comment 19 Build Bot 2017-05-03 09:41:00 PDT Comment hidden (obsolete)
Comment 20 Build Bot 2017-05-03 09:56:03 PDT Comment hidden (obsolete)
Comment 21 Build Bot 2017-05-03 09:56:05 PDT Comment hidden (obsolete)
Comment 22 Claudio Saavedra 2017-05-04 04:15:19 PDT 
Created attachment 309030 [details]
Patch
Comment 23 Claudio Saavedra 2017-05-08 04:02:06 PDT 
Could I get a new review please?
Comment 24 Geoffrey Garen 2017-05-08 12:14:14 PDT 
> I had explained this in the bug that depends on this, which is about
> handling web pages sending endless amount of prompts (via while(1) alert(),
> for example). The problem with dealing with this on the client side (by just
> not handling the prompt requests) is that this doesn't stop the web process
> from sending the prompt requests to the UI process, which in turn locks the
> UI process.
> 
> We need a way to stop these requests from reaching the UI process in the
> first place, so that only the WebProcess is in infinite loop and users can
> just close the offending tab without the browser UI having to be killed.

The design in WebCore is that the client gets a completion handler, and no further messages are sent to the client until the completion handler is invoked. This allows the client to make a decision at its leisure, or to close the webpage, without hanging.

Is there a particular reason that this design doesn't work for the GTK port?

It looks like the GTK implementation of runJavaScriptAlert doesn't pass the completion handler to its API client, and instead unconditionally invokes the completion handler right away. That probably explains why GTK clients hang:

    void runJavaScriptAlert(WebPageProxy*, const String& message, WebFrameProxy*, const WebCore::SecurityOriginData&, Function<void ()>&& completionHandler) override
    {
        webkitWebViewRunJavaScriptAlert(m_webView, message.utf8());
        completionHandler();
    }

Is it possible to update the GTK API to match WebCore's design and provide a completion handler to its client?

I read bug 164853 and I'm still not sure what the intended purpose of this setting is. 
Is the idea that to prevent the webpage from hanging? If so, this setting doesn't achieve its goal because "while (1) alert();" will hang even if you don't display a prompt.

Is the idea that the user would click a checkbox that said "disable further prompts from this webpage"? If so, the client can implement that behavior by not displaying a prompt in response to runJavaScriptAlert().

Is the idea that the webpage should stop messaging the UI process at all? If so, this setting is not sufficient, since there are lots of ways for a webpage to message the UI process.
Comment 25 Claudio Saavedra 2017-05-09 05:07:46 PDT 
There are two important cases that need to be addressed.

1. The page is displaying alerts every once in a while which are disruptive but do not block the webpage entirely (say, one ad every 30 seconds).
2. The page is maliciously blocking the browser with a "while(1) alert()" script.

I think we need to be able to address both in a consistent way. (1) is something for which the user should be able to block the prompts and continue using the page. For (2) the user should be able to close the page without losing the browser UI and other tabs.

The way Safari deals with these issues is by making prompts modal to the web content only, and not to the browser. This is useful to prevent (2) (as it allows users to close the blocked tab), but does nothing to address (1).

For WebKitGTK+, I would like to be able to address both issues consistently. This is what the proposed patch is about.

It's also worth noting that currently WebKitGTK+'s prompts are modal to the whole browser UI, which is something I am working on changing at the moment as well. This makes a big difference, because in the case of (2) the whole browser is blocked. In any case, just addressing the modality of the dialogs is not enough, as explained above, because of (1).

(In reply to Geoffrey Garen from comment #24)

> The design in WebCore is that the client gets a completion handler, and no
> further messages are sent to the client until the completion handler is
> invoked. This allows the client to make a decision at its leisure, or to
> close the webpage, without hanging.
> 
> Is there a particular reason that this design doesn't work for the GTK port?
> 
> It looks like the GTK implementation of runJavaScriptAlert doesn't pass the
> completion handler to its API client, and instead unconditionally invokes
> the completion handler right away. That probably explains why GTK clients
> hang:
> 
>     void runJavaScriptAlert(WebPageProxy*, const String& message,
> WebFrameProxy*, const WebCore::SecurityOriginData&, Function<void ()>&&
> completionHandler) override
>     {
>         webkitWebViewRunJavaScriptAlert(m_webView, message.utf8());
>         completionHandler();
>     }
> 
> Is it possible to update the GTK API to match WebCore's design and provide a
> completion handler to its client?

To the best of my understanding what you're proposing would only be useful for (2). A user can decide to ignore further prompts, our API client would not call the completion handler, and then the tab would be blocked waiting for it, but the user can now close the page.

However this is not useful for (1).

> Is the idea that to prevent the webpage from hanging? If so, this setting
> doesn't achieve its goal because "while (1) alert();" will hang even if you
> don't display a prompt.

Yes, but only the web process will hang, as explained above.

> Is the idea that the user would click a checkbox that said "disable further
> prompts from this webpage"? If so, the client can implement that behavior by not displaying a prompt in response to runJavaScriptAlert().
> 

If we do this, then we're not taking care of (2).
Comment 26 Michael Catanzaro 2017-05-09 06:52:34 PDT 
> To the best of my understanding what you're proposing would only be useful
> for (2). A user can decide to ignore further prompts, our API client would
> not call the completion handler, and then the tab would be blocked waiting
> for it, but the user can now close the page.
> 
> However this is not useful for (1).

What happens if you call webkitWebViewRunJavaScriptAlert() but never call the completionHandler? Then you should get the behavior you desire for case (1), right? No further dialogs would be created. So this could be fixed at the API layer if you figure out how to store the completionHandler so it can be invoked after the user closes the dialog and does not check the "no more dialogs please" checkbox. In general, WebCore prefers to outsource these sorts of policy decisions to the API layer.
Comment 27 Michael Catanzaro 2017-05-09 07:00:27 PDT 
(In reply to Geoffrey Garen from comment #24)
> Is the idea that the webpage should stop messaging the UI process at all? If
> so, this setting is not sufficient, since there are lots of ways for a
> webpage to message the UI process.

I think this is a key point too. It's bad if the web process can hang the UI process, but if there are lots of ways for a malicious webpage that has not compromised the web process to hang the UI process from JavaScript, then preventing this one case is not particularly valuable.

However, if there are not lots of ways for a malicious webpage to do that with JavaScript, then it seems worthwhile to fix this, and it also seems like that requires Claudio's approach here. Correct?

I wonder: are there currently lots of ways for a malicious webpage to hang the UI process, or is this the only one we know of?
Comment 28 Claudio Saavedra 2017-05-09 07:29:20 PDT 
(In reply to Michael Catanzaro from comment #26)
> > To the best of my understanding what you're proposing would only be useful
> > for (2). A user can decide to ignore further prompts, our API client would
> > not call the completion handler, and then the tab would be blocked waiting
> > for it, but the user can now close the page.
> > 
> > However this is not useful for (1).
> 
> What happens if you call webkitWebViewRunJavaScriptAlert() but never call
> the completionHandler? 

I tested this earlier today, the page hangs waiting for the completion handler.

> Then you should get the behavior you desire for case
> (1), right? No further dialogs would be created. So this could be fixed at
> the API layer if you figure out how to store the completionHandler so it can
> be invoked after the user closes the dialog and does not check the "no more
> dialogs please" checkbox. In general, WebCore prefers to outsource these
> sorts of policy decisions to the API layer.

This is already happening in this order, because the dialogs are running in a gtk_dialog_run(), which has its own loop. I think that's why the completion handler is not passed to the API client.
Comment 29 Geoffrey Garen 2017-05-09 08:56:20 PDT 
> There are two important cases that need to be addressed.
> 
> 1. The page is displaying alerts every once in a while which are disruptive
> but do not block the webpage entirely (say, one ad every 30 seconds).
> 2. The page is maliciously blocking the browser with a "while(1) alert()"
> script.

OK, I understand these examples.

Note that when you talk about use case (1), you fold together both a description of a problem and an assumption about an imaginary solution.

I agree with your description of the problem: There should be a defense against a malicious webpage that periodically pops up an alert, even in web browsers for which alerts are app-modal. (The defense I've suggested is that the app should just stop displaying those alerts.)

But I don't agree with your assumption that a defense against alerts can also reliably return a malicious website to safety and usefulness.

> I think we need to be able to address both in a consistent way. (1) is
> something for which the user should be able to block the prompts and
> continue using the page. For (2) the user should be able to close the page
> without losing the browser UI and other tabs.

I think I would need more evidence to be convinced that, for (1), it's a goal to make a webpage that has been compromised by a malicious ad safe and useable, or that a setting about simple prompts achieves that goal.

Can you give some examples of webpages that have been compromised by malicious ads, where the only malicious behavior in the ads was to display an alert every 30 seconds?

Once you eliminate the alert API, what makes you believe that malicious ads won't use other techniques to disrupt webpages?

Why is it good for users to interact with webpages that have been compromised by malicious ads?

In our current design, the preferred way for a client to deny alerts to a malicious webpage is just to not invoke the completion handler in runJavaScriptAlert(), thereby interrupting the webpage and allowing the user to close it.

You object to this solution because you want to allow the malicious webpage to keep running.

Our current design allows for this non-preferred solution too: a client can respond to runJavaScriptAlert() by doing nothing and then invoking the completion handler immediately, allowing the malicious webpage to keep running.

You object to this solution because, while it achieves your primary goals of not displaying an alert and allowing the webpage to continue running, it is still subject to a secondary attack where the webpage keeps messaging the UI process at a high enough rate to cause responsiveness problems.

This secondary attack is possible through many different APIs that message the UI process. Therefore, your proposed setting about alerts does not solve this secondary attack.

The only way I can think of to solve this secondary attack is to stall the malicious web process. That's exactly what the preferred solution of not invoking the completion handler in runJavaScriptAlert() achieves.

I understand that you would prefer not to stall the malicious web process and not to be subject to performance problems caused by it. But I don't think you've proposed a solution that achieves that goal, and I can't think of any solution that would achieve that goal.
Comment 30 Claudio Saavedra 2017-05-09 09:18:02 PDT 
(In reply to Geoffrey Garen from comment #29)
> > There are two important cases that need to be addressed.
> > 
> > 1. The page is displaying alerts every once in a while which are disruptive
> > but do not block the webpage entirely (say, one ad every 30 seconds).
> > 2. The page is maliciously blocking the browser with a "while(1) alert()"
> > script.
> 
> OK, I understand these examples.
> 
> Note that when you talk about use case (1), you fold together both a
> description of a problem and an assumption about an imaginary solution.
> 
> I agree with your description of the problem: There should be a defense
> against a malicious webpage that periodically pops up an alert, even in web
> browsers for which alerts are app-modal. (The defense I've suggested is that
> the app should just stop displaying those alerts.)
> 
> But I don't agree with your assumption that a defense against alerts can
> also reliably return a malicious website to safety and usefulness.
> 
> > I think we need to be able to address both in a consistent way. (1) is
> > something for which the user should be able to block the prompts and
> > continue using the page. For (2) the user should be able to close the page
> > without losing the browser UI and other tabs.
> 
> I think I would need more evidence to be convinced that, for (1), it's a
> goal to make a webpage that has been compromised by a malicious ad safe and
> useable, or that a setting about simple prompts achieves that goal.

I never said that (1) is necessarily a compromised page. It just need to be an annoying web page, of which there are plenty, that displays alerts unnecessarily. I don't know why you're talking exclusively about compromised pages.

> 
> Can you give some examples of webpages that have been compromised by
> malicious ads, where the only malicious behavior in the ads was to display
> an alert every 30 seconds?

No, because that was just a trivial example. I literally wrote "The page is displaying alerts every once in a while which are disruptive  but do not block the webpage entirely". There are plenty of those, from pages that display a popup when you change fields in a form to pages that ask you to bookmark them. Just because they make poor design choices it doesn't mean that the pages are malicious.

> Once you eliminate the alert API, what makes you believe that malicious ads
> won't use other techniques to disrupt webpages?
>
> Why is it good for users to interact with webpages that have been
> compromised by malicious ads?

Again, you keep talking exclusively of the case of malicious ads. This is not what I intended when I made that example.

> 
> In our current design, the preferred way for a client to deny alerts to a
> malicious webpage is just to not invoke the completion handler in
> runJavaScriptAlert(), thereby interrupting the webpage and allowing the user
> to close it.
> 
> You object to this solution because you want to allow the malicious webpage
> to keep running.

I object to this because I think non-malicious pages with poor ui interaction choices to continue to run.

> > 
> This secondary attack is possible through many different APIs that message
> the UI process. Therefore, your proposed setting about alerts does not solve
> this secondary attack.

I don't understand your reasoning. You're basically telling me that because there are other ways in which a page could DOS a browser we shouldn't fix this one?

What other possible attacks do you have in mind? How common are they? If you can give me concrete examples I can work in a way to deal with all them if feasible. But if your answer is "we cannot solve every imaginable attack so let's not fix this one" I don't think it's reasonable.
Comment 31 Brady Eidson 2017-05-09 15:02:25 PDT 
Is your goal to change WebKit such that it is actually impossible for malicious JS to message the UI process in a tight loop?

If so, I assure you that goal is not possible.

Geoff's suggestions here are rooted in significant experience exploring and solving this exact same problem, over years, as the attacks developed. I believe he is right that this can be solved in the WebKit client (e.g., Safari)

But let's say it can hypothetically be demonstrated that there is some fundamental issue with WebKitGTK or the platforms it runs on that makes the Web->UI process message somehow more problematic than it is on macOS/iOS. Even in that scenario, this patch is wrong in that it touches WebCore.

There's no reason to touch WebCore at all to accomplish what this patch is trying to do - It can all be done in WebKit2 in WebProcess code.

That said, I still think approaching the problem this way is folly.
Comment 32 Geoffrey Garen 2017-05-10 13:17:59 PDT 
> I never said that (1) is necessarily a compromised page. It just need to be
> an annoying web page, of which there are plenty, that displays alerts
> unnecessarily. I don't know why you're talking exclusively about compromised
> pages.

The reason I've focused on malicious webpages is that WebCore's current API works fine for benign but poorly designed webpages: If a webpage has displayed too many alerts, and you don't want it to display more, then respond to runJavaScriptAlert() by doing nothing and invoking the completion handler right away. A separate setting for this would be redundant, and it would obfuscate the purpose of the completion handler design.

Can you comment on whether the completion handler design works in the case of a benign but poorly designed webpage that asks the user to add a bookmark once every 30 seconds?

> I don't understand your reasoning. You're basically telling me that because
> there are other ways in which a page could DOS a browser we shouldn't fix
> this one?

Notice that you're talking about a Denial of Service attack here. I usually consider those attacks to be malicious. But you've told me that we're not talking about malicious pages. So I'm confused.

If we are talking about malicious pages, then I'll repeat that the best way to combat over-messaging from a malicious page is to interrupt the page, which the completion handler design allows, and the alert setting does not achieve.
Comment 33 Claudio Saavedra 2017-05-11 00:26:16 PDT 
(In reply to Geoffrey Garen from comment #32)

> Notice that you're talking about a Denial of Service attack here. I usually
> consider those attacks to be malicious. But you've told me that we're not
> talking about malicious pages. So I'm confused.

You're confused because you keep merging two different issues (ie, (1) and (2)) into one. But whatever.
Comment 34 Michael Catanzaro 2017-05-11 05:31:06 PDT 
Geoff's explanation makes sense to me. Can you explain more clearly what part you think he is wrong about?