From 02f65bd835702aab6c4aaf8c248411827fe311c1 Mon Sep 17 00:00:00 2001 From: cryaciccl Date: Tue, 27 Oct 2026 22:57:48 +0800 Subject: [PATCH] fix(file): inherit the directory DACL when publishing a new file Staging objects are created with a protected, creator-only DACL so partial bytes are never readable. The create path never restored inheritance after publication, so a new file carried exactly one explicit ACE: a confined workspace token authorizes through inherited capability grants and therefore could not unlink a file it had just written, and every other grant the directory carries was lost. 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. The creator keeps full control, so publication never narrows the publisher's own access. Overwrites are unchanged: they still capture and restore the target's own descriptor. Refs #420 --- docs/CHANGELOG.md | 13 +++ native/Cargo.lock | 1 + native/crates/ops-files/Cargo.toml | 3 + .../crates/ops-files/src/file_publication.rs | 2 +- native/crates/ops-files/src/file_single.rs | 86 +++++++++++++++++++ native/windows-metadata/src/lib.rs | 46 +++++++++- 6 files changed, 149 insertions(+), 2 deletions(-) diff --git a/docs/CHANGELOG.md b/docs/CHANGELOG.md index 77b9854d..30069aa3 100644 --- a/docs/CHANGELOG.md +++ b/docs/CHANGELOG.md @@ -5,6 +5,19 @@ All notable changes to this package are recorded here. The format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this package adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### 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. + ## [0.5.0] — 2026-10-27 — Preset-mode component roster ### Breaking changes 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 {