From c353763e616bb188a51d0e3a6f8933a11ba86952 Mon Sep 17 00:00:00 2001 From: Vladyslav Nikonov Date: Tue, 4 Aug 2026 17:27:16 +0300 Subject: [PATCH 1/2] fix(agent): trust LOCAL SERVICE writes on broker security checks The Devolutions Agent installer creates C:\ProgramData\Devolutions\Agent with write access granted to LOCAL SERVICE. The broker's anti-tampering verification rejected such a DACL, so the policy file failed to load on stock installations. LOCAL SERVICE already holds elevated rights on the host, so trusting it does not extend the attack surface. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../src/broker/policy_security.rs | 48 +++++++++++++++---- 1 file changed, 38 insertions(+), 10 deletions(-) diff --git a/devolutions-agent/src/broker/policy_security.rs b/devolutions-agent/src/broker/policy_security.rs index 190f094fc..fa8ad3939 100644 --- a/devolutions-agent/src/broker/policy_security.rs +++ b/devolutions-agent/src/broker/policy_security.rs @@ -15,10 +15,10 @@ //! a trusted principal and that its DACL does not grant write access to any other //! principal. Callers fail closed when this check fails. //! -//! For the policy file, the trusted principals are SYSTEM and the built-in -//! Administrators group. For executables, `NT SERVICE\TrustedInstaller` is trusted as -//! well, since Windows-protected binaries -//! (`System32`, `Program Files`, `WindowsApps`) are owned by and writable by that service. +//! For the policy file, the trusted principals are SYSTEM, `LOCAL SERVICE`, and the +//! built-in Administrators group. For executables, `NT SERVICE\TrustedInstaller` is +//! trusted as well, since Windows-protected binaries (`System32`, `Program Files`, +//! `WindowsApps`) are owned by and writable by that service. //! //! For elevated executables the verification additionally defends against //! time-of-check/time-of-use races: the file is opened without write or delete sharing @@ -40,7 +40,8 @@ use windows::Win32::Foundation::{ERROR_SUCCESS, GENERIC_ALL, GENERIC_WRITE, HAND use windows::Win32::Security::Authorization::{ConvertSidToStringSidW, GetSecurityInfo, SE_FILE_OBJECT}; use windows::Win32::Security::{ ACCESS_ALLOWED_ACE, ACE_HEADER, ACL, DACL_SECURITY_INFORMATION, GetAce, INHERIT_ONLY_ACE, IsWellKnownSid, - OWNER_SECURITY_INFORMATION, PSECURITY_DESCRIPTOR, PSID, WinBuiltinAdministratorsSid, WinLocalSystemSid, + OWNER_SECURITY_INFORMATION, PSECURITY_DESCRIPTOR, PSID, WinBuiltinAdministratorsSid, WinLocalServiceSid, + WinLocalSystemSid, }; use windows::Win32::Storage::FileSystem::{ DELETE, FILE_APPEND_DATA, FILE_DELETE_CHILD, FILE_FLAG_BACKUP_SEMANTICS, FILE_FLAG_OPEN_REPARSE_POINT, @@ -94,7 +95,7 @@ const TRUSTED_INSTALLER_SID: &str = "S-1-5-80-956008885-3418522649-1831038044-18 /// Principals trusted to hold write access over a verified file. #[derive(Clone, Copy, PartialEq, Eq)] enum TrustedWriters { - /// SYSTEM and the built-in Administrators group (policy file). + /// SYSTEM, `LOCAL SERVICE`, and the built-in Administrators group (policy file). AdminOnly, /// Additionally trusts `NT SERVICE\TrustedInstaller` (Windows-protected executables). AdminOrTrustedInstaller, @@ -143,7 +144,8 @@ impl Drop for OwnedSecurityDescriptor { } } -/// Verify that the policy file may only be written by SYSTEM or built-in Administrators. +/// Verify that the policy file may only be written by SYSTEM, `LOCAL SERVICE`, or +/// built-in Administrators. /// /// The check is performed on the already-opened file handle so the verified security /// descriptor belongs to the very same file that is subsequently read (no TOCTOU window @@ -197,9 +199,9 @@ impl VerifiedExecutable { /// - The file is opened without write or delete sharing; the open fails if a writer /// already has it open, and the guard prevents modification, deletion, and renaming /// of the verified object until it is dropped. -/// - The executable's owner and DACL must only allow writes by SYSTEM, built-in -/// Administrators, or `NT SERVICE\TrustedInstaller` (see [`verify_policy_file_security`] -/// for the exact DACL rules). +/// - The executable's owner and DACL must only allow writes by SYSTEM, `LOCAL SERVICE`, +/// built-in Administrators, or `NT SERVICE\TrustedInstaller` (see +/// [`verify_policy_file_security`] for the exact DACL rules). /// - Every ancestor directory of the final path (resolved from the verified handle) must /// not allow untrusted principals to rename or delete path components, so the name used /// for execution cannot be redirected to a different file. @@ -646,6 +648,14 @@ unsafe fn is_trusted_sid(sid: PSID, trusted_writers: TrustedWriters) -> bool { return true; } + // The Devolutions Agent installer creates `C:\ProgramData\Devolutions\Agent` with + // write access for `LOCAL SERVICE`, which already holds elevated rights on the host, + // so trusting it does not extend the attack surface. + // SAFETY: Per function contract, `sid` points to a valid SID. + if unsafe { IsWellKnownSid(sid, WinLocalServiceSid) }.as_bool() { + return true; + } + // SAFETY: Per function contract, `sid` points to a valid SID. if unsafe { IsWellKnownSid(sid, WinBuiltinAdministratorsSid) }.as_bool() { return true; @@ -798,6 +808,24 @@ mod tests { sd.verify().expect("Administrators owner must be accepted"); } + #[test] + fn local_service_write_ace_is_accepted() { + // The installer creates the Agent ProgramData directory with write access for + // LOCAL SERVICE, which is already an elevated principal. + let sd = SddlDescriptor::parse("O:SYD:(A;;FA;;;SY)(A;;FA;;;BA)(A;;FA;;;LS)"); + sd.verify().expect("LOCAL SERVICE write access must be accepted"); + } + + #[test] + fn network_service_write_ace_is_rejected() { + let sd = SddlDescriptor::parse("O:SYD:(A;;FA;;;SY)(A;;FA;;;BA)(A;;FA;;;NS)"); + let error = sd.verify().unwrap_err(); + assert!( + error.to_string().contains("grants write access"), + "unexpected error: {error}" + ); + } + #[test] fn app_exec_alias_reparse_buffer_is_parsed() { // Synthetic AppExecLink buffer: version 3, then package family, entry point, From 0945aff040e1e017b9f375769c314b39ad2c357a Mon Sep 17 00:00:00 2001 From: Vladyslav Nikonov Date: Tue, 4 Aug 2026 17:35:21 +0300 Subject: [PATCH 2/2] fix(agent): restrict LOCAL SERVICE trust to policy-file verification LOCAL SERVICE is a low-privilege shared service identity, so it is only trusted for the policy file (AdminOnly mode), not for elevated executables where accepting it would open a privilege-escalation path. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../src/broker/policy_security.rs | 40 +++++++++++++------ 1 file changed, 28 insertions(+), 12 deletions(-) diff --git a/devolutions-agent/src/broker/policy_security.rs b/devolutions-agent/src/broker/policy_security.rs index fa8ad3939..4ebb290d6 100644 --- a/devolutions-agent/src/broker/policy_security.rs +++ b/devolutions-agent/src/broker/policy_security.rs @@ -16,9 +16,9 @@ //! principal. Callers fail closed when this check fails. //! //! For the policy file, the trusted principals are SYSTEM, `LOCAL SERVICE`, and the -//! built-in Administrators group. For executables, `NT SERVICE\TrustedInstaller` is -//! trusted as well, since Windows-protected binaries (`System32`, `Program Files`, -//! `WindowsApps`) are owned by and writable by that service. +//! built-in Administrators group. For executables, `LOCAL SERVICE` is not trusted, but +//! `NT SERVICE\TrustedInstaller` is, since Windows-protected binaries (`System32`, +//! `Program Files`, `WindowsApps`) are owned by and writable by that service. //! //! For elevated executables the verification additionally defends against //! time-of-check/time-of-use races: the file is opened without write or delete sharing @@ -97,7 +97,10 @@ const TRUSTED_INSTALLER_SID: &str = "S-1-5-80-956008885-3418522649-1831038044-18 enum TrustedWriters { /// SYSTEM, `LOCAL SERVICE`, and the built-in Administrators group (policy file). AdminOnly, - /// Additionally trusts `NT SERVICE\TrustedInstaller` (Windows-protected executables). + /// SYSTEM, the built-in Administrators group, and `NT SERVICE\TrustedInstaller` + /// (Windows-protected executables). `LOCAL SERVICE` is deliberately not trusted here: + /// it is a low-privilege shared service identity, and accepting it for elevated + /// executables would open a privilege-escalation path. AdminOrTrustedInstaller, } @@ -199,9 +202,9 @@ impl VerifiedExecutable { /// - The file is opened without write or delete sharing; the open fails if a writer /// already has it open, and the guard prevents modification, deletion, and renaming /// of the verified object until it is dropped. -/// - The executable's owner and DACL must only allow writes by SYSTEM, `LOCAL SERVICE`, -/// built-in Administrators, or `NT SERVICE\TrustedInstaller` (see -/// [`verify_policy_file_security`] for the exact DACL rules). +/// - The executable's owner and DACL must only allow writes by SYSTEM, built-in +/// Administrators, or `NT SERVICE\TrustedInstaller` (see [`verify_policy_file_security`] +/// for the exact DACL rules). /// - Every ancestor directory of the final path (resolved from the verified handle) must /// not allow untrusted principals to rename or delete path components, so the name used /// for execution cannot be redirected to a different file. @@ -649,10 +652,11 @@ unsafe fn is_trusted_sid(sid: PSID, trusted_writers: TrustedWriters) -> bool { } // The Devolutions Agent installer creates `C:\ProgramData\Devolutions\Agent` with - // write access for `LOCAL SERVICE`, which already holds elevated rights on the host, - // so trusting it does not extend the attack surface. + // write access for `LOCAL SERVICE`, so it must be trusted for the policy file. + // It is a low-privilege shared service identity, however, so it is not trusted for + // elevated executables, where accepting it would open a privilege-escalation path. // SAFETY: Per function contract, `sid` points to a valid SID. - if unsafe { IsWellKnownSid(sid, WinLocalServiceSid) }.as_bool() { + if trusted_writers == TrustedWriters::AdminOnly && unsafe { IsWellKnownSid(sid, WinLocalServiceSid) }.as_bool() { return true; } @@ -809,13 +813,25 @@ mod tests { } #[test] - fn local_service_write_ace_is_accepted() { + fn local_service_write_ace_is_accepted_for_policy_file() { // The installer creates the Agent ProgramData directory with write access for - // LOCAL SERVICE, which is already an elevated principal. + // LOCAL SERVICE, so the policy-file check must accept it. let sd = SddlDescriptor::parse("O:SYD:(A;;FA;;;SY)(A;;FA;;;BA)(A;;FA;;;LS)"); sd.verify().expect("LOCAL SERVICE write access must be accepted"); } + #[test] + fn local_service_write_ace_is_rejected_for_executables() { + // LOCAL SERVICE is a low-privilege shared service identity; a LOCAL + // SERVICE-writable executable must not pass elevated-executable verification. + let sd = SddlDescriptor::parse("O:SYD:(A;;FA;;;SY)(A;;FA;;;BA)(A;;FA;;;LS)"); + let error = sd.verify_as_executable().unwrap_err(); + assert!( + error.to_string().contains("grants write access"), + "unexpected error: {error}" + ); + } + #[test] fn network_service_write_ace_is_rejected() { let sd = SddlDescriptor::parse("O:SYD:(A;;FA;;;SY)(A;;FA;;;BA)(A;;FA;;;NS)");