diff --git a/libs/hbb_common b/libs/hbb_common index f08ce5d6d..618922b2a 160000 --- a/libs/hbb_common +++ b/libs/hbb_common @@ -1 +1 @@ -Subproject commit f08ce5d6d07cd200713418ce2932769d14ff21d2 +Subproject commit 618922b2a77f7be44fc7b86e41f6cfba87d62193 diff --git a/src/client/io_loop.rs b/src/client/io_loop.rs index e0b3fcd6d..78d9a4e40 100644 --- a/src/client/io_loop.rs +++ b/src/client/io_loop.rs @@ -586,7 +586,6 @@ impl Remote { file_num, include_hidden, is_remote, - Vec::new(), od, )); allow_err!( @@ -659,7 +658,6 @@ impl Remote { file_num, include_hidden, is_remote, - Vec::new(), od, ); job.is_last_job = true; @@ -845,19 +843,7 @@ impl Remote { } } Data::CancelJob(id) => { - let mut msg_out = Message::new(); - 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); + self.cancel_transfer_job(id, peer).await; } Data::RemoveDir((id, path)) => { let mut msg_out = Message::new(); @@ -1053,6 +1039,22 @@ impl Remote { } } + 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 Remote { fs::transform_windows_path(&mut entries); } } - self.handler - .update_folder_files(fd.id, &entries, fd.path, false, false); + // We cannot call cancel_transfer_job/handle_job_status while holding + // 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); - job.set_finished_size_on_resume(); + if let Err(err) = job.set_files(entries) { + 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)) => { diff --git a/src/ui_cm_interface.rs b/src/ui_cm_interface.rs index 75e724007..19a9e74e7 100644 --- a/src/ui_cm_interface.rs +++ b/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 = 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) { #[cfg(not(any(target_os = "ios")))] async fn rename_file(path: String, new_name: String, id: i32, tx: &UnboundedSender) { handle_result( - spawn_blocking(move || { - // 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, + spawn_blocking(move || 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]