Bug 199621 – [WPE][GTK] UI process crash due to NULL dereference in webkitWebViewResourceLoadStarted()

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Milan Crha]

Moving this from a downstream bug report:
https://bugzilla.redhat.com/show_bug.cgi?id=1728026

Evolution is crashing regularly in Rawhide. The crash is in webkit - WebKit::WebFrameProxy::isMainFrame() , and started happening after the webkit2gtk3-2.25.2.

Notes from Michael in the downstream bug:

 * The main change in 2.25.2 is PSON (process swap on navigation), which is surely related
 * We don't know from the backtrace why the WebFrameProxy is invalid, because the invalid message is coming from the web process (from PageResourceLoadClient::didInitiateLoadForResource)
 * There are at least two bugs here: first that the web process is sending an invalid frame, second that this crashes the UI process. The UI process must be robust to a malicious web process sending invalid messages.

---------------------------------------------------------------

The backtrace:

Thread 1 (Thread 0x7f162ace4c80 (LWP 12812)):
#0  0x00007f16322c0ad4 in WTF::WeakPtr<WebKit::WebPageProxy>::operator bool() const (this=0x10) at DerivedSources/ForwardingHeaders/wtf/WeakPtr.h:90
#1  0x00007f16322c0ad4 in WebKit::WebFrameProxy::isMainFrame() const (this=this@entry=0x0) at ../Source/WebKit/UIProcess/WebFrameProxy.cpp:75
#2  0x00007f16323c8eaa in webkitWebViewResourceLoadStarted(_WebKitWebView*, WebKit::WebFrameProxy*, unsigned long, _WebKitURIRequest*) (webView=webView@entry=0x561624a015a0 [EMailDisplay], frame=frame@entry=0x0, resourceIdentifier=<optimized out>, request=request@entry=0x7f155000b830 [WebKitURIRequest]) at ../Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:2380
        priv = 0x561624a01180
        isMainResource = <optimized out>
        resource = <optimized out>
#3  0x00007f163239fddc in WebKitInjectedBundleClient::didReceiveWebViewMessageFromInjectedBundle(_WebKitWebView*, char const*, API::Dictionary&) (webView=0x561624a015a0 [EMailDisplay], messageName=<optimized out>, message=...) at ../Source/WebKit/UIProcess/API/glib/WebKitInjectedBundleClient.cpp:52
        frame = 0x0
        resourceIdentifier = 0x7f161424cee8
        webRequest = <optimized out>
        request = {m_ptr = 0x7f155000b830 [WebKitURIRequest]}
#4  0x00007f16323a1618 in WebKitInjectedBundleClient::didReceiveMessageFromInjectedBundle(WebKit::WebProcessPool&, WTF::String const&, API::Object*) (this=0x5616255c1560, messageName=..., messageBody=0x7f16142ce550) at DerivedSources/ForwardingHeaders/wtf/text/CString.h:66
        page = <optimized out>
        webView = <optimized out>
        message = @0x7f16142ce550: {<API::ObjectImpl<(API::Object::Type)9>> = {<API::Object> = {<WTF::ThreadSafeRefCounted<API::Object, (WTF::DestructionThread)0>> = {<WTF::ThreadSafeRefCountedBase> = {m_refCount = {<std::__atomic_base<unsigned int>> = {static _S_alignment = 4, _M_i = 1}, static is_always_lock_free = true}}, <No data fields>}, _vptr.Object = 0x7f1634874d30 <vtable for API::Dictionary+16>}, static APIType = API::Object::Type::Dictionary}, m_map = {m_impl = {static m_maxLoad = 2, static m_minLoad = 6, m_table = 0x7f16142b7e00, m_tableSize = 16, m_tableSizeMask = 15, m_keyCount = 4, m_deletedCount = 0}}}
        messageNameUTF8 = {m_buffer = {static isRefPtr = <error reading variable: Missing ELF symbol "WTF::RefPtr<WTF::CStringBuffer, WTF::DumbPtrTraits<WTF::CStringBuffer> >::isRefPtr".>, m_ptr = 0x7f16142f38f8}}
#5  0x00007f163231e3de in WebKit::WebProcessPool::handleMessage(IPC::Connection&, WTF::String const&, WebKit::UserData const&) (this=0x7f16140fec00, connection=..., messageName=..., messageBody=...) at /usr/include/c++/9/bits/atomic_base.h:326
        webProcessProxy = <optimized out>
