From 6f1eb164d616e0e2bfbcf8c6b7c8083b09d7ed06 Mon Sep 17 00:00:00 2001 From: fufesou Date: Tue, 4 Aug 2026 14:11:27 +0800 Subject: [PATCH] fix(clipboard): validate files (#15693) * fix(clipboard): validate files Signed-off-by: fufesou * fix(clipboard): address file validation review feedback - remove unreachable empty-prefix test assertions - name the shared COM/LPT prefix length - document non-atomic path validation behavior Signed-off-by: fufesou * update hbb_common Signed-off-by: fufesou * fix(clipboard): reject traversal in file descriptors - reuse parser validation for outgoing descriptor names - propagate descriptor serialization errors - add regression coverage for parent path components Signed-off-by: fufesou * fix: clipboard, validate file name length Signed-off-by: fufesou * fix: clipboard, comments Signed-off-by: fufesou * fix(clipboard): support multi-root file selections Use each top-level path's parent as its relative root so file descriptors remain safe and relative across different directories. Add regression coverage for multi-root selections. Signed-off-by: fufesou * fix(clipboard): unix, select multiple items Signed-off-by: fufesou --------- Signed-off-by: fufesou --- libs/clipboard/src/platform/unix/filetype.rs | 119 ++++++++- .../clipboard/src/platform/unix/local_file.rs | 200 +++++++++++----- .../src/platform/unix/macos/paste_task.rs | 225 +++++++++++++++--- libs/clipboard/src/platform/unix/mod.rs | 4 + .../clipboard/src/platform/unix/serv_files.rs | 9 +- libs/clipboard/src/platform/windows.rs | 76 ++++++ libs/clipboard/src/windows/wf_cliprdr.c | 120 ++++++++++ 7 files changed, 641 insertions(+), 112 deletions(-) diff --git a/libs/clipboard/src/platform/unix/filetype.rs b/libs/clipboard/src/platform/unix/filetype.rs index 8436ba05e..ca5cc0a5e 100644 --- a/libs/clipboard/src/platform/unix/filetype.rs +++ b/libs/clipboard/src/platform/unix/filetype.rs @@ -1,4 +1,7 @@ -use super::{FLAGS_FD_ATTRIBUTES, FLAGS_FD_LAST_WRITE, FLAGS_FD_UNIX_MODE, LDAP_EPOCH_DELTA}; +use super::{ + FILE_NAME_FIELD_SIZE, FLAGS_FD_ATTRIBUTES, FLAGS_FD_LAST_WRITE, FLAGS_FD_UNIX_MODE, + LDAP_EPOCH_DELTA, +}; use crate::CliprdrError; use hbb_common::{ bytes::{Buf, Bytes}, @@ -47,6 +50,23 @@ pub struct FileDescription { pub perm: u16, } +pub(super) fn validate_file_name(name: &str) -> Result<(), CliprdrError> { + if matches!(name.as_bytes(), [letter, b':', b'/', ..] if letter.is_ascii_alphabetic()) + || name + .split('/') + .any(|component| component.is_empty() || component == ".") + { + return Err(CliprdrError::InvalidRequest { + description: "clipboard file name is not a normalized relative path".to_string(), + }); + } + hbb_common::fs::validate_file_name_no_traversal(name).map_err(|error| { + CliprdrError::InvalidRequest { + description: error.to_string(), + } + }) +} + impl FileDescription { fn parse_file_descriptor( bytes: &mut Bytes, @@ -68,13 +88,21 @@ impl FileDescription { // file size let file_size_high = bytes.get_u32_le(); let file_size_low = bytes.get_u32_le(); - // utf16 file name, double \0 terminated, in 520 bytes block + // NUL-terminated UTF-16 file name in a fixed-size field. // read with another pointer, and advance the main pointer let block = bytes.clone(); - bytes.advance(520); + bytes.advance(FILE_NAME_FIELD_SIZE); - let block = &block[..520]; - let wstr = WStr::from_utf16le(block).map_err(|e| { + let block = &block[..FILE_NAME_FIELD_SIZE]; + let utf16_unit_size = std::mem::size_of::(); + let name_end = block + .chunks_exact(utf16_unit_size) + .position(|unit| unit == [0_u8, 0_u8]) + .ok_or_else(|| CliprdrError::InvalidRequest { + description: "clipboard file name is not null-terminated".to_string(), + })? + * utf16_unit_size; + let wstr = WStr::from_utf16le(&block[..name_end]).map_err(|e| { log::error!("cannot convert file descriptor path: {:?}", e); CliprdrError::ConversionFailure })?; @@ -136,7 +164,8 @@ impl FileDescription { }; let name = wstr.to_utf8().replace('\\', "/"); - let name = PathBuf::from(name.trim_end_matches('\0')); + validate_file_name(&name)?; + let name = PathBuf::from(name); let desc = FileDescription { conn_id, @@ -186,3 +215,81 @@ impl FileDescription { Ok(files) } } + +#[cfg(test)] +mod tests { + use super::*; + use std::mem::size_of; + + const PDU_HEADER_SIZE: usize = size_of::(); + const DESCRIPTOR_SIZE: usize = 592; + const ATTRIBUTES_OFFSET: usize = PDU_HEADER_SIZE + 36; + const NAME_OFFSET: usize = PDU_HEADER_SIZE + 72; + const FILE_NAME_CODE_UNITS: usize = 260; + const INVALID_UTF16_UNIT: u16 = 0xdc00; + const FILE_ATTRIBUTE_NORMAL: u32 = 0x80; + + fn descriptor_pdu(name: &str) -> Vec { + let mut pdu = vec![0_u8; PDU_HEADER_SIZE + DESCRIPTOR_SIZE]; + pdu[..PDU_HEADER_SIZE].copy_from_slice(&1_u32.to_le_bytes()); + pdu[PDU_HEADER_SIZE..PDU_HEADER_SIZE + size_of::()] + .copy_from_slice(&FLAGS_FD_ATTRIBUTES.to_le_bytes()); + pdu[ATTRIBUTES_OFFSET..ATTRIBUTES_OFFSET + size_of::()] + .copy_from_slice(&FILE_ATTRIBUTE_NORMAL.to_le_bytes()); + for (index, unit) in name.encode_utf16().enumerate() { + let offset = NAME_OFFSET + index * size_of::(); + pdu[offset..offset + size_of::()].copy_from_slice(&unit.to_le_bytes()); + } + pdu + } + + fn parse_name(name: &str) -> Result, CliprdrError> { + FileDescription::parse_file_descriptors(descriptor_pdu(name), 0) + } + + #[test] + fn rejects_unsafe_file_names() { + for name in [ + "../payload", + "/tmp/payload", + "C:\\payload", + "folder//payload", + "folder/./payload", + "folder/", + "", + ".", + ] { + assert!(matches!( + parse_name(name), + Err(CliprdrError::InvalidRequest { .. }) + )); + } + } + + #[test] + fn accepts_nested_relative_file_name() { + let files = parse_name("folder\\nested\\file.txt").unwrap(); + assert_eq!(files[0].name, PathBuf::from("folder/nested/file.txt")); + } + + #[test] + fn ignores_data_after_null_terminator() { + let name = "file.txt"; + let mut pdu = descriptor_pdu(name); + let padding_offset = NAME_OFFSET + (name.encode_utf16().count() + 1) * size_of::(); + pdu[padding_offset..padding_offset + size_of::()] + .copy_from_slice(&INVALID_UTF16_UNIT.to_le_bytes()); + + let files = FileDescription::parse_file_descriptors(pdu, 0).unwrap(); + assert_eq!(files[0].name, PathBuf::from("file.txt")); + } + + #[test] + fn rejects_non_terminated_file_name() { + let name = "a".repeat(FILE_NAME_CODE_UNITS); + assert!(matches!( + parse_name(&name), + Err(CliprdrError::InvalidRequest { .. }) + )); + } +} diff --git a/libs/clipboard/src/platform/unix/local_file.rs b/libs/clipboard/src/platform/unix/local_file.rs index 50c67b68f..36728e1ce 100644 --- a/libs/clipboard/src/platform/unix/local_file.rs +++ b/libs/clipboard/src/platform/unix/local_file.rs @@ -1,4 +1,7 @@ -use super::{BLOCK_SIZE, LDAP_EPOCH_DELTA}; +use super::{ + filetype::validate_file_name, BLOCK_SIZE, FILE_NAME_CODE_UNITS, FILE_NAME_FIELD_SIZE, + LDAP_EPOCH_DELTA, +}; use crate::{ platform::unix::{ FLAGS_FD_ATTRIBUTES, FLAGS_FD_LAST_WRITE, FLAGS_FD_PROGRESSUI, FLAGS_FD_SIZE, @@ -21,15 +24,19 @@ use std::{ }; use utf16string::WString; +const FILE_DESCRIPTOR_SIZE: usize = 592; +const MAX_FILE_NAME_CODE_UNITS: usize = FILE_NAME_CODE_UNITS - 1; +const UTF16_CODE_UNIT_SIZE: usize = std::mem::size_of::(); + #[derive(Debug)] pub(super) struct LocalFile { - pub relative_root: PathBuf, pub path: PathBuf, pub handle: Option>, pub offset: AtomicU64, pub name: String, + descriptor_name: String, pub size: u64, pub last_write_time: SystemTime, pub is_dir: bool, @@ -42,7 +49,38 @@ pub(super) struct LocalFile { } impl LocalFile { + fn descriptor_name_too_long_error() -> CliprdrError { + CliprdrError::InvalidRequest { + description: format!( + "clipboard file name exceeds {MAX_FILE_NAME_CODE_UNITS} UTF-16 code units" + ), + } + } + + fn validated_descriptor_name( + relative_root: &Path, + path: &Path, + ) -> Result { + let descriptor_path = + path.strip_prefix(relative_root) + .map_err(|_| CliprdrError::InvalidRequest { + description: "clipboard file path is outside its relative root".to_string(), + })?; + if descriptor_path.is_absolute() { + return Err(CliprdrError::InvalidRequest { + description: "clipboard file path must be relative".to_string(), + }); + } + let descriptor_name = descriptor_path.to_string_lossy().into_owned(); + validate_file_name(&descriptor_name)?; + if descriptor_name.encode_utf16().count() > MAX_FILE_NAME_CODE_UNITS { + return Err(Self::descriptor_name_too_long_error()); + } + Ok(descriptor_name) + } + pub fn try_open(relative_root: &Path, path: &Path) -> Result { + let descriptor_name = Self::validated_descriptor_name(relative_root, path)?; let mt = std::fs::metadata(path).map_err(|e| CliprdrError::FileError { path: path.to_string_lossy().to_string(), err: e, @@ -70,11 +108,11 @@ impl LocalFile { Ok(Self { name, - relative_root: relative_root.to_path_buf(), path: path.to_path_buf(), handle, offset, size, + descriptor_name, last_write_time, is_dir, read_only, @@ -85,17 +123,37 @@ impl LocalFile { normal, }) } - pub fn as_bin(&self) -> Vec { - let mut buf = BytesMut::with_capacity(592); + fn put_descriptor_name(&self, buf: &mut BytesMut) -> Result<(), CliprdrError> { + validate_file_name(&self.descriptor_name)?; + let wstr: WString = WString::from(&self.descriptor_name); + let name = wstr.as_bytes(); + let Some(name_field_size) = name.len().checked_add(UTF16_CODE_UNIT_SIZE) else { + return Err(Self::descriptor_name_too_long_error()); + }; + if name_field_size > FILE_NAME_FIELD_SIZE { + return Err(Self::descriptor_name_too_long_error()); + } + log::trace!( + "put file to list: name_len {}, name {}", + name.len(), + &self.name + ); + buf.put(name); + buf.put_u16_le(0); + buf.put_bytes(0, FILE_NAME_FIELD_SIZE - name_field_size); + Ok(()) + } + + pub fn as_bin(&self) -> Result, CliprdrError> { + let mut buf = BytesMut::with_capacity(FILE_DESCRIPTOR_SIZE); let read_only_flag = if self.read_only { 0x1 } else { 0 }; let hidden_flag = if self.hidden { 0x2 } else { 0 }; let system_flag = if self.system { 0x4 } else { 0 }; let directory_flag = if self.is_dir { 0x10 } else { 0 }; let archive_flag = if self.archive { 0x20 } else { 0 }; let normal_flag = if self.normal { 0x80 } else { 0 }; - - let file_attributes: u32 = read_only_flag + let file_attributes = read_only_flag | hidden_flag | system_flag | directory_flag @@ -112,23 +170,6 @@ impl LocalFile { let size_high = (self.size >> 32) as u32; let size_low = (self.size & (u32::MAX as u64)) as u32; - - let path = self - .path - .strip_prefix(&self.relative_root) - .unwrap_or(&self.path) - .to_string_lossy() - .into_owned(); - - let wstr: WString = WString::from(&path); - let name = wstr.as_bytes(); - - log::trace!( - "put file to list: name_len {}, name {}", - name.len(), - &self.name - ); - let flags = FLAGS_FD_SIZE | FLAGS_FD_LAST_WRITE | FLAGS_FD_ATTRIBUTES @@ -157,12 +198,10 @@ impl LocalFile { buf.put_u32_le(size_high); // file size (low) buf.put_u32_le(size_low); - // put name and padding to 520 bytes - let name_len = name.len(); - buf.put(name); - buf.put(&vec![0u8; 520 - name_len][..]); + // Put the null-terminated name and padding into the fixed-size field. + self.put_descriptor_name(&mut buf)?; - buf.to_vec() + Ok(buf.to_vec()) } #[inline] @@ -263,20 +302,18 @@ pub(super) fn construct_file_list(paths: &[PathBuf]) -> Result, C } let mut file_list = Vec::new(); - let mut visited = HashSet::new(); - let relative_root = paths - .first() - .ok_or(CliprdrError::InvalidRequest { + if paths.is_empty() { + return Err(CliprdrError::InvalidRequest { description: "empty file list".to_string(), - })? - .parent() - .ok_or(CliprdrError::InvalidRequest { - description: "empty parent".to_string(), - })? - .to_path_buf(); + }); + } for path in paths { - constr_file_lst(&relative_root, path, &mut file_list, &mut visited)?; + let relative_root = path.parent().ok_or(CliprdrError::InvalidRequest { + description: "empty parent".to_string(), + })?; + let mut visited = HashSet::new(); + constr_file_lst(relative_root, path, &mut file_list, &mut visited)?; } Ok(file_list) } @@ -284,7 +321,7 @@ pub(super) fn construct_file_list(paths: &[PathBuf]) -> Result, C #[cfg(test)] mod file_list_test { use std::{ - path::PathBuf, + path::{Path, PathBuf}, sync::atomic::{AtomicU64, Ordering}, }; @@ -292,7 +329,7 @@ mod file_list_test { use crate::{platform::unix::filetype::FileDescription, CliprdrError}; - use super::LocalFile; + use super::{LocalFile, FILE_DESCRIPTOR_SIZE, MAX_FILE_NAME_CODE_UNITS, UTF16_CODE_UNIT_SIZE}; #[inline] fn generate_tree(prefix: &str) -> Vec { @@ -304,10 +341,10 @@ mod file_list_test { #[inline] fn generate_file(path: &str, name: &str, is_dir: bool) -> LocalFile { LocalFile { - relative_root: PathBuf::from("."), path: PathBuf::from(path), handle: None, name: name.to_string(), + descriptor_name: path.to_string(), size: 0, offset: AtomicU64::new(0), last_write_time: std::time::SystemTime::UNIX_EPOCH, @@ -352,29 +389,22 @@ mod file_list_test { let mut pdu = BytesMut::with_capacity(4 + 592 * tree.len()); pdu.put_u32_le(tree.len() as u32); for file in tree { - pdu.put(file.as_bin().as_slice()); + pdu.put(file.as_bin()?.as_slice()); } let parsed = FileDescription::parse_file_descriptors(pdu.to_vec(), 0)?; assert_eq!(parsed.len(), 4); - if !prefix.is_empty() { - assert_eq!(parsed[0].name.to_str().unwrap(), format!("{}", prefix)); - assert_eq!( - parsed[1].name.to_str().unwrap(), - format!("{}/a.txt", prefix) - ); - assert_eq!(parsed[2].name.to_str().unwrap(), format!("{}/b", prefix)); - assert_eq!( - parsed[3].name.to_str().unwrap(), - format!("{}/b/c.txt", prefix) - ); - } else { - assert_eq!(parsed[0].name.to_str().unwrap(), "."); - assert_eq!(parsed[1].name.to_str().unwrap(), "a.txt"); - assert_eq!(parsed[2].name.to_str().unwrap(), "b"); - assert_eq!(parsed[3].name.to_str().unwrap(), "b/c.txt"); - } + assert_eq!(parsed[0].name.to_str().unwrap(), format!("{}", prefix)); + assert_eq!( + parsed[1].name.to_str().unwrap(), + format!("{}/a.txt", prefix) + ); + assert_eq!(parsed[2].name.to_str().unwrap(), format!("{}/b", prefix)); + assert_eq!( + parsed[3].name.to_str().unwrap(), + format!("{}/b/c.txt", prefix) + ); assert!(parsed[0].perm & 0o777 == 0o754); assert!(parsed[1].perm & 0o777 == 0o754); @@ -386,10 +416,52 @@ mod file_list_test { #[test] fn test_parse_file_descriptors() -> Result<(), CliprdrError> { - as_bin_parse_test("")?; - as_bin_parse_test("/")?; as_bin_parse_test("test")?; - as_bin_parse_test("/test")?; + as_bin_parse_test("test/nested")?; + Ok(()) + } + + #[test] + fn rejects_file_outside_relative_root() { + let result = LocalFile::try_open(Path::new("/relative/root"), Path::new("/other/file")); + assert!(matches!(result, Err(CliprdrError::InvalidRequest { .. }))); + + let result = LocalFile::try_open( + Path::new("relative/root"), + Path::new("relative/root/../outside"), + ); + assert!(matches!(result, Err(CliprdrError::InvalidRequest { .. }))); + + let mut file = generate_tree("root").remove(0); + file.descriptor_name = "../outside".to_string(); + assert!(matches!( + file.as_bin(), + Err(CliprdrError::InvalidRequest { .. }) + )); + } + + #[test] + fn validates_utf16_descriptor_name_length() -> Result<(), CliprdrError> { + let validate = |name: &str| { + let path = Path::new("root").join(name); + LocalFile::validated_descriptor_name(Path::new("root"), &path) + }; + let valid_name = validate(&"a".repeat(MAX_FILE_NAME_CODE_UNITS))?; + let oversized_name = "a".repeat(MAX_FILE_NAME_CODE_UNITS + 1); + let invalid_name = validate(&oversized_name); + let mut valid_file = generate_tree("").remove(0); + valid_file.descriptor_name = valid_name; + let valid_descriptor = valid_file.as_bin()?; + valid_file.descriptor_name = oversized_name; + let invalid_descriptor = valid_file.as_bin(); + + assert_eq!(valid_descriptor.len(), FILE_DESCRIPTOR_SIZE); + assert!(valid_descriptor.ends_with(&[0_u8; UTF16_CODE_UNIT_SIZE])); + assert!(invalid_name.is_err()); + assert!(matches!( + invalid_descriptor, + Err(CliprdrError::InvalidRequest { .. }) + )); Ok(()) } diff --git a/libs/clipboard/src/platform/unix/macos/paste_task.rs b/libs/clipboard/src/platform/unix/macos/paste_task.rs index 33a11ed6c..5885ea655 100644 --- a/libs/clipboard/src/platform/unix/macos/paste_task.rs +++ b/libs/clipboard/src/platform/unix/macos/paste_task.rs @@ -2,10 +2,10 @@ use crate::{ platform::unix::{FileDescription, FileType, BLOCK_SIZE}, send_data, ClipboardFile, CliprdrError, ProgressPercent, }; -use hbb_common::{allow_err, log, tokio::time::Instant}; +use hbb_common::{allow_err, fs::join_validated_path, log, tokio::time::Instant}; use std::{ cmp::min, - fs::{File, FileTimes}, + fs::{File, FileTimes, OpenOptions}, io::{BufWriter, Write}, os::macos::fs::FileTimesExt, path::{Path, PathBuf}, @@ -27,6 +27,10 @@ const RECEIVE_WAIT_TIMEOUT: Duration = Duration::from_millis(5_000); const TIMESTAMP_FOR_FILE_PROGRESS_COMPLETED: u64 = 443779200; const ATTR_PROGRESS_FRACTION_COMPLETED: &str = "com.apple.progress.fractionCompleted"; +fn create_new_file(path: impl AsRef) -> std::io::Result { + OpenOptions::new().write(true).create_new(true).open(path) +} + pub struct FileContentsResponse { pub conn_id: i32, pub msg_flags: i32, @@ -117,7 +121,15 @@ impl PasteTask { target_dir, files, }; - task_handle.update_next(0).ok(); + // Path validation and creation are not atomic. Local filesystem changes can + // invalidate checked paths, and entries created before an error are not rolled back. + if let Err(error) = task_handle + .validate_paths() + .and_then(|_| task_handle.update_next(0)) + { + log::error!("Failed to initialize paste task: {}", &error); + task_handle.on_error(error); + } if task_handle.is_finished() { task_handle.on_finished(); } else { @@ -250,6 +262,13 @@ impl PasteTask { } impl PasteTaskHandle { + fn validate_paths(&self) -> Result<(), CliprdrError> { + for file in &self.files { + Self::join_file_path(&self.target_dir, &file.name)?; + } + Ok(()) + } + fn update_next(&mut self, size: u64) -> Result<(), CliprdrError> { if self.is_finished() { return Ok(()); @@ -259,7 +278,7 @@ impl PasteTaskHandle { let is_start = self.progress.list_index == -1; if is_start || (self.progress.offset + size) >= self.progress.download_file_size { if !is_start { - self.on_done(); + self.on_done()?; } for i in (self.progress.list_index + 1)..self.files.len() as i32 { let Some(file_desc) = self.files.get(i as usize) else { @@ -270,14 +289,12 @@ impl PasteTaskHandle { match file_desc.kind { FileType::File => { if file_desc.size == 0 { - if let Some(new_file_path) = - Self::get_new_filename(&self.target_dir, file_desc) - { - if let Ok(f) = std::fs::File::create(&new_file_path) { - f.set_len(0).ok(); - Self::set_file_metadata(&f, file_desc); - } - }; + let path = Self::join_file_path(&self.target_dir, &file_desc.name)?; + if let Some(path) = Self::get_new_filename(path, file_desc) { + let f = create_new_file(&path) + .map_err(|err| CliprdrError::FileError { path, err })?; + Self::set_file_metadata(&f, file_desc); + } } else { self.progress.list_index = i; self.progress.offset = 0; @@ -286,10 +303,11 @@ impl PasteTaskHandle { } } FileType::Directory => { - let path = self.target_dir.join(&file_desc.name); - if !path.exists() { - std::fs::create_dir_all(path).ok(); - } + let path = Self::join_file_path(&self.target_dir, &file_desc.name)?; + std::fs::create_dir_all(&path).map_err(|err| CliprdrError::FileError { + path: path.to_string_lossy().to_string(), + err, + })?; } FileType::Symlink => { // to-do: handle symlink @@ -362,9 +380,7 @@ impl PasteTaskHandle { }); }; - let original_file_path = self - .target_dir - .join(&file.name) + let original_file_path = Self::join_file_path(&self.target_dir, &file.name)? .to_string_lossy() .to_string(); let Some(download_file_path) = Self::get_first_filename( @@ -391,7 +407,7 @@ impl PasteTaskHandle { }); } } - match std::fs::File::create(&download_file_path) { + match create_new_file(&download_file_path) { Ok(handle) => { let writer = BufWriter::with_capacity(BLOCK_SIZE as usize * 2, handle); self.progress.download_file_index = self.progress.list_index; @@ -446,6 +462,15 @@ impl PasteTaskHandle { None } + fn join_file_path(target_dir: &PathBuf, name: &Path) -> Result { + let name = name.to_str().ok_or_else(|| CliprdrError::InvalidRequest { + description: "clipboard file name is not valid UTF-8".to_string(), + })?; + join_validated_path(target_dir, name).map_err(|error| CliprdrError::InvalidRequest { + description: error.to_string(), + }) + } + fn progress_percent(&self) -> ProgressPercent { let percent = self.progress.current_size as f64 / self.progress.total_size as f64; ProgressPercent { @@ -476,8 +501,12 @@ impl PasteTaskHandle { fn on_finished(&mut self) { if self.progress.error.is_some() { self.on_cancelled(); - } else { - self.on_done(); + return; + } + if let Err(error) = self.on_done() { + log::error!("Failed to finish paste task: {}", &error); + self.on_error(error); + return; } if self.progress.current_size != self.progress.total_size { self.progress.error = Some(CliprdrError::InvalidRequest { @@ -496,15 +525,16 @@ impl PasteTaskHandle { std::fs::remove_file(&self.progress.download_file_path).ok(); } - fn on_done(&mut self) { + fn on_done(&mut self) -> Result<(), CliprdrError> { self.update_progress_completed(Some(1.0)); Self::remove_progress_completed(&self.progress.download_file_path); let Some(file) = self.progress.file_handle.as_mut() else { - return; + return Ok(()); }; if self.progress.download_file_index == PasteTask::INVALID_FILE_INDEX { - return; + log::error!("Invalid download file index"); + return Ok(()); } if let Err(e) = file.flush() { @@ -518,26 +548,26 @@ impl PasteTaskHandle { "Failed to get file description: {}", self.progress.download_file_index ); - return; + return Ok(()); }; - let Some(rename_to_path) = Self::get_new_filename(&self.target_dir, file_desc) else { - return; + let path = Self::join_file_path(&self.target_dir, &file_desc.name)?; + let Some(rename_to_path) = Self::get_new_filename(path, file_desc) else { + return Ok(()); }; - match std::fs::rename(&self.progress.download_file_path, &rename_to_path) { - Ok(_) => Self::set_file_metadata2(&rename_to_path, file_desc), - Err(e) => { - log::error!("Failed to rename file: {:?}", e); + std::fs::rename(&self.progress.download_file_path, &rename_to_path).map_err(|err| { + CliprdrError::FileError { + path: rename_to_path.clone(), + err, } - } + })?; + Self::set_file_metadata2(&rename_to_path, file_desc); self.progress.download_file_path = "".to_owned(); self.progress.download_file_index = PasteTask::INVALID_FILE_INDEX; + Ok(()) } - fn get_new_filename(target_dir: &PathBuf, file_desc: &FileDescription) -> Option { - let mut rename_to_path = target_dir - .join(&file_desc.name) - .to_string_lossy() - .to_string(); + fn get_new_filename(path: PathBuf, file_desc: &FileDescription) -> Option { + let mut rename_to_path = path.to_string_lossy().to_string(); if Path::new(&rename_to_path).exists() { let Some(new_path) = Self::get_first_filename(rename_to_path.clone(), file_desc.kind) else { @@ -637,3 +667,122 @@ impl PasteTaskHandle { Ok(()) } } + +#[cfg(test)] +mod tests { + use super::*; + use std::os::unix::fs::symlink; + + struct TestDirectory(PathBuf); + + impl Drop for TestDirectory { + fn drop(&mut self) { + std::fs::remove_dir_all(&self.0).ok(); + } + } + + fn test_directories() -> (TestDirectory, PathBuf, PathBuf) { + let temp = TestDirectory(std::env::temp_dir().join(uuid::Uuid::new_v4().to_string())); + std::fs::create_dir(&temp.0).unwrap(); + let target = temp.0.join("target"); + let outside = temp.0.join("outside"); + std::fs::create_dir(&target).unwrap(); + std::fs::create_dir(&outside).unwrap(); + (temp, target, outside) + } + + fn file_description(name: &str, kind: FileType, size: u64) -> FileDescription { + FileDescription { + conn_id: 0, + name: PathBuf::from(name), + kind, + atime: SystemTime::UNIX_EPOCH, + last_modified: SystemTime::UNIX_EPOCH, + last_metadata_changed: SystemTime::UNIX_EPOCH, + creation_time: SystemTime::UNIX_EPOCH, + size, + perm: 0, + } + } + + fn paste_task_handle(target_dir: PathBuf, files: Vec) -> PasteTaskHandle { + PasteTaskHandle { + progress: PasteTaskProgress { + list_index: -1, + offset: 0, + total_size: files.iter().map(|file| file.size).sum(), + current_size: 0, + last_sent_time: Instant::now(), + download_file_index: PasteTask::INVALID_FILE_INDEX, + download_file_size: 0, + download_file_path: String::new(), + download_file_current_size: 0, + file_handle: None, + error: None, + is_canceled: false, + }, + target_dir, + files, + } + } + + #[test] + fn validates_all_paths_before_creating_files() { + let (_temp, target, outside) = test_directories(); + symlink(&outside, target.join("link")).unwrap(); + + let files = vec![ + file_description("created.txt", FileType::File, 0), + file_description("link/escaped", FileType::Directory, 0), + ]; + let mut task = paste_task_handle(target.clone(), files); + + assert!(matches!( + task.validate_paths().and_then(|_| task.update_next(0)), + Err(CliprdrError::InvalidRequest { .. }) + )); + assert!(!target.join("created.txt").exists()); + assert!(!outside.join("escaped").exists()); + } + + #[test] + fn final_path_validation_failure_marks_task_failed_and_removes_download() { + let (_temp, target, outside) = test_directories(); + + let download_path = target.join("file.rddownload"); + let download_file = create_new_file(&download_path).unwrap(); + let files = vec![file_description("link/file.txt", FileType::File, 1)]; + let mut task = paste_task_handle(target.clone(), files); + task.progress.list_index = 1; + task.progress.current_size = 1; + task.progress.download_file_index = 0; + task.progress.download_file_size = 1; + task.progress.download_file_path = download_path.to_string_lossy().to_string(); + task.progress.download_file_current_size = 1; + task.progress.file_handle = Some(BufWriter::new(download_file)); + symlink(&outside, target.join("link")).unwrap(); + + task.on_finished(); + + assert!(matches!( + task.progress.error, + Some(CliprdrError::InvalidRequest { .. }) + )); + assert!(!download_path.exists()); + assert!(!outside.join("file.txt").exists()); + } + + #[test] + fn rejects_symlink_component_when_creating_directory() { + let (_temp, target, outside) = test_directories(); + symlink(&outside, target.join("link")).unwrap(); + + let directory = file_description("link/escaped", FileType::Directory, 0); + let mut task = paste_task_handle(target, vec![directory]); + assert!(matches!( + task.update_next(0), + Err(CliprdrError::InvalidRequest { .. }) + )); + assert!(!outside.join("escaped").exists()); + } +} diff --git a/libs/clipboard/src/platform/unix/mod.rs b/libs/clipboard/src/platform/unix/mod.rs index de5917f49..b908f3e11 100644 --- a/libs/clipboard/src/platform/unix/mod.rs +++ b/libs/clipboard/src/platform/unix/mod.rs @@ -34,6 +34,10 @@ pub const FILECONTENTS_FORMAT_NAME: &str = "FileContents"; /// block size for fuse, align to our asynchronic request size over FileContentsRequest. pub(crate) const BLOCK_SIZE: u32 = 4 * 1024 * 1024; +/// `FILEDESCRIPTORW::cFileName` capacity, including the trailing NUL code unit. +pub(super) const FILE_NAME_CODE_UNITS: usize = 260; +pub(super) const FILE_NAME_FIELD_SIZE: usize = FILE_NAME_CODE_UNITS * std::mem::size_of::(); + // begin of epoch used by microsoft // 1601-01-01 00:00:00 + LDAP_EPOCH_DELTA*(100 ns) = 1970-01-01 00:00:00 const LDAP_EPOCH_DELTA: u64 = 116444772610000000; diff --git a/libs/clipboard/src/platform/unix/serv_files.rs b/libs/clipboard/src/platform/unix/serv_files.rs index 6fe66517c..507de3ef3 100644 --- a/libs/clipboard/src/platform/unix/serv_files.rs +++ b/libs/clipboard/src/platform/unix/serv_files.rs @@ -93,13 +93,14 @@ impl ClipFiles { Ok(()) } - fn build_file_list_pdu(&mut self) { + fn build_file_list_pdu(&mut self) -> Result<(), CliprdrError> { let mut data = BytesMut::with_capacity(4 + 592 * self.file_list.len()); data.put_u32_le(self.file_list.len() as u32); for file in self.file_list.iter() { - data.put(file.as_bin().as_slice()); + data.put(file.as_bin()?.as_slice()); } - self.files_pdu = data.to_vec() + self.files_pdu = data.to_vec(); + Ok(()) } fn get_files_for_audit(&self, request: &FileContentsRequest) -> Option { @@ -301,7 +302,7 @@ pub fn sync_files(files: &[String]) -> Result<(), CliprdrError> { return Ok(()); } files_lock.sync_files(files, current)?; - Ok(files_lock.build_file_list_pdu()) + files_lock.build_file_list_pdu() } pub fn get_file_list_pdu() -> Vec { diff --git a/libs/clipboard/src/platform/windows.rs b/libs/clipboard/src/platform/windows.rs index cdeb3e4b0..20c516d77 100644 --- a/libs/clipboard/src/platform/windows.rs +++ b/libs/clipboard/src/platform/windows.rs @@ -521,6 +521,8 @@ extern "C" { pub(crate) fn init_cliprdr(context: *mut CliprdrClientContext) -> BOOL; pub(crate) fn uninit_cliprdr(context: *mut CliprdrClientContext) -> BOOL; pub(crate) fn empty_cliprdr(context: *mut CliprdrClientContext, connID: UINT32) -> BOOL; + #[cfg(test)] + fn wf_cliprdr_file_descriptor_name_valid(name: *const WCHAR) -> BOOL; } unsafe impl Send for CliprdrClientContext {} @@ -1325,3 +1327,77 @@ extern "C" fn client_file_contents_response( } } } + +#[cfg(test)] +mod tests { + use super::*; + use std::iter::once; + + const FILE_NAME_CODE_UNITS: usize = 260; + const FILE_NAME_CASES: &[(&str, bool)] = &[ + ("", false), + ("/absolute", false), + ("C:\\absolute", false), + ("dir\\..\\payload", false), + ("dir//payload", false), + ("file.", false), + ("file ", false), + (" report.txt", false), + (" NUL.txt", false), + ("dir\\ nested.txt", false), + ("CON", false), + ("nul.txt", false), + ("dir\\AUX.log", false), + ("PRN.tar.gz", false), + ("com1", false), + ("COM\u{00b9}.txt", false), + ("COM\u{00b2}.txt", false), + ("lpt9.log", false), + ("dir/LPT\u{00b3}", false), + ("CONIN$", false), + ("dir\\conout$", false), + ("CLOCK$", false), + ("badname", false), + ("bad:name", false), + ("bad\"name", false), + ("bad|name", false), + ("bad?name", false), + ("bad*name", false), + ("bad\u{0001}name", false), + ("dir\\bad\u{001f}name", false), + ("normal.txt", true), + (".gitignore", true), + ("dir\\nested file.txt", true), + ("dir/nested file.txt", true), + ("com10.txt", true), + ("auxiliary.log", true), + ("clock$.txt", true), + ("conin$.txt", true), + ]; + + fn file_descriptor_name_valid(name: &str) -> bool { + let wide_name: Vec<_> = name.encode_utf16().chain(once(0)).collect(); + unsafe { wf_cliprdr_file_descriptor_name_valid(wide_name.as_ptr()) == TRUE } + } + + #[test] + fn validates_file_descriptor_names() { + for &(name, expected) in FILE_NAME_CASES { + assert_eq!( + file_descriptor_name_valid(name), + expected, + "unexpected validity for {name:?}" + ); + } + } + + #[test] + fn rejects_non_terminated_file_descriptor_name() { + let wide_name = [WCHAR::from(b'a'); FILE_NAME_CODE_UNITS]; + assert_eq!( + unsafe { wf_cliprdr_file_descriptor_name_valid(wide_name.as_ptr()) }, + FALSE + ); + } +} diff --git a/libs/clipboard/src/windows/wf_cliprdr.c b/libs/clipboard/src/windows/wf_cliprdr.c index b535b2ec7..d918ee1db 100644 --- a/libs/clipboard/src/windows/wf_cliprdr.c +++ b/libs/clipboard/src/windows/wf_cliprdr.c @@ -27,6 +27,7 @@ #include #include +#include #include #include #include @@ -49,6 +50,9 @@ #define WF_CLIPRDR_MAX_FORMAT_NAME_WCHARS 255u /* Bound the peer-provided UTF-8 scan separately from the converted Windows name. */ #define WF_CLIPRDR_MAX_FORMAT_NAME_UTF8_BYTES (WF_CLIPRDR_MAX_FORMAT_NAME_WCHARS * 4u) +#define WF_CLIPRDR_COM_LPT_PREFIX_LENGTH 3u +static const WCHAR WF_CLIPRDR_SUPERSCRIPT_DIGITS[] = L"\x00B9\x00B2\x00B3"; +static const WCHAR WF_CLIPRDR_INVALID_FILE_NAME_CHARS[] = L"<>:\"|?*"; /* Validates the remote descriptor array size after cItems has been read safely. */ static BOOL wf_cliprdr_file_group_descriptor_size_valid(SIZE_T size, UINT count) @@ -69,6 +73,119 @@ static BOOL wf_cliprdr_file_group_descriptor_size_valid(SIZE_T size, UINT count) return size >= descriptors_size; } +static BOOL wf_cliprdr_file_name_equals(const WCHAR *component, SIZE_T length, + const WCHAR *expected) +{ + SIZE_T expected_length = wcslen(expected); + SIZE_T i; + + if (length != expected_length) + return FALSE; + for (i = 0; i < length; i++) + { + WCHAR value = component[i]; + if (value >= L'a' && value <= L'z') + value -= L'a' - L'A'; + if (value != expected[i]) + return FALSE; + } + return TRUE; +} + +/* Windows reserves COM/LPT followed by ASCII 1-9 or superscript 1, 2, and 3. + * https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file */ +static BOOL wf_cliprdr_file_name_numbered_device(const WCHAR *component, SIZE_T length) +{ + SIZE_T prefix_length = WF_CLIPRDR_COM_LPT_PREFIX_LENGTH; + return length == prefix_length + 1 && + (wf_cliprdr_file_name_equals(component, prefix_length, L"COM") || + wf_cliprdr_file_name_equals(component, prefix_length, L"LPT")) && + ((component[prefix_length] >= L'1' && component[prefix_length] <= L'9') || + wcschr(WF_CLIPRDR_SUPERSCRIPT_DIGITS, component[prefix_length]) != NULL); +} + +/* CON/PRN/AUX/NUL/COM/LPT remain reserved when followed by an extension, so + * compare their portion before the first dot. + * https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file + * CONIN$/CONOUT$ console device names: + * https://learn.microsoft.com/en-us/windows/win32/api/fileapi/nf-fileapi-createfilew#consoles + * CLOCK$ reserved device name: + * https://learn.microsoft.com/en-us/biztalk/core/restrictions-when-configuring-the-file-adapter */ +static BOOL wf_cliprdr_file_name_reserved_device(const WCHAR *component, SIZE_T length) +{ + const WCHAR *dot = wmemchr(component, L'.', length); + SIZE_T base_length = dot ? (SIZE_T)(dot - component) : length; + return wf_cliprdr_file_name_equals(component, length, L"CONIN$") || + wf_cliprdr_file_name_equals(component, length, L"CONOUT$") || + wf_cliprdr_file_name_equals(component, length, L"CLOCK$") || + wf_cliprdr_file_name_equals(component, base_length, L"CON") || + wf_cliprdr_file_name_equals(component, base_length, L"PRN") || + wf_cliprdr_file_name_equals(component, base_length, L"AUX") || + wf_cliprdr_file_name_equals(component, base_length, L"NUL") || + wf_cliprdr_file_name_numbered_device(component, base_length); +} + +static BOOL wf_cliprdr_file_name_component_valid(const WCHAR *component, SIZE_T length) +{ + SIZE_T i; + + /* Windows removes leading/trailing ASCII spaces and trailing periods. + * Reject them so a validated remote name cannot become a different local name. + * https://learn.microsoft.com/en-us/troubleshoot/windows-client/shell-experience/file-folder-name-whitespace-characters */ + if (length == 0 || component[0] == L' ' || component[length - 1] == L'.' || + component[length - 1] == L' ') + return FALSE; + /* Path separators are parsed by the caller; reject other Win32-reserved + * punctuation and control characters here. + * https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file */ + for (i = 0; i < length; i++) + { + if (component[i] < L' ' || + wcschr(WF_CLIPRDR_INVALID_FILE_NAME_CHARS, component[i]) != NULL) + return FALSE; + } + return !wf_cliprdr_file_name_reserved_device(component, length); +} + +BOOL wf_cliprdr_file_descriptor_name_valid(const WCHAR *name) +{ + SIZE_T component_start = 0; + SIZE_T i; + + if (!name || name[0] == L'\\' || name[0] == L'/') + return FALSE; + + /* FILEDESCRIPTORW::cFileName is WCHAR[MAX_PATH]; reject names without a + * terminator within that fixed field. */ + for (i = 0; i < MAX_PATH; i++) + { + WCHAR value = name[i]; + if (value != L'\0' && value != L'\\' && value != L'/') + continue; + if (!wf_cliprdr_file_name_component_valid(&name[component_start], i - component_start)) + return FALSE; + if (value == L'\0') + return TRUE; + component_start = i + 1; + } + + return FALSE; +} + +static BOOL wf_cliprdr_file_group_descriptor_names_valid( + const FILEGROUPDESCRIPTORW *group, UINT count) +{ + UINT i; + + for (i = 0; i < count; i++) + { + if (!wf_cliprdr_file_descriptor_name_valid(group->fgd[i].cFileName)) + return FALSE; + } + + return TRUE; +} + static BOOL wf_cliprdr_bounded_strlen(const char *value, size_t max_len, size_t *len) { size_t i; @@ -909,6 +1026,9 @@ static HRESULT STDMETHODCALLTYPE CliprdrDataObject_GetData(IDataObject *This, FO if (!wf_cliprdr_file_group_descriptor_size_valid(hmem_size, stream_count)) return wf_cliprdr_fail_locked_file_descriptor_data( clipboard, pMedium, instance, NULL, 0, E_UNEXPECTED); + if (!wf_cliprdr_file_group_descriptor_names_valid(dsc, stream_count)) + return wf_cliprdr_fail_locked_file_descriptor_data( + clipboard, pMedium, instance, NULL, 0, E_UNEXPECTED); streams = (IStream **)calloc(stream_count, sizeof(IStream *)); if (!streams)