Fix buffer overflow bug in deflate::core::BitBuffer::flush

[Shnatsel]

The problem is that this function does not check that there is enough space to fit a u64, it only checks that the starting position for the write is in bounds.

I have tried adding assert!(pos.checked_add(8).unwrap() <= output.inner.get_ref().len()); before the unsafe part, but that regressed performance exactly as much as the fully safe version, so I went for the fully safe code.

[Shnatsel]

to_le_bytes() was only added in rustc 1.32, let me add a local copy so that people on older compilers could receive the fix

[Shnatsel]

Since this could potentially be exploitable, please publish an updated version on crates.io ASAP.
After that please check if it's possible to trigger the buffer overflow by passing a crafted file to the decoder; if yes, we'll need to file a security advisory with RustSec.

[oyvindln]

Yeah, this is why any use of unsafe has to be well elaborated on :)

I'm looking through the code now to see if there are any surrounding code that checks if there's space. Not seeing a check in the original C code either, so either it's checked for at some earlier point which may or may not exist in the rust port, or there's a possible overflow issue in the original too.

Pinging @Frommi for a publish

[oyvindln]

Okay, so studied the code a little. (May be a bit hard to follow if you are not familiar with how deflate compression works.)

The BitBuffer struct is only used inside compress_lz_codes, so looking at it's use there.
For an overflow to occur here, the compressed lz codes would have to exceed the size of the uncompressed lz codes by a fair bit, as the lz code buffer is 65536 bytes large, and the output buffer size is set to be 85196 - 16 when created. The function may also insert up to 3 bytes before compress_lz_codes for the header.

The lz code buffer is the raw lz77 coded data, which consists of either a literal value, or a length-distance pair. compress_lz_codes is what translates these to the huffman codes for the respective values and pushes the encoded data to the output buffer.

Now, a block will always be flushed if there's less than 8 bytes left in the lz code buffer. I am not sure if it's possible for the lz code buffer to have the combination of size and long codes for the output buffer to overflow, especially not unless forcing static blocks (not something that flate2 does or one would normally do) but I'll have to look a bit further.

[Shnatsel]

@oyvindln now that this is merged and released, I'd really appreciate if you could take a further look to determine if this is exploitable or not. This code is powering every single binary that uses reqwest, so it is the impact is enormous and we'd better know about it. I am completely unfamiliar with deflate format, so I'm afraid I won't be of much help here.

[oyvindln]

Okay looked into it a bit more.

When a OutputBufferOxide is create, the inner Cursor is created from a slice of either and input buffer or a local_buf.

From what I can see the end of the slice this cursor is created from will always at 16 before the end of the underlying buffer. If the write would overflow (something which I haven't managed to verify can happen or not) it would still write at most 7 bytes over the end of the cursor (since the start position is checked.) which would still be inside the bounds of the underlying buffer. At least as long as the compiler is not somehow doing some very crazy optimization/code generation and writing to memory somewhere else but I can't see how that would happen.

The first case is what's encountered when using it through flate2, provided there is space in the buffer. The slice the Cursor is created from [offset..offset +buf_len]. Now buf_len is a constant set that is always OUT_BUF_SIZE - 16. If the length provided buffer is smaller than OUT_BUF_SIZE, or no output buffer is provided , a local buffer (with the static size OUT_BUF_SIZE) is used instead.

If the length comparison was to underflow, the code would panic with an out of bounds check as that would require out_buf_ofs to be larger than the length of `cb.out_buf. As far as I can tell, integer overflow/wraparound is required by the rust reference to be well-defined and as such my understanding the compiler should not optimize and remove bounds checks based on an overflow not happening, provided the rust compiler follows this.

In theory calling code could rely on the extra bytes bytes not being written but that wouldn't be unsafe per se.