Created attachment 421096 [details] Patch

Comment on attachment 421096 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421096&action=review This does not seem outrageous > Source/WebCore/ChangeLog:12 > + When replaying the "begin" mark, the relpayer will create a mask ImageBuffer sp. `relpayer` > Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:160 > + if (!m_maskImageBuffer) > + return { StopReplayReason::InvalidItem, WTF::nullopt }; Is the item really invalid? Doesn't this probably mean we ran out of memory? I guess it doesn't matter. But, what is the upstream behavior of this? Also, what happens to the rest of the DL when this happens? Will you replay the items that were supposed to go to the mask onto the main context?

Comment on attachment 421096 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421096&action=review > Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:164 > + if (item.is<EndClipToDrawingCommands>()) { What happens if we never see the "end" command? What if begin/end are deeply nested? Can an attacker exploit either of these trigger buffer overflow?

Created attachment 421216 [details] Patch

Created attachment 421218 [details] Patch

Comment on attachment 421096 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421096&action=review >> Source/WebCore/ChangeLog:12 >> + When replaying the "begin" mark, the relpayer will create a mask ImageBuffer > > sp. `relpayer` Fixed. >> Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:160 >> + return { StopReplayReason::InvalidItem, WTF::nullopt }; > > Is the item really invalid? Doesn't this probably mean we ran out of memory? I guess it doesn't matter. But, what is the upstream behavior of this? > > Also, what happens to the rest of the DL when this happens? Will you replay the items that were supposed to go to the mask onto the main context? I added StopReplayReason::OutOfMemory to handle this case. I added also the following check in RemoteRenderingBackend::nextDestinationImageBufferAfterApplyingDisplayLists() MESSAGE_CHECK_WITH_RETURN_VALUE(result.reasonForStopping != DisplayList::StopReplayReason::OutOfMemory, nullptr, "Cound not allocate memory"); This will terminate the WebProcess: #define MESSAGE_CHECK_WITH_RETURN_VALUE(assertion, returnValue, message) do { \ if (UNLIKELY(!(assertion))) { \ TERMINATE_WEB_PROCESS_WITH_MESSAGE(message); \ return (returnValue); \ } \ } while (0) >> Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:164 >> + if (item.is<EndClipToDrawingCommands>()) { > > What happens if we never see the "end" command? What if begin/end are deeply nested? Can an attacker exploit either of these trigger buffer overflow? clipToDrawingCommands() can be called internally. Currently it is only called form one place which is CanvasRenderingContext2DBase::drawTextUnchecked(). It does not make sense to have nested calls from clipToDrawingCommands() because the drawing commands create a mask image. Creating the mask should not involve drawing with gradient. And the drawing happens only in the lambda function which we control.

(In reply to Said Abou-Hallawa from comment #6) > >> Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:164 > >> + if (item.is<EndClipToDrawingCommands>()) { > > > > What happens if we never see the "end" command? What if begin/end are deeply nested? Can an attacker exploit either of these trigger buffer overflow? > > clipToDrawingCommands() can be called internally. Currently it is only > called form one place which is > CanvasRenderingContext2DBase::drawTextUnchecked(). It does not make sense to > have nested calls from clipToDrawingCommands() because the drawing commands > create a mask image. Creating the mask should not involve drawing with > gradient. And the drawing happens only in the lambda function which we > control. Let me ask it this way. What happens when an IPC fuzzer generates display list IPC with lots of "begin clip to drawing command" but no "end"?

(In reply to Simon Fraser (smfr) from comment #7) > (In reply to Said Abou-Hallawa from comment #6) > > > >> Source/WebCore/platform/graphics/displaylists/DisplayListReplayer.cpp:164 > > >> + if (item.is<EndClipToDrawingCommands>()) { > > > > > > What happens if we never see the "end" command? What if begin/end are deeply nested? Can an attacker exploit either of these trigger buffer overflow? > > > > clipToDrawingCommands() can be called internally. Currently it is only > > called form one place which is > > CanvasRenderingContext2DBase::drawTextUnchecked(). It does not make sense to > > have nested calls from clipToDrawingCommands() because the drawing commands > > create a mask image. Creating the mask should not involve drawing with > > gradient. And the drawing happens only in the lambda function which we > > control. > > Let me ask it this way. What happens when an IPC fuzzer generates display > list IPC with lots of "begin clip to drawing command" but no "end"? Replayer::applyItem() will terminate the web process once it detects a "begin clip to drawing command" item while m_maskImageBuffer is not null. if (item.is<BeginClipToDrawingCommands>()) { if (m_maskImageBuffer) return { StopReplayReason::InvalidItem, WTF::nullopt }; ... return { WTF::nullopt, WTF::nullopt }; }

Comment on attachment 421218 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421218&action=review > Source/WebCore/platform/graphics/displaylists/DisplayListItemType.h:55 > + BeginClipToDrawingCommands, > + EndClipToDrawingCommands, At some point we should make this more generic (for filters etc).

Comment on attachment 421218 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=421218&action=review >> Source/WebCore/platform/graphics/displaylists/DisplayListItemType.h:55 >> + EndClipToDrawingCommands, > > At some point we should make this more generic (for filters etc). I wish we could encode/decode the DisplayList instead of these two markers. But the current DisplayList implementation makes it a little bit hard unless we do a significant code re-factoring.

Committed r273291: <https://commits.webkit.org/r273291> All reviewed patches have been landed. Closing bug and clearing flags on attachment 421218 [details].

<rdar://problem/74622997>

*** Bug 220377 has been marked as a duplicate of this bug. ***