Bug 221441 – [CoreIPC] Nullptr crash in IPC::Decoder::decode<WebCore::ResourceError>

• Component: WebKit2
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Created attachment 419350 [details]
Test

Using the new IPC testing code I added in https://trac.webkit.org/r268239,
we can reproduce the following crash in macOS ASAN builds (tested in r272114):

=================================================================
==53800==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fff2040301d bp 0x7ffeed1a8070 sp 0x7ffeed1a7e98 T0)

    #0 0x7fff2040301d in CFDictionaryGetCount+0x6 (CoreFoundation:x86_64h+0x2201d)
    #1 0x114ea3fcc in IPC::ArgumentCoder<WebCore::ResourceError, void>::decodePlatformData(IPC::Decoder&, WebCore::ResourceError&) WebCoreArgumentCodersMac.mm:435
    #2 0x1161ecbb7 in IPC::ArgumentCoder<WebCore::ResourceError, void>::decode(IPC::Decoder&, WebCore::ResourceError&) WebCoreArgumentCoders.cpp:1297
    #3 0x1140151a8 in bool IPC::Decoder::decode<WebCore::ResourceError>(WebCore::ResourceError&) Decoder.h:91
    #4 0x1151050c5 in API::Error::decode(IPC::Decoder&, WTF::RefPtr<API::Object, WTF::RawPtrTraits<API::Object>, WTF::DefaultRefDerefTraits<API::Object> >&) APIError.cpp:95
    #5 0x114ffd6dc in WebKit::UserData::decode(IPC::Decoder&, WTF::RefPtr<API::Object, WTF::RawPtrTraits<API::Object>, WTF::DefaultRefDerefTraits<API::Object> >&) UserData.cpp:385
    #6 0x114ffde1a in WebKit::UserData::decode(IPC::Decoder&, WTF::RefPtr<API::Object, WTF::RawPtrTraits<API::Object>, WTF::DefaultRefDerefTraits<API::Object> >&) UserData.cpp:368
    #7 0x114ffd348 in WebKit::UserData::decode(IPC::Decoder&, WebKit::UserData&) UserData.cpp:151
    #8 0x1140621fc in IPC::ArgumentCoder<WebKit::UserData, void>::decode(IPC::Decoder&) ArgumentCoder.h:56
    #9 0x114061fc3 in IPC::Decoder& IPC::Decoder::operator>><WebKit::UserData>(WTF::Optional<WebKit::UserData>&) Decoder.h:107
    #10 0x114680201 in IPC::TupleDecoderImpl<WebKit::UserData>::decode(IPC::Decoder&) ArgumentCoders.h:317
    #11 0x1161d042e in IPC::TupleDecoderImpl<WTF::String, WebKit::UserData>::decode(IPC::Decoder&) ArgumentCoders.h:304
    #12 0x1161d032d in IPC::TupleDecoder<2ul, WTF::String, WebKit::UserData>::decode(IPC::Decoder&) ArgumentCoders.h:328
    #13 0x1161d024d in IPC::ArgumentCoder<std::__1::tuple<WTF::String, WebKit::UserData>, void>::decode(IPC::Decoder&) ArgumentCoders.h:348
    #14 0x1161d0087 in IPC::Decoder& IPC::Decoder::operator>><std::__1::tuple<WTF::String, WebKit::UserData> >(WTF::Optional<std::__1::tuple<WTF::String, WebKit::UserData> >&) Decoder.h:107
    #15 0x1161cfe25 in void IPC::handleMessage<Messages::WebConnection::HandleMessage, WebKit::WebConnection, void (WebKit::WebConnection::*)(WTF::String const&, WebKit::UserData const&)>(IPC::Decoder&, WebKit::WebConnection*, void (WebKit::WebConnection::*)(WTF::String const&, WebKit::UserData const&)) HandleMessage.h:114
    #16 0x1161cfcbf in WebKit::WebConnection::didReceiveMessage(IPC::Connection&, IPC::Decoder&) WebConnectionMessageReceiver.cpp:41
    #17 0x114311bbf in IPC::MessageReceiverMap::dispatchMessage(IPC::Connection&, IPC::Decoder&) MessageReceiverMap.cpp:118
    #18 0x1152518fc in WebKit::AuxiliaryProcessProxy::dispatchMessage(IPC::Connection&, IPC::Decoder&) AuxiliaryProcessProxy.cpp:216
    #19 0x11563ee47 in WebKit::WebProcessProxy::didReceiveMessage(IPC::Connection&, IPC::Decoder&) WebProcessProxy.cpp:808
    #20 0x113dfe8a3 in IPC::Connection::dispatchMessage(IPC::Decoder&) Connection.cpp:1038
    #21 0x113e00217 in IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >) Connection.cpp:1138
    #22 0x113dfd029 in IPC::Connection::dispatchIncomingMessages() Connection.cpp:1242
    #23 0x113e1f32e in IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_8::operator()() Connection.cpp:999
    #24 0x113e1f29c in WTF::Detail::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_8, void>::call() Function.h:52
    #25 0x10b67674e in WTF::Function<void ()>::operator()() const Function.h:83
    #26 0x10b711818 in WTF::RunLoop::performWork() RunLoop.cpp:128
    #27 0x10b714b35 in WTF::RunLoop::performWork(void*) RunLoopCF.cpp:46

<rdar://problem/73568927>

[Darin Adler]

The bug here is in decodeNSError. The problem here is that the hasUnderlyingError support does not work properly when the userInfo dictionary is nullptr. Trivial to fix. I’d be happy to fix this but I don’t know how to make regression tests for an issue like this.

[Darin Adler]

I’m going to grab this and fix it, but I don’t plan to add a regression test.

Ryosuke, let me know if that’s not right.

[Darin Adler]

I read the encode function more carefully and it’s clear it won’t ever encode a null, so the fix here is just to add a null check. But I also did a tiny refactor to get rid of the need to use WARN_UNUSED_RETURN.

[Darin Adler]

Since this is just a null pointer dereference, I am thinking that maybe it’s not security sensitive?

[Darin Adler]

Created attachment 419459 [details]
Patch

[Darin Adler]

Committed r272450: <https://trac.webkit.org/changeset/272450>

[Ryosuke Niwa]

(In reply to Darin Adler from comment #4)
> Since this is just a null pointer dereference, I am thinking that maybe it’s
> not security sensitive?

Doesn't seem like it. Maybe we can add a test then.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #2)
> I’m going to grab this and fix it, but I don’t plan to add a regression test.
> 
> Ryosuke, let me know if that’s not right.

So if it's just a nullptr crash without any security implications, we want to add a test like any other bug.

[Darin Adler]

The only way to test it is the IPC fuzzing; not sure.

[Ryosuke Niwa]

Hm... this IPC test might be a bit problematic in that it may break right away when the format of WebConnection_HandleMessage changes so maybe it's okay not to add it.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #9)
> The only way to test it is the IPC fuzzing; not sure.

FWIW, IPC fuzzing API is available in debug & ASAN release builds so if we added API, we can test it but it's true that this input is probably not going to be valid in a long term.