fix(core): enforce owner-only Windows ACLs on sensitive files and dirs - #3495
pkhodade-NV wants to merge 4 commits into
Conversation
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 <pkhodade@nvidia.com> (cherry picked from commit 71560e947f85819efbcddf70ddda94befab62b0b)
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 <pkhodade@nvidia.com> (cherry picked from commit 46e635a4ef1d6937cdb088f46aa85baee3d6ad28)
shailendra-nv
left a comment
shailendra-nv
left a comment
There was a problem hiding this comment.
Requesting changes because the new Windows ACL audit can report a path as owner-only when it is not. The inline comments cover three false-negative cases: a foreign object owner is neither changed nor audited, Win32 inspection errors are mapped to safe, and non-basic access-allow ACE types are skipped.
Please also update architecture/gateway.md:449, which still says SQLite files are tightened to Unix mode 0o600. The canonical architecture documentation should distinguish Unix modes from the new protected-DACL behavior on Windows.
Validation on this head was otherwise clean: the native Windows ARM64 workspace check passed; the Windows pre-commit suite passed 4,949 tests with 26 skipped; and cargo fmt --all --check plus git diff --check passed. The requested changes concern the security contract rather than build correctness.
| PWSTR::from_raw(path_hstring.as_ptr().cast_mut()), | ||
| SE_FILE_OBJECT, | ||
| DACL_SECURITY_INFORMATION | PROTECTED_DACL_SECURITY_INFORMATION, | ||
| None, |
There was a problem hiding this comment.
This updates only the DACL and leaves the object owner unchanged. For a pre-existing or migrated sensitive path owned by another SID, the call can succeed when the current process has WRITE_DAC, but the foreign owner still has implicit WRITE_DAC and can later replace this DACL. The audit below also retrieves only the DACL, so it will report that path as secure. Please request/check OWNER_SECURITY_INFORMATION and either set the owner to the current user or reject a foreign-owned object, with a regression test. Microsoft documents the implicit DACL-modification right of the owner here: https://learn.microsoft.com/en-us/windows/win32/secauthz/owner-of-a-new-object
| /// 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) |
There was a problem hiding this comment.
unwrap_or(false) converts every Win32 inspection failure into permissions are not too open. Missing READ_CONTROL, unsupported filesystem behavior, token-query failures, or an invalid ACL can therefore produce a security false negative. Please expose a Result<bool> to callers, or conservatively treat an inspection failure as too open, and add an error-path regression test.
| 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; |
There was a problem hiding this comment.
The comment is not true for all other ACE types: Windows also defines access-allowed object, callback, and callback-object ACEs, each of which may grant rights to a foreign trustee. Skipping them lets this audit return false for a non-owner-only DACL. Please parse all access-allow layouts or conservatively flag unsupported allow ACEs, with coverage for these variants. See the Microsoft ACE type table: https://learn.microsoft.com/en-us/windows/win32/secauthz/ace-strings
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 <pkhodade@nvidia.com>
…ner-only-permissions # Conflicts: # Cargo.toml
This branch has not been deployed
2 participants
Summary
set_dir_owner_only/set_file_owner_onlyinopenshell-core/src/paths.rswere unconditional no-ops on Windows (documented as such), so locally-stored CLI credential material (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.is_file_permissions_too_open(the one available detection mechanism) was also Unix-only, so there was no way to audit for this after the fact either.Related Issue
No linked issue — this is a security-sensitive, localized fix to platform-specific permission-hardening helpers already documented as required (the doc comments on these functions state the intent plainly; only the Windows implementation was missing).
Changes
paths.rs:set_dir_owner_only/set_file_owner_onlynow apply a real owner-only Windows ACL viaSetEntriesInAclW/SetNamedSecurityInfoWwithPROTECTED_DACL_SECURITY_INFORMATION, which strips inherited ACEs from the parent directory — the actual source of the unintended access. Directories additionally propagate the ACE to children (SUB_CONTAINERS_AND_OBJECTS_INHERIT).is_file_permissions_too_openis no longer Unix-only; on Windows it inspects the DACL viaGetNamedSecurityInfoWand flags any ACE granted to a trustee other than the current user.windowscrate (already a workspace dependency, used by the MXC driver's ETW consumer) as a Windows-only dependency ofopenshell-core, with the additionalWin32_Security*/Win32_Storage_FileSystem/Win32_System_Memory/Win32_System_SystemServices/Win32_System_Threadingfeatures needed for the ACL APIs. TheunsafeFFI surface is confined to a privatewindows_aclsubmodule, matching the existing precedent inopenshell-driver-mxc.openshell-driver-db-credstore's key-encryption-key generation (previously#[cfg(unix)]-only, so the gateway's master key-encryption key had no Windows test coverage at all).openshell-server/src/persistence/sqlite.rs.Cargo.lockfiles (governance-interceptor,supervisor-middleware-content-guard) since both path-depend onopenshell-core; diffs are purely additive (newwindows-*crate entries only, no other package bumped).No caller changes were needed in
openshell-bootstrap/mtls.rs,edge_token.rs,oidc_token.rs, oropenshell-cli/ssh.rs— they already route through the fixedpaths.rsfunctions.Testing
paths.rsmirroring the four existing Unix tests.icacls: a restricted file gets a single owner-only ACE (user:(F)), a restricted directory getsuser:(OI)(CI)(F)— no leftover SYSTEM/Administrators/Users entries from inheritance.cargo test -p openshell-driver-db-credstore --target x86_64-pc-windows-msvc generated_key_encryption_key_file_is_owner_onlypasses on Windows.Checklist
Originally opened as GitLab MR !113 against our internal mirror; re-opened here against
windowsfor upstream review.