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 miniz_oxide #2

Closed Shnatsel closed 5 years ago

Shnatsel commented 5 years ago

https://crates.io/crates/miniz_oxide

DEFLATE encoder/decoder. 5000 downloads/day, plenty of unsafe blocks, exposed to untrusted data from the network through reqwest :scream:

Tracking issues on miniz_oxide side: https://github.com/Frommi/miniz_oxide/issues/16 https://github.com/Frommi/miniz_oxide/issues/52 https://github.com/Frommi/miniz_oxide/issues/53

Shnatsel commented 5 years ago

I've started doing that, the crate maintainer then stepped up and killed off most of the unsafe code by themselves.

Critical fixes: https://github.com/Frommi/miniz_oxide/pull/47 - buffer overflow, not exploitable in practice https://github.com/Frommi/miniz_oxide/pull/49 - memory unsafety, exploitability analysis still TODO

Unsafe usage reduction: https://github.com/Frommi/miniz_oxide/commit/335261cbfabdd365713f22faefcd4b8007518a45 https://github.com/Frommi/miniz_oxide/commit/3b7208b221eb7c184f80de64cf383c229e3cd1a4

Still TODO:

  1. Convert unaligned reads to safe code as described in https://github.com/Frommi/miniz_oxide/pull/48 - but this will bump minimum required Rust version to 1.34
  2. Call the Rust API directly from flate2 instead of going through the C API, which already had one memory corruption bug in it. Tracked here: https://github.com/Frommi/miniz_oxide/issues/16
Shnatsel commented 5 years ago

The core crate now forbids unsafe code: https://github.com/Frommi/miniz_oxide/pull/56

Still TODO:

  1. Make flate2 use the safe Rust API of miniz_oxide directly instead of going through the C API
  2. Exploitability analysis on https://github.com/Frommi/miniz_oxide/pull/49
oyvindln commented 5 years ago

flate2 now uses miniz_oxide directly through safe functions. flate2 still makes use of unsafe to avoid zero-initializing the input array though, and assumes the underlying implementation does not read those bytes. With a the rust back-end it may be possible to work around that in a safe manner, not so much when calling into C.

Avoiding zero-initialization of buffers seem to be a common use of unsafe in general, so maybe it would be worth looking into some best practices or ways to do it safely.

As for Frommi/miniz_oxide#49 , I think that would only really have been an issue when using it from C, and calling deflate on a stream initialized with inflate, though I could be wrong.

Shnatsel commented 5 years ago

flate2 code such as this can be easily refactored into safe code:

https://github.com/alexcrichton/flate2-rs/blob/537fb77132a15b772fcc9c35a4c8c679d40aedf7/src/mem.rs#L317-L323

The outer function accepts the output buffer as &mut Vec<u8>, converts it to a slice internally and passes the slice as output buffer to the backend.

Exposure of uninitialized memory can be easily avoided by passing the &mut Vec<u8> to miniz_oxide and decoding to it instead of decoding to a slice. I believe miniz_oxide already implements something very similar with its decompress_to_vec() function.

oyvindln commented 5 years ago

decompress_to_vec() used a zero-allocated vector, but yeah, shouldn't be too complicated to do.

Shnatsel commented 5 years ago

miniz_oxide is now 100% safe, closing. Following up on flate2 in #32.