Unsafe code in Formatter is not panic safe

[Qwaz]

Hello,
we (Rust group @sslab-gatech) found a memory-safety/soundness issue in this crate while scanning Rust code on crates.io for potential vulnerabilities.

Issue Description

https://github.com/SergioBenitez/Rocket/blob/c24f15c18f02319be83af4f3c1951dc220b52c5e/core/http/src/uri/formatter.rs#L334-L357

with_prefix() method converts &str to &'static str with the justification above.
Unfortunately, the prefix is not popped if line 353 panics.
In such case, the transmuted &'static str persists in prefixes field and will lead to use-after-free if the formatter is accessed again.

Reproduction

Below is an example program that demonstrates use-after-free using safe APIs of rocket_http.

Show Detail

The program is expected to write hello=world.
However, due to the aforementioned use-after-free, it prints 12345=world instead.

#![forbid(unsafe_code)]

use rocket_http::uri::{Formatter, Query, UriDisplay};

struct MyDisplay;

struct MyValue;

impl UriDisplay<Query> for MyValue {
    fn fmt(&self, _f: &mut Formatter<Query>) -> std::fmt::Result {
        panic!()
    }
}

struct Wrapper<'a, 'b>(&'a mut Formatter<'b, Query>);

impl UriDisplay<Query> for MyDisplay {
    fn fmt(&self, formatter: &mut Formatter<Query>) -> std::fmt::Result {
        let wrapper = Wrapper(formatter);
        let temporary_string = String::from("hello");
        wrapper.0.write_named_value(&temporary_string, MyValue)
    }
}

impl<'a, 'b> Drop for Wrapper<'a, 'b> {
    fn drop(&mut self) {
        let _overlap = String::from("12345");
        self.0.write_raw("world").ok();
    }
}

fn main() {
    println!("{}", &MyDisplay as &dyn UriDisplay<Query>);
}

Output:

thread 'main' panicked at 'explicit panic', src/main.rs:28:9
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
12345=world
Return code 101

Tested Environment

• Crate: rocket_http
• Version: 0.4.6
• OS: Ubuntu 20.04.2 LTS
• Rustc version: rustc 1.51.0-nightly (04caa632d 2021-01-30)

[SergioBenitez]

This is a good find. I can confirm this is a soundness issue that remains present in 0.5. I am working on a fix now.

The impact of this bug is minimal at worst. We already advise users to #[derive(UriDisplay)], so custom implementations of UriDisplay are unlikely, which are necessary to trigger this issue. Furthermore, a panic!(), direct or indirect, is unlikely to appear in a UriDisplay implementation, considering its purpose. Thus, the chance for an issue to arise as a result is minimal.

Nevertheless, the implementation is incorrect. I will be rolling out a fix in a new release immediately.

[SergioBenitez]

Rocket 0.4.7, just released, includes the fix in e325e2f.

[Qwaz]

Thanks for the quick fix! Would you mind requesting an unsound advisory to RustSec advisory database about this bug? An unsound advisory is used to represent a bug that has security implications but unlikely to be triggered in normal cases (like this one) and surfaces as a warning instead of an error in security auditing tools. We usually file RustSec advisory request by ourselves for minor crates, but it would be great if it can be written by someone who has more domain knowledge about how the crate is used in this case. An advisory can be marked as an unsound advisory with informational = "unsound" line in the [advisory] metadata section (it is currently not documented in the advisory example in README).