test: cover the owner, locality, replace-acl, and sweep-race branches of the windows hardening

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"
+    );
+}

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<bool> = 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<R>(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()
+    }
+}

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");
+}

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

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<SidBuffer> {
+    #[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<SidBuffer> {
     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<SidBuffer> {
     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<bool> = 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<SidBuffer> {
+        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<R>(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()
+    }
+}

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))
 }

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");
+}

crates/windows/src/system/private_file/replace.rs

@@ -88,7 +88,10 @@ pub(super) fn acquire_write_lease(parent: &Path) -> std::io::Result<TempDirLease
                     liveness_handle: Some(handle),
                 });
             }
-            Err(error) if error.kind() == ErrorKind::NotFound => 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<TempDirLease
     ))
 }
 
+/// A concurrent same-parent writer racing the sweep can leave a just-created
+/// lease directory swept, delete-pending, or held by the sweep's probe handle,
+/// so the liveness open surfaces `NotFound`, `ERROR_ACCESS_DENIED` (5), or
+/// `ERROR_SHARING_VIOLATION` (32). Those are transient collisions retried on a
+/// fresh nonce. Keying the OS-error cases on `raw_os_error` keeps the
+/// owner/locality/reparse refusals — `PermissionDenied` with no OS code —
+/// fatal, never retried.
+fn lease_open_collision_is_retryable(error: &std::io::Error) -> 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<File> {
     let handle = OpenOptions::new()
         .read(true)

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"
+    );
+}