diff --git a/src/fs.rs b/src/fs.rs index 1aa385c58..53a82990b 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -398,7 +398,7 @@ pub struct TransferJob { pub is_resume: bool, pub file_num: i32, #[serde(skip_serializing)] - pub files: Vec, + files: Vec, 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(¤t) { + 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 { + 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, 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) -> ResultType { + 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) { + pub fn set_files(&mut self, files: Vec) -> 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 { + 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 { + 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"); + } +}