fix: file transfer, path traversal (#14678)

* fix: file transfer, path traversal

Signed-off-by: fufesou <linlong1266@gmail.com>

* fix(fs): remove stale files

Signed-off-by: fufesou <linlong1266@gmail.com>

* fix(fs): update_folder_files() after set_files()

Signed-off-by: fufesou <linlong1266@gmail.com>

* fix(fs): reduce .clone()

Signed-off-by: fufesou <linlong1266@gmail.com>

* fix(fs): undo checking "done message for unkown id"

Signed-off-by: fufesou <linlong1266@gmail.com>

* fix(fs): refactor

1. Hide `files` in `new_write()`.
2. Use `set_files()` to validate `files` before writing.

Signed-off-by: fufesou <linlong1266@gmail.com>

* fix(fs): comments

Signed-off-by: fufesou <linlong1266@gmail.com>

* fix(fs): Remove redundant checks

Signed-off-by: fufesou <linlong1266@gmail.com>

* fix(fs): update hbb_common

Signed-off-by: fufesou <linlong1266@gmail.com>

---------

Signed-off-by: fufesou <linlong1266@gmail.com>

src/client/io_loop.rs

@@ -586,7 +586,6 @@ impl<T: InvokeUiSession> Remote<T> {
                         file_num,
                         include_hidden,
                         is_remote,
-                        Vec::new(),
                         od,
                     ));
                     allow_err!(
@@ -659,7 +658,6 @@ impl<T: InvokeUiSession> Remote<T> {
                         file_num,
                         include_hidden,
                         is_remote,
-                        Vec::new(),
                         od,
                     );
                     job.is_last_job = true;
@@ -845,19 +843,7 @@ impl<T: InvokeUiSession> Remote<T> {
                 }
             }
             Data::CancelJob(id) => {
-                let mut msg_out = Message::new();
+                self.cancel_transfer_job(id, peer).await;
-                let mut file_action = FileAction::new();
-                file_action.set_cancel(FileTransferCancel {
-                    id: id,
-                    ..Default::default()
-                });
-                msg_out.set_file_action(file_action);
-                allow_err!(peer.send(&msg_out).await);
-                if let Some(job) = fs::remove_job(id, &mut self.write_jobs) {
-                    job.remove_download_file();
-                }
-                let _ = fs::remove_job(id, &mut self.read_jobs);
-                self.remove_jobs.remove(&id);
             }
             Data::RemoveDir((id, path)) => {
                 let mut msg_out = Message::new();
@@ -1053,6 +1039,22 @@ impl<T: InvokeUiSession> Remote<T> {
         }
     }
 
