Merge pull request #90 from demarches-simplifiees/ywh

prevent traversal attack

src/config.rs

@@ -34,7 +34,7 @@ pub struct EncryptConfig {
 
 #[derive(Debug, Clone)]
 pub struct HttpConfig {
-    pub upstream_base_url: String,
+    pub upstream_base_url: Url,
     pub keyring: Keyring,
     pub chunk_size: usize,
     pub address: SocketAddr,
@@ -121,14 +121,16 @@ impl Config {
                 )
             });
 
-            let upstream_base_url = match &args.flag_upstream_url {
+            let raw_upstream_base_url = match &args.flag_upstream_url {
                 Some(upstream_url) => Some(upstream_url.to_string()),
                 None => Some(env::var("DS_UPSTREAM_URL").expect(
                     "Missing upstream_url, use DS_UPSTREAM_URL env or --upstream-url cli argument",
                 )),
             }
             .unwrap();
 
+            let upstream_base_url = normalize_and_parse_upstream_url(raw_upstream_base_url);
+
             let address = match &args.flag_address {
                 Some(address) => match address.to_socket_addrs() {
                     Ok(mut sockets) => Some(sockets.next().unwrap()),
@@ -155,16 +157,36 @@ impl Config {
     }
 }
 
+// ensure upstream_url ends with a "/ to avoid
+// upstream url: "https://upstream/dir"
+// request: "https://proxy/file"
+// "https://upstream/dir".join('file') => https://upstream/file
+// instead ".../upstream/dir/".join('file') => https://upstream/dir/file
+fn normalize_and_parse_upstream_url(mut url: String) -> Url {
+    if !url.ends_with('/') {
+        url.push('/');
+    }
+    Url::parse(&url).unwrap()
+}
+
 impl HttpConfig {
-    pub fn create_upstream_url(&self, req: &HttpRequest) -> String {
-        let base = Url::parse(self.upstream_base_url.as_ref()).unwrap();
-        let mut url = base.join(&req.match_info()["name"]).unwrap();
+    pub fn create_upstream_url(&self, req: &HttpRequest) -> Option<String> {
+        // Warning: join process '../'
+        // "https://a.com/jail/".join('../escape') => "https://a.com/escape"
+        let mut url = self
+            .upstream_base_url
+            .join(&req.match_info()["name"])
+            .unwrap();
+
+        if self.is_traversal_attack(&url) {
+            return None;
+        }
 
         if !req.query_string().is_empty() {
             url.set_query(Some(req.query_string()));
         }
 
-        url.to_string()
+        Some(url.to_string())
     }
 
     pub fn local_encryption_path_for(&self, req: &HttpRequest) -> PathBuf {
@@ -173,6 +195,27 @@ impl HttpConfig {
         filepath.push(name);
         filepath
     }
+
+    fn is_traversal_attack(&self, url: &Url) -> bool {
+        // https://upstream.com => [Some("")]
+        // https://upstream.com/jail/cell/ => [Some("jail"), Some("cell"), Some("")]
+        let mut base_segments: Vec<&str> =
+            self.upstream_base_url.path_segments().unwrap().collect();
+
+        // remove the last segment corresponding to "/"
+        base_segments.pop();
+
+        let mut url_segments = url.path_segments().unwrap();
+
+        // ensure that all segment of the upstream_base_url
+        // are present in the final url
+        let safe = base_segments.iter().all(|base_segment| {
+            let url_segment = url_segments.next().unwrap();
+            base_segment == &url_segment
+        });
+
+        !safe
+    }
 }
 
 fn read_file_content(path_string: &str) -> String {
@@ -189,36 +232,75 @@ mod tests {
     use super::*;
     use actix_web::test::TestRequest;
 
+    #[test]
+    fn test_normalize_and_parse_upstream_url() {
+        assert_eq!(
+            normalize_and_parse_upstream_url("https://upstream.com/dir".to_string()),
+            Url::parse("https://upstream.com/dir/").unwrap()
+        );
+    }
+
     #[test]
     fn test_create_upstream_url() {
-        let req = TestRequest::default()
-            .uri("https://proxy.com/bucket/file.zip?p1=ok1&p2=ok2")
-            .param("name", "bucket/file.zip") // hack to force parsing
+        let base = "https://upstream.com/";
+        let jailed_base = "https://upstream.com/jail/cell/";
+
+        let config = default_config(base);
+        let jailed_config = default_config(jailed_base);
+
+        let file = TestRequest::default()
+            .uri("https://proxy.com/file")
+            .param("name", "file") // hack to force parsing
+            .to_http_request();
+
+        assert_eq!(
+            config.create_upstream_url(&file),
+            Some("https://upstream.com/file".to_string())
+        );
+
+        assert_eq!(
+            jailed_config.create_upstream_url(&file),
+            Some("https://upstream.com/jail/cell/file".to_string())
+        );
+
+        let sub_dir_file = TestRequest::default()
+            .uri("https://proxy.com/sub/dir/file")
+            .param("name", "sub/dir/file") // hack to force parsing
             .to_http_request();
 
         assert_eq!(
-            default_config("https://upstream.com").create_upstream_url(&req),
-            "https://upstream.com/bucket/file.zip?p1=ok1&p2=ok2"
+            config.create_upstream_url(&sub_dir_file),
+            Some("https://upstream.com/sub/dir/file".to_string())
         );
 
         assert_eq!(
-            default_config("https://upstream.com/").create_upstream_url(&req),
-            "https://upstream.com/bucket/file.zip?p1=ok1&p2=ok2"
+            jailed_config.create_upstream_url(&sub_dir_file),
+            Some("https://upstream.com/jail/cell/sub/dir/file".to_string())
         );
 
+        let path_traversal_file = TestRequest::default()
+            .uri("https://proxy.com/../escape")
+            .param("name", "../escape") // hack to force parsing
+            .to_http_request();
+
         assert_eq!(
-            default_config("https://upstream.com/sub_folder/").create_upstream_url(&req),
-            "https://upstream.com/sub_folder/bucket/file.zip?p1=ok1&p2=ok2"
+            config.create_upstream_url(&path_traversal_file),
+            Some("https://upstream.com/escape".to_string())
         );
 
-        let req = TestRequest::default()
-            .uri("https://proxy.com/bucket/file.zip")
+        assert_eq!(
+            jailed_config.create_upstream_url(&path_traversal_file),
+            None
+        );
+
+        let file_with_query_string = TestRequest::default()
+            .uri("https://proxy.com/bucket/file.zip?p1=ok1&p2=ok2")
             .param("name", "bucket/file.zip") // hack to force parsing
             .to_http_request();
 
         assert_eq!(
-            default_config("https://upstream.com").create_upstream_url(&req),
-            "https://upstream.com/bucket/file.zip"
+            config.create_upstream_url(&file_with_query_string),
+            Some("https://upstream.com/bucket/file.zip?p1=ok1&p2=ok2".to_string())
         );
     }
 
@@ -228,7 +310,7 @@ mod tests {
         HttpConfig {
             keyring,
             chunk_size: DEFAULT_CHUNK_SIZE,
-            upstream_base_url: upstream_base_url.to_string(),
+            upstream_base_url: normalize_and_parse_upstream_url(upstream_base_url.to_string()),
             address: "127.0.0.1:1234".to_socket_addrs().unwrap().next().unwrap(),
             local_encryption_directory: PathBuf::from(DEFAULT_LOCAL_ENCRYPTION_DIRECTORY),
         }

src/http/handlers/fetch.rs

@@ -11,8 +11,12 @@ pub async fn fetch(
 ) -> Result<HttpResponse, Error> {
     let get_url = config.create_upstream_url(&req);
 
+    if get_url.is_none() {
+        return not_found();
+    }
+
     let mut fetch_req = client
-        .request_from(get_url.as_str(), req.head())
+        .request_from(get_url.unwrap(), req.head())
         .force_close();
 
     let raw_range = req

src/http/handlers/forward.rs

@@ -30,8 +30,12 @@ pub async fn forward(
 ) -> Result<HttpResponse, Error> {
     let put_url = config.create_upstream_url(&req);
 
+    if put_url.is_none() {
+        return not_found();
+    }
+
     let mut forwarded_req = client
-        .request_from(put_url.as_str(), req.head())
+        .request_from(put_url.unwrap(), req.head())
         .force_close()
         .timeout(UPLOAD_TIMEOUT);
 

src/http/handlers/mod.rs

@@ -36,3 +36,11 @@ pub static FETCH_REQUEST_HEADERS_TO_REMOVE: [header::HeaderName; 2] = [
     header::CONNECTION,
     header::RANGE,
 ];
+
+pub fn not_found() -> Result<HttpResponse, Error> {
+    let response = HttpResponse::NotFound()
+        .insert_header((header::CONTENT_TYPE, "application/json"))
+        .body("{}");
+
+    Ok(response)
+}

src/http/handlers/simple_proxy.rs

@@ -8,7 +8,11 @@ pub async fn simple_proxy(
 ) -> Result<HttpResponse, Error> {
     let url = config.create_upstream_url(&req);
 
-    let mut proxied_req = client.request_from(url.as_str(), req.head()).force_close();
+    if url.is_none() {
+        return not_found();
+    }
+
+    let mut proxied_req = client.request_from(url.unwrap(), req.head()).force_close();
 
     for header in &FETCH_REQUEST_HEADERS_TO_REMOVE {
         proxied_req.headers_mut().remove(header);

tests/concurrent_uploads.rs

@@ -51,8 +51,10 @@ fn concurent_uploads() {
 
                 let stored_filename = Uuid::new_v4();
                 let stored_file_url = format!("localhost:4444/upstream/{}", stored_filename);
-                let uploaded_path =
-                    format!("tests/fixtures/server-static/uploads/{}", stored_filename);
+                let uploaded_path = format!(
+                    "tests/fixtures/server-static/uploads/jail/cell/{}",
+                    stored_filename
+                );
 
                 let temp = assert_fs::TempDir::new().unwrap();
                 let decrypted_file = temp.child("computer.dec.svg");
@@ -79,7 +81,7 @@ fn concurent_uploads() {
                 assert_eq!(curl_socket_download.stdout, COMPUTER_SVG_BYTES);
 
                 let curl_chunked_download = curl_get(&format!(
-                    "localhost:4444/upstream/chunked/{}",
+                    "localhost:4444/upstream/{}?chunked=true",
                     stored_filename
                 ));
                 assert_eq!(curl_chunked_download.stdout.len(), COMPUTER_SVG_BYTES.len());

tests/content_length_and_transfert_encoding.rs

@@ -50,7 +50,7 @@ async fn uploaded_and_downloaded_content_length(content: &[u8]) -> (usize, usize
 
     curl_put("/tmp/foo", "localhost:4444/upstream/file");
 
-    let last_put_headers = curl_get("localhost:4444/upstream/last_put_headers").stdout;
+    let last_put_headers = curl_get("localhost:3333/last_put_headers").stdout;
 
     let deserialized: TestHeaders = serde_json::from_slice(&last_put_headers).unwrap();
 

tests/download_witness_file.rs

@@ -13,7 +13,7 @@ fn download_witness_file() {
      - copy a witness file in the right directory to be downloaded
      - downloads the uploaded file via the proxy, and checks that its content matches the initial content
     */
-    let uploaded_path = "tests/fixtures/server-static/uploads/computer.svg.enc";
+    let uploaded_path = "tests/fixtures/server-static/uploads/jail/cell/computer.svg.enc";
 
     std::fs::copy(ENCRYPTED_COMPUTER_SVG_PATH, uploaded_path).expect("copy failed");
 

tests/fixtures/server-static/server.js

@@ -52,15 +52,6 @@ app.get('/last_put_headers', function(req, res){
   res.json(last_put_headers);
 });
 
-app.get('/chunked/*', function(req, res){
-  const path = req.url.substr(8)
-
-  const readStream = fs.createReadStream(__dirname + '/uploads/' + path, { highWaterMark: 1 * 1024});
-
-  res.writeHead(200, {'Content-Type': 'text/plain'});
-  readStream.pipe(res);
-});
-
 app.get('/get/500', function(req, res){
   res.writeHead(500, {'Content-Type': 'text/plain'});
   res.end('KO: 500');
@@ -71,5 +62,18 @@ app.get('/get/400', function(req, res){
   res.end('KO: 400');
 });
 
+// return a file by chunked if the query param chunked is present
+const chunked_static = function (req, res, next) {
+  if (!req.query.chunked) {
+    return next();
+  }
+
+  const path = req.path.substr(1);
+  const readStream = fs.createReadStream(__dirname + '/uploads/' + path, { highWaterMark: 1 * 1024});
+  res.writeHead(200, {'Content-Type': 'text/plain'});
+  readStream.pipe(res);
+}
+
+app.use(chunked_static);
 app.use(express.static(__dirname + '/uploads'));
 app.listen(3333);

tests/fixtures/server-static/uploads/out_of_jail.txt

@@ -0,0 +1 @@
+fail

tests/helpers/curl.rs

@@ -5,6 +5,7 @@ pub fn curl_get_status(url: &str) -> String {
     let stdout = Command::new("curl")
         .arg("-XGET")
         .arg(url)
+        .arg("--path-as-is")
         .arg("-o")
         .arg("/dev/null")
         .arg("-s")

tests/helpers/mod.rs

@@ -66,7 +66,7 @@ pub fn launch_proxy(log: PrintServerLogs, keyring_path: Option<&str>) -> ChildGu
     command
         .arg("proxy")
         .arg("--address=localhost:4444")
-        .arg("--upstream-url=http://localhost:3333")
+        .arg("--upstream-url=http://localhost:3333/jail/cell")
         .env("DS_KEYRING", keyring)
         .env("DS_PASSWORD", PASSWORD)
         .env("DS_SALT", SALT)

tests/keyring.rs

@@ -24,7 +24,10 @@ fn multiple_keys() {
         add_a_key(keyring_path);
 
         let upload_url = format!("localhost:4444/upstream/victory_{}", i);
-        let upload_path = format!("tests/fixtures/server-static/uploads/victory_{}", i);
+        let upload_path = format!(
+            "tests/fixtures/server-static/uploads/jail/cell/victory_{}",
+            i
+        );
         ensure_is_absent(&upload_path);
 
         let _proxy_and_node = ProxyAndNode::start_with_keyring_path(keyring_path);

tests/traversal_attack.rs

@@ -0,0 +1,14 @@
+use serial_test::serial;
+
+mod helpers;
+pub use helpers::*;
+
+#[test]
+#[serial(servers)]
+fn traversal_attack_is_avoided() {
+    let _proxy_and_node = ProxyAndNode::start();
+
+    let curl_download = curl_get_status("localhost:4444/upstream/../../out_of_jail.txt");
+    println!("curl_download: {:?}", curl_download);
+    assert_eq!(curl_download, "404");
+}

tests/upload_and_download.rs

@@ -17,7 +17,7 @@ fn upload_and_download() {
      - decrypt the uploaded file by the decrypted command and check the result
      - downloads the uploaded file via the proxy, and checks that its content matches the initial content
     */
-    let uploaded_path = "tests/fixtures/server-static/uploads/victory";
+    let uploaded_path = "tests/fixtures/server-static/uploads/jail/cell/victory";
 
     let temp = assert_fs::TempDir::new().unwrap();
     let decrypted_file = temp.child("computer.dec.svg");
@@ -45,7 +45,7 @@ fn upload_and_download() {
     let curl_socket_download = curl_socket_get("localhost:4444/upstream/victory");
     assert_eq!(curl_socket_download.stdout, COMPUTER_SVG_BYTES);
 
-    let curl_chunked_download = curl_get("localhost:4444/upstream/chunked/victory");
+    let curl_chunked_download = curl_get("localhost:4444/upstream/victory?chunked=true");
     assert_eq!(curl_chunked_download.stdout, COMPUTER_SVG_BYTES);
 
     temp.close().unwrap();