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

src/gd_png.c

@@ -634,6 +634,7 @@ BGD_DECLARE(void) gdImagePng (gdImagePtr im, FILE * outFile)
 	out->gd_free (out);
 }
 
+static int _gdImagePngCtxEx(gdImagePtr im, gdIOCtx * outfile, int level);
 
 /*
   Function: gdImagePngPtr
@@ -657,8 +658,11 @@ BGD_DECLARE(void *) gdImagePngPtr (gdImagePtr im, int *size)
 	void *rv;
 	gdIOCtx *out = gdNewDynamicCtx (2048, NULL);
 	if (out == NULL) return NULL;
-	gdImagePngCtxEx (im, out, -1);
-	rv = gdDPExtractData (out, size);
+	if (!_gdImagePngCtxEx (im, out, -1)) {
+		rv = gdDPExtractData (out, size);
+	} else {
+		rv = NULL;
+	}
 	out->gd_free (out);
 	return rv;
 }
@@ -692,8 +696,11 @@ BGD_DECLARE(void *) gdImagePngPtrEx (gdImagePtr im, int *size, int level)
 	void *rv;
 	gdIOCtx *out = gdNewDynamicCtx (2048, NULL);
 	if (out == NULL) return NULL;
-	gdImagePngCtxEx (im, out, level);
-	rv = gdDPExtractData (out, size);
+	if (!_gdImagePngCtxEx (im, out, level)) {
+		rv = gdDPExtractData (out, size);
+	} else {
+		rv = NULL;
+	}
 	out->gd_free (out);
 	return rv;
 }
@@ -742,12 +749,17 @@ BGD_DECLARE(void) gdImagePngCtx (gdImagePtr im, gdIOCtx * outfile)
     Nothing.
 
 */
+BGD_DECLARE(void) gdImagePngCtxEx (gdImagePtr im, gdIOCtx * outfile, int level)
+{
+	_gdImagePngCtxEx(im, outfile, level);
+}
 
 /* This routine is based in part on code from Dale Lutz (Safe Software Inc.)
  *  and in part on demo code from Chapter 15 of "PNG: The Definitive Guide"
  *  (http://www.libpng.org/pub/png/book/).
  */
