Merge pull request #515 from fufesou/fix/file-transfer-path-traversal-client-write

fix: file transfer, path traversal

src/fs.rs

@@ -398,7 +398,7 @@ pub struct TransferJob {
     pub is_resume: bool,
     pub file_num: i32,
     #[serde(skip_serializing)]
-    pub files: Vec<FileEntry>,
+    files: Vec<FileEntry>,
     pub conn_id: i32, // server only
 
     #[serde(skip_serializing)]
@@ -457,6 +457,108 @@ fn is_compressed_file(name: &str) -> bool {
     compressed_exts.contains(&ext)
 }
 
+pub fn validate_file_name_no_traversal(name: &str) -> ResultType<()> {
+    if name.bytes().any(|b| b == 0) {
+        bail!("file name contains null bytes");
+    }
+    let has_traversal = name
+        .split(|c: char| c == '/' || (cfg!(windows) && c == '\\'))
+        .filter(|s| !s.is_empty())
+        .any(|s| s == "..");
+    if has_traversal {
+        bail!("path traversal detected in file name");
+    }
+    #[cfg(windows)]
+    {
+        if name.len() >= 2 {
+            let bytes = name.as_bytes();
+            if bytes[0].is_ascii_alphabetic() && bytes[1] == b':' {
+                bail!("absolute path detected in file name");
+            }
+        }
+        if name.starts_with('/') || name.starts_with('\\') {
+            bail!("absolute path detected in file name");
+        }
+    }
+    #[cfg(not(windows))]
+    if name.starts_with('/') {
+        bail!("absolute path detected in file name");
+    }
+    Ok(())
+}
+
+fn validate_transfer_file_names(files: &[FileEntry]) -> ResultType<()> {
+    // Single-file transfer may use an empty relative name, because
+    // the destination file path is carried by transfer metadata.
+    if files.len() == 1 && files.first().map_or(false, |f| f.name.is_empty()) {
+        return Ok(());
+    }
+    for file in files {
+        if file.name.is_empty() {
+            bail!("empty file name in multi-file transfer");
+        }
+        validate_file_name_no_traversal(&file.name)?;
+    }
+    Ok(())
+}
+
+#[inline]
+fn validate_fs_path_argument(path: &str, arg_name: &str) -> ResultType<()> {
+    if path.is_empty() {
+        bail!("{arg_name} cannot be empty");
+    }
+    if path.bytes().any(|b| b == 0) {
+        bail!("{arg_name} contains null bytes");
+    }
+    Ok(())
+}
+
+fn validate_no_symlink_components(base: &PathBuf, name: &str) -> ResultType<()> {
+    if name.is_empty() {
+        return Ok(());
+    }
+    let mut current = base.clone();
+    for component in Path::new(name).components() {
+        match component {
+            std::path::Component::Normal(seg) => {
+                current.push(seg);
+                // Best-effort guard: path-based checks are inherently TOCTOU-prone
+                // if local filesystem state changes between validation and write.
+                match std::fs::symlink_metadata(&current) {
+                    Ok(meta) => {
+                        // This is inherent to filesystem-based checks and acknowledged as a limitation.
+                        // For true protection, you'd need openat(2) / O_NOFOLLOW at write time.
+                        if meta.file_type().is_symlink() {
+                            bail!("symlink path component is not allowed");
+                        }
+                    }
+                    Err(err) if err.kind() == std::io::ErrorKind::NotFound => {
+                        // Component does not exist yet, continue best-effort validation.
+                    }
+                    Err(err) => {
+                        bail!(
+                            "failed to validate path component '{}': {}",
+                            current.display(),
+                            err
+                        );
+                    }
+                }
+            }
+            std::path::Component::CurDir => {}
+            _ => {
+                bail!("invalid file name component");
+            }
+        }
+    }
+    Ok(())
+}
+
+fn join_validated_path(base: &PathBuf, name: &str) -> ResultType<PathBuf> {
+    validate_file_name_no_traversal(name)?;
+    validate_no_symlink_components(base, name)?;
+    Ok(TransferJob::join(base, name))
+}
+
 impl TransferJob {
     #[allow(clippy::too_many_arguments)]
     pub fn new_write(
@@ -467,11 +569,9 @@ impl TransferJob {
         file_num: i32,
         show_hidden: bool,
         is_remote: bool,
-        files: Vec<FileEntry>,
         enable_overwrite_detection: bool,
     ) -> Self {
         log::info!("new write {}", data_source);
-        let total_size = files.iter().map(|x| x.size).sum();
         Self {
             id,
             r#type,
@@ -480,13 +580,18 @@ impl TransferJob {
             file_num,
             show_hidden,
             is_remote,
-            files,
-            total_size,
+            files: Vec::new(),
+            total_size: 0,
             enable_overwrite_detection,
             ..Default::default()
         }
     }
 
+    pub fn with_files(mut self, files: Vec<FileEntry>) -> ResultType<Self> {
+        self.set_files(files)?;
+        Ok(self)
+    }
+
     pub fn new_read(
         id: i32,
         r#type: JobType,
@@ -538,8 +643,16 @@ impl TransferJob {
     }
 
     #[inline]
-    pub fn set_files(&mut self, files: Vec<FileEntry>) {
+    pub fn set_files(&mut self, files: Vec<FileEntry>) -> ResultType<()> {
+        validate_transfer_file_names(&files)?;
+        if let DataSource::FilePath(base) = &self.data_source {
+            for file in &files {
+                validate_no_symlink_components(base, &file.name)?;
+            }
+        }
+        self.total_size = files.iter().map(|x| x.size).sum();
         self.files = files;
+        Ok(())
     }
 
     #[inline]
@@ -573,6 +686,20 @@ impl TransferJob {
         self.file_num
     }
 
+    fn resolve_entry_path(&self, base: &PathBuf, name: &str) -> Option<PathBuf> {
+        if self.r#type == JobType::Generic {
+            match join_validated_path(base, name) {
+                Ok(path) => Some(path),
+                Err(err) => {
+                    log::error!("Invalid file name in transfer job {}: {}", self.id, err);
+                    None
+                }
+            }
+        } else {
+            Some(Self::join(base, name))
+        }
+    }
+
     pub fn modify_time(&self) {
         if self.r#type == JobType::Printer {
             return;
@@ -581,7 +708,9 @@ impl TransferJob {
             let file_num = self.file_num as usize;
             if file_num < self.files.len() {
                 let entry = &self.files[file_num];
-                let path = Self::join(p, &entry.name);
+                let Some(path) = self.resolve_entry_path(p, &entry.name) else {
+                    return;
+                };
                 let download_path = format!("{}.download", get_string(&path));
                 let digest_path = format!("{}.digest", get_string(&path));
                 std::fs::remove_file(digest_path).ok();
@@ -603,7 +732,9 @@ impl TransferJob {
             let file_num = self.file_num as usize;
             if file_num < self.files.len() {
                 let entry = &self.files[file_num];
-                let path = Self::join(p, &entry.name);
+                let Some(path) = self.resolve_entry_path(p, &entry.name) else {
+                    return;
+                };
                 let download_path = format!("{}.download", get_string(&path));
                 let digest_path = format!("{}.digest", get_string(&path));
                 std::fs::remove_file(download_path).ok();
@@ -645,7 +776,11 @@ impl TransferJob {
                     let (path, digest_path) = if self.r#type == JobType::Printer {
                         (p.to_string_lossy().to_string(), None)
                     } else {
-                        let path = Self::join(p, &entry.name);
+                        let path = join_validated_path(p, &entry.name)?;
+                        // NOTE: We intentionally keep path-based validation + regular file open here.
+                        // This still has a known TOCTOU window for symlink races, but avoids a large
+                        // cross-platform rewrite for now.
+                        // Revisit with descriptor/handle-based no-follow open in future hardening.
                         if let Some(pp) = path.parent() {
                             std::fs::create_dir_all(pp).ok();
                         }
@@ -969,13 +1104,17 @@ impl TransferJob {
     async fn set_stream_offset(&mut self, file_num: usize, offset: u64) {
         if let DataSource::FilePath(p) = &self.data_source {
             let entry = &self.files[file_num];
-            let path = Self::join(p, &entry.name);
+            let Some(path) = self.resolve_entry_path(p, &entry.name) else {
+                return;
+            };
             let file_path = get_string(&path);
             let download_path = format!("{}.download", &file_path);
             let digest_path = format!("{}.digest", &file_path);
 
             let mut f = if Path::new(&download_path).exists() && Path::new(&digest_path).exists() {
                 // If both download and digest files exist, seek (writer) to the offset
+                // NOTE: same as write path: best-effort symlink validation happened earlier,
+                // but this reopen remains TOCTOU-prone by design for now.
                 match OpenOptions::new()
                     .create(true)
                     .write(true)
@@ -1260,18 +1399,25 @@ pub fn remove_all_empty_dir(path: &Path) -> ResultType<()> {
 
 #[inline]
 pub fn remove_file(file: &str) -> ResultType<()> {
+    validate_fs_path_argument(file, "file path")?;
     std::fs::remove_file(get_path(file))?;
     Ok(())
 }
 
 #[inline]
 pub fn create_dir(dir: &str) -> ResultType<()> {
+    validate_fs_path_argument(dir, "directory path")?;
     std::fs::create_dir_all(get_path(dir))?;
     Ok(())
 }
 
 #[inline]
 pub fn rename_file(path: &str, new_name: &str) -> ResultType<()> {
+    validate_fs_path_argument(path, "path")?;
+    if new_name.is_empty() {
+        bail!("new file name cannot be empty");
+    }
+    validate_file_name_no_traversal(new_name)?;
     let path = std::path::Path::new(&path);
     if path.exists() {
         let dir = path
@@ -1378,3 +1524,283 @@ pub fn serialize_transfer_job(job: &TransferJob, done: bool, cancel: bool, error
     value["error"] = json!(error);
     serde_json::to_string(&value).unwrap_or_default()
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    struct TestTempDir {
+        path: PathBuf,
+    }
+
+    impl TestTempDir {
+        fn new(prefix: &str) -> Self {
+            Self {
+                path: unique_temp_dir(prefix),
+            }
+        }
+
+        fn join(&self, path: &str) -> PathBuf {
+            self.path.join(path)
+        }
+    }
+
+    impl Drop for TestTempDir {
+        fn drop(&mut self) {
+            let _ = std::fs::remove_dir_all(&self.path);
+        }
+    }
+
+    fn unique_temp_dir(prefix: &str) -> PathBuf {
+        let timestamp = SystemTime::now()
+            .duration_since(UNIX_EPOCH)
+            .unwrap_or_default()
+            .as_nanos();
+        std::env::temp_dir().join(format!("{}_{}_{}", prefix, std::process::id(), timestamp))
+    }
+
+    fn new_file_entry(name: &str) -> FileEntry {
+        let mut entry = FileEntry::new();
+        entry.name = name.to_string();
+        entry
+    }
+
+    fn new_validation_job(id: i32) -> TransferJob {
+        TransferJob::new_write(
+            id,
+            JobType::Generic,
+            "/fake/remote".to_string(),
+            DataSource::FilePath(std::env::temp_dir().join(format!("rustdesk_validation_{id}"))),
+            0,
+            false,
+            true,
+            false,
+        )
+    }
+
+    fn new_write_job(id: i32, download_dir: PathBuf, name: &str) -> ResultType<TransferJob> {
+        let job = TransferJob::new_write(
+            id,
+            JobType::Generic,
+            "/fake/remote".to_string(),
+            DataSource::FilePath(download_dir),
+            0,
+            false,
+            true,
+            false,
+        )
+        .with_files(vec![new_file_entry(name)])?;
+        Ok(job)
+    }
+
+    fn assert_err_contains(err: anyhow::Error, expected: &str) {
+        assert!(
+            err.to_string().contains(expected),
+            "expected error containing '{}', got: {}",
+            expected,
+            err
+        );
+    }
+
+    #[test]
+    fn path_traversal_e2e_write_rejects_relative_escape() {
+        let tmp_root = TestTempDir::new("rustdesk_e2e_relative");
+        let downloads = tmp_root.join("downloads");
+        std::fs::create_dir_all(&downloads).expect("create downloads dir");
+
+        let err = new_write_job(1, downloads, "../traversal_proof.txt")
+            .expect_err("relative path traversal must be rejected");
+        assert_err_contains(err, "path traversal");
+        assert!(!tmp_root.join("traversal_proof.txt").exists());
+    }
+
+    #[test]
+    fn path_traversal_e2e_write_rejects_absolute_path() {
+        let tmp_root = TestTempDir::new("rustdesk_e2e_absolute");
+        let downloads = tmp_root.join("downloads");
+        let absolute_target = tmp_root.join("fake_ssh").join("authorized_keys");
+        std::fs::create_dir_all(&downloads).expect("create downloads dir");
+
+        let err = new_write_job(2, downloads, &absolute_target.to_string_lossy())
+            .expect_err("absolute path must be rejected");
+        assert_err_contains(err, "absolute path");
+        assert!(!absolute_target.exists());
+    }
+
+    #[test]
+    #[cfg_attr(windows, ignore = "requires symlink privilege to create test symlink")]
+    fn path_traversal_e2e_write_rejects_symlink_escape() {
+        let tmp_root = TestTempDir::new("rustdesk_e2e_symlink");
+        let downloads = tmp_root.join("downloads");
+        let outside = tmp_root.join("outside");
+        let escaped_target = outside.join("escape.txt");
+        std::fs::create_dir_all(&downloads).expect("create downloads dir");
+        std::fs::create_dir_all(&outside).expect("create outside dir");
+
+        let symlink_path = downloads.join("link");
+        #[cfg(unix)]
+        {
+            use std::os::unix::fs::symlink;
+            symlink(&outside, &symlink_path).expect("create symlink for test");
+        }
+        #[cfg(windows)]
+        {
+            use std::os::windows::fs::symlink_dir;
+            symlink_dir(&outside, &symlink_path).expect("create directory symlink for test");
+        }
+
+        let err = new_write_job(3, downloads, "link/escape.txt")
+            .expect_err("symlink traversal must be rejected");
+        assert_err_contains(err, "symlink");
+        assert!(!escaped_target.exists());
+    }
+
+    #[test]
+    fn set_files_allows_single_empty_name_for_single_file_transfer() {
+        let mut job = new_validation_job(101);
+        assert!(job.set_files(vec![new_file_entry("")]).is_ok());
+    }
+
+    #[test]
+    fn set_files_rejects_empty_name_in_multi_file_transfer() {
+        let mut job = new_validation_job(102);
+        let err = job
+            .set_files(vec![new_file_entry(""), new_file_entry("ok.txt")])
+            .expect_err("empty name in multi-file transfer must be rejected");
+        assert_err_contains(err, "empty file name");
+    }
+
+    #[test]
+    fn set_files_rejects_null_byte_name() {
+        let mut job = new_validation_job(103);
+        let err = job
+            .set_files(vec![new_file_entry("bad\0name.txt")])
+            .expect_err("null byte in file name must be rejected");
+        assert_err_contains(err, "null bytes");
+    }
+
+    #[test]
+    fn set_files_rejects_mixed_entries_when_one_is_traversal() {
+        let mut job = new_validation_job(104);
+        let err = job
+            .set_files(vec![
+                new_file_entry("safe/file.txt"),
+                new_file_entry("../../escape.txt"),
+            ])
+            .expect_err("any traversal entry must reject the full file list");
+        assert_err_contains(err, "path traversal");
+    }
+
+    #[cfg(windows)]
+    #[test]
+    fn set_files_rejects_unc_absolute_path() {
+        let mut job = new_validation_job(105);
+        let err = job
+            .set_files(vec![new_file_entry("\\\\server\\share\\payload.txt")])
+            .expect_err("UNC absolute path must be rejected");
+        assert_err_contains(err, "absolute path");
+    }
+
+    #[cfg(not(windows))]
+    #[test]
+    fn set_files_allows_backslash_prefixed_name_on_unix() {
+        let mut job = new_validation_job(105);
+        assert!(job
+            .set_files(vec![new_file_entry("\\\\server\\share\\payload.txt")])
+            .is_ok());
+    }
+
+    #[test]
+    fn remove_file_rejects_empty_path() {
+        let err = remove_file("").expect_err("empty file path must be rejected");
+        assert_err_contains(err, "cannot be empty");
+    }
+
+    #[test]
+    fn remove_file_rejects_null_byte_path() {
+        let err = remove_file("bad\0path").expect_err("null byte path must be rejected");
+        assert_err_contains(err, "null bytes");
+    }
+
+    #[test]
+    fn create_dir_rejects_empty_path() {
+        let err = create_dir("").expect_err("empty directory path must be rejected");
+        assert_err_contains(err, "cannot be empty");
+    }
+
+    #[test]
+    fn create_dir_rejects_null_byte_path() {
+        let err = create_dir("bad\0path").expect_err("null byte path must be rejected");
+        assert_err_contains(err, "null bytes");
+    }
+
+    #[test]
+    fn rename_file_rejects_invalid_new_name() {
+        let tmp_root = TestTempDir::new("rustdesk_rename_invalid");
+        let src = tmp_root.join("source.txt");
+        std::fs::create_dir_all(&tmp_root.path).expect("create temp dir");
+        std::fs::write(&src, b"content").expect("create source file");
+
+        let src_str = src.to_string_lossy().to_string();
+
+        let err_empty =
+            rename_file(&src_str, "").expect_err("empty new file name must be rejected");
+        assert_err_contains(err_empty, "cannot be empty");
+
+        let err_traversal = rename_file(&src_str, "../escape.txt")
+            .expect_err("traversal new file name must be rejected");
+        assert_err_contains(err_traversal, "path traversal");
+
+        let err_null = rename_file(&src_str, "bad\0name.txt")
+            .expect_err("null byte in new file name must be rejected");
+        assert_err_contains(err_null, "null bytes");
+
+        #[cfg(windows)]
+        {
+            let err_abs = rename_file(&src_str, "C:\\Windows\\Temp\\payload.txt")
+                .expect_err("absolute new file name must be rejected");
+            assert_err_contains(err_abs, "absolute path");
+        }
+        #[cfg(not(windows))]
+        {
+            let err_abs = rename_file(&src_str, "/tmp/payload.txt")
+                .expect_err("absolute new file name must be rejected");
+            assert_err_contains(err_abs, "absolute path");
+        }
+    }
+
+    #[test]
+    fn rename_file_accepts_valid_new_name() {
+        let tmp_root = TestTempDir::new("rustdesk_rename_ok");
+        let src = tmp_root.join("rename_src.txt");
+        let dst = tmp_root.join("renamed.txt");
+        std::fs::create_dir_all(&tmp_root.path).expect("create temp dir");
+        std::fs::write(&src, b"content").expect("create source file");
+
+        let src_str = src.to_string_lossy().to_string();
+        rename_file(&src_str, "renamed.txt").expect("rename should succeed");
+
+        assert!(!src.exists());
+        assert!(dst.exists());
+    }
+
+    #[cfg(windows)]
+    #[test]
+    fn set_files_rejects_windows_drive_absolute_path() {
+        let mut job = new_validation_job(106);
+        let err = job
+            .set_files(vec![new_file_entry("C:\\Windows\\Temp\\payload.txt")])
+            .expect_err("drive-letter absolute path must be rejected");
+        assert_err_contains(err, "absolute path");
+    }
+
+    #[cfg(windows)]
+    #[test]
+    fn set_files_rejects_windows_verbatim_drive_absolute_path() {
+        let mut job = new_validation_job(1061);
+        let err = job
+            .set_files(vec![new_file_entry(r"\\?\C:\Windows\Temp\x.txt")])
+            .expect_err("verbatim drive absolute path must be rejected");
+        assert_err_contains(err, "absolute path");
+    }
+}