Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Audit claxon crate #4

Open
Shnatsel opened this issue Jul 21, 2019 · 3 comments
Open

Audit claxon crate #4

Shnatsel opened this issue Jul 21, 2019 · 3 comments

Comments

@Shnatsel
Copy link
Member

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
Copy link

nico-abram commented Aug 22, 2019

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
Copy link
Member Author

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
Copy link
Member Author

Shnatsel commented Nov 1, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants