fix(clipboard): validate files (#15693)

* fix(clipboard): validate files Signed-off-by: fufesou <linlong1266@gmail.com> * 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 <linlong1266@gmail.com> * update hbb_common Signed-off-by: fufesou <linlong1266@gmail.com> * 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 <linlong1266@gmail.com> * fix: clipboard, validate file name length Signed-off-by: fufesou <linlong1266@gmail.com> * fix: clipboard, comments Signed-off-by: fufesou <linlong1266@gmail.com> * 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 <linlong1266@gmail.com> * fix(clipboard): unix, select multiple items Signed-off-by: fufesou <linlong1266@gmail.com> --------- Signed-off-by: fufesou <linlong1266@gmail.com>
2026-08-25 07:36:41 +00:00 · 2026-08-04 14:11:27 +08:00
parent 4389687d9d
commit 6f1eb164d6
7 changed files with 641 additions and 112 deletions
@@ -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::<u16>();
+        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::<u32>();
+    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<u8> {
+        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::<u32>()]
+            .copy_from_slice(&FLAGS_FD_ATTRIBUTES.to_le_bytes());
+        pdu[ATTRIBUTES_OFFSET..ATTRIBUTES_OFFSET + size_of::<u32>()]
+            .copy_from_slice(&FILE_ATTRIBUTE_NORMAL.to_le_bytes());
+        for (index, unit) in name.encode_utf16().enumerate() {
+            let offset = NAME_OFFSET + index * size_of::<u16>();
+            pdu[offset..offset + size_of::<u16>()].copy_from_slice(&unit.to_le_bytes());
+        }
+        pdu
+    }
+
+    fn parse_name(name: &str) -> Result<Vec<FileDescription>, 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::<u16>();
+        pdu[padding_offset..padding_offset + size_of::<u16>()]
+            .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 { .. })
+        ));
+    }
+}
@@ -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::<u16>();
+
 #[derive(Debug)]
 pub(super) struct LocalFile {
-    pub relative_root: PathBuf,
    pub path: PathBuf,

    pub handle: Option<BufReader<File>>,
    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<String, CliprdrError> {
+        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<Self, CliprdrError> {
+        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<u8> {
-        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<utf16string::LE> = 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<Vec<u8>, 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<utf16string::LE> = 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<Vec<LocalFile>, 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<Vec<LocalFile>, 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<LocalFile> {
@@ -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(())
    }

@@ -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<Path>) -> std::io::Result<File> {
+    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<PathBuf, CliprdrError> {
+        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<String> {
-        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<String> {
+        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<FileDescription>) -> 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());
+    }
+}
@@ -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::<u16>();
+
 // 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;
@@ -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<ClipboardFile> {
@@ -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<u8> {
@@ -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),
+        ("bad<name", false),
+        ("bad>name", 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
+        );
+    }
+}
@@ -27,6 +27,7 @@

 #include <ole2.h>
 #include <shlobj.h>
+#include <wchar.h>
 #include <windows.h>
 #include <winuser.h>
 #include <tchar.h>
@@ -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)