+    async fn cancel_transfer_job(&mut self, id: i32, peer: &mut Stream) {
+        let mut msg_out = Message::new();
+        let mut file_action = FileAction::new();
+        file_action.set_cancel(FileTransferCancel {
+            id,
+            ..Default::default()
+        });
+        msg_out.set_file_action(file_action);
+        allow_err!(peer.send(&msg_out).await);
+        if let Some(job) = fs::remove_job(id, &mut self.write_jobs) {
+            job.remove_download_file();
+        }
+        let _ = fs::remove_job(id, &mut self.read_jobs);
+        self.remove_jobs.remove(&id);
+    }
+
     pub async fn sync_jobs_status_to_local(&mut self) -> bool {
         if !self.is_connected {
             return false;
@@ -1470,14 +1472,43 @@ impl<T: InvokeUiSession> Remote<T> {
                                     fs::transform_windows_path(&mut entries);
                                 }
                             }
-                            self.handler
+                            // We cannot call cancel_transfer_job/handle_job_status while holding
-                                .update_folder_files(fd.id, &entries, fd.path, false, false);
+                            // a mutable borrow from fs::get_job(&mut self.write_jobs), so defer
+                            // the error handling until after the borrow scope ends.
+                            let mut set_files_err = None;
                             if let Some(job) = fs::get_job(fd.id, &mut self.write_jobs) {
                                 log::info!("job set_files: {:?}", entries);
-                                job.set_files(entries);
+                                if let Err(err) = job.set_files(entries) {
-                                job.set_finished_size_on_resume();
+                                    set_files_err = Some(err.to_string());
+                                } else {
+                                    job.set_finished_size_on_resume();
+                                    self.handler.update_folder_files(
+                                        fd.id,
+                                        job.files(),
+                                        fd.path,
+                                        false,
+                                        false,
+                                    );
+                                }
                             } else if let Some(job) = self.remove_jobs.get_mut(&fd.id) {
+                                // Intentionally keep raw entries here:
+                                // - remote remove flow executes deletions on peer side;
+                                // - local remove flow is populated from local get_recursive_files().
                                 job.files = entries;
+                                self.handler
+                                    .update_folder_files(fd.id, &job.files, fd.path, false, false);
+                            } else {
+                                self.handler
+                                    .update_folder_files(fd.id, &entries, fd.path, false, false);
+                            }
+                            if let Some(err) = set_files_err {
+                                log::warn!(
+                                    "Rejected unsafe file list from remote peer for job {}: {}",
+                                    fd.id,
+                                    err
+                                );
+                                self.cancel_transfer_job(fd.id, peer).await;
+                                self.handle_job_status(fd.id, -1, Some(err));
                             }
                         }
                         Some(file_response::Union::Digest(digest)) => {

src/ui_cm_interface.rs

@@ -941,15 +941,6 @@ async fn handle_fs(
             total_size,
             conn_id,
         } => {
-            // Validate file names to prevent path traversal attacks.
-            // This must be done BEFORE any path operations to ensure attackers cannot
-            // escape the target directory using names like "../../malicious.txt"
-            if let Err(e) = validate_transfer_file_names(&files) {
-                log::warn!("Path traversal attempt detected for {}: {}", path, e);
-                send_raw(fs::new_error(id, e, file_num), tx);
-                return;
-            }
-
             // Convert files to FileEntry
             let file_entries: Vec<FileEntry> = files
                 .drain(..)
@@ -970,9 +961,13 @@ async fn handle_fs(
                 file_num,
                 false,
                 false,
-                file_entries,
                 overwrite_detection,
             );
+            if let Err(e) = job.set_files(file_entries) {
+                log::warn!("Reject unsafe transfer file list for {}: {}", path, e);
+                send_raw(fs::new_error(id, e, file_num), tx);
+                return;
+            }
             job.total_size = total_size;
             job.conn_id = conn_id;
             write_jobs.push(job);
@@ -1160,73 +1155,6 @@ async fn handle_fs(
     }
 }
 
-/// Validates that a file name does not contain path traversal sequences.
-/// This prevents attackers from escaping the base directory by using names like
-/// "../../../etc/passwd" or "..\\..\\Windows\\System32\\malicious.dll".
-#[cfg(not(any(target_os = "ios")))]
-fn validate_file_name_no_traversal(name: &str) -> ResultType<()> {
-    // Check for null bytes which could cause path truncation in some APIs
-    if name.bytes().any(|b| b == 0) {
-        bail!("file name contains null bytes");
-    }
-
-    // Check for path traversal patterns
-    // We check for both Unix and Windows path separators
-    if name
-        .split(|c| c == '/' || c == '\\')
-        .filter(|s| !s.is_empty())
-        .any(|component| component == "..")
-    {
-        bail!("path traversal detected in file name");
-    }
-
-    // On Windows, also check for drive letters (e.g., "C:")
-    #[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");
-            }
-        }
-    }
-
-    // Check for names starting with path separator:
-    // - Unix absolute paths (e.g., "/etc/passwd")
-    // - Windows UNC paths (e.g., "\\server\share")
-    if name.starts_with('/') || name.starts_with('\\') {
-        bail!("absolute path detected in file name");
-    }
-
-    Ok(())
-}
-
-#[inline]
-fn is_single_file_with_empty_name(files: &[(String, u64)]) -> bool {
-    files.len() == 1 && files.first().map_or(false, |f| f.0.is_empty())
-}
-
-/// Validates all file names in a transfer request to prevent path traversal attacks.
-/// Returns an error if any file name contains dangerous path components.
-#[cfg(not(any(target_os = "ios")))]
-fn validate_transfer_file_names(files: &[(String, u64)]) -> ResultType<()> {
-    if is_single_file_with_empty_name(files) {
-        // Allow empty name for single file.
-        // The full path is provided in the `path` parameter for single file transfers.
-        return Ok(());
-    }
-
-    for (name, _) in files {
-        // In multi-file transfers, empty names are not allowed.
-        // Each file must have a valid name to construct the destination path.
-        if name.is_empty() {
-            bail!("empty file name in multi-file transfer");
-        }
-        validate_file_name_no_traversal(name)?;
-    }
-    Ok(())
-}
-
 /// Start a read job in CM for file transfer from server to client (Windows only).
 ///
 /// This creates a `TransferJob` using `new_read()`, validates it, and sends the
