Created attachment 3011 [details]
Test program to reproduce crashes.

Here's the test program AFL was using. You could just run it like:

for f in SDL-loadwav-fuzzing-crashes/* ; do ./testwav $f ; done

To see every crash.

Easy to compile: gcc -g -o testwav testwav.c `sdl2-config --cflags --libs`

Created attachment 3193 [details]
Draft patch that makes SDL_LoadWAV_RW much more robust.

I had a look at this and made some additions to SDL_wave.c.

The attached patch adds many checks and error messages. For some reason I also added A-law and µ-law decoders. Forgot exactly why... but hey, they're small.

The WAVE format is seriously underspecified (at least by the documents that are publicly available on the internet) and it's a shame Microsoft never put something better out there. The language used in them is so loose at times, it's not surprising the encoders and decoders behave very differently. The Windows Media Player doesn't even support MS ADPCM correctly.

The patch also adds some hints to make the decoder more strict at the cost of compatibility with weird WAVE files.

I still think it needs a bit of cleaning up (Not happy with the MultiplySize function. Don't like the name and other SDL code may want to use something like this too.) and some duplicated code may be folded together. It does work in this state and I have thrown all kinds of WAVE files at it. The AFL files also pass with it and some even play (obviously just noise). Crafty little fuzzer.

Any critique would be welcome. I have a fork of SDL with a audio-loadwav branch over here if someone wants to use the commenting feature of Bitbucket:

https://bitbucket.org/ChliHug/SDL

I also cobbled some Lua scripts together to create WAVE test files:

https://bitbucket.org/ChliHug/gendat

A few bits I noticed:

In new static procedure WaveLoad():  count <= SDL_MAX_UINT32 is always
true, because count is an unsigned int.

New procedure MultiplySize() having SDL_bool return type but returning
SDL_TRUE upon overflow is confusing and wrong: if overflow is an error
as the procedure description implies, the return should be to be an int
and returned values should be changed to 1 or -1 for overflow and 0 for
normal case.

Thank you for the feedback.

It took me a minute to realize why I wrote that check. It's probably safe to assume that all modern compilers that target the most used architectures always use 32 bits for the int type. However, that's not guaranteed by C. Maybe the ILP64 data model gets popular in the (far?) future.

The usual error return values are of course better for the the multiply function. I made the correction.

Wow...

In general the SDL WAV loader is designed to be simpler and the one in SDL_mixer is intended to support more formats. I'm not opposed to the changes you present here, but can you also port them to SDL_mixer for the next release?

I'll be happy to accept both patches at the same time.

Thanks!

Oh, I forgot about SDL_mixer. I'm not familiar with its code base, but I'll give it a try.

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.

Simon, can you take a look at this patch again? It doesn't cleanly apply against the code in Mercurial anymore.

Actually, I backed out the other change and applied this patch to SDL, as it's way more robust and fixes the other CVE issues as well.
https://hg.libsdl.org/SDL/rev/b06fa7da012b

It may make sense to drop SDL_mixer's WAVE loading code in favor of this code in SDL. Is there anything that the SDL_mixer code does that the SDL WAVE loader doesn't do now?

Created attachment 3811 [details]
Minor cleanup patch for tip.

I'm sorry for never getting back to this.

The chunk part of SDL_mixer uses SDL's SDL_LoadWAV_RW to load wave files. The music part can only do 8 and 16-bit PCM and no extensible header. It seems to support loops and samples, but I haven't tested that yet. I'm trying to integrate the code from this patch into SDL_mixer now. Don't hold up the new SDL release for that, though!

Attached is a minor cleanup patch. It changes the option name of one hint to something better, puts one or two more checks in, and adds explicit casting where warnings could appear otherwise.

I hope the naming of the hints and their options is acceptable. It would be kind of awkward to change them after they get released with an official SDL version.

Oh, and the #ifdef'd debugging code was never meant to be included in the final patch (it's kind of ugly to look at), but if you want to leave it in that's ok.

Created attachment 3812 [details]
Minor cleanup patch for tip.

I see that there was an issue with the struct initialization in may code. Updated the cleanup patch to reflect that.

The cleanup is in:
https://hg.libsdl.org/SDL/rev/572f29f98da0

If the debug code would be helpful in the future, feel free to leave it in.

Maybe double-check this commit: 
bug 4842 - https://hg.libsdl.org/SDL/rev/26581b48aef9
TruncVeryStrict was check twice !

(In reply to Sylvain from comment #13)
> Maybe double-check this commit: 
> bug 4842 - https://hg.libsdl.org/SDL/rev/26581b48aef9
> TruncVeryStrict was check twice !

That commit looks correct.

This bug looks like it can be closed; is there anything pending that prevents that?

I'm thinking we don't change SDL_mixer at this time, since it appears to be depending on SDL for the part of WAV decode SDL does anyhow...I'm not sure if anything else is still pending in here.

--ryan.

Closing this bug, as it appears to be resolved. Please reopen if there's more work to be done!

--ryan.