Summary: | The WebContent process should not process incoming IPC while waiting for a sync IPC reply | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||||||||||||||
Component: | WebKit2 | Assignee: | 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
Chris Dumez
2018-01-11 15:16:34 PST
Created attachment 331124 [details]
WIP Patch
Wouldn’t this cause unresolvable deadlocks? (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). Created attachment 331162 [details]
WIP Patch
(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. Created attachment 331206 [details]
WIP Patch
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. 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
Created attachment 331215 [details]
WIP Patch
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.
Created attachment 331220 [details]
WIP Patch
Created attachment 331224 [details]
WIP Patch
Created attachment 331226 [details]
WIP Patch
Created attachment 331229 [details]
WIP Patch
Created attachment 331231 [details]
WIP Patch
Created attachment 331240 [details]
WIP Patch
Created attachment 331242 [details]
Patch
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. (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. (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. > 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. (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 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 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 on attachment 331242 [details] Patch Clearing flags on attachment: 331242 Committed r227216: <https://trac.webkit.org/changeset/227216> All reviewed patches have been landed. Closing bug. |