Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Heap-Buffer Overflow in SDL_GetRGB pertaining to SDL_pixels.c #789

Closed
SDLBugzilla opened this issue Feb 10, 2021 · 0 comments
Closed

Heap-Buffer Overflow in SDL_GetRGB pertaining to SDL_pixels.c #789

SDLBugzilla opened this issue Feb 10, 2021 · 0 comments

Comments

@SDLBugzilla
Copy link
Collaborator

This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

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

Comments on the original bug report:

On 2019-03-05 05:32:53 +0000, zhangweiye(topsec) wrote:

Created attachment 3689
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

On 2019-03-17 01:16:08 +0000, Sam Lantinga wrote:

Ryan, can you double check that this isn't active in SDL 2.0 or sdl12-compat?

On 2019-07-30 17:49:37 +0000, Ryan C. Gordon wrote:

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

On 2019-09-03 10:56:52 +0000, Sylvain wrote:

I can't download the attached file ... browser blocks it ?! can you compress/archive it or something ?

On 2019-09-03 13:51:02 +0000, Ozkan Sezer wrote:

@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?

On 2019-09-03 18:55:42 +0000, Sam Lantinga wrote:

Fixed, thanks!
https://hg.libsdl.org/SDL/rev/6203d73874ab

On 2019-09-03 19:00:03 +0000, Sylvain wrote:

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

On 2019-09-03 20:00:20 +0000, Ozkan Sezer wrote:

(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

On 2019-09-20 20:47:37 +0000, Ryan C. Gordon wrote:

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.

On 2019-09-20 20:48:39 +0000, Ryan C. Gordon wrote:

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant