Re: Bug#413040: graphicsmagick: Segfault during conversion from XWD coder.

[Daniel Kobras <kobras@debian.org>]

tag 413040 + patch
clone 413040 -1
clone 413040 -2
reassign -1 libx11 2:1.0.3-5
retitle -1 libX11: Buffer overflow in XGetPixel().
severity -1 critical
tag -1 + patch
tag -1 + security
reassign -2 xlibs 4.3.0.dfsg.1-14sarge3
retitle -2 libX11: Buffer overflow in XGetPixel().
severity -2 critical
tag -2 + patch
tag -2 + security
thanks

On Thu, Mar 01, 2007 at 09:01:48PM +0100, Daniel Kobras wrote:
> On Thu, Mar 01, 2007 at 05:37:39AM +0200, Sami Liedes wrote:
> > The attached files all crash imagemagick (eg. XXXtojpg $filename) on
> > amd64, some with SEGV, some with glibc detected heap corruption. I
> > consider it quite likely that some of these are exploitable, but as
> > I'm not sure, only filing as Severity: normal as to not annoy you :)
> 
> Thanks. I've done a quick screening to investigate which of those affect
> graphicsmagick, and have cloned individual bugs as I'm probably unable
> to deal with all of them in one go. Bug severity might change once I've
> had a closer look at the individual issues. Here's the detailed list for
> current graphicsmagick:
(...)
> Broken conversion
> =================
> 
> The following coders show no problems on "gm identify", but break with
> "gm convert" to jpg and gif.
(...)
> xwd:
>         broken.xwd ... Segmentation fault

While not a problem in imagemagick/graphicsmagick themselves, this turns
out as the most grave bug the testcases have uncovered so far. The
*magick XWD coder initializes an XImage structure from a user-supplied
image in X window dump format, passes it to libX11's XInitImage() for
validation, properly checking its return value. Later on it uses
XGetPixel() to obtain individual pixel values.

The broken.xwd testcase supplied in the original bug report contains
specifies a very large bit_per_pixel value. XInitImage() does not check
this case and validates the image. Calling XGetPixel() with the bogus
XImage structure causes an overflow of the px variable that is allocated
on the stack of function _XGetPixel() in src/ImUtil.c. Similar scenarios
arise for different image variants if the bitmap_unit member of XImage
contains excessively large values. The first attached patch is
completely untested so please check before applying. It adds more sanity
checks to XInitImage() to prevent the described buffer overflows in
XGetPixel(). I haven't considered other code paths, so the patch might
not be comprehensive. The second attached patch extends the
graphicsmagick code by a few more sanity checks to plug the hole even
with the present libX11, but from my reading of XInitImage man page,
libX11 ought to take of this itself.

I've done the analysis on an etch system, identical code is already
present in the affected functions in xfree86 on sarge, though. Looks
like stable requires an update as well.

Regards,

Daniel.

--- src/ImUtil.c.orig	2007-03-08 21:24:13.000000000 +0100
+++ src/ImUtil.c	2007-03-08 21:28:22.000000000 +0100
@@ -385,6 +385,8 @@
     XImage *image;
 {
 	if (image->depth == 0 || image->depth > 32 ||
+	    image->bits_per_pixel > 32 || image->bitmap_unit > 32 ||
+	    image->bits_per_pixel < 0 || image->bitmap_unit < 0 ||
 	    (image->format != XYBitmap &&
 	     image->format != XYPixmap &&
 	     image->format != ZPixmap) ||

--- a/coders/xwd.c	Tue Mar 06 08:34:38 2007 +0100
+++ b/coders/xwd.c	Thu Mar 08 21:13:04 2007 +0100
@@ -239,6 +239,13 @@ static Image *ReadXWDImage(const ImageIn
   ximage->red_mask=header.red_mask;
   ximage->green_mask=header.green_mask;
   ximage->blue_mask=header.blue_mask;
+  /* Why those are signed ints is beyond me. */
+  if (ximage->depth < 0 || ximage->width < 0 || ximage->height < 0 ||
+      ximage->bitmap_pad < 0 || ximage->bytes_per_line < 0)
+    ThrowReaderException(CorruptImageError,ImproperImageHeader,image);
+  /* Guard against buffer overflow in libX11. */
+  if (ximage->bits_per_pixel > 32 || ximage->bitmap_unit > 32)
+    ThrowReaderException(CorruptImageError,ImproperImageHeader,image);
   status=XInitImage(ximage);
   if (status == False)
     ThrowReaderException(CorruptImageError,UnrecognizedXWDHeader,image);