Skip to content

Commit

Permalink
Fix OOB read in SgiRleDecode.c
Browse files Browse the repository at this point in the history
* From Pillow 4.3.0->8.1.0
* CVE-2021-25293
  • Loading branch information
wiredfool authored and radarhere committed Mar 1, 2021
1 parent cbfdde7 commit f891baa
Show file tree
Hide file tree
Showing 9 changed files with 81 additions and 14 deletions.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
Binary file not shown.
7 changes: 7 additions & 0 deletions Tests/test_sgi_crash.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,13 @@
"Tests/images/sgi_crash.bin",
"Tests/images/crash-6b7f2244da6d0ae297ee0754a424213444e92778.sgi",
"Tests/images/ossfuzz-5730089102868480.sgi",
"Tests/images/crash-754d9c7ec485ffb76a90eeaab191ef69a2a3a3cd.sgi",
"Tests/images/crash-465703f71a0f0094873a3e0e82c9f798161171b8.sgi",
"Tests/images/crash-64834657ee604b8797bf99eac6a194c124a9a8ba.sgi",
"Tests/images/crash-abcf1c97b8fe42a6c68f1fb0b978530c98d57ced.sgi",
"Tests/images/crash-b82e64d4f3f76d7465b6af535283029eda211259.sgi",
"Tests/images/crash-c1b2595b8b0b92cc5f38b6635e98e3a119ade807.sgi",
"Tests/images/crash-db8bfa78b19721225425530c5946217720d7df4e.sgi",
],
)
def test_crashes(test_file):
Expand Down
88 changes: 74 additions & 14 deletions src/libImaging/SgiRleDecode.c
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,59 @@ static void read4B(UINT32* dest, UINT8* buf)
*dest = (UINT32)((buf[0] << 24) | (buf[1] << 16) | (buf[2] << 8) | buf[3]);
}

