alexcrichton / curl-rust

Rust bindings to libcurl
MIT License
1k stars 234 forks source link

Unsound usages of unsafe implementation leading to potential use-after-free bug #561

Closed llooFlashooll closed 3 weeks ago

llooFlashooll commented 3 weeks ago

Hi, I am scanning the curl-rust in the latest version with my own static analyzer tool.

Use-after-free bug is found at: curl-rust/src/easy/handler.rs#L3274

unsafe {
    let mut len = 0;
    let p = curl_sys::curl_easy_unescape(
        self.inner.handle,
        s.as_ptr() as *const _,
        orig_len as c_int,
        &mut len,
    );
    assert!(!p.is_null());
    let slice = slice::from_raw_parts(p as *const u8, len as usize);
    let ret = slice.to_vec();
    curl_sys::curl_free(p as *mut _);
    ret
}

We can see that slice::from_raw_parts creates a alias of p with longer lifetime. However, curl_free is called to free the memory pointed by p. This would potentially lead to use-after-free bug.

This would potentially cause undefined behaviors in Rust. If we further manipulate the value ret, it would potentially lead to different consequences. I am reporting this issue for your attention.

alexcrichton commented 3 weeks ago

It's true that the return value of slice::from_raw_parts has an unbounded lifetime, but it is structurally only in use for the call of slice.to_vec() and is dynamically not accessed after that call. Given that while this has the possibility of UB I don't believe that it is directly, but that also sort of depends on exactly how you slice and dice unsafe Rust's guarantees. If you're able to model this in something like MIRI I think that would be great (it won't run as-is as I doubt MIRI knows about curl-the-system-library), but otherwise I think this is likely benign (unless you know of a concrete case where this is causing problems?)

llooFlashooll commented 3 weeks ago

I see, thanks for your instructive reply! It really depends on the case.