Brad Bishop | 4fe7a13 | 2019-10-07 09:34:48 -0400 | [diff] [blame^] | 1 | From 553702980ae89c83f2d6e254d62cf82e204956d0 Mon Sep 17 00:00:00 2001 |
| 2 | From: "Christoph M. Becker" <cmbecker69@gmx.de> |
| 3 | Date: Thu, 17 Jan 2019 11:54:55 +0100 |
| 4 | Subject: [PATCH] Fix #492: Potential double-free in gdImage*Ptr() |
| 5 | |
| 6 | Whenever `gdImage*Ptr()` calls `gdImage*Ctx()` and the latter fails, we |
| 7 | must not call `gdDPExtractData()`; otherwise a double-free would |
| 8 | happen. Since `gdImage*Ctx()` are void functions, and we can't change |
| 9 | that for BC reasons, we're introducing static helpers which are used |
| 10 | internally. |
| 11 | |
| 12 | We're adding a regression test for `gdImageJpegPtr()`, but not for |
| 13 | `gdImageGifPtr()` and `gdImageWbmpPtr()` since we don't know how to |
| 14 | trigger failure of the respective `gdImage*Ctx()` calls. |
| 15 | |
| 16 | This potential security issue has been reported by Solmaz Salimi (aka. |
| 17 | Rooney). |
| 18 | --- |
| 19 | src/gd_gif_out.c | 18 +++++++++++++++--- |
| 20 | src/gd_jpeg.c | 20 ++++++++++++++++---- |
| 21 | src/gd_wbmp.c | 21 ++++++++++++++++++--- |
| 22 | tests/jpeg/.gitignore | 1 + |
| 23 | tests/jpeg/CMakeLists.txt | 1 + |
| 24 | tests/jpeg/Makemodule.am | 3 ++- |
| 25 | tests/jpeg/jpeg_ptr_double_free.c | 31 +++++++++++++++++++++++++++++++ |
| 26 | 7 files changed, 84 insertions(+), 11 deletions(-) |
| 27 | create mode 100644 tests/jpeg/jpeg_ptr_double_free.c |
| 28 | |
| 29 | Upstream-Status: Backport [https://github.com/libgd/libgd/commit/553702980ae89c83f2d6e254d62cf82e204956d0] |
| 30 | CVE: CVE-2019-6978 |
| 31 | |
| 32 | Signed-off-by: Trevor Gamblin <trevor.gamblin@windriver.com> |
| 33 | |
| 34 | |
| 35 | diff --git a/src/gd_gif_out.c b/src/gd_gif_out.c |
| 36 | index 298a581..d5a9534 100644 |
| 37 | --- a/src/gd_gif_out.c |
| 38 | +++ b/src/gd_gif_out.c |
| 39 | @@ -99,6 +99,7 @@ static void char_init(GifCtx *ctx); |
| 40 | static void char_out(int c, GifCtx *ctx); |
| 41 | static void flush_char(GifCtx *ctx); |
| 42 | |
| 43 | +static int _gdImageGifCtx(gdImagePtr im, gdIOCtxPtr out); |
| 44 | |
| 45 | |
| 46 | |
| 47 | @@ -131,8 +132,11 @@ BGD_DECLARE(void *) gdImageGifPtr(gdImagePtr im, int *size) |
| 48 | void *rv; |
| 49 | gdIOCtx *out = gdNewDynamicCtx(2048, NULL); |
| 50 | if (out == NULL) return NULL; |
| 51 | - gdImageGifCtx(im, out); |
| 52 | - rv = gdDPExtractData(out, size); |
| 53 | + if (!_gdImageGifCtx(im, out)) { |
| 54 | + rv = gdDPExtractData(out, size); |
| 55 | + } else { |
| 56 | + rv = NULL; |
| 57 | + } |
| 58 | out->gd_free(out); |
| 59 | return rv; |
| 60 | } |
| 61 | @@ -220,6 +224,12 @@ BGD_DECLARE(void) gdImageGif(gdImagePtr im, FILE *outFile) |
| 62 | |
| 63 | */ |
| 64 | BGD_DECLARE(void) gdImageGifCtx(gdImagePtr im, gdIOCtxPtr out) |
| 65 | +{ |
| 66 | + _gdImageGifCtx(im, out); |
| 67 | +} |
| 68 | + |
| 69 | +/* returns 0 on success, 1 on failure */ |
| 70 | +static int _gdImageGifCtx(gdImagePtr im, gdIOCtxPtr out) |
| 71 | { |
| 72 | gdImagePtr pim = 0, tim = im; |
| 73 | int interlace, BitsPerPixel; |
| 74 | @@ -231,7 +241,7 @@ BGD_DECLARE(void) gdImageGifCtx(gdImagePtr im, gdIOCtxPtr out) |
| 75 | based temporary image. */ |
| 76 | pim = gdImageCreatePaletteFromTrueColor(im, 1, 256); |
| 77 | if(!pim) { |
| 78 | - return; |
| 79 | + return 1; |
| 80 | } |
| 81 | tim = pim; |
| 82 | } |
| 83 | @@ -247,6 +257,8 @@ BGD_DECLARE(void) gdImageGifCtx(gdImagePtr im, gdIOCtxPtr out) |
| 84 | /* Destroy palette based temporary image. */ |
| 85 | gdImageDestroy( pim); |
| 86 | } |
| 87 | + |
| 88 | + return 0; |
| 89 | } |
| 90 | |
| 91 | |
| 92 | diff --git a/src/gd_jpeg.c b/src/gd_jpeg.c |
| 93 | index fc05842..96ef430 100644 |
| 94 | --- a/src/gd_jpeg.c |
| 95 | +++ b/src/gd_jpeg.c |
| 96 | @@ -117,6 +117,8 @@ static void fatal_jpeg_error(j_common_ptr cinfo) |
| 97 | exit(99); |
| 98 | } |
| 99 | |
| 100 | +static int _gdImageJpegCtx(gdImagePtr im, gdIOCtx *outfile, int quality); |
| 101 | + |
| 102 | /* |
| 103 | * Write IM to OUTFILE as a JFIF-formatted JPEG image, using quality |
| 104 | * QUALITY. If QUALITY is in the range 0-100, increasing values |
| 105 | @@ -231,8 +233,11 @@ BGD_DECLARE(void *) gdImageJpegPtr(gdImagePtr im, int *size, int quality) |
| 106 | void *rv; |
| 107 | gdIOCtx *out = gdNewDynamicCtx(2048, NULL); |
| 108 | if (out == NULL) return NULL; |
| 109 | - gdImageJpegCtx(im, out, quality); |
| 110 | - rv = gdDPExtractData(out, size); |
| 111 | + if (!_gdImageJpegCtx(im, out, quality)) { |
| 112 | + rv = gdDPExtractData(out, size); |
| 113 | + } else { |
| 114 | + rv = NULL; |
| 115 | + } |
| 116 | out->gd_free(out); |
| 117 | return rv; |
| 118 | } |
| 119 | @@ -253,6 +258,12 @@ void jpeg_gdIOCtx_dest(j_compress_ptr cinfo, gdIOCtx *outfile); |
| 120 | |
| 121 | */ |
| 122 | BGD_DECLARE(void) gdImageJpegCtx(gdImagePtr im, gdIOCtx *outfile, int quality) |
| 123 | +{ |
| 124 | + _gdImageJpegCtx(im, outfile, quality); |
| 125 | +} |
| 126 | + |
| 127 | +/* returns 0 on success, 1 on failure */ |
| 128 | +static int _gdImageJpegCtx(gdImagePtr im, gdIOCtx *outfile, int quality) |
| 129 | { |
| 130 | struct jpeg_compress_struct cinfo; |
| 131 | struct jpeg_error_mgr jerr; |
| 132 | @@ -287,7 +298,7 @@ BGD_DECLARE(void) gdImageJpegCtx(gdImagePtr im, gdIOCtx *outfile, int quality) |
| 133 | if(row) { |
| 134 | gdFree(row); |
| 135 | } |
| 136 | - return; |
| 137 | + return 1; |
| 138 | } |
| 139 | |
| 140 | cinfo.err->emit_message = jpeg_emit_message; |
| 141 | @@ -328,7 +339,7 @@ BGD_DECLARE(void) gdImageJpegCtx(gdImagePtr im, gdIOCtx *outfile, int quality) |
| 142 | if(row == 0) { |
| 143 | gd_error("gd-jpeg: error: unable to allocate JPEG row structure: gdCalloc returns NULL\n"); |
| 144 | jpeg_destroy_compress(&cinfo); |
| 145 | - return; |
| 146 | + return 1; |
| 147 | } |
| 148 | |
| 149 | rowptr[0] = row; |
| 150 | @@ -405,6 +416,7 @@ BGD_DECLARE(void) gdImageJpegCtx(gdImagePtr im, gdIOCtx *outfile, int quality) |
| 151 | jpeg_finish_compress(&cinfo); |
| 152 | jpeg_destroy_compress(&cinfo); |
| 153 | gdFree(row); |
| 154 | + return 0; |
| 155 | } |
| 156 | |
| 157 | |
| 158 | diff --git a/src/gd_wbmp.c b/src/gd_wbmp.c |
| 159 | index f19a1c9..a49bdbe 100644 |
| 160 | --- a/src/gd_wbmp.c |
| 161 | +++ b/src/gd_wbmp.c |
| 162 | @@ -88,6 +88,8 @@ int gd_getin(void *in) |
| 163 | return (gdGetC((gdIOCtx *)in)); |
| 164 | } |
| 165 | |
| 166 | +static int _gdImageWBMPCtx(gdImagePtr image, int fg, gdIOCtx *out); |
| 167 | + |
| 168 | /* |
| 169 | Function: gdImageWBMPCtx |
| 170 | |
| 171 | @@ -100,6 +102,12 @@ int gd_getin(void *in) |
| 172 | out - the stream where to write |
| 173 | */ |
| 174 | BGD_DECLARE(void) gdImageWBMPCtx(gdImagePtr image, int fg, gdIOCtx *out) |
| 175 | +{ |
| 176 | + _gdImageWBMPCtx(image, fg, out); |
| 177 | +} |
| 178 | + |
| 179 | +/* returns 0 on success, 1 on failure */ |
| 180 | +static int _gdImageWBMPCtx(gdImagePtr image, int fg, gdIOCtx *out) |
| 181 | { |
| 182 | int x, y, pos; |
| 183 | Wbmp *wbmp; |
| 184 | @@ -107,7 +115,7 @@ BGD_DECLARE(void) gdImageWBMPCtx(gdImagePtr image, int fg, gdIOCtx *out) |
| 185 | /* create the WBMP */ |
| 186 | if((wbmp = createwbmp(gdImageSX(image), gdImageSY(image), WBMP_WHITE)) == NULL) { |
| 187 | gd_error("Could not create WBMP\n"); |
| 188 | - return; |
| 189 | + return 1; |
| 190 | } |
| 191 | |
| 192 | /* fill up the WBMP structure */ |
| 193 | @@ -123,11 +131,15 @@ BGD_DECLARE(void) gdImageWBMPCtx(gdImagePtr image, int fg, gdIOCtx *out) |
| 194 | |
| 195 | /* write the WBMP to a gd file descriptor */ |
| 196 | if(writewbmp(wbmp, &gd_putout, out)) { |
| 197 | + freewbmp(wbmp); |
| 198 | gd_error("Could not save WBMP\n"); |
| 199 | + return 1; |
| 200 | } |
| 201 | |
| 202 | /* des submitted this bugfix: gdFree the memory. */ |
| 203 | freewbmp(wbmp); |
| 204 | + |
| 205 | + return 0; |
| 206 | } |
| 207 | |
| 208 | /* |
| 209 | @@ -271,8 +283,11 @@ BGD_DECLARE(void *) gdImageWBMPPtr(gdImagePtr im, int *size, int fg) |
| 210 | void *rv; |
| 211 | gdIOCtx *out = gdNewDynamicCtx(2048, NULL); |
| 212 | if (out == NULL) return NULL; |
| 213 | - gdImageWBMPCtx(im, fg, out); |
| 214 | - rv = gdDPExtractData(out, size); |
| 215 | + if (!_gdImageWBMPCtx(im, fg, out)) { |
| 216 | + rv = gdDPExtractData(out, size); |
| 217 | + } else { |
| 218 | + rv = NULL; |
| 219 | + } |
| 220 | out->gd_free(out); |
| 221 | return rv; |
| 222 | } |
| 223 | diff --git a/tests/jpeg/.gitignore b/tests/jpeg/.gitignore |
| 224 | index c28aa87..13bcf04 100644 |
| 225 | --- a/tests/jpeg/.gitignore |
| 226 | +++ b/tests/jpeg/.gitignore |
| 227 | @@ -3,5 +3,6 @@ |
| 228 | /jpeg_empty_file |
| 229 | /jpeg_im2im |
| 230 | /jpeg_null |
| 231 | +/jpeg_ptr_double_free |
| 232 | /jpeg_read |
| 233 | /jpeg_resolution |
| 234 | diff --git a/tests/jpeg/CMakeLists.txt b/tests/jpeg/CMakeLists.txt |
| 235 | index 19964b0..a8d8162 100644 |
| 236 | --- a/tests/jpeg/CMakeLists.txt |
| 237 | +++ b/tests/jpeg/CMakeLists.txt |
| 238 | @@ -2,6 +2,7 @@ IF(JPEG_FOUND) |
| 239 | LIST(APPEND TESTS_FILES |
| 240 | jpeg_empty_file |
| 241 | jpeg_im2im |
| 242 | + jpeg_ptr_double_free |
| 243 | jpeg_null |
| 244 | ) |
| 245 | |
| 246 | diff --git a/tests/jpeg/Makemodule.am b/tests/jpeg/Makemodule.am |
| 247 | index 7e5d317..b89e169 100644 |
| 248 | --- a/tests/jpeg/Makemodule.am |
| 249 | +++ b/tests/jpeg/Makemodule.am |
| 250 | @@ -2,7 +2,8 @@ if HAVE_LIBJPEG |
| 251 | libgd_test_programs += \ |
| 252 | jpeg/jpeg_empty_file \ |
| 253 | jpeg/jpeg_im2im \ |
| 254 | - jpeg/jpeg_null |
| 255 | + jpeg/jpeg_null \ |
| 256 | + jpeg/jpeg_ptr_double_free |
| 257 | |
| 258 | if HAVE_LIBPNG |
| 259 | libgd_test_programs += \ |
| 260 | diff --git a/tests/jpeg/jpeg_ptr_double_free.c b/tests/jpeg/jpeg_ptr_double_free.c |
| 261 | new file mode 100644 |
| 262 | index 0000000..df5a510 |
| 263 | --- /dev/null |
| 264 | +++ b/tests/jpeg/jpeg_ptr_double_free.c |
| 265 | @@ -0,0 +1,31 @@ |
| 266 | +/** |
| 267 | + * Test that failure to convert to JPEG returns NULL |
| 268 | + * |
| 269 | + * We are creating an image, set its width to zero, and pass this image to |
| 270 | + * `gdImageJpegPtr()` which is supposed to fail, and as such should return NULL. |
| 271 | + * |
| 272 | + * See also <https://github.com/libgd/libgd/issues/381> |
| 273 | + */ |
| 274 | + |
| 275 | + |
| 276 | +#include "gd.h" |
| 277 | +#include "gdtest.h" |
| 278 | + |
| 279 | + |
| 280 | +int main() |
| 281 | +{ |
| 282 | + gdImagePtr src, dst; |
| 283 | + int size; |
| 284 | + |
| 285 | + src = gdImageCreateTrueColor(1, 10); |
| 286 | + gdTestAssert(src != NULL); |
| 287 | + |
| 288 | + src->sx = 0; /* this hack forces gdImageJpegPtr() to fail */ |
| 289 | + |
| 290 | + dst = gdImageJpegPtr(src, &size, 0); |
| 291 | + gdTestAssert(dst == NULL); |
| 292 | + |
| 293 | + gdImageDestroy(src); |
| 294 | + |
| 295 | + return gdNumFailures(); |
| 296 | +} |
| 297 | -- |
| 298 | 2.17.1 |
| 299 | |