Summary: | [GTK] Crash attempting to load Flash plugin in Wayland | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Catanzaro <mcatanzaro> | ||||||||||
Component: | WebKitGTK | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | bugs-noreply, buildbot, cgarcia, mcatanzaro, rniwa, tpopela | ||||||||||
Priority: | P2 | ||||||||||||
Version: | Other | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | Linux | ||||||||||||
See Also: |
https://bugzilla.redhat.com/show_bug.cgi?id=1382789 https://bugzilla.redhat.com/show_bug.cgi?id=1420127 https://bugzilla.redhat.com/show_bug.cgi?id=1420909 |
||||||||||||
Attachments: |
|
Description
Michael Catanzaro
2016-10-07 17:13:21 PDT
So this is probably a security bug. Attempting to use GtkSocket outside X11 is guaranteed to result in a segmentation violation. But our untrusted secondary processes can force this to happen at will by sending the UI process a WebPageProxy::CreatePluginContainer message. So we literally have a message that untrusted processes can use that does nothing except crash the UI process! I remember complaining about this a long time ago, actually; shame we never fixed it. At the very least, we need to check PlatformDisplay type in WebPageProxy::createPluginContainer and bail if it's not X11. Does that sound right, Carlos? As for why the Flash plugin process is doing this, I have no clue. Guess: maybe it is somehow playing with environment variables to trick PlatformDisplay in the plugin process into creating the wrong display type. That should not cause the UI process to crash, though, because plugin process is untrusted. Created attachment 302393 [details]
Patch
Comment on attachment 302393 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302393&action=review As I said, it surely also requires an early return in WebPageProxy::CreatePluginContainer. > Source/WebKit2/UIProcess/Plugins/unix/PluginInfoStoreUnix.cpp:80 > + if (PlatformDisplay::sharedDisplay().type() == PlatformDisplay::Type::Wayland) The check should be if type != X11, not if type == Wayland. I know that makes the conditionals come out less-cleanly, but let's do it right. (In reply to comment #3) > Comment on attachment 302393 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=302393&action=review > > As I said, it surely also requires an early return in > WebPageProxy::CreatePluginContainer. No, it will never reach there, now that the bug is fixed. > > Source/WebKit2/UIProcess/Plugins/unix/PluginInfoStoreUnix.cpp:80 > > + if (PlatformDisplay::sharedDisplay().type() == PlatformDisplay::Type::Wayland) > > The check should be if type != X11, not if type == Wayland. I know that > makes the conditionals come out less-cleanly, but let's do it right. Nope, I'm not sure if it's possible but if you build with wayland and X11 disabled, you can't do that, because Type::X11 is not defined. (In reply to comment #4) > No, it will never reach there, now that the bug is fixed. As I said above, the UI process should be robust against the message being sent from a compromised or malfunctioning plugin process. > > > Source/WebKit2/UIProcess/Plugins/unix/PluginInfoStoreUnix.cpp:80 > > > + if (PlatformDisplay::sharedDisplay().type() == PlatformDisplay::Type::Wayland) > > > > The check should be if type != X11, not if type == Wayland. I know that > > makes the conditionals come out less-cleanly, but let's do it right. > > Nope, I'm not sure if it's possible but if you build with wayland and X11 > disabled, you can't do that, because Type::X11 is not defined. Then always return in that case. But you don't even have to bother with that. You know PLATFORM(X11) is guaranteed inside ENABLE_PLUGIN_PROCESS_GTK2 or ENABLE_NETSCAPE_PLUGIN_API because both have dependencies on it: # OptionsGTK.cmake WEBKIT_OPTION_DEPEND(ENABLE_NETSCAPE_PLUGIN_API ENABLE_X11_TARGET) WEBKIT_OPTION_DEPEND(ENABLE_PLUGIN_PROCESS_GTK2 ENABLE_X11_TARGET) Created attachment 302498 [details]
Patch
Comment on attachment 302498 [details] Patch Attachment 302498 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/3177489 New failing tests: media/modern-media-controls/volume-down-support/volume-down-support.html Created attachment 302500 [details]
Archive of layout-test-results from ews116 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 302498 [details] Patch Attachment 302498 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3178367 New failing tests: editing/spelling/spellcheck-async.html Created attachment 302505 [details]
Archive of layout-test-results from ews100 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 302498 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=302498&action=review This looks better. > Source/WebKit2/UIProcess/gtk/WebPageProxyGtk.cpp:106 > + ASSERT(WebCore::PlatformDisplay::sharedDisplay().type() == WebCore::PlatformDisplay::Type::X11); r=me if and only if you change this to CRASH() or RELEASE_ASSERT(). (In reply to comment #11) > Comment on attachment 302498 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=302498&action=review > > This looks better. > > > Source/WebKit2/UIProcess/gtk/WebPageProxyGtk.cpp:106 > > + ASSERT(WebCore::PlatformDisplay::sharedDisplay().type() == WebCore::PlatformDisplay::Type::X11); > > r=me if and only if you change this to CRASH() or RELEASE_ASSERT(). This message is not sent by the plugin process (untrusted) but by the web process (trusted) so I don't think we need a release assert. I added it only to make you happy, not because I think is needed. Carlos, the web process is not trusted either. I know we don't sandbox the web process yet, but that's not reason to ignore the WebKit2 security model. Committed r212891: <http://trac.webkit.org/changeset/212891> |