search

rust-secure-code / safety-dance

Auditing crates for unsafe code which can be safely replaced
Apache License 2.0
536 stars 10 forks source link

Audit claxon crate #4

Open Shnatsel opened 5 years ago

Shnatsel commented 5 years ago

https://crates.io/crates/claxon

FLAC decoder in Rust, within 10% performance margin compared to reference C implementation. Not terribly widely used (300 downloads/day) but high-risk due to being a binary format decoder. Uses unsafe code.

It is highly optimized, but on an older compiler version, so it should be possible to either rewrite it safely or identify missing safe abstractions.

nico-abram commented 5 years ago

I just skimmed the unsafes in the master branch. There's a mem::uninitialized in a bench, and there's 3 instances of setting a vec's len and then making a slice from it in this file: https://github.com/ruuda/claxon/blob/cd82be35f413940ba446d2a19f10d74b86466487/src/metadata.rs#L459-L461 read_into coerces it into a slice: https://github.com/ruuda/claxon/blob/cd82be35f413940ba446d2a19f10d74b86466487/src/input.rs#L157

There's a few get_unchecked's but they seemed alright.

I think that's technically UB but I'll leave it to someone more knowledgeable if it's worth repalcing/how.

Shnatsel commented 5 years ago

A while ago I've found one instance of such code to be exploitable: https://rustsec.org/advisories/RUSTSEC-2018-0004.html

The solution is to either zero-initialize the vector which is essentially free in one-shot programs, or appending to a vector instead of coercing it to a slice and writing to the slice. The reason Claxon does this is so that they don't have to carry a separate length field everywhere; I've detailed it more here.

This could potentially be solved by providing a fixed-capacity buffer that nevertheless doesn't expose uninitialized memory. The API of buffer crate is very close, although I have not vetted the implementation.

This would also help with the problem that if you want to read a fixed number of elements, you need to zero-initialize the memory because read() accepts a slice, unlike read_to_end()` which accepts a vector. This has led to issues with uninitialized memory in e.g. libflate.

Shnatsel commented 5 years ago

Looks like auditing is done here, we just need to land the PR. Some of the unsafe cannot be removed without a performance penalty - blocked on a fixed-capacity Vec view abstraction, see #34