Contents of uninitialized memory are leaked into the output on malformed inputs

[Shnatsel]

On certain malformed inputs contents of uninitialized memory are written to the decoded audio.

This is a security vulnerability. Examples of similar vulnerabilities in C code and discussion of the potential impact can be found here. I have also discussed a similar bug in Rust png crate in my Auditing popular crates post.

This issue has been discovered using differential fuzzing with afl-fuzz, similar to the C vulnerabilities linked above. I shall relay further details on the issue to the maintainer privately by email.

The trivial hotfix is replacing the following line

https://github.com/ruuda/claxon/blob/b4a89e4b67b31f04c2e4f785cba109115ea2c355/src/frame.rs#L618

with buffer.resize(new_len, 0);, but is likely to degrade performance. There are some tricks that can reduce the cost of zeroing the memory, but this approach is bound to be slower than using uninitialized memory.

However, these kinds of issues can be ruled out systemically by passing a reference to the vector to subframe::decode instead of a slice with uninitialized memory and using something like Write trait or extend_from_slice() to write to the vector safely without zeroing the memory first.

Once a fix is published, the issue should be added to the Rust security advisory database.

[ruuda]

Thank you for the report. If you want to report more details privately, you can find my GPG key here.

[Shnatsel]

I have already emailed further details to the address listed at https://ruudvanasseldonk.com/contact; you should have received it by now. If not, please check the spam folder; if it's still missing, we'll have to try another method of communication.

[ruuda]

Thanks again, the detailed steps to reproduce allowed me to reproduce the issue and pinpoint the cause quickly.

I fixed the root cause of the bug: a value that was assumed to be a multiple of value read from the bitstream could be rounded down if it was not a multiple. This caused some parts of the buffer to not be overwritten. If the buffer was a new buffer of uninitialized memory, that memory could be exposed. I believe it would have been possible to return part of the memory unaltered, although it would have required a very specifically crafted input.

The reason that this bug was not caught earlier despite extensive fuzzing, is that it did not cause decoding to fail, nor did it cause any invariants or debug asserts to be violated, or trigger arithmetic overflows. Differential fuzzing was a great idea to catch this kind of issue!

Knowing the cause of the bug, I took the following measures:

• Added the test samples to the test suite, and decode all regression test samples twice, with different initial buffers. Without the test applied this catches the bug.
• Added a fuzz target that decodes twice into different buffers that were pre-filled with different marker bytes. With checksums disabled this fuzzer hits this issue after a few minutes of fuzzing. I am still running this fuzzer to look for further similar issues.
• I still intend to audit all places where the buffer is sliced: ultimately the problem was not failing to overwrite the entire buffer, but not slicing up the entire buffer in the places where slicing happens.

If nothing else comes up I’ll make a new release this weekend. For users of Claxon, I do not think the issue requires immediate attention; a malicious input would have to be specifically crafted against Claxon’s internals, and as far as I am aware Claxon is not widely used, so this would be a targeted attack. Nonetheless I am going hold off from publishing the samples until the release. In the mean time the fix suggested by @Shnatsel will mitigate the issue.

[ruuda]

Claxon 0.3.2 and 0.4.1 that include a fix have been published. After the fix, further fuzzing for about 48 CPU hours turned up no additional results. I opened https://github.com/RustSec/advisory-db/pull/54.

[Shnatsel]

Regular fuzzing for 1 billion executions to explore the binary and subsequent differential fuzzing for 250 million executions did not turn up anything else for me either.

I will publish my fuzzing harness shortly and open a PR to include the generated corpus into the repository to kickstart further fuzzing.

Thank you for handling this so swiftly and responsibly!

[ruuda]

I deliberately do not include the fuzzing corpus in the repository to keep the repository size small. My local corpus is 1.2 GB, that’s not something you want to include in the repository. A few minimal cases that triggered bugs previously are in testsamples/fuzz, these should help to kickstart fuzzing.

I tried to run fuzzing on CI at some point and cache the corpus between builds, but then disabled it because it was broken.

[Shnatsel]

That's weird. I've got a corpus of just 5Mb total to get "cov: 11604 ft: 68260" libfuzzer coverage metrics, or roughly 1000 files after afl fuzz cmin

[Shnatsel]

I have published the differential fuzzing harness I've used to discover the issue as well as a custom tool I've hacked together to make fuzzers able to detect such issues at all.

I intend to publish a blog post explaining how it works and how to apply it to other crates soon.

[Shnatsel]

I've published a blog post about the custom tool I've built that made discovering this issue possible: https://medium.com/@shnatsel/how-ive-found-vulnerability-in-a-popular-rust-crate-and-you-can-too-3db081a67fb