New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libgd double-free vulnerability #381

Closed
varsleak opened this Issue Feb 14, 2017 · 12 comments

Comments

Projects
None yet
3 participants
@varsleak

varsleak commented Feb 14, 2017

Description

I find the libgd double-free vulnerability when call gdImagePngPtr function.

Environment

Ubuntu x64 16.04
libgd 2.2.4 (Latest commit e65415d)

Detail

➜  gcc_asan_build git:(master) ✗ Bin/test_webp_bug_double_free
GD Warning: gd-png error: no colors in palette
=================================================================
==9574==ERROR: AddressSanitizer: attempting **double-free** on 0x61d00001ea80 in thread T0:
    #0 0x7f39d2d5e2ca in __interceptor_free (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x982ca)
    #1 0x7f39d2a63d19 in gdFree /home/varsleak/github/libgd/src/gdhelpers.c:115
    #2 0x7f39d2a37551 in gdReallocDynamic /home/varsleak/github/libgd/src/gd_io_dp.c:412
    #3 0x7f39d2a37662 in trimDynamic /home/varsleak/github/libgd/src/gd_io_dp.c:427
    #4 0x7f39d2a35fca in gdDPExtractData /home/varsleak/github/libgd/src/gd_io_dp.c:127
    #5 0x7f39d2a440e1 in gdImagePngPtr /home/varsleak/github/libgd/src/gd_png.c:661
    #6 0x400b06 in main /home/varsleak/github/libgd/tests/webp/bug_double_free.c:19
    #7 0x7f39d262582f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #8 0x400968 in _start (/home/varsleak/github/libgd/gcc_asan_build/Bin/test_webp_bug_double_free+0x400968)

0x61d00001ea80 is located 0 bytes inside of 2048-byte region [0x61d00001ea80,0x61d00001f280)
freed by thread T0 here:
    #0 0x7f39d2d5e961 in realloc (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x98961)
    #1 0x7f39d2a63cb8 in gdRealloc /home/varsleak/github/libgd/src/gdhelpers.c:81
    #2 0x7f39d2a373e8 in gdReallocDynamic /home/varsleak/github/libgd/src/gd_io_dp.c:397
    #3 0x7f39d2a37662 in trimDynamic /home/varsleak/github/libgd/src/gd_io_dp.c:427
    #4 0x7f39d2a35fca in gdDPExtractData /home/varsleak/github/libgd/src/gd_io_dp.c:127
    #5 0x7f39d2a440e1 in gdImagePngPtr /home/varsleak/github/libgd/src/gd_png.c:661
    #6 0x400b06 in main /home/varsleak/github/libgd/tests/webp/bug_double_free.c:19
    #7 0x7f39d262582f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)

previously allocated by thread T0 here:
    #0 0x7f39d2d5e602 in malloc (/usr/lib/x86_64-linux-gnu/libasan.so.2+0x98602)
    #1 0x7f39d2a63c93 in gdMalloc /home/varsleak/github/libgd/src/gdhelpers.c:75
    #2 0x7f39d2a36e8a in allocDynamic /home/varsleak/github/libgd/src/gd_io_dp.c:327
    #3 0x7f39d2a366f5 in newDynamic /home/varsleak/github/libgd/src/gd_io_dp.c:230
    #4 0x7f39d2a35d74 in gdNewDynamicCtxEx /home/varsleak/github/libgd/src/gd_io_dp.c:91
    #5 0x7f39d2a35d2d in gdNewDynamicCtx /home/varsleak/github/libgd/src/gd_io_dp.c:75
    #6 0x7f39d2a440a4 in gdImagePngPtr /home/varsleak/github/libgd/src/gd_png.c:658
    #7 0x400b06 in main /home/varsleak/github/libgd/tests/webp/bug_double_free.c:19
    #8 0x7f39d262582f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)

SUMMARY: AddressSanitizer: double-free ??:0 __interceptor_free
==9574==ABORTING

the c code

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


int main() {
    gdImagePtr im1 = 0;
    void * im2 = 0;
    int size = 0;

    im1 = gdImageCreate(100, 100);

    im2 = gdImagePngPtr(im1, &size);

    gdFree(im2);

    gdImageDestroy(im1);

    return 0;
}
@cmb69

This comment has been minimized.

Show comment
Hide comment
@cmb69

cmb69 Feb 14, 2017

Contributor

The problem is that gdImagePngPtr() calls gdImangePngCtxEx(), but the latter bails out because there are no colors in the palette. However, gdImagePngCtxEx() doesn't provide a meaningful return value, so gdImagePngPtr() can't check whether the operation succeeded.

It seems that we need a fix analogous to commt a49feea. Other image output functions might be affected as well.

Contributor

cmb69 commented Feb 14, 2017

The problem is that gdImagePngPtr() calls gdImangePngCtxEx(), but the latter bails out because there are no colors in the palette. However, gdImagePngCtxEx() doesn't provide a meaningful return value, so gdImagePngPtr() can't check whether the operation succeeded.

It seems that we need a fix analogous to commt a49feea. Other image output functions might be affected as well.

@cmb69 cmb69 added the bug label Feb 14, 2017

