We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 4536 - Heap-Buffer Overflow in SDL_GetRGB pertaining to SDL_pixels.c
Summary: Heap-Buffer Overflow in SDL_GetRGB pertaining to SDL_pixels.c
Status: RESOLVED FIXED
Alias: None
Product: SDL
Classification: Unclassified
Component: video (show other bugs)
Version: HG 1.2
Hardware: x86_64 Linux
: P2 critical
Assignee: Ryan C. Gordon
QA Contact: Sam Lantinga
URL:
Keywords: target-2.0.12
Depends on:
Blocks:
 
Reported: 2019-03-05 05:32 UTC by zhangweiye(topsec)
Modified: 2019-09-20 20:48 UTC (History)
2 users (show)

See Also:


Attachments
the poc file (34 bytes, image/bmp)
2019-03-05 05:32 UTC, zhangweiye(topsec)
Details

Note You need to log in before you can comment on or make changes to this bug.
Description zhangweiye(topsec) 2019-03-05 05:32:53 UTC
Created attachment 3689 [details]
the poc file

heap-buffer overflow vulnerability was discovered in hg-SDL-1.2  library.

rename POC file to icon.bmp then run ./testsprite 

```
fuzz@fuzz:~/Desktop/sdl/SDL-19d8c3b9c251/test$ ./testsprite 
=================================================================
==19159==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x602000009de8 at pc 0x7f2b733ab018 bp 0x7ffe0505d2b0 sp 0x7ffe0505d2a0
READ of size 1 at 0x602000009de8 thread T0
    #0 0x7f2b733ab017 in SDL_GetRGB src/video/SDL_pixels.c:416
    #1 0x7f2b733b63f4 in SDL_ConvertSurface src/video/SDL_surface.c:877
    #2 0x5628f544de26 in LoadSprite /home/fuzz/Desktop/sdl/SDL-19d8c3b9c251/test/testsprite.c:49
    #3 0x5628f544cf9a in main /home/fuzz/Desktop/sdl/SDL-19d8c3b9c251/test/testsprite.c:224
    #4 0x7f2b72b78b96 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x21b96)
    #5 0x5628f544dc89 in _start (/home/fuzz/Desktop/sdl/SDL-19d8c3b9c251/test/testsprite+0x2c89)

Address 0x602000009de8 is a wild pointer.
SUMMARY: AddressSanitizer: heap-buffer-overflow src/video/SDL_pixels.c:416 in SDL_GetRGB
Shadow bytes around the buggy address:
  0x0c047fff9360: fa fa 01 fa fa fa 00 00 fa fa 00 00 fa fa fa fa
  0x0c047fff9370: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff9380: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff9390: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff93a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c047fff93b0: fa fa fa fa fa fa fa fa fa fa fa fa fa[fa]fa fa
  0x0c047fff93c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff93d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff93e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff93f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c047fff9400: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
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
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  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
==19159==ABORTING
```
Comment 1 Sam Lantinga 2019-03-17 01:16:08 UTC
Ryan, can you double check that this isn't active in SDL 2.0 or sdl12-compat?
Comment 2 Ryan C. Gordon 2019-07-30 17:49:37 UTC
(Sorry if you get several emails like this, we're marking a bunch of bugs.)

We're hoping to ship SDL 2.0.11 on a much shorter timeframe than we have historically done releases, so I'm starting to tag bugs we hope to have closed in this release cycle.

Note that this tag means we just intend to scrutinize this bug for the 2.0.11 release: we may fix it, reject it, or even push it back to a later release for now, but this helps give us both a goal and a wishlist for the next release.

If this bug has been quiet for a few months and you have new information (such as, "this is definitely still broken" or "this got fixed at some point"), please feel free to retest and/or add more notes to the bug.

--ryan.
Comment 3 Sylvain 2019-09-03 10:56:52 UTC
I can't download the attached file ... browser blocks it ?! can you compress/archive it or something ?
Comment 4 Ozkan Sezer 2019-09-03 13:51:02 UTC
@Sylvain:  try doing:
 wget -O icon.bmp "https://bugzilla.libsdl.org/attachment.cgi?id=3689"


As for the issue: This bmp reports bpp=0, therefore SDL_CalculatePitch()
returns pitch==0, which is then fed to SDL_malloc() (which is malloc())
and malloc(0) returns _something_ which is not NULL but not someting
that we expect..  Then testsprite.c:LoadSprite() accesses the pixels
as *(Uint8*)pixels which valrind reports as:

==15533== Invalid read of size 1
==15533==    at 0x8048C08: LoadSprite (testsprite.c:45)
==15533==    by 0x80492FC: main (testsprite.c:224)
==15533==  Address 0x449e588 is 0 bytes after a block of size 0 alloc'd
==15533==    at 0x40072B2: malloc (vg_replace_malloc.c:270)
==15533==    by 0x4045719: SDL_CreateRGBSurface (SDL_surface.c:126)
==15533==    by 0x40403C1: SDL_LoadBMP_RW (SDL_bmp.c:237)
==15533==    by 0x8048BB2: LoadSprite (testsprite.c:36)
==15533==    by 0x80492FC: main (testsprite.c:224)

Besides, valrind also reports this:
==15533== Conditional jump or move depends on uninitialised value(s)
==15533==    at 0x40403F3: SDL_LoadBMP_RW (SDL_bmp.c:247)
==15533==    by 0x8048BB2: LoadSprite (testsprite.c:36)
==15533==    by 0x80492FC: main (testsprite.c:224)


Easy/quick solution would be early-rejecting a bmp with 0 bpp from
SDL_bmp.c:SDL_LoadBMP_RW().  E.g. something like:
diff --git a/src/video/SDL_bmp.c b/src/video/SDL_bmp.c
--- a/src/video/SDL_bmp.c
+++ b/src/video/SDL_bmp.c
@@ -165,12 +165,13 @@ SDL_Surface * SDL_LoadBMP_RW (SDL_RWops 
 	switch (biBitCount) {
 		case 1:
 		case 4:
 			ExpandBMP = biBitCount;
 			biBitCount = 8;
 			break;
+		case 0:
 		case 2:
 		case 3:
 		case 5:
 		case 6:
 		case 7:
 			SDL_SetError("%d-bpp BMP images are not supported", biBitCount);

Comments?
Comment 5 Sam Lantinga 2019-09-03 18:55:42 UTC
Fixed, thanks!
https://hg.libsdl.org/SDL/rev/6203d73874ab
Comment 6 Sylvain 2019-09-03 19:00:03 UTC
The patch seems to make sense to me.


But I can't reproduce the memory issue.
the icon.bmp I got is rejected by SDL_CreateRGBSurface(), 
with error: "Unknown pixel format", but no valgrind leak.

I added some trace before CreateRGBSurface in SDL_LoadBMP:
SDL_Log("surface=%p biWidth=%d biHeight=%d biBitCount=%d Rmask=%08x Gmask=%08x Bmask=%08x Amask=%08x", surface, biWidth, biHeight, biBi    tCount, Rmask, Gmask, Bmask, Amask );

surface=(nil) biWidth=250 biHeight=2560 biBitCount=0 Rmask=00000000 Gmask=00000000 Bmask=00000000 Amask=00000000
Indeed biBitCount is 0, but the surface created is NULL so the function LoadBMP somehow fails



hexdump icon.bmp 
0000000 4d42 ffec ff22 0aff 0000 0000 0000 0000
0000010 ff00 00fa 0000 0a00 0000 0000 0000 0000
0000020 0000                                   
0000022


md5sum icon.bmp 
ad09706f94d97935a3377f0c394018ff  icon.bmp
Comment 7 Ozkan Sezer 2019-09-03 20:00:20 UTC
(In reply to Sylvain from comment #6)
> But I can't reproduce the memory issue.
The issue was reported for SDL1.2 branch.

Fix now applied to SDL-1.2 branch, too:
https://hg.libsdl.org/SDL/rev/13b0e1be3ca2
Comment 8 Ryan C. Gordon 2019-09-20 20:47:37 UTC
We're changing how we do SDL release versions; now releases will be even numbers (2.0.10, 2.0.12, etc), and as soon as we tag a release, we'll move the internal version number to an odd number (2.0.12 ships, we tag the latest in revision control as 2.0.13 immediately, which will become 2.0.14 on release, etc).

As such, I'm moving the bugs tagged with target-2.0.11 to target 2.0.12. Sorry if you get a lot of email from this change!

Thanks,
--ryan.
Comment 9 Ryan C. Gordon 2019-09-20 20:48:39 UTC
We're changing how we do SDL release versions; now releases will be even numbers (2.0.10, 2.0.12, etc), and as soon as we tag a release, we'll move the internal version number to an odd number (2.0.12 ships, we tag the latest in revision control as 2.0.13 immediately, which will become 2.0.14 on release, etc).

As such, I'm moving the bugs tagged with target-2.0.11 to target 2.0.12. Sorry if you get a lot of email from this change!

Thanks,
--ryan.