From 868498ee49669b2fdd033b9a00789f7463a00ee0 Mon Sep 17 00:00:00 2001 From: Prashant Khodade Date: Thu, 17 Sep 2026 08:17:03 +0200 Subject: [PATCH 1/3] fix(core): enforce owner-only Windows ACLs on sensitive files and dirs set_dir_owner_only/set_file_owner_only were unconditional no-ops on Windows, so the CLI's mTLS client private key, OIDC/edge tokens, cached SSH keys, and the gateway's key-encryption key relied entirely on inherited NTFS ACLs with no OpenShell-applied restriction. Apply an owner-only DACL via SetEntriesInAclW/SetNamedSecurityInfoW with PROTECTED_DACL_SECURITY_INFORMATION to strip inherited ACEs, matching the 0700/0600 guarantee already provided on Unix. is_file_permissions_too_open now also works on Windows instead of being Unix-only, closing the detection gap alongside the prevention gap. Signed-off-by: Prashant Khodade (cherry picked from commit 71560e947f85819efbcddf70ddda94befab62b0b) --- Cargo.lock | 1 + Cargo.toml | 2 +- crates/openshell-core/Cargo.toml | 3 + crates/openshell-core/src/paths.rs | 318 +++++++++++++++++- .../openshell-driver-db-credstore/src/lib.rs | 13 + .../src/persistence/sqlite.rs | 3 +- examples/governance-interceptor/Cargo.lock | 105 ++++++ .../Cargo.lock | 105 ++++++ 8 files changed, 533 insertions(+), 17 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 431b79335a..4532d9a9a3 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4060,6 +4060,7 @@ dependencies = [ "tonic-types", "tracing", "url", + "windows", ] [[package]] diff --git a/Cargo.toml b/Cargo.toml index e916b4c52a..a7e0d012a0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -57,7 +57,7 @@ miette = { version = "7", features = ["fancy"] } thiserror = "2" # Windows platform APIs (ETW/TDH audit consumer in openshell-driver-mxc; Windows-only) -windows = { version = "0.62", features = ["Wdk_System_Threading", "Win32_Foundation", "Win32_System_Diagnostics_Etw", "Win32_System_Time"] } +windows = { version = "0.62", features = ["Wdk_System_Threading", "Win32_Foundation", "Win32_Security", "Win32_Security_Authorization", "Win32_Storage_FileSystem", "Win32_System_Diagnostics_Etw", "Win32_System_Memory", "Win32_System_SystemServices", "Win32_System_Threading", "Win32_System_Time"] } anyhow = "1" # Logging/Tracing diff --git a/crates/openshell-core/Cargo.toml b/crates/openshell-core/Cargo.toml index 601232def8..ddf1750044 100644 --- a/crates/openshell-core/Cargo.toml +++ b/crates/openshell-core/Cargo.toml @@ -41,6 +41,9 @@ tempfile = { version = "3", optional = true } nix = { workspace = true } rustix = { workspace = true } +[target.'cfg(windows)'.dependencies] +windows = { workspace = true } + [features] default = ["telemetry"] ## Compile in anonymous telemetry emission support. On by default; disable with diff --git a/crates/openshell-core/src/paths.rs b/crates/openshell-core/src/paths.rs index 2501958270..d9da5d237e 100644 --- a/crates/openshell-core/src/paths.rs +++ b/crates/openshell-core/src/paths.rs @@ -74,8 +74,8 @@ pub fn xdg_data_dir() -> Result { Ok(PathBuf::from(home).join(".local").join("share")) } -/// Create a directory (and parents) with owner-only permissions (`0o700`) on -/// Unix. On non-Unix platforms, falls back to default permissions. +/// Create a directory (and parents) with owner-only permissions (`0o700` on +/// Unix; an owner-only DACL with inheritance disabled on Windows). /// /// This should be used for any directory that contains sensitive material /// (tokens, private keys, certificates). @@ -87,9 +87,9 @@ pub fn create_dir_restricted(path: &Path) -> Result<()> { Ok(()) } -/// Set a directory to owner-only access (`0o700`) on Unix. -/// -/// No-op on non-Unix platforms. +/// Restrict a directory to owner-only access: `0o700` on Unix, or an +/// owner-only DACL (with inherited ACEs stripped, and the ACE propagated to +/// children) on Windows. pub fn set_dir_owner_only(path: &Path) -> Result<()> { #[cfg(unix)] { @@ -98,14 +98,13 @@ pub fn set_dir_owner_only(path: &Path) -> Result<()> { .into_diagnostic() .wrap_err_with(|| format!("failed to set permissions on {}", path.display()))?; } - #[cfg(not(unix))] - let _ = path; + #[cfg(windows)] + windows_acl::restrict_to_current_user(path, true)?; Ok(()) } -/// Set a file to owner-only read/write (`0o600`) on Unix. -/// -/// No-op on non-Unix platforms. +/// Restrict a file to owner-only read/write: `0o600` on Unix, or an +/// owner-only DACL (with inherited ACEs stripped) on Windows. pub fn set_file_owner_only(path: &Path) -> Result<()> { #[cfg(unix)] { @@ -114,8 +113,8 @@ pub fn set_file_owner_only(path: &Path) -> Result<()> { .into_diagnostic() .wrap_err_with(|| format!("failed to set permissions on {}", path.display()))?; } - #[cfg(not(unix))] - let _ = path; + #[cfg(windows)] + windows_acl::restrict_to_current_user(path, false)?; Ok(()) } @@ -130,16 +129,250 @@ pub fn ensure_parent_dir_restricted(path: &Path) -> Result<()> { Ok(()) } -/// Check whether a file has permissions that are too open (group/other readable). +/// Check whether a file has permissions that are too open. /// -/// Returns `true` if the file has group or other read/write/execute bits set. -/// Always returns `false` on non-Unix platforms. +/// On Unix, returns `true` if the file has group or other read/write/execute +/// bits set. On Windows, returns `true` if the file's DACL grants access to +/// any trustee other than the current user. Returns `false` if the file's +/// permissions/ACL cannot be read. #[cfg(unix)] pub fn is_file_permissions_too_open(path: &Path) -> bool { use std::os::unix::fs::PermissionsExt; std::fs::metadata(path).is_ok_and(|m| m.permissions().mode() & 0o077 != 0) } +/// Check whether a file has permissions that are too open. +/// +/// See the Unix doc comment above for the cross-platform contract. +#[cfg(windows)] +pub fn is_file_permissions_too_open(path: &Path) -> bool { + windows_acl::has_foreign_trustee(path).unwrap_or(false) +} + +/// Windows ACL/DACL implementation of the owner-only permission helpers +/// above. Confined to this submodule so the `unsafe` FFI surface stays out +/// of the rest of the crate, matching the precedent set by +/// `openshell-driver-mxc`'s ETW consumer. +#[cfg(windows)] +mod windows_acl { + #![allow(unsafe_code)] + + use miette::{IntoDiagnostic, Result, WrapErr}; + use std::path::Path; + use windows::Win32::Foundation::{CloseHandle, HANDLE, HLOCAL, LocalFree}; + use windows::Win32::Security::Authorization::{ + EXPLICIT_ACCESS_W, GetNamedSecurityInfoW, SE_FILE_OBJECT, SET_ACCESS, SetEntriesInAclW, + SetNamedSecurityInfoW, TRUSTEE_IS_SID, TRUSTEE_IS_USER, TRUSTEE_W, + }; + use windows::Win32::Security::{ + ACCESS_ALLOWED_ACE, ACE_HEADER, ACL, ACL_SIZE_INFORMATION, AclSizeInformation, + DACL_SECURITY_INFORMATION, EqualSid, GetAce, GetAclInformation, GetTokenInformation, + IsValidAcl, NO_INHERITANCE, PROTECTED_DACL_SECURITY_INFORMATION, PSECURITY_DESCRIPTOR, + PSID, SUB_CONTAINERS_AND_OBJECTS_INHERIT, TOKEN_QUERY, TOKEN_USER, TokenUser, + }; + use windows::Win32::Storage::FileSystem::FILE_ALL_ACCESS; + use windows::Win32::System::SystemServices::ACCESS_ALLOWED_ACE_TYPE; + use windows::Win32::System::Threading::{GetCurrentProcess, OpenProcessToken}; + use windows::core::{HSTRING, PWSTR}; + + /// An owned, `TOKEN_USER`-aligned buffer (backed by `Vec` purely for + /// its alignment guarantee; the contents are opaque bytes filled in by + /// `GetTokenInformation`). + struct TokenUserBuf(Vec); + + /// Fetch the current process's user token info as an aligned buffer. + /// + /// Callers get the `PSID` out via [`sid_from_token_info`], which borrows + /// from the returned buffer; keep it alive for as long as the `PSID` is + /// used. + fn current_user_token_info() -> Result { + unsafe { + let mut token = HANDLE::default(); + OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &raw mut token) + .into_diagnostic() + .wrap_err("failed to open process token")?; + let _guard = HandleGuard(token); + + let mut needed = 0u32; + // First call is expected to fail with ERROR_INSUFFICIENT_BUFFER; + // we only want the required buffer size out of it. + let _ = GetTokenInformation(token, TokenUser, None, 0, &raw mut needed); + if needed == 0 { + return Err(miette::miette!("GetTokenInformation returned no size")); + } + let words = (needed as usize).div_ceil(size_of::()); + let mut buf = TokenUserBuf(vec![0u64; words]); + GetTokenInformation( + token, + TokenUser, + Some(buf.0.as_mut_ptr().cast()), + needed, + &raw mut needed, + ) + .into_diagnostic() + .wrap_err("failed to read current process token user")?; + Ok(buf) + } + } + + /// Extract the `PSID` from a `TOKEN_USER` buffer produced by + /// [`current_user_token_info`]. The `PSID` borrows from `buf`. + fn sid_from_token_info(buf: &TokenUserBuf) -> PSID { + // SAFETY: `buf` was sized and filled by `GetTokenInformation` for + // `TokenUser` in `current_user_token_info`, and `Vec`'s 8-byte + // alignment satisfies `TOKEN_USER`'s alignment requirement. + let token_user = unsafe { &*buf.0.as_ptr().cast::() }; + token_user.User.Sid + } + + struct HandleGuard(HANDLE); + impl Drop for HandleGuard { + fn drop(&mut self) { + // SAFETY: `self.0` is a valid handle owned by this guard. + let _ = unsafe { CloseHandle(self.0) }; + } + } + + struct LocalFreeGuard(*mut core::ffi::c_void); + impl Drop for LocalFreeGuard { + fn drop(&mut self) { + if !self.0.is_null() { + // SAFETY: `self.0` was allocated by a Win32 API documented to + // return LocalAlloc-owned memory (e.g. `SetEntriesInAclW`). + let _ = unsafe { LocalFree(Some(HLOCAL(self.0))) }; + } + } + } + + /// Overwrite `path`'s DACL with a single, non-inherited ACE granting full + /// control to the current user, stripping any inherited ACEs. `is_dir` + /// controls whether the ACE propagates to children (directories only). + pub(super) fn restrict_to_current_user(path: &Path, is_dir: bool) -> Result<()> { + let token_info = current_user_token_info()?; + let sid = sid_from_token_info(&token_info); + + let trustee = TRUSTEE_W { + TrusteeForm: TRUSTEE_IS_SID, + TrusteeType: TRUSTEE_IS_USER, + ptstrName: PWSTR(sid.0.cast()), + ..Default::default() + }; + + let entry = EXPLICIT_ACCESS_W { + grfAccessPermissions: FILE_ALL_ACCESS.0, + grfAccessMode: SET_ACCESS, + grfInheritance: if is_dir { + SUB_CONTAINERS_AND_OBJECTS_INHERIT + } else { + NO_INHERITANCE + }, + Trustee: trustee, + }; + + let mut new_acl: *mut ACL = core::ptr::null_mut(); + // SAFETY: `entry` is a valid, fully-initialized EXPLICIT_ACCESS_W + // whose Trustee SID borrows from `token_info`, kept alive for this + // call. `new_acl` receives a LocalAlloc-owned pointer on success. + unsafe { SetEntriesInAclW(Some(&[entry]), None, &raw mut new_acl) } + .ok() + .into_diagnostic() + .wrap_err_with(|| format!("failed to build ACL for {}", path.display()))?; + let _acl_guard = LocalFreeGuard(new_acl.cast()); + + let path_hstring = HSTRING::from(path.as_os_str()); + // SAFETY: `path_hstring` is a valid, NUL-terminated wide string for + // the lifetime of this call; `new_acl` is a valid ACL just built + // above. `PROTECTED_DACL_SECURITY_INFORMATION` is the flag that + // strips inherited ACEs, which is the entire point of this call. + unsafe { + SetNamedSecurityInfoW( + PWSTR::from_raw(path_hstring.as_ptr().cast_mut()), + SE_FILE_OBJECT, + DACL_SECURITY_INFORMATION | PROTECTED_DACL_SECURITY_INFORMATION, + None, + None, + Some(new_acl), + None, + ) + } + .ok() + .into_diagnostic() + .wrap_err_with(|| format!("failed to set owner-only ACL on {}", path.display()))?; + + Ok(()) + } + + /// Returns `true` if `path`'s DACL grants access to any trustee other + /// than the current user, or `None` if the ACL could not be read. + pub(super) fn has_foreign_trustee(path: &Path) -> Option { + let token_info = current_user_token_info().ok()?; + let owner_sid = sid_from_token_info(&token_info); + + let path_hstring = HSTRING::from(path.as_os_str()); + let mut dacl: *mut ACL = core::ptr::null_mut(); + let mut security_descriptor = PSECURITY_DESCRIPTOR::default(); + // SAFETY: `path_hstring` is valid for the call; the out-params are + // simple pointers filled in by the API on success. The security + // descriptor `dacl` points into is LocalAlloc-owned and freed below. + let status = unsafe { + GetNamedSecurityInfoW( + PWSTR::from_raw(path_hstring.as_ptr().cast_mut()), + SE_FILE_OBJECT, + DACL_SECURITY_INFORMATION, + None, + None, + Some(&raw mut dacl), + None, + &raw mut security_descriptor, + ) + }; + status.ok().ok()?; + let _sd_guard = LocalFreeGuard(security_descriptor.0); + + if dacl.is_null() || unsafe { !IsValidAcl(dacl).as_bool() } { + return Some(false); + } + + let mut size_info = ACL_SIZE_INFORMATION::default(); + // SAFETY: `dacl` was just validated above. + unsafe { + GetAclInformation( + dacl, + (&raw mut size_info).cast(), + u32::try_from(size_of::()) + .expect("ACL_SIZE_INFORMATION size fits in u32"), + AclSizeInformation, + ) + } + .ok()?; + + for index in 0..size_info.AceCount { + let mut ace_ptr: *mut core::ffi::c_void = core::ptr::null_mut(); + // SAFETY: `dacl` is valid and `index` is within `AceCount`. + if unsafe { GetAce(dacl, index, &raw mut ace_ptr) }.is_err() { + continue; + } + // SAFETY: `GetAce` returned a pointer to a valid ACE header. + let header = unsafe { &*ace_ptr.cast::() }; + if u32::from(header.AceType) != ACCESS_ALLOWED_ACE_TYPE { + // Deny/other ACE types don't grant access; skip them for + // this "is anyone but me granted access" check. + continue; + } + // SAFETY: header.AceType confirms this is an ACCESS_ALLOWED_ACE. + let ace = unsafe { &*ace_ptr.cast::() }; + let ace_sid = PSID((&raw const ace.SidStart).cast_mut().cast()); + // SAFETY: both SIDs come from Windows APIs (`GetTokenInformation` + // and `GetAce`) and are valid for the duration of this call. + let is_owner = unsafe { EqualSid(owner_sid, ace_sid) }.is_ok(); + if !is_owner { + return Some(true); + } + } + Some(false) + } +} + /// Normalize a filesystem path by collapsing redundant separators /// and removing trailing slashes, without requiring the path to exist on disk. /// @@ -252,6 +485,61 @@ mod tests { assert!(!is_file_permissions_too_open(&file)); } + #[cfg(windows)] + #[test] + fn create_dir_restricted_sets_owner_only_acl() { + let tmp = tempfile::tempdir().unwrap(); + let dir = tmp.path().join("restricted"); + create_dir_restricted(&dir).unwrap(); + assert!( + !is_file_permissions_too_open(&dir), + "expected owner-only ACL on {}", + dir.display() + ); + } + + #[cfg(windows)] + #[test] + fn set_file_owner_only_sets_owner_only_acl() { + let tmp = tempfile::tempdir().unwrap(); + let file = tmp.path().join("secret"); + std::fs::write(&file, "secret-data").unwrap(); + set_file_owner_only(&file).unwrap(); + assert!( + !is_file_permissions_too_open(&file), + "expected owner-only ACL on {}", + file.display() + ); + } + + #[cfg(windows)] + #[test] + fn is_file_permissions_too_open_detects_world_readable() { + let tmp = tempfile::tempdir().unwrap(); + let file = tmp.path().join("open-file"); + std::fs::write(&file, "data").unwrap(); + // Grant Everyone read access, mirroring the Unix 0o644 case: an ACE + // for a trustee other than the current user. + let status = std::process::Command::new("icacls") + .arg(&file) + .arg("/grant") + .arg("Everyone:(R)") + .status() + .unwrap(); + assert!(status.success(), "icacls grant failed"); + assert!(is_file_permissions_too_open(&file)); + } + + #[cfg(windows)] + #[test] + fn is_file_permissions_too_open_accepts_restricted() { + let tmp = tempfile::tempdir().unwrap(); + let file = tmp.path().join("restricted-file"); + std::fs::write(&file, "data").unwrap(); + set_file_owner_only(&file).unwrap(); + assert!(!is_file_permissions_too_open(&file)); + } + #[test] fn normalize_path_collapses_separators() { assert_eq!(normalize_path("/usr//lib"), "/usr/lib"); diff --git a/crates/openshell-driver-db-credstore/src/lib.rs b/crates/openshell-driver-db-credstore/src/lib.rs index c9813870b2..c5c29494b8 100644 --- a/crates/openshell-driver-db-credstore/src/lib.rs +++ b/crates/openshell-driver-db-credstore/src/lib.rs @@ -1261,4 +1261,17 @@ mod tests { & 0o777; assert_eq!(key_encryption_key_mode, 0o600); } + + #[cfg(windows)] + #[test] + fn generated_key_encryption_key_file_is_owner_only() { + let tmp = tempfile::tempdir().unwrap(); + let key_encryption_key_path = tmp.path().join(DEFAULT_KEY_ENCRYPTION_KEY_FILE); + let _crypto = crypto_for_key_encryption_key_path(&key_encryption_key_path); + assert!( + !openshell_core::paths::is_file_permissions_too_open(&key_encryption_key_path), + "expected owner-only ACL on {}", + key_encryption_key_path.display() + ); + } } diff --git a/crates/openshell-server/src/persistence/sqlite.rs b/crates/openshell-server/src/persistence/sqlite.rs index 4c274386ca..93f0e6a318 100644 --- a/crates/openshell-server/src/persistence/sqlite.rs +++ b/crates/openshell-server/src/persistence/sqlite.rs @@ -1652,7 +1652,8 @@ WHERE "object_type" = ?1 AND "scope" = ?2 /// and `-shm` (shared memory index). They mirror the same sensitive data /// as the main file, so they get the same `0o600` treatment whenever they exist on disk. /// -/// Delegates to `set_file_owner_only`, which is a no-op on non-Unix platforms. +/// Delegates to `set_file_owner_only`, which restricts to an owner-only ACL +/// on Windows and `0o600` on Unix. pub(super) fn restrict_db_file_permissions(path: &Path) -> PersistenceResult<()> { set_file_owner_only(path).map_err(|err| PersistenceError::Database(err.to_string()))?; diff --git a/examples/governance-interceptor/Cargo.lock b/examples/governance-interceptor/Cargo.lock index 958e37d814..bcf2e7baa6 100644 --- a/examples/governance-interceptor/Cargo.lock +++ b/examples/governance-interceptor/Cargo.lock @@ -1085,6 +1085,7 @@ dependencies = [ "tonic-types", "tracing", "url", + "windows", ] [[package]] @@ -2365,12 +2366,107 @@ dependencies = [ "windows-sys 0.61.2", ] +[[package]] +name = "windows" +version = "0.62.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "527fadee13e0c05939a6a05d5bd6eec6cd2e3dbd648b9f8e447c6518133d8580" +dependencies = [ + "windows-collections", + "windows-core", + "windows-future", + "windows-numerics", +] + +[[package]] +name = "windows-collections" +version = "0.3.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "23b2d95af1a8a14a3c7367e1ed4fc9c20e0a26e79551b1454d72583c97cc6610" +dependencies = [ + "windows-core", +] + +[[package]] +name = "windows-core" +version = "0.62.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b8e83a14d34d0623b51dce9581199302a221863196a1dde71a7663a4c2be9deb" +dependencies = [ + "windows-implement", + "windows-interface", + "windows-link", + "windows-result", + "windows-strings", +] + +[[package]] +name = "windows-future" +version = "0.3.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e1d6f90251fe18a279739e78025bd6ddc52a7e22f921070ccdc67dde84c605cb" +dependencies = [ + "windows-core", + "windows-link", + "windows-threading", +] + +[[package]] +name = "windows-implement" +version = "0.60.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "053e2e040ab57b9dc951b72c264860db7eb3b0200ba345b4e4c3b14f67855ddf" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + +[[package]] +name = "windows-interface" +version = "0.59.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3f316c4a2570ba26bbec722032c4099d8c8bc095efccdc15688708623367e358" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "windows-link" version = "0.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f0805222e57f7521d6a62e36fa9163bc891acd422f971defe97d64e70d0a4fe5" +[[package]] +name = "windows-numerics" +version = "0.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6e2e40844ac143cdb44aead537bbf727de9b044e107a0f1220392177d15b0f26" +dependencies = [ + "windows-core", + "windows-link", +] + +[[package]] +name = "windows-result" +version = "0.4.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7781fa89eaf60850ac3d2da7af8e5242a5ea78d1a11c49bf2910bb5a73853eb5" +dependencies = [ + "windows-link", +] + +[[package]] +name = "windows-strings" +version = "0.5.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7837d08f69c77cf6b07689544538e017c1bfcf57e34b4c0ff58e6c2cd3b37091" +dependencies = [ + "windows-link", +] + [[package]] name = "windows-sys" version = "0.52.0" @@ -2405,6 +2501,15 @@ dependencies = [ "windows_x86_64_msvc", ] +[[package]] +name = "windows-threading" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3949bd5b99cafdf1c7ca86b43ca564028dfe27d66958f2470940f73d86d75b37" +dependencies = [ + "windows-link", +] + [[package]] name = "windows_aarch64_gnullvm" version = "0.52.6" diff --git a/examples/supervisor-middleware-content-guard/Cargo.lock b/examples/supervisor-middleware-content-guard/Cargo.lock index f65d9a8318..8fb3844bd5 100644 --- a/examples/supervisor-middleware-content-guard/Cargo.lock +++ b/examples/supervisor-middleware-content-guard/Cargo.lock @@ -972,6 +972,7 @@ dependencies = [ "tonic-types", "tracing", "url", + "windows", ] [[package]] @@ -1948,12 +1949,107 @@ version = "0.11.1+wasi-snapshot-preview1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ccf3ec651a847eb01de73ccad15eb7d99f80485de043efb2f370cd654f4ea44b" +[[package]] +name = "windows" +version = "0.62.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "527fadee13e0c05939a6a05d5bd6eec6cd2e3dbd648b9f8e447c6518133d8580" +dependencies = [ + "windows-collections", + "windows-core", + "windows-future", + "windows-numerics", +] + +[[package]] +name = "windows-collections" +version = "0.3.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "23b2d95af1a8a14a3c7367e1ed4fc9c20e0a26e79551b1454d72583c97cc6610" +dependencies = [ + "windows-core", +] + +[[package]] +name = "windows-core" +version = "0.62.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b8e83a14d34d0623b51dce9581199302a221863196a1dde71a7663a4c2be9deb" +dependencies = [ + "windows-implement", + "windows-interface", + "windows-link", + "windows-result", + "windows-strings", +] + +[[package]] +name = "windows-future" +version = "0.3.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e1d6f90251fe18a279739e78025bd6ddc52a7e22f921070ccdc67dde84c605cb" +dependencies = [ + "windows-core", + "windows-link", + "windows-threading", +] + +[[package]] +name = "windows-implement" +version = "0.60.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "053e2e040ab57b9dc951b72c264860db7eb3b0200ba345b4e4c3b14f67855ddf" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + +[[package]] +name = "windows-interface" +version = "0.59.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3f316c4a2570ba26bbec722032c4099d8c8bc095efccdc15688708623367e358" +dependencies = [ + "proc-macro2", + "quote", + "syn", +] + [[package]] name = "windows-link" version = "0.2.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "f0805222e57f7521d6a62e36fa9163bc891acd422f971defe97d64e70d0a4fe5" +[[package]] +name = "windows-numerics" +version = "0.3.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6e2e40844ac143cdb44aead537bbf727de9b044e107a0f1220392177d15b0f26" +dependencies = [ + "windows-core", + "windows-link", +] + +[[package]] +name = "windows-result" +version = "0.4.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7781fa89eaf60850ac3d2da7af8e5242a5ea78d1a11c49bf2910bb5a73853eb5" +dependencies = [ + "windows-link", +] + +[[package]] +name = "windows-strings" +version = "0.5.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7837d08f69c77cf6b07689544538e017c1bfcf57e34b4c0ff58e6c2cd3b37091" +dependencies = [ + "windows-link", +] + [[package]] name = "windows-sys" version = "0.52.0" @@ -1988,6 +2084,15 @@ dependencies = [ "windows_x86_64_msvc", ] +[[package]] +name = "windows-threading" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3949bd5b99cafdf1c7ca86b43ca564028dfe27d66958f2470940f73d86d75b37" +dependencies = [ + "windows-link", +] + [[package]] name = "windows_aarch64_gnullvm" version = "0.52.6" From a7060e39a6358618c3a3aef1a4c02a1f8ccbbf3f Mon Sep 17 00:00:00 2001 From: Prashant Khodade Date: Thu, 17 Sep 2026 09:14:52 +0200 Subject: [PATCH 2/3] fix(core): treat a NULL DACL as too open in is_file_permissions_too_open has_foreign_trustee conflated a NULL DACL with an unreadable/invalid ACL and returned Some(false) (not too open) for both. Per the Win32 contract, a NULL DACL means the object grants full access to everyone -- the most permissive state possible -- so it must be flagged as too open. Split the null and invalid-ACL branches: null now returns Some(true), invalid ACL keeps the existing unreadable-ACL fallback (None, which the caller maps to false via unwrap_or). Adds a regression test that constructs a real NULL DACL via a SetNamedSecurityInfoW helper confined to the windows_acl module, consistent with the existing unsafe-FFI confinement in that module. Found by CodeRabbit review on MR !113. Signed-off-by: Prashant Khodade (cherry picked from commit 46e635a4ef1d6937cdb088f46aa85baee3d6ad28) --- crates/openshell-core/src/paths.rs | 57 ++++++++++++++++++++++++++++-- 1 file changed, 55 insertions(+), 2 deletions(-) diff --git a/crates/openshell-core/src/paths.rs b/crates/openshell-core/src/paths.rs index d9da5d237e..e83bffbe91 100644 --- a/crates/openshell-core/src/paths.rs +++ b/crates/openshell-core/src/paths.rs @@ -302,6 +302,32 @@ mod windows_acl { Ok(()) } + /// Test-only: explicitly set a NULL DACL on `path`, the Win32 API's own + /// documented "grant everyone full access" state. Used to regression-test + /// that [`has_foreign_trustee`] treats a NULL DACL as too open rather + /// than conflating it with an unreadable/invalid ACL. + #[cfg(test)] + pub(super) fn set_null_dacl_for_test(path: &Path) -> Result<()> { + let path_hstring = HSTRING::from(path.as_os_str()); + // SAFETY: `path_hstring` is valid for the duration of this call; + // passing `None` for pdacl with `DACL_SECURITY_INFORMATION` set + // explicitly requests a NULL DACL, per the documented Win32 contract. + unsafe { + SetNamedSecurityInfoW( + PWSTR::from_raw(path_hstring.as_ptr().cast_mut()), + SE_FILE_OBJECT, + DACL_SECURITY_INFORMATION, + None, + None, + None, + None, + ) + } + .ok() + .into_diagnostic() + .wrap_err_with(|| format!("failed to set a NULL DACL on {}", path.display())) + } + /// Returns `true` if `path`'s DACL grants access to any trustee other /// than the current user, or `None` if the ACL could not be read. pub(super) fn has_foreign_trustee(path: &Path) -> Option { @@ -329,8 +355,15 @@ mod windows_acl { status.ok().ok()?; let _sd_guard = LocalFreeGuard(security_descriptor.0); - if dacl.is_null() || unsafe { !IsValidAcl(dacl).as_bool() } { - return Some(false); + // A NULL DACL is a real, distinct state from "unreadable ACL": per + // the Win32 contract, it means the object grants full access to + // everyone -- the most permissive state possible -- so it must be + // flagged as too open, not treated as safe. + if dacl.is_null() { + return Some(true); + } + if unsafe { !IsValidAcl(dacl).as_bool() } { + return None; } let mut size_info = ACL_SIZE_INFORMATION::default(); @@ -540,6 +573,26 @@ mod tests { assert!(!is_file_permissions_too_open(&file)); } + #[cfg(windows)] + #[test] + fn is_file_permissions_too_open_detects_null_dacl() { + // A NULL DACL is the Win32 API's own documented "grant everyone full + // access" state -- the most permissive possible -- and is a distinct + // condition from an unreadable/invalid ACL. Regression test for a + // CodeRabbit-flagged bug where the two were conflated and a NULL + // DACL was reported as safe. + let tmp = tempfile::tempdir().unwrap(); + let file = tmp.path().join("null-dacl-file"); + std::fs::write(&file, "data").unwrap(); + + windows_acl::set_null_dacl_for_test(&file).unwrap(); + + assert!( + is_file_permissions_too_open(&file), + "a NULL DACL grants everyone full access and must be flagged as too open" + ); + } + #[test] fn normalize_path_collapses_separators() { assert_eq!(normalize_path("/usr//lib"), "/usr/lib"); From 25042e240daae06cf7a0b8d4ef9a390ce3318458 Mon Sep 17 00:00:00 2001 From: Prashant Khodade Date: Mon, 21 Sep 2026 23:34:33 -0700 Subject: [PATCH 3/3] fix(core): close three false-negative gaps in the Windows ACL audit restrict_to_current_user() updated only the DACL, leaving a foreign owner's implicit WRITE_DAC right intact -- they could later replace the DACL we just set. Query OWNER_SECURITY_INFORMATION and take ownership in the same SetNamedSecurityInfoW call; if the caller can't (a genuinely foreign-owned object), the call now fails instead of silently leaving the object insecure. is_file_permissions_too_open() mapped every Win32 inspection failure (missing READ_CONTROL, an invalid ACL, a token-query failure) to "not too open" via unwrap_or(false). Fail closed instead: an inspection failure is a security false-negative risk, not a green light. has_foreign_trustee()'s ACE loop only recognized plain ACCESS_ALLOWED_ACE_TYPE and treated every other type as non-granting. Windows also defines access-allowed object, callback, and callback-object ACE variants that can grant rights to a foreign trustee; this audit doesn't parse their wider layouts, so their mere presence is now conservatively flagged as too open instead of silently skipped. Also updates architecture/gateway.md, which still described the SQLite file-tightening behavior only in terms of Unix mode 0o600, to distinguish it from the owner-only DACL behavior on Windows. Addresses review comments on PR #3495. Signed-off-by: Prashant Khodade --- architecture/gateway.md | 12 +- crates/openshell-core/src/paths.rs | 234 +++++++++++++++++++++++++++-- 2 files changed, 225 insertions(+), 21 deletions(-) diff --git a/architecture/gateway.md b/architecture/gateway.md index bccebfd9f4..0732687b5e 100644 --- a/architecture/gateway.md +++ b/architecture/gateway.md @@ -446,11 +446,13 @@ For in-memory SQLite, the adapter retains a dedicated keepalive connection for the store lifetime. Operational connection replacement therefore preserves the shared in-memory schema and objects instead of creating an empty database. -The SQLite adapter tightens the on-disk database file to mode `0o600` on every -connect so that provider API keys, SSH session tokens, and sandbox metadata are -not readable by other local users on shared hosts. The same restriction is -reapplied to the `-wal` and `-shm` sidecars (created by SQLite's -default WAL journal mode), which mirror the same sensitive contents. +The SQLite adapter tightens the on-disk database file to owner-only access on +every connect so that provider API keys, SSH session tokens, and sandbox +metadata are not readable by other local users on shared hosts: mode `0o600` +on Unix, or a protected, owner-only DACL (with inherited ACEs stripped and +ownership taken) on Windows. The same restriction is reapplied to the +`-wal` and `-shm` sidecars (created by SQLite's default WAL journal +mode), which mirror the same sensitive contents. Persisted state includes sandboxes, providers, provider profiles, provider credential refresh state, SSH sessions, policy revisions, settings, deployment diff --git a/crates/openshell-core/src/paths.rs b/crates/openshell-core/src/paths.rs index e83bffbe91..fd5a75ecf9 100644 --- a/crates/openshell-core/src/paths.rs +++ b/crates/openshell-core/src/paths.rs @@ -132,9 +132,10 @@ pub fn ensure_parent_dir_restricted(path: &Path) -> Result<()> { /// Check whether a file has permissions that are too open. /// /// On Unix, returns `true` if the file has group or other read/write/execute -/// bits set. On Windows, returns `true` if the file's DACL grants access to -/// any trustee other than the current user. Returns `false` if the file's -/// permissions/ACL cannot be read. +/// bits set, and `false` if the file's metadata cannot be read. On Windows, +/// returns `true` if the file's DACL grants access to any trustee other than +/// the current user, and also `true` (fails closed) if the ACL cannot be +/// inspected at all -- see the Windows doc comment below. #[cfg(unix)] pub fn is_file_permissions_too_open(path: &Path) -> bool { use std::os::unix::fs::PermissionsExt; @@ -143,10 +144,14 @@ pub fn is_file_permissions_too_open(path: &Path) -> bool { /// Check whether a file has permissions that are too open. /// -/// See the Unix doc comment above for the cross-platform contract. +/// See the Unix doc comment above for the cross-platform contract. A Win32 +/// inspection failure (missing `READ_CONTROL`, an invalid ACL, a token-query +/// failure, etc.) is treated as too open rather than safe: `unwrap_or(false)` +/// would turn every such failure into a security false negative, so this +/// fails closed instead. #[cfg(windows)] pub fn is_file_permissions_too_open(path: &Path) -> bool { - windows_acl::has_foreign_trustee(path).unwrap_or(false) + windows_acl::has_foreign_trustee(path).unwrap_or(true) } /// Windows ACL/DACL implementation of the owner-only permission helpers @@ -167,11 +172,15 @@ mod windows_acl { use windows::Win32::Security::{ ACCESS_ALLOWED_ACE, ACE_HEADER, ACL, ACL_SIZE_INFORMATION, AclSizeInformation, DACL_SECURITY_INFORMATION, EqualSid, GetAce, GetAclInformation, GetTokenInformation, - IsValidAcl, NO_INHERITANCE, PROTECTED_DACL_SECURITY_INFORMATION, PSECURITY_DESCRIPTOR, - PSID, SUB_CONTAINERS_AND_OBJECTS_INHERIT, TOKEN_QUERY, TOKEN_USER, TokenUser, + IsValidAcl, NO_INHERITANCE, OWNER_SECURITY_INFORMATION, + PROTECTED_DACL_SECURITY_INFORMATION, PSECURITY_DESCRIPTOR, PSID, + SUB_CONTAINERS_AND_OBJECTS_INHERIT, TOKEN_QUERY, TOKEN_USER, TokenUser, }; use windows::Win32::Storage::FileSystem::FILE_ALL_ACCESS; - use windows::Win32::System::SystemServices::ACCESS_ALLOWED_ACE_TYPE; + use windows::Win32::System::SystemServices::{ + ACCESS_ALLOWED_ACE_TYPE, ACCESS_ALLOWED_CALLBACK_ACE_TYPE, + ACCESS_ALLOWED_CALLBACK_OBJECT_ACE_TYPE, ACCESS_ALLOWED_OBJECT_ACE_TYPE, + }; use windows::Win32::System::Threading::{GetCurrentProcess, OpenProcessToken}; use windows::core::{HSTRING, PWSTR}; @@ -282,14 +291,28 @@ mod windows_acl { let path_hstring = HSTRING::from(path.as_os_str()); // SAFETY: `path_hstring` is a valid, NUL-terminated wide string for // the lifetime of this call; `new_acl` is a valid ACL just built - // above. `PROTECTED_DACL_SECURITY_INFORMATION` is the flag that - // strips inherited ACEs, which is the entire point of this call. + // above; `sid` borrows from `token_info`, kept alive for this call. + // `PROTECTED_DACL_SECURITY_INFORMATION` is the flag that strips + // inherited ACEs, which is the entire point of this call. + // + // Setting the owner (not just the DACL) matters for a pre-existing or + // migrated sensitive path owned by another SID: a DACL-only update + // can succeed with WRITE_DAC while a foreign owner retains their + // implicit WRITE_DAC right and can later replace this DACL (see + // https://learn.microsoft.com/en-us/windows/win32/secauthz/owner-of-a-new-object). + // Setting the owner to a SID already present in the caller's own + // token needs only WRITE_OWNER on the object, not + // SeTakeOwnershipPrivilege; if the caller can't take ownership (a + // genuinely foreign-owned object), this call fails and the error + // propagates below instead of silently leaving the object insecure. unsafe { SetNamedSecurityInfoW( PWSTR::from_raw(path_hstring.as_ptr().cast_mut()), SE_FILE_OBJECT, - DACL_SECURITY_INFORMATION | PROTECTED_DACL_SECURITY_INFORMATION, - None, + OWNER_SECURITY_INFORMATION + | DACL_SECURITY_INFORMATION + | PROTECTED_DACL_SECURITY_INFORMATION, + Some(sid), None, Some(new_acl), None, @@ -297,7 +320,12 @@ mod windows_acl { } .ok() .into_diagnostic() - .wrap_err_with(|| format!("failed to set owner-only ACL on {}", path.display()))?; + .wrap_err_with(|| { + format!( + "failed to set owner-only ACL and take ownership of {}", + path.display() + ) + })?; Ok(()) } @@ -328,6 +356,75 @@ mod windows_acl { .wrap_err_with(|| format!("failed to set a NULL DACL on {}", path.display())) } + /// Test-only: set a DACL containing a single `ACCESS_ALLOWED_OBJECT_ACE` + /// (rather than the plain `ACCESS_ALLOWED_ACE` [`restrict_to_current_user`] + /// writes) granting the current user access. Used to regression-test that + /// [`has_foreign_trustee`] conservatively flags the non-basic + /// access-allow ACE layouts (object/callback/callback-object) it doesn't + /// parse, instead of silently skipping them as if they were a + /// non-granting type like deny/audit. + #[cfg(test)] + pub(super) fn set_object_ace_dacl_for_test(path: &Path) -> Result<()> { + use windows::Win32::Security::{ + ACE_FLAGS, ACL_REVISION, AddAccessAllowedObjectAce, InitializeAcl, + }; + + let token_info = current_user_token_info()?; + let sid = sid_from_token_info(&token_info); + + // Oversized fixed buffer: plenty of room for an ACL header plus one + // object ACE (which is wider than a plain ACE but still well under + // 1 KiB even with a SID). Backed by `Vec` purely for its 8-byte + // alignment guarantee, matching `TokenUserBuf` above -- `ACL` has a + // stricter alignment than a `Vec` buffer provides. + let mut acl_buf = vec![0u64; 128]; + let acl_len_bytes = size_of_val(acl_buf.as_slice()); + let acl_ptr = acl_buf.as_mut_ptr().cast::(); + let acl_len = u32::try_from(acl_len_bytes).expect("test buffer size fits in u32"); + // SAFETY: `acl_ptr` points at `acl_len` bytes of writable memory + // that outlives this call (owned by `acl_buf`, alive until this + // function returns). + unsafe { InitializeAcl(acl_ptr, acl_len, ACL_REVISION) } + .into_diagnostic() + .wrap_err("failed to initialize test ACL")?; + // SAFETY: `acl_ptr` was just initialized above and has room for one + // more ACE; `sid` borrows from `token_info`, kept alive for this + // call. Passing `None` for both GUIDs still produces an ACE typed + // `ACCESS_ALLOWED_OBJECT_ACE_TYPE` per the documented Win32 contract, + // which is exactly the non-basic layout under test. + unsafe { + AddAccessAllowedObjectAce( + acl_ptr, + ACL_REVISION, + ACE_FLAGS(0), + FILE_ALL_ACCESS.0, + None, + None, + sid, + ) + } + .into_diagnostic() + .wrap_err("failed to add object ACE to test ACL")?; + + let path_hstring = HSTRING::from(path.as_os_str()); + // SAFETY: `path_hstring` is valid for the duration of this call; + // `acl_ptr` is a valid, fully-built ACL from the calls above. + unsafe { + SetNamedSecurityInfoW( + PWSTR::from_raw(path_hstring.as_ptr().cast_mut()), + SE_FILE_OBJECT, + DACL_SECURITY_INFORMATION, + None, + None, + Some(acl_ptr), + None, + ) + } + .ok() + .into_diagnostic() + .wrap_err_with(|| format!("failed to set an object-ACE DACL on {}", path.display())) + } + /// Returns `true` if `path`'s DACL grants access to any trustee other /// than the current user, or `None` if the ACL could not be read. pub(super) fn has_foreign_trustee(path: &Path) -> Option { @@ -387,9 +484,25 @@ mod windows_acl { } // SAFETY: `GetAce` returned a pointer to a valid ACE header. let header = unsafe { &*ace_ptr.cast::() }; - if u32::from(header.AceType) != ACCESS_ALLOWED_ACE_TYPE { - // Deny/other ACE types don't grant access; skip them for - // this "is anyone but me granted access" check. + let ace_type = u32::from(header.AceType); + if ace_type == ACCESS_ALLOWED_OBJECT_ACE_TYPE + || ace_type == ACCESS_ALLOWED_CALLBACK_ACE_TYPE + || ace_type == ACCESS_ALLOWED_CALLBACK_OBJECT_ACE_TYPE + { + // Windows also defines access-allowed object, callback, and + // callback-object ACE variants (each with a different, wider + // layout than plain ACCESS_ALLOWED_ACE), any of which may + // grant rights to a foreign trustee. This audit doesn't parse + // their layouts, so treat their mere presence as too open + // rather than silently skip them -- a false positive here is + // an unnecessary re-tightening, but a false negative is a + // security hole. See the ACE type table: + // https://learn.microsoft.com/en-us/windows/win32/secauthz/ace-strings + return Some(true); + } + if ace_type != ACCESS_ALLOWED_ACE_TYPE { + // Deny/audit/alarm ACE types don't grant access; skip them + // for this "is anyone but me granted access" check. continue; } // SAFETY: header.AceType confirms this is an ACCESS_ALLOWED_ACE. @@ -404,6 +517,45 @@ mod windows_acl { } Some(false) } + + /// Test-only: returns `true` if `path`'s current owner SID equals the + /// current process's user SID. Used to regression-test that + /// [`restrict_to_current_user`] actually takes ownership of the object, + /// not just its DACL. + #[cfg(test)] + pub(super) fn owner_is_current_user_for_test(path: &Path) -> Result { + let token_info = current_user_token_info()?; + let expected_sid = sid_from_token_info(&token_info); + + let path_hstring = HSTRING::from(path.as_os_str()); + let mut owner = PSID::default(); + let mut security_descriptor = PSECURITY_DESCRIPTOR::default(); + // SAFETY: `path_hstring` is valid for the call; the out-params are + // simple pointers filled in by the API on success. The security + // descriptor `owner` points into is LocalAlloc-owned and freed below. + let status = unsafe { + GetNamedSecurityInfoW( + PWSTR::from_raw(path_hstring.as_ptr().cast_mut()), + SE_FILE_OBJECT, + OWNER_SECURITY_INFORMATION, + Some(&raw mut owner), + None, + None, + None, + &raw mut security_descriptor, + ) + }; + status + .ok() + .into_diagnostic() + .wrap_err_with(|| format!("failed to query owner of {}", path.display()))?; + let _sd_guard = LocalFreeGuard(security_descriptor.0); + + // SAFETY: both SIDs come from Windows APIs (`GetTokenInformation` and + // `GetNamedSecurityInfoW`) and are valid for the duration of this + // call. + Ok(unsafe { EqualSid(expected_sid, owner) }.is_ok()) + } } /// Normalize a filesystem path by collapsing redundant separators @@ -593,6 +745,56 @@ mod tests { ); } + #[cfg(windows)] + #[test] + fn restrict_to_current_user_also_takes_ownership() { + // A DACL-only update leaves a foreign owner's implicit WRITE_DAC + // right intact, letting them later replace the DACL we just set. + // Regression test for a review comment on the owner-only ACL PR. + let tmp = tempfile::tempdir().unwrap(); + let file = tmp.path().join("owned-file"); + std::fs::write(&file, "data").unwrap(); + + set_file_owner_only(&file).unwrap(); + + assert!( + windows_acl::owner_is_current_user_for_test(&file).unwrap(), + "restrict_to_current_user must take ownership, not just set the DACL" + ); + } + + #[cfg(windows)] + #[test] + fn is_file_permissions_too_open_fails_closed_on_inspection_error() { + // A nonexistent path can't have its ACL read, so GetNamedSecurityInfoW + // fails. `unwrap_or(false)` would turn that failure into "safe"; + // fail closed instead -- an inspection failure is a security false + // negative risk, not a green light. + let tmp = tempfile::tempdir().unwrap(); + let missing = tmp.path().join("does-not-exist"); + assert!(is_file_permissions_too_open(&missing)); + } + + #[cfg(windows)] + #[test] + fn is_file_permissions_too_open_detects_object_ace_type() { + // Windows also defines access-allowed object/callback/callback-object + // ACE types, each wider than the plain ACCESS_ALLOWED_ACE this audit + // parses. Any of them may grant rights to a foreign trustee, so their + // mere presence must be flagged conservatively rather than silently + // skipped as a non-granting (deny/audit) type would be. + let tmp = tempfile::tempdir().unwrap(); + let file = tmp.path().join("object-ace-file"); + std::fs::write(&file, "data").unwrap(); + + windows_acl::set_object_ace_dacl_for_test(&file).unwrap(); + + assert!( + is_file_permissions_too_open(&file), + "an unparsed access-allow ACE layout must be conservatively flagged as too open" + ); + } + #[test] fn normalize_path_collapses_separators() { assert_eq!(normalize_path("/usr//lib"), "/usr/lib");