php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Sec Bug #66815 imagecrop(): insufficient fix for NULL defer CVE-2013-7327
Submitted: 2014-03-03 15:16 UTC Modified: 2014-03-06 13:10 UTC
From: thoger at redhat dot com Assigned: remi (profile)
Status: Closed Package: GD related
PHP Version: 5.5.9 OS:
Private report: No CVE-ID: None
Welcome back! If you're the original bug submitter, here's where you can edit the bug or add additional notes.
If this is not your bug, you can add a comment by following this link.
If this is your bug, but you forgot your password, you can retrieve your password here.
Password:
Status:
Package:
Bug Type:
Summary:
From: thoger at redhat dot com
New email:
PHP Version: OS:

 

 [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.


Patches

test.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)

Add a Patch

Pull Requests

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2014-03-03 15:22 UTC] thoger at redhat dot com
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 */
 [2014-03-03 15:42 UTC] johannes@php.net
-Status: Open +Status: Assigned -Assigned To: +Assigned To: pajoye
 [2014-03-03 16:06 UTC] remi@php.net
Notice, dropping the /* check size */ raise an additional warning (instead of a silent fail), but this is more consistent when the system libgd is used.
 [2014-03-03 16:10 UTC] remi@php.net
The following patch has been added/updated:

Patch Name: tests.patch
Revision:   1393863058
URL:        https://bugs.php.net/patch-display.php?bug=66815&patch=tests.patch&revision=1393863058
 [2014-03-03 16:13 UTC] remi@php.net
The following patch has been added/updated:

Patch Name: libgd-test.patch
Revision:   1393863190
URL:        https://bugs.php.net/patch-display.php?bug=66815&patch=libgd-test.patch&revision=1393863190
 [2014-03-03 16:14 UTC] remi@php.net
libgd-test.patch includes
- thoger fix
- fix for new warning
- test for new segfault case

Notice buld using system libgd are not affected (no segfault).
 [2014-03-03 16:19 UTC] remi@php.net
The following patch has been added/updated:

Patch Name: 0001-Fix-NULL-deref-in-gdImageCrop.patch
Revision:   1393863561
URL:        https://bugs.php.net/patch-display.php?bug=66815&patch=0001-Fix-NULL-deref-in-gdImageCrop.patch&revision=1393863561
 [2014-03-03 16:20 UTC] remi@php.net
The following patch has been added/updated:

Patch Name: test.patch
Revision:   1393863613
URL:        https://bugs.php.net/patch-display.php?bug=66815&patch=test.patch&revision=1393863613
 [2014-03-03 16:22 UTC] remi@php.net
-Assigned To: pajoye +Assigned To: remi
 [2014-03-05 10:15 UTC] remi@php.net
-Status: Assigned +Status: Closed
 [2014-03-10 11:04 UTC] ab@php.net
Automatic comment on behalf of remi
Revision: http://git.php.net/?p=php-src.git;a=commit;h=af09d8b96a8aacdd7d738fec81b695c1c58368f7
Log: Fixed Bug #66815 imagecrop(): insufficient fix for NULL defer CVE-2013-7327
 [2014-03-10 11:29 UTC] ab@php.net
Automatic comment on behalf of remi
Revision: http://git.php.net/?p=php-src.git;a=commit;h=af09d8b96a8aacdd7d738fec81b695c1c58368f7
Log: Fixed Bug #66815 imagecrop(): insufficient fix for NULL defer CVE-2013-7327
 [2014-04-10 04:47 UTC] tyrael@php.net
Automatic comment on behalf of remi
Revision: http://git.php.net/?p=php-src.git;a=commit;h=af09d8b96a8aacdd7d738fec81b695c1c58368f7
Log: Fixed Bug #66815 imagecrop(): insufficient fix for NULL defer CVE-2013-7327
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Sat Apr 20 04:01:28 2024 UTC