tag 383314 + patch
thanks
On Wed, Aug 16, 2006 at 05:20:01PM +0200, Daniel Kobras wrote:
> On Wed, Aug 16, 2006 at 03:51:15PM +0200, Martin Pitt wrote:
> > http://www.overflow.pl/adv/imsgiheap.txt reported a buffer overflow in
> > the SGI parser (demo exploit linked in the report).
> >
> > This has been assigned CVE-2006-4144, please mention this number in
> > the changelog when you fix this.
> >
> > Ubuntu patch:
> >
> > http://people.ubuntu.com/patches/imagemagick.CVE-2006-4144.diff
>
> This patch looks insufficient. In only deals sanitises user input for
> the run-length encoded format, but the overflow as described on the page
> linked above is present in the non-RLE case as well.
I've now had the time to look at it in detail:
- The heap overflow described in CVE-2006-4144 has been fixed for ages.
It is present in graphicsmagick (forked from imagemagick 5), but not
in any imagemagick 6.x version we ship.
- The heap overflow triggered by the demo exploit to CVE-2006-4144
triggers a different vulnerability that was fixed upstream in 6.2.9.
This is the patch you extracted.
- I've discovered two more heap overflows in sgi.c that apparently have
gone unnoticed so far: ReadSGIImage() implicitly assumes iris_info.depth
to be no greater than four. Anything else will trivially overflow the
irix_pixels array. Also the checks in SGIDecode() for an overflow
condition come too late, and an overflow up to 126 bytes might already
have happened.
Hope this clears up the confusion. Here's a proposed fix for the current
version in unstable and testing. I haven't checked yet, but it's
probably applicable to stable as well. The first three hunks are not yet
contained in Ubuntu's security update, either.
Regards,
Daniel.
--- imagemagick-6.2.4.5.dfsg1.orig/coders/sgi.c
+++ imagemagick-6.2.4.5.dfsg1/coders/sgi.c
@@ -171,13 +171,13 @@
q=pixels;
if (bytes_per_pixel == 2)
{
- for (i=0; i < (long) width; )
+ for ( ; ; )
{
pixel=(unsigned long) (*p++) << 8;
pixel|=(*p++);
count=(ssize_t) (pixel & 0x7f);
i+=count;
- if (count == 0)
+ if (count == 0 || i >= (long) width)
break;
if ((pixel & 0x80) != 0)
for ( ; count != 0; count--)
@@ -200,13 +200,13 @@
}
return;
}
- for (i=0; i < (long) width; )
+ for ( ; ; )
{
pixel=(unsigned long) (*p++);
count=(ssize_t) (pixel & 0x7f);
- if (count == 0)
- break;
i+=count;
+ if (count == 0 || i >= (long) width)
+ break;
if ((pixel & 0x80) != 0)
for ( ; count != 0; count--)
{
@@ -304,6 +304,8 @@
image->columns=iris_info.columns;
image->rows=iris_info.rows;
image->depth=(unsigned long) (iris_info.depth <= 8 ? 8 : QuantumDepth);
+ if (iris_info.depth > 4 || iris_info.depth == 0)
+ ThrowReaderException(CorruptImageError,"ImproperImageHeader");
if (iris_info.depth < 3)
{
image->storage_class=PseudoClass;
@@ -396,7 +398,11 @@
for (i=0; i < (int) (iris_info.rows*iris_info.depth); i++)
offsets[i]=(ssize_t) ReadBlobMSBLong(image);
for (i=0; i < (int) (iris_info.rows*iris_info.depth); i++)
- runlength[i]=ReadBlobMSBLong(image);
+ {
+ runlength[i]=ReadBlobMSBLong(image);
+ if (runlength[i] > 4*iris_info.columns+10)
+ ThrowReaderException(CorruptImageError,"ImproperImageHeader");
+ }
/*
Check data order.
*/
