Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unsafe code in Formatter is not panic safe #1534

Closed
Qwaz opened this issue Feb 9, 2021 · 3 comments
Closed

Unsafe code in Formatter is not panic safe #1534

Qwaz opened this issue Feb 9, 2021 · 3 comments
Labels
bug Deviation from the specification or expected behavior

Comments

@Qwaz
Copy link

Qwaz commented Feb 9, 2021

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 SergioBenitez added the bug Deviation from the specification or expected behavior label Feb 9, 2021
@SergioBenitez
Copy link
Member

SergioBenitez commented Feb 10, 2021

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
Copy link
Member

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

@Qwaz
Copy link
Author

Qwaz commented Feb 10, 2021

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Deviation from the specification or expected behavior
Projects
None yet
Development

No branches or pull requests

2 participants