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);