]>
Commit | Line | Data |
---|---|---|
75d12200 AM |
1 | From 4d9d8368d08c3a2be3ea4193b9314fffeddace52 Mon Sep 17 00:00:00 2001 |
2 | From: Ondrej Dubaj <odubaj@redhat.com> | |
3 | Date: Tue, 4 Jun 2019 13:38:41 +0200 | |
4 | Subject: [PATCH] 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 | 19 +++++++++++++++---- | |
20 | src/gd_jpeg.c | 20 ++++++++++++++++---- | |
21 | src/gd_wbmp.c | 21 ++++++++++++++++++--- | |
22 | tests/jpeg/CMakeLists.txt | 1 + | |
23 | tests/jpeg/Makemodule.am | 3 ++- | |
24 | tests/jpeg/jpeg_ptr_double_free.c | 31 +++++++++++++++++++++++++++++++ | |
25 | 6 files changed, 83 insertions(+), 12 deletions(-) | |
26 | create mode 100644 tests/jpeg/jpeg_ptr_double_free.c | |
27 | ||
28 | diff --git a/src/gd_gif_out.c b/src/gd_gif_out.c | |
29 | index 6fe707d..4a05c09 100755 | |
30 | --- a/src/gd_gif_out.c | |
31 | +++ b/src/gd_gif_out.c | |
32 | @@ -99,7 +99,7 @@ static void char_init(GifCtx *ctx); | |
33 | static void char_out(int c, GifCtx *ctx); | |
34 | static void flush_char(GifCtx *ctx); | |
35 | ||
36 | - | |
37 | +static int _gdImageGifCtx(gdImagePtr im, gdIOCtxPtr out); | |
38 | ||
39 | ||
40 | /* | |
41 | @@ -131,8 +131,11 @@ BGD_DECLARE(void *) gdImageGifPtr(gdImagePtr im, int *size) | |
42 | void *rv; | |
43 | gdIOCtx *out = gdNewDynamicCtx(2048, NULL); | |
44 | if (out == NULL) return NULL; | |
45 | - gdImageGifCtx(im, out); | |
46 | - rv = gdDPExtractData(out, size); | |
47 | + if (!_gdImageGifCtx(im, out)) { | |
48 | + rv = gdDPExtractData(out, size); | |
49 | + } else { | |
50 | + rv = NULL; | |
51 | + } | |
52 | out->gd_free(out); | |
53 | return rv; | |
54 | } | |
55 | @@ -220,6 +223,12 @@ BGD_DECLARE(void) gdImageGif(gdImagePtr im, FILE *outFile) | |
56 | ||
57 | */ | |
58 | BGD_DECLARE(void) gdImageGifCtx(gdImagePtr im, gdIOCtxPtr out) | |
59 | +{ | |
60 | + _gdImageGifCtx(im, out); | |
61 | +} | |
62 | + | |
63 | +/* returns 0 on success, 1 on failure */ | |
64 | +static int _gdImageGifCtx(gdImagePtr im, gdIOCtxPtr out) | |
65 | { | |
66 | gdImagePtr pim = 0, tim = im; | |
67 | int interlace, BitsPerPixel; | |
68 | @@ -231,7 +240,7 @@ BGD_DECLARE(void) gdImageGifCtx(gdImagePtr im, gdIOCtxPtr out) | |
69 | based temporary image. */ | |
70 | pim = gdImageCreatePaletteFromTrueColor(im, 1, 256); | |
71 | if(!pim) { | |
72 | - return; | |
73 | + return 1; | |
74 | } | |
75 | tim = pim; | |
76 | } | |
77 | @@ -247,6 +256,8 @@ BGD_DECLARE(void) gdImageGifCtx(gdImagePtr im, gdIOCtxPtr out) | |
78 | /* Destroy palette based temporary image. */ | |
79 | gdImageDestroy( pim); | |
80 | } | |
81 | + | |
82 | + return 0; | |
83 | } | |
84 | ||
85 | ||
86 | diff --git a/src/gd_jpeg.c b/src/gd_jpeg.c | |
87 | index 271ef46..bd8fc27 100755 | |
88 | --- a/src/gd_jpeg.c | |
89 | +++ b/src/gd_jpeg.c | |
90 | @@ -123,6 +123,8 @@ static void fatal_jpeg_error(j_common_ptr cinfo) | |
91 | exit(99); | |
92 | } | |
93 | ||
94 | +static int _gdImageJpegCtx(gdImagePtr im, gdIOCtx *outfile, int quality); | |
95 | + | |
96 | /* | |
97 | * Write IM to OUTFILE as a JFIF-formatted JPEG image, using quality | |
98 | * QUALITY. If QUALITY is in the range 0-100, increasing values | |
99 | @@ -237,8 +239,11 @@ BGD_DECLARE(void *) gdImageJpegPtr(gdImagePtr im, int *size, int quality) | |
100 | void *rv; | |
101 | gdIOCtx *out = gdNewDynamicCtx(2048, NULL); | |
102 | if (out == NULL) return NULL; | |
103 | - gdImageJpegCtx(im, out, quality); | |
104 | - rv = gdDPExtractData(out, size); | |
105 | + if (!_gdImageJpegCtx(im, out, quality)) { | |
106 | + rv = gdDPExtractData(out, size); | |
107 | + } else { | |
108 | + rv = NULL; | |
109 | + } | |
110 | out->gd_free(out); | |
111 | return rv; | |
112 | } | |
113 | @@ -259,6 +264,12 @@ void jpeg_gdIOCtx_dest(j_compress_ptr cinfo, gdIOCtx *outfile); | |
114 | ||
115 | */ | |
116 | BGD_DECLARE(void) gdImageJpegCtx(gdImagePtr im, gdIOCtx *outfile, int quality) | |
117 | +{ | |
118 | + _gdImageJpegCtx(im, outfile, quality); | |
119 | +} | |
120 | + | |
121 | +/* returns 0 on success, 1 on failure */ | |
122 | +static int _gdImageJpegCtx(gdImagePtr im, gdIOCtx *outfile, int quality) | |
123 | { | |
124 | struct jpeg_compress_struct cinfo; | |
125 | struct jpeg_error_mgr jerr; | |
126 | @@ -293,7 +304,7 @@ BGD_DECLARE(void) gdImageJpegCtx(gdImagePtr im, gdIOCtx *outfile, int quality) | |
127 | if(row) { | |
128 | gdFree(row); | |
129 | } | |
130 | - return; | |
131 | + return 1; | |
132 | } | |
133 | ||
134 | cinfo.err->emit_message = jpeg_emit_message; | |
135 | @@ -334,7 +345,7 @@ BGD_DECLARE(void) gdImageJpegCtx(gdImagePtr im, gdIOCtx *outfile, int quality) | |
136 | if(row == 0) { | |
137 | gd_error("gd-jpeg: error: unable to allocate JPEG row structure: gdCalloc returns NULL\n"); | |
138 | jpeg_destroy_compress(&cinfo); | |
139 | - return; | |
140 | + return 1; | |
141 | } | |
142 | ||
143 | rowptr[0] = row; | |
144 | @@ -411,6 +422,7 @@ BGD_DECLARE(void) gdImageJpegCtx(gdImagePtr im, gdIOCtx *outfile, int quality) | |
145 | jpeg_finish_compress(&cinfo); | |
146 | jpeg_destroy_compress(&cinfo); | |
147 | gdFree(row); | |
148 | + return 0; | |
149 | } | |
150 | ||
151 | ||
152 | diff --git a/src/gd_wbmp.c b/src/gd_wbmp.c | |
153 | index 0028273..341ff6e 100755 | |
154 | --- a/src/gd_wbmp.c | |
155 | +++ b/src/gd_wbmp.c | |
156 | @@ -88,6 +88,8 @@ int gd_getin(void *in) | |
157 | return (gdGetC((gdIOCtx *)in)); | |
158 | } | |
159 | ||
160 | +static int _gdImageWBMPCtx(gdImagePtr image, int fg, gdIOCtx *out); | |
161 | + | |
162 | /* | |
163 | Function: gdImageWBMPCtx | |
164 | ||
165 | @@ -100,6 +102,12 @@ int gd_getin(void *in) | |
166 | out - the stream where to write | |
167 | */ | |
168 | BGD_DECLARE(void) gdImageWBMPCtx(gdImagePtr image, int fg, gdIOCtx *out) | |
169 | +{ | |
170 | + _gdImageWBMPCtx(image, fg, out); | |
171 | +} | |
172 | + | |
173 | +/* returns 0 on success, 1 on failure */ | |
174 | +static int _gdImageWBMPCtx(gdImagePtr image, int fg, gdIOCtx *out) | |
175 | { | |
176 | int x, y, pos; | |
177 | Wbmp *wbmp; | |
178 | @@ -107,7 +115,7 @@ BGD_DECLARE(void) gdImageWBMPCtx(gdImagePtr image, int fg, gdIOCtx *out) | |
179 | /* create the WBMP */ | |
180 | if((wbmp = createwbmp(gdImageSX(image), gdImageSY(image), WBMP_WHITE)) == NULL) { | |
181 | gd_error("Could not create WBMP\n"); | |
182 | - return; | |
183 | + return 1; | |
184 | } | |
185 | ||
186 | /* fill up the WBMP structure */ | |
187 | @@ -123,11 +131,15 @@ BGD_DECLARE(void) gdImageWBMPCtx(gdImagePtr image, int fg, gdIOCtx *out) | |
188 | ||
189 | /* write the WBMP to a gd file descriptor */ | |
190 | if(writewbmp(wbmp, &gd_putout, out)) { | |
191 | + freewbmp(wbmp); | |
192 | gd_error("Could not save WBMP\n"); | |
193 | + return 1; | |
194 | } | |
195 | ||
196 | /* des submitted this bugfix: gdFree the memory. */ | |
197 | freewbmp(wbmp); | |
198 | + | |
199 | + return 0; | |
200 | } | |
201 | ||
202 | /* | |
203 | @@ -271,8 +283,11 @@ BGD_DECLARE(void *) gdImageWBMPPtr(gdImagePtr im, int *size, int fg) | |
204 | void *rv; | |
205 | gdIOCtx *out = gdNewDynamicCtx(2048, NULL); | |
206 | if (out == NULL) return NULL; | |
207 | - gdImageWBMPCtx(im, fg, out); | |
208 | - rv = gdDPExtractData(out, size); | |
209 | + if (!_gdImageWBMPCtx(im, fg, out)) { | |
210 | + rv = gdDPExtractData(out, size); | |
211 | + } else { | |
212 | + rv = NULL; | |
213 | + } | |
214 | out->gd_free(out); | |
215 | return rv; | |
216 | } | |
217 | diff --git a/tests/jpeg/CMakeLists.txt b/tests/jpeg/CMakeLists.txt | |
218 | index 19964b0..a8d8162 100755 | |
219 | --- a/tests/jpeg/CMakeLists.txt | |
220 | +++ b/tests/jpeg/CMakeLists.txt | |
221 | @@ -2,6 +2,7 @@ IF(JPEG_FOUND) | |
222 | LIST(APPEND TESTS_FILES | |
223 | jpeg_empty_file | |
224 | jpeg_im2im | |
225 | + jpeg_ptr_double_free | |
226 | jpeg_null | |
227 | ) | |
228 | ||
229 | diff --git a/tests/jpeg/Makemodule.am b/tests/jpeg/Makemodule.am | |
230 | index 7e5d317..b89e169 100755 | |
231 | --- a/tests/jpeg/Makemodule.am | |
232 | +++ b/tests/jpeg/Makemodule.am | |
233 | @@ -2,7 +2,8 @@ if HAVE_LIBJPEG | |
234 | libgd_test_programs += \ | |
235 | jpeg/jpeg_empty_file \ | |
236 | jpeg/jpeg_im2im \ | |
237 | - jpeg/jpeg_null | |
238 | + jpeg/jpeg_null \ | |
239 | + jpeg/jpeg_ptr_double_free | |
240 | ||
241 | if HAVE_LIBPNG | |
242 | libgd_test_programs += \ | |
243 | diff --git a/tests/jpeg/jpeg_ptr_double_free.c b/tests/jpeg/jpeg_ptr_double_free.c | |
244 | new file mode 100644 | |
245 | index 0000000..c80aeb6 | |
246 | --- /dev/null | |
247 | +++ b/tests/jpeg/jpeg_ptr_double_free.c | |
248 | @@ -0,0 +1,31 @@ | |
249 | +/** | |
250 | + * Test that failure to convert to JPEG returns NULL | |
251 | + * | |
252 | + * We are creating an image, set its width to zero, and pass this image to | |
253 | + * `gdImageJpegPtr()` which is supposed to fail, and as such should return NULL. | |
254 | + * | |
255 | + * See also <https://github.com/libgd/libgd/issues/381> | |
256 | + */ | |
257 | + | |
258 | + | |
259 | +#include "gd.h" | |
260 | +#include "gdtest.h" | |
261 | + | |
262 | + | |
263 | +int main() | |
264 | +{ | |
265 | + gdImagePtr src, dst; | |
266 | + int size; | |
267 | + | |
268 | + src = gdImageCreateTrueColor(1, 10); | |
269 | + gdTestAssert(src != NULL); | |
270 | + | |
271 | + src->sx = 0; /* this hack forces gdImageJpegPtr() to fail */ | |
272 | + | |
273 | + dst = gdImageJpegPtr(src, &size, 0); | |
274 | + gdTestAssert(dst == NULL); | |
275 | + | |
276 | + gdImageDestroy(src); | |
277 | + | |
278 | + return gdNumFailures(); | |
279 | +} | |
280 | \ No newline at end of file | |
281 | -- | |
282 | 2.17.1 | |
283 |