gdImageGdPtr memory leak

[meweez]

Hello,

I found that 'gdImageGdPtr' in gd_gd.c and 'gdImageWebpPtr' in gd_webp.c are similar functions for different picture formats. You have changed 'gdImageWebpPtr' because of CVE-2016-6912 (double free), So It seems that you need to change 'gdImageGdPtr' too.
I run two test files with ASAN, and the result is shown below. The test files are located in the 'tests/webp' folder.

Test1:

#include "gd.h"
#include "gdtest.h"


int main()
{
gdImagePtr im1, im2;
int size;

im1 = NULL;
im2 = gdImageGdPtr(im1, &size);
gdTestAssert(im2 == NULL);

gdImageDestroy(im1);

return gdNumFailures();
}

ASAN result:

AddressSanitizer:DEADLYSIGNAL
=================================================================
==4303==ERROR: AddressSanitizer: SEGV on unknown address 0x000000001c50 (pc 0x7f368bd976d8 bp 0x7ffd77fc7d70 sp 0x7ffd77fc7d60 T0)
==4303==The signal is caused by a READ memory access.
#0 0x7f368bd976d7 in _gdPutHeader /libgd/src/gd_gd.c:353
#1 0x7f368bd977d6 in _gdImageGd /libgd/src/gd_gd.c:370
#2 0x7f368bd97aec in gdImageGdPtr /libgd/src/gd_gd.c:403
#3 0x557763c46493 in main /libgd/tests/webp/gd_mytest.c:12
#4 0x7f368b68309a in __libc_start_main ../csu/libc-start.c:308
#5 0x557763c46339 in _start (/libgd/build/Bin/test_webp_gd_mytest+0x2339)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /libgd/src/gd_gd.c:353 in _gdPutHeader
==4303==ABORTING

Test2:

#include "gd.h"
#include "gdtest.h"


int main()
{
gdImagePtr im1, im2;
FILE *fp;
int size;

fp = gdTestFileOpen2("webp", "bug_double_free.jpg");
gdTestAssert(fp != NULL);
im1 = gdImageCreateFromJpeg(fp);
gdTestAssert(im1 != NULL);
fclose(fp);


im2 = gdImageGdPtr(im1, &size);
gdTestAssert(im2 == NULL);

gdImageDestroy(im1);

return gdNumFailures();
}

ASAN result:

=================================================================
==4395==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 33190811 byte(s) in 1 object(s) allocated from:
#0 0x7f2a27953720 in __interceptor_realloc (/lib/x86_64-linux-gnu/libasan.so.5+0xe9720)
#1 0x7f2a2768fb9d in gdRealloc /libgd/src/gdhelpers.c:81
#2 0x7f2a2766cb8d in gdReallocDynamic /libgd/src/gd_io_dp.c:407
#3 0x7f2a2766ce07 in trimDynamic /libgd/src/gd_io_dp.c:437
#4 0x7f2a2766b6ea in gdDPExtractData /libgd/src/gd_io_dp.c:128
#5 0x7f2a27650aff in gdImageGdPtr /libgd/src/gd_gd.c:404
#6 0x5570c9a34525 in main /libgd/tests/webp/gd_mytest.c:20
#7 0x7f2a26f3c09a in __libc_start_main ../csu/libc-start.c:308

SUMMARY: AddressSanitizer: 33190811 byte(s) leaked in 1 allocation(s).

Is there another CVE here?

[cmb69]

