Fixed Bug #66815 imagecrop(): insufficient fix for NULL defer CVE-2013-7327

This amends commit 8f4a537, which aimed to correct NULL dereference because of
missing check of gdImageCreateTrueColor() / gdImageCreate() return value.  That
commit checks for negative crop rectangle width and height, but
gdImageCreate*() can also return NULL when width * height overflows.  Hence
NULL deref is still possible, as gdImageSaveAlpha() and gdImagePaletteCopy()
is called before dst == NULL check.

This moves NULL check to happen right after gdImageCreate*().  It also removes
width and height check before gdImageCreate*(), as the same check is done by
image create functions (with an extra warning).

From thoger redhat com

diff --git a/ext/gd/libgd/gd_crop.c b/ext/gd/libgd/gd_crop.c
index bba425d0e3fccd30aac03da36da0e86ac7770dba..84edb5d1f7f8fd2ac6104e93c700853e15b3b058 100644 (file)
--- 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 */

diff --git a/ext/gd/tests/bug66356.phpt b/ext/gd/tests/bug66356.phpt
index 2da91d61a9f3e2abe2b6c3973db5c5077f7b89fe..583d74942c8cc21123f33f29d5048ccb5b483230 100644 (file)
--- a/ext/gd/tests/bug66356.phpt
+++ b/ext/gd/tests/bug66356.phpt
@@ -24,6 +24,8 @@ var_dump(imagecrop($img, array("x" => -20, "y" => -20, "width" => 10, "height" =
 // POC #4
 var_dump(imagecrop($img, array("x" => 0x7fffff00, "y" => 0, "width" => 10, "height" => 10)));
 
+// bug 66815
+var_dump(imagecrop($img, array("x" => 0, "y" => 0, "width" => 65535, "height" => 65535)));
 ?>
 --EXPECTF--
 resource(%d) of type (gd)
@@ -35,6 +37,13 @@ Array
     [width] => 10
     [height] => 10
 )
+
+Warning: imagecrop(): gd warning: one parameter to a memory allocation multiplication is negative or zero, failing operation gracefully
+ in %sbug66356.php on line %d
 bool(false)
 resource(%d) of type (gd)
-resource(%d) of type (gd)
\ No newline at end of file
+resource(%d) of type (gd)
+
+Warning: imagecrop(): gd warning: product of memory allocation multiplication would exceed INT_MAX, failing operation gracefully
+ in %sbug66356.php on line %d
+bool(false)
\ No newline at end of file