CESA-2004-001 - rev 3

libPNG 1.2.5 stack-based buffer overflow and other code concerns
================================================================

Programs         : libpng users including mozilla, konqueror, various e-mail
                   clients, generally lots. Also reports that some versions of
                   IE are vulnerable to some of the problems.
Severity         : - A malicious website serving a malicious PNG file could
                     compromise the browsers of visitors.
                   - A malicious PNG could be sent via e-mail and compromise
                     the e-mail viewer of the recipient.
                   - For systems with user-providable images for "face
                     browsers", a local system compromise could be possible via
                     a malicious PNG.
CAN identifier(s): CAN-2004-0597 (the serious one), CAN-2004-0598,
                   CAN-2004-0599
CERT VU#s        : VU#388984 (the serious one), VU#236656, VU#160448,
                   VU#477512, VU#817368, VU#286464

This advisory lists code flaws discovered by inspection of the libpng-1.2.5
code. Only the first one has been examined in practice to confirm
exploitability. The other flaws certainly warrant fixing.

A patch which should plug all these issues is appended beneath the advisory.
NOTE! This patch serves as demo purposes for the flaws only. An official
v1.2.6 libpng with an official, slightly different fix will be released by
the libpng team in parallel with this advisory.

1) Remotely exploitable stack-based buffer overrun in png_handle_tRNS
(pngrutil.c)

If a PNG file is of the correct format, a length check on PNG data is missed
prior to filling a buffer on the stack from the PNG data. The exact flaw would
seem to be a logic error; failure to bail out of a function after a warning
condition is hit, here:

      if (!(png_ptr->mode & PNG_HAVE_PLTE))
      {
         /* Should be an error, but we can cope with it */
         png_warning(png_ptr, "Missing PLTE before tRNS");
      }
      else if (length > (png_uint_32)png_ptr->num_palette)
      {
         png_warning(png_ptr, "Incorrect tRNS chunk length");
         png_crc_finish(png_ptr, length);
         return;
      }

We can see, if the first warning condition is hit, the length check is missed
due to the use of an "else if".

A PNG crafted to trip this is available at
http://scary.beasts.org/misc/pngtest_bad.png

It crashes both mozilla and konqueror.
A scarier possibility is targetted exploitation by e-mailing a nasty PNG to
someone who uses a graphical e-mail client to decode PNGs with a vulnerable
libpng.


2) Dangerous code in png_handle_sBIT (pngrutil.c) (Similar code in
png_handle_hIST).

Although seemingly not exploitable, there is dangerous code in this function.
It relies on checks scattered elsewhere in the code in order to not overflow
a 4-byte stack buffer. This line here should upper-bound the read onto the
stack to 4 bytes:

   png_crc_read(png_ptr, buf, truelen);


3) Possible NULL-pointer crash in png_handle_iCCP (pngrutil.c) (this flaw is
duplicated in multiple other locations).

There are lots of lines such as these in the code:

   chunkdata = (png_charp)png_malloc(png_ptr, length + 1);

Where "length" comes from the PNG. If length is set to UINT_MAX then length + 1 will equate to zero, leading to the PNG malloc routines to return NULL and
subsequent access to crash. These lengths are sometimes checked to ensure
they are smaller that INT_MAX, but it is not clear that all code paths perform
this check, i.e. png_push_read_chunk in pngpread.c does not do this check
(this is progressive reading mode as used by browsers).


4) Theoretical integer overflow in allocation in png_handle_sPLT (pngrutil.c)

This isn't likely to cause problems in practice, but there's the possibility
of an integer overflow during this allocation:

   new_palette.entries = (png_sPLT_entryp)png_malloc(
       png_ptr, new_palette.nentries * sizeof(png_sPLT_entry));


5) Integer overflow in png_read_png (pngread.c)

A PNG with excessive height may cause an integer overflow on a memory
allocation and subsequent crash allocating row pointers. This line is possibly
faulty; I can't see anywhere that enforces a maximum PNG height:

      info_ptr->row_pointers = (png_bytepp)png_malloc(png_ptr,
         info_ptr->height * sizeof(png_bytep));


6) Integer overflows during progressive reading.

There are many lines like the following, which are prone to integer overflow:

      if (png_ptr->push_length + 4 > png_ptr->buffer_size)

It is not clear how dangerous this is.


7) Other flaws.

There is broad potential for other integer overflows which I have not spotted -
the amount of integer arithmetic surrounding buffer handling is large,
unfortunately.


CESA-2004-001 - rev 3
Chris Evans
chris@scary.beasts.org

[Advertisement: I am interested in moving into a security related field
 full-time. E-mail me to discuss.]


