Fixed CVE-2019-7635 and bug 4498 - Heap-Buffer Overflow in Blit1to4 p…

…ertaining to SDL_blit_1.c

Petr Pisar

The root cause is that the POC BMP file declares 3 colors used and 4 bpp palette, but pixel at line 28 and column 1 (counted from 0) has color number 3. Then when the image loaded into a surface is passed to SDL_DisplayFormat(), in order to convert it to a video format, a used bliting function looks up a color number 3 in a 3-element long color bliting map. (The map obviously has the same number entries as the surface format has colors.)

Proper fix should refuse broken BMP images that have a pixel with a color index higher than declared number of "used" colors. Possibly more advanced fix could try to relocate the out-of-range color index into a vacant index (if such exists).

IMG_bmp.c

@@ -371,6 +371,14 @@ static SDL_Surface *LoadBMP_RW (SDL_RWops *src, int freesrc)
             ExpandBMP = biBitCount;
             biBitCount = 8;
             break;
+        case 2:
+        case 3:
+        case 5:
+        case 6:
+        case 7:
+            SDL_SetError("%d-bpp BMP images are not supported", biBitCount);
+            was_error = SDL_TRUE;
+            goto done;
         default:
             ExpandBMP = 0;
             break;
@@ -505,48 +513,63 @@ static SDL_Surface *LoadBMP_RW (SDL_RWops *src, int freesrc)
         switch (ExpandBMP) {
             case 1:
             case 4: {
-            Uint8 pixel = 0;
-            int   shift = (8-ExpandBMP);
-            for ( i=0; i<surface->w; ++i ) {
-                if ( i%(8/ExpandBMP) == 0 ) {
-                    if ( !SDL_RWread(src, &pixel, 1, 1) ) {
-                        IMG_SetError("Error reading from BMP");
+                Uint8 pixel = 0;
+                int   shift = (8-ExpandBMP);
+                for ( i=0; i<surface->w; ++i ) {
+                    if ( i%(8/ExpandBMP) == 0 ) {
+                        if ( !SDL_RWread(src, &pixel, 1, 1) ) {
+                            IMG_SetError("Error reading from BMP");
+                            was_error = SDL_TRUE;
+                            goto done;
+                        }
+                    }
+                    bits[i] = (pixel >> shift);
+                    if (bits[i] >= biClrUsed) {
+                        IMG_SetError("A BMP image contains a pixel with a color out of the palette");
                         was_error = SDL_TRUE;
                         goto done;
                     }
+                    pixel <<= ExpandBMP;
                 }
-                *(bits+i) = (pixel>>shift);
-                pixel <<= ExpandBMP;
-            } }
+            }
             break;
 
             default:
-            if ( SDL_RWread(src, bits, 1, surface->pitch) != surface->pitch ) {
-                SDL_Error(SDL_EFREAD);
-                was_error = SDL_TRUE;
-                goto done;
-            }
-#if SDL_BYTEORDER == SDL_BIG_ENDIAN
-            /* Byte-swap the pixels if needed. Note that the 24bpp
-               case has already been taken care of above. */
-            switch(biBitCount) {
-                case 15:
-                case 16: {
-                    Uint16 *pix = (Uint16 *)bits;
-                    for(i = 0; i < surface->w; i++)
-                        pix[i] = SDL_Swap16(pix[i]);
-                    break;
+                if ( SDL_RWread(src, bits, 1, surface->pitch) != surface->pitch ) {
+                    SDL_Error(SDL_EFREAD);
+                    was_error = SDL_TRUE;
+                    goto done;
+                }
+                if (biBitCount == 8 && palette && biClrUsed < (1 << biBitCount)) {
+                    for (i = 0; i < surface->w; ++i) {
+                        if (bits[i] >= biClrUsed) {
+                            SDL_SetError("A BMP image contains a pixel with a color out of the palette");
+                            was_error = SDL_TRUE;
+                            goto done;
+                        }
+                    }
                 }
+#if SDL_BYTEORDER == SDL_BIG_ENDIAN
+                /* Byte-swap the pixels if needed. Note that the 24bpp
+                   case has already been taken care of above. */
+                switch(biBitCount) {
+                    case 15:
+                    case 16: {
+                        Uint16 *pix = (Uint16 *)bits;
+                        for(i = 0; i < surface->w; i++)
+                            pix[i] = SDL_Swap16(pix[i]);
+                        break;
+                    }
 
-                case 32: {
-                    Uint32 *pix = (Uint32 *)bits;
-                    for(i = 0; i < surface->w; i++)
-                        pix[i] = SDL_Swap32(pix[i]);
-                    break;
+                    case 32: {
+                        Uint32 *pix = (Uint32 *)bits;
+                        for(i = 0; i < surface->w; i++)
+                            pix[i] = SDL_Swap32(pix[i]);
+                        break;
+                    }
                 }
-            }
 #endif
-            break;
+                break;
         }
         /* Skip padding bytes, ugh */
         if ( pad ) {