From ef4d4e8cef54009d0f0cdafdaa15774d65972957 Mon Sep 17 00:00:00 2001 From: Kang Date: Sun, 20 Sep 2026 20:55:49 +0800 Subject: [PATCH] fix(transport): restore private download staging --- Cargo.lock | 1 + crates/nyaterm-transport/Cargo.toml | 9 + crates/nyaterm-transport/src/download_path.rs | 125 ++++++++-- .../src/download_path/windows.rs | 234 ++++++++++++++++++ 4 files changed, 352 insertions(+), 17 deletions(-) create mode 100644 crates/nyaterm-transport/src/download_path/windows.rs diff --git a/Cargo.lock b/Cargo.lock index f7bef35d..6e5e6f34 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6302,6 +6302,7 @@ dependencies = [ "tracing", "unicode-normalization", "uuid", + "windows-sys 0.61.2", "zeroize", "zmodem2", ] diff --git a/crates/nyaterm-transport/Cargo.toml b/crates/nyaterm-transport/Cargo.toml index ed1cd8b6..4283c035 100644 --- a/crates/nyaterm-transport/Cargo.toml +++ b/crates/nyaterm-transport/Cargo.toml @@ -33,3 +33,12 @@ unicode-normalization.workspace = true uuid.workspace = true zeroize.workspace = true zmodem2.workspace = true + +[target.'cfg(windows)'.dependencies] +windows-sys = { version = "0.61", features = [ + "Win32_Foundation", + "Win32_Security", + "Win32_Security_Authorization", + "Win32_Storage_FileSystem", + "Win32_System_Threading", +] } diff --git a/crates/nyaterm-transport/src/download_path.rs b/crates/nyaterm-transport/src/download_path.rs index ac819058..747dc338 100644 --- a/crates/nyaterm-transport/src/download_path.rs +++ b/crates/nyaterm-transport/src/download_path.rs @@ -1,9 +1,12 @@ //! Download target name validation and local path boundary checks. use std::fs::{self, OpenOptions}; -use std::io::{self, Write}; +use std::io::{self, Read, Write}; use std::path::{Component, Path, PathBuf}; +#[cfg(windows)] +mod windows; + /// Unlike Path::exists, treat dangling links as occupied and propagate probe errors. pub(crate) fn target_exists(target: &Path) -> io::Result { match fs::symlink_metadata(target) { @@ -130,6 +133,59 @@ pub(crate) struct DownloadTemporary { replace_existing: bool, } +struct NewTargetGuard { + path: PathBuf, + file: Option, + committed: bool, +} + +impl NewTargetGuard { + fn create(target: &Path) -> io::Result { + let file = OpenOptions::new() + .write(true) + .create_new(true) + .open(target)?; + Ok(Self { + path: target.to_path_buf(), + file: Some(file), + committed: false, + }) + } + + fn file_mut(&mut self) -> &mut fs::File { + self.file + .as_mut() + .expect("new download target file must remain open until commit") + } + + fn commit(mut self) -> io::Result<()> { + self.file_mut().sync_all()?; + self.file.take(); + self.committed = true; + Ok(()) + } +} + +impl Drop for NewTargetGuard { + fn drop(&mut self) { + if self.committed { + return; + } + self.file.take(); + let _ = fs::remove_file(&self.path); + } +} + +fn populate_new_target(source: &mut R, target: &Path) -> anyhow::Result<()> { + use anyhow::Context as _; + let mut destination = NewTargetGuard::create(target)?; + io::copy(source, destination.file_mut()) + .context("download commit failed while populating new target")?; + destination + .commit() + .context("failed to sync committed download") +} + impl DownloadTemporary { /// Copy the resume prefix during preparation, before entering the download loop. /// Any preparation failure leaves the original file intact. @@ -166,11 +222,19 @@ impl DownloadTemporary { path, replace_existing, }; - let file = OpenOptions::new() - .read(true) - .write(true) - .create_new(true) - .open(&temporary.path)?; + #[cfg(not(windows))] + let file = { + let mut options = OpenOptions::new(); + options.read(true).write(true).create_new(true); + #[cfg(unix)] + { + use std::os::unix::fs::OpenOptionsExt as _; + options.mode(0o600); + } + options.open(&temporary.path)? + }; + #[cfg(windows)] + let file = windows::create_private_file(&temporary.path)?; Ok((temporary, file)) } @@ -204,16 +268,7 @@ impl DownloadTemporary { .context("failed to sync committed download")?; } else { let mut source = fs::File::open(&self.path)?; - let mut destination = OpenOptions::new() - .write(true) - .create_new(true) - .open(target)?; - use anyhow::Context as _; - io::copy(&mut source, &mut destination) - .context("download commit failed; target may contain partial data")?; - destination - .sync_all() - .context("failed to sync committed download")?; + populate_new_target(&mut source, target)?; } Ok(()) } @@ -409,6 +464,37 @@ mod tests { Ok(()) } + struct FailingReader { + first: Option<&'static [u8]>, + } + + impl std::io::Read for FailingReader { + fn read(&mut self, buffer: &mut [u8]) -> std::io::Result { + if let Some(bytes) = self.first.take() { + let length = bytes.len().min(buffer.len()); + buffer[..length].copy_from_slice(&bytes[..length]); + return Ok(length); + } + Err(std::io::Error::other("injected read failure")) + } + } + + #[test] + fn failed_new_target_commit_removes_partial_final_file() -> anyhow::Result<()> { + let root = + std::env::temp_dir().join(format!("nyaterm-commit-cleanup-{}", nyaterm_core::uuid())); + std::fs::create_dir(&root)?; + let target = root.join("download"); + let mut source = FailingReader { + first: Some(b"partial"), + }; + assert!(super::populate_new_target(&mut source, &target).is_err()); + assert!(!target.exists()); + assert_eq!(std::fs::read_dir(&root)?.count(), 0); + std::fs::remove_dir_all(root)?; + Ok(()) + } + #[tokio::test] async fn failed_resume_preparation_preserves_source_and_cleans_temporary() -> anyhow::Result<()> { @@ -429,7 +515,7 @@ mod tests { } #[tokio::test] - async fn temporary_prefix_is_dropped_without_committing() -> anyhow::Result<()> { + async fn temporary_prefix_is_private_and_dropped_without_committing() -> anyhow::Result<()> { let root = std::env::temp_dir().join(format!("nyaterm-download-prefix-{}", nyaterm_core::uuid())); std::fs::create_dir(&root)?; @@ -438,6 +524,11 @@ mod tests { let (temporary, file) = super::DownloadTemporary::prepare_async(&root, &target, 6).await?; let path = temporary.path.clone(); assert_eq!(path.parent(), Some(root.as_path())); + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt as _; + assert_eq!(std::fs::metadata(&path)?.permissions().mode() & 0o077, 0); + } assert_eq!(std::fs::read(&path)?, b"prefix"); drop(file); drop(temporary); diff --git a/crates/nyaterm-transport/src/download_path/windows.rs b/crates/nyaterm-transport/src/download_path/windows.rs new file mode 100644 index 00000000..806958e0 --- /dev/null +++ b/crates/nyaterm-transport/src/download_path/windows.rs @@ -0,0 +1,234 @@ +//! Restrict temporary file DACLs at creation to avoid inheriting broader directory read access. + +use std::ffi::c_void; +use std::fs::File; +use std::io; +use std::mem::size_of; +use std::os::windows::ffi::OsStrExt as _; +use std::os::windows::io::{AsRawHandle as _, FromRawHandle as _, OwnedHandle}; +use std::path::Path; +use std::ptr::null_mut; + +use windows_sys::Win32::Foundation::{ + ERROR_INSUFFICIENT_BUFFER, GENERIC_READ, GENERIC_WRITE, INVALID_HANDLE_VALUE, LocalFree, +}; +use windows_sys::Win32::Security::Authorization::{ + ConvertSidToStringSidW, ConvertStringSecurityDescriptorToSecurityDescriptorW, SDDL_REVISION_1, +}; +use windows_sys::Win32::Security::{ + GetTokenInformation, SECURITY_ATTRIBUTES, TOKEN_QUERY, TOKEN_USER, TokenUser, +}; +use windows_sys::Win32::Storage::FileSystem::{ + CREATE_NEW, CreateFileW, FILE_ATTRIBUTE_NORMAL, FILE_SHARE_DELETE, FILE_SHARE_READ, + FILE_SHARE_WRITE, +}; +use windows_sys::Win32::System::Threading::{GetCurrentProcess, OpenProcessToken}; + +struct LocalAllocation(*mut c_void); + +impl Drop for LocalAllocation { + fn drop(&mut self) { + // SAFETY: The pointer comes from a Windows conversion function requiring LocalFree. + unsafe { + LocalFree(self.0); + } + } +} + +fn current_user_sid() -> io::Result { + let mut raw = null_mut(); + // SAFETY: The process pseudo-handle and output pointer are valid; + // OwnedHandle takes ownership of the token on success. + if unsafe { OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &mut raw) } == 0 { + return Err(io::Error::last_os_error()); + } + let token = unsafe { OwnedHandle::from_raw_handle(raw) }; + let mut length = 0; + unsafe { + GetTokenInformation(token.as_raw_handle(), TokenUser, null_mut(), 0, &mut length); + } + let error = io::Error::last_os_error(); + if error.raw_os_error() != Some(ERROR_INSUFFICIENT_BUFFER as i32) { + return Err(error); + } + if (length as usize) < size_of::() { + return Err(io::ErrorKind::InvalidData.into()); + } + // TOKEN_USER contains pointers; use usize for alignment and query the buffer size from Windows. + let mut buffer = vec![0usize; (length as usize).div_ceil(size_of::())]; + if unsafe { + GetTokenInformation( + token.as_raw_handle(), + TokenUser, + buffer.as_mut_ptr().cast(), + length, + &mut length, + ) + } == 0 + { + return Err(io::Error::last_os_error()); + } + let user = unsafe { &*buffer.as_ptr().cast::() }; + let mut sid = null_mut(); + if unsafe { ConvertSidToStringSidW(user.User.Sid, &mut sid) } == 0 { + return Err(io::Error::last_os_error()); + } + let _allocation = LocalAllocation(sid.cast()); + let mut length = 0; + // SAFETY: Successful conversion returns NUL-terminated UTF-16; + // the allocation remains alive until the copy completes. + unsafe { + while *sid.add(length) != 0 { + length += 1; + } + Ok(String::from_utf16_lossy(std::slice::from_raw_parts( + sid, length, + ))) + } +} + +pub(super) fn create_private_file(path: &Path) -> io::Result { + // Specify the user SID explicitly: an elevated process's default owner may be Administrators. + // P disables parent ACL inheritance; grant access only to the current user and SYSTEM. + let user = current_user_sid()?; + let sddl: Vec = format!("O:{user}D:P(A;;FA;;;{user})(A;;FA;;;SY)") + .encode_utf16() + .chain(Some(0)) + .collect(); + let mut descriptor = null_mut(); + if unsafe { + ConvertStringSecurityDescriptorToSecurityDescriptorW( + sddl.as_ptr(), + SDDL_REVISION_1, + &mut descriptor, + null_mut(), + ) + } == 0 + { + return Err(io::Error::last_os_error()); + } + let _descriptor = LocalAllocation(descriptor); + let attributes = SECURITY_ATTRIBUTES { + nLength: size_of::() as u32, + lpSecurityDescriptor: descriptor, + bInheritHandle: 0, + }; + // Support absolute paths like std file operations; canonicalizing only the parent + // does not require the new file to exist. + let parent = path + .parent() + .unwrap_or_else(|| Path::new(".")) + .canonicalize()?; + let name = path + .file_name() + .ok_or_else(|| io::Error::from(io::ErrorKind::InvalidInput))?; + let mut wide: Vec = parent.join(name).as_os_str().encode_wide().collect(); + if wide.contains(&0) { + return Err(io::ErrorKind::InvalidInput.into()); + } + wide.push(0); + // SAFETY: The path and security descriptor remain valid during the call; + // CREATE_NEW prevents overwriting an existing file. + let handle = unsafe { + CreateFileW( + wide.as_ptr(), + GENERIC_READ | GENERIC_WRITE, + FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE, + &attributes, + CREATE_NEW, + FILE_ATTRIBUTE_NORMAL, + null_mut(), + ) + }; + if handle == INVALID_HANDLE_VALUE { + return Err(io::Error::last_os_error()); + } + // SAFETY: Creation succeeded; File takes sole ownership of the handle. + // The security descriptor can be freed immediately. + Ok(unsafe { File::from_raw_handle(handle) }) +} + +#[cfg(test)] +mod tests { + use std::io::Write as _; + use std::os::windows::io::AsRawHandle as _; + use std::ptr::null_mut; + + use windows_sys::Win32::Security::Authorization::{ + ConvertSecurityDescriptorToStringSecurityDescriptorW, GetSecurityInfo, SDDL_REVISION_1, + SE_FILE_OBJECT, + }; + use windows_sys::Win32::Security::DACL_SECURITY_INFORMATION; + + fn dacl_sddl(file: &std::fs::File) -> anyhow::Result { + let mut descriptor = null_mut(); + let status = unsafe { + GetSecurityInfo( + file.as_raw_handle(), + SE_FILE_OBJECT, + DACL_SECURITY_INFORMATION, + null_mut(), + null_mut(), + null_mut(), + null_mut(), + &mut descriptor, + ) + }; + anyhow::ensure!(status == 0, "GetSecurityInfo failed: {status}"); + let _descriptor = super::LocalAllocation(descriptor); + let mut text = null_mut(); + let mut length = 0; + let converted = unsafe { + ConvertSecurityDescriptorToStringSecurityDescriptorW( + descriptor, + SDDL_REVISION_1, + DACL_SECURITY_INFORMATION, + &mut text, + &mut length, + ) + }; + anyhow::ensure!(converted != 0, "{}", std::io::Error::last_os_error()); + let _text = super::LocalAllocation(text.cast()); + Ok(unsafe { + String::from_utf16_lossy(std::slice::from_raw_parts(text, length as usize - 1)) + }) + } + + #[test] + fn private_file_has_protected_acl_and_exclusive_creation() -> anyhow::Result<()> { + let root = + std::env::temp_dir().join(format!("nyaterm-private-acl-{}", nyaterm_core::uuid())); + std::fs::create_dir(&root)?; + let path = root.join("download"); + let mut file = super::create_private_file(&path)?; + file.write_all(b"private")?; + let sddl = dacl_sddl(&file)?; + assert!(sddl.starts_with("D:P"), "{sddl}"); + assert_eq!(sddl.matches("(A;;FA;;;").count(), 2, "{sddl}"); + assert!(sddl.contains(";;;SY)"), "{sddl}"); + assert!( + !sddl.contains(";;;WD)") && !sddl.contains(";;;BU)"), + "{sddl}" + ); + drop(file); + assert!(super::create_private_file(&path).is_err()); + assert_eq!(std::fs::read(&path)?, b"private"); + std::fs::remove_dir_all(root)?; + Ok(()) + } + #[test] + fn committed_download_uses_normal_destination_acl() -> anyhow::Result<()> { + let root = std::env::temp_dir().join(format!("nyaterm-final-acl-{}", nyaterm_core::uuid())); + std::fs::create_dir(&root)?; + let reference = root.join("reference"); + std::fs::write(&reference, b"normal")?; + let target = root.join("download"); + crate::download_path::staged_write_file(&root, &target, b"downloaded")?; + let reference_sddl = dacl_sddl(&std::fs::File::open(&reference)?)?; + let target_sddl = dacl_sddl(&std::fs::File::open(&target)?)?; + assert_eq!(target_sddl, reference_sddl); + assert_eq!(std::fs::read(&target)?, b"downloaded"); + std::fs::remove_dir_all(root)?; + Ok(()) + } +}