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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=239154

--- Comment #10 from Kimmo Kinnunen <kkinnunen at apple.com> ---
Comment on attachment 457590
  --> https://bugs.webkit.org/attachment.cgi?id=457590
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

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20220414/6fa0ae2b/attachment-0001.htm>