From bd24e27f93eba0c5f336a3cfaa1ac4ba2716d70a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mateusz=20Szczygie=C5=82?= <mateusz.szczygiel@nordsec.com>
Date: Fri, 12 Jul 2024 09:02:07 +0300
Subject: [PATCH] Validate file ID for path traversal
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Mateusz Szczygieł <mateusz.szczygiel@nordsec.com>
---
 changelog.md                       |  1 +
 drop-transfer/src/ws/server/mod.rs | 42 +++++++++++++++++++++++++++++-
 2 files changed, 42 insertions(+), 1 deletion(-)

diff --git a/changelog.md b/changelog.md
index fd32eb1..8fa1778 100644
--- a/changelog.md
+++ b/changelog.md
@@ -2,6 +2,7 @@
 ### **Fresh air**
 ---
 * Remove support for deprecated, unsecure protocols V1, V2, V4 and V5
+* Sanitaze file ID before downloading a file, to prevent the temporary file from being allowed to traverse parent dirs
 
 ---
 <br>
diff --git a/drop-transfer/src/ws/server/mod.rs b/drop-transfer/src/ws/server/mod.rs
index b92c019..719da93 100644
--- a/drop-transfer/src/ws/server/mod.rs
+++ b/drop-transfer/src/ws/server/mod.rs
@@ -880,6 +880,7 @@ impl FileXferTask {
     ) {
         let task = async {
             validate_subpath_for_download(self.file.subpath())?;
+            validate_file_id_for_download(self.file.id())?;
 
             let emit_checksum_events = {
                 if let Some(threshold) = state.config.checksum_events_size_threshold {
@@ -1164,9 +1165,24 @@ fn validate_subpath_for_download(subpath: &FileSubPath) -> crate::Result<()> {
     Ok(())
 }
 
+/// Check file ID for illegal characters so that the temp file is not created in
+/// parent directories
+fn validate_file_id_for_download(file_id: &FileId) -> crate::Result<()> {
+    // Ideally we would allow only alphanumeric characters but we use URL base64
+    // which allows for more
+    const DISALLOWED: &[char] = &['.', '/', '\\', ':'];
+
+    let invalid = file_id.as_ref().chars().any(|c| DISALLOWED.contains(&c));
+    if invalid {
+        return Err(Error::BadFileId);
+    }
+
+    Ok(())
+}
+
 #[cfg(test)]
 mod tests {
-    use crate::file::FileSubPath;
+    use crate::{file::FileSubPath, FileId};
 
     #[test]
     fn validate_subpath() {
@@ -1188,4 +1204,28 @@ mod tests {
             Err(crate::Error::FilenameTooLong)
         ));
     }
+
+    #[test]
+    fn validate_file_id() {
+        let id = FileId::from("x_ALOz5YRCXZSiS5V5Pd8VN_qqC6abyDyIBnjitzGe8");
+        assert!(super::validate_file_id_for_download(&id).is_ok());
+
+        let id = FileId::from("../../asdfasdf");
+        assert!(matches!(
+            super::validate_file_id_for_download(&id),
+            Err(crate::Error::BadFileId)
+        ));
+
+        let id = FileId::from("/asdfasdf");
+        assert!(matches!(
+            super::validate_file_id_for_download(&id),
+            Err(crate::Error::BadFileId)
+        ));
+
+        let id = FileId::from("C:\\ABC");
+        assert!(matches!(
+            super::validate_file_id_for_download(&id),
+            Err(crate::Error::BadFileId)
+        ));
+    }
 }