search

mackron / dr_libs

Audio decoding libraries for C/C++, each in a single source file.
Other
1.24k stars 205 forks source link

Add FLAC fuzzer and fix found bugs #225

Closed ktmf01 closed 2 years ago

ktmf01 commented 2 years ago

The first commit (41c6caf) of this PR adds a fuzzer to the FLAC tests, which can be build exclusively with clang. It is inspired by (though is not a derivative of) the libFLAC fuzzer. When combined with -fsanitize options it is able to find bugs that can potentially cause security issues.

Commit c51ad32 silences certain (IMO benign) warnings generated when fuzzing with -fsanitize=undefined, the other 5 commits fix various problems found with the fuzzer. I have ran the test program on a set of testvectors with and without these patches on my Raspberry Pi 4 (ARM64) machine, and there seems to be no measureable impact of these patches combined together on decoding speed.

The second of the commits (04552a5), is in my view the most important, because it caused dr_flac to run extremely slow (42s for a fuzz execution where less than 0.1ms is the average), as it could be exploited for a denial-of-service attack.

Commit 066aacf is also important IMO, because it could cause extremely loud, distorted output when a FLAC stream switches to a higher number of bits-per-sample, for example from 16-bit to 24-bit, as the now 24-bit values were shifted left as if they were 16-bit. As the line below already mentiones dr_flac does not support samplerate of channel count changes mid-stream, it seems adding not supporting changing bit per sample to that list would be the most logical solution to this problem.

https://github.com/mackron/dr_libs/blob/4aff56541f5e6bd4b74053a0d5c9e6156e736059/dr_flac.h#L217

mackron commented 2 years ago

Thanks a lot for this. I'm happy with most of this, but I want to do some profiling on this before merging. Specifically, the added check in drflac__read_rice_parts_x1() for the underflow thing. This is the most frequently called function in the library and is the most sensitive to performance degradation so I'm just being cautious. From what I can tell, it looks like that change is in the slow path so hopefully shouldn't be too bad.

With commit https://github.com/mackron/dr_libs/pull/225/commits/4ec4a7485eefc8b32b0c0788603df394931e3232, I think the correct behaviour here is to abort with an error. The reason I think this is that the STREAMINFO block has a field that represents the maximum size of a block which is 16 bits. The block size of a frame is defined as the block size, minus 1 (hence why 1 added after the read). My thinking is that since the maximum size of a block is 0xFFFF, if the frame block size before adding 1 is 0xFFFF then it's too big and should be treated as invalid.

ktmf01 commented 2 years ago

I'm in not hurry whatsoever. I've a few PRs against libFLAC which have gone half a year without a single comment, so the fact that you reply to this within 24 hours is already a treat.

If you have any questions let me know.

mackron commented 2 years ago

I've pushed a couple of commits to your branch to change that header block size thing to return an error and also just cleaned up some comments just for consistency with my style.

I did some profiling and there doesn't seem to be any meaningful degradation in performance. Will get this merged soon.

mackron commented 2 years ago

This has been merged into the dev branch. Thanks!