diff -ru libpng-1.2.5/png.h libpng-1.2.5.fix/png.h
--- libpng-1.2.5/png.h  2002-10-03 12:32:26.000000000 +0100
+++ libpng-1.2.5.fix/png.h  2004-07-13 23:18:10.000000000 +0100
@@ -835,6 +835,9 @@
 /* Maximum positive integer used in PNG is (2^31)-1 */
 #define PNG_MAX_UINT ((png_uint_32)0x7fffffffL)
 
+/* Constraints on width, height, (2 ^ 24) - 1*/
+#define PNG_MAX_DIMENSION 16777215
+
 /* These describe the color_type field in png_info. */
 /* color type masks */
 #define PNG_COLOR_MASK_PALETTE    1
diff -ru libpng-1.2.5/pngpread.c libpng-1.2.5.fix/pngpread.c
--- libpng-1.2.5/pngpread.c  2002-10-03 12:32:28.000000000 +0100
+++ libpng-1.2.5.fix/pngpread.c  2004-07-13 23:03:58.000000000 +0100
@@ -209,6 +209,8 @@
 
       png_push_fill_buffer(png_ptr, chunk_length, 4);
       png_ptr->push_length = png_get_uint_32(chunk_length);
+      if (png_ptr->push_length > PNG_MAX_UINT)
+         png_error(png_ptr, "Invalid chunk length.");
       png_reset_crc(png_ptr);
       png_crc_read(png_ptr, png_ptr->chunk_name, 4);
       png_ptr->mode |= PNG_HAVE_CHUNK_HEADER;
@@ -638,6 +640,8 @@
 
       png_push_fill_buffer(png_ptr, chunk_length, 4);
       png_ptr->push_length = png_get_uint_32(chunk_length);
+      if (png_ptr->push_length > PNG_MAX_UINT)
+         png_error(png_ptr, "Invalid chunk length.");
 
       png_reset_crc(png_ptr);
       png_crc_read(png_ptr, png_ptr->chunk_name, 4);
diff -ru libpng-1.2.5/pngrutil.c libpng-1.2.5.fix/pngrutil.c
--- libpng-1.2.5/pngrutil.c  2004-07-13 13:36:37.000000000 +0100
+++ libpng-1.2.5.fix/pngrutil.c  2004-07-13 23:43:02.000000000 +0100
@@ -350,7 +350,11 @@
    png_crc_finish(png_ptr, 0);
 
    width = png_get_uint_32(buf);
+   if (width > PNG_MAX_DIMENSION)
+      png_error(png_ptr, "Width is too large");
    height = png_get_uint_32(buf + 4);
+   if (height > PNG_MAX_DIMENSION)
+      png_error(png_ptr, "Height is too large");
    bit_depth = buf[8];
    color_type = buf[9];
    compression_type = buf[10];
@@ -675,7 +679,7 @@
    else
       truelen = (png_size_t)png_ptr->channels;
 
-   if (length != truelen)
+   if (length != truelen || length > 4)
    {
       png_warning(png_ptr, "Incorrect sBIT chunk length");
       png_crc_finish(png_ptr, length);
@@ -1244,7 +1248,8 @@
          /* Should be an error, but we can cope with it */
          png_warning(png_ptr, "Missing PLTE before tRNS");
       }
-      else if (length > (png_uint_32)png_ptr->num_palette)
+      if (length > (png_uint_32)png_ptr->num_palette ||
+          length > PNG_MAX_PALETTE_LENGTH)
       {
          png_warning(png_ptr, "Incorrect tRNS chunk length");
          png_crc_finish(png_ptr, length);
@@ -1400,7 +1405,7 @@
 void /* PRIVATE */
 png_handle_hIST(png_structp png_ptr, png_infop info_ptr, png_uint_32 length)
 {
-   int num, i;
+   unsigned int num, i;
    png_uint_16 readbuf[PNG_MAX_PALETTE_LENGTH];
 
    png_debug(1, "in png_handle_hIST\n");
@@ -1426,8 +1431,8 @@
       return;
    }
 
-   num = (int)length / 2 ;
-   if (num != png_ptr->num_palette)
+   num = length / 2 ;
+   if (num != png_ptr->num_palette || num > PNG_MAX_PALETTE_LENGTH)
    {
       png_warning(png_ptr, "Incorrect hIST chunk length");
       png_crc_finish(png_ptr, length);
@@ -2868,6 +2873,9 @@
                png_read_data(png_ptr, chunk_length, 4);
                png_ptr->idat_size = png_get_uint_32(chunk_length);
 
+               if (png_ptr->idat_size > PNG_MAX_UINT)
+                  png_error(png_ptr, "Invalid chunk length.");
+            
                png_reset_crc(png_ptr);
                png_crc_read(png_ptr, png_ptr->chunk_name, 4);
                if (png_memcmp(png_ptr->chunk_name, (png_bytep)png_IDAT, 4))