From 699e17c7de0b40c1e5f7e4171683710378e4af58 Mon Sep 17 00:00:00 2001 From: Ali MJ Al-Nasrawy Date: Tue, 23 Mar 2021 22:42:28 +0300 Subject: [PATCH] file_upload.rs: sanitize path input Signed-off-by: Ali MJ Al-Nasrawy --- src/file_upload.rs | 52 +++++++++++++++++++++------ tests/upload_files.rs | 81 ++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 122 insertions(+), 11 deletions(-) diff --git a/src/file_upload.rs b/src/file_upload.rs index 5f9738cb0..2319b6a9d 100644 --- a/src/file_upload.rs +++ b/src/file_upload.rs @@ -2,7 +2,7 @@ use actix_web::{http::header, HttpRequest, HttpResponse}; use futures::TryStreamExt; use std::{ io::Write, - path::{Component, PathBuf}, + path::{Component, Path, PathBuf}, }; use crate::errors::ContextualError; @@ -37,7 +37,7 @@ async fn save_file( /// Create new future to handle file as multipart data. async fn handle_multipart( field: actix_multipart::Field, - file_path: PathBuf, + path: PathBuf, overwrite_files: bool, ) -> Result { let filename = field @@ -50,21 +50,25 @@ async fn handle_multipart( ) })?; - match std::fs::metadata(&file_path) { + let filename = sanitize_path(Path::new(&filename), false).ok_or_else(|| { + ContextualError::InvalidPathError("Invalid file name to upload".to_string()) + })?; + + match std::fs::metadata(&path) { Err(_) => Err(ContextualError::InsufficientPermissionsError( - file_path.display().to_string(), + path.display().to_string(), )), Ok(metadata) if !metadata.is_dir() => Err(ContextualError::InvalidPathError(format!( "cannot upload file to {}, since it's not a directory", - &file_path.display() + &path.display() ))), Ok(metadata) if metadata.permissions().readonly() => Err( - ContextualError::InsufficientPermissionsError(file_path.display().to_string()), + ContextualError::InsufficientPermissionsError(path.display().to_string()), ), Ok(_) => Ok(()), }?; - save_file(field, file_path.join(filename), overwrite_files).await + save_file(field, path.join(filename), overwrite_files).await } /// Handle incoming request to upload file. @@ -87,9 +91,9 @@ pub async fn upload_file( let upload_path = query_params.path.as_ref().ok_or_else(|| { ContextualError::InvalidHttpRequestError("Missing query parameter 'path'".to_string()) })?; - let upload_path = upload_path - .strip_prefix(Component::RootDir) - .unwrap_or(upload_path); + let upload_path = sanitize_path(upload_path, conf.show_hidden).ok_or_else(|| { + ContextualError::InvalidPathError("Invalid value for 'path' parameter".to_string()) + })?; let app_root_dir = conf.path.canonicalize().map_err(|e| { ContextualError::IoError("Failed to resolve path served by miniserve".to_string(), e) @@ -97,6 +101,7 @@ pub async fn upload_file( // If the target path is under the app root directory, save the file. let target_dir = match app_root_dir.join(upload_path).canonicalize() { + Ok(path) if !conf.no_symlinks => Ok(path), Ok(path) if path.starts_with(&app_root_dir) => Ok(path), _ => Err(ContextualError::InvalidHttpRequestError( "Invalid value for 'path' parameter".to_string(), @@ -113,3 +118,30 @@ pub async fn upload_file( .append_header((header::LOCATION, return_path)) .finish()) } + +/// Guarantee that the path is relative and cannot traverse back to parent directories +/// and optionally prevent traversing hidden directories. +fn sanitize_path(path: &Path, traverse_hidden: bool) -> Option { + let mut buf = PathBuf::new(); + + for comp in path.components() { + match comp { + Component::Normal(name) => buf.push(name), + Component::ParentDir => { + buf.pop(); + } + _ => (), + } + } + + // Double-check that all components are Normal and check for hidden dirs + for comp in buf.components() { + match comp { + Component::Normal(_) if traverse_hidden => (), + Component::Normal(name) if !name.to_str()?.starts_with('.') => (), + _ => return None, + } + } + + Some(buf) +} diff --git a/tests/upload_files.rs b/tests/upload_files.rs index 698eb4626..26750d939 100644 --- a/tests/upload_files.rs +++ b/tests/upload_files.rs @@ -1,6 +1,7 @@ mod fixtures; -use fixtures::{server, Error, TestServer}; +use assert_fs::fixture::TempDir; +use fixtures::{server, server_no_stderr, tmpdir, Error, TestServer}; use reqwest::blocking::{multipart, Client}; use rstest::rstest; use select::document::Document; @@ -78,3 +79,81 @@ fn uploading_files_is_prevented(server: TestServer) -> Result<(), Error> { Ok(()) } + +#[rstest] +#[case("foo", "bar", "foo/bar")] +#[case("/../foo", "bar", "foo/bar")] +#[case("/foo", "/../bar", "foo/bar")] +#[case("C:/foo", "C:/bar", if cfg!(windows) { "foo/bar" } else { "C:/foo/C:/bar" })] +#[case(r"C:\foo", r"C:\bar", if cfg!(windows) { "foo/bar" } else { r"C:\foo/C:\bar" })] +#[case(r"\foo", r"\..\bar", if cfg!(windows) { "foo/bar" } else { r"\foo/\..\bar" })] +fn path_traversal( + #[with(&["-u"])] server: TestServer, + #[case] path: &str, + #[case] filename: &'static str, + #[case] expected: &str, +) -> Result<(), Error> { + // create test directories + use std::fs::create_dir_all; + create_dir_all(server.path().join("foo")).unwrap(); + if !cfg!(windows) { + for dir in &["C:/foo/C:", r"C:\foo", r"\foo"] { + create_dir_all(server.path().join(dir)).expect(&format!("failed to create: {:?}", dir)); + } + } + + let expected_path = server.path().join(expected); + assert!(!expected_path.exists()); + + // Perform the actual upload. + let part = multipart::Part::text("this should be uploaded") + .file_name(filename) + .mime_str("text/plain")?; + let form = multipart::Form::new().part("file_to_upload", part); + + Client::new() + .post(server.url().join(&format!("/upload?path={}", path))?) + .multipart(form) + .send()? + .error_for_status()?; + + // Make sure that the file was uploaded to the expected path + assert!(expected_path.exists()); + + Ok(()) +} + +#[rstest] +#[case(server(&["-u"]), true)] +#[case(server_no_stderr(&["-u", "--no-symlinks"]), false)] +fn symlink(#[case] server: TestServer, #[case] ok: bool, tmpdir: TempDir) -> Result<(), Error> { + #[cfg(unix)] + use std::os::unix::fs::symlink as symlink_dir; + #[cfg(windows)] + use std::os::windows::fs::symlink_dir; + + // create symlink directory "foo" to point outside the root + let (dir, filename) = ("foo", "bar"); + symlink_dir(tmpdir.path(), server.path().join(dir)).unwrap(); + + let full_path = server.path().join(dir).join(filename); + assert!(!full_path.exists()); + + // try to upload + let part = multipart::Part::text("this should be uploaded") + .file_name(filename) + .mime_str("text/plain")?; + let form = multipart::Form::new().part("file_to_upload", part); + + let status = Client::new() + .post(server.url().join(&format!("/upload?path={}", dir))?) + .multipart(form) + .send()? + .error_for_status(); + + // Make sure upload behave as expected + assert_eq!(status.is_ok(), ok); + assert_eq!(full_path.exists(), ok); + + Ok(()) +}