diff options
| -rw-r--r-- | doc/Design.md | 6 | ||||
| -rw-r--r-- | plans/ImplementationPlan.md | 3 | ||||
| -rw-r--r-- | src/Agent.Windows/Security/TaskAclManager.cs | 23 | ||||
| -rw-r--r-- | src/Agent.Windows/Tools/WindowsToolBackend.cs | 32 | ||||
| -rw-r--r-- | tests/Agent.Windows.Tests/CrashScenarioBootstrap.cs | 6 | ||||
| -rw-r--r-- | tests/Agent.Windows.Tests/SecurityPolicyTests.cs | 147 |
6 files changed, 169 insertions, 48 deletions
diff --git a/doc/Design.md b/doc/Design.md index 693cdeb..a908441 100644 --- a/doc/Design.md +++ b/doc/Design.md @@ -477,8 +477,10 @@ network: - AppContainer外の通常user tokenに戻るbreakawayを許可しない。 ### 11.2 NTFS ACL - Policy Engineが許可pathから必要ACEを導出する。 -- 既存DACL全体を書き換えず、アプリケーションSID向けの必要ACEだけをtransactionalに追加する。 -- 元security descriptorまたは追加ACEの逆操作情報をRecovery Journalへ記録する。 +- 許可pathがdirectoryの場合、そのrootへ`ContainerInherit | ObjectInherit`付きのApplication SID ACEを1件だけtransactionalに追加し、既存およびTask中に作成されたdescendantへ継承させる。descendantごとのDACL列挙・snapshot・明示ACE追加は行わない。 +- 許可pathがfileの場合は、そのfileだけへ継承なしACEを追加する。異なる権限を持つnested rootは別scopeとして逆順cleanupする。 +- protected DACLを持つdescendantの継承保護は解除しない。そのpathはOS拒否のままとし、Sandbox権限を暗黙に拡張しない。 +- rootの元security descriptorをRecovery Journalへ1件記録する。 - Task終了で追加分を除去し、元状態との整合を検査する。 ### 11.3 Job Object - TaskごとにJob Objectを1つ作成する。 diff --git a/plans/ImplementationPlan.md b/plans/ImplementationPlan.md index 8127701..5af7606 100644 --- a/plans/ImplementationPlan.md +++ b/plans/ImplementationPlan.md @@ -364,7 +364,7 @@ LLM→Tool→Policyの境界が固定され、Policyを通らない副作用経 - FilesystemPolicy R/W/E/Delete - canonical path - reparse point防御 -- NTFS ACL +- NTFS ACL（承認directory rootへの継承ACE、root単位snapshot、protected descendantのfail-safe維持） - Process approval canonical path + SHA-256 - shell toggle - Job Object @@ -407,6 +407,7 @@ ST-01〜11、16全PASS。Security不具合を残したまま進行禁止。 - Recovery Journal - CLEAN / PREPARING / APPLIED / RUNNING / CLEANING / RECOVERY_REQUIRED - ACL適用前journal flush +- ACL journalは承認root単位とし、既存・新規descendantの継承とroot復元を検証 - Task Resource ownership - cleanup逆順処理 - 起動前Recovery diff --git a/src/Agent.Windows/Security/TaskAclManager.cs b/src/Agent.Windows/Security/TaskAclManager.cs index f01e859..2b091a6 100644 --- a/src/Agent.Windows/Security/TaskAclManager.cs +++ b/src/Agent.Windows/Security/TaskAclManager.cs @@ -26,7 +26,13 @@ public sealed class TaskAclManager(SecurityIdentifier appContainerSid, ISecurity security.AddAccessRule(rule); WriteSecurity(canonical, security); if (journalEntry is not null) journal!.MarkApplied(journalEntry.Value); - audit.Write(new(DateTimeOffset.UtcNow, taskId, "acl.apply", "applied", new Dictionary<string, string> { ["path"] = canonical, ["sid"] = appContainerSid.Value, ["rights"] = rights.ToString() })); + audit.Write(new(DateTimeOffset.UtcNow, taskId, "acl.apply", "applied", new Dictionary<string, string> + { + ["path"] = canonical, + ["sid"] = appContainerSid.Value, + ["rights"] = rights.ToString(), + ["scope"] = directory ? "root_inheritance" : "file" + })); return new(canonical, original, journalEntry); } @@ -39,21 +45,6 @@ public sealed class TaskAclManager(SecurityIdentifier appContainerSid, ISecurity audit.Write(new(DateTimeOffset.UtcNow, taskId, "acl.restore", "restored", new Dictionary<string, string> { ["path"] = snapshot.Path })); } - public void NormalizeNewEntry(Guid taskId, string path) - { - string canonical = CanonicalPath.Resolve(path); - FileSystemSecurity security = ReadSecurity(canonical); - security.SetAccessRuleProtection(isProtected: true, preserveInheritance: false); - foreach (FileSystemAccessRule rule in security.GetAccessRules(includeExplicit: true, includeInherited: false, typeof(SecurityIdentifier)) - .OfType<FileSystemAccessRule>() - .Where(rule => rule.IdentityReference == appContainerSid) - .ToArray()) - security.RemoveAccessRuleSpecific(rule); - security.SetAccessRuleProtection(isProtected: false, preserveInheritance: false); - WriteSecurity(canonical, security); - audit.Write(new(DateTimeOffset.UtcNow, taskId, "acl.normalize", "normalized", new Dictionary<string, string> { ["path"] = canonical })); - } - private static FileSystemSecurity ReadSecurity(string path) => Directory.Exists(path) ? FileSystemAclExtensions.GetAccessControl(new DirectoryInfo(path), AccessControlSections.Access) : FileSystemAclExtensions.GetAccessControl(new FileInfo(path), AccessControlSections.Access); diff --git a/src/Agent.Windows/Tools/WindowsToolBackend.cs b/src/Agent.Windows/Tools/WindowsToolBackend.cs index a21544b..c0d1561 100644 --- a/src/Agent.Windows/Tools/WindowsToolBackend.cs +++ b/src/Agent.Windows/Tools/WindowsToolBackend.cs @@ -49,8 +49,10 @@ public sealed class WindowsToolBackend(WindowsSandboxSettings settings, ISecurit // Do not rewrite a file ACL when execute is the only requested capability; CreateProcess remains // constrained by AppContainer access plus canonical path/SHA-256 approval. if (File.Exists(path) && capabilities == FileCapability.Execute) continue; - context.Roots.Add(path); - foreach (string target in ExistingAclTargets(path)) context.Acls.Add(acl.Grant(taskId, target, capabilities)); + // A single inheritable ACE on an approved directory covers existing and newly-created + // descendants. Snapshotting and rewriting every child ACL is both redundant and + // prohibitively expensive for runtime trees such as Python installations. + context.Acls.Add(acl.Grant(taskId, path, capabilities)); } IReadOnlyList<NetworkEndpoint> allowances = settings.NetworkMode == "allow" ? await NetworkAllowanceSnapshot.ResolveAsync(settings.NetworkAllowances, cancellationToken).ConfigureAwait(false) @@ -193,11 +195,6 @@ public sealed class WindowsToolBackend(WindowsSandboxSettings settings, ISecurit List<Exception> failures = []; try { context.Network?.Dispose(); } catch (Exception ex) { failures.Add(ex); } foreach (AclSnapshot snapshot in context.Acls.AsEnumerable().Reverse()) try { context.Acl.Restore(taskId, snapshot); } catch (Exception ex) { failures.Add(ex); } - var existingSnapshots = context.Acls.Select(static snapshot => snapshot.Path).ToHashSet(StringComparer.OrdinalIgnoreCase); - foreach (string entry in context.Roots.SelectMany(ExistingAclTargets).Distinct(StringComparer.OrdinalIgnoreCase) - .Where(entry => !existingSnapshots.Contains(entry)) - .OrderBy(static entry => entry.Count(character => character == Path.DirectorySeparatorChar))) - try { context.Acl.NormalizeNewEntry(taskId, entry); } catch (Exception ex) { failures.Add(ex); } if (failures.Count == 0) context.Journal.CompleteAndDelete(); await Task.CompletedTask; if (failures.Count > 0) throw new AggregateException("Task resource cleanup failed.", failures); @@ -215,26 +212,6 @@ public sealed class WindowsToolBackend(WindowsSandboxSettings settings, ISecurit var regex = new Regex(query, RegexOptions.CultureInvariant, TimeSpan.FromSeconds(1)); return files.Where(path => IsTextMatch(path, regex.IsMatch)); } - private static IEnumerable<string> ExistingAclTargets(string path) - { - if (File.Exists(path)) - { - yield return path; - yield break; - } - if (!Directory.Exists(path)) yield break; - var options = new EnumerationOptions - { - RecurseSubdirectories = true, - IgnoreInaccessible = false, - ReturnSpecialDirectories = false, - AttributesToSkip = FileAttributes.ReparsePoint - }; - foreach (string entry in Directory.EnumerateFileSystemEntries(path, "*", options) - .OrderByDescending(static entry => entry.Count(character => character == Path.DirectorySeparatorChar))) - yield return entry; - yield return path; - } private static string ApplyPatch(string path, string patch) { string current = File.Exists(path) ? File.ReadAllText(path) : string.Empty; @@ -251,7 +228,6 @@ public sealed class WindowsToolBackend(WindowsSandboxSettings settings, ISecurit public TaskResourceJournal Journal { get; } = journal; public TaskAclManager Acl { get; } = acl; public List<AclSnapshot> Acls { get; } = []; - public HashSet<string> Roots { get; } = new(StringComparer.OrdinalIgnoreCase); public TaskNetworkPolicy? Network { get; set; } } } diff --git a/tests/Agent.Windows.Tests/CrashScenarioBootstrap.cs b/tests/Agent.Windows.Tests/CrashScenarioBootstrap.cs index 7dd3629..dd170ab 100644 --- a/tests/Agent.Windows.Tests/CrashScenarioBootstrap.cs +++ b/tests/Agent.Windows.Tests/CrashScenarioBootstrap.cs @@ -26,6 +26,12 @@ internal static class CrashScenarioBootstrap var acl = new TaskAclManager(AppContainerProfile.EnsureFixed().Sid, new MemorySecurityAuditSink(), journal); _ = acl.Grant(taskId, Path.Combine(root, "resource"), FileCapability.Read | FileCapability.Write); break; + case "acl-inheritance": + var inheritedAcl = new TaskAclManager( + new System.Security.Principal.SecurityIdentifier("S-1-15-2-330000001-330000002-330000003-330000004-330000005-330000006-330000007"), + new MemorySecurityAuditSink(), journal); + _ = inheritedAcl.Grant(taskId, Path.Combine(root, "resource"), FileCapability.Read | FileCapability.Write); + break; case "acl-before": byte[] descriptor = FileSystemAclExtensions.GetAccessControl(new DirectoryInfo(Path.Combine(root, "resource")), AccessControlSections.Access) .GetSecurityDescriptorBinaryForm(); diff --git a/tests/Agent.Windows.Tests/SecurityPolicyTests.cs b/tests/Agent.Windows.Tests/SecurityPolicyTests.cs index d54ab13..8d920b2 100644 --- a/tests/Agent.Windows.Tests/SecurityPolicyTests.cs +++ b/tests/Agent.Windows.Tests/SecurityPolicyTests.cs @@ -192,6 +192,75 @@ public sealed class SecurityPolicyTests } [Fact] + public void Root_acl_inheritance_covers_existing_and_new_descendants_and_restores_exactly() + { + using TestDirectory directory = new(); + using TestDirectory state = new(); + string existingDirectory = Path.Combine(directory.Path, "existing"); + string existingFile = Path.Combine(existingDirectory, "before.txt"); + Directory.CreateDirectory(existingDirectory); + File.WriteAllText(existingFile, "before"); + byte[] rootBefore = AccessDescriptor(directory.Path); + byte[] existingBefore = AccessDescriptor(existingFile); + var sid = new System.Security.Principal.SecurityIdentifier( + "S-1-15-2-310000001-310000002-310000003-310000004-310000005-310000006-310000007"); + var audit = new MemorySecurityAuditSink(); + Guid taskId = Guid.NewGuid(); + TaskResourceJournal journal = TaskResourceJournal.Create(Path.Combine(state.Path, "journals"), taskId); + TaskAclManager manager = new(sid, audit, journal); + + AclSnapshot snapshot = manager.Grant(taskId, directory.Path, FileCapability.Read | FileCapability.Write); + JournalEntry aclEntry = Assert.Single(journal.Entries); + Assert.Equal("acl", aclEntry.Kind); + Assert.Equal(directory.Path, aclEntry.Target, ignoreCase: true); + Assert.Contains(AccessRules(directory.Path, sid), static rule => !rule.IsInherited); + Assert.Contains(AccessRules(existingFile, sid), static rule => rule.IsInherited); + + string createdDirectory = Path.Combine(directory.Path, "created"); + string createdFile = Path.Combine(createdDirectory, "during.txt"); + Directory.CreateDirectory(createdDirectory); + File.WriteAllText(createdFile, "during"); + Assert.Contains(AccessRules(createdFile, sid), static rule => rule.IsInherited); + + manager.Restore(taskId, snapshot); + journal.CompleteAndDelete(); + + Assert.Equal(Convert.ToHexString(rootBefore), Convert.ToHexString(AccessDescriptor(directory.Path))); + Assert.Equal(Convert.ToHexString(existingBefore), Convert.ToHexString(AccessDescriptor(existingFile))); + Assert.Empty(AccessRules(existingFile, sid)); + Assert.Empty(AccessRules(createdFile, sid)); + SecurityAuditEvent applied = Assert.Single(audit.Events, item => item.Event == "acl.apply"); + Assert.Equal("root_inheritance", applied.Data["scope"]); + Assert.Single(audit.Events, item => item.Event == "acl.restore"); + Assert.Empty(Directory.EnumerateFiles(Path.Combine(state.Path, "journals"))); + } + + [Fact] + public void Root_acl_inheritance_does_not_override_a_protected_descendant() + { + using TestDirectory directory = new(); + string protectedDirectory = Path.Combine(directory.Path, "protected"); + Directory.CreateDirectory(protectedDirectory); + DirectorySecurity protectedAcl = FileSystemAclExtensions.GetAccessControl( + new DirectoryInfo(protectedDirectory), AccessControlSections.Access); + protectedAcl.SetAccessRuleProtection(isProtected: true, preserveInheritance: true); + FileSystemAclExtensions.SetAccessControl(new DirectoryInfo(protectedDirectory), protectedAcl); + var sid = new System.Security.Principal.SecurityIdentifier( + "S-1-15-2-320000001-320000002-320000003-320000004-320000005-320000006-320000007"); + TaskAclManager manager = new(sid, new MemorySecurityAuditSink()); + + AclSnapshot snapshot = manager.Grant(Guid.NewGuid(), directory.Path, FileCapability.Read); + try + { + Assert.Empty(AccessRules(protectedDirectory, sid)); + } + finally + { + manager.Restore(Guid.NewGuid(), snapshot); + } + } + + [Fact] public void Brokered_process_runs_in_fixed_app_container_with_task_acl() { AppContainerProfile profile = AppContainerProfile.EnsureFixed(); @@ -273,14 +342,18 @@ public sealed class SecurityPolicyTests string journals = Path.Combine(directory.Path, "journals"); string resource = Path.Combine(directory.Path, "resource"); Directory.CreateDirectory(resource); + string child = Path.Combine(resource, "existing.txt"); + File.WriteAllText(child, "existing"); byte[] aclBefore = FileSystemAclExtensions.GetAccessControl(new DirectoryInfo(resource), AccessControlSections.Access) .GetSecurityDescriptorBinaryForm(); + byte[] childAclBefore = AccessDescriptor(child); var audit = new MemorySecurityAuditSink(); Guid taskId = Guid.NewGuid(); TaskResourceJournal journal = TaskResourceJournal.Create(journals, taskId); var acl = new TaskAclManager(profile.Sid, audit, journal); _ = acl.Grant(taskId, resource, FileCapability.Read | FileCapability.Write); + Assert.Contains(AccessRules(child, profile.Sid), static rule => rule.IsInherited); Assert.False(LoopbackExemption.Contains(profile.Sid)); Guid loopbackEntry = journal.Prepare("network.loopback_exemption", profile.Sid.Value, @@ -307,6 +380,8 @@ public sealed class SecurityPolicyTests byte[] aclAfter = FileSystemAclExtensions.GetAccessControl(new DirectoryInfo(resource), AccessControlSections.Access) .GetSecurityDescriptorBinaryForm(); Assert.Equal(Convert.ToHexString(aclBefore), Convert.ToHexString(aclAfter)); + Assert.Equal(Convert.ToHexString(childAclBefore), Convert.ToHexString(AccessDescriptor(child))); + Assert.Empty(AccessRules(child, profile.Sid)); Assert.Empty(Directory.EnumerateFiles(journals)); } @@ -324,9 +399,12 @@ public sealed class SecurityPolicyTests string resourceDirectory = Path.Combine(directory.Path, "resource"); string resourceFile = Path.Combine(directory.Path, "resource.txt"); Directory.CreateDirectory(resourceDirectory); + string childFile = Path.Combine(resourceDirectory, "child.txt"); + File.WriteAllText(childFile, "child-before-crash"); File.WriteAllText(resourceFile, "before-crash"); byte[] aclBefore = FileSystemAclExtensions.GetAccessControl(new DirectoryInfo(resourceDirectory), AccessControlSections.Access) .GetSecurityDescriptorBinaryForm(); + byte[] childAclBefore = AccessDescriptor(childFile); Guid taskId = Guid.NewGuid(); using var child = new Process { @@ -358,6 +436,8 @@ public sealed class SecurityPolicyTests byte[] aclAfter = FileSystemAclExtensions.GetAccessControl(new DirectoryInfo(resourceDirectory), AccessControlSections.Access) .GetSecurityDescriptorBinaryForm(); Assert.Equal(Convert.ToHexString(aclBefore), Convert.ToHexString(aclAfter)); + Assert.Equal(Convert.ToHexString(childAclBefore), Convert.ToHexString(AccessDescriptor(childFile))); + Assert.Empty(AccessRules(childFile, AppContainerProfile.EnsureFixed().Sid)); Assert.False(LoopbackExemption.Contains(AppContainerProfile.EnsureFixed().Sid)); string backups = Path.Combine(directory.Path, "backups"); Assert.True(!Directory.Exists(backups) || !Directory.EnumerateFileSystemEntries(backups).Any()); @@ -371,6 +451,51 @@ public sealed class SecurityPolicyTests } } + [Fact] + public void Independent_process_crash_recovers_root_inheritance_from_existing_child() + { + using TestDirectory directory = new(); + string resource = Path.Combine(directory.Path, "resource"); + string childFile = Path.Combine(resource, "child.txt"); + Directory.CreateDirectory(resource); + File.WriteAllText(childFile, "before-crash"); + byte[] rootBefore = AccessDescriptor(resource); + byte[] childBefore = AccessDescriptor(childFile); + var sid = new System.Security.Principal.SecurityIdentifier( + "S-1-15-2-330000001-330000002-330000003-330000004-330000005-330000006-330000007"); + Guid taskId = Guid.NewGuid(); + using var child = new Process + { + StartInfo = new ProcessStartInfo + { + FileName = Environment.ProcessPath!, + UseShellExecute = false, + CreateNoWindow = true, + Environment = + { + ["AGENT_MCP_CRASH_SCENARIO"] = "acl-inheritance", + ["AGENT_MCP_CRASH_ROOT"] = directory.Path, + ["AGENT_MCP_CRASH_TASK"] = taskId.ToString("D") + } + } + }; + + Assert.True(child.Start()); + Assert.True(child.WaitForExit(30_000)); + Assert.NotEqual(0, child.ExitCode); + Assert.False(File.Exists(Path.Combine(directory.Path, "crash-error.txt")), + File.Exists(Path.Combine(directory.Path, "crash-error.txt")) ? File.ReadAllText(Path.Combine(directory.Path, "crash-error.txt")) : string.Empty); + Assert.Contains(AccessRules(childFile, sid), static rule => rule.IsInherited); + + RecoveryResult recovered = new RecoveryManager(Path.Combine(directory.Path, "journals"), new MemorySecurityAuditSink()).RecoverAll(); + + Assert.True(recovered.IsClean, string.Join(Environment.NewLine, recovered.Errors)); + Assert.Equal(Convert.ToHexString(rootBefore), Convert.ToHexString(AccessDescriptor(resource))); + Assert.Equal(Convert.ToHexString(childBefore), Convert.ToHexString(AccessDescriptor(childFile))); + Assert.Empty(AccessRules(childFile, sid)); + Assert.Empty(Directory.EnumerateFiles(Path.Combine(directory.Path, "journals"))); + } + private static bool ProcessExists(int pid) { try { using Process process = Process.GetProcessById(pid); return !process.HasExited; } @@ -440,7 +565,8 @@ public sealed class SecurityPolicyTests string backups = Path.Combine(directory.Path, "backups"); Assert.True(!Directory.Exists(backups) || !Directory.EnumerateFileSystemEntries(backups).Any()); Assert.Contains(audit.Events, item => item.Event == "wfp.remove" && item.Data["residual_filters"] == "0"); - Assert.Contains(audit.Events, item => item.Event == "acl.restore"); + Assert.Single(audit.Events, item => item.Event == "acl.apply"); + Assert.Single(audit.Events, item => item.Event == "acl.restore"); } [Fact(Timeout = 60_000)] @@ -514,6 +640,25 @@ public sealed class SecurityPolicyTests private static RestrictedProcessResult Curl(RestrictedProcessLauncher launcher, Guid taskId, string curl, string url, string cwd) => launcher.Run(taskId, curl, ["--noproxy", "*", "--silent", "--show-error", "--max-time", "2", url], cwd, TimeSpan.FromSeconds(10)); + private static byte[] AccessDescriptor(string path) + { + FileSystemSecurity security = Directory.Exists(path) + ? FileSystemAclExtensions.GetAccessControl(new DirectoryInfo(path), AccessControlSections.Access) + : FileSystemAclExtensions.GetAccessControl(new FileInfo(path), AccessControlSections.Access); + return security.GetSecurityDescriptorBinaryForm(); + } + + private static FileSystemAccessRule[] AccessRules(string path, System.Security.Principal.SecurityIdentifier sid) + { + FileSystemSecurity security = Directory.Exists(path) + ? FileSystemAclExtensions.GetAccessControl(new DirectoryInfo(path), AccessControlSections.Access) + : FileSystemAclExtensions.GetAccessControl(new FileInfo(path), AccessControlSections.Access); + return security.GetAccessRules(includeExplicit: true, includeInherited: true, typeof(System.Security.Principal.SecurityIdentifier)) + .OfType<FileSystemAccessRule>() + .Where(rule => rule.IdentityReference == sid && rule.AccessControlType == AccessControlType.Allow) + .ToArray(); + } + private sealed class TestDirectory : IDisposable { public TestDirectory() |