Bug 163159 – [GTK] Crash attempting to load Flash plugin in Wayland

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Michael Catanzaro]

Seems we still have problems with windowed plugins being somehow loaded in Wayland:

flash-plugin-11.2.202.635-release.x86_64
webkitgtk4-2.14.0-1.fc26.x86_64

#0  0x00007ffff78be9ee in XGetWindowAttributes (dpy=0x555555871100, w=w@entry=0, attr=attr@entry=0x7fffffffd1a0) at GetWAttrs.c:149
#1  0x00007ffff21f76e4 in gtk_socket_realize (widget=0x55555665a170 [GtkSocket]) at gtksocket.c:420
#2  0x00007ffff02c85f4 in _g_closure_invoke_va (closure=closure@entry=0x5555558a6d90, return_value=return_value@entry=0x0, instance=instance@entry=0x55555665a170, args=args@entry=0x7fffffffd480, n_params=<optimized out>, param_types=0x0) at gclosure.c:867
#3  0x00007ffff02e2db9 in g_signal_emit_valist (instance=0x55555665a170, signal_id=<optimized out>, detail=0, var_args=var_args@entry=0x7fffffffd480) at gsignal.c:3300
#4  0x00007ffff02e341f in g_signal_emit (instance=instance@entry=0x55555665a170, signal_id=<optimized out>, detail=detail@entry=0) at gsignal.c:3447
#5  0x00007ffff21bf354 in gtk_widget_realize (widget=widget@entry=0x55555665a170 [GtkSocket]) at gtkwidget.c:5454
#6  0x00007ffff21c2b68 in gtk_widget_set_parent (widget=0x55555665a170 [GtkSocket], parent=0x55555662f990 [EphyWebView]) at gtkwidget.c:9566
#7  0x00007ffff4c64474 in webkitWebViewBaseContainerAdd(GtkContainer*, GtkWidget*) (container=0x55555662f990 [EphyWebView], widget=<optimized out>, widget@entry=0x55555665a170 [GtkSocket])
    at /usr/src/debug/webkitgtk-2.14.0/Source/WebKit2/UIProcess/API/gtk/WebKitWebViewBase.cpp:421
#8  0x00007ffff02cb450 in g_cclosure_marshal_VOID__OBJECTv (closure=0x5555558b24d0, return_value=<optimized out>, instance=0x55555662f990, args=<optimized out>, marshal_data=<optimized out>, n_params=<optimized out>, param_types=0x5555558b25f0) at gmarshal.c:2102
#9  0x00007ffff02c85f4 in _g_closure_invoke_va (closure=closure@entry=0x5555558b24d0, return_value=return_value@entry=0x0, instance=instance@entry=0x55555662f990, args=args@entry=0x7fffffffd8d0, n_params=<optimized out>, param_types=0x5555558b25f0) at gclosure.c:867
#10 0x00007ffff02e2db9 in g_signal_emit_valist (instance=0x55555662f990, signal_id=<optimized out>, detail=0, var_args=var_args@entry=0x7fffffffd8d0) at gsignal.c:3300
#11 0x00007ffff02e341f in g_signal_emit (instance=instance@entry=0x55555662f990, signal_id=<optimized out>, detail=detail@entry=0) at gsignal.c:3447
#12 0x00007ffff1f9cab5 in gtk_container_add (container=0x55555662f990 [EphyWebView], widget=0x55555665a170 [GtkSocket]) at gtkcontainer.c:1875
#13 0x00007ffff4c8639c in WebKit::WebPageProxy::createPluginContainer(unsigned long&) (this=this@entry=0x7fffdf73b000, windowID=windowID@entry=@0x7fffffffda10: 0)
    at /usr/src/debug/webkitgtk-2.14.0/Source/WebKit2/UIProcess/gtk/WebPageProxyGtk.cpp:107
#14 0x00007ffff4cd6fa4 in IPC::callMemberFunctionImpl<WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(unsigned long&), std::tuple<>, , std::tuple<unsigned long>, 0ul>(WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)(unsigned long&), std::tuple<>&&, std::tuple<unsigned long>&, std::integer_sequence<unsigned long>, std::integer_sequence<unsigned long, 0ul>) (args=<optimized out>, replyArgs=std::tuple containing = {...}, function=<optimized out>, object=0x7fffdf73b000) at /usr/src/debug/webkitgtk-2.14.0/Source/WebKit2/Platform/IPC/HandleMessage.h:27
#15 0x00007ffff4cd6fa4 in IPC::callMemberFunction<WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(unsigned long&), std::tuple<>, std::integer_sequence<unsigned long>, std::tuple<unsigned long>, std::integer_sequence<unsigned long, 0ul> >(std::tuple<>&&, std::tuple<unsigned long>&, WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)(unsigned long&)) (args=<optimized out>, function=<optimized out>, object=0x7fffdf73b000, replyArgs=std::tuple containing = {...}) at /usr/src/debug/webkitgtk-2.14.0/Source/WebKit2/Platform/IPC/HandleMessage.h:33
#16 0x00007ffff4cd6fa4 in IPC::handleMessage<Messages::WebPageProxy::CreatePluginContainer, WebKit::WebPageProxy, void (WebKit::WebPageProxy::*)(unsigned long&)>(IPC::Decoder&, IPC::Encoder&, WebKit::WebPageProxy*, void (WebKit::WebPageProxy::*)(unsigned long&)) (decoder=..., function=<optimized out>, object=0x7fffdf73b000, replyEncoder=...)
    at /usr/src/debug/webkitgtk-2.14.0/Source/WebKit2/Platform/IPC/HandleMessage.h:112
