|
php.net | support | documentation | report a bug | advanced search | search howto | statistics | random bug | login |
[2014-03-03 15:16 UTC] thoger at redhat dot com
Description: ------------ Bug 66356 describes few issues related to imagecrop(). One of the issues (POC #2) is a NULL deref with close-to-NULL write caused by a missing checks for gdImageCreate*() return values. POC #2 triggers NULL return using negative crop rectangle width or height. Patch addresses it using the following check: 48 /* check size */ 49 if (crop->width<=0 || crop->height<=0) { 50 return NULL; 51 } http://git.php.net/?p=php-src.git;a=commitdiff;h=8f4a5373bb71590352fd934028d6dde5bc18530b This fix is insufficient, as gdImageCreate*() can also return NULL when width * height overflows. Patch also adds return value check: 61 if (dst == NULL) { 62 return NULL; 63 } which only happens after gdImageSaveAlpha() / gdImagePaletteCopy() calls, which dereference dst. NULL deref crash can still be triggered with e.g.: imagecrop($img, array("x" => 0, "y" => 0, "width" => 65535, "height" => 65535)); Attached patch suggests following changes: - do dst == NULL right after gdImageCreate*() calls - remove width / height check before gdImageCreate*() - afaics, it is redundant, as both gdImageCreate() and gdImageCreateTrueColor() call overflow2(sx, sy) as the first thing, rejecting non-positive values One other nitpick concern regarding the existing patch is this code: 65 /* check position in the src image */ 66 if (crop->x < 0 || crop->x>=src->sx || crop->y<0 || crop->y>=src->sy) { 67 return dst; 68 } If crop x or y are outside of the source image, function returns new image with no source image data copied to it. This is inconsistent with what is returned when imagecopy() with negative src_x or src_y is called, and afaics what would happen if PHP used upstream libgd 2.1, where gdImageCrop() is thin wrapper around gdImageCopy(). It may be less surprising to gdImageDestroy(dst) and return NULL. Patchestest.patch (last revision 2014-03-03 16:20 UTC by remi@php.net)0001-Fix-NULL-deref-in-gdImageCrop.patch (last revision 2014-03-03 16:19 UTC by remi@php.net) libgd-test.patch (last revision 2014-03-03 16:13 UTC by remi@php.net) tests.patch (last revision 2014-03-03 16:10 UTC by remi@php.net) Pull RequestsHistoryAllCommentsChangesGit/SVN commits
|
|||||||||||||||||||||||||||
Copyright © 2001-2025 The PHP GroupAll rights reserved. |
Last updated: Wed Oct 29 06:00:01 2025 UTC |
I do not seem to be able to attach patch without having full bug tracker account and only using per-bug password. Hence adding it as comment: diff --git a/ext/gd/libgd/gd_crop.c b/ext/gd/libgd/gd_crop.c index bba425d..84edb5d 100644 --- a/ext/gd/libgd/gd_crop.c +++ b/ext/gd/libgd/gd_crop.c @@ -45,22 +45,20 @@ gdImagePtr gdImageCrop(gdImagePtr src, const gdRectPtr crop) gdImagePtr dst; int y; - /* check size */ - if (crop->width<=0 || crop->height<=0) { - return NULL; - } - /* allocate the requested size (could be only partially filled) */ if (src->trueColor) { dst = gdImageCreateTrueColor(crop->width, crop->height); + if (dst == NULL) { + return NULL; + } gdImageSaveAlpha(dst, 1); } else { dst = gdImageCreate(crop->width, crop->height); + if (dst == NULL) { + return NULL; + } gdImagePaletteCopy(dst, src); } - if (dst == NULL) { - return NULL; - } dst->transparent = src->transparent; /* check position in the src image */