| Summary: | HEAP BUFFER OVERFLOW in SDL_LoadWAV | ||
|---|---|---|---|
| Product: | SDL | Reporter: | A Annour <anass.annour> |
| Component: | audio | Assignee: | Ryan C. Gordon <icculus> |
| Status: | RESOLVED FIXED | QA Contact: | Sam Lantinga <slouken> |
| Severity: | critical | ||
| Priority: | P2 | CC: | 1909675781 |
| Version: | HG 2.1 | ||
| Hardware: | x86_64 | ||
| OS: | Linux | ||
| Attachments: | Sample of WAV files that trigger the vulnerability | ||
Looking at this. --ryan.
Okay, this is happening not because of the wave file but because SDL's error string stuff freaks out if there are more than 5 printf arguments. :/
In SDL/src/SDL_error_c.h ...
#define ERR_MAX_ARGS 5
In WaveCheckFormat, in SDL/src/audio/SDL_wave.c ...
const char *errstr = "Unknown WAVE format GUID: %08x-%04x-%04x-%02x%02x%02x%02x%02x%02x%02x%02x";
So when we call SDL_GetError later, it blows up, as it walks past the end of SDL_error::args for all those '%' formatters.
The quick fix for this particular problem is use SDL_snprintf() to build the GUID string and then pass that as one argument to SDL_SetError(), the slightly more general fix is to make ERR_MAX_ARGS bigger, but I don't know why this system is built like this at all: why don't we format the string during SDL_SetError() and not have an array of args at all (if nothing else: if I pass a '%s' in there and then free the string, does it get dereferenced later during SDL_GetError()?
I suggest we make ERR_MAX_ARGS 16 for now ("should be enough for anyone!"), because 5 seems low in general, but we need to look at this further before closing this bug in any case.
As for the fuzzed .wav files: they all correctly reject without overflowing any buffers once this error string issue is mitigated.
--ryan.
The idea was to avoid printf performance hit in case the error wasn't read. In retrospect this seems silly. Feel free to fix. :) (In reply to Sam Lantinga from comment #3) > The idea was to avoid printf performance hit in case the error wasn't read. > In retrospect this seems silly. Feel free to fix. :) Ok, working on it. --ryan. I took a flamethrower to this in https://hg.libsdl.org/SDL/rev/6a1ff60e03fb Sam, if this is acceptable, we can also dump SDL_GetErrorMsg, which was just made public after 2.0.12 shipped, since the usual SDL_GetError() is now thread-safe again. https://discourse.libsdl.org/t/is-sdl-geterror-thread-safe-or-not/26873/ --ryan. |
Created attachment 4307 [details] Sample of WAV files that trigger the vulnerability I've used the lastest version of SDL-355a4f94a782. Compiled my program that uses the function SDL_LoadWAV with AFL-clang and ASAN enabled. I've got a Heap Buffer Overflow vulnerability. tester@fuzzer:~$programtest 1.wav ================================================================= ==767399==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x618000000388 at pc 0x7f35982d6de8 bp 0x7ffd055792f0 sp 0x7ffd055792e8 READ of size 4 at 0x618000000388 thread T0 #0 0x7f35982d6de7 in SDL_GetErrorMsg /SDL-355a4f94a782/src/SDL_error.c:254:58 #1 0x521ab0 in main /SDL-355a4f94a782/test/test2021.c:30:22 #2 0x7f3597f21e0a in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x26e0a) #3 0x41d3f9 in _start (/SDL-355a4f94a782/test/test2021+0x41d3f9) 0x618000000388 is located 0 bytes to the right of 776-byte region [0x618000000080,0x618000000388) allocated by thread T0 here: #0 0x4f0cef in malloc (/SDL-355a4f94a782/test/test2021+0x4f0cef) #1 0x7f3598447f19 in SDL_malloc_REAL /SDL-355a4f94a782/src/stdlib/SDL_malloc.c:5387:11 SUMMARY: AddressSanitizer: heap-buffer-overflow /SDL-355a4f94a782/src/SDL_error.c:254:58 in SDL_GetErrorMsg Shadow bytes around the buggy address: 0x0c307fff8020: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c307fff8030: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c307fff8040: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c307fff8050: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c307fff8060: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 =>0x0c307fff8070: 00[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c307fff8080: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c307fff8090: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c307fff80a0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c307fff80b0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa 0x0c307fff80c0: 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 ==767399==ABORTING