From 67bc27263f83ecf410b4b51b70427e6023e21696 Mon Sep 17 00:00:00 2001 From: fufesou Date: Mon, 30 Mar 2026 16:31:35 +0800 Subject: [PATCH 1/7] fix: file transfer, path traversal Signed-off-by: fufesou --- src/fs.rs | 333 +++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 328 insertions(+), 5 deletions(-) diff --git a/src/fs.rs b/src/fs.rs index 1aa385c58..b18f38a86 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -457,6 +457,92 @@ fn is_compressed_file(name: &str) -> bool { compressed_exts.contains(&ext) } +#[inline] +fn validate_file_name_no_traversal(name: &str) -> ResultType<()> { + if name.bytes().any(|b| b == 0) { + bail!("file name contains null bytes"); + } + #[cfg(windows)] + if name + .split(|c| c == '/' || c == '\\') + .filter(|s| !s.is_empty()) + .any(|component| component == "..") + { + bail!("path traversal detected in file name"); + } + #[cfg(not(windows))] + if name + .split('/') + .filter(|s| !s.is_empty()) + .any(|component| component == "..") + { + 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(()) +} + +#[inline] +fn validate_transfer_file_names(files: &[FileEntry]) -> ResultType<()> { + 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_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); + if let Ok(meta) = std::fs::symlink_metadata(¤t) { + if meta.file_type().is_symlink() { + bail!("symlink path component is not allowed"); + } + } + } + std::path::Component::CurDir => {} + _ => { + bail!("invalid file name component"); + } + } + } + Ok(()) +} + +#[inline] +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( @@ -538,8 +624,10 @@ 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)?; self.files = files; + Ok(()) } #[inline] @@ -581,7 +669,17 @@ 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 path = if self.r#type == JobType::Generic { + match join_validated_path(p, &entry.name) { + Ok(path) => path, + Err(err) => { + log::error!("Invalid file name in transfer job {}: {}", self.id, err); + return; + } + } + } else { + Self::join(p, &entry.name) + }; 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 +701,17 @@ 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 path = if self.r#type == JobType::Generic { + match join_validated_path(p, &entry.name) { + Ok(path) => path, + Err(err) => { + log::error!("Invalid file name in transfer job {}: {}", self.id, err); + return; + } + } + } else { + Self::join(p, &entry.name) + }; 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 +753,7 @@ 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)?; if let Some(pp) = path.parent() { std::fs::create_dir_all(pp).ok(); } @@ -969,7 +1077,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 path = if self.r#type == JobType::Generic { + match join_validated_path(p, &entry.name) { + Ok(path) => path, + Err(err) => { + log::error!("Invalid file name in transfer job {}: {}", self.id, err); + return; + } + } + } else { + Self::join(p, &entry.name) + }; let file_path = get_string(&path); let download_path = format!("{}.download", &file_path); let digest_path = format!("{}.digest", &file_path); @@ -1378,3 +1496,208 @@ 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::*; + + 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, + Vec::new(), + false, + ) + } + + fn new_write_job(id: i32, download_dir: PathBuf, name: &str) -> TransferJob { + TransferJob::new_write( + id, + JobType::Generic, + "/fake/remote".to_string(), + DataSource::FilePath(download_dir), + 0, + false, + true, + vec![new_file_entry(name)], + false, + ) + } + + fn new_block(id: i32, payload: &[u8]) -> FileTransferBlock { + let mut block = FileTransferBlock::new(); + block.id = id; + block.file_num = 0; + block.data = payload.to_vec().into(); + block + } + + fn assert_err_contains(err: anyhow::Error, expected: &str) { + assert!( + err.to_string().contains(expected), + "expected error containing '{}', got: {}", + expected, + err + ); + } + + #[tokio::test] + async fn path_traversal_e2e_write_rejects_relative_escape() { + let tmp_root = unique_temp_dir("rustdesk_e2e_relative"); + let downloads = tmp_root.join("downloads"); + std::fs::create_dir_all(&downloads).expect("create downloads dir"); + + let mut job = new_write_job(1, downloads, "../traversal_proof.txt"); + let block = new_block(1, b"malicious payload"); + let err = job + .write(block) + .await + .expect_err("relative path traversal must be rejected"); + assert_err_contains(err, "path traversal"); + assert!(!tmp_root.join("traversal_proof.txt").exists()); + + let _ = std::fs::remove_dir_all(&tmp_root); + } + + #[tokio::test] + async fn path_traversal_e2e_write_rejects_absolute_path() { + let tmp_root = unique_temp_dir("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 mut job = new_write_job(2, downloads, &absolute_target.to_string_lossy()); + let block = new_block(2, b"ssh key payload"); + let err = job + .write(block) + .await + .expect_err("absolute path must be rejected"); + assert_err_contains(err, "absolute path"); + assert!(!absolute_target.exists()); + + let _ = std::fs::remove_dir_all(&tmp_root); + } + + #[tokio::test] + async fn path_traversal_e2e_write_rejects_symlink_escape() { + let tmp_root = unique_temp_dir("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; + if symlink(&outside, &symlink_path).is_err() { + let _ = std::fs::remove_dir_all(&tmp_root); + return; + } + } + #[cfg(windows)] + { + use std::os::windows::fs::symlink_dir; + if symlink_dir(&outside, &symlink_path).is_err() { + let _ = std::fs::remove_dir_all(&tmp_root); + return; + } + } + + let mut job = new_write_job(3, downloads, "link/escape.txt"); + let block = new_block(3, b"symlink escape payload"); + let err = job + .write(block) + .await + .expect_err("symlink traversal must be rejected"); + assert_err_contains(err, "symlink"); + assert!(!escaped_target.exists()); + + let _ = std::fs::remove_dir_all(&tmp_root); + } + + #[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()); + } + + #[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"); + } +} From b50bcfb158d3aebbc24e6714780559ffdf0864af Mon Sep 17 00:00:00 2001 From: fufesou Date: Fri, 3 Apr 2026 18:03:03 +0800 Subject: [PATCH 2/7] fix(fs): tests Signed-off-by: fufesou --- src/fs.rs | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/src/fs.rs b/src/fs.rs index b18f38a86..f8e9d5aa9 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -1543,7 +1543,7 @@ mod tests { ) } - fn new_block(id: i32, payload: &[u8]) -> FileTransferBlock { + fn make_test_block(id: i32, payload: &[u8]) -> FileTransferBlock { let mut block = FileTransferBlock::new(); block.id = id; block.file_num = 0; @@ -1567,7 +1567,7 @@ mod tests { std::fs::create_dir_all(&downloads).expect("create downloads dir"); let mut job = new_write_job(1, downloads, "../traversal_proof.txt"); - let block = new_block(1, b"malicious payload"); + let block = make_test_block(1, b"malicious payload"); let err = job .write(block) .await @@ -1586,7 +1586,7 @@ mod tests { std::fs::create_dir_all(&downloads).expect("create downloads dir"); let mut job = new_write_job(2, downloads, &absolute_target.to_string_lossy()); - let block = new_block(2, b"ssh key payload"); + let block = make_test_block(2, b"ssh key payload"); let err = job .write(block) .await @@ -1610,7 +1610,8 @@ mod tests { #[cfg(unix)] { use std::os::unix::fs::symlink; - if symlink(&outside, &symlink_path).is_err() { + if let Err(err) = symlink(&outside, &symlink_path) { + eprintln!("Skipping symlink test: failed to create symlink: {err}"); let _ = std::fs::remove_dir_all(&tmp_root); return; } @@ -1618,14 +1619,17 @@ mod tests { #[cfg(windows)] { use std::os::windows::fs::symlink_dir; - if symlink_dir(&outside, &symlink_path).is_err() { + if let Err(err) = symlink_dir(&outside, &symlink_path) { + eprintln!( + "Skipping symlink test: failed to create directory symlink (requires privileges): {err}" + ); let _ = std::fs::remove_dir_all(&tmp_root); return; } } let mut job = new_write_job(3, downloads, "link/escape.txt"); - let block = new_block(3, b"symlink escape payload"); + let block = make_test_block(3, b"symlink escape payload"); let err = job .write(block) .await From 4b6eb8f9096a167f8c9f7f1441e0524d141cfb62 Mon Sep 17 00:00:00 2001 From: fufesou Date: Fri, 3 Apr 2026 20:30:57 +0800 Subject: [PATCH 3/7] fix(fs): validate symlink in set_files Signed-off-by: fufesou --- src/fs.rs | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/src/fs.rs b/src/fs.rs index f8e9d5aa9..ef1e4a2f6 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -626,6 +626,11 @@ impl TransferJob { #[inline] 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.files = files; Ok(()) } @@ -1704,4 +1709,49 @@ mod tests { .expect_err("drive-letter absolute path must be rejected"); assert_err_contains(err, "absolute path"); } + + #[test] + fn set_files_rejects_symlink_path_component() { + let tmp_root = unique_temp_dir("rustdesk_set_files_symlink"); + let downloads = tmp_root.join("downloads"); + let outside = tmp_root.join("outside"); + 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; + if symlink(&outside, &symlink_path).is_err() { + let _ = std::fs::remove_dir_all(&tmp_root); + return; + } + } + #[cfg(windows)] + { + use std::os::windows::fs::symlink_dir; + if symlink_dir(&outside, &symlink_path).is_err() { + let _ = std::fs::remove_dir_all(&tmp_root); + return; + } + } + + let mut job = TransferJob::new_write( + 107, + JobType::Generic, + "/fake/remote".to_string(), + DataSource::FilePath(downloads), + 0, + false, + true, + Vec::new(), + false, + ); + let err = job + .set_files(vec![new_file_entry("link/escape.txt")]) + .expect_err("symlink component must be rejected"); + assert_err_contains(err, "symlink"); + + let _ = std::fs::remove_dir_all(&tmp_root); + } } From 9633ad27ff4aa0f527e6a982d91b7ad3f42b7764 Mon Sep 17 00:00:00 2001 From: fufesou Date: Thu, 9 Apr 2026 14:48:52 +0800 Subject: [PATCH 4/7] refact(fs): file transfer refactor 1. Hide `files` in `new_write()`. 2. Remove duplicted code. - `resolve_entry_path()`. 3. Remove unseless `inline`. 4. Add comments. 5. Reduce `#[cfg]`. Signed-off-by: fufesou --- src/fs.rs | 138 ++++++++++++++++++++---------------------------------- 1 file changed, 51 insertions(+), 87 deletions(-) diff --git a/src/fs.rs b/src/fs.rs index ef1e4a2f6..301d7efd3 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,25 +457,15 @@ fn is_compressed_file(name: &str) -> bool { compressed_exts.contains(&ext) } -#[inline] -fn validate_file_name_no_traversal(name: &str) -> ResultType<()> { +pub fn validate_file_name_no_traversal(name: &str) -> ResultType<()> { if name.bytes().any(|b| b == 0) { bail!("file name contains null bytes"); } - #[cfg(windows)] - if name - .split(|c| c == '/' || c == '\\') + let has_traversal = name + .split(|c: char| c == '/' || (cfg!(windows) && c == '\\')) .filter(|s| !s.is_empty()) - .any(|component| component == "..") - { - bail!("path traversal detected in file name"); - } - #[cfg(not(windows))] - if name - .split('/') - .filter(|s| !s.is_empty()) - .any(|component| component == "..") - { + .any(|component| component == ".."); + if has_traversal { bail!("path traversal detected in file name"); } #[cfg(windows)] @@ -497,8 +487,9 @@ fn validate_file_name_no_traversal(name: &str) -> ResultType<()> { Ok(()) } -#[inline] 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(()); } @@ -511,7 +502,6 @@ fn validate_transfer_file_names(files: &[FileEntry]) -> ResultType<()> { Ok(()) } -#[inline] fn validate_no_symlink_components(base: &PathBuf, name: &str) -> ResultType<()> { if name.is_empty() { return Ok(()); @@ -521,6 +511,8 @@ fn validate_no_symlink_components(base: &PathBuf, name: &str) -> ResultType<()> 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. if let Ok(meta) = std::fs::symlink_metadata(¤t) { if meta.file_type().is_symlink() { bail!("symlink path component is not allowed"); @@ -536,7 +528,6 @@ fn validate_no_symlink_components(base: &PathBuf, name: &str) -> ResultType<()> Ok(()) } -#[inline] fn join_validated_path(base: &PathBuf, name: &str) -> ResultType { validate_file_name_no_traversal(name)?; validate_no_symlink_components(base, name)?; @@ -553,11 +544,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, @@ -566,13 +555,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, @@ -631,6 +625,7 @@ impl TransferJob { validate_no_symlink_components(base, &file.name)?; } } + self.total_size = files.iter().map(|x| x.size).sum(); self.files = files; Ok(()) } @@ -666,6 +661,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; @@ -674,16 +683,8 @@ impl TransferJob { let file_num = self.file_num as usize; if file_num < self.files.len() { let entry = &self.files[file_num]; - let path = if self.r#type == JobType::Generic { - match join_validated_path(p, &entry.name) { - Ok(path) => path, - Err(err) => { - log::error!("Invalid file name in transfer job {}: {}", self.id, err); - return; - } - } - } else { - 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)); @@ -706,16 +707,8 @@ impl TransferJob { let file_num = self.file_num as usize; if file_num < self.files.len() { let entry = &self.files[file_num]; - let path = if self.r#type == JobType::Generic { - match join_validated_path(p, &entry.name) { - Ok(path) => path, - Err(err) => { - log::error!("Invalid file name in transfer job {}: {}", self.id, err); - return; - } - } - } else { - 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)); @@ -1082,16 +1075,8 @@ 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 = if self.r#type == JobType::Generic { - match join_validated_path(p, &entry.name) { - Ok(path) => path, - Err(err) => { - log::error!("Invalid file name in transfer job {}: {}", self.id, err); - return; - } - } - } else { - 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); @@ -1529,13 +1514,12 @@ mod tests { 0, false, true, - Vec::new(), false, ) } - fn new_write_job(id: i32, download_dir: PathBuf, name: &str) -> TransferJob { - TransferJob::new_write( + fn new_write_job(id: i32, download_dir: PathBuf, name: &str) -> ResultType { + let job = TransferJob::new_write( id, JobType::Generic, "/fake/remote".to_string(), @@ -1543,17 +1527,10 @@ mod tests { 0, false, true, - vec![new_file_entry(name)], false, ) - } - - fn make_test_block(id: i32, payload: &[u8]) -> FileTransferBlock { - let mut block = FileTransferBlock::new(); - block.id = id; - block.file_num = 0; - block.data = payload.to_vec().into(); - block + .with_files(vec![new_file_entry(name)])?; + Ok(job) } fn assert_err_contains(err: anyhow::Error, expected: &str) { @@ -1565,17 +1542,13 @@ mod tests { ); } - #[tokio::test] - async fn path_traversal_e2e_write_rejects_relative_escape() { + #[test] + fn path_traversal_e2e_write_rejects_relative_escape() { let tmp_root = unique_temp_dir("rustdesk_e2e_relative"); let downloads = tmp_root.join("downloads"); std::fs::create_dir_all(&downloads).expect("create downloads dir"); - let mut job = new_write_job(1, downloads, "../traversal_proof.txt"); - let block = make_test_block(1, b"malicious payload"); - let err = job - .write(block) - .await + 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()); @@ -1583,18 +1556,14 @@ mod tests { let _ = std::fs::remove_dir_all(&tmp_root); } - #[tokio::test] - async fn path_traversal_e2e_write_rejects_absolute_path() { + #[test] + fn path_traversal_e2e_write_rejects_absolute_path() { let tmp_root = unique_temp_dir("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 mut job = new_write_job(2, downloads, &absolute_target.to_string_lossy()); - let block = make_test_block(2, b"ssh key payload"); - let err = job - .write(block) - .await + 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()); @@ -1602,8 +1571,8 @@ mod tests { let _ = std::fs::remove_dir_all(&tmp_root); } - #[tokio::test] - async fn path_traversal_e2e_write_rejects_symlink_escape() { + #[test] + fn path_traversal_e2e_write_rejects_symlink_escape() { let tmp_root = unique_temp_dir("rustdesk_e2e_symlink"); let downloads = tmp_root.join("downloads"); let outside = tmp_root.join("outside"); @@ -1633,11 +1602,7 @@ mod tests { } } - let mut job = new_write_job(3, downloads, "link/escape.txt"); - let block = make_test_block(3, b"symlink escape payload"); - let err = job - .write(block) - .await + 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()); @@ -1744,7 +1709,6 @@ mod tests { 0, false, true, - Vec::new(), false, ); let err = job From 7cee9938bb137fb9d5631c6fcf1dee08b1859a2c Mon Sep 17 00:00:00 2001 From: fufesou Date: Thu, 9 Apr 2026 15:22:19 +0800 Subject: [PATCH 5/7] fix(fs): refact 1. Only contiue if err is not found in validate_no_symlink_components(). 2. Refact tests, RAII to remove test dirs. 3. Comments. Signed-off-by: fufesou --- src/fs.rs | 62 ++++++++++++++++++++++++++++++++++++++----------------- 1 file changed, 43 insertions(+), 19 deletions(-) diff --git a/src/fs.rs b/src/fs.rs index 301d7efd3..9cd8b312a 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -513,9 +513,23 @@ fn validate_no_symlink_components(base: &PathBuf, name: &str) -> ResultType<()> current.push(seg); // Best-effort guard: path-based checks are inherently TOCTOU-prone // if local filesystem state changes between validation and write. - if let Ok(meta) = std::fs::symlink_metadata(¤t) { - if meta.file_type().is_symlink() { - bail!("symlink path component is not allowed"); + 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 + ); } } } @@ -1491,6 +1505,28 @@ pub fn serialize_transfer_job(job: &TransferJob, done: bool, cancel: bool, error 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) @@ -1544,7 +1580,7 @@ mod tests { #[test] fn path_traversal_e2e_write_rejects_relative_escape() { - let tmp_root = unique_temp_dir("rustdesk_e2e_relative"); + let tmp_root = TestTempDir::new("rustdesk_e2e_relative"); let downloads = tmp_root.join("downloads"); std::fs::create_dir_all(&downloads).expect("create downloads dir"); @@ -1552,13 +1588,11 @@ mod tests { .expect_err("relative path traversal must be rejected"); assert_err_contains(err, "path traversal"); assert!(!tmp_root.join("traversal_proof.txt").exists()); - - let _ = std::fs::remove_dir_all(&tmp_root); } #[test] fn path_traversal_e2e_write_rejects_absolute_path() { - let tmp_root = unique_temp_dir("rustdesk_e2e_absolute"); + 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"); @@ -1567,13 +1601,11 @@ mod tests { .expect_err("absolute path must be rejected"); assert_err_contains(err, "absolute path"); assert!(!absolute_target.exists()); - - let _ = std::fs::remove_dir_all(&tmp_root); } #[test] fn path_traversal_e2e_write_rejects_symlink_escape() { - let tmp_root = unique_temp_dir("rustdesk_e2e_symlink"); + 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"); @@ -1586,7 +1618,6 @@ mod tests { use std::os::unix::fs::symlink; if let Err(err) = symlink(&outside, &symlink_path) { eprintln!("Skipping symlink test: failed to create symlink: {err}"); - let _ = std::fs::remove_dir_all(&tmp_root); return; } } @@ -1597,7 +1628,6 @@ mod tests { eprintln!( "Skipping symlink test: failed to create directory symlink (requires privileges): {err}" ); - let _ = std::fs::remove_dir_all(&tmp_root); return; } } @@ -1606,8 +1636,6 @@ mod tests { .expect_err("symlink traversal must be rejected"); assert_err_contains(err, "symlink"); assert!(!escaped_target.exists()); - - let _ = std::fs::remove_dir_all(&tmp_root); } #[test] @@ -1677,7 +1705,7 @@ mod tests { #[test] fn set_files_rejects_symlink_path_component() { - let tmp_root = unique_temp_dir("rustdesk_set_files_symlink"); + let tmp_root = TestTempDir::new("rustdesk_set_files_symlink"); let downloads = tmp_root.join("downloads"); let outside = tmp_root.join("outside"); std::fs::create_dir_all(&downloads).expect("create downloads dir"); @@ -1688,7 +1716,6 @@ mod tests { { use std::os::unix::fs::symlink; if symlink(&outside, &symlink_path).is_err() { - let _ = std::fs::remove_dir_all(&tmp_root); return; } } @@ -1696,7 +1723,6 @@ mod tests { { use std::os::windows::fs::symlink_dir; if symlink_dir(&outside, &symlink_path).is_err() { - let _ = std::fs::remove_dir_all(&tmp_root); return; } } @@ -1715,7 +1741,5 @@ mod tests { .set_files(vec![new_file_entry("link/escape.txt")]) .expect_err("symlink component must be rejected"); assert_err_contains(err, "symlink"); - - let _ = std::fs::remove_dir_all(&tmp_root); } } From 77ab15f41f12adafaad47d9fdb24d52abe848bf9 Mon Sep 17 00:00:00 2001 From: fufesou Date: Thu, 9 Apr 2026 17:23:33 +0800 Subject: [PATCH 6/7] fix(fs): validate create, rename and remove Signed-off-by: fufesou --- src/fs.rs | 88 ++++++++++++++++++++++++++++++++++++++++++++----------- 1 file changed, 71 insertions(+), 17 deletions(-) diff --git a/src/fs.rs b/src/fs.rs index 9cd8b312a..5ce2bf913 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -464,7 +464,7 @@ pub fn validate_file_name_no_traversal(name: &str) -> ResultType<()> { let has_traversal = name .split(|c: char| c == '/' || (cfg!(windows) && c == '\\')) .filter(|s| !s.is_empty()) - .any(|component| component == ".."); + .any(is_parent_component); if has_traversal { bail!("path traversal detected in file name"); } @@ -487,6 +487,20 @@ pub fn validate_file_name_no_traversal(name: &str) -> ResultType<()> { Ok(()) } +#[inline] +fn is_parent_component(component: &str) -> bool { + #[cfg(windows)] + { + // Win32 trims trailing spaces/dots in path segments for many file APIs. + // Reject variants like ".. " / ".. ." to avoid bypassing traversal checks. + component.trim_end_matches(|c| c == ' ' || c == '.') == ".." + } + #[cfg(not(windows))] + { + component == ".." + } +} + 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. @@ -502,6 +516,17 @@ fn validate_transfer_file_names(files: &[FileEntry]) -> ResultType<()> { 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(()); @@ -766,6 +791,10 @@ impl TransferJob { (p.to_string_lossy().to_string(), None) } else { 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(); } @@ -1098,6 +1127,8 @@ impl TransferJob { 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) @@ -1382,18 +1413,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 @@ -1604,6 +1642,7 @@ mod tests { } #[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"); @@ -1616,20 +1655,12 @@ mod tests { #[cfg(unix)] { use std::os::unix::fs::symlink; - if let Err(err) = symlink(&outside, &symlink_path) { - eprintln!("Skipping symlink test: failed to create symlink: {err}"); - return; - } + symlink(&outside, &symlink_path).expect("create symlink for test"); } #[cfg(windows)] { use std::os::windows::fs::symlink_dir; - if let Err(err) = symlink_dir(&outside, &symlink_path) { - eprintln!( - "Skipping symlink test: failed to create directory symlink (requires privileges): {err}" - ); - return; - } + symlink_dir(&outside, &symlink_path).expect("create directory symlink for test"); } let err = new_write_job(3, downloads, "link/escape.txt") @@ -1703,7 +1734,34 @@ mod tests { assert_err_contains(err, "absolute path"); } + #[cfg(windows)] #[test] + fn set_files_rejects_windows_parent_with_trailing_space_or_dot() { + let mut job = new_validation_job(1061); + + let err_space = job + .set_files(vec![new_file_entry(".. /escape.txt")]) + .expect_err("parent component with trailing space must be rejected"); + assert_err_contains(err_space, "path traversal"); + + let err_dot = job + .set_files(vec![new_file_entry(".. .\\escape.txt")]) + .expect_err("parent component with trailing dot must be rejected"); + assert_err_contains(err_dot, "path traversal"); + } + + #[cfg(windows)] + #[test] + fn is_parent_component_windows_trim_end_matches_behavior() { + assert!(is_parent_component("..")); + assert!(is_parent_component(".. ")); + assert!(is_parent_component(".. .")); + assert!(!is_parent_component("...")); + assert!(!is_parent_component(".")); + } + + #[test] + #[cfg_attr(windows, ignore = "requires symlink privilege to create test symlink")] fn set_files_rejects_symlink_path_component() { let tmp_root = TestTempDir::new("rustdesk_set_files_symlink"); let downloads = tmp_root.join("downloads"); @@ -1715,16 +1773,12 @@ mod tests { #[cfg(unix)] { use std::os::unix::fs::symlink; - if symlink(&outside, &symlink_path).is_err() { - return; - } + symlink(&outside, &symlink_path).expect("create symlink for test"); } #[cfg(windows)] { use std::os::windows::fs::symlink_dir; - if symlink_dir(&outside, &symlink_path).is_err() { - return; - } + symlink_dir(&outside, &symlink_path).expect("create directory symlink for test"); } let mut job = TransferJob::new_write( From 6d44e4096466cf4da5a466bbee8d486d19f12298 Mon Sep 17 00:00:00 2001 From: fufesou Date: Thu, 9 Apr 2026 18:19:14 +0800 Subject: [PATCH 7/7] fix(fs): debug windows Signed-off-by: fufesou --- src/fs.rs | 151 ++++++++++++++++++++++++++++-------------------------- 1 file changed, 79 insertions(+), 72 deletions(-) diff --git a/src/fs.rs b/src/fs.rs index 5ce2bf913..53a82990b 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -464,7 +464,7 @@ pub fn validate_file_name_no_traversal(name: &str) -> ResultType<()> { let has_traversal = name .split(|c: char| c == '/' || (cfg!(windows) && c == '\\')) .filter(|s| !s.is_empty()) - .any(is_parent_component); + .any(|s| s == ".."); if has_traversal { bail!("path traversal detected in file name"); } @@ -487,20 +487,6 @@ pub fn validate_file_name_no_traversal(name: &str) -> ResultType<()> { Ok(()) } -#[inline] -fn is_parent_component(component: &str) -> bool { - #[cfg(windows)] - { - // Win32 trims trailing spaces/dots in path segments for many file APIs. - // Reject variants like ".. " / ".. ." to avoid bypassing traversal checks. - component.trim_end_matches(|c| c == ' ' || c == '.') == ".." - } - #[cfg(not(windows))] - { - component == ".." - } -} - 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. @@ -1724,6 +1710,80 @@ mod tests { .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() { @@ -1736,64 +1796,11 @@ mod tests { #[cfg(windows)] #[test] - fn set_files_rejects_windows_parent_with_trailing_space_or_dot() { + fn set_files_rejects_windows_verbatim_drive_absolute_path() { let mut job = new_validation_job(1061); - - let err_space = job - .set_files(vec![new_file_entry(".. /escape.txt")]) - .expect_err("parent component with trailing space must be rejected"); - assert_err_contains(err_space, "path traversal"); - - let err_dot = job - .set_files(vec![new_file_entry(".. .\\escape.txt")]) - .expect_err("parent component with trailing dot must be rejected"); - assert_err_contains(err_dot, "path traversal"); - } - - #[cfg(windows)] - #[test] - fn is_parent_component_windows_trim_end_matches_behavior() { - assert!(is_parent_component("..")); - assert!(is_parent_component(".. ")); - assert!(is_parent_component(".. .")); - assert!(!is_parent_component("...")); - assert!(!is_parent_component(".")); - } - - #[test] - #[cfg_attr(windows, ignore = "requires symlink privilege to create test symlink")] - fn set_files_rejects_symlink_path_component() { - let tmp_root = TestTempDir::new("rustdesk_set_files_symlink"); - let downloads = tmp_root.join("downloads"); - let outside = tmp_root.join("outside"); - 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 mut job = TransferJob::new_write( - 107, - JobType::Generic, - "/fake/remote".to_string(), - DataSource::FilePath(downloads), - 0, - false, - true, - false, - ); let err = job - .set_files(vec![new_file_entry("link/escape.txt")]) - .expect_err("symlink component must be rejected"); - assert_err_contains(err, "symlink"); + .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"); } }