#17 0x00007ffff4cd6fa4 in WebKit::WebPageProxy::didReceiveSyncMessage(IPC::Connection&, IPC::Decoder&, std::unique_ptr<IPC::Encoder, std::default_delete<IPC::Encoder> >&) (this=0x7fffdf73b000, connection=..., decoder=..., replyEncoder=std::unique_ptr<IPC::Encoder> containing 0x7fffdf73d000) at /usr/src/debug/webkitgtk-2.14.0/x86_64-redhat-linux-gnu/DerivedSources/WebKit2/WebPageProxyMessageReceiver.cpp:1457
#18 0x00007ffff49e9a31 in IPC::MessageReceiverMap::dispatchSyncMessage(IPC::Connection&, IPC::Decoder&, std::unique_ptr<IPC::Encoder, std::default_delete<IPC::Encoder> >&) (this=this@entry=0x7fffdf7eb638, connection=..., decoder=..., replyEncoder=std::unique_ptr<IPC::Encoder> containing 0x7fffdf73d000) at /usr/src/debug/webkitgtk-2.14.0/Source/WebKit2/Platform/IPC/MessageReceiverMap.cpp:140
#19 0x00007ffff4a495d9 in WebKit::ChildProcessProxy::dispatchSyncMessage(IPC::Connection&, IPC::Decoder&, std::unique_ptr<IPC::Encoder, std::default_delete<IPC::Encoder> >&) (this=this@entry=0x7fffdf7eb600, connection=..., decoder=..., replyEncoder=std::unique_ptr<IPC::Encoder> containing 0x7fffdf73d000) at /usr/src/debug/webkitgtk-2.14.0/Source/WebKit2/UIProcess/ChildProcessProxy.cpp:157
#20 0x00007ffff4a9bf4b in WebKit::WebProcessProxy::didReceiveSyncMessage(IPC::Connection&, IPC::Decoder&, std::unique_ptr<IPC::Encoder, std::default_delete<IPC::Encoder> >&) (this=
    0x7fffdf7eb600, connection=..., decoder=..., replyEncoder=std::unique_ptr<IPC::Encoder> containing 0x7fffdf73d000) at /usr/src/debug/webkitgtk-2.14.0/Source/WebKit2/UIProcess/WebProcessProxy.cpp:514
#21 0x00007ffff49e57db in IPC::Connection::dispatchSyncMessage(IPC::Decoder&) (this=0x7fffdf75e168, decoder=...) at /usr/src/debug/webkitgtk-2.14.0/Source/WebKit2/Platform/IPC/Connection.cpp:789
#22 0x00007ffff49e58cd in IPC::Connection::dispatchMessage(std::unique_ptr<IPC::Decoder, std::default_delete<IPC::Decoder> >) (this=this@entry=0x7fffdf75e168, message=std::unique_ptr<IPC::Decoder> containing 0x7fffdf726478) at /usr/src/debug/webkitgtk-2.14.0/Source/WebKit2/Platform/IPC/Connection.cpp:856
#23 0x00007ffff49e64e8 in IPC::Connection::dispatchOneMessage() (this=0x7fffdf75e168) at /usr/src/debug/webkitgtk-2.14.0/Source/WebKit2/Platform/IPC/Connection.cpp:889
#24 0x00007ffff42da715 in WTF::Function<void ()>::operator()() const (this=<synthetic pointer>) at /usr/src/debug/webkitgtk-2.14.0/Source/WTF/wtf/Function.h:50
#25 0x00007ffff42da715 in WTF::RunLoop::performWork() (this=0x7fffdf7f7000) at /usr/src/debug/webkitgtk-2.14.0/Source/WTF/wtf/RunLoop.cpp:105
#26 0x00007ffff43011d9 in WTF::RunLoop::<lambda(gpointer)>::operator() (__closure=0x0, userData=<optimized out>) at /usr/src/debug/webkitgtk-2.14.0/Source/WTF/wtf/glib/RunLoopGLib.cpp:66
#27 0x00007ffff43011d9 in WTF::RunLoop::<lambda(gpointer)>::_FUN(gpointer) () at /usr/src/debug/webkitgtk-2.14.0/Source/WTF/wtf/glib/RunLoopGLib.cpp:68
#28 0x00007fffefff0e62 in g_main_dispatch (context=0x555555867380) at gmain.c:3201
#29 0x00007fffefff0e62 in g_main_context_dispatch (context=context@entry=0x555555867380) at gmain.c:3854
#30 0x00007fffefff11e0 in g_main_context_iterate (context=context@entry=0x555555867380, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at gmain.c:3927
#31 0x00007fffefff128c in g_main_context_iteration (context=context@entry=0x555555867380, may_block=may_block@entry=1) at gmain.c:3988
#32 0x00007ffff05a8bad in g_application_run (application=0x5555558de140 [EphyShell], argc=1, argv=0x7fffffffdf28) at gapplication.c:2381
#33 0x00005555555867d4 in main ()

Should I ask for a full backtrace?

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

Created attachment 302393 [details]
Patch

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

(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.

[Michael Catanzaro]

(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)

[Carlos Garcia Campos]

Created attachment 302498 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Michael Catanzaro]

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().

[Carlos Garcia Campos]

(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.

[Michael Catanzaro]

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.

[Carlos Garcia Campos]

Committed r212891: <http://trac.webkit.org/changeset/212891>