@cmb69

This comment has been minimized.

Show comment
Hide comment
@cmb69

cmb69 Feb 14, 2017

Contributor

To clarify: this is not a security issue, but rather a programmer error, but still it should be fixed.

Contributor

cmb69 commented Feb 14, 2017

To clarify: this is not a security issue, but rather a programmer error, but still it should be fixed.

@varsleak

This comment has been minimized.

Show comment
Hide comment
@varsleak

varsleak Feb 15, 2017

OK. thank you very much.

varsleak commented Feb 15, 2017

OK. thank you very much.

@varsleak

This comment has been minimized.

Show comment
Hide comment
@varsleak

varsleak Feb 17, 2017

I find the double-free vulnerability again, the file leading to the crash is double-free.txt.

int main(int argc, char * argv[])
{
    gdImagePtr im = 0;
    FILE *fp = 0;
    void * im2 = 0;
    int size = 0;

    if (argc != 2) return -1;

    fp = fopen(argv[1], "rb");
    if (fp){
	    im = gdImageCreateFromGd(fp);

	    if (im){
	    	im2 = gdImagePngPtr(im, &size);

	    	if (im2){
	    		gdFree(im2);
	    	}
	    	gdImageDestroy(im);
		}
	    
	}

    fclose(fp);

    return gdNumFailures();
}

varsleak commented Feb 17, 2017

I find the double-free vulnerability again, the file leading to the crash is double-free.txt.

int main(int argc, char * argv[])
{
    gdImagePtr im = 0;
    FILE *fp = 0;
    void * im2 = 0;
    int size = 0;

    if (argc != 2) return -1;

    fp = fopen(argv[1], "rb");
    if (fp){
	    im = gdImageCreateFromGd(fp);

	    if (im){
	    	im2 = gdImagePngPtr(im, &size);

	    	if (im2){
	    		gdFree(im2);
	    	}
	    	gdImageDestroy(im);
		}
	    
	}

    fclose(fp);

    return gdNumFailures();
}

@varsleak varsleak closed this Feb 17, 2017

@cmb69

This comment has been minimized.

Show comment
Hide comment
@cmb69

cmb69 Feb 17, 2017

Contributor

Why did you close this ticket? I assume that was by accident, so i'm re-opening.

Contributor

cmb69 commented Feb 17, 2017

Why did you close this ticket? I assume that was by accident, so i'm re-opening.

@cmb69 cmb69 reopened this Feb 17, 2017

@varsleak

This comment has been minimized.

Show comment
Hide comment
@varsleak

varsleak Feb 18, 2017

Yes, that's an accident.
I'm so sorry.

varsleak commented Feb 18, 2017

Yes, that's an accident.
I'm so sorry.

@varsleak

This comment has been minimized.

Show comment
Hide comment
@varsleak

varsleak Feb 24, 2017

Analysis

The second argument(int required) of the gdReallocDynamic function is 0.

