Clone implementation for LocalRequest is unsound

[Qwaz]

The Bug

LocalRequest is one of a few places that Rocket uses unsafe Rust. While auditing the code, I found that its Clone implementation is unsound.

First, let's look at the definition of LocalRequest, which implements a mutable Rc with a raw pointer:
https://github.com/SergioBenitez/Rocket/blob/ca4d1572d408fd8dd13db103926ad96da878126d/core/lib/src/local/request.rs#L68-L100

The comment justifies that it is safe to have LocalRequest and LocalResponse which share the same Request pointer because modification from LocalRequest is not observable from LocalResponse. However, LocalRequest's Clone implementation allows to create two LocalRequest that share the same Request raw pointer, which was not part of the justification:
https://github.com/SergioBenitez/Rocket/blob/ca4d1572d408fd8dd13db103926ad96da878126d/core/lib/src/local/request.rs#L477-L487

This ultimately permits the violation of aliasing rule with two LocalRequest instances that point to the same Request instance.

Proof of Concept

PoC code:

use rocket::http::Header;
use rocket::local::Client;
use rocket::Request;

fn main() {
    let client = Client::new(rocket::ignite()).unwrap();

    // creates two LocalRequest instances that share the same Request pointer
    let request1 = client.get("/").header(Header::new("key", "val1"));
    let request2 = request1.clone();

    // sanity check
    assert_eq!(
        request1.inner() as *const Request<'_>,
        request2.inner() as *const Request<'_>
    );

    // save the iterator, which internally holds a slice
    let mut iter = request1.inner().headers().get("key");

    // insert headers to reallocate the header map
    request2
        .header(Header::new("1", "v1"))
        .header(Header::new("2", "v2"))
        .header(Header::new("3", "v3"))
        .header(Header::new("4", "v4"))
        .header(Header::new("5", "v5"))
        .header(Header::new("6", "v6"))
        .header(Header::new("key", "val2"));

    // heap massage
    let _: Vec<usize> = vec![0, 0xcafebabe, 31337, 0];

    // iter is dangling now!
    let s = iter.next().unwrap();

    // address and length controlled
    dbg!(s.as_ptr());
    dbg!(s.len());

    // segfaults
    println!("{}", s);
}

Output:

   Compiling rocket-unsound v0.1.0 (/home/yechan/rust/rocket-unsound)
    Finished dev [unoptimized + debuginfo] target(s) in 1.33s
     Running `target/debug/rocket-unsound`
🔧 Configured for development.
    => address: localhost
    => port: 8000
    => log: normal
    => workers: 32
    => secret key: generated
    => limits: forms = 32KiB
    => keep-alive: 5s
    => tls: disabled
[src/main.rs:38] s.as_ptr() = 0x00000000cafebabe
[src/main.rs:39] s.len() = 31337
zsh: segmentation fault (core dumped)  cargo run

The PoC was tested on the following environment:

• Rocket: v0.4.4 (latest at the time of writing)
• OS: Ubuntu 18.04.4 LTS
• Rust: rustc 1.45.0-nightly (a74d1862d 2020-05-14)
• Target: x86_64-unknown-linux-gnu

The PoC used a classic iterator invalidation to show it is possible to overwrite the pointer and the length of a string slice.

Is this a security bug?

Considering that

1. LocalRequest is intended to be used for testing
2. The exact pattern to trigger the bug is unlikely to happen in the wild

I believe the security impact of this bug is minimal. I think It is still worthwhile to file a RustSec advisory after the bug confirmation considering the people who want to be notified about unsound APIs.

Possible Fixes

I believe LocalRequest can be redesigned without breaking LocalRequest API with safe alternatives such as get_mut() or make_mut().

I think it is also possible to fix Clone to clone the internal Request, but the first solution which reduces the number of unsafe code should be preferred if possible.

[SergioBenitez]

Absolutely fantastic find. I can confirm that this is, indeed, a soundness bug. The use of unsafe in LocalRequest is, by far, the most worrisome.

The comment justifies that it is safe to have LocalRequest and LocalResponse which share the same Request pointer because modification from LocalRequest is not observable from LocalResponse. However, LocalRequest's Clone implementation allows to create two LocalRequest that share the same Request raw pointer, which was not part of the justification:

This "proof" was written and argued on an earlier version of LocalRequest in which the Clone implementation did not exist. Previously, we had cloned_dispatch, which operated correctly: 56c6a96#diff-108b840beda9b69a05bed320c98f260bL342-L364.

The fragility of this and other unsafe-soundness proofs in Rocket are that they are not machine checked. I had taken great care when writing the proof originally to check that it was correct, but future changes to the code are not subject to any machine-checked scrutiny, and recalling all cases in the original proof structure in response to a code change is a difficult proposition. As evidenced here, we clearly need to do better.

Is this a security bug?

Considering that

1. LocalRequest is intended to be used for testing
2. The exact pattern to trigger the bug is unlikely to happen in the wild

I believe the security impact of this bug is minimal. I think It is still worthwhile to file a RustSec advisory after the bug confirmation considering the people who want to be notified about unsound APIs.

I am happy to have a RustSec advisory filed.

I think it is also possible to fix Clone to clone the internal Request [...].

This is the approach I've taken in fixing this particular issue.

Possible Fixes

I believe LocalRequest can be redesigned without breaking LocalRequest API with safe alternatives such as get_mut() or make_mut().

This is the approach I'd like to take. I am not sure if the safe Rc APIs are enough to implement the existing LocalRequest/Response API. While get_mut and make_mut solve one part of the puzzle, you'll note that we also use unsafe to "extend" the lifetime of a Request. Given that we make no use of this extension, such an extension is safe, but I do not know how to circumvent the need to do this, nor do I know of a way to emulate this in safe code. I am happy to pay a performance cost to remove unsafe in LocalRequest, and I'd be happy for any ideas in this direction.