diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 6d74cc29..346a867d 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -9,6 +9,14 @@ this package adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Fixed +- **A new file published by `ops_write_file` keeps the directory's inherited grants.** Staging + objects are created with a protected, creator-only DACL, and publication never restored + inheritance for the create path (#420), so the published file carried exactly one explicit ACE. + Any principal that authorizes through the directory — a confined workspace token, another tool, + another user — was cut off, and the sandbox that wrote the file could not unlink it. `publish_new` + now adopts the directory's object-inherit ACEs before the hard link is made, which is after the + bytes are complete, so the private staging DACL keeps its purpose. Overwrites are unchanged: they + still capture and restore the target's own descriptor. - Withdraw PTC publication and cancel calls when a session leaves its selected preset; keep other sessions' discovery selections intact and authorize external calls only in their owned wrapper. - Diagnose the selected discovery/shell services and scoped publication; clarify the host's diff --git a/native/Cargo.lock b/native/Cargo.lock index 0f17a9d2..a7d4f197 100644 --- a/native/Cargo.lock +++ b/native/Cargo.lock @@ -393,6 +393,7 @@ dependencies = [ "serde_json", "sha2", "uuid", + "windows-permissions", ] [[package]] diff --git a/native/crates/ops-files/Cargo.toml b/native/crates/ops-files/Cargo.toml index f31239b3..625c47b6 100644 --- a/native/crates/ops-files/Cargo.toml +++ b/native/crates/ops-files/Cargo.toml @@ -20,5 +20,8 @@ uuid = { version = "1.26", features = ["v4"] } [target.'cfg(windows)'.dependencies.dsh-ops-windows-metadata] path = "../../windows-metadata" +[target.'cfg(windows)'.dev-dependencies] +windows-permissions = "=0.2.4" + [target.'cfg(target_os = "macos")'.dependencies.dsh-ops-darwin-metadata] path = "../../darwin-metadata" diff --git a/native/crates/ops-files/src/file_publication.rs b/native/crates/ops-files/src/file_publication.rs index 39f7fbae..659b32eb 100644 --- a/native/crates/ops-files/src/file_publication.rs +++ b/native/crates/ops-files/src/file_publication.rs @@ -220,7 +220,7 @@ pub(super) fn create_sibling( } #[cfg(windows)] -fn metadata_error(error: std::io::Error) -> Failure { +pub(super) fn metadata_error(error: std::io::Error) -> Failure { // The source was opened separately: failures inside capture/restore prove // metadata cannot be guaranteed, not that the DSH selector denied a write. Failure::new("METADATA_UNSUPPORTED", error) diff --git a/native/crates/ops-files/src/file_single.rs b/native/crates/ops-files/src/file_single.rs index a2426655..3803ee21 100644 --- a/native/crates/ops-files/src/file_single.rs +++ b/native/crates/ops-files/src/file_single.rs @@ -414,6 +414,21 @@ fn publish_new( return Err(error); } } + #[cfg(windows)] + if source.is_none() { + // The private staging DACL exists to hide half-written bytes; it must not outlive + // publication. Adopt the directory's inheritable grants before the link is made so + // the workspace token that owns the new file can still unlink it afterwards. + let parent = checked.parent.try_clone()?.into_std_file(); + let child = file.try_clone()?.into_std(); + let adopted = dsh_ops_windows_metadata::adopt_parent_dacl(&parent, &child) + .map_err(publication::metadata_error); + drop(child); + if let Err(error) = adopted { + publication::discard_sibling(&checked.parent, &temporary, file)?; + return Err(error); + } + } #[cfg(target_os = "macos")] { // RENAME_EXCL publishes an absent destination without a staging unlink. @@ -638,4 +653,75 @@ mod tests { drop(checked); std::fs::remove_dir_all(root).unwrap(); } + + #[cfg(windows)] + fn fixture() -> std::path::PathBuf { + let root = std::fs::canonicalize(std::env::temp_dir()) + .unwrap() + .join(format!("dsh-ops-inherit-{}", uuid::Uuid::new_v4())); + std::fs::create_dir(&root).unwrap(); + root + } + + #[cfg(windows)] + #[test] + fn new_file_adopts_directory_grants() { + use windows_permissions::{ + LocalBox, SecurityDescriptor, + constants::{AceFlags, SeObjectType::SE_FILE_OBJECT, SecurityInformation as Info}, + wrappers::{GetNamedSecurityInfo, SetNamedSecurityInfo}, + }; + let root = fixture(); + let creator = windows_permissions::utilities::current_process_sid().unwrap(); + // A confined token authorizes through inherited grants, never through the creator, + // so the directory carries a mask the private staging DACL does not name. + let directory: LocalBox = + format!("D:(A;OICI;FA;;;{creator})(A;OICI;0x10001;;;S-1-5-32-545)").parse().unwrap(); + SetNamedSecurityInfo(&root, SE_FILE_OBJECT, Info::Dacl, None, None, directory.dacl(), None) + .unwrap(); + let destination = root.join("published"); + publish_new(&destination, b"payload", None, None).unwrap(); + let published: LocalBox = + GetNamedSecurityInfo(&destination, SE_FILE_OBJECT, Info::Dacl).unwrap(); + let granted = published.dacl().expect("a published file has a DACL"); + let inherited = (0..granted.len()).any(|index| { + granted.get_ace(index).is_some_and(|ace| { + ace.flags().contains(AceFlags::Inherited) + && ace.mask().bits() == 0x10001 + && ace.sid().is_some_and(|sid| { + windows_permissions::wrappers::ConvertSidToStringSid(sid) + .is_ok_and(|sid| sid == "S-1-5-32-545") + }) + }) + }); + assert!( + inherited, + "a published file must inherit the directory grants the staging DACL hid: {:?}", + published.as_sddl().map(|text| text.to_string_lossy().into_owned()) + ); + std::fs::remove_dir_all(root).unwrap(); + } + + #[cfg(windows)] + #[test] + fn a_directory_without_inheritable_grants_still_publishes_an_accessible_file() { + use windows_permissions::{ + LocalBox, SecurityDescriptor, + constants::{SeObjectType::SE_FILE_OBJECT, SecurityInformation as Info}, + wrappers::SetNamedSecurityInfo, + }; + let root = fixture(); + let creator = windows_permissions::utilities::current_process_sid().unwrap(); + // Nothing here is inheritable, so adoption has no grant to copy; the publisher must + // still be able to read back and delete what it published. + let locked: LocalBox = + format!("D:P(A;;FA;;;{creator})").parse().unwrap(); + SetNamedSecurityInfo(&root, SE_FILE_OBJECT, Info::Dacl | Info::ProtectedDacl, None, None, locked.dacl(), None) + .unwrap(); + let destination = root.join("published"); + publish_new(&destination, b"payload", None, None).unwrap(); + std::fs::File::open(&destination).unwrap(); + std::fs::remove_file(&destination).unwrap(); + std::fs::remove_dir_all(root).unwrap(); + } } diff --git a/native/windows-metadata/src/lib.rs b/native/windows-metadata/src/lib.rs index e9b4ad63..0e39e8eb 100644 --- a/native/windows-metadata/src/lib.rs +++ b/native/windows-metadata/src/lib.rs @@ -12,7 +12,7 @@ use std::{ use winapi::shared::ntdef::{OBJECT_ATTRIBUTES, UNICODE_STRING}; use windows_permissions::{ LocalBox, SecurityDescriptor, - constants::{SeObjectType::SE_FILE_OBJECT, SecurityInformation as Info}, + constants::{AceFlags, AceType, SeObjectType::SE_FILE_OBJECT, SecurityInformation as Info}, wrappers, }; use windows_sys::Win32::{ @@ -188,6 +188,50 @@ pub fn verify_cache(file: &File, write_sid: Option<&str>) -> io::Result<()> { Ok(()) } +/// Give a just-published object the grants it would have inherited from `parent`. +/// +/// Staging objects are created with a protected, creator-only DACL so partial bytes are +/// never readable by anyone else. That protection must not outlive publication: a file +/// left with only the creator's ACE ignores every other grant the directory carries, +/// including the confined token that owns the workspace, so the file cannot even be +/// unlinked by the sandbox that wrote it. Only object-inherit ACEs reach a non-container +/// child; the creator keeps full control so publication never narrows its own access. +pub fn adopt_parent_dacl(parent: &File, child: &File) -> io::Result<()> { + let parent_sd = descriptor(parent)?; + let Some(granted) = parent_sd.dacl() else { + // A null directory DACL grants everyone; the private DACL is the safe reading. + return Ok(()); + }; + let creator = windows_permissions::utilities::current_process_sid()?; + let mut aces = format!("(A;;FA;;;{creator})"); + for index in 0..granted.len() { + let Some(ace) = granted.get_ace(index) else { continue }; + if !ace.flags().contains(AceFlags::ObjectInherit) { continue; } + let kind = match ace.ace_type() { + AceType::ACCESS_ALLOWED_ACE_TYPE => 'A', + AceType::ACCESS_DENIED_ACE_TYPE => 'D', + // Object and callback ACEs carry GUID scoping this rebuild would drop. + _ => continue, + }; + let Some(sid) = ace.sid() else { continue }; + let sid = wrappers::ConvertSidToStringSid(sid)?; + aces.push_str(&format!("({kind};;0x{:x};;;{})", ace.mask().bits(), sid.to_string_lossy())); + } + let mut adopted: LocalBox = format!("D:{aces}").parse()?; + // SAFETY: adopted is a live OS-owned descriptor, child is a borrowed live handle. + let result = unsafe { + SetKernelObjectSecurity( + child.as_raw_handle(), + Info::Dacl.bits(), + &mut *adopted as *mut _ as *mut _, + ) + }; + if result == 0 { + return Err(io::Error::last_os_error()); + } + Ok(()) +} + /// Captures mandatory security and supported basic metadata. Named streams, /// extended attributes, EFS/compression/sparse/other special storage fail closed. pub struct Metadata {