From d24a86bb8ef15110b2ddff4c4fc88b95e8da01aa Mon Sep 17 00:00:00 2001 From: Lahfir <70215676+lahfir@users.noreply.github.com> Date: Mon, 27 Jul 2026 05:28:06 -0600 Subject: [PATCH] test: cover the owner, locality, replace-acl, and sweep-race branches of the windows hardening --- crates/core/src/private_file_ops_tests.rs | 25 ++++ .../src/system/private_file/locality.rs | 38 ++++- .../src/system/private_file/locality_tests.rs | 35 ++++- crates/windows/src/system/private_file/mod.rs | 22 +-- .../windows/src/system/private_file/owner.rs | 63 ++++++++- .../src/system/private_file/owner_tests.rs | 132 ++++++++++++++++-- .../src/system/private_file/path_tests.rs | 27 ++++ .../src/system/private_file/replace.rs | 22 ++- .../src/system/private_file/replace_tests.rs | 47 +++++++ 9 files changed, 384 insertions(+), 27 deletions(-) diff --git a/crates/core/src/private_file_ops_tests.rs b/crates/core/src/private_file_ops_tests.rs index 6ac15a6..59f575e 100644 --- a/crates/core/src/private_file_ops_tests.rs +++ b/crates/core/src/private_file_ops_tests.rs @@ -163,3 +163,28 @@ fn installing_ops_a_second_time_is_rejected() { assert!(install_private_file_ops(Box::new(PortablePrivateFileOps)).is_ok()); assert!(install_private_file_ops(Box::new(PortablePrivateFileOps)).is_err()); } + +#[test] +fn temporary_file_name_has_the_hidden_hex_nonce_shape_and_varies_between_calls() { + let first = temporary_file_name(OsStr::new("refmap.json")); + let first = first + .to_str() + .expect("the temporary name must be valid UTF-8"); + + let nonce = first + .strip_prefix(".refmap.json.") + .and_then(|rest| rest.strip_suffix(".tmp")) + .expect("the name must lead with a dot and the destination name and end with .tmp"); + assert_eq!(nonce.len(), 16, "the nonce must be 16 hex digits: {nonce}"); + assert!( + nonce.chars().all(|digit| digit.is_ascii_hexdigit()), + "the nonce must be hexadecimal: {nonce}" + ); + + let second = temporary_file_name(OsStr::new("refmap.json")); + assert_ne!( + OsString::from(first), + second, + "two successive calls must produce different nonces" + ); +} diff --git a/crates/windows/src/system/private_file/locality.rs b/crates/windows/src/system/private_file/locality.rs index 6046b53..59eb88e 100644 --- a/crates/windows/src/system/private_file/locality.rs +++ b/crates/windows/src/system/private_file/locality.rs @@ -68,10 +68,18 @@ pub(super) fn require_local_for_private_write(file: &File, what: &str) -> std::i pub(super) fn assess_file_locality(file: &File) -> SurfaceLocality { let control_succeeded = basic_info_control_succeeds(control_probe_handle(file)); - let remote_probe = remote_protocol_probe(file.as_raw_handle()); + let remote_probe = remote_protocol_probe_result(file); classify_surface_locality(control_succeeded, remote_probe) } +fn remote_protocol_probe_result(file: &File) -> Result<(), u32> { + #[cfg(test)] + if forced_remote_locality::is_active() { + return Ok(()); + } + remote_protocol_probe(file.as_raw_handle()) +} + pub(super) fn classify_surface_locality( control_succeeded: bool, remote_probe: Result<(), u32>, @@ -148,3 +156,31 @@ pub(super) mod forced_control_failure { run() } } + +/// Forces the remote-protocol probe to report success while the class-0 +/// control call still runs for real, so the classifier yields `Remote` and +/// the remote-storage refusal branch can be exercised on ordinary local disk. +#[cfg(test)] +pub(super) mod forced_remote_locality { + use std::cell::Cell; + + thread_local! { + static FORCE_REMOTE_LOCALITY: Cell = const { Cell::new(false) }; + } + + pub(in super::super) fn is_active() -> bool { + FORCE_REMOTE_LOCALITY.with(Cell::get) + } + + pub(in super::super) fn with_forced_remote_locality(run: impl FnOnce() -> R) -> R { + struct ResetOnDrop; + impl Drop for ResetOnDrop { + fn drop(&mut self) { + FORCE_REMOTE_LOCALITY.with(|flag| flag.set(false)); + } + } + FORCE_REMOTE_LOCALITY.with(|flag| flag.set(true)); + let _reset = ResetOnDrop; + run() + } +} diff --git a/crates/windows/src/system/private_file/locality_tests.rs b/crates/windows/src/system/private_file/locality_tests.rs index 1e65762..f103ad6 100644 --- a/crates/windows/src/system/private_file/locality_tests.rs +++ b/crates/windows/src/system/private_file/locality_tests.rs @@ -3,7 +3,8 @@ use crate::system::private_file::WindowsPrivateFile; use crate::system::private_file::locality::{ AlignedRemoteProtocolInfo, FILE_REMOTE_PROTOCOL_INFO_SIZE, SurfaceLocality, assess_file_locality, basic_info_control_succeeds, classify_surface_locality, - forced_control_failure::with_forced_control_failure, remote_protocol_probe, + forced_control_failure::with_forced_control_failure, + forced_remote_locality::with_forced_remote_locality, remote_protocol_probe, }; use agent_desktop_core::PrivateFileOps; use std::io::ErrorKind; @@ -110,3 +111,35 @@ fn a_forced_control_failure_yields_unknown_and_the_private_write_is_refused() { ) .expect("writes must work again once the control call is restored"); } + +#[test] +fn a_forced_remote_locality_refuses_the_private_write_while_the_control_call_succeeds() { + let scratch = Scratch::new("locality-remote"); + let probe_path = scratch.path().join("probe.bin"); + std::fs::write(&probe_path, b"probe").unwrap(); + let probe_file = std::fs::File::open(&probe_path).unwrap(); + let refused_artifact = scratch.path().join("gated-parent").join("artifact.json"); + + with_forced_remote_locality(|| { + assert_eq!(assess_file_locality(&probe_file), SurfaceLocality::Remote); + let refused = WindowsPrivateFile::new() + .write_atomic(&refused_artifact, b"secret") + .unwrap_err(); + assert_eq!(refused.kind(), ErrorKind::PermissionDenied); + assert!( + refused.to_string().contains("remote storage"), + "the refusal must name the remote storage: {refused}" + ); + }); + + assert!( + !refused_artifact.exists(), + "no artifact may exist after the refused write" + ); + WindowsPrivateFile::new() + .write_atomic( + &scratch.path().join("ungated-parent").join("artifact.json"), + b"local again", + ) + .expect("writes must work again once the locality probe is restored"); +} diff --git a/crates/windows/src/system/private_file/mod.rs b/crates/windows/src/system/private_file/mod.rs index 213279d..c0514ab 100644 --- a/crates/windows/src/system/private_file/mod.rs +++ b/crates/windows/src/system/private_file/mod.rs @@ -9,14 +9,20 @@ //! nonce temp naming, and the bounded-read limits — and adds only the //! hardening the measured evidence justifies. //! -//! Deliberately absent: descriptor authoring and DACL validation. A plain -//! leaf under the user profile already inherits `NT AUTHORITY\SYSTEM`, -//! `BUILTIN\Administrators`, and the user — with no `BUILTIN\Users` — so -//! authoring would re-state what Windows grants and validating would -//! re-check it with exactly the ACE-parsing code whose `AceSize` handling -//! sank the deleted v0.5.0 layer. Nothing here calls the ACL/ACE family, a -//! test pins that absence, and a structural test pins the inherited-ACL -//! assumption itself so an OS change breaks a test rather than the product. +//! Deliberately absent: descriptor authoring and DACL validation. A freshly +//! created leaf under the user profile inherits its parent's ACL — +//! `NT AUTHORITY\SYSTEM`, `BUILTIN\Administrators`, and the user, with no +//! `BUILTIN\Users` — while a `ReplaceFileW` rewrite instead keeps the DACL of +//! the leaf it replaces: for our own artifacts the same SYSTEM/Administrators/ +//! user grants with no `BUILTIN\Users`, though re-materialized as explicit +//! ACEs rather than inherited ones. Either way the security-relevant grants +//! carry across, so authoring would re-state what Windows grants and +//! validating would re-check it with exactly the ACE-parsing code whose +//! `AceSize` handling sank the deleted v0.5.0 layer. Nothing here calls the +//! ACL/ACE family, a test pins that absence, and a structural test pins the +//! inherited grants on the freshly created leaf and the same principals' +//! survival across the replace path so an OS change breaks a test rather than +//! the product. //! //! Locality gates only the write surfaces (atomic writes, appends, lock //! opens); reads stay ungated so observation commands keep working wherever diff --git a/crates/windows/src/system/private_file/owner.rs b/crates/windows/src/system/private_file/owner.rs index 3b0823a..ca83824 100644 --- a/crates/windows/src/system/private_file/owner.rs +++ b/crates/windows/src/system/private_file/owner.rs @@ -78,7 +78,7 @@ impl SidBuffer { pub(super) fn require_owned_by_token_owner(file: &File, what: &str) -> std::io::Result<()> { let expected = process_token_owner_sid()?; - let actual = file_owner_sid(file)?; + let actual = actual_owner_for_comparison(file)?; if !expected.matches(&actual) { return Err(permission_denied(format!( "{what} is owned by a foreign principal, not this process's token owner" @@ -87,6 +87,14 @@ pub(super) fn require_owned_by_token_owner(file: &File, what: &str) -> std::io:: Ok(()) } +fn actual_owner_for_comparison(file: &File) -> std::io::Result { + #[cfg(test)] + if forced_foreign_owner::is_active() { + return forced_foreign_owner::foreign_sid(); + } + file_owner_sid(file) +} + pub(super) fn file_owner_sid(file: &File) -> std::io::Result { let mut owner: PSID = std::ptr::null_mut(); let mut descriptor: *mut core::ffi::c_void = std::ptr::null_mut(); @@ -179,3 +187,56 @@ pub(super) fn process_token_user_sid_for_tests() -> std::io::Result { let user: TOKEN_USER = unsafe { std::ptr::read(buffer.as_ptr().cast()) }; SidBuffer::copied_from_valid(user.User.Sid) } + +/// Forces the owner comparison to observe a foreign principal so the +/// refusal branch of `require_owned_by_token_owner` can be exercised without +/// a file actually pre-created by another account. The substituted owner is +/// the `WinLocalSystemSid`, built programmatically so the seam stays portable +/// and privilege-free. +#[cfg(test)] +pub(super) mod forced_foreign_owner { + use std::cell::Cell; + + use windows_sys::Win32::Security::{ + CreateWellKnownSid, SECURITY_MAX_SID_SIZE, WinLocalSystemSid, + }; + + use super::SidBuffer; + + thread_local! { + static FORCE_FOREIGN_OWNER: Cell = const { Cell::new(false) }; + } + + pub(in super::super) fn is_active() -> bool { + FORCE_FOREIGN_OWNER.with(Cell::get) + } + + pub(in super::super) fn foreign_sid() -> std::io::Result { + let mut storage = [0_u64; 9]; + let mut size: u32 = SECURITY_MAX_SID_SIZE; + let created = unsafe { + CreateWellKnownSid( + WinLocalSystemSid, + std::ptr::null_mut(), + storage.as_mut_ptr().cast(), + &mut size, + ) + }; + if created == 0 { + return Err(std::io::Error::last_os_error()); + } + SidBuffer::copied_from_valid(storage.as_mut_ptr().cast()) + } + + pub(in super::super) fn with_forced_foreign_owner(run: impl FnOnce() -> R) -> R { + struct ResetOnDrop; + impl Drop for ResetOnDrop { + fn drop(&mut self) { + FORCE_FOREIGN_OWNER.with(|flag| flag.set(false)); + } + } + FORCE_FOREIGN_OWNER.with(|flag| flag.set(true)); + let _reset = ResetOnDrop; + run() + } +} diff --git a/crates/windows/src/system/private_file/owner_tests.rs b/crates/windows/src/system/private_file/owner_tests.rs index 7b812b3..a805654 100644 --- a/crates/windows/src/system/private_file/owner_tests.rs +++ b/crates/windows/src/system/private_file/owner_tests.rs @@ -1,10 +1,12 @@ use super::{Scratch, scratch_nonce}; use crate::system::private_file::WindowsPrivateFile; +use crate::system::private_file::owner::forced_foreign_owner::with_forced_foreign_owner; use crate::system::private_file::owner::{ SidBuffer, file_owner_sid, process_token_owner_sid, process_token_user_sid_for_tests, require_owned_by_token_owner, }; use agent_desktop_core::PrivateFileOps; +use std::io::ErrorKind; use std::path::Path; use windows_sys::Win32::Foundation::LocalFree; use windows_sys::Win32::Security::Authorization::ConvertSidToStringSidW; @@ -30,7 +32,87 @@ fn a_freshly_created_files_owner_equals_the_process_token_owner() { } #[test] -fn a_fresh_profile_artifact_inherits_system_admins_and_user_classes_and_no_users_class() { +fn the_token_owner_does_not_match_a_foreign_well_known_principal() { + let token_owner = process_token_owner_sid().unwrap(); + let foreign = well_known_sid_buffer(WinLocalSystemSid); + + assert!( + !token_owner.matches(&foreign), + "the LocalSystem principal must be foreign to this test process's token owner" + ); +} + +#[test] +fn require_owned_by_token_owner_refuses_a_foreign_owner_via_the_forced_seam() { + let scratch = Scratch::new("owner-foreign"); + let path = scratch.path().join("fresh.txt"); + let file = std::fs::File::create(&path).unwrap(); + + let refused = with_forced_foreign_owner(|| { + require_owned_by_token_owner(&file, "seam target").unwrap_err() + }); + + assert_eq!(refused.kind(), ErrorKind::PermissionDenied); + assert!( + refused.to_string().contains("foreign principal"), + "the refusal must name the foreign principal: {refused}" + ); +} + +#[test] +fn write_atomic_refuses_when_the_owner_seam_forces_a_foreign_principal() { + let scratch = Scratch::new("owner-foreign-write"); + let artifact = scratch.path().join("artifact.json"); + let ops = WindowsPrivateFile::new(); + + let refused = with_forced_foreign_owner(|| ops.write_atomic(&artifact, b"secret").unwrap_err()); + + assert_eq!(refused.kind(), ErrorKind::PermissionDenied); + assert!( + refused.to_string().contains("foreign principal"), + "the refused write must name the foreign principal: {refused}" + ); + assert!( + !artifact.exists(), + "no artifact may land when ownership validation refuses the write" + ); +} + +#[test] +fn read_private_bounded_refuses_when_the_owner_seam_forces_a_foreign_principal() { + let scratch = Scratch::new("owner-foreign-read"); + let artifact = scratch.path().join("artifact.json"); + let ops = WindowsPrivateFile::new(); + ops.write_atomic(&artifact, b"payload").unwrap(); + + let refused = + with_forced_foreign_owner(|| ops.read_private_bounded(&artifact, 64).unwrap_err()); + + assert_eq!(refused.kind(), ErrorKind::PermissionDenied); + assert!( + refused.to_string().contains("foreign principal"), + "the refused read must name the foreign principal: {refused}" + ); +} + +#[test] +fn copied_from_valid_rejects_a_null_and_an_oversized_sid_with_invalid_data() { + let null_kind = SidBuffer::copied_from_valid(std::ptr::null_mut()) + .err() + .map(|error| error.kind()); + assert_eq!(null_kind, Some(ErrorKind::InvalidData)); + + let mut oversized = [0_u8; 16]; + oversized[0] = 1; + oversized[1] = 200; + let oversized_kind = SidBuffer::copied_from_valid(oversized.as_mut_ptr().cast()) + .err() + .map(|error| error.kind()); + assert_eq!(oversized_kind, Some(ErrorKind::InvalidData)); +} + +#[test] +fn a_profile_artifact_keeps_system_admins_and_user_and_never_users_across_create_and_replace() { let local_app_data = std::env::var_os("LOCALAPPDATA").expect("LOCALAPPDATA must exist on Windows"); let fresh = Scratch::adopt(Path::new(&local_app_data).join("Temp").join(format!( @@ -39,18 +121,35 @@ fn a_fresh_profile_artifact_inherits_system_admins_and_user_classes_and_no_users scratch_nonce() ))); let artifact = fresh.path().join("artifact.json"); - WindowsPrivateFile::new() - .write_atomic(&artifact, b"{}") - .expect("the pinned write must succeed under the user profile"); + let ops = WindowsPrivateFile::new(); - let entries = inherited_ace_entries(&artifact); + ops.write_atomic(&artifact, b"{}") + .expect("the pinned create-path write must succeed under the user profile"); + let created = inherited_ace_entries(&artifact); + assert_every_ace_is_inherited(&created); + assert_profile_security_principals(&created); + + assert!( + artifact.exists(), + "the create-path write must leave a destination for the replace path to overwrite" + ); + ops.write_atomic(&artifact, b"{\"v\":2}") + .expect("the pinned replace-path write must succeed over the pre-existing leaf"); + assert_profile_security_principals(&inherited_ace_entries(&artifact)); +} + +fn assert_every_ace_is_inherited(entries: &[(String, bool)]) { assert!(!entries.is_empty(), "the artifact must report ACL entries"); - for (sid, inherited) in &entries { + for (sid, inherited) in entries { assert!( *inherited, - "every entry on a plain profile leaf must be inherited; {sid} is explicit" + "every entry on a freshly created profile leaf must be inherited; {sid} is explicit" ); } +} + +fn assert_profile_security_principals(entries: &[(String, bool)]) { + assert!(!entries.is_empty(), "the artifact must report ACL entries"); let sids: Vec<&str> = entries.iter().map(|(sid, _)| sid.as_str()).collect(); let system_class = well_known_sid_string(WinLocalSystemSid); let administrators_class = well_known_sid_string(WinBuiltinAdministratorsSid); @@ -59,19 +158,19 @@ fn a_fresh_profile_artifact_inherits_system_admins_and_user_classes_and_no_users let token_owner = sid_string(process_token_owner_sid().unwrap()); assert!( sids.contains(&system_class.as_str()), - "a SYSTEM-class principal must be inherited" + "a SYSTEM-class principal must be present" ); assert!( sids.contains(&administrators_class.as_str()), - "an Administrators-class principal must be inherited" + "an Administrators-class principal must be present" ); assert!( sids.contains(&token_user.as_str()) || sids.contains(&token_owner.as_str()), - "the current-user principal must be inherited" + "the current-user principal must be present" ); assert!( !sids.contains(&users_class.as_str()), - "no Users-class principal may appear on a plain profile leaf" + "no Users-class principal may appear on a profile leaf" ); } @@ -119,7 +218,7 @@ fn sid_string(sid: &SidBuffer) -> String { value } -fn well_known_sid_string(kind: WELL_KNOWN_SID_TYPE) -> String { +fn well_known_sid_buffer(kind: WELL_KNOWN_SID_TYPE) -> SidBuffer { let mut storage = [0_u64; 9]; let mut size: u32 = SECURITY_MAX_SID_SIZE; let created = unsafe { @@ -134,7 +233,10 @@ fn well_known_sid_string(kind: WELL_KNOWN_SID_TYPE) -> String { created != 0, "CreateWellKnownSid must succeed for kind {kind}" ); - let buffer = SidBuffer::copied_from_valid(storage.as_mut_ptr().cast()) - .expect("a well-known SID must be valid"); - sid_string(&buffer) + SidBuffer::copied_from_valid(storage.as_mut_ptr().cast()) + .expect("a well-known SID must be valid") +} + +fn well_known_sid_string(kind: WELL_KNOWN_SID_TYPE) -> String { + sid_string(&well_known_sid_buffer(kind)) } diff --git a/crates/windows/src/system/private_file/path_tests.rs b/crates/windows/src/system/private_file/path_tests.rs index 9360b16..ee04c8a 100644 --- a/crates/windows/src/system/private_file/path_tests.rs +++ b/crates/windows/src/system/private_file/path_tests.rs @@ -2,6 +2,7 @@ use super::{Scratch, create_junction}; use crate::system::private_file::WindowsPrivateFile; use agent_desktop_core::PrivateFileOps; use std::io::{ErrorKind, Read, Write}; +use std::path::Path; #[test] fn a_junction_component_on_the_write_path_is_refused_and_nothing_lands_at_its_target() { @@ -189,3 +190,29 @@ fn parent_traversal_components_are_rejected() { let refused = outcome.unwrap_err(); assert_eq!(refused.kind(), ErrorKind::InvalidData); } + +struct RestoreCurrentDirOnDrop(std::path::PathBuf); + +impl Drop for RestoreCurrentDirOnDrop { + fn drop(&mut self) { + let _ = std::env::set_current_dir(&self.0); + } +} + +#[test] +fn a_leading_current_dir_component_is_skipped_and_the_write_lands_in_the_working_directory() { + let root = Scratch::new("curdir"); + let original_working_dir = + std::env::current_dir().expect("the current working directory must be readable"); + let _restore = RestoreCurrentDirOnDrop(original_working_dir); + std::env::set_current_dir(root.path()) + .expect("the scratch root must be enterable as the working directory"); + + let relative = Path::new(".").join("sub").join("artifact.json"); + let ops = WindowsPrivateFile::new(); + ops.write_atomic(&relative, b"payload") + .expect("a leading current-dir component must be skipped, not rejected"); + + let landed = root.path().join("sub").join("artifact.json"); + assert_eq!(ops.read_private_bounded(&landed, 64).unwrap(), b"payload"); +} diff --git a/crates/windows/src/system/private_file/replace.rs b/crates/windows/src/system/private_file/replace.rs index e588e47..aa1ab5f 100644 --- a/crates/windows/src/system/private_file/replace.rs +++ b/crates/windows/src/system/private_file/replace.rs @@ -88,7 +88,10 @@ pub(super) fn acquire_write_lease(parent: &Path) -> std::io::Result continue, + Err(error) if lease_open_collision_is_retryable(&error) => { + let _ = std::fs::remove_dir_all(&directory); + continue; + } Err(error) => { let _ = std::fs::remove_dir_all(&directory); return Err(error); @@ -101,6 +104,23 @@ pub(super) fn acquire_write_lease(parent: &Path) -> std::io::Result bool { + if error.kind() == ErrorKind::NotFound { + return true; + } + match error.raw_os_error() { + Some(code) => code == ERROR_SHARING_VIOLATION as i32 || code == ERROR_ACCESS_DENIED as i32, + None => false, + } +} + fn open_verified_liveness_handle(directory: &Path) -> std::io::Result { let handle = OpenOptions::new() .read(true) diff --git a/crates/windows/src/system/private_file/replace_tests.rs b/crates/windows/src/system/private_file/replace_tests.rs index 7772dfe..7214ca5 100644 --- a/crates/windows/src/system/private_file/replace_tests.rs +++ b/crates/windows/src/system/private_file/replace_tests.rs @@ -279,3 +279,50 @@ fn write_atomic_leaves_no_temp_lease_residue_on_success_or_failure() { ); assert_eq!(std::fs::read(&destination).unwrap(), b"first"); } + +#[test] +fn two_concurrent_writers_to_the_same_parent_both_succeed_with_their_final_content() { + let scratch = Scratch::new("concurrent-writers"); + let iterations = 64_usize; + + let parent_a = scratch.path().to_path_buf(); + let writer_a = std::thread::spawn(move || { + let ops = WindowsPrivateFile::new(); + for iteration in 0..iterations { + let content = format!("a-{iteration}"); + ops.write_atomic(&parent_a.join("a.json"), content.as_bytes()) + .unwrap_or_else(|error| { + panic!("writer a iteration {iteration} must succeed: {error}") + }); + } + }); + + let parent_b = scratch.path().to_path_buf(); + let writer_b = std::thread::spawn(move || { + let ops = WindowsPrivateFile::new(); + for iteration in 0..iterations { + let content = format!("b-{iteration}"); + ops.write_atomic(&parent_b.join("b.json"), content.as_bytes()) + .unwrap_or_else(|error| { + panic!("writer b iteration {iteration} must succeed: {error}") + }); + } + }); + + writer_a.join().expect("writer a must not panic"); + writer_b.join().expect("writer b must not panic"); + + let ops = WindowsPrivateFile::new(); + assert_eq!( + ops.read_private_bounded(&scratch.path().join("a.json"), 64) + .unwrap(), + format!("a-{}", iterations - 1).into_bytes(), + "writer a's final content must survive the concurrent writes" + ); + assert_eq!( + ops.read_private_bounded(&scratch.path().join("b.json"), 64) + .unwrap(), + format!("b-{}", iterations - 1).into_bytes(), + "writer b's final content must survive the concurrent writes" + ); +}