static int expandrow(UINT8* dest, UINT8* src, int n, int z, int xsize)
/*
SgiRleDecoding is done in a single channel row oriented set of RLE chunks.
* The file is arranged as
- SGI Header
- Rle Offset Table
- Rle Length Table
- Scanline Data
* Each RLE atom is c->bpc bytes wide (1 or 2)
* Each RLE Chunk is [specifier atom] [ 1 or n data atoms ]
* Copy Atoms are a byte with the high bit set, and the low 7 are
the number of bytes to copy from the source to the
destination. e.g.
CBBBBBBBB or 0CHLHLHLHLHLHL (B=byte, H/L = Hi low bytes)
* Run atoms do not have the high bit set, and the low 7 bits are
the number of copies of the next atom to copy to the
destination. e.g.:
RB -> BBBBB or RHL -> HLHLHLHLHL
The upshot of this is, there is no way to determine the required
length of the input buffer from reloffset and rlelength without
going through the data at that scan line.
Furthermore, there's no requirement that individual scan lines
pointed to from the rleoffset table are in any sort of order or
used only once, or even disjoint. There's also no requirement that
all of the data in the scan line area of the image file be used
*/
static int expandrow(UINT8* dest, UINT8* src, int n, int z, int xsize, UINT8* end_of_buffer)
{
/*
* n here is the number of rlechunks
* z is the number of channels, for calculating the interleave
* offset to go to RGBA style pixels
* xsize is the row width
* end_of_buffer is the address of the end of the input buffer
*/

UINT8 pixel, count;
int x = 0;

for (;n > 0; n--)
{
if (src > end_of_buffer) {
return -1;
}
pixel = *src++;
if (n == 1 && pixel != 0) {
return n;
Expand All @@ -45,13 +91,19 @@ static int expandrow(UINT8* dest, UINT8* src, int n, int z, int xsize)
}
x += count;
if (pixel & RLE_COPY_FLAG) {
if (src + count > end_of_buffer) {
return -1;
}
while(count--) {
*dest = *src++;
dest += z;
}

}
else {
if (src > end_of_buffer) {
return -1;
}
pixel = *src++;
while (count--) {
*dest = pixel;
Expand All @@ -63,14 +115,16 @@ static int expandrow(UINT8* dest, UINT8* src, int n, int z, int xsize)
return 0;
}

static int expandrow2(UINT8* dest, const UINT8* src, int n, int z, int xsize)
static int expandrow2(UINT8* dest, const UINT8* src, int n, int z, int xsize, UINT8* end_of_buffer)
{
UINT8 pixel, count;

int x = 0;

for (;n > 0; n--)
{
if (src + 1 > end_of_buffer) {
return -1;
}
pixel = src[1];
src+=2;
if (n == 1 && pixel != 0) {
Expand All @@ -85,13 +139,19 @@ static int expandrow2(UINT8* dest, const UINT8* src, int n, int z, int xsize)
}
x += count;
if (pixel & RLE_COPY_FLAG) {
if (src + 2 * count > end_of_buffer) {
return -1;
}
while(count--) {
memcpy(dest, src, 2);
src += 2;
dest += z * 2;
}
}
else {
if (src + 2 > end_of_buffer) {
return -1;
}
while (count--) {
memcpy(dest, src, 2);
dest += z * 2;
Expand Down Expand Up @@ -141,7 +201,10 @@ ImagingSgiRleDecode(Imaging im, ImagingCodecState state,
return -1;
}
_imaging_seek_pyFd(state->fd, SGI_HEADER_SIZE, SEEK_SET);
_imaging_read_pyFd(state->fd, (char*)ptr, c->bufsize);
if (_imaging_read_pyFd(state->fd, (char*)ptr, c->bufsize) != c->bufsize) {
state->errcode = IMAGING_CODEC_UNKNOWN;
return -1;
}


/* decoder initialization */
Expand Down Expand Up @@ -175,28 +238,28 @@ ImagingSgiRleDecode(Imaging im, ImagingCodecState state,
read4B(&c->lengthtab[c->tabindex], &ptr[c->bufindex]);
}

state->count += c->tablen * sizeof(UINT32) * 2;

/* read compressed rows */
for (c->rowno = 0; c->rowno < im->ysize; c->rowno++, state->y += state->ystep)
{
for (c->channo = 0; c->channo < im->bands; c->channo++)
{
c->rleoffset = c->starttab[c->rowno + c->channo * im->ysize];
c->rlelength = c->lengthtab[c->rowno + c->channo * im->ysize];
c->rleoffset -= SGI_HEADER_SIZE;

if (c->rleoffset + c->rlelength > c->bufsize) {
// Check for underflow of rleoffset-SGI_HEADER_SIZE
if (c->rleoffset < SGI_HEADER_SIZE) {
state->errcode = IMAGING_CODEC_OVERRUN;
goto sgi_finish_decode;
}

c->rleoffset -= SGI_HEADER_SIZE;

/* row decompression */
if (c->bpc ==1) {
status = expandrow(&state->buffer[c->channo], &ptr[c->rleoffset], c->rlelength, im->bands, im->xsize);
status = expandrow(&state->buffer[c->channo], &ptr[c->rleoffset], c->rlelength, im->bands, im->xsize, &ptr[c->bufsize-1]);
}
else {
status = expandrow2(&state->buffer[c->channo * 2], &ptr[c->rleoffset], c->rlelength, im->bands, im->xsize);
status = expandrow2(&state->buffer[c->channo * 2], &ptr[c->rleoffset], c->rlelength, im->bands, im->xsize, &ptr[c->bufsize-1]);
}
if (status == -1) {
state->errcode = IMAGING_CODEC_OVERRUN;
Expand All @@ -205,16 +268,13 @@ ImagingSgiRleDecode(Imaging im, ImagingCodecState state,
goto sgi_finish_decode;
}

state->count += c->rlelength;
}

/* store decompressed data in image */
state->shuffle((UINT8*)im->image[state->y], state->buffer, im->xsize);

}

c->bufsize++;

sgi_finish_decode: ;

free(c->starttab);
Expand All @@ -224,5 +284,5 @@ sgi_finish_decode: ;
state->errcode=err;
return -1;
}
return state->count - c->bufsize;
return 0;
}

0 comments on commit f891baa

Please sign in to comment.