Bug 239154 – [CoreIPC][WebGL] Heap Buffer Overflow from CoreIPC WebGL MultiDraw* due to discarded firsts/counts length in favour of attacker controlled drawcount

• Component: WebGL
• Status: RESOLVED FIXED

[John Cunningham]

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.

[Radar WebKit Bug Importer]

<rdar://problem/91645934>

[John Cunningham]

Created attachment 457412 [details]
Patch

[John Cunningham]

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 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]

Created attachment 457585 [details]
Patch

[John Cunningham]

Created attachment 457589 [details]
Patch

[John Cunningham]

Created attachment 457590 [details]
Patch

[Kimmo Kinnunen]

Comment on attachment 457590 [details]
Patch

need to implement the same thing for non-cocoa

[Kimmo Kinnunen]

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 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 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]

Created attachment 457999 [details]
Patch

[John Cunningham]

Created attachment 458001 [details]
Patch

[Kimmo Kinnunen]

(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]

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].