libpng extra row (CVE-2010-1205)

This bug is a fairly straightforward one. I am interested in it for two main reasons. The first, and smaller reason, is that it's an interesting bug in that two parties are at fault if an application is vulnerable: the makers of the application and libpng itself. Each had to do something wrong in order for this to be an actual problem. The second, larger reason why I am so interested in this bug is because I had fuzzed into this bug back in April and sat on it too long. What happened next? Lo and behold, I wasn't the only one looking at pngs -- someone else reported it (first public reference: Google Chrome Issue, see also the alert on the libpng home page, and most recently, a Mozilla Security Advisory). Props to the guy who reported it (Aki Helin). With that in mind, here's a brief description of the vuln: an extra row in the image data is bad [*sarcasm*].

Actually, it's slightly more complicated than that. Libpng uses callbacks to aid with progressive reading of a png. That way, as the image rows become available, the application can handle them. The root problem lies in the fact that libpng versions prior to the fix would call the row_callback() function in the application for _every_ row without checking to see if more rows had been processed than the declared height in the IHDR chunk (which is what memory allocations are [usually] based on). Since libpng didn't check the number of rows processed (or the number of times the row_callback() was called), the last place to catch this bug would be in the application. If the application checked how many times its callback had been called against the height of the png stored in the png_ptr, it could bail when too many rows were received. But, libpng has been around for quite a while (15+ years), so everyone trusts it and never does their own checks, merely copying the inflated image rows into their own buffers, using a calculation based on their base buffer address, image width, row number, and the number of bytes per pixel.

Below is a snippet of Firefox's row_callback function. The statement that copies the image row data to the application's buffer is on line 822:

// Firefox 3.6.3
// modules/libpr0n/decoders/png/nsPNGDecoder.cpp  <-- who named this module??

725 void
726 row_callback(png_structp png_ptr, png_bytep new_row,
727              png_uint_32 row_num, int pass)
728 {
...
762   if (new_row) {
...
791     switch (decoder->format) {
...
819       case gfxASurface::ImageFormatARGB32:
820       {
821         for (PRUint32 x=width; x>0; --x) {
822           *cptr32++ = GFX_PACKED_PIXEL(line[3], line[0], line[1], line[2]);
823           if (line[3] != 0xff)
824             rowHasNoAlpha = PR_FALSE;
825           line += 4;
826         }
827       }
828       break;
...
833     }
...
851   }
852 }

Below is a snippet of Google Chrome's row_callback. The destination address in the code below is calculated on line 356, and the statement that copies the image row data to the destination is on line 360:

// Google Chrome, r44471
// gfx/codec/png_codec.cc

338 void DecodeRowCallback(png_struct* png_ptr, png_byte* new_row,
339                        png_uint_32 row_num, int pass) {
340   PngDecoderState* state = static_cast(
341       png_get_progressive_ptr(png_ptr));
342 
343   DCHECK(pass == 0) << "We didn't turn on interlace handling, but libpng is "
344                        "giving us interlaced data.";
345   if (static_cast(row_num) > state->height) {
346     NOTREACHED() << "Invalid row";
347     return;
348   }
349 
350   unsigned char* base = NULL;
351   if (state->bitmap)
352     base = reinterpret_cast(state->bitmap->getAddr32(0, 0));
353   else if (state->output)
354     base = &state->output->front();
355 
356   unsigned char* dest = &base[state->width * state->output_channels * row_num];
357   if (state->row_converter)
358     state->row_converter(new_row, state->width, dest, &state->is_opaque);
359   else
360     memcpy(dest, new_row, state->width * state->output_channels);
361 }

Well, enough of the rant-ish speaking. Below is how the libpng code worked before the fix:

First, the function png_process_data() is called by the application:

  // pngpread.c

  30 void PNGAPI
  31 png_process_data(png_structp png_ptr, png_infop info_ptr,
  32    png_bytep buffer, png_size_t buffer_size)
  33 {
  34    if (png_ptr == NULL || info_ptr == NULL)
  35       return;
  36 
  37    png_push_restore_buffer(png_ptr, buffer, buffer_size);
  38 
  39    while (png_ptr->buffer_size)
  40    {
  41       png_process_some_data(png_ptr, info_ptr);
  42    }
  43 }