#6  0x00007f16320dcb85 in IPC::callMemberFunctionImpl<WebKit::WebProcessPool, void (WebKit::WebProcessPool::*)(IPC::Connection&, WTF::String const&, WebKit::UserData const&), std::tuple<WTF::String, WebKit::UserData>, 0ul, 1ul>(WebKit::WebProcessPool*, void (WebKit::WebProcessPool::*)(IPC::Connection&, WTF::String const&, WebKit::UserData const&), IPC::Connection&, std::tuple<WTF::String, WebKit::UserData>&&, std::integer_sequence<unsigned long, 0ul, 1ul>) (args=..., connection=..., function=(void (WebKit::WebProcessPool::*)(WebKit::WebProcessPool * const, IPC::Connection &, const WTF::String &, const WebKit::UserData &)) 0x7f163231e320 <WebKit::WebProcessPool::handleMessage(IPC::Connection&, WTF::String const&, WebKit::UserData const&)>, object=0x7f16140fec00) at /usr/include/c++/9/tuple:1332
        arguments = {<WTF::Optional_base<std::tuple<WTF::String, WebKit::UserData> >> = {init_ = true, storage_ = {dummy_ = 40 '(', value_ = std::tuple containing = {[1] = {static MaxLength = 2147483647, m_impl = {static isRefPtr = <error reading variable: Missing ELF symbol "WTF::RefPtr<WTF::StringImpl, WTF::DumbPtrTraits<WTF::StringImpl> >::isRefPtr".>, m_ptr = 0x7f16142f38c0}}, [2] = {m_object = {static isRefPtr = <error reading variable: Missing ELF symbol "WTF::RefPtr<API::Object, WTF::DumbPtrTraits<API::Object> >::isRefPtr".>, m_ptr = 0x7f16142ce528}}}}}, <No data fields>}
#7  0x00007f16320dcb85 in IPC::callMemberFunction<WebKit::WebProcessPool, void (WebKit::WebProcessPool::*)(IPC::Connection&, WTF::String const&, WebKit::UserData const&), std::tuple<WTF::String, WebKit::UserData>, std::integer_sequence<unsigned long, 0ul, 1ul> >(IPC::Connection&, std::tuple<WTF::String, WebKit::UserData>&&, WebKit::WebProcessPool*, void (WebKit::WebProcessPool::*)(IPC::Connection&, WTF::String const&, WebKit::UserData const&)) (function=(void (WebKit::WebProcessPool::*)(WebKit::WebProcessPool * const, IPC::Connection &, const WTF::String &, const WebKit::UserData &)) 0x7f163231e320 <WebKit::WebProcessPool::handleMessage(IPC::Connection&, WTF::String const&, WebKit::UserData const&)>, object=0x7f16140fec00, args=..., connection=...) at ../Source/WebKit/Platform/IPC/HandleMessage.h:89
        arguments = {<WTF::Optional_base<std::tuple<WTF::String, WebKit::UserData> >> = {init_ = true, storage_ = {dummy_ = 40 '(', value_ = std::tuple containing = {[1] = {static MaxLength = 2147483647, m_impl = {static isRefPtr = <error reading variable: Missing ELF symbol "WTF::RefPtr<WTF::StringImpl, WTF::DumbPtrTraits<WTF::StringImpl> >::isRefPtr".>, m_ptr = 0x7f16142f38c0}}, [2] = {m_object = {static isRefPtr = <error reading variable: Missing ELF symbol "WTF::RefPtr<API::Object, WTF::DumbPtrTraits<API::Object> >::isRefPtr".>, m_ptr = 0x7f16142ce528}}}}}, <No data fields>}
#8  0x00007f16320dcb85 in IPC::handleMessage<Messages::WebProcessPool::HandleMessage, WebKit::WebProcessPool, void (WebKit::WebProcessPool::*)(IPC::Connection&, WTF::String const&, WebKit::UserData const&)>(IPC::Connection&, IPC::Decoder&, WebKit::WebProcessPool*, void (WebKit::WebProcessPool::*)(IPC::Connection&, WTF::String const&, WebKit::UserData const&)) (connection=..., decoder=..., object=object@entry=0x7f16140fec00, function=(void (WebKit::WebProcessPool::*)(WebKit::WebProcessPool * const, IPC::Connection &, const WTF::String &, const WebKit::UserData &)) 0x7f163231e320 <WebKit::WebProcessPool::handleMessage(IPC::Connection&, WTF::String const&, WebKit::UserData const&)>) at ../Source/WebKit/Platform/IPC/HandleMessage.h:132
        arguments = {<WTF::Optional_base<std::tuple<WTF::String, WebKit::UserData> >> = {init_ = true, storage_ = {dummy_ = 40 '(', value_ = std::tuple containing = {[1] = {static MaxLength = 2147483647, m_impl = {static isRefPtr = <error reading variable: Missing ELF symbol "WTF::RefPtr<WTF::StringImpl, WTF::DumbPtrTraits<WTF::StringImpl> >::isRefPtr".>, m_ptr = 0x7f16142f38c0}}, [2] = {m_object = {static isRefPtr = <error reading variable: Missing ELF symbol "WTF::RefPtr<API::Object, WTF::DumbPtrTraits<API::Object> >::isRefPtr".>, m_ptr = 0x7f16142ce528}}}}}, <No data fields>}
