diff --git a/src/config.rs b/src/config.rs index 3217975..0b19946 100644 --- a/src/config.rs +++ b/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,7 +121,7 @@ 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", @@ -129,6 +129,8 @@ impl Config { } .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 { + // 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), } diff --git a/src/http/handlers/fetch.rs b/src/http/handlers/fetch.rs index 50d756e..5c1596a 100644 --- a/src/http/handlers/fetch.rs +++ b/src/http/handlers/fetch.rs @@ -11,8 +11,12 @@ pub async fn fetch( ) -> Result { 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 diff --git a/src/http/handlers/forward.rs b/src/http/handlers/forward.rs index 0751ba8..e4e797a 100644 --- a/src/http/handlers/forward.rs +++ b/src/http/handlers/forward.rs @@ -30,8 +30,12 @@ pub async fn forward( ) -> Result { 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); diff --git a/src/http/handlers/mod.rs b/src/http/handlers/mod.rs index 278d33e..d12f052 100644 --- a/src/http/handlers/mod.rs +++ b/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 { + let response = HttpResponse::NotFound() + .insert_header((header::CONTENT_TYPE, "application/json")) + .body("{}"); + + Ok(response) +} diff --git a/src/http/handlers/simple_proxy.rs b/src/http/handlers/simple_proxy.rs index 49b352d..1cd7ee0 100644 --- a/src/http/handlers/simple_proxy.rs +++ b/src/http/handlers/simple_proxy.rs @@ -8,7 +8,11 @@ pub async fn simple_proxy( ) -> Result { 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); diff --git a/tests/concurrent_uploads.rs b/tests/concurrent_uploads.rs index 1584c05..9a17853 100644 --- a/tests/concurrent_uploads.rs +++ b/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()); diff --git a/tests/content_length_and_transfert_encoding.rs b/tests/content_length_and_transfert_encoding.rs index e3724fa..b284ca0 100644 --- a/tests/content_length_and_transfert_encoding.rs +++ b/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(); diff --git a/tests/download_witness_file.rs b/tests/download_witness_file.rs index 6cea158..09151b8 100644 --- a/tests/download_witness_file.rs +++ b/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"); diff --git a/tests/fixtures/server-static/server.js b/tests/fixtures/server-static/server.js index 3e14ed4..76a0e7c 100644 --- a/tests/fixtures/server-static/server.js +++ b/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); diff --git a/tests/fixtures/server-static/uploads/out_of_jail.txt b/tests/fixtures/server-static/uploads/out_of_jail.txt new file mode 100644 index 0000000..9dae856 --- /dev/null +++ b/tests/fixtures/server-static/uploads/out_of_jail.txt @@ -0,0 +1 @@ +fail diff --git a/tests/helpers/curl.rs b/tests/helpers/curl.rs index 1d9c583..78827a1 100644 --- a/tests/helpers/curl.rs +++ b/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") diff --git a/tests/helpers/mod.rs b/tests/helpers/mod.rs index 47feed5..862bd67 100644 --- a/tests/helpers/mod.rs +++ b/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) diff --git a/tests/keyring.rs b/tests/keyring.rs index 23550ef..00e3d3a 100644 --- a/tests/keyring.rs +++ b/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); diff --git a/tests/traversal_attack.rs b/tests/traversal_attack.rs new file mode 100644 index 0000000..10ccf78 --- /dev/null +++ b/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"); +} diff --git a/tests/upload_and_download.rs b/tests/upload_and_download.rs index a66639d..6ad5685 100644 --- a/tests/upload_and_download.rs +++ b/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();