Merge pull request #802 from svenstaro/fix-symlink-following

Fix security issue with --no-symlinks

CHANGELOG.md

@@ -8,6 +8,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 <!-- next-header -->
 
 ## [Unreleased] - ReleaseDate
+- Fix security issue where `--no-symlinks` would only hide symlinks from listing but it would
+  still be possible to follow them if the path was known
 
 ## [0.19.4] - 2022-04-02
 - Fix random route leaking on error pages [#764](https://github.com/svenstaro/miniserve/pull/764) (thanks @steffhip)

src/args.rs

@@ -75,7 +75,7 @@ pub struct CliArgs {
     #[clap(long = "random-route", conflicts_with("route-prefix"))]
     pub random_route: bool,
 
-    /// Do not follow symbolic links
+    /// Hide symlinks in listing and prevent them from being followed
     #[clap(short = 'P', long = "no-symlinks")]
     pub no_symlinks: bool,
 
@@ -160,7 +160,7 @@ pub struct CliArgs {
     )]
     pub header: Vec<HeaderMap>,
 
-    /// Show symlink info
+    /// Visualize symlinks in directory listing
     #[clap(short = 'l', long = "show-symlink-info")]
     pub show_symlink_info: bool,
 

src/main.rs

@@ -304,6 +304,8 @@ fn configure_header(conf: &MiniserveConfig) -> middleware::DefaultHeaders {
 }
 
 /// Configures the Actix application
+///
+/// This is where we configure the app to serve an index file, the file listing, or a single file.
 fn configure_app(app: &mut web::ServiceConfig, conf: &MiniserveConfig) {
     let files_service = || {
         let files = actix_files::Files::new("", &conf.path);
@@ -332,11 +334,28 @@ fn configure_app(app: &mut web::ServiceConfig, conf: &MiniserveConfig) {
             true => files.use_hidden_files(),
             false => files,
         };
+
+        let base_path = conf.path.clone();
+        let symlinks_may_be_followed = !conf.no_symlinks;
         files
             .show_files_listing()
             .files_listing_renderer(listing::directory_listing)
             .prefer_utf8(true)
             .redirect_to_slash_directory()
+            .path_filter(move |path, _| {
+                // Only allow symlinks to be followed in case conf.no_symlinks is false.
+                let path_is_symlink = base_path
+                    .join(path)
+                    .symlink_metadata()
+                    .map(|m| m.file_type().is_symlink())
+                    .unwrap_or(false);
+
+                if path_is_symlink {
+                    symlinks_may_be_followed
+                } else {
+                    true
+                }
+            })
     };
 
     if !conf.path.is_file() {

tests/serve_request.rs

@@ -131,41 +131,49 @@ fn serves_requests_symlinks(
     #[case] show_symlink_info: bool,
     #[case] server: TestServer,
 ) -> Result<(), Error> {
-    let files = &["symlink-file.html"];
-    let dirs = &["symlink-dir/"];
-    let broken = &["symlink broken"];
-
-    for &directory in dirs {
-        let orig = DIRECTORIES[0].strip_suffix("/").unwrap();
-        let link = server.path().join(directory.strip_suffix("/").unwrap());
-        symlink_dir(orig, link).expect("Couldn't create symlink");
-    }
-    for &file in files {
-        symlink_file(FILES[0], server.path().join(file)).expect("Couldn't create symlink");
-    }
-    for &file in broken {
-        symlink_file("should-not-exist.xxx", server.path().join(file))
-            .expect("Couldn't create symlink");
-    }
+    let file = "symlink-file.html";
+    let dir = "symlink-dir/";
+    let broken = "symlink broken";
+
+    // Set up some basic symlinks:
+    // to dir, to file, to non-existant location
+    let orig = DIRECTORIES[0].strip_suffix("/").unwrap();
+    let link = server.path().join(dir.strip_suffix("/").unwrap());
+    symlink_dir(orig, link).expect("Couldn't create symlink");
+    symlink_file(FILES[0], server.path().join(file)).expect("Couldn't create symlink");
+    symlink_file("should-not-exist.xxx", server.path().join(broken))
+        .expect("Couldn't create symlink");
 
     let body = reqwest::blocking::get(server.url())?.error_for_status()?;
     let parsed = Document::from_read(body)?;
 
-    for &entry in files.into_iter().chain(dirs) {
+    for &entry in &[file, dir] {
+        let status = reqwest::blocking::get(server.url().join(&entry)?)?.status();
+        // We expect a 404 here for when `no_symlinks` is `true`.
+        if no_symlinks {
+            assert_eq!(status, StatusCode::NOT_FOUND);
+        } else {
+            assert_eq!(status, StatusCode::OK);
+        }
+
         let node = parsed
             .find(|x: &Node| x.name().unwrap_or_default() == "a" && x.text() == entry)
             .next();
+
+        // If symlinks are deactivated, none should be shown in the listing.
         assert_eq!(node.is_none(), no_symlinks);
         if node.is_some() && show_symlink_info {
             assert_eq!(node.unwrap().attr("class").unwrap(), "symlink");
         }
+
+        // If following symlinks is deactivated, we can just skip this iteration as we assorted
+        // above tht no entries in the listing can be found for symlinks in that case.
         if no_symlinks {
             continue;
         }
 
         let node = node.unwrap();
         assert_eq!(node.attr("href").unwrap().strip_prefix("/").unwrap(), entry);
-        reqwest::blocking::get(server.url().join(&entry)?)?.error_for_status()?;
         if entry.ends_with("/") {
             let node = parsed
                 .find(|x: &Node| x.name().unwrap_or_default() == "a" && x.text() == DIRECTORIES[0])
@@ -178,9 +186,7 @@ fn serves_requests_symlinks(
             assert_eq!(node.unwrap().attr("class").unwrap(), "file");
         }
     }
-    for &entry in broken {
-        assert!(parsed.find(|x: &Node| x.text() == entry).next().is_none());
-    }
+    assert!(parsed.find(|x: &Node| x.text() == broken).next().is_none());
 
     Ok(())
 }