CVE-2016-6911: Out-of-bounds Read (CWE-125)
value: The dynamicGetbuf function in the GD Graphics Library (aka libgd) before 2.2.4 allows remote attackers to cause a denial of service (out-of-bounds read) via a crafted TIFF image.
CWE description: The product reads data past the end, or before the beginning, of the intended buffer.
CVSS v3 base score: 5.5
Published: 2017-01-26T15:59Z
Repository: https://github.com/libgd/libgd
Language: C
Files changed: src/gd_io_dp.c, src/gd_tiff.c, tests/tiff/.gitignore, tests/tiff/CMakeLists.txt, tests/tiff/Makemodule.am, tests/tiff/tiff_invalid_read.c
Commit date: 2016-12-13 13:55:48 +0100
Commit message: Fix invalid read in gdImageCreateFromTiffPtr()

tiff_invalid_read.tiff is corrupt, and causes an invalid read in
gdImageCreateFromTiffPtr(), but not in gdImageCreateFromTiff(). The culprit
is dynamicGetbuf(), which doesn't check for out-of-bound reads. In this case,
dynamicGetbuf() is called with a negative dp->pos, but also positive buffer
overflows have to be handled, in which case 0 has to be returned (cf. commit
75e29a9).

Fixing dynamicGetbuf() exhibits that the corrupt TIFF would still create
the image, because the return value of TIFFReadRGBAImage() is not checked.
We do that, and let createFromTiffRgba() fail if TIFFReadRGBAImage() fails.

This issue had been reported by Ibrahim El-Sayed to security@libgd.org.

CVE-2016-6911

Diff: diff --git a/src/gd_io_dp.c b/src/gd_io_dp.c
index 228bfa5c..eda2eeb0 100644
--- a/src/gd_io_dp.c
+++ b/src/gd_io_dp.c
@@ -261,34 +261,39 @@ static void dynamicPutchar(struct gdIOCtx *ctx, int a)
 	dctx = (dpIOCtxPtr) ctx;
 
 	appendDynamic(dctx->dp, &b, 1);
 }
 
+/* returns the number of bytes actually read; 0 on EOF and error */
 static int dynamicGetbuf(gdIOCtxPtr ctx, void *buf, int len)
 {
 	int rlen, remain;
 	dpIOCtxPtr dctx;
 	dynamicPtr *dp;
 
 	dctx = (dpIOCtxPtr) ctx;
 	dp = dctx->dp;
 
+	if (dp->pos < 0 || dp->pos >= dp->realSize) {
+		return 0;
+	}
+
 	remain = dp->logicalSize - dp->pos;
 	if(remain >= len) {
 		rlen = len;
 	} else {
 		if(remain <= 0) {
-			/* 2.0.34: EOF is incorrect. We use 0 for
-			 * errors and EOF, just like fileGetbuf,
-			 * which is a simple fread() wrapper.
-			 * TBB. Original bug report: Daniel Cowgill. */
-			return 0; /* NOT EOF */
+			return 0;
 		}
 
 		rlen = remain;
 	}
 
+	if (dp->pos + rlen > dp->realSize) {
+		rlen = dp->realSize - dp->pos;
+	}
+
 	memcpy(buf, (void *) ((char *)dp->data + dp->pos), rlen);
 	dp->pos += rlen;
 
 	return rlen;
 }
diff --git a/src/gd_tiff.c b/src/gd_tiff.c
index b4f1e637..3f20c5b2 100644
--- a/src/gd_tiff.c
+++ b/src/gd_tiff.c
@@ -757,10 +757,11 @@ static int createFromTiffRgba(TIFF * tif, gdImagePtr im)
 	int color;
 	int width = im->sx;
 	int height = im->sy;
 	uint32 *buffer;
 	uint32 rgba;
+	int success;
 
 	/* switch off colour merging on target gd image just while we write out
 	 * content - we want to preserve the alpha data until the user chooses
 	 * what to do with the image */
 	alphaBlendingFlag = im->alphaBlendingFlag;
@@ -769,30 +770,32 @@ static int createFromTiffRgba(TIFF * tif, gdImagePtr im)
 	buffer = (uint32 *) gdCalloc(sizeof(uint32), width * height);
 	if (!buffer) {
 		return GD_FAILURE;
 	}
 
