4588f4972b
- Fix stack based buffer overflow when passing negative `rlen` as size to memcpy() (CVE-2016-8670) - Fix possible overflow in gdImageWebpCtx (CVE-2016-7568)
234 lines
11 KiB
Diff
234 lines
11 KiB
Diff
From: "Christoph M. Becker" <cmbecker69@gmx.de>
|
||
Date: Tue, 2 Aug 2016 12:10:33 +0200
|
||
Subject: 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.
|
||
---
|
||
src/gd_io_dp.c | 15 ++++++---
|
||
src/gd_tiff.c | 27 +++++++++-------
|
||
tests/tiff/CMakeLists.txt | 1 +
|
||
tests/tiff/tiff_invalid_read.c | 61 ++++++++++++++++++++++++++++++++++++
|
||
tests/tiff/tiff_invalid_read_1.tiff | Bin 0 -> 3304 bytes
|
||
tests/tiff/tiff_invalid_read_2.tiff | Bin 0 -> 429 bytes
|
||
tests/tiff/tiff_invalid_read_3.tiff | Bin 0 -> 428 bytes
|
||
7 files changed, 87 insertions(+), 17 deletions(-)
|
||
create mode 100644 tests/tiff/tiff_invalid_read.c
|
||
create mode 100644 tests/tiff/tiff_invalid_read_1.tiff
|
||
create mode 100644 tests/tiff/tiff_invalid_read_2.tiff
|
||
create mode 100644 tests/tiff/tiff_invalid_read_3.tiff
|
||
|
||
diff --git a/src/gd_io_dp.c b/src/gd_io_dp.c
|
||
index 228bfa5..eda2eeb 100644
|
||
--- a/src/gd_io_dp.c
|
||
+++ b/src/gd_io_dp.c
|
||
@@ -263,6 +263,7 @@ static void dynamicPutchar(struct gdIOCtx *ctx, int a)
|
||
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;
|
||
@@ -272,21 +273,25 @@ static int dynamicGetbuf(gdIOCtxPtr ctx, void *buf, int len)
|
||
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;
|
||
|
||
diff --git a/src/gd_tiff.c b/src/gd_tiff.c
|
||
index b4f1e63..3f20c5b 100644
|
||
--- a/src/gd_tiff.c
|
||
+++ b/src/gd_tiff.c
|
||
@@ -759,6 +759,7 @@ static int createFromTiffRgba(TIFF * tif, gdImagePtr im)
|
||
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
|
||
@@ -771,18 +772,20 @@ static int createFromTiffRgba(TIFF * tif, gdImagePtr im)
|
||
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);
|
||
+ }
|
||
}
|
||
}
|
||
|
||
@@ -790,7 +793,7 @@ static int createFromTiffRgba(TIFF * tif, gdImagePtr im)
|
||
|
||
/* now reset colour merge for alpha blending routines */
|
||
gdImageAlphaBlending(im, alphaBlendingFlag);
|
||
- return GD_SUCCESS;
|
||
+ return success;
|
||
}
|
||
|
||
/*
|
||
diff --git a/tests/tiff/CMakeLists.txt b/tests/tiff/CMakeLists.txt
|
||
index 03f630c..81f2303 100644
|
||
--- a/tests/tiff/CMakeLists.txt
|
||
+++ b/tests/tiff/CMakeLists.txt
|
||
@@ -1,6 +1,7 @@
|
||
IF(TIFF_FOUND)
|
||
LIST(APPEND TESTS_FILES
|
||
tiff_im2im
|
||
+ tiff_invalid_read
|
||
tiff_null
|
||
tiff_dpi
|
||
)
|
||
diff --git a/tests/tiff/tiff_invalid_read.c b/tests/tiff/tiff_invalid_read.c
|
||
new file mode 100644
|
||
index 0000000..bed5389
|
||
--- /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 0000000..e0bfa7b
|
||
--- /dev/null
|
||
+++ b/tests/tiff/tiff_invalid_read_1.tiff
|
||
@@ -0,0 +1,3 @@
|
||
+II* |