feat(server): do path joins more safely (#247)

* Do path joins more safely

* Improve path cleaning and tests

* Lower-case error message

Co-authored-by: Orhun Parmaksız <orhunparmaksiz@gmail.com>

* Correct handle potential errors in `get_path`

* Use `expect` in tests, rather than `unwrap`

* Correctly handle invalid upload path without panic

* Correctly handle filesystem create errors

* Use result rather than option to allow easier error handling

---------

Co-authored-by: Orhun Parmaksız <orhunparmaksiz@gmail.com>

Cargo.lock

@@ -1938,6 +1938,12 @@ version = "1.0.14"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "de3145af08024dea9fa9914f381a17b8fc6034dfb00f3a84013f7ff43f29ed4c"
 
+[[package]]
+name = "path-clean"
+version = "1.0.1"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "17359afc20d7ab31fdb42bb844c8b3bb1dabd7dcf7e68428492da7f16966fcef"
+
 [[package]]
 name = "pathdiff"
 version = "0.2.1"
@@ -2425,6 +2431,7 @@ dependencies = [
  "infer",
  "lazy-regex",
  "mime",
+ "path-clean",
  "petname",
  "rand",
  "regex",

Cargo.toml

@@ -48,6 +48,7 @@ tokio = { version = "1.35.1", optional = true }
 tracing = "0.1.40"
 tracing-subscriber = { version = "0.3.18", features = ["env-filter"] }
 uts2ts = "0.4.1"
+path-clean = "1.0.1"
 
 [dependencies.config]
 version = "0.13.4"

src/main.rs

@@ -78,7 +78,7 @@ fn setup(config_folder: &Path) -> IoResult<(Data<RwLock<Config>>, ServerConfig,
     // Create necessary directories.
     fs::create_dir_all(&server_config.upload_path)?;
     for paste_type in &[PasteType::Url, PasteType::Oneshot, PasteType::OneshotUrl] {
-        fs::create_dir_all(paste_type.get_path(&server_config.upload_path))?;
+        fs::create_dir_all(paste_type.get_path(&server_config.upload_path)?)?;
     }
 
     // Set up a watcher for the configuration file changes.

src/paste.rs

@@ -58,12 +58,12 @@ impl PasteType {
     }
 
     /// Returns the given path with [`directory`](Self::get_dir) adjoined.
-    pub fn get_path(&self, path: &Path) -> PathBuf {
+    pub fn get_path(&self, path: &Path) -> IoResult<PathBuf> {
         let dir = self.get_dir();
         if dir.is_empty() {
-            path.to_path_buf()
+            Ok(path.to_path_buf())
         } else {
-            path.join(dir)
+            util::safe_path_join(path, Path::new(&dir))
         }
     }
 
@@ -122,10 +122,9 @@ impl Paste {
         if let Some(handle_spaces_config) = config.server.handle_spaces {
             file_name = handle_spaces_config.process_filename(&file_name);
         }
-        let mut path = self
+
-            .type_
+        let mut path =
-            .get_path(&config.server.upload_path)
+            util::safe_path_join(self.type_.get_path(&config.server.upload_path)?, &file_name)?;
-            .join(&file_name);
         let mut parts: Vec<&str> = file_name.split('.').collect();
         let mut dotfile = false;
         let mut lower_bound = 1;
@@ -261,10 +260,8 @@ impl Paste {
                 file_name = random_text;
             }
         }
-        let mut path = self
+        let mut path =
-            .type_
+            util::safe_path_join(self.type_.get_path(&config.server.upload_path)?, &file_name)?;
-            .get_path(&config.server.upload_path)
-            .join(&file_name);
         if let Some(timestamp) = expiry_date {
             path.set_file_name(format!("{file_name}.{timestamp}"));
         }
@@ -430,7 +427,11 @@ mod tests {
         fs::remove_file(file_name)?;
 
         for paste_type in &[PasteType::Url, PasteType::Oneshot] {
-            fs::create_dir_all(paste_type.get_path(&config.server.upload_path))?;
+            fs::create_dir_all(
+                paste_type
+                    .get_path(&config.server.upload_path)
+                    .expect("Bad upload path"),
+            )?;
         }
 
         config.paste.random_url = None;
@@ -442,6 +443,7 @@ mod tests {
         let file_name = paste.store_file("test.file", Some(expiry_date), None, &config)?;
         let file_path = PasteType::Oneshot
             .get_path(&config.server.upload_path)
+            .expect("Bad upload path")
             .join(format!("{file_name}.{expiry_date}"));
         assert_eq!("test", fs::read_to_string(&file_path)?);
         fs::remove_file(file_path)?;
@@ -458,6 +460,7 @@ mod tests {
         let file_name = paste.store_url(None, &config)?;
         let file_path = PasteType::Url
             .get_path(&config.server.upload_path)
+            .expect("Bad upload path")
             .join(&file_name);
         assert_eq!(url, fs::read_to_string(&file_path)?);
         fs::remove_file(file_path)?;
@@ -485,6 +488,7 @@ mod tests {
             .await?;
         let file_path = PasteType::RemoteFile
             .get_path(&config.server.upload_path)
+            .expect("Bad upload path")
             .join(file_name);
         assert_eq!(
             "8c712905b799905357b8202d0cb7a244cefeeccf7aa5eb79896645ac50158ffa",
@@ -493,7 +497,11 @@ mod tests {
         fs::remove_file(file_path)?;
 
         for paste_type in &[PasteType::Url, PasteType::Oneshot] {
-            fs::remove_dir(paste_type.get_path(&config.server.upload_path))?;
+            fs::remove_dir(
+                paste_type
+                    .get_path(&config.server.upload_path)
+                    .expect("Bad upload path"),
+            )?;
         }
 
         Ok(())

src/server.rs

@@ -4,7 +4,7 @@ use crate::file::Directory;
 use crate::header::{self, ContentDisposition};
 use crate::mime as mime_util;
 use crate::paste::{Paste, PasteType};
-use crate::util;
+use crate::util::{self, safe_path_join};
 use actix_files::NamedFile;
 use actix_multipart::Multipart;
 use actix_web::http::StatusCode;
@@ -89,12 +89,11 @@ async fn serve(
     let config = config
         .read()
         .map_err(|_| error::ErrorInternalServerError("cannot acquire config"))?;
-    let path = config.server.upload_path.join(&*file);
+    let mut path = util::glob_match_file(safe_path_join(&config.server.upload_path, &*file)?)?;
-    let mut path = util::glob_match_file(path)?;
     let mut paste_type = PasteType::File;
     if !path.exists() || path.is_dir() {
         for type_ in &[PasteType::Url, PasteType::Oneshot, PasteType::OneshotUrl] {
-            let alt_path = type_.get_path(&config.server.upload_path).join(&*file);
+            let alt_path = safe_path_join(type_.get_path(&config.server.upload_path)?, &*file)?;
             let alt_path = util::glob_match_file(alt_path)?;
             if alt_path.exists()
                 || path.file_name().and_then(|v| v.to_str()) == Some(&type_.get_dir())
@@ -159,8 +158,7 @@ async fn delete(
     let config = config
         .read()
         .map_err(|_| error::ErrorInternalServerError("cannot acquire config"))?;
-    let path = config.server.upload_path.join(&*file);
+    let path = util::glob_match_file(safe_path_join(&config.server.upload_path, &*file)?)?;
-    let path = util::glob_match_file(path)?;
     if !path.is_file() || !path.exists() {
         return Err(error::ErrorNotFound("file is not found or expired :(\n"));
     }
@@ -1087,7 +1085,9 @@ mod tests {
         )
         .await;
 
-        let url_upload_path = PasteType::Url.get_path(&config.server.upload_path);
+        let url_upload_path = PasteType::Url
+            .get_path(&config.server.upload_path)
+            .expect("Bad upload path");
         fs::create_dir_all(&url_upload_path)?;
 
         let response = test::call_service(
@@ -1125,7 +1125,9 @@ mod tests {
         )
         .await;
 
-        let oneshot_upload_path = PasteType::Oneshot.get_path(&config.server.upload_path);
+        let oneshot_upload_path = PasteType::Oneshot
+            .get_path(&config.server.upload_path)
+            .expect("Bad upload path");
         fs::create_dir_all(&oneshot_upload_path)?;
 
         let file_name = "oneshot.txt";
@@ -1185,7 +1187,9 @@ mod tests {
         )
         .await;
 
-        let url_upload_path = PasteType::OneshotUrl.get_path(&config.server.upload_path);
+        let url_upload_path = PasteType::OneshotUrl
+            .get_path(&config.server.upload_path)
+            .expect("Bad upload path");
         fs::create_dir_all(&url_upload_path)?;
 
         let response = test::call_service(

src/util.rs

@@ -2,6 +2,7 @@ use crate::paste::PasteType;
 use actix_web::{error, Error as ActixError};
 use glob::glob;
 use lazy_regex::{lazy_regex, Lazy, Regex};
+use path_clean::PathClean;
 use ring::digest::{Context, SHA256};
 use std::fmt::Write;
 use std::io::{BufReader, Read};
@@ -64,7 +65,8 @@ pub fn get_expired_files(base_path: &Path) -> Vec<PathBuf> {
         PasteType::OneshotUrl,
     ]
     .into_iter()
-    .filter_map(|v| glob(&v.get_path(base_path).join("*.[0-9]*").to_string_lossy()).ok())
+    .filter_map(|v| v.get_path(base_path).ok())
+    .filter_map(|v| glob(&v.join("*.[0-9]*").to_string_lossy()).ok())
     .flat_map(|glob| glob.filter_map(|v| v.ok()).collect::<Vec<PathBuf>>())
     .filter(|path| {
         if let Some(extension) = path
@@ -108,6 +110,27 @@ pub fn sha256_digest<R: Read>(input: R) -> Result<String, ActixError> {
         })?)
 }
 
+/// Joins the paths whilst ensuring the path doesn't drastically change.
+/// `base` is assumed to be a trusted value.
+pub fn safe_path_join<B: AsRef<Path>, P: AsRef<Path>>(base: B, part: P) -> IoResult<PathBuf> {
+    let new_path = base.as_ref().join(part).clean();
+
+    let cleaned_base = base.as_ref().clean();
+
+    if !new_path.starts_with(cleaned_base) {
+        return Err(IoError::new(
+            IoErrorKind::InvalidData,
+            format!(
+                "{} is outside of {}",
+                new_path.display(),
+                base.as_ref().display()
+            ),
+        ));
+    }
+
+    Ok(new_path)
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -170,4 +193,24 @@ mod tests {
         assert_eq!(Vec::<PathBuf>::new(), get_expired_files(&current_dir));
         Ok(())
     }
+
+    #[test]
+    fn test_safe_join_path() {
+        assert_eq!(safe_path_join("/foo", "bar").ok(), Some("/foo/bar".into()));
+        assert_eq!(safe_path_join("/", "bar").ok(), Some("/bar".into()));
+        assert_eq!(safe_path_join("/", "././bar").ok(), Some("/bar".into()));
+        assert_eq!(
+            safe_path_join("/foo/bar", "baz/").ok(),
+            Some("/foo/bar/baz/".into())
+        );
+        assert_eq!(
+            safe_path_join("/foo/bar/../", "baz").ok(),
+            Some("/foo/baz".into())
+        );
+
+        assert!(safe_path_join("/foo", "/foobar").is_err());
+        assert!(safe_path_join("/foo", "/bar").is_err());
+        assert!(safe_path_join("/foo/bar", "..").is_err());
+        assert!(safe_path_join("/foo/bar", "../").is_err());
+    }
 }