Repository: svenstaro/miniserve
Pull request #858 (2022-08-02 14:11:20)

Added option restrict-upload-dir
To fix https://github.com/svenstaro/miniserve/issues/842


svenstaro: Cool, before I do a proper review, could you add a test to check that other directories are now indeed forbidden?
jonasdiemer: Sure, let me have a look into testing in Rust :)
svenstaro: You can take a look at the other tests concerning the upload. It should be somewhat straight forward. Just remember: If you messed up and the test doesn't pick it up, this is a security issue so we need to be really careful here. :)
jonasdiemer: @svenstaro thanks for the suggestion. Fixed an issue (running tests is actually helpful :)) and added a test similar to existing one `uploading_files_is_prevented`.
jonasdiemer: Reworked the solution to use PathBuf and with this also fixed subdirectories not working.

@svenstaro I'd appreciate some feedback (esp. on improving the style). Thanks again! :)
svenstaro: I'd be happy to merge as-is. Please update the `README.md` with the new output of `miniserve --help` and fix the formatting issue and we're good to merge.
jonasdiemer: 



> Hey, I think we're almost there now. Only some tiny stuff now. I'd like to put this into the next miniserve release, too! Do you have some time to finish it up soon so we can land it with the release?

Except for the decision for using -u or -A (and some cleanup in case we go with -u), it should be ready. 
svenstaro: Please run `cargo fmt` to fix CI.
jonasdiemer: Fixed formatting and also sanitized paths so that specification of `-u ./some_dir' also works.
svenstaro: There's a small lint to fix there.
jonasdiemer: Looks like CICD on Windows fails, might have to do with the tests that include a dash in the directory name. Unfortunately, I don't have a windows machine to test on...
Atreyagaurav: I also don't have a Windows machine to build miniserve to test, but I made a colleague try to make a directory named "-h" and it was a success (I tried both with "New Folder" button and `mkdir` in cmd. `cd -h` & `cd ./-h`worked fine, but `dir ./-h` gave `Invalid switch - "h".`

I don't know if it's a good idea, but we might be able to use `cfg` to only test that on Linux? At least for now...
svenstaro: I'd be fine if we don't run that test on Windows.
jonasdiemer: > 


svenstaro: It seems like there's a conflict with the recent QR stuff merge. Could you rebase?

Patch:
diff --git a/src/args.rs b/src/args.rs
index a8718bd..8757bc8 100644
--- a/src/args.rs
+++ b/src/args.rs
@@ -107,23 +107,28 @@ pub struct CliArgs {
     #[clap(short = 'q', long = "qrcode")]
     pub qrcode: bool,
 
-    /// Enable file uploading
-    #[clap(short = 'u', long = "upload-files")]
-    pub file_upload: bool,
+    /// Enable file uploading (and optionally specify for which directory)
+    #[clap(short = 'u', long = "upload-files", value_hint = ValueHint::FilePath, min_values = 0)]
+    pub allowed_upload_dir: Option<Vec<PathBuf>>,
 
     /// Enable creating directories
-    #[clap(short = 'U', long = "mkdir", requires = "file-upload")]
+    #[clap(short = 'U', long = "mkdir", requires = "allowed-upload-dir")]
     pub mkdir_enabled: bool,
 
     /// Specify uploadable media types
-    #[clap(arg_enum, short = 'm', long = "media-type", requires = "file-upload")]
+    #[clap(
+        arg_enum,
+        short = 'm',
+        long = "media-type",
+        requires = "allowed-upload-dir"
+    )]
     pub media_type: Option<Vec<MediaType>>,
 
     /// Directly specify the uploadable media type expression
     #[clap(
         short = 'M',
         long = "raw-media-type",
-        requires = "file-upload",
+        requires = "allowed-upload-dir",
         conflicts_with = "media-type"
     )]
     pub media_type_raw: Option<String>,
diff --git a/src/config.rs b/src/config.rs
index 5bcbd62..7ca0693 100644
--- a/src/config.rs
+++ b/src/config.rs
@@ -16,6 +16,7 @@ use rustls_pemfile as pemfile;
 use crate::{
     args::{CliArgs, MediaType},
     auth::RequiredAuth,
+    file_upload::sanitize_path,
 };
 
 /// Possible characters for random routes
@@ -87,6 +88,9 @@ pub struct MiniserveConfig {
     /// Enable file upload
     pub file_upload: bool,
 
+    /// List of allowed upload directories
+    pub allowed_upload_dir: Vec<String>,
+
     /// HTML accept attribute value
     pub uploadable_media_type: Option<String>,
 
@@ -247,7 +251,19 @@ impl MiniserveConfig {
             overwrite_files: args.overwrite_files,
             show_qrcode: args.qrcode,
             mkdir_enabled: args.mkdir_enabled,
-            file_upload: args.file_upload,
+            file_upload: args.allowed_upload_dir.is_some(),
+            allowed_upload_dir: args
+                .allowed_upload_dir
+                .unwrap_or_default()
+                .iter()
+                .map(|x| {
+                    sanitize_path(x, false)
+                        .unwrap()
+                        .to_str()
+                        .unwrap()
+                        .replace('\\', "/")
+                })
+                .collect(),
             uploadable_media_type,
             tar_enabled: args.enable_tar,
             tar_gz_enabled: args.enable_tar_gz,
diff --git a/src/errors.rs b/src/errors.rs
index b2ed459..06569d3 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -22,6 +22,10 @@ pub enum ContextualError {
     #[error("File already exists, and the overwrite_files option has not been set")]
     DuplicateFileError,
 
+    /// Upload not allowed
+    #[error("Upload not allowed to this directory")]
+    UploadForbiddenError,
+
     /// Any error related to an invalid path (failed to retrieve entry name, unexpected entry type, etc)
     #[error("Invalid path\ncaused by: {0}")]
     InvalidPathError(String),
@@ -88,6 +92,8 @@ impl ResponseError for ContextualError {
             Self::InsufficientPermissionsError(_) => StatusCode::FORBIDDEN,
             Self::InvalidHttpCredentials => StatusCode::UNAUTHORIZED,
             Self::InvalidHttpRequestError(_) => StatusCode::BAD_REQUEST,
+            Self::DuplicateFileError => StatusCode::FORBIDDEN,
+            Self::UploadForbiddenError => StatusCode::FORBIDDEN,
             _ => StatusCode::INTERNAL_SERVER_ERROR,
         }
     }
diff --git a/src/file_upload.rs b/src/file_upload.rs
index 6643c68..cf214b8 100644
--- a/src/file_upload.rs
+++ b/src/file_upload.rs
@@ -171,6 +171,17 @@ pub async fn upload_file(
         ContextualError::IoError("Failed to resolve path served by miniserve".to_string(), e)
     })?;
 
+    // Disallow paths outside of allowed directories
+    let upload_allowed = conf.allowed_upload_dir.is_empty()
+        || conf
+            .allowed_upload_dir
+            .iter()
+            .any(|s| upload_path.starts_with(s));
+
+    if !upload_allowed {
+        return Err(ContextualError::UploadForbiddenError);
+    }
+
     // Disallow the target path to go outside of the served directory
     // The target directory shouldn't be canonicalized when it gets passed to
     // handle_multipart so that it can check for symlinks if needed
@@ -207,7 +218,7 @@ pub async fn upload_file(
 /// 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> {
+pub fn sanitize_path(path: &Path, traverse_hidden: bool) -> Option<PathBuf> {
     let mut buf = PathBuf::new();
 
     for comp in path.components() {
diff --git a/src/renderer.rs b/src/renderer.rs
index c8958fe..38d2617 100644
--- a/src/renderer.rs
+++ b/src/renderer.rs
@@ -40,6 +40,12 @@ pub fn page(
 
     let title_path = breadcrumbs_to_path_string(breadcrumbs);
 
+    let upload_allowed = conf.allowed_upload_dir.is_empty()
+        || conf
+            .allowed_upload_dir
+            .iter()
+            .any(|x| encoded_dir.starts_with(&format!("/{}", x)));
+
     html! {
         (DOCTYPE)
         html {
@@ -120,7 +126,7 @@ pub fn page(
                             }
                         }
                         div.toolbar_box_group {
-                            @if conf.file_upload {
+                            @if conf.file_upload && upload_allowed {
                                 div.toolbar_box {
                                     form id="file_submit" action=(upload_action) method="POST" enctype="multipart/form-data" {
                                         p { "Select a file to upload or drag it anywhere into the window" }


Test patch:
diff --git a/tests/upload_files.rs b/tests/upload_files.rs
index 71fcbc4..196f3cd 100644
--- a/tests/upload_files.rs
+++ b/tests/upload_files.rs
@@ -6,6 +6,8 @@ use reqwest::blocking::{multipart, Client};
 use rstest::rstest;
 use select::document::Document;
 use select::predicate::{Attr, Text};
+use std::fs::create_dir_all;
+use std::path::Path;
 
 #[rstest]
 fn uploading_files_works(#[with(&["-u"])] server: TestServer) -> Result<(), Error> {
@@ -72,7 +74,7 @@ fn uploading_files_is_prevented(server: TestServer) -> Result<(), Error> {
         .error_for_status()
         .is_err());
 
-    // After uploading, check whether the uploaded file is now getting listed.
+    // After uploading, check whether the uploaded file is NOT getting listed.
     let body = reqwest::blocking::get(server.url())?;
     let parsed = Document::from_read(body)?;
     assert!(!parsed.find(Text).any(|x| x.text() == test_file_name));
@@ -80,6 +82,92 @@ fn uploading_files_is_prevented(server: TestServer) -> Result<(), Error> {
     Ok(())
 }
 
+/// This test runs the server with --allowed-upload-dir argument and
+/// checks that file upload to a different directory is actually prevented.
+#[rstest]
+#[case(server_no_stderr(&["-u", "someDir"]))]
+#[case(server_no_stderr(&["-u", "someDir/some_sub_dir"]))]
+fn uploading_files_is_restricted(#[case] server: TestServer) -> Result<(), Error> {
+    let test_file_name = "uploaded test file.txt";
+
+    // Then try to upload file to root directory (which is not the --allowed-upload-dir)
+    let form = multipart::Form::new();
+    let part = multipart::Part::text("this should not be uploaded")
+        .file_name(test_file_name)
+        .mime_str("text/plain")?;
+    let form = form.part("file_to_upload", part);
+
+    let client = Client::new();
+    // Ensure uploading fails and returns an error
+    assert_eq!(
+        403,
+        client
+            .post(server.url().join("/upload?path=/")?)
+            .multipart(form)
+            .send()?
+            .status()
+    );
+
+    // After uploading, check whether the uploaded file is NOT getting listed.
+    let body = reqwest::blocking::get(server.url())?;
+    let parsed = Document::from_read(body)?;
+    assert!(!parsed.find(Text).any(|x| x.text() == test_file_name));
+
+    Ok(())
+}
+
+/// This tests that we can upload files to the directory specified by --allow-upload-dir
+#[rstest]
+#[case(server(&["-u", "someDir"]), vec!["someDir"])]
+#[case(server(&["-u", "./-someDir"]), vec!["./-someDir"])]
+#[case(server(&["-u", Path::new("someDir/some_sub_dir").to_str().unwrap()]),
+  vec!["someDir/some_sub_dir"])]
+#[case(server(&["-u", Path::new("someDir/some_sub_dir").to_str().unwrap(), 
+                "-u", Path::new("someDir/some_other_dir").to_str().unwrap()]), 
+       vec!["someDir/some_sub_dir", "someDir/some_other_dir"])]
+fn uploading_files_to_allowed_dir_works(
+    #[case] server: TestServer,
+    #[case] upload_dirs: Vec<&str>,
+) -> Result<(), Error> {
+    let test_file_name = "uploaded test file.txt";
+
+    for upload_dir in upload_dirs {
+        // Create test directory
+        create_dir_all(server.path().join(Path::new(upload_dir))).unwrap();
+
+        // Before uploading, check whether the uploaded file does not yet exist.
+        let body = reqwest::blocking::get(server.url().join(upload_dir)?)?.error_for_status()?;
+        let parsed = Document::from_read(body)?;
+        assert!(parsed.find(Text).all(|x| x.text() != test_file_name));
+
+        // Perform the actual upload.
+        let upload_action = parsed
+            .find(Attr("id", "file_submit"))
+            .next()
+            .expect("Couldn't find element with id=file_submit")
+            .attr("action")
+            .expect("Upload form doesn't have action attribute");
+        let form = multipart::Form::new();
+        let part = multipart::Part::text("this should be uploaded")
+            .file_name(test_file_name)
+            .mime_str("text/plain")?;
+        let form = form.part("file_to_upload", part);
+
+        let client = Client::new();
+        client
+            .post(server.url().join(upload_action)?)
+            .multipart(form)
+            .send()?
+            .error_for_status()?;
+
+        // After uploading, check whether the uploaded file is now getting listed.
+        let body = reqwest::blocking::get(server.url().join(upload_dir)?)?;
+        let parsed = Document::from_read(body)?;
+        assert!(parsed.find(Text).any(|x| x.text() == test_file_name));
+    }
+    Ok(())
+}
+
 /// Test for path traversal vulnerability (CWE-22) in both path parameter of query string and in
 /// file name (Content-Disposition)
 ///
@@ -98,7 +186,6 @@ fn prevent_path_traversal_attacks(
     #[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"] {