-	TIFFReadRGBAImage(tif, width, height, buffer, 0);
-
-	for(y = 0; y < height; y++) {
-		for(x = 0; x < width; x++) {
-			/* if it doesn't already exist, allocate a new colour,
-			 * else use existing one */
-			rgba = buffer[(y * width + x)];
-			a = (0xff - TIFFGetA(rgba)) / 2;
-			color = gdTrueColorAlpha(TIFFGetR(rgba), TIFFGetG(rgba), TIFFGetB(rgba), a);
-
-			/* set pixel colour to this colour */
-			gdImageSetPixel(im, x, height - y - 1, color);
+	success = TIFFReadRGBAImage(tif, width, height, buffer, 1);
+
+	if (success) {
+		for(y = 0; y < height; y++) {
+			for(x = 0; x < width; x++) {
+				/* if it doesn't already exist, allocate a new colour,
+				 * else use existing one */
+				rgba = buffer[(y * width + x)];
+				a = (0xff - TIFFGetA(rgba)) / 2;
+				color = gdTrueColorAlpha(TIFFGetR(rgba), TIFFGetG(rgba), TIFFGetB(rgba), a);
+	
+				/* set pixel colour to this colour */
+				gdImageSetPixel(im, x, height - y - 1, color);
+			}
 		}
 	}
 
 	gdFree(buffer);
 
 	/* now reset colour merge for alpha blending routines */
 	gdImageAlphaBlending(im, alphaBlendingFlag);
-	return GD_SUCCESS;
+	return success;
 }
 
 /*
 	Function: gdImageCreateFromTiffCtx
 
diff --git a/tests/tiff/.gitignore b/tests/tiff/.gitignore
index 7cf35ee8..273e8499 100644
--- a/tests/tiff/.gitignore
+++ b/tests/tiff/.gitignore
@@ -1,3 +1,4 @@
 /tiff_dpi
 /tiff_im2im
 /tiff_null
+/tiff_invalid_read
diff --git a/tests/tiff/CMakeLists.txt b/tests/tiff/CMakeLists.txt
index 03f630ca..81f23033 100644
--- a/tests/tiff/CMakeLists.txt
+++ b/tests/tiff/CMakeLists.txt
@@ -1,8 +1,9 @@
 IF(TIFF_FOUND)
 LIST(APPEND TESTS_FILES
 	tiff_im2im
+	tiff_invalid_read
 	tiff_null
 	tiff_dpi
 )
 ENDIF(TIFF_FOUND)
 
diff --git a/tests/tiff/Makemodule.am b/tests/tiff/Makemodule.am
index fd396990..670434a7 100644
--- a/tests/tiff/Makemodule.am
+++ b/tests/tiff/Makemodule.am
@@ -1,9 +1,13 @@
 if HAVE_LIBTIFF
 libgd_test_programs += \
 	tiff/tiff_dpi \
 	tiff/tiff_im2im \
+	tiff/tiff_invalid_read \
 	tiff/tiff_null
 endif
 
 EXTRA_DIST += \
-	tiff/CMakeLists.txt
+	tiff/CMakeLists.txt \
+	tiff/tiff_invalid_read_1.tiff \
+	tiff/tiff_invalid_read_2.tiff \
+	tiff/tiff_invalid_read_3.tiff
diff --git a/tests/tiff/tiff_invalid_read.c b/tests/tiff/tiff_invalid_read.c
new file mode 100644
index 00000000..bed53891
--- /dev/null
+++ b/tests/tiff/tiff_invalid_read.c
@@ -0,0 +1,61 @@
+/*
+We're testing that reading corrupt TIFF files doesn't cause any memory issues,
+and that the operation gracefully fails (i.e. gdImageCreateFromTiffPtr() returns
+NULL).
+*/
+
+#include "gd.h"
+#include "gdtest.h"
+
+
+static void check_file(char *basename);
+static size_t read_test_file(char **buffer, char *basename);
+
+
+int main()
+{
+    check_file("tiff_invalid_read_1.tiff");
+    check_file("tiff_invalid_read_2.tiff");
+    check_file("tiff_invalid_read_3.tiff");
+
+    return gdNumFailures();
+}
+
+
+static void check_file(char *basename)
+{
+    gdImagePtr im;
+    char *buffer;
+    size_t size;
+
+    size = read_test_file(&buffer, basename);
+    im = gdImageCreateFromTiffPtr(size, (void *) buffer);
+    gdTestAssert(im == NULL);
+    free(buffer);
+}
+
+
+static size_t read_test_file(char **buffer, char *basename)
+{
+    char *filename;
+    FILE *fp;
+    size_t exp_size, act_size;
+
+    filename = gdTestFilePath2("tiff", basename);
+    fp = fopen(filename, "rb");
+    gdTestAssert(fp != NULL);
+
+	fseek(fp, 0, SEEK_END);
+	exp_size = ftell(fp);
+	fseek(fp, 0, SEEK_SET);
+
+    *buffer = malloc(exp_size);
+    gdTestAssert(*buffer != NULL);
+    act_size = fread(*buffer, sizeof(**buffer), exp_size, fp);
+    gdTestAssert(act_size == exp_size);
+
+    fclose(fp);
+    free(filename);
+
+    return act_size;
+}
diff --git a/tests/tiff/tiff_invalid_read_1.tiff b/tests/tiff/tiff_invalid_read_1.tiff
new file mode 100644
index 00000000..b3877e33
Binary files /dev/null and b/tests/tiff/tiff_invalid_read_1.tiff differ
diff --git a/tests/tiff/tiff_invalid_read_2.tiff b/tests/tiff/tiff_invalid_read_2.tiff
new file mode 100644
index 00000000..f9967e54
Binary files /dev/null and b/tests/tiff/tiff_invalid_read_2.tiff differ
diff --git a/tests/tiff/tiff_invalid_read_3.tiff b/tests/tiff/tiff_invalid_read_3.tiff
new file mode 100644
index 00000000..67ca2a7c
Binary files /dev/null and b/tests/tiff/tiff_invalid_read_3.tiff differ

Vulnerable code: 			/* 2.0.34: EOF is incorrect. We use 0 for
			 * errors and EOF, just like fileGetbuf,
			 * which is a simple fread() wrapper.
			 * TBB. Original bug report: Daniel Cowgill. */
			return 0; /* NOT EOF */
	TIFFReadRGBAImage(tif, width, height, buffer, 0);

	for(y = 0; y < height; y++) {
		for(x = 0; x < width; x++) {
			/* if it doesn't already exist, allocate a new colour,
			 * else use existing one */
			rgba = buffer[(y * width + x)];
			a = (0xff - TIFFGetA(rgba)) / 2;
			color = gdTrueColorAlpha(TIFFGetR(rgba), TIFFGetG(rgba), TIFFGetB(rgba), a);

			/* set pixel colour to this colour */
			gdImageSetPixel(im, x, height - y - 1, color);
	return GD_SUCCESS;
	tiff/CMakeLists.txt
Fixed code: /* returns the number of bytes actually read; 0 on EOF and error */
	if (dp->pos < 0 || dp->pos >= dp->realSize) {
		return 0;
	}

			return 0;
	if (dp->pos + rlen > dp->realSize) {
		rlen = dp->realSize - dp->pos;
	}

	int success;
	success = TIFFReadRGBAImage(tif, width, height, buffer, 1);

	if (success) {
		for(y = 0; y < height; y++) {
			for(x = 0; x < width; x++) {
				/* if it doesn't already exist, allocate a new colour,
				 * else use existing one */
				rgba = buffer[(y * width + x)];
				a = (0xff - TIFFGetA(rgba)) / 2;
				color = gdTrueColorAlpha(TIFFGetR(rgba), TIFFGetG(rgba), TIFFGetB(rgba), a);
	
				/* set pixel colour to this colour */
				gdImageSetPixel(im, x, height - y - 1, color);
			}
	return success;
/tiff_invalid_read
	tiff_invalid_read
	tiff/tiff_invalid_read \
	tiff/CMakeLists.txt \
	tiff/tiff_invalid_read_1.tiff \
	tiff/tiff_invalid_read_2.tiff \
	tiff/tiff_invalid_read_3.tiff
/*
We're testing that reading corrupt TIFF files doesn't cause any memory issues,
and that the operation gracefully fails (i.e. gdImageCreateFromTiffPtr() returns
NULL).
*/

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


static void check_file(char *basename);
static size_t read_test_file(char **buffer, char *basename);


int main()
{
    check_file("tiff_invalid_read_1.tiff");
    check_file("tiff_invalid_read_2.tiff");
    check_file("tiff_invalid_read_3.tiff");

    return gdNumFailures();
}


static void check_file(char *basename)
{
    gdImagePtr im;
    char *buffer;
    size_t size;

    size = read_test_file(&buffer, basename);
    im = gdImageCreateFromTiffPtr(size, (void *) buffer);
    gdTestAssert(im == NULL);
    free(buffer);
}


static size_t read_test_file(char **buffer, char *basename)
{
    char *filename;
    FILE *fp;
    size_t exp_size, act_size;

    filename = gdTestFilePath2("tiff", basename);
    fp = fopen(filename, "rb");
    gdTestAssert(fp != NULL);

	fseek(fp, 0, SEEK_END);
	exp_size = ftell(fp);
	fseek(fp, 0, SEEK_SET);

    *buffer = malloc(exp_size);
    gdTestAssert(*buffer != NULL);
    act_size = fread(*buffer, sizeof(**buffer), exp_size, fp);
    gdTestAssert(act_size == exp_size);

    fclose(fp);
    free(filename);

    return act_size;
}