#9  0x00007f16320db0e5 in WebKit::WebProcessPool::didReceiveMessage(IPC::Connection&, IPC::Decoder&) (this=0x7f16140fec00, connection=..., decoder=...) at DerivedSources/WebKit/WebProcessPoolMessageReceiver.cpp:72
#10 0x00007f1632224183 in IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&, IPC::Decoder&) (this=<optimized out>, connection=..., decoder=...) at ../Source/WebKit/Platform/IPC/MessageReceiverMap.cpp:123
        messageReceiver = <optimized out>
#11 0x00007f163231bc0d in WebKit::WebProcessPool::dispatchMessage(IPC::Connection&, IPC::Decoder&) (this=<optimized out>, connection=..., decoder=...) at ../Source/WebKit/UIProcess/WebProcessPool.cpp:1676
#12 0x00007f163231bc46 in WebKit::WebProcessProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder&) (this=0x7f16142a3b40, connection=..., decoder=...) at DerivedSources/ForwardingHeaders/wtf/WeakPtr.h:89
#13 0x00007f163221d154 in IPC::Connection::dispatchMessage(IPC::Decoder&) (this=0x7f16142ca680, decoder=...) at ../Source/WebKit/Platform/IPC/Connection.cpp:983
#14 0x00007f163221e751 in IPC::Connection::dispatchMessage(std::unique_ptr<IPC::Decoder, std::default_delete<IPC::Decoder> >) (this=0x7f16142ca680, message=std::unique_ptr<IPC::Decoder> = {...}) at /usr/include/c++/9/bits/unique_ptr.h:357
        oldDidReceiveInvalidMessage = false
#15 0x00007f163221fa23 in IPC::Connection::dispatchIncomingMessages() (this=0x7f16142ca680) at /usr/include/c++/9/bits/move.h:74
        i = 9
        message = std::unique_ptr<IPC::Decoder> = {get() = 0x0}
        messagesToProcess = 18
#16 0x00007f162f86c72c in WTF::Function<void ()>::operator()() const (this=<synthetic pointer>) at ../Source/WTF/wtf/Function.h:76
        function = {m_callableWrapper = std::unique_ptr<WTF::Detail::CallableWrapperBase<void>> = {get() = 0x7f1614243020}}
        functionsHandled = 1
        functionsToHandle = 7
#17 0x00007f162f86c72c in WTF::RunLoop::performWork() (this=0x7f16142fa000) at ../Source/WTF/wtf/RunLoop.cpp:123
        function = {m_callableWrapper = std::unique_ptr<WTF::Detail::CallableWrapperBase<void>> = {get() = 0x7f1614243020}}
        functionsHandled = 1
        functionsToHandle = 7
#18 0x00007f162f8b6d5d in WTF::RunLoop::<lambda(gpointer)>::operator() (__closure=0x0, userData=<optimized out>) at ../Source/WTF/wtf/glib/RunLoopGLib.cpp:68
#19 0x00007f162f8b6d5d in WTF::RunLoop::<lambda(gpointer)>::_FUN(gpointer) () at ../Source/WTF/wtf/glib/RunLoopGLib.cpp:70
#20 0x00007f1634ac8fed in g_main_dispatch (context=0x5616239b7000) at ../glib/gmain.c:3193
        dispatch = <optimized out>
        prev_source = 0x0
        was_in_call = <optimized out>
        user_data = 0x7f16142fa000
        callback = 0x7f162f8b6d50 <WTF::RunLoop::<lambda(gpointer)>::_FUN(gpointer)>
        cb_funcs = 0x7f1634b9c280 <g_source_callback_funcs>
        cb_data = 0x561624a52630
        need_destroy = <optimized out>
        source = 0x561624a542f0
        current = 0x5616239b70c0
        i = 0
        __FUNCTION__ = "g_main_dispatch"
#21 0x00007f1634ac8fed in g_main_context_dispatch (context=context@entry=0x5616239b7000) at ../glib/gmain.c:3858
#22 0x00007f1634ac9380 in g_main_context_iterate (context=0x5616239b7000, block=block@entry=1, dispatch=dispatch@entry=1, self=<optimized out>) at ../glib/gmain.c:3931
        max_priority = 100
        timeout = 0
        some_ready = 1
        nfds = <optimized out>
        allocated_nfds = <optimized out>
        fds = 0x7f15f00013d0
