Bug 181560

Summary: The WebContent process should not process incoming IPC while waiting for a sync IPC reply
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, beidson, commit-queue, ews-watchlist, ggaren, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=184183
Bug Depends on: 182013    
Bug Blocks:    
Attachments:
Description Flags
WIP Patch
none
WIP Patch
none
WIP Patch
ews-watchlist: commit-queue-
Archive of layout-test-results from ews104 for mac-sierra-wk2
none
WIP Patch
none
WIP Patch
none
WIP Patch
none
WIP Patch
none
WIP Patch
none
WIP Patch
none
WIP Patch
none
Patch none

Description Chris Dumez 2018-01-11 15:16:34 PST
The WebContent process should not process incoming IPC while waiting for a sync IPC reply.
Comment 1 Chris Dumez 2018-01-11 15:17:17 PST
Created attachment 331124 [details]
WIP Patch
Comment 2 Alexey Proskuryakov 2018-01-11 19:39:10 PST
Wouldn’t this cause unresolvable deadlocks?
Comment 3 Chris Dumez 2018-01-11 20:00:27 PST
(In reply to Alexey Proskuryakov from comment #2)
> Wouldn’t this cause unresolvable deadlocks?

It could but as you can see tests are passing. I also tried API tests locally.
I do not agree that it would be unresolvable though. If somebody relies on a Sync IPC 1from the WebProcess to another process and then that other process does another sync IPC 2 to the WebProcess, then we can fix it by refactoring the code so that IPC 2 is not needed (or if we know that it is a safe time, we can set the flag on the sendSync() to allow it).
Comment 4 Chris Dumez 2018-01-11 20:07:02 PST
Created attachment 331162 [details]
WIP Patch
Comment 5 Ryosuke Niwa 2018-01-11 20:23:50 PST
(In reply to Alexey Proskuryakov from comment #2)
> Wouldn’t this cause unresolvable deadlocks?

The key is that we still allow UI process to process sync messages while it awaits for a  sync reply. It means that if WebContent process needs a sync reply from UI process's sync message, then UIProcess can respond to that.

Note that it is possible to introduce a deadlock in this situation if WebContent process sends a sync message and UI process needs to send WebContent process another sync message before answering that sync message. Hopefully, we don't have such a message, and Chris' testing shows that we don't seem to have such a message.
Comment 6 Chris Dumez 2018-01-12 09:01:22 PST
Created attachment 331206 [details]
WIP Patch
Comment 7 EWS Watchlist 2018-01-12 10:12:06 PST
Comment on attachment 331206 [details]
WIP Patch

Attachment 331206 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/6050841

Number of test failures exceeded the failure limit.
Comment 8 EWS Watchlist 2018-01-12 10:12:08 PST
Created attachment 331212 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 9 Chris Dumez 2018-01-12 10:47:30 PST
Created attachment 331215 [details]
WIP Patch
Comment 10 EWS Watchlist 2018-01-12 10:49:26 PST
Attachment 331215 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/Plugins/PluginProcessConnection.cpp:53:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Chris Dumez 2018-01-12 12:05:30 PST
Created attachment 331220 [details]
WIP Patch
Comment 12 Chris Dumez 2018-01-12 12:22:13 PST
Created attachment 331224 [details]
WIP Patch
Comment 13 Chris Dumez 2018-01-12 12:31:16 PST
Created attachment 331226 [details]
WIP Patch
Comment 14 Chris Dumez 2018-01-12 13:09:20 PST
Created attachment 331229 [details]
WIP Patch
Comment 15 Chris Dumez 2018-01-12 13:26:27 PST
Created attachment 331231 [details]
WIP Patch
Comment 16 Chris Dumez 2018-01-12 15:12:05 PST
Created attachment 331240 [details]
WIP Patch
Comment 17 Chris Dumez 2018-01-12 15:22:06 PST
Created attachment 331242 [details]
Patch
Comment 18 Alexey Proskuryakov 2018-01-12 22:02:18 PST
Comment on attachment 331242 [details]
Patch

Layout tests are useless to verify this change, as we always use the UseFullySynchronousModeForTesting mode there. Also, we don't have high fidelity tests for inline input.

> Note that it is possible to introduce a deadlock in this situation if WebContent process sends a sync message and UI process needs to send WebContent process another sync message before answering that sync message. Hopefully, we don't have such a message, and Chris' testing shows that we don't seem to have such a message.

UI processes sends sync messages at arbitrary times (text input client calls is a notable case of sync messages without a timeout). So I don't see why you hope that there aren't such messages.

This approach continues to look wrong to me. Furthermore, I do not agree that this behavior is worth being concerned about. It is not unexpected that things can happen while waiting for a sync response. In fact, it is exactly the same behavior that we have in legacy WebKit - delegate callouts may and do reenter.
Comment 19 Ryosuke Niwa 2018-01-12 23:18:41 PST
(In reply to Alexey Proskuryakov from comment #18)
> Comment on attachment 331242 [details]
> Patch
> 
> Layout tests are useless to verify this change, as we always use the
> UseFullySynchronousModeForTesting mode there. Also, we don't have high
> fidelity tests for inline input.
> 
> > Note that it is possible to introduce a deadlock in this situation if WebContent process sends a sync message and UI process needs to send WebContent process another sync message before answering that sync message. Hopefully, we don't have such a message, and Chris' testing shows that we don't seem to have such a message.
> 
> UI processes sends sync messages at arbitrary times (text input client calls
> is a notable case of sync messages without a timeout). So I don't see why
> you hope that there aren't such messages.

That's okay. UI processes sending a sync message to WebContent process would always work unless it's doing so while responding to a sync message from WebContent process.

> This approach continues to look wrong to me. Furthermore, I do not agree
> that this behavior is worth being concerned about. It is not unexpected that
> things can happen while waiting for a sync response. In fact, it is exactly
> the same behavior that we have in legacy WebKit - delegate callouts may and
> do reenter.

The problem is that many sync IPC messages are sent from WebContent process while it's not save to execute arbitrary scripts. We have a security bug whenever we happen to process sync messages while waiting for the reply.
Comment 20 Chris Dumez 2018-01-13 09:26:40 PST
(In reply to Alexey Proskuryakov from comment #18)
> Comment on attachment 331242 [details]
> Patch
> 
> Layout tests are useless to verify this change, as we always use the
> UseFullySynchronousModeForTesting mode there. Also, we don't have high
> fidelity tests for inline input.
> 
> > Note that it is possible to introduce a deadlock in this situation if WebContent process sends a sync message and UI process needs to send WebContent process another sync message before answering that sync message. Hopefully, we don't have such a message, and Chris' testing shows that we don't seem to have such a message.
> 
> UI processes sends sync messages at arbitrary times (text input client calls
> is a notable case of sync messages without a timeout). So I don't see why
> you hope that there aren't such messages.

Yes, the UIProcess sends sync messages to the WebProcess at arbitrary times, and this is fine. The only thing that is not fine is that the WebProcess would potentially process those incoming sync IPC messages while waiting for the reply to another Sync message the WebProcess sent. This keeps being a source of crashes and security bug, which is why we are trying to change this behavior.

Note that these sync IPC messages from the UIProcess will still be processed. However, they will not be processed while the WebProcess is waiting for a sync reply. It will wait for that sync reply to be received before processing other incoming IPC.

Yes, there is a case where this could cause deadlocks in the specific scenario mentioned earlier. So far, we do not have evidence this is happening though. However, we have a lot of evidence of crashes happening due to the current IPC behavior. If we do cause a deadlock (for something not covered by tests), we can either refactor the code or whitelist the message in question if it happens at a safe time (I.e script is allowed to run).

> 
> This approach continues to look wrong to me. Furthermore, I do not agree
> that this behavior is worth being concerned about. It is not unexpected that
> things can happen while waiting for a sync response. In fact, it is exactly
> the same behavior that we have in legacy WebKit - delegate callouts may and
> do reenter.
Comment 21 Alexey Proskuryakov 2018-01-13 19:29:40 PST
> UI processes sending a sync message to WebContent process would always work unless it's doing so while responding to a sync message from WebContent process.

I see. Maybe we don't have such cases between WebContent process and other processes indeed. But I don't know about all sync messages that UI process sends.

> The problem is that many sync IPC messages are sent from WebContent process while it's not save to execute arbitrary scripts. We have a security bug whenever we happen to process sync messages while waiting for the reply.

Sure, but how many are cases where the same issue won't happen in WebKit1, to with bundle clients? It's normal for a delegate to perform unexpected work, and swiping one specific case under the carpet seems like a poor defense.
Comment 22 Ryosuke Niwa 2018-01-13 20:00:42 PST
(In reply to Alexey Proskuryakov from comment #21)
>
> > The problem is that many sync IPC messages are sent from WebContent process while it's not save to execute arbitrary scripts. We have a security bug whenever we happen to process sync messages while waiting for the reply.
> 
> Sure, but how many are cases where the same issue won't happen in WebKit1,
> to with bundle clients? It's normal for a delegate to perform unexpected
> work, and swiping one specific case under the carpet seems like a poor
> defense.

Indeed, delegate callbacks in WK1 and bundle callbacks are an issue. In fact, we had to disable my security assertion in WK1 because many embedding apps are running scripts when it's unsafe to do so. I don't think there is a practical way to fix that in WK1 unfortunately because we simply can't make running arbitrary scripts in the middle of layout, etc... safe. Since injected bundle's callbacks aren't a part of public API (they're only usable by the third party apps), this patch still materially improves the security of WK2 clients including but not limited to Safari.

As is, we have dozens upon dozens of crashes and security bugs due to the current IPC behavior, and there is no practical way to address all those issues by moving IPC outside the unsafe code paths because there are simply too many of them.
Comment 23 Alexey Proskuryakov 2018-01-18 09:04:44 PST
Comment on attachment 331242 [details]
Patch

I'm not comfortable enough with this patch to r+, but I don't feel comfortable blocking it either. Sorry for the muddled message.
Comment 24 Ryosuke Niwa 2018-01-18 14:33:55 PST
Comment on attachment 331242 [details]
Patch

r=me I think this change makes sense. We'll do more manual testing like typing fast on iPads per Alexey's suggestion.
Comment 25 WebKit Commit Bot 2018-01-19 10:38:56 PST
Comment on attachment 331242 [details]
Patch

Clearing flags on attachment: 331242

Committed r227216: <https://trac.webkit.org/changeset/227216>
Comment 26 WebKit Commit Bot 2018-01-19 10:38:58 PST
All reviewed patches have been landed.  Closing bug.
Comment 27 Radar WebKit Bug Importer 2018-01-19 10:41:05 PST
<rdar://problem/36663395>