Merge pull request #590 from aliemjay/sanitze-path

file_upload.rs: sanitize path input

2 files changed, 171 insertions, 13 deletions

diff --git a/src/file_upload.rs b/src/file_upload.rs
index 5f9738c..5009f36 100644
--- a/src/file_upload.rs
+++ b/src/file_upload.rs
@@ -2,13 +2,16 @@ 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;
 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,10 +37,10 @@ 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,
-    file_path: PathBuf,
+    path: PathBuf,
     overwrite_files: bool,
 ) -> Result<u64, ContextualError> {
     let filename = field
@@ -50,21 +53,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 +94,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 +104,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 +121,64 @@ 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.
+///
+/// See the unit tests tests::test_sanitize_path* for examples
+fn sanitize_path(path: &Path, traverse_hidden: bool) -> Option<PathBuf> {
+    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)
+}
+
+#[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 698eb46..5e764ba 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,91 @@ 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")]
+#[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 prevent_path_traversal_attacks(
+    #[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(())
+}
+
+/// 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 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)]
+    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(())
+}