Step 1: at if((newPtr = gdRealloc(dp->data, required))) { , dp->data will be free and the newPtr will be set NULL due to required is 0.

Step 2: newPtr = gdMalloc(required); newPtr be set new memory space. funny is the newPtr and dp->data are the same address.

Step 3: memcpy is work.

Step 4: gdFree(dp->data) ,notice newPtr and dp->data is same. The first free occured,

Step 5: return dp->data to previous function.

The end:
Under normal circumstances we should free this pointer. The second free. occured.

static int gdReallocDynamic(dynamicPtr *dp, int required)
{
	void *newPtr;

	/* First try gdRealloc().  If that doesn't work, make a new
	 * memory block and copy. */
	if((newPtr = gdRealloc(dp->data, required))) {
		dp->realSize = required;
		dp->data = newPtr;
		return TRUE;
	}

	/* create a new pointer */
	newPtr = gdMalloc(required);
	if(!newPtr) {
		dp->dataGood = FALSE;
		return FALSE;
	}

	/* copy the old data into it */
	memcpy(newPtr, dp->data, dp->logicalSize);
	gdFree(dp->data);                           **<- the first free.**
	dp->data = newPtr;

	dp->realSize = required;
	return TRUE;
}

.....


int main(int argc, char * argv[])
{
    gdImagePtr im = 0;
    FILE *fp = 0;
    void * im2 = 0;
    int size = 0;

    if (argc != 2) return -1;

    fp = fopen(argv[1], "rb");
    if (fp){
	    im = gdImageCreateFromGd(fp);

	    if (im){
	    	im2 = gdImagePngPtr(im, &size);

	    	if (im2){
	    		gdFree(im2);               **<- the second free.**
	    	}
	    	gdImageDestroy(im);
		}
	    
	}

    fclose(fp);

    return gdNumFailures();
}

varsleak commented Feb 24, 2017

Analysis

The second argument(int required) of the gdReallocDynamic function is 0.

Step 1: at if((newPtr = gdRealloc(dp->data, required))) { , dp->data will be free and the newPtr will be set NULL due to required is 0.

Step 2: newPtr = gdMalloc(required); newPtr be set new memory space. funny is the newPtr and dp->data are the same address.

Step 3: memcpy is work.

Step 4: gdFree(dp->data) ,notice newPtr and dp->data is same. The first free occured,

Step 5: return dp->data to previous function.

The end:
Under normal circumstances we should free this pointer. The second free. occured.

static int gdReallocDynamic(dynamicPtr *dp, int required)
{
	void *newPtr;

	/* First try gdRealloc().  If that doesn't work, make a new
	 * memory block and copy. */
	if((newPtr = gdRealloc(dp->data, required))) {
		dp->realSize = required;
		dp->data = newPtr;
		return TRUE;
	}

	/* create a new pointer */
	newPtr = gdMalloc(required);
	if(!newPtr) {
		dp->dataGood = FALSE;
		return FALSE;
	}

	/* copy the old data into it */
	memcpy(newPtr, dp->data, dp->logicalSize);
	gdFree(dp->data);                           **<- the first free.**
	dp->data = newPtr;

	dp->realSize = required;
	return TRUE;
}

.....


int main(int argc, char * argv[])
{
    gdImagePtr im = 0;
    FILE *fp = 0;
    void * im2 = 0;
    int size = 0;

    if (argc != 2) return -1;

    fp = fopen(argv[1], "rb");
    if (fp){
	    im = gdImageCreateFromGd(fp);

	    if (im){
	    	im2 = gdImagePngPtr(im, &size);

	    	if (im2){
	    		gdFree(im2);               **<- the second free.**
	    	}
	    	gdImageDestroy(im);
		}
	    
	}

    fclose(fp);

    return gdNumFailures();
}

@varsleak

This comment has been minimized.

Show comment
Hide comment
@varsleak

varsleak Feb 28, 2017

CVE-2017-6362 was assigned

varsleak commented Feb 28, 2017

CVE-2017-6362 was assigned

@pierrejoye

This comment has been minimized.

Show comment
Hide comment
@pierrejoye

pierrejoye May 4, 2017

Contributor

Is this bug still valid?

Contributor

pierrejoye commented May 4, 2017

Is this bug still valid?

@cmb69

This comment has been minimized.

Show comment
Hide comment
@cmb69

cmb69 May 4, 2017

Contributor

@pierrejoye Yes, still valid (I've send a patch in 41c4ecbc-f5a4-4592-8a74-b4b1155ae2d5@gmx.de). PHP's bundled libgd is affected as well, so we should coordinate the releases. I'm not sure what to do for PHP 5.6. Is there any schedule for 5.6.31?

Contributor

cmb69 commented May 4, 2017

@pierrejoye Yes, still valid (I've send a patch in 41c4ecbc-f5a4-4592-8a74-b4b1155ae2d5@gmx.de). PHP's bundled libgd is affected as well, so we should coordinate the releases. I'm not sure what to do for PHP 5.6. Is there any schedule for 5.6.31?

@cmb69 cmb69 added this to the GD 2.2.5 milestone Aug 7, 2017

@cmb69

This comment has been minimized.

Show comment
Hide comment
@cmb69

cmb69 Aug 27, 2017

Contributor

(I've send a patch in 41c4ecbc-f5a4-4592-8a74-b4b1155ae2d5@gmx.de).

I did, but this was about a completely unrelated issue.

PS:

this is not a security issue, but rather a programmer error, but still it should be fixed.

As the second example (importing a GD image) shows, this is a security issue.

Contributor

cmb69 commented Aug 27, 2017

(I've send a patch in 41c4ecbc-f5a4-4592-8a74-b4b1155ae2d5@gmx.de).

I did, but this was about a completely unrelated issue.

PS:

this is not a security issue, but rather a programmer error, but still it should be fixed.

As the second example (importing a GD image) shows, this is a security issue.

@cmb69 cmb69 closed this in 2207e3c Aug 27, 2017

cmb69 added a commit that referenced this issue Aug 27, 2017

Fix #381: libgd double-free vulnerability
The issue is that `gdImagePngCtxEx` (which is called by `gdImagePngPtr`
and the other PNG output functions to do the real work) does not return
whether it succeeded or failed, so this is not checked in
`gdImagePngPtr` and the function wrongly assumes everything is okay,
which is not, in this case, because the palette image contains no
palette entries.

We can't change the signature of `gdImagePngCtxEx` for API
compatibility reasons, so we introduce the static helper
`_gdImagePngCtxEx` which returns success respective failure, so
`gdImagePngPtr` and `gdImagePngPtrEx` can check the return value. We
leave it solely to libpng for now to report warnings regarding the
failing write.

CVE-2017-6362

(cherry picked from commit 2207e3c)
@cmb69

This comment has been minimized.

Show comment
Hide comment
@cmb69

cmb69 Aug 27, 2017

Contributor

PHP's bundled libgd is affected as well, so we should coordinate the releases.

No. Actually, gdImagePngPtr(Ex) is not used by PHP's GD extension at all.

Contributor

cmb69 commented Aug 27, 2017

PHP's bundled libgd is affected as well, so we should coordinate the releases.

No. Actually, gdImagePngPtr(Ex) is not used by PHP's GD extension at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment