Heap-Buffer Overflow in Blit1to4 pertaining to SDL_blit_1.c

[SDLBugzilla]

This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

• PoC (poc_vid2.bpp, image/bmp, 2019-02-07 23:08:51 +0000, 578 bytes)
• Fix for 1-, 4-, 8-bpp images (0001-CVE-2019-7635-Reject-BMP-images-with-pixel-colors-ou.patch, text/plain, 2019-02-19 14:08:23 +0000, 2312 bytes)
• Reject 2, 3, 5, 6, 7-bpp BMP images (0001-Reject-2-3-5-6-7-bpp-BMP-images.patch, text/plain, 2019-02-21 10:05:33 +0000, 1159 bytes)

Reported in version: HG 2.1
Reported for operating system, platform: Linux, x86_64

Comments on the original bug report:

On 2019-02-07 23:08:51 +0000, Radue wrote:

Created attachment 3603
PoC

A heap-buffer overflow vulnerability was discovered in SDL-1.2.15 library.

Asan report:

=================================================================
==15035==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x60200006b49c at pc 0x7f2dc3d9a796 bp 0x7ffc133ade60 sp 0x7ffc133ade58
READ of size 4 at 0x60200006b49c thread T0
# 0 0x7f2dc3d9a795 in Blit1to4 /home/radu/apps/sdl_player_lib/SDL-1.2.15/build/../src/video/SDL_blit_1.c:252:3
# 1 0x7f2dc3d879ad in SDL_SoftBlit /home/radu/apps/sdl_player_lib/SDL-1.2.15/build/../src/video/SDL_blit.c:97:3
# 2 0x7f2dc3df50fc in SDL_LowerBlit /home/radu/apps/sdl_player_lib/SDL-1.2.15/build/../src/video/SDL_surface.c:440:9
# 3 0x7f2dc3df50fc in SDL_ConvertSurface /home/radu/apps/sdl_player_lib/SDL-1.2.15/build/../src/video/SDL_surface.c:866
# 4 0x7f2dc3dfaa40 in SDL_DisplayFormat /home/radu/apps/sdl_player_lib/SDL-1.2.15/build/../src/video/SDL_video.c:946:9
# 5 0x4dba11 in LoadSprite /home/radu/apps/sdl_player_lib/SDL-1.2.15/test/testsprite.c:41:9
# 6 0x4dbc8f in main /home/radu/apps/sdl_player_lib/SDL-1.2.15/test/testsprite.c:98:7
# 7 0x7f2dc2ace82f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
# 8 0x435528 in _start (/home/radu/apps/sdl_player_lib/SDL-1.2.15/test/testsprite+0x435528)

0x60200006b49c is located 0 bytes to the right of 12-byte region [0x60200006b490,0x60200006b49c)
allocated by thread T0 here:
# 0 0x4bc4f2 in malloc (/home/radu/apps/sdl_player_lib/SDL-1.2.15/test/testsprite+0x4bc4f2)
# 1 0x7f2dc3de8195 in Map1toN /home/radu/apps/sdl_player_lib/SDL-1.2.15/build/../src/video/SDL_pixels.c:473:17
# 2 0x7f2dc3de8195 in SDL_MapSurface /home/radu/apps/sdl_player_lib/SDL-1.2.15/build/../src/video/SDL_pixels.c:583
# 3 0x7f2dc3df4deb in SDL_LowerBlit /home/radu/apps/sdl_player_lib/SDL-1.2.15/build/../src/video/SDL_surface.c:417:8
# 4 0x7f2dc3df4deb in SDL_ConvertSurface /home/radu/apps/sdl_player_lib/SDL-1.2.15/build/../src/video/SDL_surface.c:866
# 5 0x7f2dc3dfaa40 in SDL_DisplayFormat /home/radu/apps/sdl_player_lib/SDL-1.2.15/build/../src/video/SDL_video.c:946:9

SUMMARY: AddressSanitizer: heap-buffer-overflow /home/radu/apps/sdl_player_lib/SDL-1.2.15/build/../src/video/SDL_blit_1.c:252 Blit1to4
Shadow bytes around the buggy address:
0x0c0480005640: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c0480005650: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c0480005660: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c0480005670: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
0x0c0480005680: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c0480005690: fa fa 00[04]fa fa 00 00 fa fa 00 00 fa fa 00 00
0x0c04800056a0: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
0x0c04800056b0: fa fa 00 00 fa fa 00 04 fa fa 00 04 fa fa 00 04
0x0c04800056c0: fa fa 00 04 fa fa 00 04 fa fa 00 04 fa fa 00 04
0x0c04800056d0: fa fa 00 04 fa fa fd fd fa fa fd fd fa fa fd fd
0x0c04800056e0: fa fa fd fd fa fa fd fd fa fa fd fd fa fa fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
Addressable: 00
Partially addressable: 01 02 03 04 05 06 07
Heap left redzone: fa
Heap right redzone: fb
Freed heap region: fd
Stack left redzone: f1
Stack mid redzone: f2
Stack right redzone: f3
Stack partial redzone: f4
Stack after return: f5
Stack use after scope: f8
Global redzone: f9
Global init order: f6
Poisoned by user: f7
Container overflow: fc
Array cookie: ac
Intra object redzone: bb
ASan internal: fe
Left alloca redzone: ca
Right alloca redzone: cb
==15035==ABORTING

