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