Edit report at https://bugs.php.net/bug.php?id=79385&edit=1
ID: 79385
Updated by: cmb@php.net
Reported by: dev dot davidmatosse at outlook dot com
Summary: Global Out-of-Bounds Read
Status: Not a bug
Type: Bug
Package: GD related
Operating System: Linux
PHP Version: Irrelevant
Assigned To: cmb
Block user comment: N
Private report: N
New Comment:
> <let's compile and debug this test code>
Why? That code is broken in the first place, since
gdImagePtr gdImageCreateFromWebpPtr (int size, void *data);
data is the start address of the buffer, size is the size of the
buffer[1]. So a correct test program would be like
int main(){
void *data = "AAAAAAAAAAAAAAAAAAAAAAAAAAA";
int size = strlen(data) + 1;
gdImageCreateFromWebpPtr (size, data);
return 0;
}
And even if that fails with PHP's bundled libgd, that wouldn't
matter, because PHP does never call gdImageCreateFromWebpPtr().
It might still be an upstream issue, but to verify this, you
would have to test against libgd master branch.
[1] <http://libgd.github.io/manuals/2.2.5/files/gd_webp-c.html#gdImageCreateFromWebpPtr>
Previous Comments:
------------------------------------------------------------------------
[2020-03-17 00:11:46] dev dot davidmatosse at outlook dot com
> Harald, this is how C works; when passing (the address of) a
buffer to a function, the buffer's size has to be known by the
callee,
This is indeed truth, let's look closer [1]
```
#define GD_WEBP_ALLOC_STEP (4*1024) // GD_WEBP_ALLOC_STEP = 4096
...
BGD_DECLARE(gdImagePtr) gdImageCreateFromWebpCtx (gdIOCtx * infile)
{
...
uint8_t *filedata = NULL;
...
unsigned char *read, *temp;
size_t size = 0, n;
...
do {
temp = gdRealloc(filedata, size+GD_WEBP_ALLOC_STEP); // -> realloc(filedata, 4096)
if (temp) { // true
filedata = temp;
read = temp + size;
} else {
...
}
n = gdGetBuf(read, GD_WEBP_ALLOC_STEP, infile);
/*
int gdGetBuf(void *buf, int size, gdIOCtx *ctx)
{return (ctx->getBuf)(ctx, buf, size);}
*/
...
```
As we can see, on the first interaction gdGetBuf is called with size GD_WEBP_ALLOC_STEP
> and this is typically accomplished by passing the size of
the buffer as well.
but is not this size that is considered in memcpy call at dynamicGetbuf
<let's compile and debug this test code>
```
#include <gd.h>
/* clang-9 poc.c -o poc -lgd -fsanitize=address */
int main(){
void *data = "AAAAAAAAAAAAAAAAAAAAAAAAAAA";
int size = 48;
gdImageCreateFromWebpPtr (size, data);
return 0;
}
```
debugging
```
gdb ./poc
$ b gdImageCreateFromWebpPtr
$ r
$ b dynamicGetbuf
$ c
Breakpoint 2, dynamicGetbuf (ctx=0x607000000090, buf=0x621000000100, len=4096) at gd_io_dp.c:276
# as we can see the size is 4096 (GD_WEBP_ALLOC_STEP)
$ disassemble dynamicGetbuf # here we break before the memcpy call
$ b *0x00007ffff7b8c361
$ p rlen
$1 = 48 # user defined size
$ x/4gx dp->data
0x4d4c00 <.str>: 0x4141414141414141 0x4141414141414141
0x4d4c10 <.str+16>: 0x4141414141414141 0x0000000000414141
# user defined data
$ p dp->pos
$3 = 0
```
as it sums up,
```
static int dynamicGetbuf(gdIOCtxPtr ctx, void *buf, int len)
{
...
memcpy(buf, (void *) ((char *)dp->data + dp->pos), rlen);
...
}
```
we call memcpy with large dest (buf = 4096), arbitrary size (rlen) and
arbitrary source (dp->data), if rlen is greater than size of the dp->data
we read up ends of the dp->data, if buf (read on gdImageCreateFromWebpCtx)
were printed out later, we would have a situation similar to heartbleed[2].
[1] - https://github.com/libgd/libgd/blob/master/src/gd_webp.c
[2] - heartbleed.com
------------------------------------------------------------------------
[2020-03-16 11:22:58] cmb@php.net
> that's not how security works in the real world
Harald, this is how C works; when passing (the address of) a
buffer to a function, the buffer's size has to be known by the
callee, and this is typically accomplished by passing the size of
the buffer as well.
------------------------------------------------------------------------
[2020-03-16 09:29:50] bugreports at gmail dot com
> Of course, the client of libgd is supposed to pass valid values
that's not how security works in the real world
------------------------------------------------------------------------
[2020-03-16 08:00:59] cmb@php.net
> Both arguments of presented functions
> (gdImageCreateFromWebpPtr(int size, void *data) and
> gdImageCreateFromJpegPtr(int size, void *data)) are user supplied
> data with no prior sanitize.
No, they are not. Of course, the client of libgd is supposed to
pass valid values, just like when calling memcpy(), for instance.
------------------------------------------------------------------------
[2020-03-16 00:03:48] stas@php.net
I imagine this has to be reported to libgd maintainers? Especially given it reproduces without PHP
in the picture at all? Or the problem is already fixed in libgd and PHP bundled one is behind?
------------------------------------------------------------------------
The remainder of the comments for this report are too long. To view
the rest of the comments, please view the bug report online at
https://bugs.php.net/bug.php?id=79385
--
Edit this bug report at https://bugs.php.net/bug.php?id=79385&edit=1