From b8f9011c04f104bb4a20f44e5bce31a1e9b46e64 Mon Sep 17 00:00:00 2001
From: Sergio Benitez <sb@sergio.bz>
Date: Wed, 27 May 2020 01:09:12 -0700
Subject: [PATCH] Fix 'LocalRequest::clone()' soundness issue.

The existing implementation of 'LocalRequest::clone()' mistakenly copied
the internal 'Request' pointer from the existing 'LocalRequest' to the
cloned 'LocalRequest'. This resulted in an aliased '*mut Request'
pointer, a clear soundness issue. The fix in this commit is to clone the
internal 'Request', replacing the internal pointer with the newly cloned
'Request' when producing the cloned 'LocalRequest'. A fix that removes
all 'unsafe' code should be explored.

Fixes #1312.
---
 core/lib/src/local/request.rs                | 24 +++++++++++---
 core/lib/tests/unsound-local-request-1312.rs | 33 ++++++++++++++++++++
 2 files changed, 53 insertions(+), 4 deletions(-)
 create mode 100644 core/lib/tests/unsound-local-request-1312.rs

diff --git a/core/lib/src/local/request.rs b/core/lib/src/local/request.rs
index 3e14c3b3..887c22f4 100644
--- a/core/lib/src/local/request.rs
+++ b/core/lib/src/local/request.rs
@@ -476,19 +476,35 @@ impl fmt::Debug for LocalResponse<'_> {
 
 impl<'c> Clone for LocalRequest<'c> {
     fn clone(&self) -> LocalRequest<'c> {
+        // Don't alias the existing `Request`. See #1312.
+        let mut request = Rc::new(self.inner().clone());
+        let ptr = Rc::get_mut(&mut request).unwrap() as *mut Request<'_>;
+
         LocalRequest {
+            ptr, request,
             client: self.client,
-            ptr: self.ptr,
-            request: self.request.clone(),
             data: self.data.clone(),
             uri: self.uri.clone()
         }
     }
 }
 
-// #[cfg(test)]
+#[cfg(test)]
 mod tests {
-    // Someday...
+    use crate::Request;
+    use crate::local::Client;
+
+    #[test]
+    fn clone_unique_ptr() {
+        let client = Client::new(crate::ignite()).unwrap();
+        let r1 = client.get("/");
+        let r2 = r1.clone();
+
+        assert_ne!(
+            r1.inner() as *const Request<'_>,
+            r2.inner() as *const Request<'_>
+        );
+    }
 
     // #[test]
     // #[compile_fail]
diff --git a/core/lib/tests/unsound-local-request-1312.rs b/core/lib/tests/unsound-local-request-1312.rs
new file mode 100644
index 00000000..b373ac24
--- /dev/null
+++ b/core/lib/tests/unsound-local-request-1312.rs
@@ -0,0 +1,33 @@
+use rocket::http::Header;
+use rocket::local::Client;
+
+#[test]
+fn test_local_request_clone_soundness() {
+    let client = Client::new(rocket::ignite()).unwrap();
+
+    // creates two LocalRequest instances that shouldn't share the same req
+    let r1 = client.get("/").header(Header::new("key", "val1"));
+    let mut r2 = r1.clone();
+
+    // save the iterator, which internally holds a slice
+    let mut iter = r1.inner().headers().get("key");
+
+    // insert headers to force header map reallocation.
+    for i in 0..100 {
+        r2.add_header(Header::new(i.to_string(), i.to_string()));
+    }
+
+    // Replace the original key/val.
+    r2.add_header(Header::new("key", "val2"));
+
+    // Heap massage: so we've got crud to print.
+    let _: Vec<usize> = vec![0, 0xcafebabe, 31337, 0];
+
+    // Ensure we're good.
+    let s = iter.next().unwrap();
+    println!("{}", s);
+
+    // And that we've got the right data.
+    assert_eq!(r1.inner().headers().get("key").collect::<Vec<_>>(), vec!["val1"]);
+    assert_eq!(r2.inner().headers().get("key").collect::<Vec<_>>(), vec!["val1", "val2"]);
+}