tag 413039 + patch
thanks
On Thu, Mar 01, 2007 at 09:01:48PM +0100, Daniel Kobras wrote:
> sun:
> broken.sun ... Segmentation fault
Simply out-of-bounds read access due to insufficient validation of input
file. No severe security implications, patch attached. Hopefully I've
covered all cases - this code is horribly convoluted even for *magick
standards.
Daniel.
--- a/coders/sun.c Thu Mar 08 21:13:15 2007 +0100
+++ b/coders/sun.c Thu Mar 08 22:51:13 2007 +0100
@@ -427,62 +427,73 @@ static Image *ReadSUNImage(const ImageIn
}
else
if (image->storage_class == PseudoClass)
- for (y=0; y < (long) image->rows; y++)
- {
- q=SetImagePixels(image,0,y,image->columns,1);
- if (q == (PixelPacket *) NULL)
- break;
- indexes=GetIndexes(image);
- for (x=0; x < (long) image->columns; x++)
- indexes[x]=(*p++);
- if ((image->columns % 2) != 0)
- p++;
- if (!SyncImagePixels(image))
- break;
- if (image->previous == (Image *) NULL)
- if (QuantumTick(y,image->rows))
- if (!MagickMonitor(LoadImageText,y,image->rows,exception))
- break;
- }
+ {
+ unsigned long n = image->rows*(image->columns+image->columns%2);
+ if (n < bytes_per_line*image->rows)
+ ThrowReaderException(CorruptImageError,ImproperImageHeader,image);
+ for (y=0; y < (long) image->rows; y++)
+ {
+ q=SetImagePixels(image,0,y,image->columns,1);
+ if (q == (PixelPacket *) NULL)
+ break;
+ indexes=GetIndexes(image);
+ for (x=0; x < (long) image->columns; x++)
+ indexes[x]=(*p++);
+ if ((image->columns % 2) != 0)
+ p++;
+ if (!SyncImagePixels(image))
+ break;
+ if (image->previous == (Image *) NULL)
+ if (QuantumTick(y,image->rows))
+ if (!MagickMonitor(LoadImageText,y,image->rows,exception))
+ break;
+ }
+ }
else
- for (y=0; y < (long) image->rows; y++)
- {
- q=SetImagePixels(image,0,y,image->columns,1);
- if (q == (PixelPacket *) NULL)
- break;
- for (x=0; x < (long) image->columns; x++)
+ {
+ unsigned long n = image->rows*(image->columns+image->columns%2);
+ n *= (image->matte) ? 4 : 3;
+ if (n < bytes_per_line*image->rows)
+ ThrowReaderException(CorruptImageError,ImproperImageHeader,image);
+ for (y=0; y < (long) image->rows; y++)
{
- if (image->matte)
- q->opacity=(Quantum) (MaxRGB-ScaleCharToQuantum(*p++));
- if (sun_info.type == RT_STANDARD)
- {
- q->blue=ScaleCharToQuantum(*p++);
- q->green=ScaleCharToQuantum(*p++);
- q->red=ScaleCharToQuantum(*p++);
- }
- else
- {
- q->red=ScaleCharToQuantum(*p++);
- q->green=ScaleCharToQuantum(*p++);
- q->blue=ScaleCharToQuantum(*p++);
- }
- if (image->colors != 0)
- {
- q->red=image->colormap[q->red].red;
- q->green=image->colormap[q->green].green;
- q->blue=image->colormap[q->blue].blue;
- }
- q++;
+ q=SetImagePixels(image,0,y,image->columns,1);
+ if (q == (PixelPacket *) NULL)
+ break;
+ for (x=0; x < (long) image->columns; x++)
+ {
+ if (image->matte)
+ q->opacity=(Quantum) (MaxRGB-ScaleCharToQuantum(*p++));
+ if (sun_info.type == RT_STANDARD)
+ {
+ q->blue=ScaleCharToQuantum(*p++);
+ q->green=ScaleCharToQuantum(*p++);
+ q->red=ScaleCharToQuantum(*p++);
+ }
+ else
+ {
+ q->red=ScaleCharToQuantum(*p++);
+ q->green=ScaleCharToQuantum(*p++);
+ q->blue=ScaleCharToQuantum(*p++);
+ }
+ if (image->colors != 0)
+ {
+ q->red=image->colormap[q->red].red;
+ q->green=image->colormap[q->green].green;
+ q->blue=image->colormap[q->blue].blue;
+ }
+ q++;
+ }
+ if (((image->columns % 2) != 0) && (image->matte == False))
+ p++;
+ if (!SyncImagePixels(image))
+ break;
+ if (image->previous == (Image *) NULL)
+ if (QuantumTick(y,image->rows))
+ if (!MagickMonitor(LoadImageText,y,image->rows,exception))
+ break;
}
- if (((image->columns % 2) != 0) && (image->matte == False))
- p++;
- if (!SyncImagePixels(image))
- break;
- if (image->previous == (Image *) NULL)
- if (QuantumTick(y,image->rows))
- if (!MagickMonitor(LoadImageText,y,image->rows,exception))
- break;
- }
+ }
if (image->storage_class == PseudoClass)
SyncImage(image);
MagickFreeMemory(sun_pixels);
