From d98e5a2ace46adcefa093c029663575fd677bf05 Mon Sep 17 00:00:00 2001 From: Ondrej Dubaj Date: Tue, 4 Jun 2019 13:05:57 +0200 Subject: [PATCH] Potential double-free in gdImage*Ptr() Whenever `gdImage*Ptr()` calls `gdImage*Ctx()` and the latter fails, we must not call `gdDPExtractData()`; otherwise a double-free would happen. Since `gdImage*Ctx()` are void functions, and we can't change that for BC reasons, we're introducing static helpers which are used internally. We're adding a regression test for `gdImageJpegPtr()`, but not for `gdImageGifPtr()` and `gdImageWbmpPtr()` since we don't know how to trigger failure of the respective `gdImage*Ctx()` calls. This potential security issue has been reported by Solmaz Salimi (aka. Rooney). --- src/gd_gif_out.c | 19 +++++++++++++++---- src/gd_jpeg.c | 20 ++++++++++++++++---- src/gd_wbmp.c | 21 ++++++++++++++++++--- tests/jpeg/CMakeLists.txt | 1 + tests/jpeg/Makemodule.am | 3 ++- tests/jpeg/jpeg_ptr_double_free.c | 31 +++++++++++++++++++++++++++++++ 6 files changed, 83 insertions(+), 12 deletions(-) create mode 100644 tests/jpeg/jpeg_ptr_double_free.c diff --git a/src/gd_gif_out.c b/src/gd_gif_out.c index 6fe707d..4a05c09 100755 --- a/src/gd_gif_out.c +++ b/src/gd_gif_out.c @@ -99,7 +99,7 @@ static void char_init(GifCtx *ctx); static void char_out(int c, GifCtx *ctx); static void flush_char(GifCtx *ctx); - +static int _gdImageGifCtx(gdImagePtr im, gdIOCtxPtr out); /* @@ -131,8 +131,11 @@ BGD_DECLARE(void *) gdImageGifPtr(gdImagePtr im, int *size) void *rv; gdIOCtx *out = gdNewDynamicCtx(2048, NULL); if (out == NULL) return NULL; - gdImageGifCtx(im, out); - rv = gdDPExtractData(out, size); + if (!_gdImageGifCtx(im, out)) { + rv = gdDPExtractData(out, size); + } else { + rv = NULL; + } out->gd_free(out); return rv; } @@ -220,6 +223,12 @@ BGD_DECLARE(void) gdImageGif(gdImagePtr im, FILE *outFile) */ BGD_DECLARE(void) gdImageGifCtx(gdImagePtr im, gdIOCtxPtr out) +{ + _gdImageGifCtx(im, out); +} + +/* returns 0 on success, 1 on failure */ +static int _gdImageGifCtx(gdImagePtr im, gdIOCtxPtr out) { gdImagePtr pim = 0, tim = im; int interlace, BitsPerPixel; @@ -231,7 +240,7 @@ BGD_DECLARE(void) gdImageGifCtx(gdImagePtr im, gdIOCtxPtr out) based temporary image. */ pim = gdImageCreatePaletteFromTrueColor(im, 1, 256); if(!pim) { - return; + return 1; } tim = pim; } @@ -247,6 +256,8 @@ BGD_DECLARE(void) gdImageGifCtx(gdImagePtr im, gdIOCtxPtr out) /* Destroy palette based temporary image. */ gdImageDestroy( pim); } + + return 0; } diff --git a/src/gd_jpeg.c b/src/gd_jpeg.c index 271ef46..bd8fc27 100755 --- a/src/gd_jpeg.c +++ b/src/gd_jpeg.c @@ -123,6 +123,8 @@ static void fatal_jpeg_error(j_common_ptr cinfo) exit(99); } +static int _gdImageJpegCtx(gdImagePtr im, gdIOCtx *outfile, int quality); + /* * Write IM to OUTFILE as a JFIF-formatted JPEG image, using quality * QUALITY. If QUALITY is in the range 0-100, increasing values @@ -237,8 +239,11 @@ BGD_DECLARE(void *) gdImageJpegPtr(gdImagePtr im, int *size, int quality) void *rv; gdIOCtx *out = gdNewDynamicCtx(2048, NULL); if (out == NULL) return NULL; - gdImageJpegCtx(im, out, quality); - rv = gdDPExtractData(out, size); + if (!_gdImageJpegCtx(im, out, quality)) { + rv = gdDPExtractData(out, size); + } else { + rv = NULL; + } out->gd_free(out); return rv; } @@ -259,6 +264,12 @@ void jpeg_gdIOCtx_dest(j_compress_ptr cinfo, gdIOCtx *outfile); */ BGD_DECLARE(void) gdImageJpegCtx(gdImagePtr im, gdIOCtx *outfile, int quality) +{ + _gdImageJpegCtx(im, outfile, quality); +} + +/* returns 0 on success, 1 on failure */ +static int _gdImageJpegCtx(gdImagePtr im, gdIOCtx *outfile, int quality) { struct jpeg_compress_struct cinfo; struct jpeg_error_mgr jerr; @@ -293,7 +304,7 @@ BGD_DECLARE(void) gdImageJpegCtx(gdImagePtr im, gdIOCtx *outfile, int quality) if(row) { gdFree(row); } - return; + return 1; } cinfo.err->emit_message = jpeg_emit_message; @@ -334,7 +345,7 @@ BGD_DECLARE(void) gdImageJpegCtx(gdImagePtr im, gdIOCtx *outfile, int quality) if(row == 0) { gd_error("gd-jpeg: error: unable to allocate JPEG row structure: gdCalloc returns NULL\n"); jpeg_destroy_compress(&cinfo); - return; + return 1; } rowptr[0] = row; @@ -411,6 +422,7 @@ BGD_DECLARE(void) gdImageJpegCtx(gdImagePtr im, gdIOCtx *outfile, int quality) jpeg_finish_compress(&cinfo); jpeg_destroy_compress(&cinfo); gdFree(row); + return 0; } diff --git a/src/gd_wbmp.c b/src/gd_wbmp.c index 0028273..341ff6e 100755 --- a/src/gd_wbmp.c +++ b/src/gd_wbmp.c @@ -88,6 +88,8 @@ int gd_getin(void *in) return (gdGetC((gdIOCtx *)in)); } +static int _gdImageWBMPCtx(gdImagePtr image, int fg, gdIOCtx *out); + /* Function: gdImageWBMPCtx @@ -100,6 +102,12 @@ int gd_getin(void *in) out - the stream where to write */ BGD_DECLARE(void) gdImageWBMPCtx(gdImagePtr image, int fg, gdIOCtx *out) +{ + _gdImageWBMPCtx(image, fg, out); +} + +/* returns 0 on success, 1 on failure */ +static int _gdImageWBMPCtx(gdImagePtr image, int fg, gdIOCtx *out) { int x, y, pos; Wbmp *wbmp; @@ -107,7 +115,7 @@ BGD_DECLARE(void) gdImageWBMPCtx(gdImagePtr image, int fg, gdIOCtx *out) /* create the WBMP */ if((wbmp = createwbmp(gdImageSX(image), gdImageSY(image), WBMP_WHITE)) == NULL) { gd_error("Could not create WBMP\n"); - return; + return 1; } /* fill up the WBMP structure */ @@ -123,11 +131,15 @@ BGD_DECLARE(void) gdImageWBMPCtx(gdImagePtr image, int fg, gdIOCtx *out) /* write the WBMP to a gd file descriptor */ if(writewbmp(wbmp, &gd_putout, out)) { + freewbmp(wbmp); gd_error("Could not save WBMP\n"); + return 1; } /* des submitted this bugfix: gdFree the memory. */ freewbmp(wbmp); + + return 0; } /* @@ -271,8 +283,11 @@ BGD_DECLARE(void *) gdImageWBMPPtr(gdImagePtr im, int *size, int fg) void *rv; gdIOCtx *out = gdNewDynamicCtx(2048, NULL); if (out == NULL) return NULL; - gdImageWBMPCtx(im, fg, out); - rv = gdDPExtractData(out, size); + if (!_gdImageWBMPCtx(im, fg, out)) { + rv = gdDPExtractData(out, size); + } else { + rv = NULL; + } out->gd_free(out); return rv; } diff --git a/tests/jpeg/CMakeLists.txt b/tests/jpeg/CMakeLists.txt index 19964b0..a8d8162 100755 --- a/tests/jpeg/CMakeLists.txt +++ b/tests/jpeg/CMakeLists.txt @@ -2,6 +2,7 @@ IF(JPEG_FOUND) LIST(APPEND TESTS_FILES jpeg_empty_file jpeg_im2im + jpeg_ptr_double_free jpeg_null ) diff --git a/tests/jpeg/Makemodule.am b/tests/jpeg/Makemodule.am index 7e5d317..b89e169 100755 --- a/tests/jpeg/Makemodule.am +++ b/tests/jpeg/Makemodule.am @@ -2,7 +2,8 @@ if HAVE_LIBJPEG libgd_test_programs += \ jpeg/jpeg_empty_file \ jpeg/jpeg_im2im \ - jpeg/jpeg_null + jpeg/jpeg_null \ + jpeg/jpeg_ptr_double_free if HAVE_LIBPNG libgd_test_programs += \ diff --git a/tests/jpeg/jpeg_ptr_double_free.c b/tests/jpeg/jpeg_ptr_double_free.c new file mode 100644 index 0000000..c80aeb6 --- /dev/null +++ b/tests/jpeg/jpeg_ptr_double_free.c @@ -0,0 +1,31 @@ +/** + * Test that failure to convert to JPEG returns NULL + * + * We are creating an image, set its width to zero, and pass this image to + * `gdImageJpegPtr()` which is supposed to fail, and as such should return NULL. + * + * See also + */ + + +#include "gd.h" +#include "gdtest.h" + + +int main() +{ + gdImagePtr src, dst; + int size; + + src = gdImageCreateTrueColor(1, 10); + gdTestAssert(src != NULL); + + src->sx = 0; /* this hack forces gdImageJpegPtr() to fail */ + + dst = gdImageJpegPtr(src, &size, 0); + gdTestAssert(dst == NULL); + + gdImageDestroy(src); + + return gdNumFailures(); +} \ No newline at end of file -- 2.17.1