Bug 239154

Summary: [CoreIPC][WebGL] Heap Buffer Overflow from CoreIPC WebGL MultiDraw* due to discarded firsts/counts length in favour of attacker controlled drawcount
Product: WebKit Reporter: John Cunningham <johncunningham>
Component: WebGLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cdumez, changseok, dino, esprehn+autocc, ews-feeder, ews-watchlist, gavin.p, gyuyoung.kim, kbr, kkinnunen, kondapallykalyan, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: iPhone / iPad   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
ews-feeder: commit-queue-
Patch
none
Patch
ews-feeder: commit-queue-
Patch none

John Cunningham
Reported 2022-04-12 13:31:13 PDT
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.
Attachments
Patch (5.08 KB, patch)
2022-04-12 13:47 PDT, John Cunningham
no flags
Patch (52.49 KB, patch)
2022-04-13 18:55 PDT, John Cunningham
no flags
Patch (55.86 KB, patch)
2022-04-13 20:19 PDT, John Cunningham
ews-feeder: commit-queue-
Patch (55.86 KB, patch)
2022-04-13 20:22 PDT, John Cunningham
no flags
Patch (54.75 KB, patch)
2022-04-20 11:33 PDT, John Cunningham
ews-feeder: commit-queue-
Patch (56.60 KB, patch)
2022-04-20 11:50 PDT, John Cunningham
no flags
Radar WebKit Bug Importer
Comment 1 2022-04-12 13:31:24 PDT
John Cunningham
Comment 2 2022-04-12 13:47:21 PDT
John Cunningham
Comment 3 2022-04-12 13:59:27 PDT
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.
Kimmo Kinnunen
Comment 4 2022-04-13 01:08:52 PDT
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);
John Cunningham
Comment 5 2022-04-13 18:55:53 PDT
John Cunningham
Comment 6 2022-04-13 20:19:52 PDT
John Cunningham
Comment 7 2022-04-13 20:22:44 PDT
Kimmo Kinnunen
Comment 8 2022-04-14 03:22:06 PDT
Comment on attachment 457590 [details] Patch need to implement the same thing for non-cocoa
Kimmo Kinnunen
Comment 9 2022-04-14 03:24:46 PDT
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.
Kimmo Kinnunen
Comment 10 2022-04-14 04:00:04 PDT
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
Kenneth Russell
Comment 11 2022-04-18 16:28:43 PDT
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?
John Cunningham
Comment 12 2022-04-20 11:33:31 PDT
John Cunningham
Comment 13 2022-04-20 11:50:35 PDT
Kimmo Kinnunen
Comment 14 2022-04-21 22:49:29 PDT
(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.
EWS
Comment 15 2022-04-21 23:09:49 PDT
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].
Note You need to log in before you can comment on or make changes to this bug.