multiDraw*ANGLE do not validate that the drawcount parameter does not exceed the length of the buffers passed in, where drawcount is later used as the maximum value to iterate through these buffers. While normal entry points to these functions do validate the drawcount, if an attacker gained control of the IPC and was able to send in arbitrary buffers + drawcounts, it could lead to out of bounds access.
<rdar://problem/91645934>
Created attachment 457412 [details] Patch
Test is incoming, however, it's a bit tricky as it requires porting a year old fuzzing sample that uses the ipc test api, and a lot of it has changed. I have verified that if I removed the normal webgl validation, and change one of the existing webgl tests such that it gets to these functions with invalid drawcounts that would have exceeded the buffer sizes that it gets caught and corrected.
Comment on attachment 457412 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457412&action=review So we need to just remove drawcount, it is redundant. Use the size of firsts, counts to deduct the draw count. Move the auto-generated RemoteGraphicsContextGLProxy::multiDrawElementsANGLE to non-autogenerated Implement RemoteGraphicsContextGLProxy::multiDrawElementsANGLE Implement RemoteGraphicsContextGL::multiDrawElementsANGLE * Add the validation, assert and early return in here Alternatively remove the drawcount and implement GCGLMultiSpan<const GCGLsizei, const CGLint> to obtain two arrays of equal length by design. This way we preserve the autogeneration. For the IPC testing, you can see LayoutTests/ipc the general sequence of how to generate calls manually However, it takes some doing to expose the IPC connection of a particular canvas, there's no example how to do it. > Source/WebCore/platform/graphics/angle/GraphicsContextGLANGLE.cpp:2844 > void GraphicsContextGLANGLE::multiDrawArraysANGLE(GCGLenum mode, GCGLSpan<const GCGLint> firsts, GCGLSpan<const GCGLsizei> counts, GCGLsizei drawcount) so here just remove drawcount and have ASSERT(firsts.size == counts.size);
Created attachment 457585 [details] Patch
Created attachment 457589 [details] Patch
Created attachment 457590 [details] Patch
Comment on attachment 457590 [details] Patch need to implement the same thing for non-cocoa
Moving out of security, GPUP variant has not shipped anywhere. This is only a security problem for GPUP in case WP can run arbitrary code to select the arbitrary drawcount. If WP variant has different threat model: if WP can already run arbitrary code to select arbitrary drawcount, it doesn't need an overflow in WP.
Comment on attachment 457590 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457590&action=review > Source/WebCore/platform/graphics/GraphicsTypesGL.h:207 > + ASSERT(static_cast<GCGLsizei>(bufSize_) >= 0); this doesn't look good, e.g. we write generic code with size_t, and then out of the blue comes GCGLsizei cast. If there's a IPC attack consideration, that needs to be handled at the IPC level, e.g. somewhere in the caller. Alternatively if GCGLSpanTuple is something that is used to refer to data that will be sent to GL, and if GL uses GCGLsizei as the count, then the bufSize should be GCGLsizei.. > Source/WebCore/platform/graphics/GraphicsTypesGL.h:211 > + : bufSize(std::min(data0_.size(), data1_.size())) So this line doesn't make sense. The contract is that GCGLSpanTuple is a tuple of arrays of same length. Constructing such a tuple with arrays of not same length makes little sense, unless a programming error. If it's a programming error, we typically catch them with the asserts. If there's a IPC attack consideration, that needs to be handled at the IPC level, e.g. somewhere in the caller. > Source/WebCore/platform/graphics/GraphicsTypesGL.h:215 > + ASSERT(data0_.size() == data1_.size()); This explains the contract to the caller. > Source/WebCore/platform/graphics/GraphicsTypesGL.h:224 > + : bufSize(std::min({ data0_.size(), data1_.size(), minDataNSize })) again, it's clearer to have one mode: either it's correct to call with incompatible vectors or it's incorrect to call with incompatible vectors not that it's incorrect to call with incompatible vectors but it still works and arbitrary things happen. > Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGL.cpp:53 > + Vector<S> result; this function maybe could be written as: return { reinterpret_cast<const S*>(arrayReference.template data<I>()), arrayReference.size()); } So it would get the reserveinitialcapacity call > Source/WebKit/Platform/IPC/ArgumentCoders.h:101 > + auto size = arrayReference.size(); UNLIKELY > Source/WebKit/Platform/IPC/ArgumentCoders.h:110 > + if (!decoder.decode(size)) UNLIKELY > Source/WebKit/Platform/IPC/ArgumentCoders.h:112 > + if (!size) UNLIKELY > Source/WebKit/Platform/IPC/ArgumentCoders.h:138 > + if (!size) UNLIKELY > Source/WebKit/Platform/IPC/ArgumentCoders.h:147 > + if (!decoder.decode(size)) UNLIKELY > Source/WebKit/Platform/IPC/ArgumentCoders.h:149 > + if (!size) UNLIKELY > Source/WebKit/WebProcess/GPU/graphics/RemoteGraphicsContextGLProxy.cpp:56 > + reinterpret_cast<const int32_t*>(spanTuple.data1), needs the T1 cast > Source/WebKit/WebProcess/GPU/graphics/RemoteGraphicsContextGLProxy.cpp:64 > + reinterpret_cast<const int32_t*>(spanTuple.data1), Needs the T1 cast > Source/WebKit/WebProcess/GPU/graphics/RemoteGraphicsContextGLProxy.cpp:65 > + reinterpret_cast<const int32_t*>(spanTuple.data2), needs the T2 cast
Comment on attachment 457590 [details] Patch Would it be more straightforward to send the spans over IPC as specified by the web app, and verify that their size is >= drawcount in GraphicsContextGLANGLE in the GPU process?
Created attachment 457999 [details] Patch
Created attachment 458001 [details] Patch
(In reply to Kenneth Russell from comment #11) > Comment on attachment 457590 [details] > Patch > > Would it be more straightforward to send the spans over IPC as specified by > the web app, and verify that their size is >= drawcount in > GraphicsContextGLANGLE in the GPU process? The web app does not specify spans, it specifies (list, offset), (list, offset), drawcount. We don't want to send the list as-is, naturally due to the case of big list, small drawcount. So we anyway don't send anything as specified by the web app, at the moment. I think it's better to use the type system to enforce that we send size_that_is_equal_to_data0size_and_data1size_and_drawcount, data0, data1 instead of drawcount, data0size, data0, data1size, data1 where we ensure drawcount == data0size == data1size on the receiving side. The API could change if we have the ability to do persistent no-copy client-side buffers in the IPC sometime in the future, though.
Committed r293211 (249882@main): <https://commits.webkit.org/249882@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 458001 [details].