Created attachment 361641 [details]
Local copy of upstream PHP-5.2.11 imagecolortransparent patch

This issue affects the versions of php package, as shipped with Red Hat
Enterprise Linux 3, 4, and 5.

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

References:
-----------
http://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2009-3293
http://www.php.net/ChangeLog-5.php#5.2.11
http://www.php.net/releases/5_2_11.php
http://www.osvdb.org/58187
http://secunia.com/advisories/36791

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

and

  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 {
            return;
        }

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;
        }
        im->colorsTotal++;
    }

So colorsTotal can be gdMaxColors at max and hence:

  color < im->colorsTotal <= gdMaxColors

implies

  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.

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:
  http://www.libgd.org/GdFileFormats

Making comment #5 public now, input sanitizing problem in _gdGetColors() is fixed in upstream SVN now and tracked via separate bug #529213.

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.