Audit miniz_oxide

[Shnatsel]

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 😱

Tracking issues on miniz_oxide side:
Frommi/miniz_oxide#16
Frommi/miniz_oxide#52
Frommi/miniz_oxide#53

[Shnatsel]

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

Critical fixes:
Frommi/miniz_oxide#47 - buffer overflow, not exploitable in practice
Frommi/miniz_oxide#49 - memory unsafety, exploitability analysis still TODO

Unsafe usage reduction:
Frommi/miniz_oxide@335261c
Frommi/miniz_oxide@3b7208b

Still TODO:

1. Convert unaligned reads to safe code as described in Add safe version of read_u16_le() that is exactly as fast Frommi/miniz_oxide#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: Use Rust API directly from flate2 Frommi/miniz_oxide#16

[Shnatsel]

The core crate now forbids unsafe code: Frommi/miniz_oxide#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 Use standard rust allocator in C API, save state type in mz_stream Frommi/miniz_oxide#49

[oyvindln]

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]

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]

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

[Shnatsel]

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