Fix 'LocalRequest::clone()' soundness issue.

SergioBenitez · SergioBenitez · commit 89150f9b8128 · 2020-05-29T01:53:06.000-07:00
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.
diff --git a/core/lib/src/local/request.rs b/core/lib/src/local/request.rs
@@ -476,19 +476,35 @@ impl<'c> fmt::Debug for LocalResponse<'c> {
 
 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
@@ -0,0 +1,35 @@
+extern crate rocket;
+
+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"]);
+}