-BGD_DECLARE(void) gdImagePngCtxEx (gdImagePtr im, gdIOCtx * outfile, int level)
+/* returns 0 on success, 1 on failure */
+static int _gdImagePngCtxEx(gdImagePtr im, gdIOCtx * outfile, int level)
 {
 	int i, j, bit_depth = 0, interlace_type;
 	int width = im->sx;
@@ -765,10 +777,11 @@ BGD_DECLARE(void) gdImagePngCtxEx (gdImagePtr im, gdIOCtx * outfile, int level)
 #ifdef PNG_SETJMP_SUPPORTED
 	jmpbuf_wrapper jbw;
 #endif
+	int ret = 0;
 
 	/* width or height of value 0 is invalid in IHDR;
 	   see http://www.w3.org/TR/PNG-Chunks.html */
-	if (width == 0 || height ==0) return;
+	if (width == 0 || height ==0) return 1;
 
 #ifdef PNG_SETJMP_SUPPORTED
 	png_ptr = png_create_write_struct (PNG_LIBPNG_VER_STRING,
@@ -779,21 +792,21 @@ BGD_DECLARE(void) gdImagePngCtxEx (gdImagePtr im, gdIOCtx * outfile, int level)
 #endif
 	if (png_ptr == NULL) {
 		gd_error("gd-png error: cannot allocate libpng main struct\n");
-		return;
+		return 1;
 	}
 
 	info_ptr = png_create_info_struct (png_ptr);
 	if (info_ptr == NULL) {
 		gd_error("gd-png error: cannot allocate libpng info struct\n");
 		png_destroy_write_struct (&png_ptr, (png_infopp) NULL);
-		return;
+		return 1;
 	}
 
 #ifdef PNG_SETJMP_SUPPORTED
 	if (setjmp(jbw.jmpbuf)) {
 		gd_error("gd-png error: setjmp returns error condition\n");
 		png_destroy_write_struct (&png_ptr, &info_ptr);
-		return;
+		return 1;
 	}
 #endif
 
@@ -845,6 +858,7 @@ BGD_DECLARE(void) gdImagePngCtxEx (gdImagePtr im, gdIOCtx * outfile, int level)
 		}
 		if (colors == 0) {
 			gd_error("gd-png error: no colors in palette\n");
+			ret = 1;
 			goto bail;
 		}
 		if (colors < im->colorsTotal) {
@@ -976,11 +990,13 @@ BGD_DECLARE(void) gdImagePngCtxEx (gdImagePtr im, gdIOCtx * outfile, int level)
 		png_bytep *prow_pointers;
 		int saveAlphaFlag = im->saveAlphaFlag;
 		if (overflow2(sizeof (png_bytep), height)) {
+			ret = 1;
 			goto bail;
 		}
 		row_pointers = gdMalloc (sizeof (png_bytep) * height);
 		if (row_pointers == NULL) {
 			gd_error("gd-png error: unable to allocate row_pointers\n");
+			ret = 1;
 			goto bail;
 		}
 		prow_pointers = row_pointers;
@@ -992,6 +1008,7 @@ BGD_DECLARE(void) gdImagePngCtxEx (gdImagePtr im, gdIOCtx * outfile, int level)
 					gdFree (row_pointers[i]);
 				/* 2.0.29: memory leak TBB */
 				gdFree(row_pointers);
+				ret = 1;
 				goto bail;
 			}
 			pOutputRow = *prow_pointers++;
@@ -1025,11 +1042,13 @@ BGD_DECLARE(void) gdImagePngCtxEx (gdImagePtr im, gdIOCtx * outfile, int level)
 		if (remap) {
 			png_bytep *row_pointers;
 			if (overflow2(sizeof (png_bytep), height)) {
+				ret = 1;
 				goto bail;
 			}
 			row_pointers = gdMalloc (sizeof (png_bytep) * height);
 			if (row_pointers == NULL) {
 				gd_error("gd-png error: unable to allocate row_pointers\n");
+				ret = 1;
 				goto bail;
 			}
 			for (j = 0; j < height; ++j) {
@@ -1039,6 +1058,7 @@ BGD_DECLARE(void) gdImagePngCtxEx (gdImagePtr im, gdIOCtx * outfile, int level)
 						gdFree (row_pointers[i]);
 					/* TBB: memory leak */
 					gdFree (row_pointers);
+					ret = 1;
 					goto bail;
 				}
 				for (i = 0; i < width; ++i)
@@ -1059,6 +1079,7 @@ BGD_DECLARE(void) gdImagePngCtxEx (gdImagePtr im, gdIOCtx * outfile, int level)
 	/* 1.6.3: maybe we should give that memory BACK! TBB */
 bail:
 	png_destroy_write_struct (&png_ptr, &info_ptr);
+	return ret;
 }
 
 

tests/png/.gitignore

@@ -3,6 +3,8 @@
 /bug00086
 /bug00088
 /bug00193
+/bug00381_1
+/bug00381_2
 /png_im2im
 /png_null
 /png_resolution

tests/png/CMakeLists.txt

@@ -8,6 +8,8 @@ LIST(APPEND TESTS_FILES
 	bug00086
 	bug00088
 	bug00193
+	bug00381_1
+	bug00381_2
 )
 ENDIF(PNG_FOUND)
 

tests/png/Makemodule.am

@@ -5,6 +5,8 @@ libgd_test_programs += \
 	png/bug00086 \
 	png/bug00088 \
 	png/bug00193 \
+	png/bug00381_1 \
+	png/bug00381_2 \
 	png/png_im2im \
 	png/png_null \
 	png/png_resolution
@@ -17,4 +19,5 @@ EXTRA_DIST += \
 	png/bug00088_1_exp.png \
 	png/bug00088_2.png \
 	png/bug00088_2_exp.png \
+	png/bug00381_2.gd \
 	png/emptyfile

tests/png/bug00381_1.c

@@ -0,0 +1,31 @@
+/**
+ * Test that failure to convert to PNG returns NULL
+ *
+ * We are creating a palette image without allocating any colors in the palette,
+ * and pass this image to `gdImagePngPtr()` which is supposed to fail, and as
+ * such should return NULL.
+ *
+ * See also <https://github.com/libgd/libgd/issues/381>
+ */
+
+
+#include "gd.h"
+#include "gdtest.h"
+
+
+int main()
+{
+    gdImagePtr im;
+    void *data;
+    int size = 0;
+
+    im = gdImageCreate(100, 100);
+    gdTestAssert(im != NULL);
+
+    data = gdImagePngPtr(im, &size);
+    gdTestAssert(data == NULL);
+
+    gdImageDestroy(im);
+
+    return gdNumFailures();
+}

tests/png/bug00381_2.c

@@ -0,0 +1,35 @@
+/**
+ * Test that failure to convert to PNG returns NULL
+ *
+ * We are reading a palette image without any colors in the palette, and pass
+ * this image to `gdImagePngPtr()` which is supposed to fail, and as such should
+ * return NULL.
+ *
+ * See also <https://github.com/libgd/libgd/issues/381>
+ */
+
+
+#include "gd.h"
+#include "gdtest.h"
+
+
+int main()
+{
+    gdImagePtr im;
+    FILE *fp;
+    void *data;
+    int size = 0;
+
+    fp = gdTestFileOpen2("png", "bug00381_2.gd");
+    gdTestAssert(fp != NULL);
+    im = gdImageCreateFromGd(fp);
+    gdTestAssert(im != NULL);
+    fclose(fp);
+
+    data = gdImagePngPtr(im, &size);
+    gdTestAssert(data == NULL);
+
+    gdImageDestroy(im);
+
+    return gdNumFailures();
+}