Bug 524217 (CVE-2009-3293) - CVE-2009-3293 php: gd - improper upper bound check in imagecolortransparent
Summary: CVE-2009-3293 php: gd - improper upper bound check in imagecolortransparent
Alias: CVE-2009-3293
Product: Security Response
Classification: Other
Component: vulnerability
Version: unspecified
Hardware: All
OS: Linux
Target Milestone: ---
Assignee: Red Hat Product Security
QA Contact:
URL: http://www.php.net/ChangeLog-5.php
Depends On:
TreeView+ depends on / blocked
Reported: 2009-09-18 12:34 UTC by Jan Lieskovsky
Modified: 2019-09-29 12:32 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Last Closed: 2009-11-25 16:32:41 UTC

Attachments (Terms of Use)
Local copy of upstream PHP-5.2.11 imagecolortransparent patch (963 bytes, patch)
2009-09-18 12:36 UTC, Jan Lieskovsky
no flags Details | Diff

Description Jan Lieskovsky 2009-09-18 12:34:12 UTC
An improper check for upper bound value of color index, when handling
transparent images, was found in php. A remote attacker could provide
a specially-crafted image, leading to php application, using the gd
extension for loading of images, crash, once processed by the application.


Upstream patch:

Comment 1 Jan Lieskovsky 2009-09-18 12:36:12 UTC
Created attachment 361641 [details]
Local copy of upstream PHP-5.2.11 imagecolortransparent patch

Comment 2 Jan Lieskovsky 2009-09-18 12:36:47 UTC
This issue affects the versions of php package, as shipped with Red Hat
Enterprise Linux 3, 4, and 5.

Comment 3 Jan Lieskovsky 2009-09-22 08:57:46 UTC
MITRE's CVE-2009-3293 record:

Unspecified vulnerability in the imagecolortransparent function in PHP
before 5.2.11 has unknown impact and attack vectors related to an
incorrect "sanity check for the color index."


Comment 4 Tomas Hoger 2009-09-28 10:21:17 UTC
Let's have a look at the patch:

-	if (color > -1 && color<im->colorsTotal && color<=gdMaxColors) {
+	if (color > -1 && color < im->colorsTotal && color < gdMaxColors) {

Relevant part is change of color<=gdMaxColors check to color<gdMaxColors, the rest is just a white-space change.  color is later used as index to im->alpha[] array, possibly allowing buffer over-write.  gdMaxColors and alpha[] are defined as:

  #define gdMaxColors 256


  typedef struct gdImageStruct {
    int alpha[gdMaxColors];

This can possibly be an off-by-one write if color == gdMaxColors.  Let's have a look at other conditions in that if statement, particularly color < im->colorsTotal.

The code in question is only reached for non-TrueColor images:

  void gdImageColorTransparent (gdImagePtr im, int color)
    if (!im->trueColor) {
        if (color > -1 && color < im->colorsTotal && color < gdMaxColors) {
            im->alpha[color] = gdAlphaTransparent;
        } else {

colorsTotal is counter for number of colors already allocated in the image using imagecolorallocate() function (or gdImageColorAllocateAlpha() gd function) for non-TrueColor images.  However, that function already performs checking of colorsTotal against gdMaxColors as:

    if (ct == (-1)) {
        ct = im->colorsTotal;
        if (ct == gdMaxColors) {
            return -1;

So colorsTotal can be gdMaxColors at max and hence:

  color < im->colorsTotal <= gdMaxColors


  color < gdMaxColors

So the change does not make any real difference and hence is not a security fix for recent PHP versions.

Looking back at some older versions (no longer supported upstream), the "color < im-> colorsTotal" check was added in 4.3.5 (for Red Hat Enterprise Linux, this means, it's only missing in php 4.3.2 in RHEL3).  It's possible to trigger the overflow in those old versions, overwriting neighbour member of the gdImageStruct structure - trueColor.  So it's possible to "upgrade" image from non-TrueColor to TrueColor this way.  As TrueColor and non-TrueColor images use different amount of memory to store one pixel, any subsequent operations on the image are likely to cause pixels array overflow.

However, I don't quite see any obvious use case where imagecolortransparent() would be called with color using a malicious value, apart from malicious script author, which does not cross trust boundary.  PHP manual states that color passed to the function should be a value returned by imagecolorallocate(), which only returns values lower than 256 for non-TrueColor images.

Comment 5 Tomas Hoger 2009-09-28 13:36:39 UTC
Sadly, colorsTotal may come from other source when using imagecreatefrom*() functions.  It should be properly validated when read from file, but it does not seem to happen in all cases:

- png version uses libpng, which does enforce palette size limit of 256 in png_handle_PLTE()

- gif version initializes colorsTotal to gdMaxColors and checks how much of the palette was really initialized and decrements colorsTotal

- gd loading does not check colorsTotal in the gd2xFlag != 0 case (see _gdGetColors()) when not sanitized 16 bit value is read to colorsTotal causing buffer overwrites in im->open[] and possibly on couple of other places (when copying palettes from image to image)

gd formats, however, are not mean for general purpose use, which reduces impact of this flaw:

Comment 7 Tomas Hoger 2009-10-15 14:53:31 UTC
Making comment #5 public now, input sanitizing problem in _gdGetColors() is fixed in upstream SVN now and tracked via separate bug #529213.

Comment 8 Tomas Hoger 2009-11-25 16:32:41 UTC
As explained above, this issue does not affect recent PHP versions.

Additionally, off-by-one write can only happen when passing untrusted values as the $color argument to the imagecolortransparent() function.  Documentation of the function states that the argument to that function should be a value returned by imagecolorallocate() (or possibly other functions returning valid color identifier, such as imagecolorat() or imagecolorclosest()).

This can, of course, be triggered by a script author, but that does not cross trust boundary.  Therefore, we don't plan to fix this problem in Red Hat Enterprise Linux 3.

_gdGetColors() overflow caused by a specially crafted gd file mentioned in comment #7 will be addressed.

Note You need to log in before you can comment on or make changes to this bug.