#23 0x00007f1634ac9673 in g_main_loop_run (loop=0x561623de5da0) at ../glib/gmain.c:4125
        __FUNCTION__ = "g_main_loop_run"
#24 0x00007f163514ca6d in gtk_main () at gtkmain.c:1323
        loop = 0x561623de5da0
#25 0x0000561623293b0a in main (argc=<optimized out>, argv=<optimized out>) at /usr/src/debug/evolution-3.33.3-1.fc31.x86_64/src/shell/main.c:691
        shell = 0x561623abf1f0 [EShell]
        settings = <optimized out>
        skip_warning_dialog = <optimized out>
        success = <optimized out>
        error = 0x0

[Adam Williamson]

I'm the original reporter. Ask me anything. =)

[Michael Catanzaro]

(In reply to Milan Crha from comment #0)
>  * There are at least two bugs here: first that the web process is sending
> an invalid frame, second that this crashes the UI process. The UI process
> must be robust to a malicious web process sending invalid messages.

The first bug is going to require some investigation.

For the second bug, basically all of WebKitInjectedBundleClient in WebKitInjectedBundleClient.cpp is missing validation to ensure API::Dictionary::get is returning valid pointers. E.g.:

        if (g_str_equal(messageName, "DidInitiateLoadForResource")) {
            WebFrameProxy* frame = static_cast<WebFrameProxy*>(message.get(String::fromUTF8("Frame")));
            API::UInt64* resourceIdentifier = static_cast<API::UInt64*>(message.get(String::fromUTF8("Identifier")));
            API::URLRequest* webRequest = static_cast<API::URLRequest*>(message.get(String::fromUTF8("Request")));
            GRefPtr<WebKitURIRequest> request = adoptGRef(webkitURIRequestCreateForResourceRequest(webRequest->resourceRequest()));

            webkitWebViewResourceLoadStarted(webView, frame, resourceIdentifier->value(), request.get());

Here we crash because we fail to check that frame is not null, but it could just as easily have been webRequest or request. Point is to make sure the UI process survives even if the web process is evil and intentionally sends bad messages.

[Carlos Garcia Campos]

I can't reproduce it, but I think this can only happen if DidDestroyFrame message is received after injected bundle message is sent, but before it's processed. I'm not sure that's actually possible, because IPC messages should be processed in the order they were sent, though. But if that can happen somehow, then the null check in the UI process would be the right fix.

[Carlos Garcia Campos]

Created attachment 374202 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Michael Catanzaro]

Comment on attachment 374202 [details]
Patch

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

> Source/WebKit/UIProcess/API/glib/WebKitInjectedBundleClient.cpp:54
>              API::UInt64* resourceIdentifier = static_cast<API::UInt64*>(message.get(String::fromUTF8("Identifier")));
> +            ASSERT(resourceIdentifier);
>              API::URLRequest* webRequest = static_cast<API::URLRequest*>(message.get(String::fromUTF8("Request")));
> +            ASSERT(webRequest);

But this is exactly what I said we must not do. We can't trust web process messages: we have to validate them. A malicious web process shouldn't be able to crash the UI process, and a crash is what we're going to get if either of these are null.

I assume we can trust message.get to return either valid data or null.

[Carlos Garcia Campos]

You are assuming the web process is sending a wrong frame, but that's not the case.

[Michael Catanzaro]

I'm trying to remind you that our message processing code must not ASSERT when it receives unexpected input, because that would introduce a security vulnerability (denial of service). The code should drop the message and move on.

[Carlos Garcia Campos]

Ok, we are already asserting when receiving an invalid injected bundle message, so I'm going to fix the existing code in a new bug that I can merge in stable, and then update this patch on top.

[Michael Catanzaro]

At the risk of complicating this further: sometimes Apple uses ASSERT() to catch the problem in debug builds, but still handles the problem in release builds anyway. E.g.

auto* something = message.get();
ASSERT(something);
if (!something)
    return;

I think that's extremely confusing because it creates the incorrect impression that the condition below is redundant and can be removed, but it's a pattern that is occasionally used in WebKit. The advantage of that is that you still get the ASSERT in debug mode for developer convenience, but not in release builds where it would be a vulnerability.

I don't suggest using this pattern, but it's not necessarily a bug if you find it elsewhere.

[Carlos Garcia Campos]

Created attachment 374210 [details]
Patch

[Michael Catanzaro]

Comment on attachment 374210 [details]
Patch

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

> Source/WebKit/ChangeLog:10
> +        * UIProcess/API/glib/WebKitInjectedBundleClient.cpp: Add asserts for all message parameters that should never be nullptr.

Update the changelog.

[Carlos Garcia Campos]

Committed r247508: <https://trac.webkit.org/changeset/247508>