@@ -1601,16 +1529,7 @@ async fn create_dir(path: String, id: i32, tx: &UnboundedSender<Data>) {
 #[cfg(not(any(target_os = "ios")))]
 async fn rename_file(path: String, new_name: String, id: i32, tx: &UnboundedSender<Data>) {
     handle_result(
-        spawn_blocking(move || {
+        spawn_blocking(move || fs::rename_file(&path, &new_name)).await,
-            // Rename target must not be empty
-            if new_name.is_empty() {
-                bail!("new file name cannot be empty");
-            }
-            // Validate that new_name doesn't contain path traversal
-            validate_file_name_no_traversal(&new_name)?;
-            fs::rename_file(&path, &new_name)
-        })
-        .await,
         id,
         0,
         tx,
@@ -1773,42 +1692,6 @@ mod tests {
         });
     }
 
-    #[test]
-    #[cfg(not(any(target_os = "ios")))]
-    fn validate_file_name_security() {
-        // Null byte injection
-        assert!(super::validate_file_name_no_traversal("file\0.txt").is_err());
-        assert!(super::validate_file_name_no_traversal("test\0").is_err());
-
-        // Path traversal
-        assert!(super::validate_file_name_no_traversal("../etc/passwd").is_err());
-        assert!(super::validate_file_name_no_traversal("foo/../bar").is_err());
-        assert!(super::validate_file_name_no_traversal("..").is_err());
-
-        // Absolute paths
-        assert!(super::validate_file_name_no_traversal("/etc/passwd").is_err());
-        assert!(super::validate_file_name_no_traversal("\\Windows").is_err());
-        #[cfg(windows)]
-        assert!(super::validate_file_name_no_traversal("C:\\Windows").is_err());
-
-        // Valid paths
-        assert!(super::validate_file_name_no_traversal("file.txt").is_ok());
-        assert!(super::validate_file_name_no_traversal("subdir/file.txt").is_ok());
-        assert!(super::validate_file_name_no_traversal("").is_ok());
-    }
-
-    #[test]
-    #[cfg(not(any(target_os = "ios")))]
-    fn validate_transfer_file_names_security() {
-        assert!(super::validate_transfer_file_names(&[("file.txt".into(), 100)]).is_ok());
-        assert!(super::validate_transfer_file_names(&[("".into(), 100)]).is_ok());
-        assert!(
-            super::validate_transfer_file_names(&[("".into(), 100), ("file.txt".into(), 100)])
-                .is_err()
-        );
-        assert!(super::validate_transfer_file_names(&[("../passwd".into(), 100)]).is_err());
-    }
-
     /// Tests that symlink creation works on this platform.
     /// This is a helper to verify the test environment supports symlinks.
     #[test]