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 --- tests/upload_files.rs | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 80 insertions(+), 1 deletion(-) (limited to 'tests') 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 --- tests/upload_files.rs | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) (limited to 'tests') 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(-) (limited to 'tests') 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