Fixed bug 4628 - SEGV_UNKNOW in function SDL_free_REAL at SDL_malloc.c:5372-5
authorSam Lantinga <slouken@libsdl.org>
Mon, 10 Jun 2019 15:41:09 -0700
changeset 646e7e9786a1a34
parent 645 93332afa1831
child 647 94e5942b92af
Fixed bug 4628 - SEGV_UNKNOW in function SDL_free_REAL at SDL_malloc.c:5372-5

Hugo Lefeuvre

The PCX format specifies pcxh.BytesPerLine, which represents the size of a
single plane's scanline in bytes. Valid PCX images should have
pcxh.BytesPerLine >= surface->pitch.

pcxh.BytesPerLine and surface->pitch can legitimately be different because
pcxh.BytesPerLine is padded to be a multiple of machine word length (where
file was created).

If src_bits == 8 we directly read a whole scanline from src to row. This is
a problem in the case where bpl > surface->pitch because row is too small.

This allows attacker to perform unlimited OOB write on the heap.

+ remove pointless check bpl > surface->pitch, this is a valid situation
+ make sure we always read into buf which is big enough
+ in the case where src_bits == 8: copy these bytes back to row afterwar
IMG_pcx.c
     1.1 --- a/IMG_pcx.c	Sun May 19 23:17:53 2019 +0200
     1.2 +++ b/IMG_pcx.c	Mon Jun 10 15:41:09 2019 -0700
     1.3 @@ -156,22 +156,22 @@
     1.4      }
     1.5      surface = SDL_CreateRGBSurface(SDL_SWSURFACE, width, height,
     1.6                     bits, Rmask, Gmask, Bmask, Amask);
     1.7 -    if ( surface == NULL )
     1.8 +    if ( surface == NULL ) {
     1.9          goto done;
    1.10 +    }
    1.11  
    1.12      bpl = pcxh.NPlanes * pcxh.BytesPerLine;
    1.13 -    if ( bpl < 0 || bpl > surface->pitch ) {
    1.14 -        error = "bytes per line is too large (corrupt?)";
    1.15 +    buf = (Uint8 *)SDL_calloc(bpl, 1);
    1.16 +    if ( !buf ) {
    1.17 +        error = "Out of memory";
    1.18          goto done;
    1.19      }
    1.20 -    buf = (Uint8 *)SDL_calloc(surface->pitch, 1);
    1.21      row = (Uint8 *)surface->pixels;
    1.22      for ( y=0; y<surface->h; ++y ) {
    1.23          /* decode a scan line to a temporary buffer first */
    1.24          int i;
    1.25 -        Uint8 *dst = buf;
    1.26          if ( pcxh.Encoding == 0 ) {
    1.27 -            if ( !SDL_RWread(src, dst, bpl, 1) ) {
    1.28 +            if ( !SDL_RWread(src, buf, bpl, 1) ) {
    1.29                  error = "file truncated";
    1.30                  goto done;
    1.31              }
    1.32 @@ -192,7 +192,7 @@
    1.33                          }
    1.34                      }
    1.35                  }
    1.36 -                dst[i] = ch;
    1.37 +                buf[i] = ch;
    1.38                  count--;
    1.39              }
    1.40          }
    1.41 @@ -214,13 +214,21 @@
    1.42                      }
    1.43                  }
    1.44              }
    1.45 +        } else if ( src_bits == 8 ) {
    1.46 +            /* directly copy buf content to row */
    1.47 +            Uint8 *innerSrc = buf;
    1.48 +            int x;
    1.49 +            Uint8 *dst = row;
    1.50 +            for ( x = 0; x < width; x++ ) {
    1.51 +                *dst++ = *innerSrc++;
    1.52 +            }
    1.53          } else if ( src_bits == 24 ) {
    1.54              /* de-interlace planes */
    1.55              Uint8 *innerSrc = buf;
    1.56              int plane;
    1.57              for ( plane = 0; plane < pcxh.NPlanes; plane++ ) {
    1.58                  int x;
    1.59 -                dst = row + plane;
    1.60 +                Uint8 *dst = row + plane;
    1.61                  for ( x = 0; x < width; x++ ) {
    1.62                      if ( dst >= row+surface->pitch ) {
    1.63                          error = "decoding out of bounds (corrupt?)";
    1.64 @@ -230,8 +238,6 @@
    1.65                      dst += pcxh.NPlanes;
    1.66                  }
    1.67              }
    1.68 -        } else {
    1.69 -            SDL_memcpy(row, buf, bpl);
    1.70          }
    1.71  
    1.72          row += surface->pitch;