diff --git a/crates/cageforge-windows/src/filesystem/acl.rs b/crates/cageforge-windows/src/filesystem/acl.rs index 0255f1b6..fe18eed6 100644 --- a/crates/cageforge-windows/src/filesystem/acl.rs +++ b/crates/cageforge-windows/src/filesystem/acl.rs @@ -111,11 +111,6 @@ struct AclOperation { protect_dacl: bool, } -struct SubtreePath { - path: PathBuf, - is_directory: bool, -} - enum AclOperationPath { Pinned(ValidatedPath), Discovered(PathBuf), @@ -706,8 +701,8 @@ impl<'plan> AclPlanBuilder<'plan> { for descendant in subtree_paths(&root, &exclusions)? { merge_pending( &mut self.continuation, - &descendant.path, - entries_for_existing_path(&entries, descendant.is_directory), + &descendant, + entries.clone(), true, Vec::new(), Vec::new(), @@ -733,8 +728,8 @@ impl<'plan> AclPlanBuilder<'plan> { for descendant in subtree_paths(&root, &exclusions)? { merge_pending( &mut self.continuation, - &descendant.path, - entries_for_existing_path(&entries, descendant.is_directory), + &descendant, + entries.clone(), true, Vec::new(), Vec::new(), @@ -813,10 +808,8 @@ impl AclOperation { }, }; let original = SecurityDescriptor::read(&path)?; - let entries = self - .entries - .iter() - .cloned() + let entries = entries_for_existing_path(&self.entries, path.is_directory()) + .into_iter() .map(|declaration| { let sid = LocalSid::parse("ACL entry", &declaration.sid)?; Ok(PreparedAclEntry { declaration, sid }) @@ -2656,7 +2649,7 @@ fn nearest_root_reuse(operation: &PreparedAclOperation, roots: &[(PathBuf, bool) fn subtree_paths( root: &Path, excluded_roots: &[PathBuf], -) -> Result, FilesystemAclError> { +) -> Result, FilesystemAclError> { let metadata = fs::symlink_metadata(root).map_err(|source| FilesystemAclError::Metadata { path: root.to_path_buf(), source, @@ -2715,13 +2708,10 @@ fn subtree_paths( if is_directory { stack.push(child.clone()); } - paths.push(SubtreePath { - path: child, - is_directory, - }); + paths.push(child); } } - paths.sort_by_key(|entry| NativePathKey::new(&entry.path)); + paths.sort_by_key(|entry| NativePathKey::new(entry)); Ok(paths) } @@ -3344,9 +3334,9 @@ mod tests { let paths = subtree_paths(&denied, std::slice::from_ref(&writable)) .expect("enumerate denied subtree"); - assert!(paths.iter().any(|entry| entry.path == sibling)); - assert!(!paths.iter().any(|entry| entry.path == writable)); - assert!(!paths.iter().any(|entry| entry.path == writable_child)); + assert!(paths.contains(&sibling)); + assert!(!paths.contains(&writable)); + assert!(!paths.contains(&writable_child)); } #[test] @@ -3387,8 +3377,8 @@ mod tests { let paths = subtree_paths(&root, &[]).expect("enumerate without following reparse point"); - assert!(!paths.iter().any(|entry| entry.path == junction)); - assert!(!paths.iter().any(|entry| entry.path == outside_child)); + assert!(!paths.contains(&junction)); + assert!(!paths.contains(&outside_child)); assert!(subtree_paths(&junction, &[]).is_err()); } diff --git a/crates/cageforge-windows/src/filesystem/path.rs b/crates/cageforge-windows/src/filesystem/path.rs index 35862e63..c669a4b0 100644 --- a/crates/cageforge-windows/src/filesystem/path.rs +++ b/crates/cageforge-windows/src/filesystem/path.rs @@ -15,17 +15,18 @@ use windows_sys::Win32::Foundation::{ ERROR_FILE_NOT_FOUND, ERROR_PATH_NOT_FOUND, GetLastError, INVALID_HANDLE_VALUE, }; use windows_sys::Win32::Storage::FileSystem::{ - CreateFileW, DELETE, FILE_ATTRIBUTE_REPARSE_POINT, FILE_ATTRIBUTE_TAG_INFO, - FILE_FLAG_BACKUP_SEMANTICS, FILE_FLAG_OPEN_REPARSE_POINT, FILE_GENERIC_READ, FILE_SHARE_READ, - FILE_SHARE_WRITE, FileAttributeTagInfo, GetFileInformationByHandleEx, - GetFinalPathNameByHandleW, GetLongPathNameW, OPEN_EXISTING, READ_CONTROL, VOLUME_NAME_DOS, - WRITE_DAC, + CreateFileW, DELETE, FILE_ATTRIBUTE_DIRECTORY, FILE_ATTRIBUTE_REPARSE_POINT, + FILE_ATTRIBUTE_TAG_INFO, FILE_FLAG_BACKUP_SEMANTICS, FILE_FLAG_OPEN_REPARSE_POINT, + FILE_GENERIC_READ, FILE_SHARE_READ, FILE_SHARE_WRITE, FileAttributeTagInfo, + GetFileInformationByHandleEx, GetFinalPathNameByHandleW, GetLongPathNameW, OPEN_EXISTING, + READ_CONTROL, VOLUME_NAME_DOS, WRITE_DAC, }; pub(crate) struct ValidatedPath { handle: OwnedHandle, final_path: PathBuf, identity: FilesystemObjectIdentity, + is_directory: bool, } #[derive(Debug, PartialEq, Eq)] @@ -174,6 +175,10 @@ impl ValidatedPath { &self.identity } + pub(crate) const fn is_directory(&self) -> bool { + self.is_directory + } + pub(crate) fn try_clone_file(&self) -> std::io::Result { self.handle.try_clone().map(File::from) } @@ -204,7 +209,7 @@ impl ValidatedPath { }); } let handle = unsafe { OwnedHandle::from_raw_handle(handle as RawHandle) }; - reject_reparse_point(path, handle.as_raw_handle() as _)?; + let is_directory = validate_object_kind(path, handle.as_raw_handle() as _)?; let identity = object_identity(path, handle.as_raw_handle() as _)?; let final_path = final_path(path, handle.as_raw_handle() as _)?; let expanded_path = long_path(path)?; @@ -218,6 +223,7 @@ impl ValidatedPath { handle, final_path, identity, + is_directory, }) } } @@ -295,10 +301,10 @@ fn validate_lexical_path(path: &Path) -> Result<(), ValidatedPathError> { } #[allow(unsafe_code)] -fn reject_reparse_point( +fn validate_object_kind( path: &Path, handle: windows_sys::Win32::Foundation::HANDLE, -) -> Result<(), ValidatedPathError> { +) -> Result { let mut attributes = FILE_ATTRIBUTE_TAG_INFO::default(); if unsafe { GetFileInformationByHandleEx( @@ -319,7 +325,7 @@ fn reject_reparse_point( path: path.to_path_buf(), }) } else { - Ok(()) + Ok(attributes.FileAttributes & FILE_ATTRIBUTE_DIRECTORY != 0) } } diff --git a/crates/cageforge-windows/tests/support/windows_sandbox_fixture.rs b/crates/cageforge-windows/tests/support/windows_sandbox_fixture.rs index 3490e8bf..5fd4de4f 100644 --- a/crates/cageforge-windows/tests/support/windows_sandbox_fixture.rs +++ b/crates/cageforge-windows/tests/support/windows_sandbox_fixture.rs @@ -87,6 +87,7 @@ const UNRELATED_NAMED_OBJECT: &str = "CAGEFORGE_WINDOWS_SANDBOX_FIXTURE_UNRELATE fn main() -> ExitCode { let result = match std::env::args_os().nth(1).as_deref() { + Some(argument) if argument == "--file-root-probe" => file_root_probe(), Some(argument) if argument == "--process-broker-child" => process_broker_child(), Some(argument) if argument == "--shell-activation-child" => shell_activation_child(), _ => run(), @@ -100,6 +101,46 @@ fn main() -> ExitCode { } } +fn file_root_probe() -> Result<(), String> { + let file = PathBuf::from(std::env::args_os().nth(2).ok_or("missing file path")?); + let sibling = PathBuf::from(std::env::args_os().nth(3).ok_or("missing sibling path")?); + let spaced_file = PathBuf::from( + std::env::args_os() + .nth(4) + .ok_or("missing spaced file path")?, + ); + let parent = file.parent().ok_or("missing parent")?; + for path in [&file, &spaced_file] { + let data = + std::fs::read(path).map_err(|error| format!("read approved file {path:?}: {error}"))?; + if data != b"cageforge-file-root\r\n" { + return Err(format!("unexpected approved file contents: {path:?}")); + } + let metadata = std::fs::metadata(path) + .map_err(|error| format!("read approved file metadata {path:?}: {error}"))?; + if !metadata.is_file() || metadata.len() != data.len() as u64 { + return Err(format!("unexpected approved file metadata: {path:?}")); + } + } + let write = std::fs::OpenOptions::new().write(true).open(&file); + let sibling_read = std::fs::read(&sibling); + let listing = std::fs::read_dir(parent); + for (operation, error) in [ + ("file write", write.err()), + ("sibling read", sibling_read.err()), + ("parent listing", listing.err()), + ] { + if error.as_ref().map(std::io::Error::kind) != Some(std::io::ErrorKind::PermissionDenied) { + return Err(format!( + "{operation}: expected access denied, got {error:?}" + )); + } + } + std::io::stdout() + .write_all(b"file-root-contract-ok") + .map_err(|error| format!("write file-root result: {error}")) +} + fn run() -> Result<(), String> { let mode = environment(MODE)?.to_string_lossy().into_owned(); match mode.as_str() { diff --git a/crates/cageforge-windows/tests/windows_backend.rs b/crates/cageforge-windows/tests/windows_backend.rs index 4ed45b63..4d8a9950 100644 --- a/crates/cageforge-windows/tests/windows_backend.rs +++ b/crates/cageforge-windows/tests/windows_backend.rs @@ -1514,6 +1514,184 @@ fn restricted_command_can_start_a_native_runtime_program() { drop(cleanup); } +#[test] +fn explicit_read_file_root_supports_launch_and_exact_acl_cleanup() { + let _setup_test_guard = setup_test_lock(); + let temporary = tempfile::tempdir().expect("temporary state"); + let config = WindowsSetupConfig::new() + .with_state_directory(temporary.path().join("state")) + .expect("absolute state directory") + .with_setup_helper_path(PathBuf::from(env!("CARGO_BIN_EXE_cageforge-windows-setup"))) + .expect("setup helper") + .with_command_runner_path(PathBuf::from(env!( + "CARGO_BIN_EXE_cageforge-windows-command-runner" + ))) + .expect("command runner"); + let setup = WindowsSetup::new(config); + let mut cleanup = SetupCleanup { + setup: &setup, + armed: true, + }; + setup.install().expect("install file-root setup"); + let backend = WindowsBackend::new( + WindowsBackendConfig::new() + .with_setup(setup.config().clone()) + .with_default_timeout(END_TO_END_PROBE_TIMEOUT) + .expect("bounded probe"), + ) + .expect("file-root backend"); + let workspace = tempfile::tempdir().expect("workspace"); + // Keep the file outside the writable workspace and grant only this leaf. + let resources = tempfile::tempdir().expect("external resources"); + let file = resources.path().join("single-resource.jar"); + let spaced_file = resources.path().join("single resource.jar"); + let sibling = resources.path().join("unrelated.txt"); + fs::write(&file, b"cageforge-file-root\r\n").expect("readable file"); + fs::write(&spaced_file, b"cageforge-file-root\r\n").expect("readable spaced file"); + fs::write(&sibling, b"unrelated").expect("neighboring file"); + let paths = [ + file.as_path(), + sibling.as_path(), + resources.path(), + spaced_file.as_path(), + ]; + let before = paths.map(raw_dacl_fingerprint); + for descriptor in &before { + assert!( + descriptor.starts_with("bytes="), + "DACL capture failed: {descriptor}" + ); + } + let system_root = PathBuf::from(std::env::var_os("SystemRoot").expect("SystemRoot")); + let fixture = workspace.path().join("file-root-probe.exe"); + fs::copy( + env!("CARGO_BIN_EXE_cageforge-windows-test-fixture"), + &fixture, + ) + .expect("copy file-root fixture"); + let direct_probe = || { + CommandSpec::new(&fixture) + .expect("fixture command") + .with_args([ + std::ffi::OsStr::new("--file-root-probe"), + file.as_os_str(), + sibling.as_os_str(), + spaced_file.as_os_str(), + ]) + .expect("fixture arguments") + }; + let shell = |script: String| { + CommandSpec::new(system_root.join("System32/cmd.exe")) + .expect("system cmd.exe") + .with_args(["/d", "/c", script.as_str()]) + .expect("cmd arguments") + }; + for (command, allow_parent, expected_success, expected_stdout) in [ + (direct_probe(), false, true, Some("file-root-contract-ok")), + (direct_probe(), false, true, Some("file-root-contract-ok")), + // TYPE enumerates the directory, beyond opening the approved file. + ( + shell(format!("type {}", file.display())), + false, + false, + Some(""), + ), + ( + shell(format!("echo forbidden>> {}", file.display())), + false, + false, + Some(""), + ), + // Keep the identical shell command as a positive control with an + // explicit directory grant; never infer that grant from a file rule. + ( + shell(format!("type {}", file.display())), + true, + true, + Some("cageforge-file-root\r\n"), + ), + ] { + assert_eq!( + raw_dacl_fingerprint(&sibling), + before[1], + "sibling DACL changed by file grant" + ); + assert_eq!( + raw_dacl_fingerprint(resources.path()), + before[2], + "parent DACL changed by file grant" + ); + let environment = EnvironmentSpec::inherit_core(); + let mut rules = vec![ + FilesystemRule::new(PathSelector::minimal(), AccessMode::Read), + FilesystemRule::new(PathSelector::workspace_root(), AccessMode::Write), + FilesystemRule::new( + PathSelector::absolute(&file).expect("absolute file"), + AccessMode::Read, + ), + FilesystemRule::new( + PathSelector::absolute(&spaced_file).expect("absolute spaced file"), + AccessMode::Read, + ), + ]; + if allow_parent { + rules.push(FilesystemRule::new( + PathSelector::absolute(resources.path()).expect("absolute resource directory"), + AccessMode::Read, + )); + } + let filesystem = FilesystemPolicy::restricted(rules); + let (request, effective, context) = request_with_filesystem_environment( + workspace.path(), + filesystem, + NetworkPolicy::disabled(), + command, + environment, + ); + let prepared = backend + .prepare(BackendRequest::new(&request, &effective), &context) + .expect("prepare explicit file-root policy"); + let mut child = backend + .spawn(prepared) + .expect("spawn with explicit read file root"); + let status = child.wait().expect("wait file-root command"); + let mut stdout = String::new(); + child + .stdout() + .expect("stdout") + .read_to_string(&mut stdout) + .expect("read stdout"); + let mut stderr = String::new(); + child + .stderr() + .expect("stderr") + .read_to_string(&mut stderr) + .expect("read stderr"); + assert_eq!( + status.success(), + expected_success, + "allow_parent={allow_parent}; status={status:?}; stdout={stdout:?}; stderr={stderr:?}" + ); + if let Some(expected_stdout) = expected_stdout { + assert_eq!(stdout, expected_stdout); + } + assert_eq!( + fs::read(&file).expect("read original file"), + b"cageforge-file-root\r\n" + ); + } + drop(backend); + setup + .uninstall() + .expect("restore file-root ACL and uninstall"); + cleanup.armed = false; + assert_eq!( + paths.map(raw_dacl_fingerprint), + before, + "original DACLs were not restored" + ); +} + #[test] fn runnable_windows_profile_launches_through_the_native_backend_api() { let _setup_test_guard = setup_test_lock(); diff --git a/specs/0016-windows-backend-implementation.md b/specs/0016-windows-backend-implementation.md index b4e3f57e..64cf44f6 100644 --- a/specs/0016-windows-backend-implementation.md +++ b/specs/0016-windows-backend-implementation.md @@ -672,8 +672,10 @@ disagreement before its no-write read-back is typed descriptor drift and blocks launch. Thus an idempotent launch cannot cause duplicate inherited ACEs in existing descendants. New descendants inherit from the protected root while new siblings outside it continue to inherit the deny. -The scan retains each object's directory-or-file kind: direct ACEs on existing -files are exact, while ACEs on existing directories retain their required +ACL preparation obtains each object's directory-or-file kind from the same +non-reparse handle used for descriptor inspection and mutation. This applies +to explicitly selected file roots as well as enumerated descendants. Direct +ACEs on files are exact, while ACEs on directories retain their required inheritance flags for future children. Windows correctly strips inheritance flags from a file ACE, so Cageforge never treats that canonicalization as either evidence of enforcement or a reason to weaken a directory ACE check. Unknown or @@ -684,6 +686,14 @@ descendant disappears before its ACL handle can be opened, only and skipped. Reparse substitution, final-path drift, access denial, malformed ACLs, and every other open failure remain typed fail-closed errors. +An explicit file read root grants access to that file without changing its +parent or siblings. The verified `SeChangeNotifyPrivilege` permits traversal +to an authorized file; it does not grant directory listing. Applications that +enumerate the containing directory need a separate directory read rule. +The backend must not infer that broader grant from an individual file rule. +File and directory grants use the same exact journal and uninstall restoration +contract. + All capability-state and ACL reconciliation uses the same protected cross-process lock. A later profile may preserve another profile's capability ACEs but must never revoke or rewrite them as its own. Persistent state records @@ -1171,6 +1181,17 @@ rights even if it writes visually equivalent ACEs. ### 12.3 Filesystem ACL review record +File-root inheritance was additionally compared with +`windows-sandbox-rs/src/acl.rs::ensure_allow_mask_aces_with_inheritance` at the +frozen baseline and the local upstream revision +`50d77959bf927293c4b5ddcca81d05331ae582ea`. Those implementations pass the +caller's inheritance flags to `SetEntriesInAclW` and apply the resulting ACL +through `SetNamedSecurityInfoW`. Cageforge determines the native object kind +through its retained handle before constructing the expected descriptor, so +file ACEs have exact scope in both the durable journal and native read-back. +Directory inheritance, masks, SID matching, descriptor equality, and original +DACL restoration retain their existing contracts. + Filesystem lowering and ACL reconciliation were reviewed line by line against the frozen versions of `src/acl.rs`, `src/allow.rs`, `src/workspace_acl.rs`, `src/deny_read_acl.rs`, `src/deny_read_state.rs`, `src/deny_read_resolver.rs`,