The first test script is bogus, since ìm1is not initialized. The second test script does notgdImageDestroy(im2)` which apperars to be the cause of the memory leak.

Anyhow, this is not related to CVE-2016-6912, because that was about a double-free in case gdImageWebpCtx() failed, but _gdImageGd() cannot fail.

[meweez]

Sorry I had a typo in test 1 and im1 = NULL; but there is no check for this exception in _gdImageGd.

You are using some conditional checks in _gdImageWebpCtx and then return 0 or one based on that to prevent double free and other problems. why don't you use such similar conditions in _gdImageGd too?

the second test is the test file you are using here with a little change that I replace gdImageWebpPtr with gdImageGdPtr.
when I added gdImageDestroy(im2) the result is here.

/libgd/tests/webp/gd_mytest.c:21: Assert failed in </libgd/tests/webp/gd_mytest.c:21>
AddressSanitizer:DEADLYSIGNAL
=================================================================
==4659==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7fefd9b0a9fb bp 0x7ffcb9b0f7c0 sp 0x7ffcb9b0f7a0 T0)
==4659==The signal is caused by a READ memory access.
==4659==Hint: address points to the zero page.
    #0 0x7fefd9b0a9fa in gdImageDestroy /libgd/src/gd.c:392
    #1 0x563ab073556b in main /libgd/tests/webp/gd_mytest.c:24
    #2 0x7fefd942009a in __libc_start_main ../csu/libc-start.c:308
    #3 0x563ab0735349 in _start (/libgd/build/Bin/test_webp_gd_mytest+0x2349)

AddressSanitizer can not provide additional info.
SUMMARY: AddressSanitizer: SEGV /libgd/src/gd.c:392 in gdImageDestroy
==4659==ABORTING

[vapier]

if you pass garbage to libgd, you can only expect garbage back. it is not gd's responsibility to make sure programmers don't pass it garbage. that includes, but is not limited to, programmers passing in NULL pointers.

gd guarantees that failures coming from the OS are handled gracefully when possible, as are corrupt input files/images.

[cmb69]

when I added gdImageDestroy(im2) the result is here.

If im2==NULL this is to be expected; hoewever, I cannot reproduce this; for me the test fails on that assertion.

[meweez]

I'm using this:

#include "gd.h"
#include "gdtest.h"


int main()
{
    gdImagePtr im1, im2;
    FILE *fp;
    int size;

    fp = gdTestFileOpen2("webp", "bug_double_free.jpg");
    gdTestAssert(fp != NULL);
    im1 = gdImageCreateFromJpeg(fp);
    gdTestAssert(im1 != NULL);
    fclose(fp);

  
    im2 = gdImageGdPtr(im1, &size);
    gdTestAssert(im2 == NULL);

    gdImageDestroy(im1);
    if(im2 != NULL)
        gdImageDestroy(im2);

    return gdNumFailures();
}

[meweez]

gd guarantees that failures coming from the OS are handled gracefully when possible, as are corrupt input files/images.

Sorry, but I couldn't understand why you are using multiple checks on _gdImageWebpCtx if it is not gd's responsibility?

thanks

[vapier]

"some places check for NULL" is not the same thing as "gd guarantees you can pass it NULL pointers". if you have code passing NULL pointers to functions, your code is bad and needs fixing. it is not a bug in gd.

we should be able to add non-null attributes to our functions so the compiler could try and enforce that too.

[cmb69]

It seems to me a very typical issue is to get NULL from calling an image creation function (e.g. due to wrong image format). And this needs to be handled by the libgd client.

libwebp may fail to encode an image, and this also needs to be handled by the client; in this case libgd is the client.

The only reason _gdImageGd() may fail is due to one of the memory allocation functions failing (i.e. out of memory); this is indeed not cleanly handled by libgd (for a start, all putter functions in gd_io.c would need to report sucess/failure). However, this is not really an issue for me, since I mainly work with an infallible allocator.

[vapier]

libwebp may fail to encode an image, and this also needs to be handled by the client; in this case libgd is the client.

correct. i would consider any library gd calls to be part of the "OS".

[meweez]

Hi,
Similar to the previous discussion, function _gdImageGd2 which is called in gdImageGd2Ptr in gd_gd2.c there is no return value to show fail in _gdImageGd2 and no return value check in gdImageGd2Ptr.

despite of _gdImageGd(), There are some conditions in _gdImageGd2 which lead it to fail1. So if it goes through these fail situations it means that out isn't allocated correctly and calling the gdDPExtractData leads to double free.

Isn't it necessary to add success and fail return value to _gdImageGd2?

[cmb69]

Isn't it necessary to add success and fail return value to _gdImageGd2?

Oh, right. Stll, not a security issue, since it is documented:

It has to be regarded as being obsolete, and should only be used for development and testing purposes.

[meweez]

not a security issue,

But there are some CVEs on GD2 IO like CVE-2016-3074 in _gd2GetHeader function.

[cmb69]

I guess the function was not documented to be not intended for production usage back then.