Gracefully handle type confusion of inflate_state and tdefl_compressor in mz_stream in C API

[mssun]

The inflate_state and tdefl_compressor state struct are not consistent. This will cause a type confusion issue when calling deflateEnd with the inflate stream buffer using the C API, resulting a "double free" crash. The zlib library doesn't have this problem because when calling the deflateEnd, it knows the type of the state buffer. Instead, miniz_oxide's C API has to cast this buffer to Rust's type and lose its original type.

This PR crates a bogus field to make the inflate_state strut same as tdefl_compressor to work around this memory safety issue.

[Frommi]

Hi,

I'm not sure that this is right either. If there is a type confusion and deflateEnd is called for mz_stream with inflate_state, then self.inner in drop_inner for tdefl_compressor treated as inflate_state will mean other location than what really is a tdefl_compressor's inner in memory: Option<CompressorOxide> is the first field in tdefl_compressor and last one in PR's inflate_state. I think drop_inner would just 0-out some random fields. Am I missing something?

[mssun]

@Frommi you are right. I should put Option<CompressorOxide> in the first field.

[Frommi]

So, I thought about it more, ran valgrind and there are some thoughts:

1. Option<CompressorOxide> is big (like 300kb, IIRC) and to just alloc it because of possible type confusion is too big of a measure.
2. I swapped inflateEnd and deflateEnd and ran valgrind. It reported Conditional jump or move depends on uninitialised value(s) and Invalid free() / delete / delete[] / realloc(). If we go with bogus inner we should at least initialize it in mz_inflate_init2_oxide. CompressorOxide has Box inside and if it happens to be not None in uninitialized memory, it can try and drop that random address. And you can't write state.inner = None; because it will drop previous uninitialized CompressorOxide, so use memset.

I think a better idea is to have some kind of enum meaning deflate or inflate as the first field and if it is wrong, return MZError instead of silently doing something arcane with bogus compressors in decompressors.

[mssun]

I didn't notice that Option<CompressorOxide> is this big. For the second point, you mean that if using a bogus inner, we should initialize the it? Please correct me if I misunderstand you. Since no matter inner is None or not, drop_inner will try to drop CompressorOxide because of type confusion. To avoid to free the Box inside the bogusCompressorOxide, we should memset it.

Thanks for the detailed investigation. I have the same concern when writing a fix. I tried to use a field to indicate the type of a mz_stream. However, it's difficult to implement in generic functions like into_mz_stream. Do you have any suggestion on this? Thank you.

[Frommi]

About initialization, I meant that if after alloc Option<CompressorOxide> is interpreted as Some(_) then in mz_inflate_init2_oxide naive line state.inner = None would trigger drop which also would trigger drop of a Box inside it.

About enum, I thought of something like a field stream_type as first field in both tdefl_compressor and inflate_state which indicates which is it, so we could add a check in oxidize! macro, maybe (on mobile, cant check right now)?

[mssun]

we could add a check in oxidize! macro

I tried this, but it needs major modifications:

1. The macro first has to know which type of stream (inflate stream or deflate stream) does this function needs. E.g., mz_deflate and mz_deflateEnd need a deflate stream (I defined a enum: StreamType::Deflate). So we should have to change the definition of the oxidize! macro.
2. BoxedState needs to be public

[Frommi]

I came back, looked at the code and I think the best place to put a check is in the StreamOxide::new(stream: &mut mz_stream) -> Self, make it return Result<Self, MZError> and return MZError::Stream in case state is not null and a field StreamType doesn't match with type parameter ST.

P.S. Better yet, put tdefl_compressor and inflate_state into a enum and do a real rust-match. No need to home-brew same thing with StreamType.

[mssun]

a field StreamType doesn't match with type parameter ST.

How do I compare a field with the type parameter ST?

Say I have a enum:

pub enum StreamType {
    Inflate,
    Deflate
}

#[repr(C)]
#[allow(bad_style)]
pub struct inflate_state {
    pub stream_type: StreamType,
    pub m_decomp: DecompressorOxide,

    pub m_dict_ofs: c_uint,
    pub m_dict_avail: c_uint,
    pub m_first_call: c_uint,
    pub m_has_flushed: c_uint,

    pub m_window_bits: c_int,
    pub m_dict: [u8; TINFL_LZ_DICT_SIZE],
    pub m_last_status: TINFLStatus,
}

