xorl %eax, %eax

CVE-2009-3607: Poppler Heap Buffer Overflow

leave a comment »

Poppler is a popular open source PDF reader based on Xpdf code. Currently, all of the releases are vulnerable to the following bug.

static cairo_surface_t *
create_surface_from_thumbnail_data (guchar *data,
				    gint    width,
				    gint    height,
				    gint    rowstride)
{
  guchar *cairo_pixels;
  cairo_surface_t *surface;
  static cairo_user_data_key_t key;
  int j;

  cairo_pixels = (guchar *)g_malloc (4 * width * height);
  surface = cairo_image_surface_create_for_data ((unsigned char *)cairo_pixels,
						 CAIRO_FORMAT_RGB24,
						 width, height, 4 * width);
  cairo_surface_set_user_data (surface, &key,
			       cairo_pixels, (cairo_destroy_func_t)g_free);

  for (j = height; j; j--) {
    guchar *p = data;
    guchar *q = cairo_pixels;
    guchar *end = p + 3 * width;
	  
    while (p < end) {
#if G_BYTE_ORDER == G_LITTLE_ENDIAN
      q[0] = p[2];
      q[1] = p[1];
      q[2] = p[0];
#else	  
      q[1] = p[0];
      q[2] = p[1];
      q[3] = p[2];
#endif
      p += 3;
      q += 4;
    }

    data += rowstride;
    cairo_pixels += 4 * width;
  }

  return surface;
}

The above function was ripped from the latest release which is 0.12.1. It’s not hard to spot that the size calculation in g_malloc() could result in an integer overflow since both ‘width’ and ‘height’ variables are derived from user controlled data. Additionally, pointer ‘surface’ is initialized using cairo_image_surface_create_for_data() which would almost certainly fail if ‘cairo_pixels’ allocation failed in the previous g_malloc(). From its official documentation we can read the following:

The caller owns the surface and should call cairo_surface_destroy() when done with it. 
This function always returns a valid pointer, but it will return a pointer to a "nil" surface 
in the case of an error such as out of memory or an invalid stride value.

But as you can read in create_surface_from_thumbnail_data() there is no check of the return value of that CAIRO library routine. This issue was fixed by applying Carlos Garcia Campos’ patch:

 {
   guchar *cairo_pixels;
+  gint cairo_stride;
   cairo_surface_t *surface;
-  static cairo_user_data_key_t key;
   int j;
 
-  cairo_pixels = (guchar *)g_malloc (4 * width * height);
-  surface = cairo_image_surface_create_for_data ((unsigned char *)cairo_pixels,
-						 CAIRO_FORMAT_RGB24,
-						 width, height, 4 * width);
-  cairo_surface_set_user_data (surface, &key,
-			       cairo_pixels, (cairo_destroy_func_t)g_free);
+  surface = cairo_image_surface_create (CAIRO_FORMAT_RGB24, width, height);
+  if (cairo_surface_status (surface))
+    return NULL;
+
+  cairo_pixels = cairo_image_surface_get_data (surface);
+  cairo_stride = cairo_image_surface_get_stride (surface);
 
   for (j = height; j; j--) {

He removed the ‘key’ static variable and inserted a new integer named ‘cairo_stride’. All of the buggy code was removed and the cairo_image_surface_create() was used to handle the allocation of the image surface. Also, cairo_image_surface_get_data() and cairo_image_surface_get_stride() to obtain a pointer to the data of the passed pointer and retrieve the stride of the image in bytes in order to initialize ‘cairo_stride’. Finally, the following update was changed:

     data += rowstride;
-    cairo_pixels += 4 * width;
+    cairo_pixels += cairo_stride;
   }

Since there is no need to perform the multiplication because it already contains the correct value in bytes.

About these ads

Written by xorl

November 10, 2009 at 00:26

Posted in bugs

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out / Change )

Twitter picture

You are commenting using your Twitter account. Log Out / Change )

Facebook photo

You are commenting using your Facebook account. Log Out / Change )

Google+ photo

You are commenting using your Google+ account. Log Out / Change )

Connecting to %s

Follow

Get every new post delivered to your Inbox.

Join 68 other followers