IPC::decodeSharedBuffer() should check the return value of SharedMemory::map()

• Component: WebKit2
• Status: RESOLVED FIXED

[David Kilzer (:ddkilzer)]

IPC::decodeSharedBuffer() should check the return value of SharedMemory::map().

<rdar://problem/60773120>

[David Kilzer (:ddkilzer)]

Created attachment 397167 [details]
Patch v1

[Darin Adler]

Comment on attachment 397167 [details]
Patch v1

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

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:179
> +    if (!sharedMemoryBuffer)
> +        return false;

When does this situation arise? Is "return false" sufficient here? Should we be crashing instead? Or making the process that passed us this handle crash?

[David Kilzer (:ddkilzer)]

(In reply to Darin Adler from comment #2)
> Comment on attachment 397167 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=397167&action=review
> 
> > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:179
> > +    if (!sharedMemoryBuffer)
> > +        return false;
> 
> When does this situation arise? Is "return false" sufficient here? Should we
> be crashing instead? Or making the process that passed us this handle crash?

There are numerous reasons that SharedMemory::map() can fail:

<https://trac.webkit.org/browser/trunk/Source/WebKit/Platform/cocoa/SharedMemoryCocoa.cpp#L212>

I think `return false` is sufficient here because callers of IPC::decodeSharedBuffer() always check their return values via WARN_UNUSED_RETURN:

static WARN_UNUSED_RETURN bool decodeSharedBuffer(Decoder& decoder, RefPtr<SharedBuffer>& buffer)

In general, decoding is a task where failures are (must be) handled since we're dealing with untrusted input.

[Geoffrey Garen]

Comment on attachment 397167 [details]
Patch v1

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

r=me

>>> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:179
>>> +        return false;
>> 
>> When does this situation arise? Is "return false" sufficient here? Should we be crashing instead? Or making the process that passed us this handle crash?
> 
> There are numerous reasons that SharedMemory::map() can fail:
> 
> <https://trac.webkit.org/browser/trunk/Source/WebKit/Platform/cocoa/SharedMemoryCocoa.cpp#L212>
> 
> I think `return false` is sufficient here because callers of IPC::decodeSharedBuffer() always check their return values via WARN_UNUSED_RETURN:
> 
> static WARN_UNUSED_RETURN bool decodeSharedBuffer(Decoder& decoder, RefPtr<SharedBuffer>& buffer)
> 
> In general, decoding is a task where failures are (must be) handled since we're dealing with untrusted input.

I think Darin is agreeing that decoding should handle failure, and then asking whether our failure handling should include killing the sender (to avoid giving a malicious sender multiple attempts at the same exploit).

I guess my take is that decodeSharedBuffer is used by lots of different clients, so it may not be appropriate for decodeSharedBuffer to kill the sender in all cases. But maybe we should audit callers of decodeSharedBuffer to consider whether they should kill the sender on failure. Here's an example I found that might want to MESSAGE_CHECK a PasteboardWebContent, which contains a SharedBuffer:

    void WebPasteboardProxy::writeWebContentToPasteboard(IPC::Connection& connection, const WebCore::PasteboardWebContent& content, const String& pasteboardName)

[EWS]

Committed r260527: <https://trac.webkit.org/changeset/260527>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397167 [details].

[Darin Adler]

(In reply to Geoffrey Garen from comment #4)
> I think Darin is agreeing that decoding should handle failure, and then
> asking whether our failure handling should include killing the sender (to
> avoid giving a malicious sender multiple attempts at the same exploit).

Yes, that’s right.

> But maybe we should audit callers of decodeSharedBuffer
> to consider whether they should kill the sender on failure. Here's an
> example I found that might want to MESSAGE_CHECK a PasteboardWebContent,
> which contains a SharedBuffer:
> 
>     void WebPasteboardProxy::writeWebContentToPasteboard(IPC::Connection&
> connection, const WebCore::PasteboardWebContent& content, const String&
> pasteboardName)

Makes sense to me.

The main thing I wanted to check/question is whether the memory mapping failure should have identical handling to other types of decoding failures, since the single boolean tells about both. And whether we are taking advantage of it sufficiently to check the integrity of messages coming from the web process.