This ends up calling png_process_some_data(), which iterates through and processes each chunk. Once the IDAT chunk (the chunk where the image data [and extra row] are stored) has been read and handled (via line 62), it is then in the PNG_READ_IDAT_MODE. At this point, it then drops down into the case at line 68, calling png_push_read_IDAT():

  // pngpread.c

  45 /* What we do with the incoming data depends on what we were previously
  46  * doing before we ran out of data...
  47  */
  48 void /* PRIVATE */
  49 png_process_some_data(png_structp png_ptr, png_infop info_ptr)
  50 {
  51    if (png_ptr == NULL)
  52       return;
  53 
  54    switch (png_ptr->process_mode)
  55    {
  ...
  62       case PNG_READ_CHUNK_MODE:
  63       {
  64          png_push_read_chunk(png_ptr, info_ptr);
  65          break;
  66       }
  67 
  68       case PNG_READ_IDAT_MODE:
  69       {
  70          png_push_read_IDAT(png_ptr);
  71          break;
  72       }
  ...
 109    }
 110 }

The png_push_read_IDAT() function is a bit longer, so I'm only going to show one relevant part:

 // pngpread.c

 735 void /* PRIVATE */
 736 png_push_read_IDAT(png_structp png_ptr)
 737 {
 738    PNG_IDAT;
 ...
 765    if (png_ptr->idat_size && png_ptr->save_buffer_size)
 766    {
 767       png_size_t save_size;
 768 
 769       if (png_ptr->idat_size < (png_uint_32)png_ptr->save_buffer_size)
 770       {
 771          save_size = (png_size_t)png_ptr->idat_size;
 772 
 773          /* Check for overflow */
 774          if ((png_uint_32)save_size != png_ptr->idat_size)
 775             png_error(png_ptr, "save_size overflowed in pngpread");
 776       }
 777       else
 778          save_size = png_ptr->save_buffer_size;
 779 
 780       png_calculate_crc(png_ptr, png_ptr->save_buffer_ptr, save_size);
 781 
 782       if (!(png_ptr->flags & PNG_FLAG_ZLIB_FINISHED))
 783          png_process_IDAT_data(png_ptr, png_ptr->save_buffer_ptr, save_size);
 784 
 785       png_ptr->idat_size -= save_size;
 786       png_ptr->buffer_size -= save_size;
 787       png_ptr->save_buffer_size -= save_size;
 788       png_ptr->save_buffer_ptr += save_size;
 789    }
 ...
 826 }

Basically, this function gets called multiple times, going down a different code path depending on what state the png_ptr->mode is in. One of the code paths makes a call to png_process_IDAT_data(), which inflates the compressed image data and begins the row processing. Notice how libpng never checks to see if the number of rows processed is greater than the declared height in the IHDR chunk:

 // pngpread.c

 828 void /* PRIVATE */
 829 png_process_IDAT_data(png_structp png_ptr, png_bytep buffer,
 830    png_size_t buffer_length)
 831 {
 832    int ret;
 833 
 834    if ((png_ptr->flags & PNG_FLAG_ZLIB_FINISHED) && buffer_length)
 835       png_benign_error(png_ptr, "Extra compression data");
 836 
 837    png_ptr->zstream.next_in = buffer;
 838    png_ptr->zstream.avail_in = (uInt)buffer_length;
 839    for (;;)
 840    {
 841       ret = inflate(&png_ptr->zstream, Z_PARTIAL_FLUSH);
 842       if (ret != Z_OK)
 843       {
 844          if (ret == Z_STREAM_END)
 845          {
 846             if (png_ptr->zstream.avail_in)
 847                png_benign_error(png_ptr, "Extra compressed data");
 848 
 849             if (!(png_ptr->zstream.avail_out))
 850             {
 851                png_push_process_row(png_ptr);
 852             }
 853 
 854             png_ptr->mode |= PNG_AFTER_IDAT;
 855             png_ptr->flags |= PNG_FLAG_ZLIB_FINISHED;
 856             break;
 857          }
 858          else if (ret == Z_BUF_ERROR)
 859             break;
 860 
 861          else
 862             png_error(png_ptr, "Decompression Error");
 863       }
 864       if (!(png_ptr->zstream.avail_out))
 865       {
 866          if ((
 867 #ifdef PNG_READ_INTERLACING_SUPPORTED
 868              png_ptr->interlaced && png_ptr->pass > 6) ||
 869              (!png_ptr->interlaced &&
 870 #endif
 871              png_ptr->row_number == png_ptr->num_rows))
 872          {
 873            if (png_ptr->zstream.avail_in)
 874              png_warning(png_ptr, "Too much data in IDAT chunks");
 875            png_ptr->flags |= PNG_FLAG_ZLIB_FINISHED;
 876            break;
 877          }
 878          png_push_process_row(png_ptr);
 879          png_ptr->zstream.avail_out =
 880              (uInt) PNG_ROWBYTES(png_ptr->pixel_depth,
 881              png_ptr->iwidth) + 1;
 882          png_ptr->zstream.next_out = png_ptr->row_buf;
 883       }
 884 
 885       else
 886          break;
 887    }
 888 }

