From d95640c597cb90849803dd65afda19e2d7a39444 Mon Sep 17 00:00:00 2001 From: Oliver Kieselbach Date: Tue, 13 Oct 2026 19:26:44 +0200 Subject: [PATCH] Agent: protected DACL on the agent data directory (D-355) %ProgramData%\AutopilotMonitor inherited ProgramData's "Users may create files and folders" entry, so a standard user could add files to the folder the SYSTEM agent and its boot task start from. The agent outlives the device phase of OOBE (user phase, desktop, reboots until the enrollment ends), and the payload catalog is checked at install time only. - AgentDirectoryAcl.EnsureProtected: protected DACL, SYSTEM and Administrators full control, Users read and execute, inherited by every subfolder; idempotent, never blocks the agent. - Install mode applies it after the agent directories exist; every runtime start re-checks it, which also reaches agents that arrived by self-update. - Tests: protected shape, apply once then leave alone, existing subfolders take it over, a widened or unprotected DACL is not taken as protected, a missing folder is not created. Co-Authored-By: Claude Opus 5.5 --- internal | 2 +- .../Security/AgentDirectoryAclTests.cs | 113 +++++++++++++++++ .../Security/AgentDirectoryAcl.cs | 115 ++++++++++++++++++ .../Program.InstallMode.cs | 22 ++++ .../AutopilotMonitor.Agent.V2/Program.cs | 5 + 5 files changed, 256 insertions(+), 1 deletion(-) create mode 100644 src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Security/AgentDirectoryAclTests.cs create mode 100644 src/Agent/AutopilotMonitor.Agent.V2.Core/Security/AgentDirectoryAcl.cs diff --git a/internal b/internal index e2b86174..c9a13d6b 160000 --- a/internal +++ b/internal @@ -1 +1 @@ -Subproject commit e2b8617406915b06dc9b9f3c488f7d77725944a2 +Subproject commit c9a13d6b4741fb12002cc2e60a50a023e844f042 diff --git a/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Security/AgentDirectoryAclTests.cs b/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Security/AgentDirectoryAclTests.cs new file mode 100644 index 00000000..3dd03ae6 --- /dev/null +++ b/src/Agent/AutopilotMonitor.Agent.V2.Core.Tests/Security/AgentDirectoryAclTests.cs @@ -0,0 +1,113 @@ +using System; +using System.IO; +using System.Linq; +using System.Security.AccessControl; +using System.Security.Principal; +using AutopilotMonitor.Agent.V2.Core.Security; +using Xunit; + +namespace AutopilotMonitor.Agent.V2.Core.Tests.Security +{ + /// + /// The agent data directory keeps standard users from adding files where the SYSTEM agent + /// starts from. Works on a temp folder the test owns; as owner it can always hand itself the + /// rights back to clean up. + /// + public sealed class AgentDirectoryAclTests : IDisposable + { + private const FileSystemRights WriteRights = + FileSystemRights.WriteData | FileSystemRights.AppendData | FileSystemRights.WriteAttributes + | FileSystemRights.WriteExtendedAttributes | FileSystemRights.Delete | FileSystemRights.DeleteSubdirectoriesAndFiles + | FileSystemRights.ChangePermissions | FileSystemRights.TakeOwnership; + + private static readonly SecurityIdentifier Users = new SecurityIdentifier(WellKnownSidType.BuiltinUsersSid, null); + + private readonly string _root = Path.Combine(Path.GetTempPath(), "apm-acl-" + Guid.NewGuid().ToString("N")); + + public AgentDirectoryAclTests() => Directory.CreateDirectory(_root); + + public void Dispose() + { + try + { + var me = WindowsIdentity.GetCurrent().User!; + var directory = new DirectoryInfo(_root); + var security = directory.GetAccessControl(); + security.AddAccessRule(new FileSystemAccessRule(me, FileSystemRights.FullControl, + InheritanceFlags.ContainerInherit | InheritanceFlags.ObjectInherit, PropagationFlags.None, AccessControlType.Allow)); + directory.SetAccessControl(security); + Directory.Delete(_root, recursive: true); + } + catch + { + // best effort: a leftover temp folder must not fail the run + } + } + + private static FileSystemRights UsersRights(string path) => + new DirectoryInfo(path).GetAccessControl() + .GetAccessRules(includeExplicit: true, includeInherited: true, typeof(SecurityIdentifier)) + .Cast() + .Where(r => r.AccessControlType == AccessControlType.Allow && Users.Equals(r.IdentityReference)) + .Aggregate((FileSystemRights)0, (all, r) => all | r.FileSystemRights); + + [Fact] + public void The_protected_security_inherits_nothing_and_gives_users_read_only() + { + var security = AgentDirectoryAcl.BuildProtectedSecurity(); + + Assert.True(security.AreAccessRulesProtected); + var users = security.GetAccessRules(true, true, typeof(SecurityIdentifier)).Cast() + .Single(r => Users.Equals(r.IdentityReference)); + // An allow rule always carries Synchronize; .NET adds it on construction. + Assert.Equal(FileSystemRights.ReadAndExecute, users.FileSystemRights & ~FileSystemRights.Synchronize); + Assert.Equal((FileSystemRights)0, users.FileSystemRights & WriteRights); + } + + [Fact] + public void A_folder_is_protected_once_and_then_left_alone() + { + var first = AgentDirectoryAcl.EnsureProtected(_root); + var second = AgentDirectoryAcl.EnsureProtected(_root); + + Assert.Equal(AgentDirectoryAclOutcome.Applied, first.Outcome); + Assert.Equal(AgentDirectoryAclOutcome.AlreadyProtected, second.Outcome); + Assert.True(AgentDirectoryAcl.IsProtectedAsExpected(new DirectoryInfo(_root).GetAccessControl(AccessControlSections.Access))); + Assert.Equal((FileSystemRights)0, UsersRights(_root) & WriteRights); + } + + [Fact] + public void Existing_subfolders_take_over_the_protection() + { + // Install mode protects the base after Agent, Spool and Logs already exist. + var agent = Directory.CreateDirectory(Path.Combine(_root, "Agent")).FullName; + + AgentDirectoryAcl.EnsureProtected(_root); + + Assert.Equal((FileSystemRights)0, UsersRights(agent) & WriteRights); + Assert.NotEqual((FileSystemRights)0, UsersRights(agent) & FileSystemRights.ReadAndExecute); + } + + [Fact] + public void An_unprotected_or_widened_dacl_is_not_taken_as_protected() + { + var unprotected = AgentDirectoryAcl.BuildProtectedSecurity(); + unprotected.SetAccessRuleProtection(isProtected: false, preserveInheritance: true); + Assert.False(AgentDirectoryAcl.IsProtectedAsExpected(unprotected)); + + var widened = AgentDirectoryAcl.BuildProtectedSecurity(); + widened.AddAccessRule(new FileSystemAccessRule(Users, FileSystemRights.Modify, + InheritanceFlags.ContainerInherit | InheritanceFlags.ObjectInherit, PropagationFlags.None, AccessControlType.Allow)); + Assert.False(AgentDirectoryAcl.IsProtectedAsExpected(widened)); + } + + [Fact] + public void A_missing_folder_is_reported_not_created() + { + var missing = Path.Combine(_root, "does-not-exist"); + + Assert.Equal(AgentDirectoryAclOutcome.Missing, AgentDirectoryAcl.EnsureProtected(missing).Outcome); + Assert.False(Directory.Exists(missing)); + } + } +} diff --git a/src/Agent/AutopilotMonitor.Agent.V2.Core/Security/AgentDirectoryAcl.cs b/src/Agent/AutopilotMonitor.Agent.V2.Core/Security/AgentDirectoryAcl.cs new file mode 100644 index 00000000..69c072f3 --- /dev/null +++ b/src/Agent/AutopilotMonitor.Agent.V2.Core/Security/AgentDirectoryAcl.cs @@ -0,0 +1,115 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using System.Security.AccessControl; +using System.Security.Principal; + +namespace AutopilotMonitor.Agent.V2.Core.Security +{ + /// + /// Keeps %ProgramData%\AutopilotMonitor out of reach of standard users. Left alone the + /// folder inherits ProgramData's "Users may create files and folders" entry, and the SYSTEM + /// agent and its boot task start from the Agent subfolder: a user who can add a file there + /// can plant code the next SYSTEM start loads. During the device phase of OOBE only SYSTEM is + /// around, but the agent keeps running through the user phase and on the desktop until the + /// enrollment ends, restarted by the boot task after every reboot; that is the window this + /// closes. The payload catalog does not cover it: it is checked at install time, a file + /// added later is never looked at. + /// + /// The DACL is protected (nothing inherited from ProgramData): SYSTEM and Administrators full + /// control, Users read and execute, all inherited by the subfolders. Explicit entries on a + /// child stay. The summary dialog's temp folder lives outside this tree anyway. + /// + /// + public static class AgentDirectoryAcl + { + private const InheritanceFlags Inherit = InheritanceFlags.ContainerInherit | InheritanceFlags.ObjectInherit; + + private static readonly SecurityIdentifier LocalSystem = new SecurityIdentifier(WellKnownSidType.LocalSystemSid, null); + private static readonly SecurityIdentifier Administrators = new SecurityIdentifier(WellKnownSidType.BuiltinAdministratorsSid, null); + private static readonly SecurityIdentifier Users = new SecurityIdentifier(WellKnownSidType.BuiltinUsersSid, null); + + /// + /// Applies the protected DACL to unless it is already in place. + /// Never throws: a folder the agent cannot protect must not stop the agent. + /// + public static AgentDirectoryAclResult EnsureProtected(string path) + { + try + { + var directory = new DirectoryInfo(path); + if (!directory.Exists) + return new AgentDirectoryAclResult(AgentDirectoryAclOutcome.Missing, null); + + if (IsProtectedAsExpected(directory.GetAccessControl(AccessControlSections.Access))) + return new AgentDirectoryAclResult(AgentDirectoryAclOutcome.AlreadyProtected, null); + + directory.SetAccessControl(BuildProtectedSecurity()); + return new AgentDirectoryAclResult(AgentDirectoryAclOutcome.Applied, null); + } + catch (Exception ex) + { + return new AgentDirectoryAclResult(AgentDirectoryAclOutcome.Failed, ex.Message); + } + } + + internal static DirectorySecurity BuildProtectedSecurity() + { + var security = new DirectorySecurity(); + security.SetAccessRuleProtection(isProtected: true, preserveInheritance: false); + security.AddAccessRule(new FileSystemAccessRule(LocalSystem, FileSystemRights.FullControl, Inherit, PropagationFlags.None, AccessControlType.Allow)); + security.AddAccessRule(new FileSystemAccessRule(Administrators, FileSystemRights.FullControl, Inherit, PropagationFlags.None, AccessControlType.Allow)); + security.AddAccessRule(new FileSystemAccessRule(Users, FileSystemRights.ReadAndExecute, Inherit, PropagationFlags.None, AccessControlType.Allow)); + return security; + } + + /// + /// True when the DACL is protected and holds exactly the three entries above. Windows adds + /// when it stores an entry, so that bit is ignored. + /// + internal static bool IsProtectedAsExpected(DirectorySecurity security) + { + if (!security.AreAccessRulesProtected) + return false; + + var rules = security.GetAccessRules(includeExplicit: true, includeInherited: true, typeof(SecurityIdentifier)) + .Cast() + .ToList(); + + return rules.Count == 3 + && Has(rules, LocalSystem, FileSystemRights.FullControl) + && Has(rules, Administrators, FileSystemRights.FullControl) + && Has(rules, Users, FileSystemRights.ReadAndExecute); + } + + private static bool Has(IEnumerable rules, SecurityIdentifier sid, FileSystemRights rights) => + rules.Any(r => r.AccessControlType == AccessControlType.Allow + && !r.IsInherited + && sid.Equals(r.IdentityReference) + && (r.FileSystemRights & ~FileSystemRights.Synchronize) == (rights & ~FileSystemRights.Synchronize) + && r.InheritanceFlags == Inherit + && r.PropagationFlags == PropagationFlags.None); + } + + public enum AgentDirectoryAclOutcome + { + Applied, + AlreadyProtected, + Missing, + Failed, + } + + /// Outcome of ; only on failure. + public sealed class AgentDirectoryAclResult + { + public AgentDirectoryAclResult(AgentDirectoryAclOutcome outcome, string error) + { + Outcome = outcome; + Error = error; + } + + public AgentDirectoryAclOutcome Outcome { get; } + public string Error { get; } + } +} diff --git a/src/Agent/AutopilotMonitor.Agent.V2/Program.InstallMode.cs b/src/Agent/AutopilotMonitor.Agent.V2/Program.InstallMode.cs index 1575eb19..09722503 100644 --- a/src/Agent/AutopilotMonitor.Agent.V2/Program.InstallMode.cs +++ b/src/Agent/AutopilotMonitor.Agent.V2/Program.InstallMode.cs @@ -57,6 +57,8 @@ namespace AutopilotMonitor.Agent.V2 logger.Info("======================= Agent install mode (--install) ======================="); EnsureAgentDirectories(logger); + LogAgentDirectoryAcl(AgentDirectoryAcl.EnsureProtected( + Environment.ExpandEnvironmentVariables(Constants.AgentDataDirectory)), logger, alwaysLog: true); var sourceExePath = Assembly.GetExecutingAssembly().Location; var sourceDir = Path.GetDirectoryName(sourceExePath) ?? string.Empty; @@ -273,6 +275,26 @@ namespace AutopilotMonitor.Agent.V2 } } + /// + /// Logs the outcome of . Install mode logs + /// every outcome; the runtime start only a change or a failure, so a normal start stays quiet. + /// + internal static void LogAgentDirectoryAcl(AgentDirectoryAclResult result, AgentLogger logger, bool alwaysLog) + { + switch (result.Outcome) + { + case AgentDirectoryAclOutcome.Applied: + logger.Info("Agent data directory protected: SYSTEM and Administrators full control, Users read and execute, nothing inherited from ProgramData."); + break; + case AgentDirectoryAclOutcome.Failed: + logger.Warning($"Could not protect the agent data directory: {result.Error}"); + break; + default: + if (alwaysLog) logger.Info($"Agent data directory ACL: {result.Outcome}."); + break; + } + } + private static void EnsureAgentDirectories(AgentLogger logger) { var basePath = Environment.ExpandEnvironmentVariables(Constants.AgentDataDirectory); diff --git a/src/Agent/AutopilotMonitor.Agent.V2/Program.cs b/src/Agent/AutopilotMonitor.Agent.V2/Program.cs index 01ca614d..07e0ceb1 100644 --- a/src/Agent/AutopilotMonitor.Agent.V2/Program.cs +++ b/src/Agent/AutopilotMonitor.Agent.V2/Program.cs @@ -168,6 +168,11 @@ namespace AutopilotMonitor.Agent.V2 var formattedArgs = FormatArgsForLog(args); logger.Info($"Command line: {(string.IsNullOrEmpty(formattedArgs) ? "(no args)" : formattedArgs)}"); + // Same protection install mode applies, re-checked on every start: it also reaches + // agents that arrived by self-update over an install made before the hardening. + LogAgentDirectoryAcl(AgentDirectoryAcl.EnsureProtected( + Environment.ExpandEnvironmentVariables(Constants.AgentDataDirectory)), logger, alwaysLog: false); + try { return RunAgent(args, logger, dataDirectory, logDirectory, consoleMode);