From 699e17c7de0b40c1e5f7e4171683710378e4af58 Mon Sep 17 00:00:00 2001 From: Ali MJ Al-Nasrawy Date: Tue, 23 Mar 2021 22:42:28 +0300 Subject: 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 5f9738c..2319b6a 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 698eb46..26750d9 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(()) +} -- cgit v1.2.3 From dd528b3a32c9d653f99c0bb41b002a6744720189 Mon Sep 17 00:00:00 2001 From: Ali MJ Al-Nasrawy Date: Sat, 4 Sep 2021 23:08:47 +0300 Subject: Adress review comments --- src/file_upload.rs | 41 +++++++++++++++++++++++++++++++++++++++-- tests/upload_files.rs | 12 ++++++++---- 2 files changed, 47 insertions(+), 6 deletions(-) diff --git a/src/file_upload.rs b/src/file_upload.rs index 2319b6a..5009f36 100644 --- a/src/file_upload.rs +++ b/src/file_upload.rs @@ -8,7 +8,10 @@ use std::{ use crate::errors::ContextualError; use crate::listing::{self}; -/// Create future to save file. +/// Saves file data from a multipart form field (`field`) to `file_path`, optionally overwriting +/// existing file. +/// +/// Returns total bytes written to file. async fn save_file( field: actix_multipart::Field, file_path: PathBuf, @@ -34,7 +37,7 @@ async fn save_file( Ok(written_len) } -/// Create new future to handle file as multipart data. +/// Handles a single field in a multipart form async fn handle_multipart( field: actix_multipart::Field, path: PathBuf, @@ -121,6 +124,8 @@ pub async fn upload_file( /// Guarantee that the path is relative and cannot traverse back to parent directories /// and optionally prevent traversing hidden directories. +/// +/// See the unit tests tests::test_sanitize_path* for examples fn sanitize_path(path: &Path, traverse_hidden: bool) -> Option { let mut buf = PathBuf::new(); @@ -145,3 +150,35 @@ fn sanitize_path(path: &Path, traverse_hidden: bool) -> Option { Some(buf) } + +#[cfg(test)] +mod tests { + use super::*; + use pretty_assertions::assert_eq; + use rstest::rstest; + + #[rstest] + #[case("/foo", "foo")] + #[case("////foo", "foo")] + #[case("C:/foo", if cfg!(windows) { "foo" } else { "C:/foo" })] + #[case("../foo", "foo")] + #[case("../foo/../bar/abc", "bar/abc")] + fn test_sanitize_path(#[case] input: &str, #[case] output: &str) { + assert_eq!( + sanitize_path(Path::new(input), true).unwrap(), + Path::new(output) + ); + assert_eq!( + sanitize_path(Path::new(input), false).unwrap(), + Path::new(output) + ); + } + + #[rstest] + #[case(".foo")] + #[case("/.foo")] + #[case("foo/.bar/foo")] + fn test_sanitize_path_no_hidden_files(#[case] input: &str) { + assert_eq!(sanitize_path(Path::new(input), false), None); + } +} diff --git a/tests/upload_files.rs b/tests/upload_files.rs index 26750d9..331db1c 100644 --- a/tests/upload_files.rs +++ b/tests/upload_files.rs @@ -80,6 +80,10 @@ fn uploading_files_is_prevented(server: TestServer) -> Result<(), Error> { Ok(()) } +/// Test for path traversal vulnerability (CWE-22) in both path parameter of query string and in +/// file name (Content-Disposition) +/// +/// see: https://github.com/svenstaro/miniserve/issues/518 #[rstest] #[case("foo", "bar", "foo/bar")] #[case("/../foo", "bar", "foo/bar")] @@ -87,13 +91,13 @@ fn uploading_files_is_prevented(server: TestServer) -> Result<(), Error> { #[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( +fn prevent_path_traversal_attacks( #[with(&["-u"])] server: TestServer, #[case] path: &str, #[case] filename: &'static str, #[case] expected: &str, ) -> Result<(), Error> { - // create test directories + // Create test directories use std::fs::create_dir_all; create_dir_all(server.path().join("foo")).unwrap(); if !cfg!(windows) { @@ -132,14 +136,14 @@ fn symlink(#[case] server: TestServer, #[case] ok: bool, tmpdir: TempDir) -> Res #[cfg(windows)] use std::os::windows::fs::symlink_dir; - // create symlink directory "foo" to point outside the root + // 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 + // Try to upload let part = multipart::Part::text("this should be uploaded") .file_name(filename) .mime_str("text/plain")?; -- cgit v1.2.3 From f8326e510adf906014df7def67237812bec5930d Mon Sep 17 00:00:00 2001 From: Ali MJ Al-Nasrawy Date: Sat, 4 Sep 2021 23:27:15 +0300 Subject: Better name and docs for symlink test --- tests/upload_files.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/upload_files.rs b/tests/upload_files.rs index 331db1c..5e764ba 100644 --- a/tests/upload_files.rs +++ b/tests/upload_files.rs @@ -127,10 +127,16 @@ fn prevent_path_traversal_attacks( Ok(()) } +/// Test uploading to symlink directories that point outside the server root. +/// See https://github.com/svenstaro/miniserve/issues/466 #[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> { +fn upload_to_symlink_directory( + #[case] server: TestServer, + #[case] ok: bool, + tmpdir: TempDir, +) -> Result<(), Error> { #[cfg(unix)] use std::os::unix::fs::symlink as symlink_dir; #[cfg(windows)] -- cgit v1.2.3