As soon as a complete row is available, it calls the png_push_process_row() function (I'm leaving out the interlaced code for simplicity):

 // pngpread.c

 890 void /* PRIVATE */
 891 png_push_process_row(png_structp png_ptr)
 892 {
 893    png_ptr->row_info.color_type = png_ptr->color_type;
 894    png_ptr->row_info.width = png_ptr->iwidth;
 895    png_ptr->row_info.channels = png_ptr->channels;
 896    png_ptr->row_info.bit_depth = png_ptr->bit_depth;
 897    png_ptr->row_info.pixel_depth = png_ptr->pixel_depth;
 898 
 899    png_ptr->row_info.rowbytes = PNG_ROWBYTES(png_ptr->row_info.pixel_depth,
 900        png_ptr->row_info.width);
 901 
 902    png_read_filter_row(png_ptr, &(png_ptr->row_info),
 903       png_ptr->row_buf + 1, png_ptr->prev_row + 1,
 904       (int)(png_ptr->row_buf[0]));
 905 
 906    png_memcpy(png_ptr->prev_row, png_ptr->row_buf, png_ptr->rowbytes + 1);
 907 
 908    if (png_ptr->transformations || (png_ptr->flags&PNG_FLAG_STRIP_ALPHA))
 909       png_do_read_transformations(png_ptr);
 910 
 911 #ifdef PNG_READ_INTERLACING_SUPPORTED
 ...
1088 #endif
1089    {
1090       png_push_have_row(png_ptr, png_ptr->row_buf + 1);
1091       png_read_push_finish_row(png_ptr);
1092    }
1093 }

Notice the two calls on line 1090 and 1091. The first call, png_push_have_row(), calls the row_callback function defined in the application:

// pngpread.c

1682 void /* PRIVATE */
1683 png_push_have_row(png_structp png_ptr, png_bytep row)
1684 {
1685    if (png_ptr->row_fn != NULL)
1686       (*(png_ptr->row_fn))(png_ptr, row, png_ptr->row_number,
1687          (int)png_ptr->pass);
1688 }

The second call, png_read_push_finish_row(), merely increments png_ptr->row_number (it does more post-processing for interlaced images as well):

// pngpread.c

1095 void /* PRIVATE */
1096 png_read_push_finish_row(png_structp png_ptr)
1097 {
...
1117    png_ptr->row_number++;
...
1157 }

So what is the error? The number of actual rows in the image data is never checked against the rows declared in the IHDR chunk. When libpng fixed this bug, they rewrote most of the png_process_IDAT_data() function. The critical fix they added was this:

 // pngpread.c, libpng-1.4.3

 827 void /* PRIVATE */
 828 png_process_IDAT_data(png_structp png_ptr, png_bytep buffer,
 829    png_size_t buffer_length)
 830 {
 ...
 845    while (png_ptr->zstream.avail_in > 0 &&
 846       !(png_ptr->flags & PNG_FLAG_ZLIB_FINISHED))
 847    {
 ...
 891       /* Did inflate output any data? */
 892       if (png_ptr->zstream.next_out != png_ptr->row_buf)
 893       {
 894      /* Is this unexpected data after the last row?
 895       * If it is, artificially terminate the LZ output
 896       * here.
 897       */
 898          if (png_ptr->row_number >= png_ptr->num_rows ||
 899          png_ptr->pass > 6)
 900          {
 901         /* Extra data. */
 902         png_warning(png_ptr, "Extra compressed data in IDAT");
 903             png_ptr->flags |= PNG_FLAG_ZLIB_FINISHED;
 904         /* Do no more processing; skip the unprocessed
 905          * input check below.
 906          */
 907             return;
 908      }
 ...
 918    }
 ...
 926 }

Notice how it has the explicit check against the row number and the height of the image on line 898. Libpng now only calls the row_callback function at a maximum height times.

I think this is a prime example about trusting third-party code. Like a firearm safety instructor told me once about using the safety, "we use them, but we don't trust them". I think that is how third-party code should be used.