Poc: See attachment
Reproducing steps:

1. Download SDL-1.2.15 library
2. ./configure with Asan enabled
3. ./make
4. sudo make install
5. cd examples
6. ./configure with Asan enabled
7. make
8. ./testsprite PoC

On 2019-02-10 14:56:55 +0000, Radue wrote:

Assigned CVE-2019-7635 by MITRE.

On 2019-02-18 16:15:30 +0000, Petr Pisar wrote:

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).

On 2019-02-19 14:08:23 +0000, Petr Pisar wrote:

Created attachment 3637
Fix for 1-, 4-, 8-bpp images

On 2019-02-21 09:48:26 +0000, Petr Pisar wrote:

By the way the BMP decoder assumes a less than 8 bit depth images have 1 or 4 bits per pixel. No other depths are correctly translated to an 8bpp surface. However, BMP specification does not forbid other depths like 2 or 3 bits per pixel.

Based on reading the code, I believe pretty strange things can happen when displaying these images. My fix only deals with 1-, 4- and 8-bpp images because the decoder does not provide an easy access to pixel values for other bpp images.

I believe it should be safer to reject the other-bpp completely.

On 2019-02-21 10:05:33 +0000, Petr Pisar wrote:

Created attachment 3645
Reject 2, 3, 5, 6, 7-bpp BMP images

This implements the rejection for strange-bpp images.

On 2019-03-17 01:37:59 +0000, Sam Lantinga wrote:

This is fixed, thanks!

SDL 2.0:
https://hg.libsdl.org/SDL/rev/7c643f1c1887

SDL 1.2:
https://hg.libsdl.org/SDL/rev/08f3b4992538
https://hg.libsdl.org/SDL/rev/4646533663ae

SDL_image:
https://hg.libsdl.org/SDL_image/rev/03bd33e8cb49

On 2019-03-18 09:04:40 +0000, Petr Pisar wrote:

SDL 1.2:
https://hg.libsdl.org/SDL/rev/08f3b4992538
https://hg.libsdl.org/SDL/rev/4646533663ae

Maybe I don't understand hg enough, but it seems to me that these two commits are identical and contain the "Reject 2, 3, 5, 6, 7-bpp BMP images" fix.

SDL-1.2 branch is still missing the "Fix for 1-, 4-, 8-bpp images" fix.

On 2019-04-11 06:57:59 +0000, AmeyaVS wrote:

Compilation failure due to duplication of switch case values.

As per the line:
https://github.com/spurious/SDL-mirror/blob/a88f1d07249cbe05e4d3fbd5e3b7b7269e4580c1/src/video/SDL_bmp.c#L40
The macro BI_RGB has not been defined any where in the source code.

The line define BI_BITFIELDS as 3:
https://github.com/spurious/SDL-mirror/blob/a88f1d07249cbe05e4d3fbd5e3b7b7269e4580c1/src/video/SDL_bmp.c#L44

Due to recent fixes in the following file reference:
https://github.com/spurious/SDL-mirror/blob/a88f1d07249cbe05e4d3fbd5e3b7b7269e4580c1/src/video/SDL_bmp.c#L212

a switch case statement was added with the value 3, which results in compilation failure due to duplicate case value.

Let me know in-case more information is needed.

Regards,
Ameya

Note: Sorry for the external reference, since in my current development environment I do not have access to mercurial.

On 2019-04-11 08:16:58 +0000, Petr Pisar wrote:

SDL 1.2:
https://hg.libsdl.org/SDL/rev/08f3b4992538
https://hg.libsdl.org/SDL/rev/4646533663ae

Maybe I don't understand hg enough, but it seems to me that these two
commits are identical and contain the "Reject 2, 3, 5, 6, 7-bpp BMP images"
fix.
[...]
Compilation failure due to duplication of switch case values.

The two commits are not identical in the end.

08f3b4992538 is the correct fix identical to here proposed "Reject 2, 3, 5, 6, 7-bpp BMP images".

4646533663ae is a patch unknown to me that breaks the compilation. The 4646533663ae commit should be reverted. It does not make sense at all.

On 2019-05-18 18:48:55 +0000, Ryan C. Gordon wrote:

Tagging a bunch of bugs with "target-2.0.10" so we have a clear list of things to address before a 2.0.10 release.

Please note that "addressing" one of these bugs might mean deciding to defer on it until after 2.0.10, or resolving it as WONTFIX, etc. This is just here to tell us we should look at it carefully, and soon.

If you have new information or feedback on this issue, this is a good time to add it to the conversation, as we're likely to be paying attention to this specific report in the next few days/weeks.

Thanks!

--ryan.

On 2019-06-09 00:48:50 +0000, Sam Lantinga wrote:

I'm not sure why that second commit was added, sorry about that!

It was fixed here:
https://hg.libsdl.org/SDL/rev/33940ce0a0ba

On 2019-06-11 09:34:48 +0000, Petr Pisar wrote:

The "Fix for 1-, 4-, 8-bpp images" patch is still missing from SDL-1.2 branch.

On 2019-06-11 13:28:59 +0000, Sam Lantinga wrote:

Patch added for SDL 1.2, thanks!
https://hg.libsdl.org/SDL/rev/f1f5878be5db