impl<'io, ST: StateType> StreamOxide<'io, ST> {
    pub unsafe fn new(stream: &mut mz_stream) -> Result<Self, MZError> {
        if stream.state.is_null() {
            return Err(MZError::Stream);
        }
        // how to compare stream.state.stream_type with ST?
    }
}

[Frommi]

One way to do it is add a method get_stream_type() -> StreamType to trait StateType and implement them in to different ST's as just always return StreamType::Deflate for tdefl_compressor and StreamType::Inflate for inflate_state. And now you can compare stream.state.stream_type with ST::get_stream_type().

But I really think that having mz_stream.state as a pointer to enum of inflate_state or tdefl_compressor would be better. More changes in the code though.

[mssun]

Hi @Frommi, I think this is a working PR. There are two questions still need to solve:

1. When calling init function, the state is null. Therefore, it seems that we cannot check if the state buffer is null.
2. Can you explain more on having mz_stream.state as a pointer to enum of inflate_state or tdefl_compressor to do a Rust match? I still didn't get it. (Sorry for my poor understanding, and not get used to the Rust's low level programming).

[Frommi]

Hi,

It's okay and is logically right to not do the check in init, it is supposed to be null here and so there is nothing to check.

I see that you use unwrap on this Result from StreamOxide::new. This defies the purpose of having a Result in the first place. It is generally not the best style to use it, using it only when you are sure it is Ok as it panics otherwise, not that graceful :). If new returns Err, you should return C int error code associated with this enum error code. In oxidize! there is as_c_return_code called just a few lines after StreamOxide::new for that. In Rusty-style code it usually is just ? to transform errors.

In mz_stream the field is state: *mut mz_internal_state. We can make mz_internal_state a enum with two options: Inflate(inflate_state) and Deflate(tdefl_compressor). This would let us remember what type was allocated in the inits. We just need to not lose this information during into_mz_stream, but that can be done as we have type parameter ST there using similar trick with the trait method. StreamOxide has a type parameter, so there is no need to store enum'ed state here, just as it is now. Does that make it more clear?

[mssun]

Thanks for you feedback. Let me try to change the code accordingly.

[mssun]

@Frommi I tried to implement in the other way. Because I'm not familiar with different types/structs, it is difficult for me to finish it.

[Frommi]

Okay, I'll implement it and push it here. Another question is should we free the struct by it's real type in case of type confusion or only return error code? If not, there will be memory leak, which is probably fine, and the other way we risk something like double free. Do you have an opinion on this and do you know what zlib does?

[mssun]

For zlib/miniz, since there is no type confusion problem (because there is no FFI), buffers will be freed regardless of inflateEnd of deflateEnd.

int mz_inflateEnd(mz_streamp pStream)
{
    if (!pStream)
        return MZ_STREAM_ERROR;
    if (pStream->state)
    {
        pStream->zfree(pStream->opaque, pStream->state);
        pStream->state = NULL;
    }
    return MZ_OK;
}

int mz_deflateEnd(mz_streamp pStream)
{
    if (!pStream)
        return MZ_STREAM_ERROR;
    if (pStream->state)
    {
        pStream->zfree(pStream->opaque, pStream->state);
        pStream->state = NULL;
    }
    return MZ_OK;
}

https://github.com/richgel999/miniz/blob/master/miniz.c

[mssun]

Any update on this PR?

[oyvindln]

Hi

I've looked over the changes.

I am a little unsure about this

Is it okay to cast an unknown pointer as a trait object like that? I haven't really used dynamic dispatch in Rust much.

[mssun]

@oyvindln what did you mean? StreamType is not a trait object.

[oyvindln]

Ah I misread the code a bit. It's not a trait object. Though the state field in mz_stream is of type pointer to mz_internal_state (which is an empty struct), so I'm not sure if reading it as if it's a statetype is correct. Might be better to put the StreamType field somewhere the mz_stream type.

Sorry for the late reply.

[mssun]

Hi, @oyvindln . Do you have any better suggestion? Thanks.

[mssun]

Hi, is it possible to merge this PR or can we discuss how to improve the fix? Thanks.

[oyvindln]

Hi, sorry for taking so long to reply.
I think it would be better to store the stream type in the mz_stream structure itself so we don't touch the inner state objects without knowing what type they are.

[Shnatsel]

Double free can actually be exploitable, so please prioritize fixing this.

This is all the more critical because miniz_oxide is used in reqwest where it's exposed to untrusted input from the network.

[oyvindln]

Working on this now. Not sure if it would fix #14, or if the issue in #14 even still exists, but we'll see.

[oyvindln]

Resolved this in #49