From 21c07a7eee62b7ead82a08129e51eb6e927a41ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Tue, 27 Oct 2026 23:05:14 +0900 Subject: [PATCH 1/9] docs(agent-installer): state the data folder ownership rules The installer must set the owner of the agent data folder, not only its permissions, and must refuse a data folder or a parent folder that another account controls, because such a folder may hold entries that account left there. A full uninstall never refuses, and neither setting the folder's security nor deleting the Agent Identity state follows a link. Issue: DGW-478 Co-Authored-By: Claude Opus 5.5 --- package/AgentWindowsManaged/INTENT.md | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/package/AgentWindowsManaged/INTENT.md b/package/AgentWindowsManaged/INTENT.md index 658f07439..0ddf4d606 100644 --- a/package/AgentWindowsManaged/INTENT.md +++ b/package/AgentWindowsManaged/INTENT.md @@ -1,3 +1,11 @@ +## Data folder + +- The installer gives `%ProgramData%\Devolutions\Agent` its owner and its permissions when it creates the folder, and resets both on every install, upgrade and repair, so that only SYSTEM and administrators can rename or replace what the agent stores there. +- An install, upgrade, repair or feature change fails before it changes anything, with an error in the installer's language naming the folder and the account at fault, when `%ProgramData%\Devolutions` or `%ProgramData%\Devolutions\Agent` is a link, can't be read, or has an owner other than SYSTEM, Administrators, TrustedInstaller or a member of Administrators, or when `%ProgramData%\Devolutions` lets another account rename or replace it or an entry of it. + The installer refuses rather than repairs, because a folder another account controlled may hold entries that account left there, and it refuses the folders the agent refuses, so that a deployment never reports success for an agent that can't use its data folder. + A full uninstall never refuses, so a machine can always be uninstalled. +- The installer never follows a link when it sets the security of a data folder, so that a link can't make it change the security of another folder. + ## Agent Identity - The token is passed as the `IDENTITY_ENROLLMENT_TOKEN` property, which is hidden so that it never appears in installer logs. @@ -10,6 +18,7 @@ - Given a token, the MSI runs `identity enroll -` on a first install or a major upgrade, never on a repair, a maintenance change or an uninstall, and never without a token. If the command fails, for example on a malformed token, the install fails, so a deployment never reports success for a machine that cannot enroll; the install never waits for the authority, whose availability the service handles. - Only a full uninstall deletes the `identity` folder of the agent's data directory, rejection markers included, and every device key with the agent's prefix, without contacting any authority. + The uninstall never follows a link while it deletes that folder, so that a link can't make it delete files elsewhere. A reinstall therefore behaves like a freshly imaged device: after a revocation, the same still-valid token enrolls a new identity and uses up a token use. A failed cleanup never fails the uninstall, so a machine can always be uninstalled. - A major upgrade keeps the identities and their keys. From db274a827fa78c41e885848bfef40707a5530817 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Tue, 27 Oct 2026 21:45:45 +0900 Subject: [PATCH 2/9] fix(agent-installer): harden the agent data folder security The installer now creates the agent data folder with its final security descriptor, so it never exists with the DACL it would inherit from ProgramData, which lets users add entries. When it resets the security of that folder and of its pedm folder, it also sets the owner, and it refuses a folder that is a reparse point rather than applying the security to the link's target. The security is set on a handle with SetKernelObjectSecurity, which leaves the entries below the folder unchanged. A full uninstall deletes the Agent Identity state without following any link: each directory is opened as itself and held without delete sharing while the entries below it are resolved, and each entry is deleted through its own handle. A link is deleted itself, never its target, and nothing is deleted when the data folder or its parent is a link. A new helper reads a folder's owner without following links and tells whether an owner is trusted: SYSTEM, Administrators, TrustedInstaller, or a member of Administrators, decided as the agent decides it. Nothing refuses an untrusted owner yet. Issue: DGW-478 Co-Authored-By: Claude Opus 5.5 --- .../SecureDirectoryTests.cs | 275 ++++++++++++ .../Actions/CustomActions.cs | 34 +- .../Actions/SecureDirectory.cs | 421 ++++++++++++++++++ package/AgentWindowsManaged/Actions/WinAPI.cs | 185 ++++++++ 4 files changed, 900 insertions(+), 15 deletions(-) create mode 100644 package/AgentWindowsManaged.Tests/SecureDirectoryTests.cs create mode 100644 package/AgentWindowsManaged/Actions/SecureDirectory.cs diff --git a/package/AgentWindowsManaged.Tests/SecureDirectoryTests.cs b/package/AgentWindowsManaged.Tests/SecureDirectoryTests.cs new file mode 100644 index 000000000..93a144f4e --- /dev/null +++ b/package/AgentWindowsManaged.Tests/SecureDirectoryTests.cs @@ -0,0 +1,275 @@ +using System; +using System.Diagnostics; +using System.IO; +using System.Linq; +using System.Security.AccessControl; +using System.Security.Principal; + +using DevolutionsAgent.Actions; + +using Xunit; +using Xunit.Abstractions; + +namespace DevolutionsAgent.Installer.Tests; + +public sealed class SecureDirectoryTests +{ + private static readonly SecurityIdentifier CurrentUser = WindowsIdentity.GetCurrent().User; + + private static readonly SecurityIdentifier LocalSystem = new(WellKnownSidType.LocalSystemSid, null); + + // Owned by the current user, with a protected DACL granting full control to the current user and SYSTEM only. + // A folder under the temporary directory inherits more entries, such as one for Administrators. + private static readonly string ProtectedSddl = $"O:{CurrentUser}D:P(A;OICI;FA;;;{CurrentUser})(A;OICI;FA;;;SY)"; + + private static readonly string[] IdentityNames = ["Devolutions", "Agent", "identity"]; + + private readonly ILogger logger; + + public SecureDirectoryTests(ITestOutputHelper output) => this.logger = new OutputLogger(output); + + // oracle: intent package/AgentWindowsManaged/INTENT.md §Agent Identity + [Fact] + public void DeleteTreeKeepsWhatAJunctionInsideTheTreePointsTo() + { + using TempFolder folder = new(); + string outside = folder.CreateDirectory("outside"); + File.WriteAllText(Path.Combine(outside, "file.txt"), "kept"); + string identity = folder.CreateDirectory(IdentityNames); + string pending = Directory.CreateDirectory(Path.Combine(identity, "pending")).FullName; + File.WriteAllText(Path.Combine(pending, "token.dat"), "deleted"); + CreateJunction(Path.Combine(pending, "link"), outside); + + SecureDirectory.DeleteTree(folder.Path, IdentityNames, this.logger); + + Assert.False(Directory.Exists(identity)); + Assert.True(File.Exists(Path.Combine(outside, "file.txt"))); + } + + // oracle: intent package/AgentWindowsManaged/INTENT.md §Agent Identity + [Fact] + public void DeleteTreeDeletesAJunctionAtThePathButNotItsTarget() + { + using TempFolder folder = new(); + string outside = folder.CreateDirectory("outside"); + File.WriteAllText(Path.Combine(outside, "file.txt"), "kept"); + folder.CreateDirectory("Devolutions", "Agent"); + string identity = Path.Combine(folder.Path, "Devolutions", "Agent", "identity"); + CreateJunction(identity, outside); + + SecureDirectory.DeleteTree(folder.Path, IdentityNames, this.logger); + + Assert.False(Directory.Exists(identity)); + Assert.True(File.Exists(Path.Combine(outside, "file.txt"))); + } + + // oracle: mutant the deletion follows a junction at the data directory into its target + [Fact] + public void DeleteTreeDeletesNothingThroughAJunctionAboveThePath() + { + using TempFolder folder = new(); + string outside = folder.CreateDirectory("outside"); + string outsideIdentity = folder.CreateDirectory("outside", "identity"); + File.WriteAllText(Path.Combine(outsideIdentity, "file.txt"), "kept"); + folder.CreateDirectory("Devolutions"); + CreateJunction(Path.Combine(folder.Path, "Devolutions", "Agent"), outside); + + SecureDirectory.DeleteTree(folder.Path, IdentityNames, this.logger); + + Assert.True(File.Exists(Path.Combine(outsideIdentity, "file.txt"))); + } + + // oracle: mutant the security is applied through a junction to its target + [Fact] + public void SetSecurityRefusesAJunctionAndLeavesItsTargetUnchanged() + { + using TempFolder folder = new(); + string target = folder.CreateDirectory("target"); + string before = Sddl(target); + string link = Path.Combine(folder.Path, "link"); + CreateJunction(link, target); + + Assert.Throws(() => SecureDirectory.SetSecurity(link, ProtectedSddl)); + + Assert.Equal(before, Sddl(target)); + } + + // oracle: mutant the owner or the DACL is left out, or the DACL loses its protection + [Fact] + public void SetSecurityAppliesTheOwnerAndTheProtectedDacl() + { + using TempFolder folder = new(); + string path = folder.CreateDirectory("data"); + + SecureDirectory.SetSecurity(path, ProtectedSddl); + + AssertProtectedSecurity(path); + } + + // oracle: mutant inheritable entries are propagated to existing children + [Fact] + public void SetSecurityLeavesExistingChildrenUnchanged() + { + using TempFolder folder = new(); + string path = folder.CreateDirectory("data"); + string child = folder.CreateDirectory("data", "child"); + string before = Sddl(child); + + SecureDirectory.SetSecurity(path, ProtectedSddl); + + Assert.Equal(before, Sddl(child)); + } + + // oracle: mutant the directory is created with the DACL inherited from its parent + [Fact] + public void CreateGivesTheNewDirectoryExactlyTheSecurityDescriptor() + { + using TempFolder folder = new(); + string path = Path.Combine(folder.Path, "data"); + + Assert.True(SecureDirectory.Create(path, ProtectedSddl)); + + AssertProtectedSecurity(path); + } + + [Theory] + // oracle: reference crates/now-package-broker/src/policy_security.rs TrustedWriters::AdminOrTrustedInstaller + [InlineData("SYSTEM", "S-1-5-18", true)] + // oracle: reference crates/agent-identity/src/windows.rs owner_is_administrator_member + [InlineData("Administrators", "S-1-5-32-544", true)] + // oracle: reference crates/now-package-broker/src/policy_security.rs TrustedWriters::AdminOrTrustedInstaller + [InlineData("TrustedInstaller", "S-1-5-80-956008885-3418522649-1831038044-1853292631-2271478464", true)] + // oracle: reference crates/agent-identity/src/windows.rs owner_is_administrator_member + [InlineData("Everyone", "S-1-1-0", false)] + // oracle: reference crates/agent-identity/src/windows.rs owner_is_administrator_member + [InlineData("Users", "S-1-5-32-545", false)] + // oracle: reference crates/agent-identity/src/windows.rs owner_is_administrator_member + [InlineData("LOCAL SERVICE", "S-1-5-19", false)] + public void OwnerIsTrustedOnlyWhenPrivileged(string name, string sid, bool expected) + { + Assert.True(expected == SecureDirectory.IsTrustedOwner(new SecurityIdentifier(sid)), name); + } + + // oracle: mutant the owner is read from the target of a junction + [Fact] + public void ReadOwnerReportsAJunctionItself() + { + using TempFolder folder = new(); + string link = Path.Combine(folder.Path, "link"); + CreateJunction(link, folder.CreateDirectory("target")); + + Assert.True(SecureDirectory.ReadOwner(link).IsReparsePoint); + } + + // oracle: mutant the owner is not read from the directory + [Fact] + public void ReadOwnerReadsTheOwnerOfADirectory() + { + using TempFolder folder = new(); + string path = folder.CreateDirectory("data"); + SecureDirectory.SetSecurity(path, ProtectedSddl); + + (SecurityIdentifier owner, bool isReparsePoint) = SecureDirectory.ReadOwner(path); + + Assert.Equal(CurrentUser, owner); + Assert.False(isReparsePoint); + } + + /// + /// Asserts that has exactly the owner and the protected DACL of , + /// with no inherited entry. + /// + private static void AssertProtectedSecurity(string path) + { + DirectorySecurity security = new DirectoryInfo(path).GetAccessControl(); + + Assert.Equal(CurrentUser, security.GetOwner(typeof(SecurityIdentifier))); + Assert.True(security.AreAccessRulesProtected); + + string[] rules = security.GetAccessRules(true, true, typeof(SecurityIdentifier)) + .Cast() + .Select(r => $"{r.IdentityReference} {r.AccessControlType} {r.FileSystemRights} {r.InheritanceFlags} {r.PropagationFlags} inherited={r.IsInherited}") + .OrderBy(r => r, StringComparer.Ordinal) + .ToArray(); + string[] expected = new[] { CurrentUser, LocalSystem } + .Select(sid => $"{sid} Allow FullControl ContainerInherit, ObjectInherit None inherited=False") + .OrderBy(r => r, StringComparer.Ordinal) + .ToArray(); + + Assert.Equal(expected, rules); + } + + /// + /// The owner and DACL of , following a link at that path. + /// + private static string Sddl(string path) => + new DirectoryInfo(path) + .GetAccessControl(AccessControlSections.Owner | AccessControlSections.Access) + .GetSecurityDescriptorSddlForm(AccessControlSections.Owner | AccessControlSections.Access); + + /// + /// Creates a junction at to the directory , which needs no + /// elevation. + /// + private static void CreateJunction(string link, string target) + { + ProcessStartInfo startInfo = new("cmd.exe", $"/c mklink /J \"{link}\" \"{target}\"") + { + UseShellExecute = false, + CreateNoWindow = true, + RedirectStandardOutput = true, + RedirectStandardError = true, + }; + + using Process process = Process.Start(startInfo); + string output = process.StandardOutput.ReadToEnd() + process.StandardError.ReadToEnd(); + process.WaitForExit(); + + Assert.True(process.ExitCode == 0, output); + } + + private sealed class OutputLogger(ITestOutputHelper output) : ILogger + { + public void Log(string msg) => output.WriteLine(msg); + + public void Log(string format, params object[] args) => output.WriteLine(format, args); + } + + /// + /// A new, empty folder under the temporary directory, deleted with its content on disposal. + /// + private sealed class TempFolder : IDisposable + { + public string Path { get; } = System.IO.Path.Combine(System.IO.Path.GetTempPath(), $"secure-directory-{Guid.NewGuid():N}"); + + public TempFolder() => Directory.CreateDirectory(this.Path); + + /// + /// Creates the directory below this folder, with its parents. + /// + public string CreateDirectory(params string[] names) => + Directory.CreateDirectory(System.IO.Path.Combine([this.Path, .. names])).FullName; + + public void Dispose() + { + // A recursive Directory.Delete fails on a junction without elevation, so junctions go first. + RemoveJunctions(this.Path); + Directory.Delete(this.Path, recursive: true); + } + + private static void RemoveJunctions(string directory) + { + foreach (string entry in Directory.GetDirectories(directory)) + { + if (File.GetAttributes(entry).HasFlag(FileAttributes.ReparsePoint)) + { + Directory.Delete(entry); + } + else + { + RemoveJunctions(entry); + } + } + } + } +} diff --git a/package/AgentWindowsManaged/Actions/CustomActions.cs b/package/AgentWindowsManaged/Actions/CustomActions.cs index e41c1f7ae..9f962f6e2 100644 --- a/package/AgentWindowsManaged/Actions/CustomActions.cs +++ b/package/AgentWindowsManaged/Actions/CustomActions.cs @@ -155,8 +155,16 @@ public static ActionResult CreateProgramDataDirectory(Session session) try { - DirectoryInfo di = Directory.CreateDirectory(path); - session.Log($"created directory at {di.FullName} or already exists"); + // The data directory gets its security descriptor when it is created, so that it never exists with the + // DACL it would inherit from ProgramData, which lets users add entries. + Directory.CreateDirectory(Path.GetDirectoryName(path)); + + if (!SecureDirectory.Create(path, Includes.PROGRAM_DATA_SDDL) && !Directory.Exists(path)) + { + throw new IOException($"{path} exists and is not a directory"); + } + + session.Log($"created directory at {path} or already exists"); } catch (Exception e) { @@ -805,22 +813,18 @@ public static ActionResult RemoveAgentIdentity(Session session) } } - string identityDirectory = Path.Combine(ProgramDataDirectory, "identity"); - + // The deletion never follows a link, so that a link in the data directory can't make the uninstall delete + // files elsewhere. try { - if (Directory.Exists(identityDirectory)) - { - // UNTESTED: Directory.Delete deletes a junction or symbolic link without recursing into its target. - // Otherwise a link planted in the identity folder would make the uninstall delete files elsewhere. - // When replacing this call, keep the deletion from following reparse points. - Directory.Delete(identityDirectory, true); - session.Log($"deleted {identityDirectory}"); - } + SecureDirectory.DeleteTree( + Environment.GetFolderPath(Environment.SpecialFolder.CommonApplicationData), + ["Devolutions", "Agent", "identity"], + LogDelegate.WithSession(session)); } catch (Exception e) { - session.Log($"failed to delete {identityDirectory}: {e}"); + session.Log($"failed to delete the Agent Identity state: {e}"); } return ActionResult.Success; @@ -1889,7 +1893,7 @@ public static ActionResult SetProgramDataDirectoryPermissions(Session session) { try { - SetFileSecurity(session, ProgramDataDirectory, Includes.PROGRAM_DATA_SDDL); + SecureDirectory.SetSecurity(ProgramDataDirectory, Includes.PROGRAM_DATA_SDDL); return ActionResult.Success; } catch (Exception e) @@ -1904,7 +1908,7 @@ public static ActionResult SetProgramDataPedmDirectoryPermissions(Session sessio { try { - SetFileSecurity(session, Path.Combine(ProgramDataDirectory, "pedm"), Includes.PROGRAM_DATA_PEDM_SDDL); + SecureDirectory.SetSecurity(Path.Combine(ProgramDataDirectory, "pedm"), Includes.PROGRAM_DATA_PEDM_SDDL); return ActionResult.Success; } catch (Exception e) diff --git a/package/AgentWindowsManaged/Actions/SecureDirectory.cs b/package/AgentWindowsManaged/Actions/SecureDirectory.cs new file mode 100644 index 000000000..5a3bb00c6 --- /dev/null +++ b/package/AgentWindowsManaged/Actions/SecureDirectory.cs @@ -0,0 +1,421 @@ +using Microsoft.Win32.SafeHandles; +using System; +using System.Collections.Generic; +using System.ComponentModel; +using System.IO; +using System.Linq; +using System.Runtime.InteropServices; +using System.Security.Principal; + +namespace DevolutionsAgent.Actions; + +/// +/// Directory operations that act on the entry a path names, never on the target of a link at that path. +/// +/// +/// Each path is opened with FILE_FLAG_OPEN_REPARSE_POINT, which opens a junction or a symbolic link itself, and +/// every decision is taken from the opened handle, so a link swapped in after a check is never followed. +/// +internal static class SecureDirectory +{ + private static readonly SecurityIdentifier Administrators = new(WellKnownSidType.BuiltinAdministratorsSid, null); + + /// + /// NT SERVICE\TrustedInstaller, the Windows Modules Installer service. + /// + private static readonly SecurityIdentifier TrustedInstaller = + new("S-1-5-80-956008885-3418522649-1831038044-1853292631-2271478464"); + + // UNTESTED: the deletion holds each directory handle, without FILE_SHARE_DELETE, until the entries below the + // directory are done. + // Renaming or replacing a directory needs delete access to it, so no path below a held directory can be redirected + // to another folder. + // When changing how the deletion opens or closes a directory, keep this share mode and keep the handle open that long. + private static readonly uint HeldShare = WinAPI.FILE_SHARE_READ | WinAPI.FILE_SHARE_WRITE; + + private static readonly uint AnyShare = WinAPI.FILE_SHARE_READ | WinAPI.FILE_SHARE_WRITE | WinAPI.FILE_SHARE_DELETE; + + /// + /// Creates the directory with the security descriptor , so that it + /// never exists with the DACL it would inherit from its parent. + /// + /// false when an entry named already exists, which is left unchanged. + internal static bool Create(string path, string sddl) + { + using SecurityDescriptor descriptor = new(sddl); + WinAPI.SECURITY_ATTRIBUTES attributes = new() + { + nLength = (uint)Marshal.SizeOf(), + lpSecurityDescriptor = descriptor.Pointer, + }; + + if (WinAPI.CreateDirectory(path, ref attributes)) + { + return true; + } + + int error = Marshal.GetLastWin32Error(); + + if (error == WinAPI.ERROR_ALREADY_EXISTS) + { + return false; + } + + throw new Win32Exception(error); + } + + /// + /// Sets the owner and the DACL of the directory to those of . + /// + /// + /// Only the directory changes: unlike SetSecurityInfo, this doesn't propagate inheritable entries to existing + /// children, so the entries below keep the security they have. + /// + /// is a reparse point or not a directory. + internal static void SetSecurity(string path, string sddl) + { + using SafeFileHandle handle = Open( + path, WinAPI.WRITE_DAC | WinAPI.WRITE_OWNER | WinAPI.FILE_READ_ATTRIBUTES, AnyShare); + + if (!IsPlainDirectory(GetAttributes(handle))) + { + throw new IOException($"{path} is a reparse point or not a directory"); + } + + using SecurityDescriptor descriptor = new(sddl); + + if (!WinAPI.SetKernelObjectSecurity( + handle, + WinAPI.OWNER_SECURITY_INFORMATION | WinAPI.DACL_SECURITY_INFORMATION, + descriptor.Pointer)) + { + throw new Win32Exception(Marshal.GetLastWin32Error()); + } + } + + /// + /// The owner of the entry names, opened without following a link, and whether that entry + /// is a reparse point. + /// + internal static (SecurityIdentifier Owner, bool IsReparsePoint) ReadOwner(string path) + { + using SafeFileHandle handle = Open(path, WinAPI.READ_CONTROL | WinAPI.FILE_READ_ATTRIBUTES, AnyShare); + bool isReparsePoint = (GetAttributes(handle) & WinAPI.FILE_ATTRIBUTE_REPARSE_POINT) != 0; + + uint error = WinAPI.GetSecurityInfo( + handle, + WinAPI.SE_FILE_OBJECT, + WinAPI.OWNER_SECURITY_INFORMATION, + out IntPtr owner, + IntPtr.Zero, + IntPtr.Zero, + IntPtr.Zero, + out IntPtr descriptor); + + if (error != 0) + { + throw new Win32Exception((int)error); + } + + try + { + return (new SecurityIdentifier(owner), isReparsePoint); + } + finally + { + WinAPI.LocalFree(descriptor); + } + } + + /// + /// Whether may own the agent's data: SYSTEM, Administrators, TrustedInstaller, or a member + /// of Administrators. + /// + /// + /// Membership is decided as the agent decides it for the owner of a pending-enrollment file + /// (owner_is_administrator_member in crates/agent-identity/src/windows.rs), so that the installer and the + /// agent accept the same owners: by the groups Authz resolves for the owner or, when Authz fails, by the local member + /// list of Administrators. + /// + /// Authz failed and the local member list of Administrators can't be read. + internal static bool IsTrustedOwner(SecurityIdentifier owner) + { + if (owner.IsWellKnown(WellKnownSidType.LocalSystemSid) || owner == Administrators || owner == TrustedInstaller) + { + return true; + } + + // UNTESTED: a member of Administrators, such as an administrator's user account, is trusted through the groups + // Authz resolves or, when Authz fails, as it may for a domain account, through the local member list. + // No test runs with such an account, and this keeps the installer accepting the owners the agent accepts. + // When changing this, keep it in line with owner_is_administrator_member in crates/agent-identity/src/windows.rs. + List groups; + + try + { + groups = AuthzGroups(owner); + } + catch (Exception) + { + return LocalAdministrators().Contains(owner); + } + + return groups.Contains(Administrators); + } + + /// + /// Deletes \[0]\…\[n-1] and everything + /// below it, without following any link. + /// + /// + /// Every name but the last must be a directory that is not a reparse point, or nothing is deleted. + /// A reparse point is deleted itself, never its target, and a directory is emptied first. + /// Each failure is logged and the deletion goes on with the next entry; nothing is thrown. + /// + internal static void DeleteTree(string root, string[] names, ILogger logger) + { + string target = Path.Combine([root, .. names]); + List handles = new(); + + try + { + string path = root; + + foreach (string name in names.Take(names.Length - 1)) + { + path = Path.Combine(path, name); + SafeFileHandle parent = Open(path, WinAPI.FILE_READ_ATTRIBUTES, HeldShare); + handles.Add(parent); + + if (!IsPlainDirectory(GetAttributes(parent))) + { + logger.Log($"kept {target}: {path} is a reparse point or not a directory"); + return; + } + } + + SafeFileHandle handle = Open(target, WinAPI.DELETE | WinAPI.FILE_READ_ATTRIBUTES, HeldShare); + handles.Add(handle); + Delete(handle, target, logger); + logger.Log($"deleted {target}"); + } + catch (Win32Exception e) when (IsNotFound(e)) + { + // Nothing to delete. + } + catch (Exception e) + { + logger.Log($"failed to delete {target}: {e}"); + } + finally + { + handles.ForEach(handle => handle.Dispose()); + } + } + + /// + /// Deletes the entry holds, after the entries below it when it is a directory that is not a + /// reparse point. + /// + /// + /// The entry goes once is closed. + /// + private static void Delete(SafeFileHandle handle, string path, ILogger logger) + { + if (IsPlainDirectory(GetAttributes(handle))) + { + foreach (string entry in Directory.GetFileSystemEntries(path)) + { + try + { + using SafeFileHandle child = Open(entry, WinAPI.DELETE | WinAPI.FILE_READ_ATTRIBUTES, HeldShare); + Delete(child, entry, logger); + } + catch (Win32Exception e) when (IsNotFound(e)) + { + // Already gone. + } + catch (Exception e) + { + logger.Log($"failed to delete {entry}: {e}"); + } + } + } + + WinAPI.FILE_DISPOSITION_INFO disposition = new() { DeleteFile = true }; + + if (!WinAPI.SetFileInformationByHandle( + handle, + WinAPI.FileDispositionInfo, + ref disposition, + (uint)Marshal.SizeOf())) + { + throw new Win32Exception(Marshal.GetLastWin32Error()); + } + } + + /// + /// Opens the entry names, a link itself rather than its target. + /// + private static SafeFileHandle Open(string path, uint access, uint share) + { + SafeFileHandle handle = WinAPI.CreateFile( + path, + access, + share, + IntPtr.Zero, + WinAPI.OPEN_EXISTING, + WinAPI.FILE_FLAG_BACKUP_SEMANTICS | WinAPI.FILE_FLAG_OPEN_REPARSE_POINT, + IntPtr.Zero); + + if (handle.IsInvalid) + { + int error = Marshal.GetLastWin32Error(); + handle.Dispose(); + throw new Win32Exception(error); + } + + return handle; + } + + private static uint GetAttributes(SafeFileHandle handle) + { + if (!WinAPI.GetFileInformationByHandle(handle, out WinAPI.BY_HANDLE_FILE_INFORMATION information)) + { + throw new Win32Exception(Marshal.GetLastWin32Error()); + } + + return information.dwFileAttributes; + } + + private static bool IsPlainDirectory(uint attributes) => + (attributes & WinAPI.FILE_ATTRIBUTE_DIRECTORY) != 0 && (attributes & WinAPI.FILE_ATTRIBUTE_REPARSE_POINT) == 0; + + private static bool IsNotFound(Win32Exception e) => + e.NativeErrorCode is WinAPI.ERROR_FILE_NOT_FOUND or WinAPI.ERROR_PATH_NOT_FOUND; + + /// + /// The groups Authz resolves for , nested and domain groups included. + /// + private static List AuthzGroups(SecurityIdentifier sid) + { + if (!WinAPI.AuthzInitializeResourceManager( + WinAPI.AUTHZ_RM_FLAG_NO_AUDIT, IntPtr.Zero, IntPtr.Zero, IntPtr.Zero, null, out IntPtr manager)) + { + throw new Win32Exception(Marshal.GetLastWin32Error()); + } + + try + { + byte[] binary = new byte[sid.BinaryLength]; + sid.GetBinaryForm(binary, 0); + + if (!WinAPI.AuthzInitializeContextFromSid( + 0, binary, manager, IntPtr.Zero, default, IntPtr.Zero, out IntPtr context)) + { + throw new Win32Exception(Marshal.GetLastWin32Error()); + } + + try + { + // The first call only reports the size the second needs. + WinAPI.AuthzGetInformationFromContext( + context, WinAPI.AuthzContextInfoGroupsSids, 0, out uint size, IntPtr.Zero); + int error = Marshal.GetLastWin32Error(); + int offset = Marshal.OffsetOf(nameof(WinAPI.TOKEN_GROUPS.Groups)).ToInt32(); + + if (size < offset) + { + throw new Win32Exception(error); + } + + IntPtr buffer = Marshal.AllocHGlobal((int)size); + + try + { + if (!WinAPI.AuthzGetInformationFromContext( + context, WinAPI.AuthzContextInfoGroupsSids, size, out size, buffer)) + { + throw new Win32Exception(Marshal.GetLastWin32Error()); + } + + int count = Marshal.ReadInt32(buffer); + int stride = Marshal.SizeOf(); + + if (count < 0 || offset + (long)count * stride > size) + { + throw new InvalidDataException("Authz returned a group list larger than its buffer"); + } + + return Enumerable.Range(0, count) + .Select(i => new SecurityIdentifier(Marshal.ReadIntPtr(buffer, offset + i * stride))) + .ToList(); + } + finally + { + Marshal.FreeHGlobal(buffer); + } + } + finally + { + WinAPI.AuthzFreeContext(context); + } + } + finally + { + WinAPI.AuthzFreeResourceManager(manager); + } + } + + /// + /// The direct members of the local Administrators group. + /// + private static List LocalAdministrators() + { + // NetLocalGroupGetMembers takes the group's name, which is localized, without its BUILTIN domain. + string name = Administrators.Translate(typeof(NTAccount)).Value; + name = name.Substring(name.LastIndexOf('\\') + 1); + + uint status = WinAPI.NetLocalGroupGetMembers( + null, name, 0, out IntPtr buffer, WinAPI.MAX_PREFERRED_LENGTH, out uint read, out _, IntPtr.Zero); + + if (status != WinAPI.NERR_Success) + { + throw new Win32Exception((int)status); + } + + try + { + // Level 0 returns LOCALGROUP_MEMBERS_INFO_0 entries, each a single SID pointer. + return Enumerable.Range(0, (int)read) + .Select(i => new SecurityIdentifier(Marshal.ReadIntPtr(buffer, i * IntPtr.Size))) + .ToList(); + } + finally + { + WinAPI.NetApiBufferFree(buffer); + } + } + + /// + /// A security descriptor converted from SDDL, freed on disposal. + /// + private sealed class SecurityDescriptor : IDisposable + { + internal IntPtr Pointer { get; } + + internal SecurityDescriptor(string sddl) + { + const uint sdRevision = 1; + + if (!WinAPI.ConvertStringSecurityDescriptorToSecurityDescriptorW(sddl, sdRevision, out IntPtr pointer, out _)) + { + throw new Win32Exception(Marshal.GetLastWin32Error()); + } + + this.Pointer = pointer; + } + + public void Dispose() => WinAPI.LocalFree(this.Pointer); + } +} diff --git a/package/AgentWindowsManaged/Actions/WinAPI.cs b/package/AgentWindowsManaged/Actions/WinAPI.cs index f9244070d..e32d7a26d 100644 --- a/package/AgentWindowsManaged/Actions/WinAPI.cs +++ b/package/AgentWindowsManaged/Actions/WinAPI.cs @@ -7,30 +7,68 @@ namespace DevolutionsAgent.Actions; internal static class WinAPI { + internal const uint AUTHZ_RM_FLAG_NO_AUDIT = 0x1; + + internal const int AuthzContextInfoGroupsSids = 2; + internal static uint CREATE_ALWAYS = 2; internal static uint CREATE_NO_WINDOW = 0x08000000; internal const uint DACL_SECURITY_INFORMATION = 0x00000004; + internal const uint DELETE = 0x00010000; + internal const int EM_SETCUEBANNER = 0x1501; + internal const int ERROR_ALREADY_EXISTS = 183; + + internal const int ERROR_FILE_NOT_FOUND = 2; + + internal const int ERROR_PATH_NOT_FOUND = 3; + + internal const uint FILE_ATTRIBUTE_DIRECTORY = 0x00000010; + internal static uint FILE_ATTRIBUTE_NORMAL = 0x00000080; + internal const uint FILE_ATTRIBUTE_REPARSE_POINT = 0x00000400; + + internal const uint FILE_FLAG_BACKUP_SEMANTICS = 0x02000000; + + internal const uint FILE_FLAG_OPEN_REPARSE_POINT = 0x00200000; + + internal const uint FILE_READ_ATTRIBUTES = 0x00000080; + + internal const uint FILE_SHARE_DELETE = 0x00000004; + internal static uint FILE_SHARE_READ = 0x00000001; internal static uint FILE_SHARE_WRITE = 0x00000002; + internal const int FileDispositionInfo = 4; + internal static uint GENERIC_WRITE = 0x40000000; + internal const uint MAX_PREFERRED_LENGTH = 0xFFFFFFFF; + internal static uint MOVEFILE_REPLACE_EXISTING = 0x1; internal static uint MOVEFILE_DELAY_UNTIL_REBOOT = 0x04; internal const uint NCRYPT_MACHINE_KEY_FLAG = 0x00000020; + internal const uint NERR_Success = 0; + internal const int NTE_NO_MORE_ITEMS = unchecked((int)0x8009002A); + internal const uint OPEN_EXISTING = 3; + + internal const uint OWNER_SECURITY_INFORMATION = 0x00000001; + + internal const uint READ_CONTROL = 0x00020000; + + internal const int SE_FILE_OBJECT = 1; + internal const uint SC_MANAGER_ALL_ACCESS = 0xF003F; internal const uint SC_MANAGER_CONNECT = 0x0001; @@ -61,6 +99,48 @@ internal static class WinAPI internal static uint WAIT_TIMEOUT = 0x00000102; + internal const uint WRITE_DAC = 0x00040000; + + internal const uint WRITE_OWNER = 0x00080000; + + [StructLayout(LayoutKind.Sequential)] + internal struct BY_HANDLE_FILE_INFORMATION + { + internal uint dwFileAttributes; + + internal System.Runtime.InteropServices.ComTypes.FILETIME ftCreationTime; + + internal System.Runtime.InteropServices.ComTypes.FILETIME ftLastAccessTime; + + internal System.Runtime.InteropServices.ComTypes.FILETIME ftLastWriteTime; + + internal uint dwVolumeSerialNumber; + + internal uint nFileSizeHigh; + + internal uint nFileSizeLow; + + internal uint nNumberOfLinks; + + internal uint nFileIndexHigh; + + internal uint nFileIndexLow; + } + + [StructLayout(LayoutKind.Sequential)] + internal struct FILE_DISPOSITION_INFO + { + [MarshalAs(UnmanagedType.U1)] internal bool DeleteFile; + } + + [StructLayout(LayoutKind.Sequential)] + internal struct LUID + { + internal uint LowPart; + + internal int HighPart; + } + [StructLayout(LayoutKind.Sequential)] internal struct QUERY_SERVICE_CONFIG { @@ -143,6 +223,14 @@ internal struct SERVICE_STATUS_PROCESS internal uint dwServiceFlags; } + [StructLayout(LayoutKind.Sequential)] + internal struct SID_AND_ATTRIBUTES + { + internal IntPtr Sid; + + internal uint Attributes; + } + [StructLayout(LayoutKind.Sequential)] internal struct STARTUPINFO { @@ -183,6 +271,53 @@ internal struct STARTUPINFO internal IntPtr hStdError; } + [StructLayout(LayoutKind.Sequential)] + internal struct TOKEN_GROUPS + { + internal uint GroupCount; + + // The first of GroupCount entries. + internal SID_AND_ATTRIBUTES Groups; + } + + [DllImport("authz")] + [return: MarshalAs(UnmanagedType.Bool)] + internal static extern bool AuthzFreeContext(IntPtr hAuthzClientContext); + + [DllImport("authz")] + [return: MarshalAs(UnmanagedType.Bool)] + internal static extern bool AuthzFreeResourceManager(IntPtr hAuthzResourceManager); + + [DllImport("authz", SetLastError = true)] + [return: MarshalAs(UnmanagedType.Bool)] + internal static extern bool AuthzGetInformationFromContext( + IntPtr hAuthzClientContext, + int InfoClass, + uint BufferSize, + out uint pSizeRequired, + IntPtr Buffer); + + [DllImport("authz", SetLastError = true)] + [return: MarshalAs(UnmanagedType.Bool)] + internal static extern bool AuthzInitializeContextFromSid( + uint Flags, + byte[] UserSid, + IntPtr hAuthzResourceManager, + IntPtr pExpirationTime, + LUID Identifier, + IntPtr DynamicGroupArgs, + out IntPtr phAuthzClientContext); + + [DllImport("authz", SetLastError = true, CharSet = CharSet.Unicode)] + [return: MarshalAs(UnmanagedType.Bool)] + internal static extern bool AuthzInitializeResourceManager( + uint Flags, + IntPtr pfnDynamicAccessCheck, + IntPtr pfnComputeDynamicGroups, + IntPtr pfnFreeDynamicGroups, + string szResourceManagerName, + out IntPtr phAuthzResourceManager); + [DllImport("advapi32", EntryPoint = "ChangeServiceConfigW", SetLastError = true)] internal static extern bool ChangeServiceConfig( IntPtr hService, @@ -211,6 +346,10 @@ internal static extern bool ChangeServiceConfig( [return: MarshalAs(UnmanagedType.Bool)] public static extern bool ControlService(IntPtr hService, uint dwControl, IntPtr lpServiceStatus); + [DllImport("kernel32", EntryPoint = "CreateDirectoryW", CharSet = CharSet.Unicode, SetLastError = true)] + [return: MarshalAs(UnmanagedType.Bool)] + internal static extern bool CreateDirectory(string lpPathName, ref SECURITY_ATTRIBUTES lpSecurityAttributes); + [DllImport("kernel32", EntryPoint = "CreateFileW", CharSet = CharSet.Unicode, SetLastError = true)] internal static extern SafeFileHandle CreateFile( [MarshalAs(UnmanagedType.LPWStr)] string lpFileName, @@ -245,6 +384,12 @@ internal static extern bool DeleteFile( [return: MarshalAs(UnmanagedType.Bool)] internal static extern bool GetExitCodeProcess(IntPtr hProcess, out uint lpExitCode); + [DllImport("kernel32", SetLastError = true)] + [return: MarshalAs(UnmanagedType.Bool)] + internal static extern bool GetFileInformationByHandle( + SafeFileHandle hFile, + out BY_HANDLE_FILE_INFORMATION lpFileInformation); + [DllImport("Kernel32", EntryPoint = "GetFinalPathNameByHandleW", CharSet = CharSet.Auto, SetLastError = true)] internal static extern uint GetFinalPathNameByHandle( IntPtr hFile, @@ -252,6 +397,17 @@ internal static extern uint GetFinalPathNameByHandle( uint cchFilePath, uint dwFlags); + [DllImport("advapi32")] + internal static extern uint GetSecurityInfo( + SafeFileHandle handle, + int ObjectType, + uint SecurityInfo, + out IntPtr ppsidOwner, + IntPtr ppsidGroup, + IntPtr ppDacl, + IntPtr ppSacl, + out IntPtr ppSecurityDescriptor); + [DllImport("kernel32", SetLastError = true)] internal static extern IntPtr GetStdHandle(int nStdHandle); @@ -273,6 +429,20 @@ internal static extern bool MoveFileEx( uint dwFlags ); + [DllImport("netapi32")] + internal static extern uint NetApiBufferFree(IntPtr Buffer); + + [DllImport("netapi32", CharSet = CharSet.Unicode)] + internal static extern uint NetLocalGroupGetMembers( + string servername, + string localgroupname, + uint level, + out IntPtr bufptr, + uint prefmaxlen, + out uint entriesread, + out uint totalentries, + IntPtr resumehandle); + [DllImport("ncrypt", CharSet = CharSet.Unicode)] internal static extern int NCryptEnumKeys( IntPtr hProvider, @@ -315,9 +485,24 @@ internal static extern int SendMessage( int wParam, [MarshalAs(UnmanagedType.LPWStr)] string lParam); + [DllImport("kernel32", SetLastError = true)] + [return: MarshalAs(UnmanagedType.Bool)] + internal static extern bool SetFileInformationByHandle( + SafeFileHandle hFile, + int FileInformationClass, + ref FILE_DISPOSITION_INFO lpFileInformation, + uint dwBufferSize); + [DllImport("advapi32", SetLastError = true, CharSet = CharSet.Unicode)] internal static extern bool SetFileSecurityW(string lpFileName, uint SecurityInformation, IntPtr pSecurityDescriptor); + [DllImport("advapi32", SetLastError = true)] + [return: MarshalAs(UnmanagedType.Bool)] + internal static extern bool SetKernelObjectSecurity( + SafeFileHandle Handle, + uint SecurityInformation, + IntPtr SecurityDescriptor); + [DllImport("advapi32", EntryPoint = "StartServiceW", SetLastError = true)] internal static extern bool StartService(IntPtr hService, uint dwNumServiceArgs, IntPtr lpServiceArgVectors); From 47181237d765904d81ca4d3cecd5251f7386ee17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Tue, 27 Oct 2026 22:57:29 +0900 Subject: [PATCH 3/9] fix(agent-installer): refuse an untrusted agent data folder An install, upgrade, repair or maintenance run of the Agent installer now fails, before it changes anything, when %ProgramData%\Devolutions or %ProgramData%\Devolutions\Agent is a link, has an owner other than SYSTEM, TrustedInstaller or an administrator, or, for Devolutions, has a DACL that lets another account delete, rename or replace it or what it contains. The installer writes the agent's data there as SYSTEM, so a folder another account controls could redirect or replace what it writes. A folder whose owner or DACL can't be read is refused too. The error names the folder and the account at fault, in the installer's language, and a silent install logs it. The check runs before RemoveExistingProducts, and again when the Agent folder is created, under a handle that keeps Devolutions from being renamed or replaced until Agent is created or checked inside it. A full uninstall is never refused, so a machine can always be uninstalled. Issue: DGW-478 Co-Authored-By: Claude Opus 5.5 --- .../AgentIdentityTests.cs | 67 +--- .../DataFolderTests.cs | 297 ++++++++++++++++++ .../InstallerSession.cs | 82 +++++ .../AgentWindowsManaged.Tests/OutputLogger.cs | 15 + .../SecureDirectoryTests.cs | 107 +------ .../AgentWindowsManaged.Tests/TempFolder.cs | 66 ++++ .../Actions/AgentActions.cs | 35 ++- .../Actions/CustomActions.cs | 45 ++- .../Actions/DataFolders.cs | 291 +++++++++++++++++ .../Actions/SecureDirectory.cs | 55 ++-- package/AgentWindowsManaged/Actions/WinAPI.cs | 24 +- .../Resources/DevolutionsAgent_en-us.wxl | 4 + .../Resources/DevolutionsAgent_fr-fr.wxl | 4 + .../Resources/DevolutionsAgent_sv-se.wxl | 4 + .../Resources/Strings.g.cs | 16 + .../Resources/Strings_en-US.json | 16 + .../Resources/Strings_fr-FR.json | 16 + .../Resources/Strings_sv-SE.json | 16 + 18 files changed, 961 insertions(+), 199 deletions(-) create mode 100644 package/AgentWindowsManaged.Tests/DataFolderTests.cs create mode 100644 package/AgentWindowsManaged.Tests/InstallerSession.cs create mode 100644 package/AgentWindowsManaged.Tests/OutputLogger.cs create mode 100644 package/AgentWindowsManaged.Tests/TempFolder.cs create mode 100644 package/AgentWindowsManaged/Actions/DataFolders.cs diff --git a/package/AgentWindowsManaged.Tests/AgentIdentityTests.cs b/package/AgentWindowsManaged.Tests/AgentIdentityTests.cs index 97154eeee..a14b0a9f6 100644 --- a/package/AgentWindowsManaged.Tests/AgentIdentityTests.cs +++ b/package/AgentWindowsManaged.Tests/AgentIdentityTests.cs @@ -17,8 +17,11 @@ using Property = WixSharp.Property; using Record = Microsoft.Deployment.WindowsInstaller.Record; +using static DevolutionsAgent.Installer.Tests.InstallerSession; + namespace DevolutionsAgent.Installer.Tests; +[Collection(InstallerSession.Collection)] public sealed class AgentIdentityTests { // A well-formed token (crates/agent-identity/src/token.rs) whose bag names https://example.test/dvls. @@ -410,12 +413,6 @@ string ErrorIn(string productLanguage) Assert.Equal(english, ErrorIn("1031")); } - /// - /// The installer's action that runs the custom action . - /// - private static ManagedAction ActionOf(string methodName) => - Assert.Single(AgentActions.Actions.OfType(), a => a.MethodName == methodName); - /// /// Runs the custom action that reads the enrollment token file, in a session of a package in /// where (NAME=value) are set. @@ -454,62 +451,4 @@ private static (ActionResult Result, string Token, List<(InstallMessage Type, st MsiInstaller.SetExternalUI(previous, InstallLogModes.None); } } - - /// - /// Evaluates with the Windows Installer engine, in a session where - /// (NAME=value) are set. - /// - private static bool EvaluateCondition(string condition, string[] properties) => - WithSession(properties, session => session.EvaluateCondition(condition)); - - private static T WithSession(string[] properties, Func action) => - WithSession(AppContext.BaseDirectory, properties, action); - - /// - /// Runs in a Windows Installer session of an empty package in - /// where (NAME=value) are set. - /// - private static T WithSession(string folder, string[] properties, Func action) - { - string path = Path.Combine(folder, $"session-{Guid.NewGuid():N}.msi"); - - try - { - using (Database database = new(path, DatabaseOpenMode.CreateDirect)) - { - database.Execute("CREATE TABLE `Property` (`Property` CHAR(72) NOT NULL, `Value` CHAR(0) NOT NULL LOCALIZABLE PRIMARY KEY `Property`)"); - database.Execute($"INSERT INTO `Property` (`Property`, `Value`) VALUES ('ProductCode', '{Guid.NewGuid():B}')"); - database.SummaryInfo.Template = "x64;1033"; - database.SummaryInfo.RevisionNumber = Guid.NewGuid().ToString("B"); - database.SummaryInfo.PageCount = 500; - database.Commit(); - } - - using Session session = MsiInstaller.OpenPackage(path, ignoreMachineState: true); - - foreach (string property in properties) - { - string[] parts = property.Split(['='], 2); - session[parts[0]] = parts[1]; - } - - return action(session); - } - finally - { - System.IO.File.Delete(path); - } - } - - /// - /// A new, empty folder under the temporary directory, deleted with its content on disposal. - /// - private sealed class TempFolder : IDisposable - { - public string Path { get; } = System.IO.Path.Combine(System.IO.Path.GetTempPath(), $"agent-identity-{Guid.NewGuid():N}"); - - public TempFolder() => Directory.CreateDirectory(Path); - - public void Dispose() => Directory.Delete(Path, recursive: true); - } } diff --git a/package/AgentWindowsManaged.Tests/DataFolderTests.cs b/package/AgentWindowsManaged.Tests/DataFolderTests.cs new file mode 100644 index 000000000..8f607050e --- /dev/null +++ b/package/AgentWindowsManaged.Tests/DataFolderTests.cs @@ -0,0 +1,297 @@ +using System.IO; +using System.Linq; +using System.Security.AccessControl; +using System.Security.Principal; +using System.Text.RegularExpressions; + +using DevolutionsAgent.Actions; +using DevolutionsAgent.Resources; + +using WixSharp; + +using Xunit; +using Xunit.Abstractions; + +using static DevolutionsAgent.Installer.Tests.InstallerSession; + +namespace DevolutionsAgent.Installer.Tests; + +[Collection(InstallerSession.Collection)] +public sealed class DataFolderTests +{ + // Values of Evaluate's checkAccess, which only Devolutions gets. + private const bool Devolutions = true; + private const bool Agent = false; + + // The DACL of a Devolutions folder that SYSTEM created in ProgramData, as read on Windows 11: inherited full control + // for SYSTEM and Administrators, an inherit-only CREATOR OWNER entry, read access for Users, and the Users entry that + // only adds files and subdirectories. + private const string InheritedDacl = + "D:AI(A;OICIID;FA;;;SY)(A;OICIID;FA;;;BA)(A;OICIIOID;GA;;;CO)(A;OICIID;0x1200a9;;;BU)(A;CIID;DCLCRPCR;;;BU)"; + + private const string Users = "S-1-5-32-545"; + + private const string Everyone = "S-1-1-0"; + + private static readonly SecurityIdentifier CurrentUser = WindowsIdentity.GetCurrent().User; + + // Protected, so that the folder inherits nothing from the temporary directory, which grants the current user full + // control, and owned by the current user, who can't make another account the owner without elevation. + private static readonly string ReplaceableByUsersSddl = + $"O:{CurrentUser}D:P(A;OICI;FA;;;SY)(A;OICI;FA;;;BA)(A;OICI;0x1200a9;;;BU)(A;;SD;;;BU)"; + + private static readonly string AgentSddl = $"O:{CurrentUser}D:P(A;OICI;FA;;;{CurrentUser})(A;OICI;FA;;;SY)"; + + private static readonly string[] DataFolderActions = + [ + nameof(CustomActions.CheckProgramDataDirectory), + nameof(CustomActions.CreateProgramDataDirectory), + ]; + + private readonly ILogger logger; + + public DataFolderTests(ITestOutputHelper output) => this.logger = new OutputLogger(output); + + [Theory] + // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + [InlineData("SYSTEM owns Devolutions", Devolutions, "O:SY" + InheritedDacl, "none")] + // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + [InlineData("Administrators own Devolutions", Devolutions, "O:BA" + InheritedDacl, "none")] + // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + [InlineData("Users own Devolutions", Devolutions, "O:BU" + InheritedDacl, "Owner " + Users)] + // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + [InlineData("Everyone owns Devolutions", Devolutions, "O:WD" + InheritedDacl, "Owner " + Everyone)] + // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + [InlineData("Users own Agent", Agent, "O:BU" + InheritedDacl, "Owner " + Users)] + // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + [InlineData("Users may delete Devolutions", Devolutions, "O:SYD:(A;;SD;;;BU)", "Access " + Users)] + // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + [InlineData("Users may change the DACL", Devolutions, "O:SYD:(A;;WD;;;BU)", "Access " + Users)] + // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + [InlineData("Users may take ownership", Devolutions, "O:SYD:(A;;WO;;;BU)", "Access " + Users)] + // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + [InlineData("Users may delete entries", Devolutions, "O:SYD:(A;;0x40;;;BU)", "Access " + Users)] + // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + [InlineData("Users have generic all", Devolutions, "O:SYD:(A;;GA;;;BU)", "Access " + Users)] + // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + [InlineData("Users may delete through a conditional entry", Devolutions, "O:SYD:(XA;;SD;;;BU;(Member_of {SID(BU)}))", "Access " + Users)] + // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + [InlineData("null DACL", Devolutions, "O:SYD:NO_ACCESS_CONTROL", "Access " + Everyone)] + // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + [InlineData("Users get full control of entries created below", Devolutions, "O:SYD:(A;OICIIO;FA;;;BU)", "none")] + // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + [InlineData("Users may only add entries", Devolutions, "O:SYD:(A;CI;DCLCRPCR;;;BU)", "none")] + // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + [InlineData("Users are denied deletion", Devolutions, "O:SYD:(D;;SD;;;BU)", "none")] + // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + [InlineData("TrustedInstaller has full control", Devolutions, "O:SYD:(A;;FA;;;S-1-5-80-956008885-3418522649-1831038044-1853292631-2271478464)", "none")] + // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + [InlineData("no owner", Devolutions, "D:(A;;FA;;;SY)", "Unreadable")] + // oracle: scaffold + [InlineData("Users may delete through an object entry", Devolutions, "O:SYD:(OA;;SD;bf967aba-0de6-11d0-a285-00aa003049e2;;BU)", "Access " + Users)] + // oracle: scaffold + [InlineData("Users have full control of Agent", Agent, "O:SYD:(A;OICI;FA;;;BU)", "none")] + public void RefusalFollowsTheOwnerAndTheDacl(string scenario, bool checkAccess, string sddl, string expected) + { + DataFolderRefusal refusal = DataFolders.Evaluate("folder", new RawSecurityDescriptor(sddl), false, checkAccess); + + Assert.True(expected == Describe(refusal), $"{scenario}: {Describe(refusal)}"); + } + + // oracle: reference crates/now-package-broker/src/policy_security.rs retain_policy_no_reparse_directory_chain + [Theory] + [InlineData(Devolutions)] + [InlineData(Agent)] + public void LinkIsRefusedWhateverItsSecurity(bool checkAccess) + { + DataFolderRefusal refusal = DataFolders.Evaluate( + "folder", new RawSecurityDescriptor("O:SY" + InheritedDacl), true, checkAccess); + + Assert.Equal("Link", Describe(refusal)); + } + + // oracle: reference crates/now-package-broker/src/policy_security.rs retain_policy_no_reparse_directory_chain + [Fact] + public void CheckRefusesAJunctionAtDevolutions() + { + using TempFolder folder = new(); + string devolutions = Path.Combine(folder.Path, "Devolutions"); + TempFolder.CreateJunction(devolutions, folder.CreateDirectory("target")); + + DataFolderRefusal refusal = DataFolders.Check(folder.Path, this.logger); + + Assert.Equal((devolutions, "Link"), (refusal?.Folder, Describe(refusal))); + } + + [Fact] + public void CreateRefusesAJunctionAtDevolutionsAndCreatesNothingInItsTarget() + { + using TempFolder folder = new(); + string target = folder.CreateDirectory("target"); + string devolutions = Path.Combine(folder.Path, "Devolutions"); + TempFolder.CreateJunction(devolutions, target); + + DataFolderRefusal refusal = DataFolders.Create(folder.Path, AgentSddl, this.logger); + + // oracle: reference crates/now-package-broker/src/policy_security.rs retain_policy_no_reparse_directory_chain + Assert.Equal((devolutions, "Link"), (refusal?.Folder, Describe(refusal))); + // oracle: scaffold + Assert.Empty(Directory.GetFileSystemEntries(target)); + } + + // oracle: scaffold + [Fact] + public void CheckRefusesADevolutionsFolderWhoseEntriesUsersMayDelete() + { + using TempFolder folder = new(); + string devolutions = Path.Combine(folder.Path, "Devolutions"); + Assert.True(SecureDirectory.Create(devolutions, ReplaceableByUsersSddl)); + + DataFolderRefusal refusal = DataFolders.Check(folder.Path, this.logger); + + Assert.Equal((devolutions, "Access " + Users), (refusal?.Folder, Describe(refusal))); + } + + // oracle: scaffold + [Fact] + public void CreateRefusesADevolutionsFolderWhoseEntriesUsersMayDeleteAndCreatesNothingInIt() + { + using TempFolder folder = new(); + string devolutions = Path.Combine(folder.Path, "Devolutions"); + Assert.True(SecureDirectory.Create(devolutions, ReplaceableByUsersSddl)); + + DataFolderRefusal refusal = DataFolders.Create(folder.Path, AgentSddl, this.logger); + + Assert.Equal((devolutions, "Access " + Users), (refusal?.Folder, Describe(refusal))); + Assert.Empty(Directory.GetFileSystemEntries(devolutions)); + } + + // oracle: scaffold + [Fact] + public void CheckRefusesADevolutionsFolderWhoseSecurityItCantRead() + { + using TempFolder folder = new(); + string devolutions = Path.Combine(folder.Path, "Devolutions"); + // The OWNER RIGHTS entry takes away the owner's implicit right to read the security descriptor, and the current + // user keeps only the rights to list the folder, read its attributes and synchronize, which deleting it needs. + Assert.True(SecureDirectory.Create(devolutions, $"O:{CurrentUser}D:P(A;;0;;;OW)(A;OICI;FA;;;SY)(A;;0x100081;;;{CurrentUser})")); + + DataFolderRefusal refusal = DataFolders.Check(folder.Path, this.logger); + + Assert.Equal((devolutions, "Unreadable"), (refusal?.Folder, Describe(refusal))); + } + + // oracle: scaffold + [Fact] + public void CheckPassesWhenTheFoldersDontExist() + { + using TempFolder folder = new(); + + Assert.Null(DataFolders.Check(folder.Path, this.logger)); + } + + // oracle: scaffold + [Fact] + public void ErrorNamesTheFolderAndTheAccountOrItsSidWhenItHasNone() + { + const string Folder = @"C:\ProgramData\Devolutions"; + const string Unnamed = "S-1-5-21-1-2-3-1001"; + SecurityIdentifier users = new(Users); + + string owned = new DataFolderRefusal(Folder, DataFolderProblem.Owner, users).Message("1033"); + string replaceable = new DataFolderRefusal(Folder, DataFolderProblem.Access, new SecurityIdentifier(Unnamed)).Message("1033"); + + Assert.Contains(Folder, owned); + Assert.Contains(users.Translate(typeof(NTAccount)).Value, owned); + Assert.Contains(Folder, replaceable); + Assert.Contains(Unnamed, replaceable); + } + + // oracle: scaffold + [Fact] + public void ErrorIsInTheInstallerLanguage() + { + DataFolderRefusal refusal = new(@"C:\ProgramData\Devolutions", DataFolderProblem.Link); + string english = refusal.Message("1033"); + + Assert.NotEqual(english, refusal.Message("1036")); + Assert.NotEqual(english, refusal.Message("1053")); + // Windows Installer applies no language transform for German, so the rest of the installer is in English too. + Assert.Equal(english, refusal.Message("1031")); + } + + // oracle: spec .agents/skills/installer-localization/SKILL.md §Change strings + [Theory] + [InlineData(Strings.DataFolderIsLink)] + [InlineData(Strings.DataFolderOwnedByUntrustedAccount)] + [InlineData(Strings.DataFolderReplaceableByUntrustedAccount)] + [InlineData(Strings.DataFolderSecurityUnreadable)] + public void ErrorIsTranslatedWithTheEnglishPlaceholders(string id) + { + string english = Program.Localize("1033", id); + + Assert.NotEqual(id, english); + Assert.All(new[] { "1036", "1053" }, language => + { + string translated = Program.Localize(language, id); + Assert.NotEqual(id, translated); + Assert.NotEqual(english, translated); + Assert.Equal(Placeholders(english), Placeholders(translated)); + }); + } + + [Theory] + [InlineData(nameof(CustomActions.CheckProgramDataDirectory))] + [InlineData(nameof(CustomActions.CreateProgramDataDirectory))] + public void RefusalFailsTheRunInTheInstallerLanguage(string method) + { + ManagedAction action = ActionOf(method); + + // oracle: scaffold + Assert.Equal(Return.check, action.Return); + // A deferred action reads only the properties its action data carries. + // oracle: spec Windows Installer §Obtaining Context Information for Deferred Execution Custom Actions + Assert.Contains("ProductLanguage=[ProductLanguage]", action.UsesProperties.Split(';')); + } + + [Theory] + // oracle: scaffold + [InlineData("first install", true)] + // oracle: scaffold + [InlineData("major upgrade", true, "WIX_UPGRADE_DETECTED={00000000-0000-0000-0000-000000000001}")] + // oracle: scaffold + [InlineData("repair", true, "Installed=00:00:00", "REINSTALL=ALL")] + // oracle: scaffold + [InlineData("maintenance adding a feature", true, "Installed=00:00:00", "ADDLOCAL=F.Session")] + // oracle: scaffold + [InlineData("maintenance removing a feature", true, "Installed=00:00:00", "REMOVE=F.Session")] + // oracle: intent package/AgentWindowsManaged/INTENT.md §Agent Identity + [InlineData("full uninstall", false, "Installed=00:00:00", "REMOVE=ALL")] + // oracle: intent package/AgentWindowsManaged/INTENT.md §Agent Identity + [InlineData("old product removed by a major upgrade", false, "Installed=00:00:00", "REMOVE=ALL", "UPGRADINGPRODUCTCODE={00000000-0000-0000-0000-000000000002}")] + public void DataFoldersAreCheckedOnEveryRunButAFullUninstall(string scenario, bool expected, params string[] properties) + { + bool[] evaluated = DataFolderActions + .Select(method => EvaluateCondition(ActionOf(method).Condition.ToString(), properties)) + .ToArray(); + + Assert.True(evaluated.All(e => e == expected), $"{scenario}: {string.Join(", ", evaluated)}"); + } + + // oracle: scaffold + [Fact] + public void CheckRunsBeforeTheMajorUpgradeRemovesTheInstalledProduct() + { + Assert.Equal(When.Before, AgentActions.checkProgramDataDirectory.When); + Assert.Equal(Step.RemoveExistingProducts.ToString(), AgentActions.checkProgramDataDirectory.Step.ToString()); + } + + /// + /// The problem and the account names, or none. + /// + private static string Describe(DataFolderRefusal refusal) => + refusal is null ? "none" : $"{refusal.Problem} {refusal.Account}".TrimEnd(); + + private static string[] Placeholders(string text) => + Regex.Matches(text, @"\{\d+\}").Cast().Select(m => m.Value).OrderBy(p => p).ToArray(); +} diff --git a/package/AgentWindowsManaged.Tests/InstallerSession.cs b/package/AgentWindowsManaged.Tests/InstallerSession.cs new file mode 100644 index 000000000..72e7e5fc7 --- /dev/null +++ b/package/AgentWindowsManaged.Tests/InstallerSession.cs @@ -0,0 +1,82 @@ +using System; +using System.IO; +using System.Linq; + +using DevolutionsAgent.Actions; + +using Microsoft.Deployment.WindowsInstaller; + +using WixSharp; + +using Xunit; + +using MsiInstaller = Microsoft.Deployment.WindowsInstaller.Installer; + +namespace DevolutionsAgent.Installer.Tests; + +/// +/// The installer's actions, and Windows Installer sessions to evaluate them in. +/// +internal static class InstallerSession +{ + /// + /// The test collection of every test class that opens a session. + /// + /// + /// Windows Installer opens one package at a time in a process, and xUnit runs the tests of one collection one at a + /// time. + /// + public const string Collection = "Windows Installer session"; + + /// + /// The installer's action that runs the custom action . + /// + public static ManagedAction ActionOf(string methodName) => + Assert.Single(AgentActions.Actions.OfType(), a => a.MethodName == methodName); + + /// + /// Evaluates with the Windows Installer engine, in a session where + /// (NAME=value) are set. + /// + public static bool EvaluateCondition(string condition, string[] properties) => + WithSession(properties, session => session.EvaluateCondition(condition)); + + public static T WithSession(string[] properties, Func action) => + WithSession(AppContext.BaseDirectory, properties, action); + + /// + /// Runs in a Windows Installer session of an empty package in + /// where (NAME=value) are set. + /// + public static T WithSession(string folder, string[] properties, Func action) + { + string path = Path.Combine(folder, $"session-{Guid.NewGuid():N}.msi"); + + try + { + using (Database database = new(path, DatabaseOpenMode.CreateDirect)) + { + database.Execute("CREATE TABLE `Property` (`Property` CHAR(72) NOT NULL, `Value` CHAR(0) NOT NULL LOCALIZABLE PRIMARY KEY `Property`)"); + database.Execute($"INSERT INTO `Property` (`Property`, `Value`) VALUES ('ProductCode', '{Guid.NewGuid():B}')"); + database.SummaryInfo.Template = "x64;1033"; + database.SummaryInfo.RevisionNumber = Guid.NewGuid().ToString("B"); + database.SummaryInfo.PageCount = 500; + database.Commit(); + } + + using Session session = MsiInstaller.OpenPackage(path, ignoreMachineState: true); + + foreach (string property in properties) + { + string[] parts = property.Split(['='], 2); + session[parts[0]] = parts[1]; + } + + return action(session); + } + finally + { + System.IO.File.Delete(path); + } + } +} diff --git a/package/AgentWindowsManaged.Tests/OutputLogger.cs b/package/AgentWindowsManaged.Tests/OutputLogger.cs new file mode 100644 index 000000000..1b8b73e61 --- /dev/null +++ b/package/AgentWindowsManaged.Tests/OutputLogger.cs @@ -0,0 +1,15 @@ +using DevolutionsAgent.Actions; + +using Xunit.Abstractions; + +namespace DevolutionsAgent.Installer.Tests; + +/// +/// Writes the log of the code under test to the test's output. +/// +internal sealed class OutputLogger(ITestOutputHelper output) : ILogger +{ + public void Log(string msg) => output.WriteLine(msg); + + public void Log(string format, params object[] args) => output.WriteLine(format, args); +} diff --git a/package/AgentWindowsManaged.Tests/SecureDirectoryTests.cs b/package/AgentWindowsManaged.Tests/SecureDirectoryTests.cs index 93a144f4e..c6cbc8940 100644 --- a/package/AgentWindowsManaged.Tests/SecureDirectoryTests.cs +++ b/package/AgentWindowsManaged.Tests/SecureDirectoryTests.cs @@ -1,5 +1,4 @@ using System; -using System.Diagnostics; using System.IO; using System.Linq; using System.Security.AccessControl; @@ -7,6 +6,8 @@ using DevolutionsAgent.Actions; +using Microsoft.Win32.SafeHandles; + using Xunit; using Xunit.Abstractions; @@ -38,7 +39,7 @@ public void DeleteTreeKeepsWhatAJunctionInsideTheTreePointsTo() string identity = folder.CreateDirectory(IdentityNames); string pending = Directory.CreateDirectory(Path.Combine(identity, "pending")).FullName; File.WriteAllText(Path.Combine(pending, "token.dat"), "deleted"); - CreateJunction(Path.Combine(pending, "link"), outside); + TempFolder.CreateJunction(Path.Combine(pending, "link"), outside); SecureDirectory.DeleteTree(folder.Path, IdentityNames, this.logger); @@ -55,7 +56,7 @@ public void DeleteTreeDeletesAJunctionAtThePathButNotItsTarget() File.WriteAllText(Path.Combine(outside, "file.txt"), "kept"); folder.CreateDirectory("Devolutions", "Agent"); string identity = Path.Combine(folder.Path, "Devolutions", "Agent", "identity"); - CreateJunction(identity, outside); + TempFolder.CreateJunction(identity, outside); SecureDirectory.DeleteTree(folder.Path, IdentityNames, this.logger); @@ -63,7 +64,7 @@ public void DeleteTreeDeletesAJunctionAtThePathButNotItsTarget() Assert.True(File.Exists(Path.Combine(outside, "file.txt"))); } - // oracle: mutant the deletion follows a junction at the data directory into its target + // oracle: intent package/AgentWindowsManaged/INTENT.md §Agent Identity [Fact] public void DeleteTreeDeletesNothingThroughAJunctionAboveThePath() { @@ -72,14 +73,14 @@ public void DeleteTreeDeletesNothingThroughAJunctionAboveThePath() string outsideIdentity = folder.CreateDirectory("outside", "identity"); File.WriteAllText(Path.Combine(outsideIdentity, "file.txt"), "kept"); folder.CreateDirectory("Devolutions"); - CreateJunction(Path.Combine(folder.Path, "Devolutions", "Agent"), outside); + TempFolder.CreateJunction(Path.Combine(folder.Path, "Devolutions", "Agent"), outside); SecureDirectory.DeleteTree(folder.Path, IdentityNames, this.logger); Assert.True(File.Exists(Path.Combine(outsideIdentity, "file.txt"))); } - // oracle: mutant the security is applied through a junction to its target + // oracle: scaffold [Fact] public void SetSecurityRefusesAJunctionAndLeavesItsTargetUnchanged() { @@ -87,14 +88,14 @@ public void SetSecurityRefusesAJunctionAndLeavesItsTargetUnchanged() string target = folder.CreateDirectory("target"); string before = Sddl(target); string link = Path.Combine(folder.Path, "link"); - CreateJunction(link, target); + TempFolder.CreateJunction(link, target); Assert.Throws(() => SecureDirectory.SetSecurity(link, ProtectedSddl)); Assert.Equal(before, Sddl(target)); } - // oracle: mutant the owner or the DACL is left out, or the DACL loses its protection + // oracle: property round-trip [Fact] public void SetSecurityAppliesTheOwnerAndTheProtectedDacl() { @@ -106,7 +107,7 @@ public void SetSecurityAppliesTheOwnerAndTheProtectedDacl() AssertProtectedSecurity(path); } - // oracle: mutant inheritable entries are propagated to existing children + // oracle: scaffold [Fact] public void SetSecurityLeavesExistingChildrenUnchanged() { @@ -120,7 +121,7 @@ public void SetSecurityLeavesExistingChildrenUnchanged() Assert.Equal(before, Sddl(child)); } - // oracle: mutant the directory is created with the DACL inherited from its parent + // oracle: property round-trip [Fact] public void CreateGivesTheNewDirectoryExactlyTheSecurityDescriptor() { @@ -150,28 +151,18 @@ public void OwnerIsTrustedOnlyWhenPrivileged(string name, string sid, bool expec Assert.True(expected == SecureDirectory.IsTrustedOwner(new SecurityIdentifier(sid)), name); } - // oracle: mutant the owner is read from the target of a junction - [Fact] - public void ReadOwnerReportsAJunctionItself() - { - using TempFolder folder = new(); - string link = Path.Combine(folder.Path, "link"); - CreateJunction(link, folder.CreateDirectory("target")); - - Assert.True(SecureDirectory.ReadOwner(link).IsReparsePoint); - } - - // oracle: mutant the owner is not read from the directory + // oracle: property round-trip [Fact] - public void ReadOwnerReadsTheOwnerOfADirectory() + public void ReadSecurityReadsTheOwnerAndTheDaclThatSetSecurityApplied() { using TempFolder folder = new(); string path = folder.CreateDirectory("data"); SecureDirectory.SetSecurity(path, ProtectedSddl); - (SecurityIdentifier owner, bool isReparsePoint) = SecureDirectory.ReadOwner(path); + using SafeFileHandle handle = SecureDirectory.OpenToRead(path, hold: false); + (RawSecurityDescriptor security, bool isReparsePoint) = SecureDirectory.ReadSecurity(handle); - Assert.Equal(CurrentUser, owner); + Assert.Equal(ProtectedSddl, security.GetSddlForm(AccessControlSections.Owner | AccessControlSections.Access)); Assert.False(isReparsePoint); } @@ -206,70 +197,4 @@ private static string Sddl(string path) => new DirectoryInfo(path) .GetAccessControl(AccessControlSections.Owner | AccessControlSections.Access) .GetSecurityDescriptorSddlForm(AccessControlSections.Owner | AccessControlSections.Access); - - /// - /// Creates a junction at to the directory , which needs no - /// elevation. - /// - private static void CreateJunction(string link, string target) - { - ProcessStartInfo startInfo = new("cmd.exe", $"/c mklink /J \"{link}\" \"{target}\"") - { - UseShellExecute = false, - CreateNoWindow = true, - RedirectStandardOutput = true, - RedirectStandardError = true, - }; - - using Process process = Process.Start(startInfo); - string output = process.StandardOutput.ReadToEnd() + process.StandardError.ReadToEnd(); - process.WaitForExit(); - - Assert.True(process.ExitCode == 0, output); - } - - private sealed class OutputLogger(ITestOutputHelper output) : ILogger - { - public void Log(string msg) => output.WriteLine(msg); - - public void Log(string format, params object[] args) => output.WriteLine(format, args); - } - - /// - /// A new, empty folder under the temporary directory, deleted with its content on disposal. - /// - private sealed class TempFolder : IDisposable - { - public string Path { get; } = System.IO.Path.Combine(System.IO.Path.GetTempPath(), $"secure-directory-{Guid.NewGuid():N}"); - - public TempFolder() => Directory.CreateDirectory(this.Path); - - /// - /// Creates the directory below this folder, with its parents. - /// - public string CreateDirectory(params string[] names) => - Directory.CreateDirectory(System.IO.Path.Combine([this.Path, .. names])).FullName; - - public void Dispose() - { - // A recursive Directory.Delete fails on a junction without elevation, so junctions go first. - RemoveJunctions(this.Path); - Directory.Delete(this.Path, recursive: true); - } - - private static void RemoveJunctions(string directory) - { - foreach (string entry in Directory.GetDirectories(directory)) - { - if (File.GetAttributes(entry).HasFlag(FileAttributes.ReparsePoint)) - { - Directory.Delete(entry); - } - else - { - RemoveJunctions(entry); - } - } - } - } } diff --git a/package/AgentWindowsManaged.Tests/TempFolder.cs b/package/AgentWindowsManaged.Tests/TempFolder.cs new file mode 100644 index 000000000..d43a21625 --- /dev/null +++ b/package/AgentWindowsManaged.Tests/TempFolder.cs @@ -0,0 +1,66 @@ +using System; +using System.Diagnostics; +using System.IO; + +using Xunit; + +namespace DevolutionsAgent.Installer.Tests; + +/// +/// A new, empty folder under the temporary directory, deleted with its content on disposal. +/// +internal sealed class TempFolder : IDisposable +{ + public string Path { get; } = System.IO.Path.Combine(System.IO.Path.GetTempPath(), $"installer-tests-{Guid.NewGuid():N}"); + + public TempFolder() => Directory.CreateDirectory(this.Path); + + /// + /// Creates the directory below this folder, with its parents. + /// + public string CreateDirectory(params string[] names) => + Directory.CreateDirectory(System.IO.Path.Combine([this.Path, .. names])).FullName; + + /// + /// Creates a junction at to the directory , which needs no + /// elevation. + /// + public static void CreateJunction(string link, string target) + { + ProcessStartInfo startInfo = new("cmd.exe", $"/c mklink /J \"{link}\" \"{target}\"") + { + UseShellExecute = false, + CreateNoWindow = true, + RedirectStandardOutput = true, + RedirectStandardError = true, + }; + + using Process process = Process.Start(startInfo); + string output = process.StandardOutput.ReadToEnd() + process.StandardError.ReadToEnd(); + process.WaitForExit(); + + Assert.True(process.ExitCode == 0, output); + } + + public void Dispose() + { + // A recursive Directory.Delete fails on a junction without elevation, so junctions go first. + RemoveJunctions(this.Path); + Directory.Delete(this.Path, recursive: true); + } + + private static void RemoveJunctions(string directory) + { + foreach (string entry in Directory.GetDirectories(directory)) + { + if (File.GetAttributes(entry).HasFlag(FileAttributes.ReparsePoint)) + { + Directory.Delete(entry); + } + else + { + RemoveJunctions(entry); + } + } + } +} diff --git a/package/AgentWindowsManaged/Actions/AgentActions.cs b/package/AgentWindowsManaged/Actions/AgentActions.cs index 795fc6648..53cc41e90 100644 --- a/package/AgentWindowsManaged/Actions/AgentActions.cs +++ b/package/AgentWindowsManaged/Actions/AgentActions.cs @@ -110,19 +110,45 @@ internal static class AgentActions // Deferred sequence /// - /// Create the path %ProgramData%\Devolutions\Agent if it does not exist + /// Fail the run, with an error naming the folder, when %ProgramData%\Devolutions or %ProgramData%\Devolutions\Agent + /// is a link or is controlled by an account that is not an administrator + /// + /// + /// The major upgrade removes the installed product at RemoveExistingProducts, right after InstallInitialize, so + /// running before it fails an install, an upgrade, a repair or a maintenance run before it changes anything. + /// A full uninstall is never refused, so that a machine can always be uninstalled. + /// + internal static readonly ElevatedManagedAction checkProgramDataDirectory = new( + new Id($"CA.{nameof(checkProgramDataDirectory)}"), + CustomActions.CheckProgramDataDirectory, + Return.check, + When.Before, Step.RemoveExistingProducts, + new Condition("NOT (REMOVE=\"ALL\")"), + Sequence.InstallExecuteSequence) + { + Execute = Execute.deferred, + Impersonate = false, + UsesProperties = "ProductLanguage=[ProductLanguage]", + }; + + /// + /// Create the path %ProgramData%\Devolutions\Agent if it does not exist, after checking its folders again as + /// does /// /// /// It's hard to tell the installer not to remove directories on uninstall. Since we want this folder to persist, /// it's easy to create it with a custom action than workaround Windows Installer. /// - private static readonly ElevatedManagedAction createProgramDataDirectory = new( + internal static readonly ElevatedManagedAction createProgramDataDirectory = new( new Id($"CA.{nameof(createProgramDataDirectory)}"), CustomActions.CreateProgramDataDirectory, Return.check, When.After, Step.CreateFolders, - Condition.Always, - Sequence.InstallExecuteSequence); + new Condition("NOT (REMOVE=\"ALL\")"), + Sequence.InstallExecuteSequence) + { + UsesProperties = "ProductLanguage=[ProductLanguage]", + }; /// /// Create the path %ProgramData%\Devolutions\Agent\Pedm and subfolders if they do not exist @@ -586,6 +612,7 @@ private static string UseProperties(IEnumerable properties) configurePsuAgent, rollbackConfigurePsuAgent, commitConfigurePsuAgent, + checkProgramDataDirectory, createProgramDataDirectory, setProgramDataDirectoryPermissions, createProgramDataPedmDirectories, diff --git a/package/AgentWindowsManaged/Actions/CustomActions.cs b/package/AgentWindowsManaged/Actions/CustomActions.cs index 9f962f6e2..c0ce90757 100644 --- a/package/AgentWindowsManaged/Actions/CustomActions.cs +++ b/package/AgentWindowsManaged/Actions/CustomActions.cs @@ -148,6 +148,24 @@ public static ActionResult CleanAgentConfigRollback(Session session) return ActionResult.Failure; } + [CustomAction] + public static ActionResult CheckProgramDataDirectory(Session session) + { + try + { + DataFolderRefusal refusal = DataFolders.Check( + Environment.GetFolderPath(Environment.SpecialFolder.CommonApplicationData), + LogDelegate.WithSession(session)); + + return refusal is null ? ActionResult.Success : Refuse(session, refusal); + } + catch (Exception e) + { + session.Log($"failed to check {ProgramDataDirectory}: {e}"); + return ActionResult.Failure; + } + } + [CustomAction] public static ActionResult CreateProgramDataDirectory(Session session) { @@ -155,13 +173,14 @@ public static ActionResult CreateProgramDataDirectory(Session session) try { - // The data directory gets its security descriptor when it is created, so that it never exists with the - // DACL it would inherit from ProgramData, which lets users add entries. - Directory.CreateDirectory(Path.GetDirectoryName(path)); + DataFolderRefusal refusal = DataFolders.Create( + Environment.GetFolderPath(Environment.SpecialFolder.CommonApplicationData), + Includes.PROGRAM_DATA_SDDL, + LogDelegate.WithSession(session)); - if (!SecureDirectory.Create(path, Includes.PROGRAM_DATA_SDDL) && !Directory.Exists(path)) + if (refusal is not null) { - throw new IOException($"{path} exists and is not a directory"); + return Refuse(session, refusal); } session.Log($"created directory at {path} or already exists"); @@ -175,6 +194,22 @@ public static ActionResult CreateProgramDataDirectory(Session session) return ActionResult.Success; } + /// + /// Logs the error for in the installer's language and shows it, which a silent install + /// only logs. + /// + private static ActionResult Refuse(Session session, DataFolderRefusal refusal) + { + // UNTESTED: a refusal is sent as an error message, which a silent install logs and an interactive one shows. + // No test runs the data folder actions, which read the machine's own ProgramData. + // When changing this, keep sending it with InstallMessage.Error, as ReadIdentityEnrollmentTokenFile does. + string message = refusal.Message(session.Property("ProductLanguage")); + session.Log(message); + using Record record = new(0) { FormatString = EscapeFormatted(message) }; + session.Message(InstallMessage.Error | (uint)MessageButtons.OK, record); + return ActionResult.Failure; + } + [CustomAction] public static ActionResult CreateProgramDataPedmDirectories(Session session) { diff --git a/package/AgentWindowsManaged/Actions/DataFolders.cs b/package/AgentWindowsManaged/Actions/DataFolders.cs new file mode 100644 index 000000000..e833c9a28 --- /dev/null +++ b/package/AgentWindowsManaged/Actions/DataFolders.cs @@ -0,0 +1,291 @@ +using DevolutionsAgent.Resources; +using Microsoft.Win32.SafeHandles; +using System; +using System.ComponentModel; +using System.IO; +using System.Security.AccessControl; +using System.Security.Principal; + +namespace DevolutionsAgent.Actions; + +/// +/// Why the installer refuses to use a folder of the agent's data path. +/// +internal enum DataFolderProblem +{ + /// + /// The folder is a link, which can point anywhere. + /// + Link, + + /// + /// The folder's owner, who can always change its permissions, is not trusted. + /// + Owner, + + /// + /// The folder's DACL lets an untrusted account replace the folder or what it contains. + /// + Access, + + /// + /// The folder's owner or DACL can't be read. + /// + Unreadable, +} + +/// +/// A folder of the agent's data path that the installer refuses to use. +/// +internal sealed class DataFolderRefusal(string folder, DataFolderProblem problem, SecurityIdentifier account = null) +{ + internal string Folder { get; } = folder; + + internal DataFolderProblem Problem { get; } = problem; + + /// + /// The owner of the folder for , or the account the offending entry grants for + /// . + /// + internal SecurityIdentifier Account { get; } = account; + + /// + /// The error the installer shows, in the language whose LCID is . + /// + internal string Message(string productLanguage) + { + string id = this.Problem switch + { + DataFolderProblem.Link => Strings.DataFolderIsLink, + DataFolderProblem.Owner => Strings.DataFolderOwnedByUntrustedAccount, + DataFolderProblem.Access => Strings.DataFolderReplaceableByUntrustedAccount, + DataFolderProblem.Unreadable => Strings.DataFolderSecurityUnreadable, + _ => throw new ArgumentOutOfRangeException(nameof(this.Problem), this.Problem, null), + }; + + return string.Format(Program.Localize(productLanguage, id), this.Folder, AccountName(this.Account)); + } + + /// + /// The account name of , or its SID string when it has none. + /// + private static string AccountName(SecurityIdentifier sid) + { + if (sid is null) + { + return null; + } + + try + { + return sid.Translate(typeof(NTAccount)).Value; + } + catch (Exception) + { + return sid.Value; + } + } +} + +/// +/// Checks Devolutions and Devolutions\Agent under ProgramData before the installer uses them. +/// +/// +/// The installer writes the agent's data there as SYSTEM, so an account that controls either folder could redirect +/// or replace what the installer and the agent write. +/// A folder that doesn't exist yet passes. +/// +internal static class DataFolders +{ + private const string Devolutions = "Devolutions"; + + private const string Agent = "Agent"; + + /// + /// The rights that let an account delete, rename or replace a folder or an entry in it, or give itself any right. + /// + private const uint ReplaceRights = + WinAPI.DELETE | WinAPI.WRITE_DAC | WinAPI.WRITE_OWNER | WinAPI.FILE_DELETE_CHILD | WinAPI.GENERIC_ALL; + + /// + /// Checks Devolutions and Devolutions\Agent under without changing anything. + /// + /// The first refusal, or null when both folders pass. + internal static DataFolderRefusal Check(string root, ILogger logger) + { + string devolutions = Path.Combine(root, Devolutions); + DataFolderRefusal refusal = Inspect(devolutions, hold: true, checkAccess: true, logger, out SafeFileHandle handle); + + using (handle) + { + if (refusal is not null || handle is null) + { + return refusal; + } + + // UNTESTED: an existing Agent is refused when it is a link or has an untrusted owner, here and in Create. + // A test reaches it only below a Devolutions folder that passes, which needs an owner that a test without + // elevation can't set. + // When changing this, keep Agent checked in both, without the DACL rule, as the installer resets its DACL. + refusal = Inspect(Path.Combine(devolutions, Agent), hold: false, checkAccess: false, logger, out SafeFileHandle agent); + agent?.Dispose(); + + return refusal; + } + } + + /// + /// Creates Devolutions\Agent under with the security descriptor + /// , after checking Devolutions and, when it already exists, Agent. + /// + /// + /// A missing Devolutions is created with the DACL it inherits, then checked like an existing one. + /// Agent gets its security descriptor when it is created, so that it never exists with the DACL it would + /// inherit from ProgramData, which lets users add entries. + /// + /// The first refusal, in which case nothing is created below Devolutions, or null. + /// Agent exists and is not a directory. + internal static DataFolderRefusal Create(string root, string sddl, ILogger logger) + { + string devolutions = Path.Combine(root, Devolutions); + string agent = Path.Combine(devolutions, Agent); + + Directory.CreateDirectory(devolutions); + + // UNTESTED: the handle on Devolutions denies delete sharing until Agent is created or checked below it. + // Renaming or replacing a folder needs delete access to it, so Agent is created or checked in the folder just + // checked, not in one put at that path afterwards. + // When changing this, keep the share mode of that handle and keep it open until Agent is done. + DataFolderRefusal refusal = Inspect(devolutions, hold: true, checkAccess: true, logger, out SafeFileHandle handle); + + using (handle) + { + if (refusal is not null) + { + return refusal; + } + + if (handle is null) + { + throw new IOException($"{devolutions} was deleted after it was created"); + } + + if (SecureDirectory.Create(agent, sddl)) + { + logger.Log($"created {agent}"); + return null; + } + + refusal = Inspect(agent, hold: false, checkAccess: false, logger, out SafeFileHandle existing); + existing?.Dispose(); + + if (refusal is null && !Directory.Exists(agent)) + { + throw new IOException($"{agent} exists and is not a directory"); + } + + return refusal; + } + } + + /// + /// Whether the installer refuses to use , given its security descriptor and whether it is a + /// link. + /// + /// + /// Whether to refuse a DACL that lets an account other than SYSTEM, Administrators or TrustedInstaller replace the + /// folder or what it contains. + /// + /// The refusal, or null when the folder passes. + internal static DataFolderRefusal Evaluate( + string folder, RawSecurityDescriptor security, bool isLink, bool checkAccess) + { + if (isLink) + { + return new DataFolderRefusal(folder, DataFolderProblem.Link); + } + + if (security.Owner is null) + { + return new DataFolderRefusal(folder, DataFolderProblem.Unreadable); + } + + // The DACL is checked before the owner, whose trust may need a directory lookup. + if (checkAccess) + { + // A null DACL grants every right to everyone. + if (security.DiscretionaryAcl is null) + { + return new DataFolderRefusal( + folder, DataFolderProblem.Access, new SecurityIdentifier(WellKnownSidType.WorldSid, null)); + } + + foreach (GenericAce ace in security.DiscretionaryAcl) + { + // An inherit-only entry applies to the entries created below the folder, not to the folder. + if (ace is KnownAce entry + && IsAllow(entry.AceType) + && (entry.AceFlags & AceFlags.InheritOnly) == 0 + && (unchecked((uint)entry.AccessMask) & ReplaceRights) != 0 + && !SecureDirectory.IsPrivileged(entry.SecurityIdentifier)) + { + return new DataFolderRefusal(folder, DataFolderProblem.Access, entry.SecurityIdentifier); + } + } + } + + bool trusted; + + try + { + trusted = SecureDirectory.IsTrustedOwner(security.Owner); + } + catch (Exception) + { + // An owner whose group membership can't be resolved isn't known to be an administrator. + trusted = false; + } + + return trusted ? null : new DataFolderRefusal(folder, DataFolderProblem.Owner, security.Owner); + } + + /// + /// Opens the entry names, a link itself rather than its target, and evaluates its security. + /// + /// The open entry, or null when nothing exists at . + /// The refusal, or null when the entry passes or doesn't exist. + private static DataFolderRefusal Inspect( + string path, bool hold, bool checkAccess, ILogger logger, out SafeFileHandle handle) + { + handle = null; + + try + { + handle = SecureDirectory.OpenToRead(path, hold); + (RawSecurityDescriptor security, bool isLink) = SecureDirectory.ReadSecurity(handle); + DataFolderRefusal refusal = Evaluate(path, security, isLink, checkAccess); + + logger.Log(refusal is null + ? $"checked {path}" + : $"refused {path}: {refusal.Problem} {refusal.Account}"); + + return refusal; + } + catch (Win32Exception e) when (handle is null && SecureDirectory.IsNotFound(e)) + { + return null; + } + catch (Exception e) + { + logger.Log($"failed to read the owner and the DACL of {path}: {e}"); + return new DataFolderRefusal(path, DataFolderProblem.Unreadable); + } + } + + private static bool IsAllow(AceType type) => + type is AceType.AccessAllowed + or AceType.AccessAllowedCompound + or AceType.AccessAllowedObject + or AceType.AccessAllowedCallback + or AceType.AccessAllowedCallbackObject; +} diff --git a/package/AgentWindowsManaged/Actions/SecureDirectory.cs b/package/AgentWindowsManaged/Actions/SecureDirectory.cs index 5a3bb00c6..c6b33bbc2 100644 --- a/package/AgentWindowsManaged/Actions/SecureDirectory.cs +++ b/package/AgentWindowsManaged/Actions/SecureDirectory.cs @@ -5,6 +5,7 @@ using System.IO; using System.Linq; using System.Runtime.InteropServices; +using System.Security.AccessControl; using System.Security.Principal; namespace DevolutionsAgent.Actions; @@ -94,39 +95,47 @@ internal static void SetSecurity(string path, string sddl) } /// - /// The owner of the entry names, opened without following a link, and whether that entry - /// is a reparse point. + /// Opens the entry names, a link itself rather than its target, to read its security. /// - internal static (SecurityIdentifier Owner, bool IsReparsePoint) ReadOwner(string path) + /// + /// Whether to deny delete sharing, so that the entry can't be renamed or replaced while the handle is open. + /// + internal static SafeFileHandle OpenToRead(string path, bool hold) => + Open(path, WinAPI.READ_CONTROL | WinAPI.FILE_READ_ATTRIBUTES, hold ? HeldShare : AnyShare); + + /// + /// The owner and the DACL of the entry holds, and whether that entry is a reparse point. + /// + internal static (RawSecurityDescriptor Security, bool IsReparsePoint) ReadSecurity(SafeFileHandle handle) { - using SafeFileHandle handle = Open(path, WinAPI.READ_CONTROL | WinAPI.FILE_READ_ATTRIBUTES, AnyShare); bool isReparsePoint = (GetAttributes(handle) & WinAPI.FILE_ATTRIBUTE_REPARSE_POINT) != 0; + const uint information = WinAPI.OWNER_SECURITY_INFORMATION | WinAPI.DACL_SECURITY_INFORMATION; - uint error = WinAPI.GetSecurityInfo( - handle, - WinAPI.SE_FILE_OBJECT, - WinAPI.OWNER_SECURITY_INFORMATION, - out IntPtr owner, - IntPtr.Zero, - IntPtr.Zero, - IntPtr.Zero, - out IntPtr descriptor); + // The first call only reports the size the second needs. + WinAPI.GetKernelObjectSecurity(handle, information, null, 0, out uint size); + int error = Marshal.GetLastWin32Error(); - if (error != 0) + if (size == 0) { - throw new Win32Exception((int)error); + throw new Win32Exception(error); } - try - { - return (new SecurityIdentifier(owner), isReparsePoint); - } - finally + byte[] descriptor = new byte[size]; + + if (!WinAPI.GetKernelObjectSecurity(handle, information, descriptor, size, out _)) { - WinAPI.LocalFree(descriptor); + throw new Win32Exception(Marshal.GetLastWin32Error()); } + + return (new RawSecurityDescriptor(descriptor, 0), isReparsePoint); } + /// + /// Whether is SYSTEM, Administrators or TrustedInstaller. + /// + internal static bool IsPrivileged(SecurityIdentifier sid) => + sid.IsWellKnown(WellKnownSidType.LocalSystemSid) || sid == Administrators || sid == TrustedInstaller; + /// /// Whether may own the agent's data: SYSTEM, Administrators, TrustedInstaller, or a member /// of Administrators. @@ -140,7 +149,7 @@ internal static (SecurityIdentifier Owner, bool IsReparsePoint) ReadOwner(string /// Authz failed and the local member list of Administrators can't be read. internal static bool IsTrustedOwner(SecurityIdentifier owner) { - if (owner.IsWellKnown(WellKnownSidType.LocalSystemSid) || owner == Administrators || owner == TrustedInstaller) + if (IsPrivileged(owner)) { return true; } @@ -291,7 +300,7 @@ private static uint GetAttributes(SafeFileHandle handle) private static bool IsPlainDirectory(uint attributes) => (attributes & WinAPI.FILE_ATTRIBUTE_DIRECTORY) != 0 && (attributes & WinAPI.FILE_ATTRIBUTE_REPARSE_POINT) == 0; - private static bool IsNotFound(Win32Exception e) => + internal static bool IsNotFound(Win32Exception e) => e.NativeErrorCode is WinAPI.ERROR_FILE_NOT_FOUND or WinAPI.ERROR_PATH_NOT_FOUND; /// diff --git a/package/AgentWindowsManaged/Actions/WinAPI.cs b/package/AgentWindowsManaged/Actions/WinAPI.cs index e32d7a26d..414f4ddb6 100644 --- a/package/AgentWindowsManaged/Actions/WinAPI.cs +++ b/package/AgentWindowsManaged/Actions/WinAPI.cs @@ -29,6 +29,8 @@ internal static class WinAPI internal const uint FILE_ATTRIBUTE_DIRECTORY = 0x00000010; + internal const uint FILE_DELETE_CHILD = 0x00000040; + internal static uint FILE_ATTRIBUTE_NORMAL = 0x00000080; internal const uint FILE_ATTRIBUTE_REPARSE_POINT = 0x00000400; @@ -47,6 +49,8 @@ internal static class WinAPI internal const int FileDispositionInfo = 4; + internal const uint GENERIC_ALL = 0x10000000; + internal static uint GENERIC_WRITE = 0x40000000; internal const uint MAX_PREFERRED_LENGTH = 0xFFFFFFFF; @@ -67,8 +71,6 @@ internal static class WinAPI internal const uint READ_CONTROL = 0x00020000; - internal const int SE_FILE_OBJECT = 1; - internal const uint SC_MANAGER_ALL_ACCESS = 0xF003F; internal const uint SC_MANAGER_CONNECT = 0x0001; @@ -397,16 +399,14 @@ internal static extern uint GetFinalPathNameByHandle( uint cchFilePath, uint dwFlags); - [DllImport("advapi32")] - internal static extern uint GetSecurityInfo( - SafeFileHandle handle, - int ObjectType, - uint SecurityInfo, - out IntPtr ppsidOwner, - IntPtr ppsidGroup, - IntPtr ppDacl, - IntPtr ppSacl, - out IntPtr ppSecurityDescriptor); + [DllImport("advapi32", SetLastError = true)] + [return: MarshalAs(UnmanagedType.Bool)] + internal static extern bool GetKernelObjectSecurity( + SafeFileHandle Handle, + uint RequestedInformation, + byte[] pSecurityDescriptor, + uint nLength, + out uint lpnLengthNeeded); [DllImport("kernel32", SetLastError = true)] internal static extern IntPtr GetStdHandle(int nStdHandle); diff --git a/package/AgentWindowsManaged/Resources/DevolutionsAgent_en-us.wxl b/package/AgentWindowsManaged/Resources/DevolutionsAgent_en-us.wxl index 8a2fd42f1..0245a2c67 100644 --- a/package/AgentWindowsManaged/Resources/DevolutionsAgent_en-us.wxl +++ b/package/AgentWindowsManaged/Resources/DevolutionsAgent_en-us.wxl @@ -21,6 +21,10 @@ Devolutions Inc. Devolutions + Can't use the folder {0} because it is a link. Delete the link, then try again. + Can't use the folder {0} because it is owned by {1}. Only SYSTEM or an administrator may own it. Change its owner, then try again. + Can't use the folder {0} because {1} can replace it or what it contains. Remove that permission, then try again. + Can't read the owner or the permissions of the folder {0}. Check them, then try again. The product requires Microsoft .NET Framework 4.8. Would you like to download it now? Set IDENTITY_ENROLLMENT_TOKEN or IDENTITY_ENROLLMENT_TOKEN_FILE, not both. The first line of the enrollment token file {0} is empty. diff --git a/package/AgentWindowsManaged/Resources/DevolutionsAgent_fr-fr.wxl b/package/AgentWindowsManaged/Resources/DevolutionsAgent_fr-fr.wxl index df95d22ba..8106d0024 100644 --- a/package/AgentWindowsManaged/Resources/DevolutionsAgent_fr-fr.wxl +++ b/package/AgentWindowsManaged/Resources/DevolutionsAgent_fr-fr.wxl @@ -15,6 +15,10 @@ Devolutions Inc. Devolutions + Impossible d'utiliser le dossier {0}, car c'est un lien. Supprimez le lien, puis réessayez. + Impossible d'utiliser le dossier {0}, car il appartient à {1}. Seul SYSTEM ou un administrateur peut en être propriétaire. Changez son propriétaire, puis réessayez. + Impossible d'utiliser le dossier {0}, car {1} peut remplacer le dossier ou son contenu. Retirez cette autorisation, puis réessayez. + Impossible de lire le propriétaire ou les autorisations du dossier {0}. Vérifiez-les, puis réessayez. Le produit nécessite Microsoft .NET Framework 4.8. Souhaiteriez-vous le télécharger maintenant? Définissez IDENTITY_ENROLLMENT_TOKEN ou IDENTITY_ENROLLMENT_TOKEN_FILE, mais pas les deux. La première ligne du fichier de jeton d'enrôlement {0} est vide. diff --git a/package/AgentWindowsManaged/Resources/DevolutionsAgent_sv-se.wxl b/package/AgentWindowsManaged/Resources/DevolutionsAgent_sv-se.wxl index 2fb1d65a7..f47608239 100644 --- a/package/AgentWindowsManaged/Resources/DevolutionsAgent_sv-se.wxl +++ b/package/AgentWindowsManaged/Resources/DevolutionsAgent_sv-se.wxl @@ -21,6 +21,10 @@ Devolutions Inc. Devolutions + Det går inte att använda mappen {0} eftersom den är en länk. Ta bort länken och försök igen. + Det går inte att använda mappen {0} eftersom den ägs av {1}. Endast SYSTEM eller en administratör får äga den. Byt ägare och försök igen. + Det går inte att använda mappen {0} eftersom {1} kan ersätta den eller dess innehåll. Ta bort den behörigheten och försök igen. + Det går inte att läsa ägaren eller behörigheterna för mappen {0}. Kontrollera dem och försök igen. Produkten kräver Microsoft .NET Framework 4.8. Vill du hämta den nu? Ange IDENTITY_ENROLLMENT_TOKEN eller IDENTITY_ENROLLMENT_TOKEN_FILE, inte båda. Första raden i registreringstokenfilen {0} är tom. diff --git a/package/AgentWindowsManaged/Resources/Strings.g.cs b/package/AgentWindowsManaged/Resources/Strings.g.cs index 599f5abbf..d45b6a9e8 100644 --- a/package/AgentWindowsManaged/Resources/Strings.g.cs +++ b/package/AgentWindowsManaged/Resources/Strings.g.cs @@ -117,6 +117,22 @@ public static string I18n(this MsiRuntime runtime, string res) /// public const string IdentityEnrollmentTokenFileEmpty = "IdentityEnrollmentTokenFileEmpty"; /// + /// Can't use the folder {0} because it is a link. Delete the link, then try again. + /// + public const string DataFolderIsLink = "DataFolderIsLink"; + /// + /// Can't use the folder {0} because it is owned by {1}. Only SYSTEM or an administrator may own it. Change its owner, then try again. + /// + public const string DataFolderOwnedByUntrustedAccount = "DataFolderOwnedByUntrustedAccount"; + /// + /// Can't use the folder {0} because {1} can replace it or what it contains. Remove that permission, then try again. + /// + public const string DataFolderReplaceableByUntrustedAccount = "DataFolderReplaceableByUntrustedAccount"; + /// + /// Can't read the owner or the permissions of the folder {0}. Check them, then try again. + /// + public const string DataFolderSecurityUnreadable = "DataFolderSecurityUnreadable"; + /// /// View /// public const string ViewButton = "ViewButton"; diff --git a/package/AgentWindowsManaged/Resources/Strings_en-US.json b/package/AgentWindowsManaged/Resources/Strings_en-US.json index 59f7829f2..edb1b10ad 100644 --- a/package/AgentWindowsManaged/Resources/Strings_en-US.json +++ b/package/AgentWindowsManaged/Resources/Strings_en-US.json @@ -111,6 +111,22 @@ { "id": "IdentityEnrollmentTokenFileEmpty", "text": "The first line of the enrollment token file {0} is empty." + }, + { + "id": "DataFolderIsLink", + "text": "Can't use the folder {0} because it is a link. Delete the link, then try again." + }, + { + "id": "DataFolderOwnedByUntrustedAccount", + "text": "Can't use the folder {0} because it is owned by {1}. Only SYSTEM or an administrator may own it. Change its owner, then try again." + }, + { + "id": "DataFolderReplaceableByUntrustedAccount", + "text": "Can't use the folder {0} because {1} can replace it or what it contains. Remove that permission, then try again." + }, + { + "id": "DataFolderSecurityUnreadable", + "text": "Can't read the owner or the permissions of the folder {0}. Check them, then try again." } ], "buttons": [ diff --git a/package/AgentWindowsManaged/Resources/Strings_fr-FR.json b/package/AgentWindowsManaged/Resources/Strings_fr-FR.json index 567a8060f..3661a3bb8 100644 --- a/package/AgentWindowsManaged/Resources/Strings_fr-FR.json +++ b/package/AgentWindowsManaged/Resources/Strings_fr-FR.json @@ -87,6 +87,22 @@ { "id": "IdentityEnrollmentTokenFileEmpty", "text": "La première ligne du fichier de jeton d'enrôlement {0} est vide." + }, + { + "id": "DataFolderIsLink", + "text": "Impossible d'utiliser le dossier {0}, car c'est un lien. Supprimez le lien, puis réessayez." + }, + { + "id": "DataFolderOwnedByUntrustedAccount", + "text": "Impossible d'utiliser le dossier {0}, car il appartient à {1}. Seul SYSTEM ou un administrateur peut en être propriétaire. Changez son propriétaire, puis réessayez." + }, + { + "id": "DataFolderReplaceableByUntrustedAccount", + "text": "Impossible d'utiliser le dossier {0}, car {1} peut remplacer le dossier ou son contenu. Retirez cette autorisation, puis réessayez." + }, + { + "id": "DataFolderSecurityUnreadable", + "text": "Impossible de lire le propriétaire ou les autorisations du dossier {0}. Vérifiez-les, puis réessayez." } ], "buttons": [ diff --git a/package/AgentWindowsManaged/Resources/Strings_sv-SE.json b/package/AgentWindowsManaged/Resources/Strings_sv-SE.json index bcb2e544f..73a6b134e 100644 --- a/package/AgentWindowsManaged/Resources/Strings_sv-SE.json +++ b/package/AgentWindowsManaged/Resources/Strings_sv-SE.json @@ -111,6 +111,22 @@ { "id": "IdentityEnrollmentTokenFileEmpty", "text": "Första raden i registreringstokenfilen {0} är tom." + }, + { + "id": "DataFolderIsLink", + "text": "Det går inte att använda mappen {0} eftersom den är en länk. Ta bort länken och försök igen." + }, + { + "id": "DataFolderOwnedByUntrustedAccount", + "text": "Det går inte att använda mappen {0} eftersom den ägs av {1}. Endast SYSTEM eller en administratör får äga den. Byt ägare och försök igen." + }, + { + "id": "DataFolderReplaceableByUntrustedAccount", + "text": "Det går inte att använda mappen {0} eftersom {1} kan ersätta den eller dess innehåll. Ta bort den behörigheten och försök igen." + }, + { + "id": "DataFolderSecurityUnreadable", + "text": "Det går inte att läsa ägaren eller behörigheterna för mappen {0}. Kontrollera dem och försök igen." } ], "buttons": [ From 8672a0d4940c0e94d80b5455a58163fccfd39bb6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Tue, 27 Oct 2026 23:04:56 +0900 Subject: [PATCH 4/9] fix(agent-installer): refuse the data folder DACLs the agent refuses The installer's check of %ProgramData%\Devolutions now refuses exactly the DACLs that the agent refuses for a directory above its data, so the installer never prepares a folder the agent then won't use. An entry for OWNER RIGHTS, whose holder is the owner the check already requires to be trusted, or for an AppContainer package or capability, which never grants access by itself, no longer causes a refusal. An entry of a type other than a plain or conditional allow entry or a deny entry is refused as unreadable, as the agent does. The pedm folder is now created after the Agent folder, so that it never creates Agent itself with the DACL Agent would inherit from ProgramData. Issue: DGW-478 Co-Authored-By: Claude Opus 5.5 --- .../DataFolderTests.cs | 25 ++++++- .../Actions/AgentActions.cs | 6 +- .../Actions/DataFolders.cs | 65 +++++++++++++++---- 3 files changed, 79 insertions(+), 17 deletions(-) diff --git a/package/AgentWindowsManaged.Tests/DataFolderTests.cs b/package/AgentWindowsManaged.Tests/DataFolderTests.cs index 8f607050e..1b4898b83 100644 --- a/package/AgentWindowsManaged.Tests/DataFolderTests.cs +++ b/package/AgentWindowsManaged.Tests/DataFolderTests.cs @@ -87,8 +87,20 @@ public sealed class DataFolderTests [InlineData("TrustedInstaller has full control", Devolutions, "O:SYD:(A;;FA;;;S-1-5-80-956008885-3418522649-1831038044-1853292631-2271478464)", "none")] // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security [InlineData("no owner", Devolutions, "D:(A;;FA;;;SY)", "Unreadable")] - // oracle: scaffold - [InlineData("Users may delete through an object entry", Devolutions, "O:SYD:(OA;;SD;bf967aba-0de6-11d0-a285-00aa003049e2;;BU)", "Access " + Users)] + // oracle: reference Devolutions/devolutions-gateway@9b8b60380f6c:crates/private-fs/src/windows/security.rs Policy::check_ancestor + [InlineData("OWNER RIGHTS has full control", Devolutions, "O:SYD:(A;OICI;FA;;;OW)", "none")] + // oracle: reference Devolutions/devolutions-gateway@9b8b60380f6c:crates/private-fs/src/windows/security.rs Policy::check_ancestor + [InlineData("an AppContainer package has full control", Devolutions, "O:SYD:(A;OICI;FA;;;S-1-15-2-1)", "none")] + // oracle: reference Devolutions/devolutions-gateway@9b8b60380f6c:crates/private-fs/src/windows/security.rs Policy::check_ancestor + [InlineData("an AppContainer capability has full control", Devolutions, "O:SYD:(A;OICI;FA;;;S-1-15-3-1)", "none")] + // oracle: reference Devolutions/devolutions-gateway@9b8b60380f6c:crates/private-fs/src/windows/security.rs Policy::check_ancestor + [InlineData("Users are denied deletion through an object entry", Devolutions, "O:SYD:(OD;;SD;bf967aba-0de6-11d0-a285-00aa003049e2;;BU)", "none")] + // oracle: reference Devolutions/devolutions-gateway@9b8b60380f6c:crates/private-fs/src/windows/security.rs Policy::check_ancestor + [InlineData("Users may delete through an object entry", Devolutions, "O:SYD:(OA;;SD;bf967aba-0de6-11d0-a285-00aa003049e2;;BU)", "Unreadable")] + // oracle: reference Devolutions/devolutions-gateway@9b8b60380f6c:crates/private-fs/src/windows/security.rs Policy::check_ancestor + [InlineData("an inherit-only object entry", Devolutions, "O:SYD:(OA;CIIO;SD;bf967aba-0de6-11d0-a285-00aa003049e2;;BU)", "Unreadable")] + // oracle: reference Devolutions/devolutions-gateway@9b8b60380f6c:crates/private-fs/src/windows/security.rs Policy::check_ancestor + [InlineData("Users may delete through a conditional object entry", Devolutions, "O:SYD:(ZA;;SD;bf967aba-0de6-11d0-a285-00aa003049e2;;BU;(Member_of {SID(BU)}))", "Unreadable")] // oracle: scaffold [InlineData("Users have full control of Agent", Agent, "O:SYD:(A;OICI;FA;;;BU)", "none")] public void RefusalFollowsTheOwnerAndTheDacl(string scenario, bool checkAccess, string sddl, string expected) @@ -286,6 +298,15 @@ public void CheckRunsBeforeTheMajorUpgradeRemovesTheInstalledProduct() Assert.Equal(Step.RemoveExistingProducts.ToString(), AgentActions.checkProgramDataDirectory.Step.ToString()); } + // oracle: scaffold + [Fact] + public void PedmFolderIsCreatedAfterTheAgentFolder() + { + // Created first, the pedm folder would also create Agent, with the DACL it inherits. + Assert.Equal(When.After, AgentActions.createProgramDataPedmDirectories.When); + Assert.Equal(AgentActions.createProgramDataDirectory.Id, AgentActions.createProgramDataPedmDirectories.Step.ToString()); + } + /// /// The problem and the account names, or none. /// diff --git a/package/AgentWindowsManaged/Actions/AgentActions.cs b/package/AgentWindowsManaged/Actions/AgentActions.cs index 53cc41e90..c53b0723e 100644 --- a/package/AgentWindowsManaged/Actions/AgentActions.cs +++ b/package/AgentWindowsManaged/Actions/AgentActions.cs @@ -156,12 +156,14 @@ internal static class AgentActions /// /// It's hard to tell the installer not to remove directories on uninstall. Since we want this folder to persist, /// it's easy to create it with a custom action than workaround Windows Installer. + /// It runs after , which checks the folders above and creates Agent with its + /// own security descriptor, so that it never creates Agent itself with an inherited DACL. /// - private static readonly ElevatedManagedAction createProgramDataPedmDirectories = new( + internal static readonly ElevatedManagedAction createProgramDataPedmDirectories = new( new Id($"CA.{nameof(createProgramDataPedmDirectories)}"), CustomActions.CreateProgramDataPedmDirectories, Return.check, - When.After, Step.CreateFolders, + When.After, new Step(createProgramDataDirectory.Id), Features.PEDM_FEATURE.BeingInstall(), Sequence.InstallExecuteSequence); diff --git a/package/AgentWindowsManaged/Actions/DataFolders.cs b/package/AgentWindowsManaged/Actions/DataFolders.cs index e833c9a28..1622f0e9c 100644 --- a/package/AgentWindowsManaged/Actions/DataFolders.cs +++ b/package/AgentWindowsManaged/Actions/DataFolders.cs @@ -107,6 +107,11 @@ internal static class DataFolders private const uint ReplaceRights = WinAPI.DELETE | WinAPI.WRITE_DAC | WinAPI.WRITE_OWNER | WinAPI.FILE_DELETE_CHILD | WinAPI.GENERIC_ALL; + /// + /// OWNER RIGHTS, whose entry applies to the owner, which is checked on its own. + /// + private static readonly SecurityIdentifier OwnerRights = new("S-1-3-4"); + /// /// Checks Devolutions and Devolutions\Agent under without changing anything. /// @@ -192,9 +197,13 @@ internal static DataFolderRefusal Create(string root, string sddl, ILogger logge /// Whether the installer refuses to use , given its security descriptor and whether it is a /// link. /// + /// + /// The DACL rule refuses what the agent refuses for a directory above its data, so that the installer never prepares + /// a folder the agent then refuses to use. + /// /// - /// Whether to refuse a DACL that lets an account other than SYSTEM, Administrators or TrustedInstaller replace the - /// folder or what it contains. + /// Whether to refuse a DACL that lets an account other than SYSTEM, Administrators, TrustedInstaller or the owner + /// replace the folder or what it contains. /// /// The refusal, or null when the folder passes. internal static DataFolderRefusal Evaluate( @@ -222,12 +231,34 @@ internal static DataFolderRefusal Evaluate( foreach (GenericAce ace in security.DiscretionaryAcl) { + switch (ace.AceType) + { + // A deny entry only takes access away. + case AceType.AccessDenied: + case AceType.AccessDeniedCallback: + case AceType.AccessDeniedObject: + case AceType.AccessDeniedCallbackObject: + continue; + case AceType.AccessAllowed: + case AceType.AccessAllowedCallback: + break; + // The agent refuses an entry of any other type, so the installer does too. + default: + return new DataFolderRefusal(folder, DataFolderProblem.Unreadable); + } + // An inherit-only entry applies to the entries created below the folder, not to the folder. - if (ace is KnownAce entry - && IsAllow(entry.AceType) - && (entry.AceFlags & AceFlags.InheritOnly) == 0 - && (unchecked((uint)entry.AccessMask) & ReplaceRights) != 0 - && !SecureDirectory.IsPrivileged(entry.SecurityIdentifier)) + if ((ace.AceFlags & AceFlags.InheritOnly) != 0) + { + continue; + } + + CommonAce entry = (CommonAce)ace; + + if ((unchecked((uint)entry.AccessMask) & ReplaceRights) != 0 + && !SecureDirectory.IsPrivileged(entry.SecurityIdentifier) + && entry.SecurityIdentifier != OwnerRights + && !IsAppContainer(entry.SecurityIdentifier)) { return new DataFolderRefusal(folder, DataFolderProblem.Access, entry.SecurityIdentifier); } @@ -282,10 +313,18 @@ private static DataFolderRefusal Inspect( } } - private static bool IsAllow(AceType type) => - type is AceType.AccessAllowed - or AceType.AccessAllowedCompound - or AceType.AccessAllowedObject - or AceType.AccessAllowedCallback - or AceType.AccessAllowedCallbackObject; + /// + /// Whether is an AppContainer package SID (S-1-15-2-…) or capability SID + /// (S-1-15-3-…). + /// + /// + /// An entry for one never grants access by itself: Windows grants an AppContainer process only the access that both + /// its user and its package and capabilities are granted, and no other process holds these SIDs. + /// + private static bool IsAppContainer(SecurityIdentifier sid) + { + // SECURITY_APP_PACKAGE_AUTHORITY, then SECURITY_APP_PACKAGE_BASE_RID or SECURITY_CAPABILITY_BASE_RID. + string[] parts = sid.Value.Split('-'); + return parts.Length > 3 && parts[2] == "15" && parts[3] is "2" or "3"; + } } From f6b2c40d015599f276f0af37343658be9e66db16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Tue, 27 Oct 2026 23:08:50 +0900 Subject: [PATCH 5/9] test(agent-installer): cite the data folder intent The tests of the data folder rules rest on the installer intent that now states them, rather than on other components' similar checks. Issue: DGW-478 Co-Authored-By: Claude Opus 5.5 --- .../DataFolderTests.cs | 78 +++++++++---------- .../SecureDirectoryTests.cs | 22 +++--- 2 files changed, 50 insertions(+), 50 deletions(-) diff --git a/package/AgentWindowsManaged.Tests/DataFolderTests.cs b/package/AgentWindowsManaged.Tests/DataFolderTests.cs index 1b4898b83..b5a696cba 100644 --- a/package/AgentWindowsManaged.Tests/DataFolderTests.cs +++ b/package/AgentWindowsManaged.Tests/DataFolderTests.cs @@ -53,39 +53,39 @@ public sealed class DataFolderTests public DataFolderTests(ITestOutputHelper output) => this.logger = new OutputLogger(output); [Theory] - // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("SYSTEM owns Devolutions", Devolutions, "O:SY" + InheritedDacl, "none")] - // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("Administrators own Devolutions", Devolutions, "O:BA" + InheritedDacl, "none")] - // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("Users own Devolutions", Devolutions, "O:BU" + InheritedDacl, "Owner " + Users)] - // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("Everyone owns Devolutions", Devolutions, "O:WD" + InheritedDacl, "Owner " + Everyone)] - // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("Users own Agent", Agent, "O:BU" + InheritedDacl, "Owner " + Users)] - // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("Users may delete Devolutions", Devolutions, "O:SYD:(A;;SD;;;BU)", "Access " + Users)] - // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("Users may change the DACL", Devolutions, "O:SYD:(A;;WD;;;BU)", "Access " + Users)] - // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("Users may take ownership", Devolutions, "O:SYD:(A;;WO;;;BU)", "Access " + Users)] - // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("Users may delete entries", Devolutions, "O:SYD:(A;;0x40;;;BU)", "Access " + Users)] - // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("Users have generic all", Devolutions, "O:SYD:(A;;GA;;;BU)", "Access " + Users)] - // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("Users may delete through a conditional entry", Devolutions, "O:SYD:(XA;;SD;;;BU;(Member_of {SID(BU)}))", "Access " + Users)] - // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("null DACL", Devolutions, "O:SYD:NO_ACCESS_CONTROL", "Access " + Everyone)] - // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("Users get full control of entries created below", Devolutions, "O:SYD:(A;OICIIO;FA;;;BU)", "none")] - // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("Users may only add entries", Devolutions, "O:SYD:(A;CI;DCLCRPCR;;;BU)", "none")] - // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("Users are denied deletion", Devolutions, "O:SYD:(D;;SD;;;BU)", "none")] - // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("TrustedInstaller has full control", Devolutions, "O:SYD:(A;;FA;;;S-1-5-80-956008885-3418522649-1831038044-1853292631-2271478464)", "none")] - // oracle: reference crates/now-package-broker/src/policy_security.rs verify_policy_ancestor_directory_security + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("no owner", Devolutions, "D:(A;;FA;;;SY)", "Unreadable")] // oracle: reference Devolutions/devolutions-gateway@9b8b60380f6c:crates/private-fs/src/windows/security.rs Policy::check_ancestor [InlineData("OWNER RIGHTS has full control", Devolutions, "O:SYD:(A;OICI;FA;;;OW)", "none")] @@ -101,7 +101,7 @@ public sealed class DataFolderTests [InlineData("an inherit-only object entry", Devolutions, "O:SYD:(OA;CIIO;SD;bf967aba-0de6-11d0-a285-00aa003049e2;;BU)", "Unreadable")] // oracle: reference Devolutions/devolutions-gateway@9b8b60380f6c:crates/private-fs/src/windows/security.rs Policy::check_ancestor [InlineData("Users may delete through a conditional object entry", Devolutions, "O:SYD:(ZA;;SD;bf967aba-0de6-11d0-a285-00aa003049e2;;BU;(Member_of {SID(BU)}))", "Unreadable")] - // oracle: scaffold + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("Users have full control of Agent", Agent, "O:SYD:(A;OICI;FA;;;BU)", "none")] public void RefusalFollowsTheOwnerAndTheDacl(string scenario, bool checkAccess, string sddl, string expected) { @@ -110,7 +110,7 @@ public void RefusalFollowsTheOwnerAndTheDacl(string scenario, bool checkAccess, Assert.True(expected == Describe(refusal), $"{scenario}: {Describe(refusal)}"); } - // oracle: reference crates/now-package-broker/src/policy_security.rs retain_policy_no_reparse_directory_chain + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [Theory] [InlineData(Devolutions)] [InlineData(Agent)] @@ -122,7 +122,7 @@ public void LinkIsRefusedWhateverItsSecurity(bool checkAccess) Assert.Equal("Link", Describe(refusal)); } - // oracle: reference crates/now-package-broker/src/policy_security.rs retain_policy_no_reparse_directory_chain + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [Fact] public void CheckRefusesAJunctionAtDevolutions() { @@ -145,13 +145,13 @@ public void CreateRefusesAJunctionAtDevolutionsAndCreatesNothingInItsTarget() DataFolderRefusal refusal = DataFolders.Create(folder.Path, AgentSddl, this.logger); - // oracle: reference crates/now-package-broker/src/policy_security.rs retain_policy_no_reparse_directory_chain + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder Assert.Equal((devolutions, "Link"), (refusal?.Folder, Describe(refusal))); - // oracle: scaffold + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder Assert.Empty(Directory.GetFileSystemEntries(target)); } - // oracle: scaffold + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [Fact] public void CheckRefusesADevolutionsFolderWhoseEntriesUsersMayDelete() { @@ -164,7 +164,7 @@ public void CheckRefusesADevolutionsFolderWhoseEntriesUsersMayDelete() Assert.Equal((devolutions, "Access " + Users), (refusal?.Folder, Describe(refusal))); } - // oracle: scaffold + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [Fact] public void CreateRefusesADevolutionsFolderWhoseEntriesUsersMayDeleteAndCreatesNothingInIt() { @@ -178,7 +178,7 @@ public void CreateRefusesADevolutionsFolderWhoseEntriesUsersMayDeleteAndCreatesN Assert.Empty(Directory.GetFileSystemEntries(devolutions)); } - // oracle: scaffold + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [Fact] public void CheckRefusesADevolutionsFolderWhoseSecurityItCantRead() { @@ -193,7 +193,7 @@ public void CheckRefusesADevolutionsFolderWhoseSecurityItCantRead() Assert.Equal((devolutions, "Unreadable"), (refusal?.Folder, Describe(refusal))); } - // oracle: scaffold + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [Fact] public void CheckPassesWhenTheFoldersDontExist() { @@ -202,7 +202,7 @@ public void CheckPassesWhenTheFoldersDontExist() Assert.Null(DataFolders.Check(folder.Path, this.logger)); } - // oracle: scaffold + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [Fact] public void ErrorNamesTheFolderAndTheAccountOrItsSidWhenItHasNone() { @@ -219,7 +219,7 @@ public void ErrorNamesTheFolderAndTheAccountOrItsSidWhenItHasNone() Assert.Contains(Unnamed, replaceable); } - // oracle: scaffold + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [Fact] public void ErrorIsInTheInstallerLanguage() { @@ -232,7 +232,7 @@ public void ErrorIsInTheInstallerLanguage() Assert.Equal(english, refusal.Message("1031")); } - // oracle: spec .agents/skills/installer-localization/SKILL.md §Change strings + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [Theory] [InlineData(Strings.DataFolderIsLink)] [InlineData(Strings.DataFolderOwnedByUntrustedAccount)] @@ -259,7 +259,7 @@ public void RefusalFailsTheRunInTheInstallerLanguage(string method) { ManagedAction action = ActionOf(method); - // oracle: scaffold + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder Assert.Equal(Return.check, action.Return); // A deferred action reads only the properties its action data carries. // oracle: spec Windows Installer §Obtaining Context Information for Deferred Execution Custom Actions @@ -267,19 +267,19 @@ public void RefusalFailsTheRunInTheInstallerLanguage(string method) } [Theory] - // oracle: scaffold + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("first install", true)] - // oracle: scaffold + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("major upgrade", true, "WIX_UPGRADE_DETECTED={00000000-0000-0000-0000-000000000001}")] - // oracle: scaffold + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("repair", true, "Installed=00:00:00", "REINSTALL=ALL")] - // oracle: scaffold + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("maintenance adding a feature", true, "Installed=00:00:00", "ADDLOCAL=F.Session")] - // oracle: scaffold + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("maintenance removing a feature", true, "Installed=00:00:00", "REMOVE=F.Session")] - // oracle: intent package/AgentWindowsManaged/INTENT.md §Agent Identity + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("full uninstall", false, "Installed=00:00:00", "REMOVE=ALL")] - // oracle: intent package/AgentWindowsManaged/INTENT.md §Agent Identity + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("old product removed by a major upgrade", false, "Installed=00:00:00", "REMOVE=ALL", "UPGRADINGPRODUCTCODE={00000000-0000-0000-0000-000000000002}")] public void DataFoldersAreCheckedOnEveryRunButAFullUninstall(string scenario, bool expected, params string[] properties) { @@ -290,7 +290,7 @@ public void DataFoldersAreCheckedOnEveryRunButAFullUninstall(string scenario, bo Assert.True(evaluated.All(e => e == expected), $"{scenario}: {string.Join(", ", evaluated)}"); } - // oracle: scaffold + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [Fact] public void CheckRunsBeforeTheMajorUpgradeRemovesTheInstalledProduct() { @@ -298,7 +298,7 @@ public void CheckRunsBeforeTheMajorUpgradeRemovesTheInstalledProduct() Assert.Equal(Step.RemoveExistingProducts.ToString(), AgentActions.checkProgramDataDirectory.Step.ToString()); } - // oracle: scaffold + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [Fact] public void PedmFolderIsCreatedAfterTheAgentFolder() { diff --git a/package/AgentWindowsManaged.Tests/SecureDirectoryTests.cs b/package/AgentWindowsManaged.Tests/SecureDirectoryTests.cs index c6cbc8940..68c7438cb 100644 --- a/package/AgentWindowsManaged.Tests/SecureDirectoryTests.cs +++ b/package/AgentWindowsManaged.Tests/SecureDirectoryTests.cs @@ -80,7 +80,7 @@ public void DeleteTreeDeletesNothingThroughAJunctionAboveThePath() Assert.True(File.Exists(Path.Combine(outsideIdentity, "file.txt"))); } - // oracle: scaffold + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [Fact] public void SetSecurityRefusesAJunctionAndLeavesItsTargetUnchanged() { @@ -95,7 +95,7 @@ public void SetSecurityRefusesAJunctionAndLeavesItsTargetUnchanged() Assert.Equal(before, Sddl(target)); } - // oracle: property round-trip + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [Fact] public void SetSecurityAppliesTheOwnerAndTheProtectedDacl() { @@ -107,7 +107,7 @@ public void SetSecurityAppliesTheOwnerAndTheProtectedDacl() AssertProtectedSecurity(path); } - // oracle: scaffold + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [Fact] public void SetSecurityLeavesExistingChildrenUnchanged() { @@ -121,7 +121,7 @@ public void SetSecurityLeavesExistingChildrenUnchanged() Assert.Equal(before, Sddl(child)); } - // oracle: property round-trip + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [Fact] public void CreateGivesTheNewDirectoryExactlyTheSecurityDescriptor() { @@ -134,24 +134,24 @@ public void CreateGivesTheNewDirectoryExactlyTheSecurityDescriptor() } [Theory] - // oracle: reference crates/now-package-broker/src/policy_security.rs TrustedWriters::AdminOrTrustedInstaller + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("SYSTEM", "S-1-5-18", true)] - // oracle: reference crates/agent-identity/src/windows.rs owner_is_administrator_member + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("Administrators", "S-1-5-32-544", true)] - // oracle: reference crates/now-package-broker/src/policy_security.rs TrustedWriters::AdminOrTrustedInstaller + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("TrustedInstaller", "S-1-5-80-956008885-3418522649-1831038044-1853292631-2271478464", true)] - // oracle: reference crates/agent-identity/src/windows.rs owner_is_administrator_member + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("Everyone", "S-1-1-0", false)] - // oracle: reference crates/agent-identity/src/windows.rs owner_is_administrator_member + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("Users", "S-1-5-32-545", false)] - // oracle: reference crates/agent-identity/src/windows.rs owner_is_administrator_member + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("LOCAL SERVICE", "S-1-5-19", false)] public void OwnerIsTrustedOnlyWhenPrivileged(string name, string sid, bool expected) { Assert.True(expected == SecureDirectory.IsTrustedOwner(new SecurityIdentifier(sid)), name); } - // oracle: property round-trip + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [Fact] public void ReadSecurityReadsTheOwnerAndTheDaclThatSetSecurityApplied() { From 37fa9582f56e06624c3c521fe7e71233dd5df3eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Tue, 27 Oct 2026 23:49:35 +0900 Subject: [PATCH 6/9] fix(agent-installer): hold data folders and reset Agent under the check A directory handle with only READ_CONTROL or FILE_READ_ATTRIBUTES takes no part in sharing checks, so denying delete sharing on it kept nothing from renaming the directory. Every handle that holds a directory now also requests FILE_TRAVERSE, through one helper used by the data folder check and by the Agent Identity deletion. Setting the security of a data folder now opens each folder above it without following links and holds it while the next one is opened, so a link at Devolutions can't redirect the change to another folder. An existing Agent folder gets its owner and DACL reset on the handle it was checked on, while Devolutions is held, and a failure fails the run. The reset actions now fail the run too and no longer run at a full uninstall. A file named Devolutions or Agent is refused with its own localized error instead of failing later. Issue: DGW-478 Co-Authored-By: Claude Opus 5.5 --- .../DataFolderTests.cs | 115 ++++++++++++++---- .../SecureDirectoryTests.cs | 111 +++++++++++++++-- .../Actions/AgentActions.cs | 10 +- .../Actions/CustomActions.cs | 18 ++- .../Actions/DataFolders.cs | 83 +++++++++---- .../Actions/SecureDirectory.cs | 115 ++++++++++++++---- package/AgentWindowsManaged/Actions/WinAPI.cs | 2 + .../Resources/DevolutionsAgent_en-us.wxl | 1 + .../Resources/DevolutionsAgent_fr-fr.wxl | 1 + .../Resources/DevolutionsAgent_sv-se.wxl | 1 + .../Resources/Strings.g.cs | 4 + .../Resources/Strings_en-US.json | 4 + .../Resources/Strings_fr-FR.json | 4 + .../Resources/Strings_sv-SE.json | 4 + 14 files changed, 386 insertions(+), 87 deletions(-) diff --git a/package/AgentWindowsManaged.Tests/DataFolderTests.cs b/package/AgentWindowsManaged.Tests/DataFolderTests.cs index b5a696cba..94912f931 100644 --- a/package/AgentWindowsManaged.Tests/DataFolderTests.cs +++ b/package/AgentWindowsManaged.Tests/DataFolderTests.cs @@ -35,17 +35,14 @@ public sealed class DataFolderTests private static readonly SecurityIdentifier CurrentUser = WindowsIdentity.GetCurrent().User; - // Protected, so that the folder inherits nothing from the temporary directory, which grants the current user full - // control, and owned by the current user, who can't make another account the owner without elevation. - private static readonly string ReplaceableByUsersSddl = - $"O:{CurrentUser}D:P(A;OICI;FA;;;SY)(A;OICI;FA;;;BA)(A;OICI;0x1200a9;;;BU)(A;;SD;;;BU)"; - private static readonly string AgentSddl = $"O:{CurrentUser}D:P(A;OICI;FA;;;{CurrentUser})(A;OICI;FA;;;SY)"; private static readonly string[] DataFolderActions = [ nameof(CustomActions.CheckProgramDataDirectory), nameof(CustomActions.CreateProgramDataDirectory), + nameof(CustomActions.SetProgramDataDirectoryPermissions), + nameof(CustomActions.SetProgramDataPedmDirectoryPermissions), ]; private readonly ILogger logger; @@ -63,25 +60,25 @@ public sealed class DataFolderTests [InlineData("Everyone owns Devolutions", Devolutions, "O:WD" + InheritedDacl, "Owner " + Everyone)] // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("Users own Agent", Agent, "O:BU" + InheritedDacl, "Owner " + Users)] - // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder + // oracle: reference Devolutions/devolutions-gateway@9b8b60380f6c:crates/private-fs/src/windows/security.rs Policy::check_ancestor [InlineData("Users may delete Devolutions", Devolutions, "O:SYD:(A;;SD;;;BU)", "Access " + Users)] - // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder + // oracle: reference Devolutions/devolutions-gateway@9b8b60380f6c:crates/private-fs/src/windows/security.rs Policy::check_ancestor [InlineData("Users may change the DACL", Devolutions, "O:SYD:(A;;WD;;;BU)", "Access " + Users)] - // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder + // oracle: reference Devolutions/devolutions-gateway@9b8b60380f6c:crates/private-fs/src/windows/security.rs Policy::check_ancestor [InlineData("Users may take ownership", Devolutions, "O:SYD:(A;;WO;;;BU)", "Access " + Users)] - // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder + // oracle: reference Devolutions/devolutions-gateway@9b8b60380f6c:crates/private-fs/src/windows/security.rs Policy::check_ancestor [InlineData("Users may delete entries", Devolutions, "O:SYD:(A;;0x40;;;BU)", "Access " + Users)] - // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder + // oracle: reference Devolutions/devolutions-gateway@9b8b60380f6c:crates/private-fs/src/windows/security.rs Policy::check_ancestor [InlineData("Users have generic all", Devolutions, "O:SYD:(A;;GA;;;BU)", "Access " + Users)] - // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder + // oracle: reference Devolutions/devolutions-gateway@9b8b60380f6c:crates/private-fs/src/windows/security.rs Policy::check_ancestor [InlineData("Users may delete through a conditional entry", Devolutions, "O:SYD:(XA;;SD;;;BU;(Member_of {SID(BU)}))", "Access " + Users)] // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("null DACL", Devolutions, "O:SYD:NO_ACCESS_CONTROL", "Access " + Everyone)] - // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder + // oracle: reference Devolutions/devolutions-gateway@9b8b60380f6c:crates/private-fs/src/windows/security.rs Policy::check_ancestor [InlineData("Users get full control of entries created below", Devolutions, "O:SYD:(A;OICIIO;FA;;;BU)", "none")] - // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder + // oracle: reference Devolutions/devolutions-gateway@9b8b60380f6c:crates/private-fs/src/windows/security.rs Policy::check_ancestor [InlineData("Users may only add entries", Devolutions, "O:SYD:(A;CI;DCLCRPCR;;;BU)", "none")] - // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder + // oracle: reference Devolutions/devolutions-gateway@9b8b60380f6c:crates/private-fs/src/windows/security.rs Policy::check_ancestor [InlineData("Users are denied deletion", Devolutions, "O:SYD:(D;;SD;;;BU)", "none")] // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("TrustedInstaller has full control", Devolutions, "O:SYD:(A;;FA;;;S-1-5-80-956008885-3418522649-1831038044-1853292631-2271478464)", "none")] @@ -105,7 +102,8 @@ public sealed class DataFolderTests [InlineData("Users have full control of Agent", Agent, "O:SYD:(A;OICI;FA;;;BU)", "none")] public void RefusalFollowsTheOwnerAndTheDacl(string scenario, bool checkAccess, string sddl, string expected) { - DataFolderRefusal refusal = DataFolders.Evaluate("folder", new RawSecurityDescriptor(sddl), false, checkAccess); + DataFolderRefusal refusal = DataFolders.Evaluate( + "folder", new RawSecurityDescriptor(sddl), isLink: false, isDirectory: true, checkAccess); Assert.True(expected == Describe(refusal), $"{scenario}: {Describe(refusal)}"); } @@ -117,11 +115,61 @@ public void RefusalFollowsTheOwnerAndTheDacl(string scenario, bool checkAccess, public void LinkIsRefusedWhateverItsSecurity(bool checkAccess) { DataFolderRefusal refusal = DataFolders.Evaluate( - "folder", new RawSecurityDescriptor("O:SY" + InheritedDacl), true, checkAccess); + "folder", new RawSecurityDescriptor("O:SY" + InheritedDacl), isLink: true, isDirectory: true, checkAccess); Assert.Equal("Link", Describe(refusal)); } + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder + [Theory] + [InlineData(Devolutions)] + [InlineData(Agent)] + public void FileIsRefusedWhateverItsSecurity(bool checkAccess) + { + DataFolderRefusal refusal = DataFolders.Evaluate( + "folder", new RawSecurityDescriptor("O:SY" + InheritedDacl), isLink: false, isDirectory: false, checkAccess); + + Assert.Equal("File", Describe(refusal)); + } + + // The agent checks Agent as a directory above its data, so the security the installer gives Agent must pass that + // check. + // oracle: reference Devolutions/devolutions-gateway@9b8b60380f6c:crates/private-fs/src/windows/security.rs Policy::check_ancestor + [Fact] + public void AgentFolderSecurityPassesTheDaclRule() + { + DataFolderRefusal refusal = DataFolders.Evaluate( + "folder", new RawSecurityDescriptor(Includes.PROGRAM_DATA_SDDL), isLink: false, isDirectory: true, Devolutions); + + Assert.Equal("none", Describe(refusal)); + } + + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder + [Fact] + public void CheckRefusesAFileAtDevolutions() + { + using TempFolder folder = new(); + string devolutions = Path.Combine(folder.Path, "Devolutions"); + System.IO.File.WriteAllText(devolutions, ""); + + DataFolderRefusal refusal = DataFolders.Check(folder.Path, this.logger); + + Assert.Equal((devolutions, "File"), (refusal?.Folder, Describe(refusal))); + } + + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder + [Fact] + public void CreateRefusesAFileAtDevolutionsAndLeavesIt() + { + using TempFolder folder = new(); + string devolutions = Path.Combine(folder.Path, "Devolutions"); + System.IO.File.WriteAllText(devolutions, "kept"); + + DataFolderRefusal refusal = DataFolders.Create(folder.Path, AgentSddl, this.logger); + + Assert.Equal((devolutions, "File", "kept"), (refusal?.Folder, Describe(refusal), System.IO.File.ReadAllText(devolutions))); + } + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [Fact] public void CheckRefusesAJunctionAtDevolutions() @@ -151,13 +199,16 @@ public void CreateRefusesAJunctionAtDevolutionsAndCreatesNothingInItsTarget() Assert.Empty(Directory.GetFileSystemEntries(target)); } + [Theory] // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder - [Fact] - public void CheckRefusesADevolutionsFolderWhoseEntriesUsersMayDelete() + [InlineData("Users may delete Devolutions", "SD")] + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder + [InlineData("Users may delete entries of Devolutions", "0x40")] + public void CheckRefusesADevolutionsFolderThatUsersMayReplaceOrWhoseEntriesTheyMayReplace(string scenario, string right) { using TempFolder folder = new(); string devolutions = Path.Combine(folder.Path, "Devolutions"); - Assert.True(SecureDirectory.Create(devolutions, ReplaceableByUsersSddl)); + Assert.True(SecureDirectory.Create(devolutions, ReplaceableByUsers(right)), scenario); DataFolderRefusal refusal = DataFolders.Check(folder.Path, this.logger); @@ -166,11 +217,11 @@ public void CheckRefusesADevolutionsFolderWhoseEntriesUsersMayDelete() // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [Fact] - public void CreateRefusesADevolutionsFolderWhoseEntriesUsersMayDeleteAndCreatesNothingInIt() + public void CreateRefusesADevolutionsFolderThatUsersMayDeleteAndCreatesNothingInIt() { using TempFolder folder = new(); string devolutions = Path.Combine(folder.Path, "Devolutions"); - Assert.True(SecureDirectory.Create(devolutions, ReplaceableByUsersSddl)); + Assert.True(SecureDirectory.Create(devolutions, ReplaceableByUsers("SD"))); DataFolderRefusal refusal = DataFolders.Create(folder.Path, AgentSddl, this.logger); @@ -235,6 +286,7 @@ public void ErrorIsInTheInstallerLanguage() // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [Theory] [InlineData(Strings.DataFolderIsLink)] + [InlineData(Strings.DataFolderIsFile)] [InlineData(Strings.DataFolderOwnedByUntrustedAccount)] [InlineData(Strings.DataFolderReplaceableByUntrustedAccount)] [InlineData(Strings.DataFolderSecurityUnreadable)] @@ -266,6 +318,15 @@ public void RefusalFailsTheRunInTheInstallerLanguage(string method) Assert.Contains("ProductLanguage=[ProductLanguage]", action.UsesProperties.Split(';')); } + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder + [Theory] + [InlineData(nameof(CustomActions.SetProgramDataDirectoryPermissions))] + [InlineData(nameof(CustomActions.SetProgramDataPedmDirectoryPermissions))] + public void FailedSecurityResetFailsTheRun(string method) + { + Assert.Equal(Return.check, ActionOf(method).Return); + } + [Theory] // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("first install", true)] @@ -281,7 +342,7 @@ public void RefusalFailsTheRunInTheInstallerLanguage(string method) [InlineData("full uninstall", false, "Installed=00:00:00", "REMOVE=ALL")] // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [InlineData("old product removed by a major upgrade", false, "Installed=00:00:00", "REMOVE=ALL", "UPGRADINGPRODUCTCODE={00000000-0000-0000-0000-000000000002}")] - public void DataFoldersAreCheckedOnEveryRunButAFullUninstall(string scenario, bool expected, params string[] properties) + public void DataFoldersAreCheckedAndResetOnEveryRunButAFullUninstall(string scenario, bool expected, params string[] properties) { bool[] evaluated = DataFolderActions .Select(method => EvaluateCondition(ActionOf(method).Condition.ToString(), properties)) @@ -307,6 +368,16 @@ public void PedmFolderIsCreatedAfterTheAgentFolder() Assert.Equal(AgentActions.createProgramDataDirectory.Id, AgentActions.createProgramDataPedmDirectories.Step.ToString()); } + /// + /// A security descriptor that grants Users on the folder. + /// + /// + /// Protected, so that the folder inherits nothing from the temporary directory, which grants the current user full + /// control, and owned by the current user, who can't make another account the owner without elevation. + /// + private static string ReplaceableByUsers(string right) => + $"O:{CurrentUser}D:P(A;OICI;FA;;;SY)(A;OICI;FA;;;BA)(A;OICI;0x1200a9;;;BU)(A;;{right};;;BU)"; + /// /// The problem and the account names, or none. /// diff --git a/package/AgentWindowsManaged.Tests/SecureDirectoryTests.cs b/package/AgentWindowsManaged.Tests/SecureDirectoryTests.cs index 68c7438cb..352efcd1b 100644 --- a/package/AgentWindowsManaged.Tests/SecureDirectoryTests.cs +++ b/package/AgentWindowsManaged.Tests/SecureDirectoryTests.cs @@ -80,6 +80,44 @@ public void DeleteTreeDeletesNothingThroughAJunctionAboveThePath() Assert.True(File.Exists(Path.Combine(outsideIdentity, "file.txt"))); } + // oracle: intent package/AgentWindowsManaged/INTENT.md §Agent Identity + [Fact] + public void DeleteTreeKeepsGoingAfterAFailureAndKeepsOnlyWhatItCouldntDelete() + { + using TempFolder folder = new(); + string identity = folder.CreateDirectory(IdentityNames); + string pending = folder.CreateDirectory([.. IdentityNames, "pending"]); + string held = Path.Combine(pending, "held.dat"); + File.WriteAllText(held, "kept"); + File.WriteAllText(Path.Combine(pending, "sibling.dat"), "deleted"); + File.WriteAllText(Path.Combine(identity, "other.dat"), "deleted"); + + // Opened without delete sharing, the file can't be deleted while the stream is open. + using (new FileStream(held, FileMode.Open, FileAccess.Read, FileShare.ReadWrite)) + { + SecureDirectory.DeleteTree(folder.Path, IdentityNames, this.logger); + } + + Assert.Equal([held], Directory.GetFileSystemEntries(identity, "*", SearchOption.AllDirectories) + .Where(File.Exists) + .ToArray()); + Assert.Equal([pending], Directory.GetDirectories(identity)); + } + + // oracle: intent package/AgentWindowsManaged/INTENT.md §Agent Identity + [Fact] + public void HeldDirectoryCantBeRenamedUntilItsHandleIsClosed() + { + AssertHoldsUntilClosed(path => SecureDirectory.Hold(path)); + } + + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder + [Fact] + public void DirectoryOpenedToReadAndHoldCantBeRenamedUntilItsHandleIsClosed() + { + AssertHoldsUntilClosed(path => SecureDirectory.OpenToRead(path, hold: true)); + } + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [Fact] public void SetSecurityRefusesAJunctionAndLeavesItsTargetUnchanged() @@ -87,36 +125,66 @@ public void SetSecurityRefusesAJunctionAndLeavesItsTargetUnchanged() using TempFolder folder = new(); string target = folder.CreateDirectory("target"); string before = Sddl(target); - string link = Path.Combine(folder.Path, "link"); - TempFolder.CreateJunction(link, target); + TempFolder.CreateJunction(Path.Combine(folder.Path, "link"), target); - Assert.Throws(() => SecureDirectory.SetSecurity(link, ProtectedSddl)); + Assert.Throws(() => SecureDirectory.SetSecurity(folder.Path, ["link"], ProtectedSddl)); Assert.Equal(before, Sddl(target)); } + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder + [Fact] + public void SetSecurityRefusesAJunctionAboveTheFolderAndLeavesTheFolderBelowItsTargetUnchanged() + { + using TempFolder folder = new(); + string target = folder.CreateDirectory("target"); + string below = folder.CreateDirectory("target", "data"); + string before = Sddl(below); + TempFolder.CreateJunction(Path.Combine(folder.Path, "link"), target); + + Assert.Throws(() => SecureDirectory.SetSecurity(folder.Path, ["link", "data"], ProtectedSddl)); + + Assert.Equal(before, Sddl(below)); + } + + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder + [Fact] + public void SetSecurityRefusesAFileAndLeavesItUnchanged() + { + using TempFolder folder = new(); + string path = Path.Combine(folder.Path, "data"); + File.WriteAllText(path, ""); + string before = File.GetAccessControl(path).GetSecurityDescriptorSddlForm(AccessControlSections.All); + + Assert.Throws(() => SecureDirectory.SetSecurity(folder.Path, ["data"], ProtectedSddl)); + + Assert.Equal(before, File.GetAccessControl(path).GetSecurityDescriptorSddlForm(AccessControlSections.All)); + } + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [Fact] public void SetSecurityAppliesTheOwnerAndTheProtectedDacl() { using TempFolder folder = new(); - string path = folder.CreateDirectory("data"); + string path = folder.CreateDirectory("parent", "data"); - SecureDirectory.SetSecurity(path, ProtectedSddl); + SecureDirectory.SetSecurity(folder.Path, ["parent", "data"], ProtectedSddl); AssertProtectedSecurity(path); } - // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder + // The agent requires its directories to have exactly the DACL it provisions, so propagated entries would make it + // refuse them. + // oracle: intent crates/agent-identity/INTENT.md §Local state [Fact] public void SetSecurityLeavesExistingChildrenUnchanged() { using TempFolder folder = new(); - string path = folder.CreateDirectory("data"); + folder.CreateDirectory("data"); string child = folder.CreateDirectory("data", "child"); string before = Sddl(child); - SecureDirectory.SetSecurity(path, ProtectedSddl); + SecureDirectory.SetSecurity(folder.Path, ["data"], ProtectedSddl); Assert.Equal(before, Sddl(child)); } @@ -151,19 +219,40 @@ public void OwnerIsTrustedOnlyWhenPrivileged(string name, string sid, bool expec Assert.True(expected == SecureDirectory.IsTrustedOwner(new SecurityIdentifier(sid)), name); } - // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder + // oracle: property round-trip [Fact] public void ReadSecurityReadsTheOwnerAndTheDaclThatSetSecurityApplied() { using TempFolder folder = new(); string path = folder.CreateDirectory("data"); - SecureDirectory.SetSecurity(path, ProtectedSddl); + SecureDirectory.SetSecurity(folder.Path, ["data"], ProtectedSddl); using SafeFileHandle handle = SecureDirectory.OpenToRead(path, hold: false); - (RawSecurityDescriptor security, bool isReparsePoint) = SecureDirectory.ReadSecurity(handle); + (RawSecurityDescriptor security, bool isReparsePoint, bool isDirectory) = SecureDirectory.ReadSecurity(handle); Assert.Equal(ProtectedSddl, security.GetSddlForm(AccessControlSections.Owner | AccessControlSections.Access)); Assert.False(isReparsePoint); + Assert.True(isDirectory); + } + + /// + /// Asserts that a directory opens can't be renamed while the handle is open, and can once it + /// is closed. + /// + private static void AssertHoldsUntilClosed(Func hold) + { + using TempFolder folder = new(); + string path = folder.CreateDirectory("held"); + string renamed = Path.Combine(folder.Path, "renamed"); + + using (hold(path)) + { + Assert.Throws(() => Directory.Move(path, renamed)); + Assert.True(Directory.Exists(path)); + } + + Directory.Move(path, renamed); + Assert.True(Directory.Exists(renamed)); } /// diff --git a/package/AgentWindowsManaged/Actions/AgentActions.cs b/package/AgentWindowsManaged/Actions/AgentActions.cs index c53b0723e..ccf722921 100644 --- a/package/AgentWindowsManaged/Actions/AgentActions.cs +++ b/package/AgentWindowsManaged/Actions/AgentActions.cs @@ -173,9 +173,9 @@ internal static class AgentActions private static readonly ElevatedManagedAction setProgramDataDirectoryPermissions = new( new Id($"CA.{nameof(setProgramDataDirectoryPermissions)}"), CustomActions.SetProgramDataDirectoryPermissions, - Return.ignore, + Return.check, When.After, new Step(createProgramDataDirectory.Id), - Condition.Always, + new Condition("NOT (REMOVE=\"ALL\")"), Sequence.InstallExecuteSequence) { Execute = Execute.deferred, @@ -183,14 +183,14 @@ internal static class AgentActions }; /// - /// Set or reset the ACL on %ProgramData%\Devolutions\Agent\pedm + /// Set or reset the ACL on %ProgramData%\Devolutions\Agent\pedm, when it exists /// private static readonly ElevatedManagedAction setProgramDataPedmDirectoryPermissions = new( new Id($"CA.{nameof(setProgramDataPedmDirectoryPermissions)}"), CustomActions.SetProgramDataPedmDirectoryPermissions, - Return.ignore, + Return.check, When.After, new Step(createProgramDataPedmDirectories.Id), - Condition.Always, + new Condition("NOT (REMOVE=\"ALL\")"), Sequence.InstallExecuteSequence) { Execute = Execute.deferred, diff --git a/package/AgentWindowsManaged/Actions/CustomActions.cs b/package/AgentWindowsManaged/Actions/CustomActions.cs index c0ce90757..0c23053cd 100644 --- a/package/AgentWindowsManaged/Actions/CustomActions.cs +++ b/package/AgentWindowsManaged/Actions/CustomActions.cs @@ -1928,7 +1928,10 @@ public static ActionResult SetProgramDataDirectoryPermissions(Session session) { try { - SecureDirectory.SetSecurity(ProgramDataDirectory, Includes.PROGRAM_DATA_SDDL); + SecureDirectory.SetSecurity( + Environment.GetFolderPath(Environment.SpecialFolder.CommonApplicationData), + ["Devolutions", "Agent"], + Includes.PROGRAM_DATA_SDDL); return ActionResult.Success; } catch (Exception e) @@ -1943,7 +1946,18 @@ public static ActionResult SetProgramDataPedmDirectoryPermissions(Session sessio { try { - SecureDirectory.SetSecurity(Path.Combine(ProgramDataDirectory, "pedm"), Includes.PROGRAM_DATA_PEDM_SDDL); + SecureDirectory.SetSecurity( + Environment.GetFolderPath(Environment.SpecialFolder.CommonApplicationData), + ["Devolutions", "Agent", "pedm"], + Includes.PROGRAM_DATA_PEDM_SDDL); + return ActionResult.Success; + } + catch (Win32Exception e) when (e.NativeErrorCode == WinAPI.ERROR_FILE_NOT_FOUND) + { + // UNTESTED: a missing pedm folder doesn't fail the run, as only the PEDM feature creates it. + // No test runs this action, which reads the machine's own ProgramData. + // When changing this, keep accepting a missing pedm folder and nothing else that fails to open. + session.Log($"no pedm folder to set permissions on: {e.Message}"); return ActionResult.Success; } catch (Exception e) diff --git a/package/AgentWindowsManaged/Actions/DataFolders.cs b/package/AgentWindowsManaged/Actions/DataFolders.cs index 1622f0e9c..4a94e650d 100644 --- a/package/AgentWindowsManaged/Actions/DataFolders.cs +++ b/package/AgentWindowsManaged/Actions/DataFolders.cs @@ -18,6 +18,11 @@ internal enum DataFolderProblem /// Link, + /// + /// The entry is a file, not a folder. + /// + File, + /// /// The folder's owner, who can always change its permissions, is not trusted. /// @@ -57,6 +62,7 @@ internal string Message(string productLanguage) string id = this.Problem switch { DataFolderProblem.Link => Strings.DataFolderIsLink, + DataFolderProblem.File => Strings.DataFolderIsFile, DataFolderProblem.Owner => Strings.DataFolderOwnedByUntrustedAccount, DataFolderProblem.Access => Strings.DataFolderReplaceableByUntrustedAccount, DataFolderProblem.Unreadable => Strings.DataFolderSecurityUnreadable, @@ -119,7 +125,9 @@ internal static class DataFolders internal static DataFolderRefusal Check(string root, ILogger logger) { string devolutions = Path.Combine(root, Devolutions); - DataFolderRefusal refusal = Inspect(devolutions, hold: true, checkAccess: true, logger, out SafeFileHandle handle); + DataFolderRefusal refusal = Inspect( + devolutions, path => SecureDirectory.OpenToRead(path, hold: true), checkAccess: true, logger, + out SafeFileHandle handle); using (handle) { @@ -132,7 +140,9 @@ internal static DataFolderRefusal Check(string root, ILogger logger) // A test reaches it only below a Devolutions folder that passes, which needs an owner that a test without // elevation can't set. // When changing this, keep Agent checked in both, without the DACL rule, as the installer resets its DACL. - refusal = Inspect(Path.Combine(devolutions, Agent), hold: false, checkAccess: false, logger, out SafeFileHandle agent); + refusal = Inspect( + Path.Combine(devolutions, Agent), path => SecureDirectory.OpenToRead(path, hold: false), + checkAccess: false, logger, out SafeFileHandle agent); agent?.Dispose(); return refusal; @@ -141,27 +151,31 @@ internal static DataFolderRefusal Check(string root, ILogger logger) /// /// Creates Devolutions\Agent under with the security descriptor - /// , after checking Devolutions and, when it already exists, Agent. + /// , after checking Devolutions, or checks an existing Agent and resets its owner + /// and DACL to those of . /// /// /// A missing Devolutions is created with the DACL it inherits, then checked like an existing one. /// Agent gets its security descriptor when it is created, so that it never exists with the DACL it would /// inherit from ProgramData, which lets users add entries. /// - /// The first refusal, in which case nothing is created below Devolutions, or null. - /// Agent exists and is not a directory. + /// + /// The first refusal, in which case nothing is created or changed below Devolutions, or null. + /// internal static DataFolderRefusal Create(string root, string sddl, ILogger logger) { string devolutions = Path.Combine(root, Devolutions); string agent = Path.Combine(devolutions, Agent); - Directory.CreateDirectory(devolutions); + SecureDirectory.Create(devolutions, sddl: null); - // UNTESTED: the handle on Devolutions denies delete sharing until Agent is created or checked below it. - // Renaming or replacing a folder needs delete access to it, so Agent is created or checked in the folder just - // checked, not in one put at that path afterwards. - // When changing this, keep the share mode of that handle and keep it open until Agent is done. - DataFolderRefusal refusal = Inspect(devolutions, hold: true, checkAccess: true, logger, out SafeFileHandle handle); + // UNTESTED: the handle on Devolutions stays open until Agent is created, or checked and reset, below it, so that + // this happens in the folder just checked, not in one put at that path afterwards. + // Tests check that a held folder can't be renamed, not how long this handle stays open. + // When changing this, keep the handle open until Agent is done. + DataFolderRefusal refusal = Inspect( + devolutions, path => SecureDirectory.OpenToRead(path, hold: true), checkAccess: true, logger, + out SafeFileHandle handle); using (handle) { @@ -181,15 +195,30 @@ internal static DataFolderRefusal Create(string root, string sddl, ILogger logge return null; } - refusal = Inspect(agent, hold: false, checkAccess: false, logger, out SafeFileHandle existing); - existing?.Dispose(); + refusal = Inspect( + agent, SecureDirectory.OpenToSetSecurity, checkAccess: false, logger, out SafeFileHandle existing); - if (refusal is null && !Directory.Exists(agent)) + using (existing) { - throw new IOException($"{agent} exists and is not a directory"); - } + if (refusal is not null) + { + return refusal; + } - return refusal; + if (existing is null) + { + throw new IOException($"{agent} was deleted after it was found"); + } + + // UNTESTED: setting the owner to SYSTEM succeeds only for a caller allowed to give an object another + // owner, as the installer's SYSTEM process is; tests run as a user and set the user as the owner. + // When changing this, keep resetting the security of an existing Agent on this handle, as the agent + // refuses a data folder whose DACL isn't the one provisioned. + SecureDirectory.SetSecurity(existing, agent, sddl); + logger.Log($"reset the owner and the DACL of {agent}"); + + return null; + } } } @@ -202,18 +231,23 @@ internal static DataFolderRefusal Create(string root, string sddl, ILogger logge /// a folder the agent then refuses to use. /// /// - /// Whether to refuse a DACL that lets an account other than SYSTEM, Administrators, TrustedInstaller or the owner + /// Whether to refuse a DACL that lets an account other than SYSTEM, Administrators, TrustedInstaller or OWNER RIGHTS /// replace the folder or what it contains. /// /// The refusal, or null when the folder passes. internal static DataFolderRefusal Evaluate( - string folder, RawSecurityDescriptor security, bool isLink, bool checkAccess) + string folder, RawSecurityDescriptor security, bool isLink, bool isDirectory, bool checkAccess) { if (isLink) { return new DataFolderRefusal(folder, DataFolderProblem.Link); } + if (!isDirectory) + { + return new DataFolderRefusal(folder, DataFolderProblem.File); + } + if (security.Owner is null) { return new DataFolderRefusal(folder, DataFolderProblem.Unreadable); @@ -281,20 +315,21 @@ internal static DataFolderRefusal Evaluate( } /// - /// Opens the entry names, a link itself rather than its target, and evaluates its security. + /// Opens the entry names with , which opens a link itself rather than + /// its target, and evaluates its security. /// /// The open entry, or null when nothing exists at . /// The refusal, or null when the entry passes or doesn't exist. private static DataFolderRefusal Inspect( - string path, bool hold, bool checkAccess, ILogger logger, out SafeFileHandle handle) + string path, Func open, bool checkAccess, ILogger logger, out SafeFileHandle handle) { handle = null; try { - handle = SecureDirectory.OpenToRead(path, hold); - (RawSecurityDescriptor security, bool isLink) = SecureDirectory.ReadSecurity(handle); - DataFolderRefusal refusal = Evaluate(path, security, isLink, checkAccess); + handle = open(path); + (RawSecurityDescriptor security, bool isLink, bool isDirectory) = SecureDirectory.ReadSecurity(handle); + DataFolderRefusal refusal = Evaluate(path, security, isLink, isDirectory, checkAccess); logger.Log(refusal is null ? $"checked {path}" diff --git a/package/AgentWindowsManaged/Actions/SecureDirectory.cs b/package/AgentWindowsManaged/Actions/SecureDirectory.cs index c6b33bbc2..3ecb6d53e 100644 --- a/package/AgentWindowsManaged/Actions/SecureDirectory.cs +++ b/package/AgentWindowsManaged/Actions/SecureDirectory.cs @@ -27,27 +27,27 @@ internal static class SecureDirectory private static readonly SecurityIdentifier TrustedInstaller = new("S-1-5-80-956008885-3418522649-1831038044-1853292631-2271478464"); - // UNTESTED: the deletion holds each directory handle, without FILE_SHARE_DELETE, until the entries below the - // directory are done. - // Renaming or replacing a directory needs delete access to it, so no path below a held directory can be redirected - // to another folder. - // When changing how the deletion opens or closes a directory, keep this share mode and keep the handle open that long. + /// + /// The share mode of a handle that holds its entry: renaming or replacing an entry needs delete access to it, which + /// this denies, so no path through a held directory can be redirected to another folder. + /// private static readonly uint HeldShare = WinAPI.FILE_SHARE_READ | WinAPI.FILE_SHARE_WRITE; private static readonly uint AnyShare = WinAPI.FILE_SHARE_READ | WinAPI.FILE_SHARE_WRITE | WinAPI.FILE_SHARE_DELETE; /// /// Creates the directory with the security descriptor , so that it - /// never exists with the DACL it would inherit from its parent. + /// never exists with the DACL it would inherit from its parent, or with the security it inherits when + /// is null. /// /// false when an entry named already exists, which is left unchanged. internal static bool Create(string path, string sddl) { - using SecurityDescriptor descriptor = new(sddl); + using SecurityDescriptor descriptor = sddl is null ? null : new(sddl); WinAPI.SECURITY_ATTRIBUTES attributes = new() { nLength = (uint)Marshal.SizeOf(), - lpSecurityDescriptor = descriptor.Pointer, + lpSecurityDescriptor = descriptor?.Pointer ?? IntPtr.Zero, }; if (WinAPI.CreateDirectory(path, ref attributes)) @@ -66,18 +66,66 @@ internal static bool Create(string path, string sddl) } /// - /// Sets the owner and the DACL of the directory to those of . + /// Sets the owner and the DACL of the directory + /// \[0]\…\[n-1] to those of + /// , without following any link. + /// + /// + /// Each directory above the target is held while the next one is opened, so the target is below the directories + /// checked. + /// + /// An entry on the path is a reparse point or not a directory. + internal static void SetSecurity(string root, string[] names, string sddl) + { + List parents = new(); + + try + { + string path = root; + + foreach (string name in names.Take(names.Length - 1)) + { + path = Path.Combine(path, name); + SafeFileHandle parent = Hold(path); + parents.Add(parent); + + if (!IsPlainDirectory(GetAttributes(parent))) + { + throw new IOException($"{path} is a reparse point or not a directory"); + } + } + + string target = Path.Combine(path, names[names.Length - 1]); + using SafeFileHandle handle = OpenToSetSecurity(target); + SetSecurity(handle, target, sddl); + } + finally + { + parents.ForEach(parent => parent.Dispose()); + } + } + + /// + /// Opens the entry names, a link itself rather than its target, to read its security and to + /// set it with . + /// + internal static SafeFileHandle OpenToSetSecurity(string path) => + Open( + path, + WinAPI.READ_CONTROL | WinAPI.WRITE_DAC | WinAPI.WRITE_OWNER | WinAPI.FILE_READ_ATTRIBUTES, + AnyShare); + + /// + /// Sets the owner and the DACL of the directory holds, opened from + /// by , to those of . /// /// /// Only the directory changes: unlike SetSecurityInfo, this doesn't propagate inheritable entries to existing /// children, so the entries below keep the security they have. /// - /// is a reparse point or not a directory. - internal static void SetSecurity(string path, string sddl) + /// The entry is a reparse point or not a directory. + internal static void SetSecurity(SafeFileHandle handle, string path, string sddl) { - using SafeFileHandle handle = Open( - path, WinAPI.WRITE_DAC | WinAPI.WRITE_OWNER | WinAPI.FILE_READ_ATTRIBUTES, AnyShare); - if (!IsPlainDirectory(GetAttributes(handle))) { throw new IOException($"{path} is a reparse point or not a directory"); @@ -97,18 +145,34 @@ internal static void SetSecurity(string path, string sddl) /// /// Opens the entry names, a link itself rather than its target, to read its security. /// - /// - /// Whether to deny delete sharing, so that the entry can't be renamed or replaced while the handle is open. - /// + /// Whether to hold the entry as does. internal static SafeFileHandle OpenToRead(string path, bool hold) => - Open(path, WinAPI.READ_CONTROL | WinAPI.FILE_READ_ATTRIBUTES, hold ? HeldShare : AnyShare); + hold + ? Hold(path, WinAPI.READ_CONTROL) + : Open(path, WinAPI.READ_CONTROL | WinAPI.FILE_READ_ATTRIBUTES, AnyShare); /// - /// The owner and the DACL of the entry holds, and whether that entry is a reparse point. + /// Opens the directory names, a link itself rather than its target, with + /// , so that it can't be renamed or replaced while the handle is open. /// - internal static (RawSecurityDescriptor Security, bool IsReparsePoint) ReadSecurity(SafeFileHandle handle) + /// + /// A share mode binds only a handle with an access right that takes part in sharing checks, which + /// READ_CONTROL and FILE_READ_ATTRIBUTES don't, so the handle also has FILE_TRAVERSE, the right + /// that resolving a path below the directory needs. + /// + internal static SafeFileHandle Hold(string path, uint access = 0) => + Open(path, access | WinAPI.FILE_TRAVERSE | WinAPI.FILE_READ_ATTRIBUTES, HeldShare); + + /// + /// The owner and the DACL of the entry holds, whether that entry is a reparse point, and + /// whether it is a directory. + /// + internal static (RawSecurityDescriptor Security, bool IsReparsePoint, bool IsDirectory) ReadSecurity( + SafeFileHandle handle) { - bool isReparsePoint = (GetAttributes(handle) & WinAPI.FILE_ATTRIBUTE_REPARSE_POINT) != 0; + uint attributes = GetAttributes(handle); + bool isReparsePoint = (attributes & WinAPI.FILE_ATTRIBUTE_REPARSE_POINT) != 0; + bool isDirectory = (attributes & WinAPI.FILE_ATTRIBUTE_DIRECTORY) != 0; const uint information = WinAPI.OWNER_SECURITY_INFORMATION | WinAPI.DACL_SECURITY_INFORMATION; // The first call only reports the size the second needs. @@ -127,7 +191,7 @@ internal static (RawSecurityDescriptor Security, bool IsReparsePoint) ReadSecuri throw new Win32Exception(Marshal.GetLastWin32Error()); } - return (new RawSecurityDescriptor(descriptor, 0), isReparsePoint); + return (new RawSecurityDescriptor(descriptor, 0), isReparsePoint, isDirectory); } /// @@ -184,6 +248,11 @@ internal static bool IsTrustedOwner(SecurityIdentifier owner) internal static void DeleteTree(string root, string[] names, ILogger logger) { string target = Path.Combine([root, .. names]); + + // UNTESTED: each directory handle stays open until the entries below the directory are done, so that no path + // below it can be redirected while the deletion runs. + // Tests check that a held directory can't be renamed, not when the deletion closes its handle. + // When changing how the deletion opens or closes a directory, keep each handle open that long. List handles = new(); try @@ -193,7 +262,7 @@ internal static void DeleteTree(string root, string[] names, ILogger logger) foreach (string name in names.Take(names.Length - 1)) { path = Path.Combine(path, name); - SafeFileHandle parent = Open(path, WinAPI.FILE_READ_ATTRIBUTES, HeldShare); + SafeFileHandle parent = Hold(path); handles.Add(parent); if (!IsPlainDirectory(GetAttributes(parent))) diff --git a/package/AgentWindowsManaged/Actions/WinAPI.cs b/package/AgentWindowsManaged/Actions/WinAPI.cs index 414f4ddb6..21a291a31 100644 --- a/package/AgentWindowsManaged/Actions/WinAPI.cs +++ b/package/AgentWindowsManaged/Actions/WinAPI.cs @@ -47,6 +47,8 @@ internal static class WinAPI internal static uint FILE_SHARE_WRITE = 0x00000002; + internal const uint FILE_TRAVERSE = 0x00000020; + internal const int FileDispositionInfo = 4; internal const uint GENERIC_ALL = 0x10000000; diff --git a/package/AgentWindowsManaged/Resources/DevolutionsAgent_en-us.wxl b/package/AgentWindowsManaged/Resources/DevolutionsAgent_en-us.wxl index 0245a2c67..4c27de9a5 100644 --- a/package/AgentWindowsManaged/Resources/DevolutionsAgent_en-us.wxl +++ b/package/AgentWindowsManaged/Resources/DevolutionsAgent_en-us.wxl @@ -21,6 +21,7 @@ Devolutions Inc. Devolutions + Can't use {0} because it is a file, not a folder. Delete the file, then try again. Can't use the folder {0} because it is a link. Delete the link, then try again. Can't use the folder {0} because it is owned by {1}. Only SYSTEM or an administrator may own it. Change its owner, then try again. Can't use the folder {0} because {1} can replace it or what it contains. Remove that permission, then try again. diff --git a/package/AgentWindowsManaged/Resources/DevolutionsAgent_fr-fr.wxl b/package/AgentWindowsManaged/Resources/DevolutionsAgent_fr-fr.wxl index 8106d0024..1da485c57 100644 --- a/package/AgentWindowsManaged/Resources/DevolutionsAgent_fr-fr.wxl +++ b/package/AgentWindowsManaged/Resources/DevolutionsAgent_fr-fr.wxl @@ -15,6 +15,7 @@ Devolutions Inc. Devolutions + Impossible d'utiliser {0}, car c'est un fichier et non un dossier. Supprimez le fichier, puis réessayez. Impossible d'utiliser le dossier {0}, car c'est un lien. Supprimez le lien, puis réessayez. Impossible d'utiliser le dossier {0}, car il appartient à {1}. Seul SYSTEM ou un administrateur peut en être propriétaire. Changez son propriétaire, puis réessayez. Impossible d'utiliser le dossier {0}, car {1} peut remplacer le dossier ou son contenu. Retirez cette autorisation, puis réessayez. diff --git a/package/AgentWindowsManaged/Resources/DevolutionsAgent_sv-se.wxl b/package/AgentWindowsManaged/Resources/DevolutionsAgent_sv-se.wxl index f47608239..d933999bf 100644 --- a/package/AgentWindowsManaged/Resources/DevolutionsAgent_sv-se.wxl +++ b/package/AgentWindowsManaged/Resources/DevolutionsAgent_sv-se.wxl @@ -21,6 +21,7 @@ Devolutions Inc. Devolutions + Det går inte att använda {0} eftersom det är en fil, inte en mapp. Ta bort filen och försök igen. Det går inte att använda mappen {0} eftersom den är en länk. Ta bort länken och försök igen. Det går inte att använda mappen {0} eftersom den ägs av {1}. Endast SYSTEM eller en administratör får äga den. Byt ägare och försök igen. Det går inte att använda mappen {0} eftersom {1} kan ersätta den eller dess innehåll. Ta bort den behörigheten och försök igen. diff --git a/package/AgentWindowsManaged/Resources/Strings.g.cs b/package/AgentWindowsManaged/Resources/Strings.g.cs index d45b6a9e8..659e58d8a 100644 --- a/package/AgentWindowsManaged/Resources/Strings.g.cs +++ b/package/AgentWindowsManaged/Resources/Strings.g.cs @@ -121,6 +121,10 @@ public static string I18n(this MsiRuntime runtime, string res) /// public const string DataFolderIsLink = "DataFolderIsLink"; /// + /// Can't use {0} because it is a file, not a folder. Delete the file, then try again. + /// + public const string DataFolderIsFile = "DataFolderIsFile"; + /// /// Can't use the folder {0} because it is owned by {1}. Only SYSTEM or an administrator may own it. Change its owner, then try again. /// public const string DataFolderOwnedByUntrustedAccount = "DataFolderOwnedByUntrustedAccount"; diff --git a/package/AgentWindowsManaged/Resources/Strings_en-US.json b/package/AgentWindowsManaged/Resources/Strings_en-US.json index edb1b10ad..963990482 100644 --- a/package/AgentWindowsManaged/Resources/Strings_en-US.json +++ b/package/AgentWindowsManaged/Resources/Strings_en-US.json @@ -116,6 +116,10 @@ "id": "DataFolderIsLink", "text": "Can't use the folder {0} because it is a link. Delete the link, then try again." }, + { + "id": "DataFolderIsFile", + "text": "Can't use {0} because it is a file, not a folder. Delete the file, then try again." + }, { "id": "DataFolderOwnedByUntrustedAccount", "text": "Can't use the folder {0} because it is owned by {1}. Only SYSTEM or an administrator may own it. Change its owner, then try again." diff --git a/package/AgentWindowsManaged/Resources/Strings_fr-FR.json b/package/AgentWindowsManaged/Resources/Strings_fr-FR.json index 3661a3bb8..c8b5aed76 100644 --- a/package/AgentWindowsManaged/Resources/Strings_fr-FR.json +++ b/package/AgentWindowsManaged/Resources/Strings_fr-FR.json @@ -92,6 +92,10 @@ "id": "DataFolderIsLink", "text": "Impossible d'utiliser le dossier {0}, car c'est un lien. Supprimez le lien, puis réessayez." }, + { + "id": "DataFolderIsFile", + "text": "Impossible d'utiliser {0}, car c'est un fichier et non un dossier. Supprimez le fichier, puis réessayez." + }, { "id": "DataFolderOwnedByUntrustedAccount", "text": "Impossible d'utiliser le dossier {0}, car il appartient à {1}. Seul SYSTEM ou un administrateur peut en être propriétaire. Changez son propriétaire, puis réessayez." diff --git a/package/AgentWindowsManaged/Resources/Strings_sv-SE.json b/package/AgentWindowsManaged/Resources/Strings_sv-SE.json index 73a6b134e..896e7c936 100644 --- a/package/AgentWindowsManaged/Resources/Strings_sv-SE.json +++ b/package/AgentWindowsManaged/Resources/Strings_sv-SE.json @@ -116,6 +116,10 @@ "id": "DataFolderIsLink", "text": "Det går inte att använda mappen {0} eftersom den är en länk. Ta bort länken och försök igen." }, + { + "id": "DataFolderIsFile", + "text": "Det går inte att använda {0} eftersom det är en fil, inte en mapp. Ta bort filen och försök igen." + }, { "id": "DataFolderOwnedByUntrustedAccount", "text": "Det går inte att använda mappen {0} eftersom den ägs av {1}. Endast SYSTEM eller en administratör får äga den. Byt ägare och försök igen." From b244bff731331d81bd764c4272cdae6835e88db0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Wed, 28 Oct 2026 00:13:06 +0900 Subject: [PATCH 7/9] test(agent-installer): name the failed open the unreadable test pins The folder can't be opened at all, so the test checks that an open that fails for a reason other than not-found is refused as unreadable. Issue: DGW-478 Co-Authored-By: Claude Opus 5.5 --- package/AgentWindowsManaged.Tests/DataFolderTests.cs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/package/AgentWindowsManaged.Tests/DataFolderTests.cs b/package/AgentWindowsManaged.Tests/DataFolderTests.cs index 94912f931..43e1c10cd 100644 --- a/package/AgentWindowsManaged.Tests/DataFolderTests.cs +++ b/package/AgentWindowsManaged.Tests/DataFolderTests.cs @@ -231,12 +231,13 @@ public void CreateRefusesADevolutionsFolderThatUsersMayDeleteAndCreatesNothingIn // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder [Fact] - public void CheckRefusesADevolutionsFolderWhoseSecurityItCantRead() + public void CheckRefusesADevolutionsFolderItCantOpenAsUnreadable() { using TempFolder folder = new(); string devolutions = Path.Combine(folder.Path, "Devolutions"); - // The OWNER RIGHTS entry takes away the owner's implicit right to read the security descriptor, and the current - // user keeps only the rights to list the folder, read its attributes and synchronize, which deleting it needs. + // Opening the folder fails with access denied, not with not-found: the current user, its owner, is granted + // neither READ_CONTROL, as the OWNER RIGHTS entry takes away the owner's implicit one, nor FILE_TRAVERSE. + // The user keeps the rights to list the folder, read its attributes and synchronize, which deleting it needs. Assert.True(SecureDirectory.Create(devolutions, $"O:{CurrentUser}D:P(A;;0;;;OW)(A;OICI;FA;;;SY)(A;;0x100081;;;{CurrentUser})")); DataFolderRefusal refusal = DataFolders.Check(folder.Path, this.logger); From 9873c43f967b237aa8ecf5d0f645ee2b6e8e2468 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Wed, 28 Oct 2026 00:29:13 +0900 Subject: [PATCH 8/9] test(agent-installer): compare security as stored on elevated runners An elevated runner's account renders as an SDDL alias, and DirectorySecurity reorders the entries it reads, so both tests now compare descriptors read and rendered the same way. Issue: DGW-478 Co-Authored-By: Claude Opus 5.5 --- .../SecureDirectoryTests.cs | 20 ++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/package/AgentWindowsManaged.Tests/SecureDirectoryTests.cs b/package/AgentWindowsManaged.Tests/SecureDirectoryTests.cs index 352efcd1b..1c2dfb617 100644 --- a/package/AgentWindowsManaged.Tests/SecureDirectoryTests.cs +++ b/package/AgentWindowsManaged.Tests/SecureDirectoryTests.cs @@ -182,11 +182,11 @@ public void SetSecurityLeavesExistingChildrenUnchanged() using TempFolder folder = new(); folder.CreateDirectory("data"); string child = folder.CreateDirectory("data", "child"); - string before = Sddl(child); + string before = RawSddl(child); SecureDirectory.SetSecurity(folder.Path, ["data"], ProtectedSddl); - Assert.Equal(before, Sddl(child)); + Assert.Equal(before, RawSddl(child)); } // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder @@ -230,7 +230,10 @@ public void ReadSecurityReadsTheOwnerAndTheDaclThatSetSecurityApplied() using SafeFileHandle handle = SecureDirectory.OpenToRead(path, hold: false); (RawSecurityDescriptor security, bool isReparsePoint, bool isDirectory) = SecureDirectory.ReadSecurity(handle); - Assert.Equal(ProtectedSddl, security.GetSddlForm(AccessControlSections.Owner | AccessControlSections.Access)); + // Both sides go through the same SDDL rendering, which writes some account SIDs as aliases such as LA. + Assert.Equal( + new RawSecurityDescriptor(ProtectedSddl).GetSddlForm(AccessControlSections.Owner | AccessControlSections.Access), + security.GetSddlForm(AccessControlSections.Owner | AccessControlSections.Access)); Assert.False(isReparsePoint); Assert.True(isDirectory); } @@ -286,4 +289,15 @@ private static string Sddl(string path) => new DirectoryInfo(path) .GetAccessControl(AccessControlSections.Owner | AccessControlSections.Access) .GetSecurityDescriptorSddlForm(AccessControlSections.Owner | AccessControlSections.Access); + + /// + /// The owner and DACL of the directory as stored, without the canonical ordering that + /// applies when it reads them. + /// + private static string RawSddl(string path) + { + using SafeFileHandle handle = SecureDirectory.OpenToRead(path, hold: false); + return SecureDirectory.ReadSecurity(handle).Security + .GetSddlForm(AccessControlSections.Owner | AccessControlSections.Access); + } } From 552be53239c2fafaa5316ea1a4bed31d6f0b9118 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Wed, 28 Oct 2026 01:46:29 +0900 Subject: [PATCH 9/9] fix(agent-installer): accept an administrator's entry on Devolutions A member of Administrators may already own the shared folder, so an entry that lets one rename or replace it gives no account more control than the owner rule accepts; it is common on a folder an administrator's own process created. Issue: DGW-478 Security: yes Co-Authored-By: Claude Opus 5.5 --- .../DataFolderTests.cs | 3 +++ .../Actions/DataFolders.cs | 25 +++++++++++-------- 2 files changed, 18 insertions(+), 10 deletions(-) diff --git a/package/AgentWindowsManaged.Tests/DataFolderTests.cs b/package/AgentWindowsManaged.Tests/DataFolderTests.cs index 43e1c10cd..fbc9a1dc3 100644 --- a/package/AgentWindowsManaged.Tests/DataFolderTests.cs +++ b/package/AgentWindowsManaged.Tests/DataFolderTests.cs @@ -62,6 +62,9 @@ public sealed class DataFolderTests [InlineData("Users own Agent", Agent, "O:BU" + InheritedDacl, "Owner " + Users)] // oracle: reference Devolutions/devolutions-gateway@9b8b60380f6c:crates/private-fs/src/windows/security.rs Policy::check_ancestor [InlineData("Users may delete Devolutions", Devolutions, "O:SYD:(A;;SD;;;BU)", "Access " + Users)] + // The built-in Administrator account (LA) is a member of Administrators on every machine. + // oracle: intent package/AgentWindowsManaged/INTENT.md §Data folder + [InlineData("a member of Administrators may delete Devolutions", Devolutions, "O:SYD:(A;OICI;FA;;;LA)", "none")] // oracle: reference Devolutions/devolutions-gateway@9b8b60380f6c:crates/private-fs/src/windows/security.rs Policy::check_ancestor [InlineData("Users may change the DACL", Devolutions, "O:SYD:(A;;WD;;;BU)", "Access " + Users)] // oracle: reference Devolutions/devolutions-gateway@9b8b60380f6c:crates/private-fs/src/windows/security.rs Policy::check_ancestor diff --git a/package/AgentWindowsManaged/Actions/DataFolders.cs b/package/AgentWindowsManaged/Actions/DataFolders.cs index 4a94e650d..128aff4f8 100644 --- a/package/AgentWindowsManaged/Actions/DataFolders.cs +++ b/package/AgentWindowsManaged/Actions/DataFolders.cs @@ -231,8 +231,8 @@ internal static DataFolderRefusal Create(string root, string sddl, ILogger logge /// a folder the agent then refuses to use. /// /// - /// Whether to refuse a DACL that lets an account other than SYSTEM, Administrators, TrustedInstaller or OWNER RIGHTS - /// replace the folder or what it contains. + /// Whether to refuse a DACL that lets an account other than SYSTEM, Administrators, TrustedInstaller, a member of + /// Administrators or OWNER RIGHTS replace the folder or what it contains. /// /// The refusal, or null when the folder passes. internal static DataFolderRefusal Evaluate( @@ -290,28 +290,33 @@ internal static DataFolderRefusal Evaluate( CommonAce entry = (CommonAce)ace; if ((unchecked((uint)entry.AccessMask) & ReplaceRights) != 0 - && !SecureDirectory.IsPrivileged(entry.SecurityIdentifier) && entry.SecurityIdentifier != OwnerRights - && !IsAppContainer(entry.SecurityIdentifier)) + && !IsAppContainer(entry.SecurityIdentifier) + && !IsTrusted(entry.SecurityIdentifier)) { return new DataFolderRefusal(folder, DataFolderProblem.Access, entry.SecurityIdentifier); } } } - bool trusted; + return IsTrusted(security.Owner) ? null : new DataFolderRefusal(folder, DataFolderProblem.Owner, security.Owner); + } + /// + /// Whether is SYSTEM, Administrators, TrustedInstaller or a member of Administrators, the + /// accounts that may own the folder or rename and replace it. + /// + private static bool IsTrusted(SecurityIdentifier sid) + { try { - trusted = SecureDirectory.IsTrustedOwner(security.Owner); + return SecureDirectory.IsTrustedOwner(sid); } catch (Exception) { - // An owner whose group membership can't be resolved isn't known to be an administrator. - trusted = false; + // An account whose group membership can't be resolved isn't known to be an administrator. + return false; } - - return trusted ? null : new DataFolderRefusal(folder, DataFolderProblem.Owner, security.Owner); } ///