trifectatechfoundation / zlib-rs

A safer zlib
zlib License
139 stars 15 forks source link

`deflate::set_header()` is unsound #210

Closed inahga closed 4 weeks ago

inahga commented 1 month ago

It's possible to trigger UB/segfaults from safe Rust when using set_header().

Example:

    #[test]
    fn inahga() {
        let mut extra = vec![0x00u8, 0x01, 0x03, 0x04];

        let mut header = gz_header {
            text: 0,
            time: 1727806483,
            extra: extra.as_mut_ptr(),
            extra_len: 1024, // OOB
            ..Default::default()
        };

        let config = DeflateConfig {
            window_bits: 31,
            mem_level: 1,
            ..Default::default()
        };

        let mut stream = z_stream::default();
        let err = init(&mut stream, config);
        assert_eq!(err, ReturnCode::Ok);

        let mut stream = unsafe { DeflateStream::from_stream_mut(&mut stream) }.unwrap();
        let err = set_header(&mut stream, Some(&mut header));
        assert_eq!(err, ReturnCode::Ok);

        let input = b"Hello World\n";
        stream.next_in = input.as_ptr() as *mut _;
        stream.avail_in = input.len() as _;

        let mut output = [0u8; 256];
        stream.next_out = output.as_mut_ptr();
        stream.avail_out = output.len() as _;

        let err = deflate(stream, DeflateFlush::Finish);
        assert_eq!(err, ReturnCode::StreamEnd);
        assert!(end(stream).is_ok());
    }

In this case, looks like we trash the heap as libc SIGABRTs us with corrupted size vs. prev_size.

Similarly UB can be triggered by providing non-null-terminated strings to name and comment, e.g. by errantly using String::as_mut_ptr(). These are particularly nefarious because I've been able to write code that doesn't crash.

I'd suggest the following fixes:

  1. Mark set_header() as unsafe.
  2. Document the preconditions for a valid gz_header at its definition.
  3. Note in set_header() that the requirements for gz_header must be fulfilled.
  4. Also note in libz-rs-sys::deflateSetHeader() the requirements for gz_header must be fulfilled (right now it only notes that the pointer should be NULL or dereferencable).
  5. (Optionally provide a safe API for constructing a gz_header, but that might not be strictly required for this crate's use cases).

(I'm happy to make these fixes myself, just checking with you first).

folkertdev commented 1 month ago

That sounds good. I don't think that step 5 is needed, at least not right now.

Also set_header() is just not a great API from a rust perspective, but we need it to implement the C api, so marking it as unsafe looks like the right solution.