CESA-2004-001 - rev 2

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

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.

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 2
Chris Evans
chris@scary.beasts.org


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))1