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

Fix security issue with --no-symlinks

5 files changed, 84 insertions, 77 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 76928b6..7658b3b 100644
--- a/CHANGELOG.md
+++ b/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)
diff --git a/Cargo.lock b/Cargo.lock
index 308dcc0..d275f01 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -1394,9 +1394,9 @@ dependencies = [
 
 [[package]]
 name = "itoa"
-version = "1.0.1"
+version = "1.0.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "1aab8fc367588b89dcee83ab0fd66b72b50b72fa1904d7095045ace2b0c81c35"
+checksum = "112c678d4050afce233f4f2852bb2eb519230b3cf12f33585275537d7e41578d"
 
 [[package]]
 name = "jobserver"
@@ -1448,9 +1448,9 @@ checksum = "e2abad23fbc42b3700f2f279844dc832adb2b2eb069b2df918f455c4e18cc646"
 
 [[package]]
 name = "libc"
-version = "0.2.125"
+version = "0.2.126"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "5916d2ae698f6de9bfb891ad7a8d65c09d232dc58cc4ac433c7da3b2fd84bc2b"
+checksum = "349d5a591cd28b49e1d1037471617a32ddcda5731b99419008085f72d5a53836"
 
 [[package]]
 name = "libflate"
@@ -1656,25 +1656,14 @@ dependencies = [
 
 [[package]]
 name = "mio"
-version = "0.8.2"
+version = "0.8.3"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "52da4364ffb0e4fe33a9841a98a3f3014fb964045ce4f7a45a398243c8d6b0c9"
+checksum = "713d550d9b44d89174e066b7a6217ae06234c10cb47819a88290d2b353c31799"
 dependencies = [
  "libc",
  "log",
- "miow",
- "ntapi",
  "wasi 0.11.0+wasi-snapshot-preview1",
- "winapi 0.3.9",
-]
-
-[[package]]
-name = "miow"
-version = "0.3.7"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "b9f1c5b025cda876f66ef43a113f91ebc9f4ccef34843000e0adf6ebbab84e21"
-dependencies = [
- "winapi 0.3.9",
+ "windows-sys",
 ]
 
 [[package]]
@@ -1699,15 +1688,6 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "61807f77802ff30975e01f4f071c8ba10c022052f98b3294119f3e615d13e5be"
 
 [[package]]
-name = "ntapi"
-version = "0.3.7"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "c28774a7fd2fbb4f0babd8237ce554b73af68021b5f695a3cebd6c59bac0980f"
-dependencies = [
- "winapi 0.3.9",
-]
-
-[[package]]
 name = "num-bigint"
 version = "0.4.3"
 source = "registry+https://github.com/rust-lang/crates.io-index"
@@ -1776,9 +1756,9 @@ checksum = "87f3e037eac156d1775da914196f0f37741a274155e34a0b7e427c35d2a2ecb9"
 
 [[package]]
 name = "os_str_bytes"
-version = "6.0.0"
+version = "6.0.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "8e22443d1643a904602595ba1cd8f7d896afe56d26712531c5ff73a15b2fbf64"
+checksum = "029d8d0b2f198229de29dca79676f2738ff952edf3fde542eb8bf94d8c21b435"
 
 [[package]]
 name = "output_vt100"
@@ -2048,11 +2028,11 @@ checksum = "dbf0c48bc1d91375ae5c3cd81e3722dff1abcf81a30960240640d223f59fe0e5"
 
 [[package]]
 name = "proc-macro2"
-version = "1.0.38"
+version = "1.0.39"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "9027b48e9d4c9175fa2218adf3557f91c1137021739951d4932f5f8268ac48aa"
+checksum = "c54b25569025b7fc9651de43004ae593a75ad88543b17178aa5e1b9c4f15f56f"
 dependencies = [
- "unicode-xid",
+ "unicode-ident",
 ]
 
 [[package]]
@@ -2331,9 +2311,9 @@ checksum = "f2cc38e8fa666e2de3c4aba7edeb5ffc5246c1c2ed0e3d17e560aeeba736b23f"
 
 [[package]]
 name = "ryu"
-version = "1.0.9"
+version = "1.0.10"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "73b4b750c782965c211b42f022f59af1fbceabdd026623714f104152f1ec149f"
+checksum = "f3f6f92acf49d1b98f7a81226834412ada05458b7364277387724a237f062695"
 
 [[package]]
 name = "same-file"
@@ -2482,12 +2462,12 @@ checksum = "f2dd574626839106c320a323308629dcb1acfc96e32a8cba364ddc61ac23ee83"
 
 [[package]]
 name = "socket2"
-version = "0.4.5"
+version = "0.4.4"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "ca642ba17f8b2995138b1d7711829c92e98c0a25ea019de790f4f09279c4e296"
+checksum = "66d72b759436ae32898a2af0a14218dbf55efde3feeb170eb623637db85ee1e0"
 dependencies = [
  "libc",
- "windows-sys",
+ "winapi 0.3.9",
 ]
 
 [[package]]
@@ -2555,13 +2535,13 @@ dependencies = [
 
 [[package]]
 name = "syn"
-version = "1.0.92"
+version = "1.0.95"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "7ff7c592601f11445996a06f8ad0c27f094a58857c2f89e97974ab9235b92c52"
+checksum = "fbaf6116ab8924f39d52792136fb74fd60a80194cf1b1c6ffa6453eef1c3f942"
 dependencies = [
  "proc-macro2",
  "quote",
- "unicode-xid",
+ "unicode-ident",
 ]
 
 [[package]]
@@ -2717,9 +2697,9 @@ checksum = "cda74da7e1a664f795bb1f8a87ec406fb89a02522cf6e50620d016add6dbbf5c"
 
 [[package]]
 name = "tokio"
-version = "1.18.1"
+version = "1.18.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "dce653fb475565de9f6fb0614b28bca8df2c430c0cf84bcd9c843f15de5414cc"
+checksum = "4903bf0427cf68dddd5aa6a93220756f8be0c34fcfa9f5e6191e103e15a31395"
 dependencies = [
  "bytes",
  "libc",
@@ -2747,9 +2727,9 @@ dependencies = [
 
 [[package]]
 name = "tokio-util"
-version = "0.7.1"
+version = "0.7.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "0edfdeb067411dba2044da6d1cb2df793dd35add7888d73c16e3381ded401764"
+checksum = "f988a1a1adc2fb21f9c12aa96441da33a1728193ae0b95d2be22dbd17fcb4e5c"
 dependencies = [
  "bytes",
  "futures-core",
@@ -2842,6 +2822,12 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "099b7128301d285f79ddd55b9a83d5e6b9e97c92e0ea0daebee7263e932de992"
 
 [[package]]
+name = "unicode-ident"
+version = "1.0.0"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "d22af068fba1eb5edcb4aea19d382b2a3deb4c8f9d475c589b6ada9e0fd493ee"
+
+[[package]]
 name = "unicode-normalization"
 version = "0.1.19"
 source = "registry+https://github.com/rust-lang/crates.io-index"
@@ -2857,12 +2843,6 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "3ed742d4ea2bd1176e236172c8429aaf54486e7ac098db29ffe6529e0ce50973"
 
 [[package]]
-name = "unicode-xid"
-version = "0.2.3"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "957e51f3646910546462e67d5f7599b9e4fb8acdd304b087a6494730f9eebf04"
-
-[[package]]
 name = "untrusted"
 version = "0.7.1"
 source = "registry+https://github.com/rust-lang/crates.io-index"
@@ -3195,18 +3175,18 @@ dependencies = [
 
 [[package]]
 name = "zstd"
-version = "0.10.0+zstd.1.5.2"
+version = "0.10.2+zstd.1.5.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "3b1365becbe415f3f0fcd024e2f7b45bacfb5bdd055f0dc113571394114e7bdd"
+checksum = "5f4a6bd64f22b5e3e94b4e238669ff9f10815c27a5180108b849d24174a83847"
 dependencies = [
  "zstd-safe",
 ]
 
 [[package]]
 name = "zstd-safe"
-version = "4.1.4+zstd.1.5.2"
+version = "4.1.6+zstd.1.5.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "2f7cd17c9af1a4d6c24beb1cc54b17e2ef7b593dc92f19e9d9acad8b182bbaee"
+checksum = "94b61c51bb270702d6167b8ce67340d2754b088d0c091b06e593aa772c3ee9bb"
 dependencies = [
  "libc",
  "zstd-sys",
diff --git a/src/args.rs b/src/args.rs
index 7667bee..757fd22 100644
--- a/src/args.rs
+++ b/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,
 
diff --git a/src/main.rs b/src/main.rs
index de10d7d..9d3f9ac 100644
--- a/src/main.rs
+++ b/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() {
diff --git a/tests/serve_request.rs b/tests/serve_request.rs
index d66ffd4..18cc9e4 100644
--- a/tests/serve_request.rs
+++ b/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(())
 }