From bc300539e84b23f859ab0de2c1132eeefed42f64 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Mon, 7 Sep 2026 23:03:27 -0400 Subject: [PATCH 1/9] fix(agent-installer): secure policy migration Create and verify the managed PackageBroker directory without promoting untrusted paths. Migrate only trusted legacy JSON with identity- and digest-bound rollback and commit cleanup, and register the Agent Event Log source through MSI. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../Actions/AgentActions.cs | 55 + .../Actions/PackageBrokerPolicyActions.cs | 1160 +++++++++++++++++ package/AgentWindowsManaged/Actions/WinAPI.cs | 72 + package/AgentWindowsManaged/Program.cs | 15 +- .../AgentWindowsManaged/Resources/Includes.cs | 13 + 5 files changed, 1314 insertions(+), 1 deletion(-) create mode 100644 package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs diff --git a/package/AgentWindowsManaged/Actions/AgentActions.cs b/package/AgentWindowsManaged/Actions/AgentActions.cs index c167ee43c..429fc5425 100644 --- a/package/AgentWindowsManaged/Actions/AgentActions.cs +++ b/package/AgentWindowsManaged/Actions/AgentActions.cs @@ -121,6 +121,18 @@ internal static class AgentActions Features.PEDM_FEATURE.BeingInstall(), Sequence.InstallExecuteSequence); + private static readonly ElevatedManagedAction ensureProgramDataPackageBrokerDirectory = new( + new Id($"CA.{nameof(ensureProgramDataPackageBrokerDirectory)}"), + PackageBrokerPolicyActions.EnsureProgramDataPackageBrokerDirectory, + Return.check, + When.After, new Step(createProgramDataDirectory.Id), + Condition.NOT_BeingRemoved, + Sequence.InstallExecuteSequence) + { + Execute = Execute.deferred, + Impersonate = false, + }; + /// /// Set or reset the ACL on %ProgramData%\Devolutions\Agent /// @@ -151,6 +163,45 @@ internal static class AgentActions Impersonate = false, }; + private static readonly ElevatedManagedAction migrateLegacyPackageBrokerPolicy = new( + new Id($"CA.{nameof(migrateLegacyPackageBrokerPolicy)}"), + PackageBrokerPolicyActions.MigrateLegacyPackageBrokerPolicy, + Return.check, + When.After, new Step(ensureProgramDataPackageBrokerDirectory.Id), + Condition.NOT_BeingRemoved, + Sequence.InstallExecuteSequence) + { + Execute = Execute.deferred, + Impersonate = false, + UsesProperties = UseProperties(new[] { AgentProperties.installId }), + }; + + private static readonly ElevatedManagedAction rollbackLegacyPackageBrokerPolicyMigration = new( + new Id($"CA.{nameof(rollbackLegacyPackageBrokerPolicyMigration)}"), + PackageBrokerPolicyActions.RollbackLegacyPackageBrokerPolicyMigration, + Return.ignore, + When.Before, new Step(migrateLegacyPackageBrokerPolicy.Id), + Condition.NOT_BeingRemoved, + Sequence.InstallExecuteSequence) + { + Execute = Execute.rollback, + Impersonate = false, + UsesProperties = UseProperties(new[] { AgentProperties.installId }), + }; + + private static readonly ElevatedManagedAction commitLegacyPackageBrokerPolicyMigration = new( + new Id($"CA.{nameof(commitLegacyPackageBrokerPolicyMigration)}"), + PackageBrokerPolicyActions.CommitLegacyPackageBrokerPolicyMigration, + Return.check, + When.After, new Step(migrateLegacyPackageBrokerPolicy.Id), + Condition.NOT_BeingRemoved, + Sequence.InstallExecuteSequence) + { + Execute = Execute.commit, + Impersonate = false, + UsesProperties = UseProperties(new[] { AgentProperties.installId }), + }; + private static readonly ElevatedManagedAction cleanAgentConfigIfNeeded = new( new Id($"CA.{nameof(cleanAgentConfigIfNeeded)}"), CustomActions.CleanAgentConfig, @@ -499,6 +550,10 @@ private static string UseProperties(IEnumerable properties) setProgramDataDirectoryPermissions, createProgramDataPedmDirectories, setProgramDataPedmDirectoryPermissions, + ensureProgramDataPackageBrokerDirectory, + rollbackLegacyPackageBrokerPolicyMigration, + migrateLegacyPackageBrokerPolicy, + commitLegacyPackageBrokerPolicyMigration, initAgentConfigIfNeeded, registerExplorerCommand, registerExplorerCommandRollback, diff --git a/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs b/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs new file mode 100644 index 000000000..7633d6e34 --- /dev/null +++ b/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs @@ -0,0 +1,1160 @@ +using DevolutionsAgent.Properties; +using DevolutionsAgent.Resources; +using Microsoft.Deployment.WindowsInstaller; +using Microsoft.Win32.SafeHandles; +using Newtonsoft.Json; +using Newtonsoft.Json.Linq; +using System; +using System.Collections.Generic; +using System.ComponentModel; +using System.IO; +using System.Linq; +using System.Runtime.CompilerServices; +using System.Runtime.InteropServices; +using System.Security.AccessControl; +using System.Security.Cryptography; +using System.Security.Principal; +using System.Text; + +[assembly: InternalsVisibleTo("DevolutionsAgent.Installer.Tests")] + +namespace DevolutionsAgent.Actions; + +public static class PackageBrokerPolicyActions +{ + private static string ProgramDataDirectory => Path.Combine( + Environment.GetFolderPath(Environment.SpecialFolder.CommonApplicationData), + "Devolutions", + "Agent"); + + internal static string ProgramDataPackageBrokerDirectory => Path.Combine( + Environment.GetFolderPath(Environment.SpecialFolder.CommonApplicationData), + "Devolutions", + "PackageBroker"); + + private static string DestinationPolicyPath => + Path.Combine(ProgramDataPackageBrokerDirectory, "package-broker-policy.json"); + + private static string LegacyPolicyPath => + Path.Combine(ProgramDataDirectory, "package-broker-policy.json"); + + private static uint PackageBrokerSecurityInformation => + WinAPI.OWNER_SECURITY_INFORMATION | + WinAPI.GROUP_SECURITY_INFORMATION | + WinAPI.DACL_SECURITY_INFORMATION | + WinAPI.PROTECTED_DACL_SECURITY_INFORMATION; + + [CustomAction] + public static ActionResult EnsureProgramDataPackageBrokerDirectory(Session session) + { + try + { + EnsureSecureDirectoryTree( + Environment.GetFolderPath(Environment.SpecialFolder.CommonApplicationData), + ProgramDataPackageBrokerDirectory); + session.Log($"securely created or verified {ProgramDataPackageBrokerDirectory}"); + return ActionResult.Success; + } + catch (Exception error) + { + session.Log($"failed to securely create or verify {ProgramDataPackageBrokerDirectory}: {error}"); + return ActionResult.Failure; + } + } + + [CustomAction] + public static ActionResult MigrateLegacyPackageBrokerPolicy(Session session) + { + string destination = DestinationPolicyPath; + string sourcePath = LegacyPolicyPath; + string temporary = Path.Combine( + ProgramDataPackageBrokerDirectory, + $".package-broker-policy.migration-{Guid.NewGuid():N}.tmp"); + string marker = MigrationMarkerPath(session); + bool migrationStarted = false; + + try + { + LogLegacyYamlMigrationRequired(session, destination); + using PinnedPath destinationPath = PinPathWithoutReparse( + destination, + leafIsDirectory: false, + allowMissingLeaf: true, + leafAccess: WinAPI.FILE_READ_ATTRIBUTES | WinAPI.READ_CONTROL); + if (destinationPath.Leaf != null) + { + VerifyPackageBrokerSecurity(SecurityFromHandle(destinationPath.Leaf, isDirectory: false)); + session.Log($"package broker policy already exists at {destination}; legacy migration skipped"); + return ActionResult.Success; + } + + using PinnedPath source = PinPathWithoutReparse( + sourcePath, + leafIsDirectory: false, + allowMissingLeaf: true, + leafAccess: WinAPI.GENERIC_READ | WinAPI.READ_CONTROL); + if (source.Leaf == null) + { + return ActionResult.Success; + } + + if (!TryVerifyLegacyPolicySourceSecurity( + SecurityFromHandle(source.Leaf, isDirectory: false), + out string sourceSecurityDiagnostic)) + { + session.Log( + $"skipping automatic package broker policy migration from {sourcePath}: " + + $"{sourceSecurityDiagnostic}. The source was left untouched and no destination was created. " + + "Restrict the source owner and write access to SYSTEM/Administrators, then validate and migrate it manually."); + return ActionResult.Success; + } + + string sourceIdentity = FileIdentity(source.Leaf); + string sourceDigest = FileContentDigest(source.Leaf); + migrationStarted = true; + using (FileStream sourceStream = OpenPinnedFileStream(source.Leaf)) + using (FileStream target = new(temporary, FileMode.CreateNew, FileAccess.ReadWrite, FileShare.None)) + { + sourceStream.CopyTo(target); + target.Flush(true); + } + + SetFileSecurity(temporary, Includes.PROGRAM_DATA_PACKAGE_BROKER_FILE_SDDL); + MigrationRecord record; + using (PinnedPath temporaryPath = PinPathWithoutReparse( + temporary, + leafIsDirectory: false, + allowMissingLeaf: false, + leafAccess: WinAPI.GENERIC_READ | WinAPI.FILE_READ_ATTRIBUTES | WinAPI.READ_CONTROL)) + { + VerifyPackageBrokerSecurity(SecurityFromHandle(temporaryPath.Leaf, isDirectory: false)); + record = new MigrationRecord( + sourceIdentity, + sourceDigest, + FileIdentity(temporaryPath.Leaf), + FileContentDigest(temporaryPath.Leaf)); + } + + WriteMigrationMarker(marker, record); + File.Move(temporary, destination); + + using PinnedPath migratedPath = PinPathWithoutReparse( + destination, + leafIsDirectory: false, + allowMissingLeaf: false, + leafAccess: WinAPI.GENERIC_READ | WinAPI.FILE_READ_ATTRIBUTES | WinAPI.READ_CONTROL); + VerifyPackageBrokerSecurity(SecurityFromHandle(migratedPath.Leaf, isDirectory: false)); + if (!FileIdentityAndDigestMatch( + migratedPath.Leaf, + record.DestinationIdentity, + record.DestinationDigest)) + { + throw new InvalidOperationException("migrated package broker policy identity changed unexpectedly"); + } + + session.Log($"migrated legacy package broker policy from {sourcePath} to {destination}"); + return ActionResult.Success; + } + catch (Exception error) + { + if (!migrationStarted) + { + session.Log( + $"skipping automatic package broker policy migration because its paths could not be trusted: {error}"); + return ActionResult.Success; + } + session.Log($"failed to migrate legacy package broker policy: {error}"); + return ActionResult.Failure; + } + finally + { + TryDeleteTemporaryFile(session, temporary); + } + } + + [CustomAction] + public static ActionResult RollbackLegacyPackageBrokerPolicyMigration(Session session) + { + string marker = MigrationMarkerPath(session); + string destination = DestinationPolicyPath; + + try + { + using PinnedPath markerPath = PinPathWithoutReparse( + marker, + leafIsDirectory: false, + allowMissingLeaf: true, + leafAccess: WinAPI.GENERIC_READ | WinAPI.DELETE | WinAPI.FILE_READ_ATTRIBUTES | WinAPI.READ_CONTROL); + if (markerPath.Leaf == null) + { + return ActionResult.Success; + } + + VerifyPackageBrokerSecurity(SecurityFromHandle(markerPath.Leaf, isDirectory: false)); + MigrationRecord record = ReadMigrationMarker(markerPath.Leaf); + + using PinnedPath destinationPath = PinPathWithoutReparse( + destination, + leafIsDirectory: false, + allowMissingLeaf: true, + leafAccess: WinAPI.GENERIC_READ | WinAPI.DELETE | WinAPI.FILE_READ_ATTRIBUTES | WinAPI.READ_CONTROL); + if (destinationPath.Leaf != null && + FileIdentityAndDigestMatch( + destinationPath.Leaf, + record.DestinationIdentity, + record.DestinationDigest)) + { + VerifyPackageBrokerSecurity(SecurityFromHandle(destinationPath.Leaf, isDirectory: false)); + DeleteFileByHandle(destinationPath.Leaf); + } + + VerifyPackageBrokerSecurity(SecurityFromHandle(markerPath.Leaf, isDirectory: false)); + DeleteFileByHandle(markerPath.Leaf); + } + catch (Exception error) + { + session.Log($"failed to roll back legacy package broker policy migration: {error}"); + } + + return ActionResult.Success; + } + + [CustomAction] + public static ActionResult CommitLegacyPackageBrokerPolicyMigration(Session session) + { + string marker = MigrationMarkerPath(session); + string sourcePath = LegacyPolicyPath; + + try + { + using PinnedPath markerPath = PinPathWithoutReparse( + marker, + leafIsDirectory: false, + allowMissingLeaf: true, + leafAccess: WinAPI.GENERIC_READ | WinAPI.DELETE | WinAPI.FILE_READ_ATTRIBUTES | WinAPI.READ_CONTROL); + if (markerPath.Leaf == null) + { + return ActionResult.Success; + } + + VerifyPackageBrokerSecurity(SecurityFromHandle(markerPath.Leaf, isDirectory: false)); + MigrationRecord record = ReadMigrationMarker(markerPath.Leaf); + + using PinnedPath source = PinPathWithoutReparse( + sourcePath, + leafIsDirectory: false, + allowMissingLeaf: true, + leafAccess: WinAPI.GENERIC_READ | WinAPI.DELETE | WinAPI.FILE_READ_ATTRIBUTES | WinAPI.READ_CONTROL); + bool sourceChanged = + source.Leaf == null || + !FileIdentityAndDigestMatch(source.Leaf, record.SourceIdentity, record.SourceDigest); + bool removeSource = !sourceChanged; + if (removeSource && + IsLegacyPackageBrokerPolicyExplicitlyConfigured( + sourcePath, + source.Leaf, + out string configuredDiagnostic)) + { + session.Log( + $"preserving the configured legacy package broker policy during commit: {configuredDiagnostic}"); + removeSource = false; + } + if (removeSource && + !TryVerifyLegacyPolicySourceSecurity( + SecurityFromHandle(source.Leaf, isDirectory: false), + out string sourceSecurityDiagnostic)) + { + session.Log( + $"preserving the legacy package broker policy during commit: {sourceSecurityDiagnostic}"); + removeSource = false; + } + + VerifyPackageBrokerSecurity(SecurityFromHandle(markerPath.Leaf, isDirectory: false)); + DeleteFileByHandle(markerPath.Leaf); + + if (!removeSource) + { + if (sourceChanged) + { + session.Log( + "legacy package broker policy changed after migration; preserving the current source"); + } + return ActionResult.Success; + } + + try + { + DeleteFileByHandle(source.Leaf); + } + catch (Exception error) + { + session.Log( + $"failed to remove the migrated legacy package broker policy; preserving both copies: {error}"); + } + + return ActionResult.Success; + } + catch (Exception error) + { + session.Log($"failed to commit legacy package broker policy migration: {error}"); + return ActionResult.Failure; + } + } + + internal static void EnsureSecureDirectoryTree(string programData, string target) + { + string programDataPath = Path.GetFullPath(programData); + string targetPath = Path.GetFullPath(target); + string prefix = programDataPath.TrimEnd(Path.DirectorySeparatorChar) + Path.DirectorySeparatorChar; + if (!targetPath.StartsWith(prefix, StringComparison.OrdinalIgnoreCase)) + { + throw new InvalidOperationException($"{targetPath} is outside the ProgramData directory"); + } + + string[] components = targetPath + .Substring(prefix.Length) + .Split(new[] { Path.DirectorySeparatorChar }, StringSplitOptions.RemoveEmptyEntries); + if (components.Length != 2 || + !string.Equals(components[0], "Devolutions", StringComparison.OrdinalIgnoreCase) || + !string.Equals(components[1], "PackageBroker", StringComparison.OrdinalIgnoreCase)) + { + throw new InvalidOperationException("package broker directory has an unexpected shape"); + } + + List handles = new(); + try + { + SafeFileHandle programDataHandle = OpenPathWithoutReparse( + programDataPath, + isDirectory: true, + WinAPI.FILE_READ_ATTRIBUTES | WinAPI.READ_CONTROL, + WinAPI.FILE_SHARE_READ | WinAPI.FILE_SHARE_WRITE, + allowMissing: false); + handles.Add(programDataHandle); + VerifyResolvedPath(programDataHandle, programDataPath); + VerifyTrustedDirectorySecurity(SecurityFromHandle(programDataHandle, isDirectory: true)); + + string vendorPath = Path.Combine(programDataPath, components[0]); + SafeFileHandle vendorHandle = OpenPathWithoutReparse( + vendorPath, + isDirectory: true, + WinAPI.FILE_READ_ATTRIBUTES | WinAPI.READ_CONTROL, + WinAPI.FILE_SHARE_READ | WinAPI.FILE_SHARE_WRITE, + allowMissing: true); + if (vendorHandle == null) + { + Directory.CreateDirectory(vendorPath); + vendorHandle = OpenPathWithoutReparse( + vendorPath, + isDirectory: true, + WinAPI.FILE_READ_ATTRIBUTES | WinAPI.READ_CONTROL, + WinAPI.FILE_SHARE_READ | WinAPI.FILE_SHARE_WRITE, + allowMissing: false); + } + handles.Add(vendorHandle); + VerifyResolvedPath(vendorHandle, vendorPath); + VerifyTrustedDirectorySecurity(SecurityFromHandle(vendorHandle, isDirectory: true)); + + string leafPath = Path.Combine(vendorPath, components[1]); + SafeFileHandle leafHandle = OpenPathWithoutReparse( + leafPath, + isDirectory: true, + WinAPI.FILE_READ_ATTRIBUTES | WinAPI.READ_CONTROL, + WinAPI.FILE_SHARE_READ | WinAPI.FILE_SHARE_WRITE, + allowMissing: true); + bool leafCreated = leafHandle == null; + if (leafCreated) + { + CreateDirectoryWithSecurity(leafPath, Includes.PROGRAM_DATA_PACKAGE_BROKER_SDDL); + leafHandle = OpenPathWithoutReparse( + leafPath, + isDirectory: true, + WinAPI.FILE_READ_ATTRIBUTES | WinAPI.READ_CONTROL, + WinAPI.FILE_SHARE_READ | WinAPI.FILE_SHARE_WRITE, + allowMissing: false); + } + handles.Add(leafHandle); + VerifyResolvedPath(leafHandle, leafPath); + FileSystemSecurity leafSecurity = SecurityFromHandle(leafHandle, isDirectory: true); + VerifyPackageBrokerSecurity(leafSecurity); + if (leafCreated) + { + VerifySecurityDescriptor(leafSecurity, Includes.PROGRAM_DATA_PACKAGE_BROKER_SDDL); + } + } + finally + { + foreach (SafeFileHandle handle in handles) + { + handle.Dispose(); + } + } + } + + internal static void VerifyPackageBrokerSecurity(FileSystemSecurity security) + { + SecurityIdentifier system = new(WellKnownSidType.LocalSystemSid, null); + SecurityIdentifier administrators = new(WellKnownSidType.BuiltinAdministratorsSid, null); + SecurityIdentifier owner = (SecurityIdentifier)security.GetOwner(typeof(SecurityIdentifier)); + if (!owner.Equals(system) && !owner.Equals(administrators)) + { + throw new InvalidOperationException($"package broker path has untrusted owner {owner.Value}"); + } + if (!security.AreAccessRulesProtected) + { + throw new InvalidOperationException("package broker path DACL inheritance is not protected"); + } + + FileSystemAccessRule[] rules = security + .GetAccessRules(true, true, typeof(SecurityIdentifier)) + .Cast() + .ToArray(); + bool IsExpectedFullControl(FileSystemAccessRule rule, SecurityIdentifier sid) => + rule.IdentityReference.Equals(sid) && + rule.AccessControlType == AccessControlType.Allow && + (rule.FileSystemRights & FileSystemRights.FullControl) == FileSystemRights.FullControl; + if (rules.Length != 2 || + !rules.Any(rule => IsExpectedFullControl(rule, system)) || + !rules.Any(rule => IsExpectedFullControl(rule, administrators))) + { + throw new InvalidOperationException("package broker path DACL is not SYSTEM/Administrators-only"); + } + } + + internal static bool TryVerifyLegacyPolicySourceSecurity( + FileSystemSecurity security, + out string diagnostic) + { + try + { + VerifyLegacyPolicySourceSecurity(security); + diagnostic = null; + return true; + } + catch (InvalidOperationException error) + { + diagnostic = error.Message; + return false; + } + } + + internal static bool TryReadConfiguredPolicyPath( + string configJson, + out string configuredPath, + out string diagnostic) + { + configuredPath = null; + diagnostic = null; + try + { + if (ContainsNonStrictJsonSyntax(configJson)) + { + diagnostic = "configuration uses non-strict JSON syntax"; + return false; + } + + using (JsonTextReader syntaxReader = new(new StringReader(configJson))) + { + while (syntaxReader.Read()) + { + if (syntaxReader.TokenType == JsonToken.Comment || + syntaxReader.TokenType == JsonToken.Undefined || + ((syntaxReader.TokenType == JsonToken.String || + syntaxReader.TokenType == JsonToken.PropertyName) && + syntaxReader.QuoteChar != '"')) + { + diagnostic = "configuration uses non-strict JSON syntax"; + return false; + } + } + } + + using JsonTextReader reader = new(new StringReader(configJson)) + { + DateParseHandling = DateParseHandling.None, + SupportMultipleContent = false, + }; + JObject config = JObject.Load( + reader, + new JsonLoadSettings + { + CommentHandling = CommentHandling.Load, + DuplicatePropertyNameHandling = DuplicatePropertyNameHandling.Error, + }); + if (reader.Read()) + { + diagnostic = "configuration contains multiple JSON values"; + return false; + } + + JToken token = config["PackageBroker"]?["PolicyPath"]; + if (token == null || token.Type == JTokenType.Null) + { + return true; + } + if (token.Type != JTokenType.String || string.IsNullOrWhiteSpace(token.Value())) + { + diagnostic = "PackageBroker.PolicyPath is not a valid path string"; + return false; + } + + configuredPath = token.Value(); + if (!Path.IsPathRooted(configuredPath)) + { + diagnostic = "PackageBroker.PolicyPath is not absolute"; + return false; + } + return true; + } + catch (Exception error) when ( + error is JsonException || + error is ArgumentException) + { + diagnostic = $"configuration could not be parsed safely: {error.Message}"; + return false; + } + } + + internal static bool ContainsNonStrictJsonSyntax(string json) + { + bool inString = false; + bool escaped = false; + for (int index = 0; index < json.Length; index++) + { + char current = json[index]; + if (inString) + { + if (escaped) + { + escaped = false; + } + else if (current == '\\') + { + escaped = true; + } + else if (current == '"') + { + inString = false; + } + continue; + } + + if (current == '"') + { + inString = true; + continue; + } + if (current == '/' && + index + 1 < json.Length && + (json[index + 1] == '/' || json[index + 1] == '*')) + { + return true; + } + if (current != ',') + { + continue; + } + + int next = index + 1; + while (next < json.Length && char.IsWhiteSpace(json[next])) + { + next++; + } + if (next < json.Length && (json[next] == '}' || json[next] == ']')) + { + return true; + } + } + return inString || escaped; + } + + internal static PinnedPath PinPathWithoutReparse( + string path, + bool leafIsDirectory, + bool allowMissingLeaf, + uint leafAccess) + { + string fullPath = Path.GetFullPath(path); + string root = Path.GetPathRoot(fullPath); + string parent = Path.GetDirectoryName(fullPath); + Stack ancestors = new(); + while (!string.IsNullOrEmpty(parent)) + { + ancestors.Push(parent); + if (string.Equals(parent, root, StringComparison.OrdinalIgnoreCase)) + { + break; + } + parent = Path.GetDirectoryName(parent); + } + + List handles = new(); + try + { + foreach (string ancestor in ancestors) + { + SafeFileHandle ancestorHandle = OpenPathWithoutReparse( + ancestor, + isDirectory: true, + WinAPI.FILE_READ_ATTRIBUTES, + WinAPI.FILE_SHARE_READ | WinAPI.FILE_SHARE_WRITE, + allowMissing: false); + VerifyResolvedPath(ancestorHandle, ancestor); + handles.Add(ancestorHandle); + } + + uint shareMode = (leafAccess & WinAPI.GENERIC_READ) != 0 + ? WinAPI.FILE_SHARE_READ + : WinAPI.FILE_SHARE_READ | WinAPI.FILE_SHARE_WRITE; + SafeFileHandle leaf = OpenPathWithoutReparse( + fullPath, + leafIsDirectory, + leafAccess, + shareMode, + allowMissingLeaf); + if (leaf != null) + { + VerifyResolvedPath(leaf, fullPath); + handles.Add(leaf); + } + return new PinnedPath(handles, leaf); + } + catch + { + foreach (SafeFileHandle handle in handles) + { + handle.Dispose(); + } + throw; + } + } + + internal static string FileIdentity(SafeFileHandle handle) + { + if (!WinAPI.GetFileInformationByHandle(handle, out WinAPI.ByHandleFileInformation information)) + { + throw new Win32Exception(Marshal.GetLastWin32Error(), "failed to query file identity"); + } + return string.Join( + ":", + information.VolumeSerialNumber, + information.FileIndexHigh, + information.FileIndexLow); + } + + internal static string FileContentDigest(SafeFileHandle handle) + { + using FileStream stream = OpenPinnedFileStream(handle); + using SHA256 sha256 = SHA256.Create(); + return Convert.ToBase64String(sha256.ComputeHash(stream)); + } + + internal static bool FileIdentityAndDigestMatch( + SafeFileHandle handle, + string expectedIdentity, + string expectedDigest) => + string.Equals(FileIdentity(handle), expectedIdentity, StringComparison.Ordinal) && + string.Equals(FileContentDigest(handle), expectedDigest, StringComparison.Ordinal); + + internal static void DeleteFileByHandle(SafeFileHandle handle) + { + WinAPI.FileDispositionInfo disposition = new() { DeleteFile = true }; + if (!WinAPI.SetFileInformationByHandle( + handle, + WinAPI.FileInfoByHandleClass.FileDispositionInfo, + ref disposition, + (uint)Marshal.SizeOf())) + { + throw new Win32Exception(Marshal.GetLastWin32Error(), "failed to delete the pinned file"); + } + } + + internal static MigrationRecord ReadMigrationMarkerJson(string markerJson) + { + JObject document = JObject.Parse(markerJson); + string sourceIdentity = document.Value("SourceIdentity"); + string sourceDigest = document.Value("SourceDigest"); + string destinationIdentity = document.Value("DestinationIdentity"); + string destinationDigest = document.Value("DestinationDigest"); + if (string.IsNullOrEmpty(sourceIdentity) || + string.IsNullOrEmpty(sourceDigest) || + string.IsNullOrEmpty(destinationIdentity) || + string.IsNullOrEmpty(destinationDigest)) + { + throw new InvalidOperationException("package broker migration marker is incomplete"); + } + return new MigrationRecord(sourceIdentity, sourceDigest, destinationIdentity, destinationDigest); + } + + private static string MigrationMarkerPath(Session session) => + Path.Combine( + ProgramDataPackageBrokerDirectory, + $".legacy-policy-migration-{session.Get(AgentProperties.installId)}.marker"); + + private static void WriteMigrationMarker(string marker, MigrationRecord record) + { + using (FileStream markerFile = new(marker, FileMode.CreateNew, FileAccess.ReadWrite, FileShare.None)) + { + byte[] markerContent = Encoding.UTF8.GetBytes(record.ToJson()); + markerFile.Write(markerContent, 0, markerContent.Length); + markerFile.Flush(true); + } + + SetFileSecurity(marker, Includes.PROGRAM_DATA_PACKAGE_BROKER_FILE_SDDL); + using PinnedPath markerPath = PinPathWithoutReparse( + marker, + leafIsDirectory: false, + allowMissingLeaf: false, + leafAccess: WinAPI.GENERIC_READ | WinAPI.FILE_READ_ATTRIBUTES | WinAPI.READ_CONTROL); + VerifyPackageBrokerSecurity(SecurityFromHandle(markerPath.Leaf, isDirectory: false)); + _ = ReadMigrationMarker(markerPath.Leaf); + } + + private static MigrationRecord ReadMigrationMarker(SafeFileHandle marker) + { + using FileStream stream = OpenPinnedFileStream(marker); + using StreamReader reader = new( + stream, + new UTF8Encoding(encoderShouldEmitUTF8Identifier: false, throwOnInvalidBytes: true), + detectEncodingFromByteOrderMarks: true); + return ReadMigrationMarkerJson(reader.ReadToEnd()); + } + + private static bool IsLegacyPackageBrokerPolicyExplicitlyConfigured( + string sourcePath, + SafeFileHandle source, + out string diagnostic) + { + string configPath = Path.Combine(ProgramDataDirectory, "agent.json"); + try + { + using PinnedPath config = PinPathWithoutReparse( + configPath, + leafIsDirectory: false, + allowMissingLeaf: true, + leafAccess: WinAPI.GENERIC_READ | WinAPI.FILE_READ_ATTRIBUTES); + if (config.Leaf == null) + { + diagnostic = null; + return false; + } + + string configJson; + using (FileStream stream = OpenPinnedFileStream(config.Leaf)) + using (StreamReader reader = new( + stream, + new UTF8Encoding(encoderShouldEmitUTF8Identifier: false, throwOnInvalidBytes: true), + detectEncodingFromByteOrderMarks: false)) + { + configJson = reader.ReadToEnd(); + } + if (!TryReadConfiguredPolicyPath(configJson, out string configuredPath, out diagnostic)) + { + return true; + } + if (configuredPath == null) + { + return false; + } + + using PinnedPath configured = PinPathWithoutReparse( + configuredPath, + leafIsDirectory: false, + allowMissingLeaf: true, + leafAccess: WinAPI.FILE_READ_ATTRIBUTES); + if (configured.Leaf == null) + { + diagnostic = $"PackageBroker.PolicyPath in {configPath} could not be resolved safely"; + return true; + } + if (!string.Equals(FileIdentity(configured.Leaf), FileIdentity(source), StringComparison.Ordinal)) + { + diagnostic = null; + return false; + } + + diagnostic = $"PackageBroker.PolicyPath in {configPath} still points to {sourcePath}"; + return true; + } + catch (Exception error) + { + diagnostic = $"could not safely determine PackageBroker.PolicyPath from {configPath}: {error.Message}"; + return true; + } + } + + private static void VerifyLegacyPolicySourceSecurity(FileSystemSecurity security) + { + const FileSystemRights unsafeRights = + FileSystemRights.WriteData | + FileSystemRights.AppendData | + FileSystemRights.WriteAttributes | + FileSystemRights.WriteExtendedAttributes | + FileSystemRights.Delete | + FileSystemRights.DeleteSubdirectoriesAndFiles | + FileSystemRights.ChangePermissions | + FileSystemRights.TakeOwnership | + (FileSystemRights)0x40000000 | + (FileSystemRights)0x10000000; + VerifyTrustedOwnerAndNoUnsafeGrants( + security, + unsafeRights, + "legacy policy", + "unsafe write or tamper"); + } + + private static void VerifyTrustedDirectorySecurity(FileSystemSecurity security) + { + const FileSystemRights tamperRights = + FileSystemRights.Delete | + FileSystemRights.DeleteSubdirectoriesAndFiles | + FileSystemRights.ChangePermissions | + FileSystemRights.TakeOwnership | + (FileSystemRights)0x10000000; + VerifyTrustedOwnerAndNoUnsafeGrants( + security, + tamperRights, + "directory", + "path-tampering"); + } + + private static void VerifyTrustedOwnerAndNoUnsafeGrants( + FileSystemSecurity security, + FileSystemRights unsafeRights, + string subject, + string accessDescription) + { + SecurityIdentifier system = new(WellKnownSidType.LocalSystemSid, null); + SecurityIdentifier administrators = new(WellKnownSidType.BuiltinAdministratorsSid, null); + SecurityIdentifier trustedInstaller = + (SecurityIdentifier)new NTAccount(@"NT SERVICE\TrustedInstaller").Translate(typeof(SecurityIdentifier)); + SecurityIdentifier owner = (SecurityIdentifier)security.GetOwner(typeof(SecurityIdentifier)); + if (!owner.Equals(system) && !owner.Equals(administrators) && !owner.Equals(trustedInstaller)) + { + throw new InvalidOperationException($"{subject} has untrusted owner {owner.Value}"); + } + + foreach (FileSystemAccessRule rule in security.GetAccessRules( + includeExplicit: true, + includeInherited: true, + targetType: typeof(SecurityIdentifier))) + { + if (rule.AccessControlType != AccessControlType.Allow || + (rule.PropagationFlags & PropagationFlags.InheritOnly) != 0 || + (rule.FileSystemRights & unsafeRights) == 0) + { + continue; + } + + SecurityIdentifier identity = (SecurityIdentifier)rule.IdentityReference; + if (!identity.Equals(system) && + !identity.Equals(administrators) && + !identity.Equals(trustedInstaller)) + { + throw new InvalidOperationException( + $"{subject} grants {accessDescription} rights to {identity.Value}"); + } + } + } + + internal static void CreateDirectoryWithSecurity(string path, string sddl) + { + const uint sdRevision = 1; + if (!WinAPI.ConvertStringSecurityDescriptorToSecurityDescriptorW( + sddl, + sdRevision, + out IntPtr securityDescriptor, + out _)) + { + throw new Win32Exception( + Marshal.GetLastWin32Error(), + $"failed to create security descriptor for {path}"); + } + + try + { + WinAPI.SECURITY_ATTRIBUTES attributes = new() + { + nLength = (uint)Marshal.SizeOf(), + lpSecurityDescriptor = securityDescriptor, + bInheritHandle = false, + }; + if (!WinAPI.CreateDirectory(path, ref attributes)) + { + int error = Marshal.GetLastWin32Error(); + if (error != WinAPI.ERROR_ALREADY_EXISTS) + { + throw new Win32Exception(error, $"failed to securely create {path}"); + } + } + } + finally + { + WinAPI.LocalFree(securityDescriptor); + } + } + + private static void SetFileSecurity(string path, string sddl) + { + const uint sdRevision = 1; + if (!WinAPI.ConvertStringSecurityDescriptorToSecurityDescriptorW( + sddl, + sdRevision, + out IntPtr securityDescriptor, + out _)) + { + throw new Win32Exception(Marshal.GetLastWin32Error(), $"failed to create security descriptor for {path}"); + } + + try + { + if (!WinAPI.SetFileSecurityW(path, PackageBrokerSecurityInformation, securityDescriptor)) + { + throw new Win32Exception(Marshal.GetLastWin32Error(), $"failed to secure {path}"); + } + } + finally + { + WinAPI.LocalFree(securityDescriptor); + } + } + + internal static void VerifySecurityDescriptor(FileSystemSecurity actual, string expectedSddl) + { + RawSecurityDescriptor expected = new(expectedSddl); + string expectedCanonical = expected.GetSddlForm(AccessControlSections.All); + string actualCanonical = actual.GetSecurityDescriptorSddlForm(AccessControlSections.All); + if (!string.Equals(actualCanonical, expectedCanonical, StringComparison.Ordinal)) + { + throw new InvalidOperationException("new directory security does not match its creation descriptor"); + } + } + + private static SafeFileHandle OpenPathWithoutReparse( + string path, + bool isDirectory, + uint desiredAccess, + uint shareMode, + bool allowMissing) + { + uint flags = WinAPI.FILE_FLAG_OPEN_REPARSE_POINT; + if (isDirectory) + { + flags |= WinAPI.FILE_FLAG_BACKUP_SEMANTICS; + } + + SafeFileHandle handle = WinAPI.CreateFile( + path, + desiredAccess, + shareMode, + IntPtr.Zero, + WinAPI.OPEN_EXISTING, + flags, + IntPtr.Zero); + if (handle.IsInvalid) + { + int error = Marshal.GetLastWin32Error(); + handle.Dispose(); + if (allowMissing && + (error == WinAPI.ERROR_FILE_NOT_FOUND || error == WinAPI.ERROR_PATH_NOT_FOUND)) + { + return null; + } + throw new Win32Exception(error, $"failed to open {path} without following reparse points"); + } + + if (!WinAPI.GetFileInformationByHandle(handle, out WinAPI.ByHandleFileInformation information)) + { + int error = Marshal.GetLastWin32Error(); + handle.Dispose(); + throw new Win32Exception(error, $"failed to inspect {path}"); + } + if ((information.FileAttributes & WinAPI.FILE_ATTRIBUTE_REPARSE_POINT) != 0) + { + handle.Dispose(); + throw new InvalidOperationException($"{path} is a reparse point"); + } + + bool actualDirectory = (information.FileAttributes & WinAPI.FILE_ATTRIBUTE_DIRECTORY) != 0; + if (actualDirectory != isDirectory) + { + handle.Dispose(); + throw new InvalidOperationException($"{path} has an unexpected filesystem type"); + } + if (!isDirectory && information.NumberOfLinks != 1) + { + handle.Dispose(); + throw new InvalidOperationException($"{path} has multiple hard links"); + } + return handle; + } + + private static void VerifyResolvedPath(SafeFileHandle handle, string expectedPath) + { + StringBuilder buffer = new(512); + uint length = WinAPI.GetFinalPathNameByHandle(handle.DangerousGetHandle(), buffer, (uint)buffer.Capacity, 0); + if (length == 0) + { + throw new Win32Exception(Marshal.GetLastWin32Error(), $"failed to resolve {expectedPath}"); + } + if (length >= buffer.Capacity) + { + buffer.EnsureCapacity((int)length + 1); + length = WinAPI.GetFinalPathNameByHandle( + handle.DangerousGetHandle(), + buffer, + (uint)buffer.Capacity, + 0); + if (length == 0 || length >= buffer.Capacity) + { + throw new Win32Exception(Marshal.GetLastWin32Error(), $"failed to resolve {expectedPath}"); + } + } + + string resolved = NormalizeExtendedPath(buffer.ToString()); + string expected = Path.GetFullPath(expectedPath).TrimEnd(Path.DirectorySeparatorChar); + if (!string.Equals(resolved.TrimEnd(Path.DirectorySeparatorChar), expected, StringComparison.OrdinalIgnoreCase)) + { + throw new InvalidOperationException($"{expectedPath} resolved to unexpected path {resolved}"); + } + } + + private static string NormalizeExtendedPath(string path) + { + const string uncPrefix = @"\\?\UNC\"; + const string localPrefix = @"\\?\"; + if (path.StartsWith(uncPrefix, StringComparison.OrdinalIgnoreCase)) + { + return @"\\" + path.Substring(uncPrefix.Length); + } + return path.StartsWith(localPrefix, StringComparison.OrdinalIgnoreCase) + ? path.Substring(localPrefix.Length) + : path; + } + + private static FileSystemSecurity SecurityFromHandle(SafeFileHandle handle, bool isDirectory) + { + uint information = + WinAPI.OWNER_SECURITY_INFORMATION | + WinAPI.GROUP_SECURITY_INFORMATION | + WinAPI.DACL_SECURITY_INFORMATION; + WinAPI.GetKernelObjectSecurity(handle, information, null, 0, out uint requiredSize); + int error = Marshal.GetLastWin32Error(); + if (requiredSize == 0 || error != WinAPI.ERROR_INSUFFICIENT_BUFFER) + { + throw new Win32Exception(error, "failed to query pinned path security descriptor size"); + } + + byte[] descriptor = new byte[requiredSize]; + if (!WinAPI.GetKernelObjectSecurity( + handle, + information, + descriptor, + (uint)descriptor.Length, + out _)) + { + throw new Win32Exception( + Marshal.GetLastWin32Error(), + "failed to query pinned path security descriptor"); + } + + FileSystemSecurity security = isDirectory ? new DirectorySecurity() : new FileSecurity(); + security.SetSecurityDescriptorBinaryForm(descriptor); + return security; + } + + private static FileStream OpenPinnedFileStream(SafeFileHandle handle) + { + SafeFileHandle borrowedHandle = new(handle.DangerousGetHandle(), ownsHandle: false); + FileStream stream = new(borrowedHandle, FileAccess.Read); + stream.Position = 0; + return stream; + } + + private static void LogLegacyYamlMigrationRequired(Session session, string destination) + { + foreach (string extension in new[] { "yaml", "yml" }) + { + string legacyYaml = Path.Combine(ProgramDataDirectory, $"package-broker-policy.{extension}"); + if (File.Exists(legacyYaml)) + { + session.Log( + $"legacy YAML package broker policy remains untouched at {legacyYaml}; " + + $"validate and migrate it manually to strict JSON at {destination}"); + } + } + } + + private static void TryDeleteTemporaryFile(Session session, string path) + { + try + { + using PinnedPath temporary = PinPathWithoutReparse( + path, + leafIsDirectory: false, + allowMissingLeaf: true, + leafAccess: WinAPI.DELETE | WinAPI.FILE_READ_ATTRIBUTES | WinAPI.READ_CONTROL); + if (temporary.Leaf == null) + { + return; + } + + VerifyPackageBrokerSecurity(SecurityFromHandle(temporary.Leaf, isDirectory: false)); + DeleteFileByHandle(temporary.Leaf); + } + catch (Exception error) + { + session.Log($"failed to remove package broker policy migration temporary file {path}: {error}"); + } + } + + internal sealed class PinnedPath : IDisposable + { + private readonly IReadOnlyList handles; + + internal PinnedPath(IReadOnlyList handles, SafeFileHandle leaf) + { + this.handles = handles; + Leaf = leaf; + } + + internal SafeFileHandle Leaf { get; } + + public void Dispose() + { + foreach (SafeFileHandle handle in handles) + { + handle.Dispose(); + } + } + } + + internal readonly struct MigrationRecord + { + internal MigrationRecord( + string sourceIdentity, + string sourceDigest, + string destinationIdentity, + string destinationDigest) + { + SourceIdentity = sourceIdentity; + SourceDigest = sourceDigest; + DestinationIdentity = destinationIdentity; + DestinationDigest = destinationDigest; + } + + internal string SourceIdentity { get; } + internal string SourceDigest { get; } + internal string DestinationIdentity { get; } + internal string DestinationDigest { get; } + + internal string ToJson() => + new JObject + { + ["SourceIdentity"] = SourceIdentity, + ["SourceDigest"] = SourceDigest, + ["DestinationIdentity"] = DestinationIdentity, + ["DestinationDigest"] = DestinationDigest, + }.ToString(Formatting.None, Array.Empty()); + } +} diff --git a/package/AgentWindowsManaged/Actions/WinAPI.cs b/package/AgentWindowsManaged/Actions/WinAPI.cs index fe25d1ed5..df7bf11fe 100644 --- a/package/AgentWindowsManaged/Actions/WinAPI.cs +++ b/package/AgentWindowsManaged/Actions/WinAPI.cs @@ -8,25 +8,68 @@ namespace DevolutionsAgent.Actions; internal static class WinAPI { internal static uint CREATE_ALWAYS = 2; + internal const int ERROR_ALREADY_EXISTS = 183; + internal const int ERROR_FILE_NOT_FOUND = 2; + internal const int ERROR_INSUFFICIENT_BUFFER = 122; + internal const int ERROR_PATH_NOT_FOUND = 3; internal static uint CREATE_NO_WINDOW = 0x08000000; internal const uint DACL_SECURITY_INFORMATION = 0x00000004; + internal const uint DELETE = 0x00010000; + internal const uint GROUP_SECURITY_INFORMATION = 0x00000002; + internal const uint OWNER_SECURITY_INFORMATION = 0x00000001; + internal const uint PROTECTED_DACL_SECURITY_INFORMATION = 0x80000000; internal const int EM_SETCUEBANNER = 0x1501; internal static uint FILE_ATTRIBUTE_NORMAL = 0x00000080; + internal const uint FILE_ATTRIBUTE_DIRECTORY = 0x00000010; + 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 static uint FILE_SHARE_READ = 0x00000001; internal static uint FILE_SHARE_WRITE = 0x00000002; + internal const uint GENERIC_READ = 0x80000000; internal static uint GENERIC_WRITE = 0x40000000; + internal const uint OPEN_EXISTING = 3; + internal const uint READ_CONTROL = 0x00020000; internal static uint MOVEFILE_REPLACE_EXISTING = 0x1; internal static uint MOVEFILE_DELAY_UNTIL_REBOOT = 0x04; + [StructLayout(LayoutKind.Sequential)] + internal struct ByHandleFileInformation + { + internal uint FileAttributes; + internal System.Runtime.InteropServices.ComTypes.FILETIME CreationTime; + internal System.Runtime.InteropServices.ComTypes.FILETIME LastAccessTime; + internal System.Runtime.InteropServices.ComTypes.FILETIME LastWriteTime; + internal uint VolumeSerialNumber; + internal uint FileSizeHigh; + internal uint FileSizeLow; + internal uint NumberOfLinks; + internal uint FileIndexHigh; + internal uint FileIndexLow; + } + + internal enum FileInfoByHandleClass + { + FileDispositionInfo = 4, + } + + [StructLayout(LayoutKind.Sequential)] + internal struct FileDispositionInfo + { + [MarshalAs(UnmanagedType.U1)] + internal bool DeleteFile; + } + internal const uint SC_MANAGER_ALL_ACCESS = 0xF003F; internal const uint SC_MANAGER_CONNECT = 0x0001; @@ -218,6 +261,21 @@ internal static extern SafeFileHandle CreateFile( IntPtr hTemplateFile ); + [DllImport("advapi32", SetLastError = true)] + [return: MarshalAs(UnmanagedType.Bool)] + internal static extern bool GetKernelObjectSecurity( + SafeFileHandle handle, + uint requestedInformation, + [Out] byte[] securityDescriptor, + uint length, + out uint lengthNeeded); + + [DllImport("kernel32", EntryPoint = "CreateDirectoryW", CharSet = CharSet.Unicode, SetLastError = true)] + [return: MarshalAs(UnmanagedType.Bool)] + internal static extern bool CreateDirectory( + [MarshalAs(UnmanagedType.LPWStr)] string lpPathName, + ref SECURITY_ATTRIBUTES lpSecurityAttributes); + [DllImport("kernel32", EntryPoint = "CreateProcessW", CharSet = CharSet.Unicode, SetLastError = true)] internal static extern bool CreateProcess( [MarshalAs(UnmanagedType.LPWStr)] string lpApplicationName, @@ -241,6 +299,20 @@ 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 ByHandleFileInformation lpFileInformation); + + [DllImport("kernel32", SetLastError = true)] + [return: MarshalAs(UnmanagedType.Bool)] + internal static extern bool SetFileInformationByHandle( + SafeFileHandle hFile, + FileInfoByHandleClass fileInformationClass, + ref FileDispositionInfo fileInformation, + uint bufferSize); + [DllImport("Kernel32", EntryPoint = "GetFinalPathNameByHandleW", CharSet = CharSet.Auto, SetLastError = true)] internal static extern uint GetFinalPathNameByHandle( IntPtr hFile, diff --git a/package/AgentWindowsManaged/Program.cs b/package/AgentWindowsManaged/Program.cs index d2a246305..09a32b242 100644 --- a/package/AgentWindowsManaged/Program.cs +++ b/package/AgentWindowsManaged/Program.cs @@ -348,7 +348,8 @@ static void Main() Win64 = project.Platform == Platform.x64, RegistryKeyAction = RegistryKeyAction.create, Feature = Features.PSU_FEATURE, - } + }, + CreateEventLogSourceRegistryValue(project.Platform == Platform.x64), }; List projectProperties = AgentProperties.Properties.Select(x => x.ToWixSharpProperty()).ToList(); @@ -422,6 +423,18 @@ static void Main() } } + internal static RegValue CreateEventLogSourceRegistryValue(bool win64) => + new( + RegistryHive.LocalMachine, + $"SYSTEM\\CurrentControlSet\\Services\\EventLog\\Application\\{Includes.PRODUCT_NAME}", + "EventMessageFile", + $"[{AgentProperties.InstallDir}]{Includes.EXECUTABLE_NAME}") + { + AttributesDefinition = "Type=string", + Win64 = win64, + RegistryKeyAction = RegistryKeyAction.createAndRemoveOnUninstall, + }; + private static void Project_UnhandledException(ExceptionEventArgs e) { string errorMessage = diff --git a/package/AgentWindowsManaged/Resources/Includes.cs b/package/AgentWindowsManaged/Resources/Includes.cs index 5ee9e12ae..667a1747c 100644 --- a/package/AgentWindowsManaged/Resources/Includes.cs +++ b/package/AgentWindowsManaged/Resources/Includes.cs @@ -60,5 +60,18 @@ internal static class Includes /// NT AUTHORITY\SYSTEM Allow FullControl /// internal static readonly string PROGRAM_DATA_PEDM_SDDL = "O:SYG:SYD:(A;OICI;FA;;;SY)"; + + /// + /// Protected ACL for the dedicated package-broker policy directory. + /// + /// + /// This directory must not inherit the LOCAL SERVICE and Users grants required by + /// unrelated Agent features under %ProgramData%\Devolutions\Agent. + /// + internal static readonly string PROGRAM_DATA_PACKAGE_BROKER_SDDL = + "O:SYG:SYD:P(A;OICI;FA;;;SY)(A;OICI;FA;;;BA)"; + + internal static readonly string PROGRAM_DATA_PACKAGE_BROKER_FILE_SDDL = + "O:SYG:SYD:P(A;;FA;;;SY)(A;;FA;;;BA)"; } } From 9185bcc8b2657b1bc0e190f78b6ec06597e18af0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Mon, 7 Sep 2026 23:03:41 -0400 Subject: [PATCH 2/9] test(agent-installer): cover policy migration Exercise the production ACL, path, identity, digest, rollback, commit, sequencing, and Event Log registry behavior on Windows. Run the focused installer suite as a required CI job. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/workflows/ci.yml | 17 +- .../DevolutionsAgent.Installer.Tests.csproj | 32 ++ .../PackageBrokerInstallerTests.cs | 376 ++++++++++++++++++ 3 files changed, 424 insertions(+), 1 deletion(-) create mode 100644 package/AgentWindowsManaged.Tests/DevolutionsAgent.Installer.Tests.csproj create mode 100644 package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f6d8729c9..4d42b3ecc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -1113,6 +1113,21 @@ jobs: run: dotnet test utils/dotnet/GatewayUtils.sln shell: pwsh + agent-installer-tests: + name: Agent installer tests + runs-on: windows-2022 + needs: [preflight] + + steps: + - name: Checkout ${{ github.repository }} + uses: actions/checkout@v6 + with: + ref: ${{ needs.preflight.outputs.ref }} + + - name: Tests + run: dotnet test package/AgentWindowsManaged.Tests/DevolutionsAgent.Installer.Tests.csproj + shell: pwsh + winapi-sanitizer-tests: name: Windows API sanitizer tests @@ -1357,7 +1372,7 @@ jobs: success: name: Success if: ${{ always() }} - needs: [tests, agent-tunnel-e2e, agent-policy-e2e, lints, check-dependencies, jetsocat-lipo, devolutions-gateway-powershell, devolutions-gateway, devolutions-gateway-merge, devolutions-pedm-desktop, devolutions-agent, devolutions-agent-merge, devolutions-pedm-client, dotnet-utils-tests, winapi-sanitizer-tests, winapi-miri, pedm-simulator, secure-memory-verifier] + needs: [tests, agent-tunnel-e2e, agent-policy-e2e, lints, check-dependencies, jetsocat-lipo, devolutions-gateway-powershell, devolutions-gateway, devolutions-gateway-merge, devolutions-pedm-desktop, devolutions-agent, devolutions-agent-merge, devolutions-pedm-client, dotnet-utils-tests, agent-installer-tests, winapi-sanitizer-tests, winapi-miri, pedm-simulator, secure-memory-verifier] runs-on: ubuntu-latest steps: diff --git a/package/AgentWindowsManaged.Tests/DevolutionsAgent.Installer.Tests.csproj b/package/AgentWindowsManaged.Tests/DevolutionsAgent.Installer.Tests.csproj new file mode 100644 index 000000000..8ee8be266 --- /dev/null +++ b/package/AgentWindowsManaged.Tests/DevolutionsAgent.Installer.Tests.csproj @@ -0,0 +1,32 @@ + + + net48 + latest + false + DevolutionsAgent.Installer.Tests + + + + + + + runtime; build; native; contentfiles; analyzers; buildtransitive + all + + + + + + + + + + + + + diff --git a/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs b/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs new file mode 100644 index 000000000..0e05951b7 --- /dev/null +++ b/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs @@ -0,0 +1,376 @@ +using DevolutionsAgent; +using DevolutionsAgent.Actions; +using System; +using System.Diagnostics; +using System.IO; +using System.Linq; +using System.Runtime.InteropServices; +using System.Security.AccessControl; +using System.Security.Principal; +using System.Text; +using WixSharp; +using Xunit; +using Action = WixSharp.Action; +using File = System.IO.File; +using RegistryHive = WixSharp.RegistryHive; + +namespace DevolutionsAgent.Installer.Tests; + +public sealed class PackageBrokerInstallerTests +{ + [Fact] + public void DedicatedPolicyAclAcceptsOnlySystemAndAdministrators() + { + DirectorySecurity security = + DirectorySecurity(DevolutionsAgent.Resources.Includes.PROGRAM_DATA_PACKAGE_BROKER_SDDL); + + PackageBrokerPolicyActions.VerifyPackageBrokerSecurity(security); + PackageBrokerPolicyActions.VerifySecurityDescriptor( + security, + DevolutionsAgent.Resources.Includes.PROGRAM_DATA_PACKAGE_BROKER_SDDL); + } + + [Theory] + [InlineData("O:BAG:SYD:P(A;;FA;;;SY)(A;;FA;;;BA)(A;;GR;;;LS)")] + [InlineData("O:SYG:SYD:AI(A;;FA;;;SY)(A;;FA;;;BA)")] + [InlineData("O:SYG:SYD:P(A;;FA;;;SY)(A;;FA;;;BA)(A;;GR;;;LS)")] + public void DedicatedPolicyAclRejectsAnythingOutsideStrictContract(string sddl) + { + Assert.Throws( + () => PackageBrokerPolicyActions.VerifyPackageBrokerSecurity(Security(sddl))); + } + + [Theory] + [InlineData("O:SYG:SYD:(A;;FA;;;SY)(A;;FA;;;BA)(A;;GR;;;LS)")] + [InlineData("O:BAG:SYD:(A;;FA;;;SY)(A;;FA;;;BA)(A;;GR;;;BU)")] + public void LegacySourceAllowsTrustedOwnerAndUntrustedRead(string sddl) + { + Assert.True( + PackageBrokerPolicyActions.TryVerifyLegacyPolicySourceSecurity( + Security(sddl), + out string diagnostic), + diagnostic); + } + + [Theory] + [InlineData("GW", "LS")] + [InlineData("0x2", "LS")] + [InlineData("GA", "BU")] + [InlineData("WD", "AU")] + [InlineData("WO", "LS")] + [InlineData("DC", "BU")] + public void LegacySourceRejectsUntrustedWriteOrTamperRights(string rights, string sid) + { + FileSecurity security = Security($"O:SYG:SYD:(A;;FA;;;SY)(A;;FA;;;BA)(A;;{rights};;;{sid})"); + + Assert.False( + PackageBrokerPolicyActions.TryVerifyLegacyPolicySourceSecurity( + security, + out string diagnostic)); + Assert.Contains("unsafe write or tamper rights", diagnostic); + } + + [Fact] + public void LegacySourceRejectsUntrustedOwner() + { + FileSecurity security = Security("O:BUG:SYD:(A;;FA;;;SY)(A;;FA;;;BA)"); + + Assert.False( + PackageBrokerPolicyActions.TryVerifyLegacyPolicySourceSecurity( + security, + out string diagnostic)); + Assert.Contains("untrusted owner", diagnostic); + } + + [Fact] + public void StrictConfigWithoutPolicyPathAllowsSourceCleanup() + { + Assert.True( + PackageBrokerPolicyActions.TryReadConfiguredPolicyPath( + """{"PackageBroker":{}}""", + out string configuredPath, + out string diagnostic), + diagnostic); + Assert.Null(configuredPath); + } + + [Fact] + public void StrictConfigReturnsAbsoluteJsonPolicyPath() + { + const string path = @"C:\ProgramData\Devolutions\Agent\package-broker-policy.json"; + + Assert.True( + PackageBrokerPolicyActions.TryReadConfiguredPolicyPath( + $"{{\"PackageBroker\":{{\"PolicyPath\":\"{path.Replace(@"\", @"\\")}\"}}}}", + out string configuredPath, + out string diagnostic), + diagnostic); + Assert.Equal(path, configuredPath); + } + + [Theory] + [InlineData("""{"PackageBroker":{"PolicyPath":"C:\\policy.json",},}""")] + [InlineData("""{"PackageBroker":{/*comment*/"PolicyPath":"C:\\policy.json"}}""")] + [InlineData("""{'PackageBroker':{'PolicyPath':'C:\\policy.json'}}""")] + [InlineData("""{PackageBroker:{PolicyPath:"C:\\policy.json"}}""")] + [InlineData("""{"PackageBroker":{"PolicyPath":"C:\\first.json","PolicyPath":"C:\\second.json"}}""")] + [InlineData("""{"PackageBroker":{}} {}""")] + [InlineData("""{"PackageBroker":{"PolicyPath":42}}""")] + [InlineData("""{"PackageBroker":{"PolicyPath":"relative.json"}}""")] + public void AmbiguousConfigPreservesLegacySource(string json) + { + Assert.False( + PackageBrokerPolicyActions.TryReadConfiguredPolicyPath( + json, + out _, + out string diagnostic)); + Assert.False(string.IsNullOrWhiteSpace(diagnostic)); + } + + [Fact] + public void MigrationMarkerRoundTripsAllBindings() + { + PackageBrokerPolicyActions.MigrationRecord record = + new("source-id", "source-digest", "destination-id", "destination-digest"); + + PackageBrokerPolicyActions.MigrationRecord parsed = + PackageBrokerPolicyActions.ReadMigrationMarkerJson(record.ToJson()); + + Assert.Equal(record.SourceIdentity, parsed.SourceIdentity); + Assert.Equal(record.SourceDigest, parsed.SourceDigest); + Assert.Equal(record.DestinationIdentity, parsed.DestinationIdentity); + Assert.Equal(record.DestinationDigest, parsed.DestinationDigest); + } + + [Theory] + [InlineData("{}")] + [InlineData("""{"SourceIdentity":"id","SourceDigest":"digest"}""")] + public void MigrationMarkerRejectsIncompleteBindings(string json) + { + Assert.Throws( + () => PackageBrokerPolicyActions.ReadMigrationMarkerJson(json)); + } + + [Fact] + public void PinnedFileIdentityAndDigestDetectContentMutation() + { + using TempDirectory temp = new(); + string path = Path.Combine(temp.Path, "policy.json"); + File.WriteAllText(path, "before"); + + string identity; + string digest; + using (PackageBrokerPolicyActions.PinnedPath pinned = PinFile(path, WinAPI.GENERIC_READ)) + { + identity = PackageBrokerPolicyActions.FileIdentity(pinned.Leaf); + digest = PackageBrokerPolicyActions.FileContentDigest(pinned.Leaf); + Assert.True(PackageBrokerPolicyActions.FileIdentityAndDigestMatch(pinned.Leaf, identity, digest)); + } + + File.WriteAllText(path, "after"); + using PackageBrokerPolicyActions.PinnedPath changed = PinFile(path, WinAPI.GENERIC_READ); + Assert.Equal(identity, PackageBrokerPolicyActions.FileIdentity(changed.Leaf)); + Assert.False(PackageBrokerPolicyActions.FileIdentityAndDigestMatch(changed.Leaf, identity, digest)); + } + + [Fact] + public void MissingPinnedLeafDoesNotCreateIt() + { + using TempDirectory temp = new(); + string path = Path.Combine(temp.Path, "missing.json"); + + using PackageBrokerPolicyActions.PinnedPath pinned = + PackageBrokerPolicyActions.PinPathWithoutReparse( + path, + leafIsDirectory: false, + allowMissingLeaf: true, + leafAccess: WinAPI.FILE_READ_ATTRIBUTES); + + Assert.Null(pinned.Leaf); + Assert.False(File.Exists(path)); + } + + [Fact] + public void HandleTargetedDeletionDeletesPinnedFile() + { + using TempDirectory temp = new(); + string path = Path.Combine(temp.Path, "delete.json"); + File.WriteAllText(path, "{}"); + + using (PackageBrokerPolicyActions.PinnedPath pinned = PinFile( + path, + WinAPI.DELETE | WinAPI.FILE_READ_ATTRIBUTES)) + { + PackageBrokerPolicyActions.DeleteFileByHandle(pinned.Leaf); + } + + Assert.False(File.Exists(path)); + } + + [Fact] + public void HardLinkedFileIsRejected() + { + using TempDirectory temp = new(); + string path = Path.Combine(temp.Path, "policy.json"); + string alias = Path.Combine(temp.Path, "alias.json"); + File.WriteAllText(path, "{}"); + Assert.True(CreateHardLink(alias, path, IntPtr.Zero)); + + Assert.Throws(() => + { + using PackageBrokerPolicyActions.PinnedPath _ = PinFile(path, WinAPI.FILE_READ_ATTRIBUTES); + }); + } + + [Fact] + public void DirectoryReparsePointIsRejectedWithoutTouchingTarget() + { + using TempDirectory temp = new(); + string target = Directory.CreateDirectory(Path.Combine(temp.Path, "target")).FullName; + string link = Path.Combine(temp.Path, "link"); + using Process process = Process.Start(new ProcessStartInfo + { + FileName = "cmd.exe", + Arguments = $"/d /c mklink /J \"{link}\" \"{target}\"", + CreateNoWindow = true, + UseShellExecute = false, + }); + process.WaitForExit(); + Assert.Equal(0, process.ExitCode); + + Assert.Throws(() => + { + using PackageBrokerPolicyActions.PinnedPath _ = + PackageBrokerPolicyActions.PinPathWithoutReparse( + link, + leafIsDirectory: true, + allowMissingLeaf: false, + leafAccess: WinAPI.FILE_READ_ATTRIBUTES); + }); + Assert.Empty(Directory.EnumerateFileSystemEntries(target)); + Directory.Delete(link); + } + + [Fact] + public void SecureDirectoryCreationAppliesDescriptorAtCreation() + { + using TempDirectory temp = new(); + string path = Path.Combine(temp.Path, "secured"); + string sid = WindowsIdentity.GetCurrent().User.Value; + string sddl = $"O:{sid}G:{sid}D:P(A;OICI;FA;;;{sid})"; + + PackageBrokerPolicyActions.CreateDirectoryWithSecurity(path, sddl); + + PackageBrokerPolicyActions.VerifySecurityDescriptor( + new DirectoryInfo(path).GetAccessControl(), + sddl); + } + + [Fact] + public void MigrationActionsUseDeferredRollbackCommitSequence() + { + ManagedAction ensure = ActionFor(nameof(PackageBrokerPolicyActions.EnsureProgramDataPackageBrokerDirectory)); + ManagedAction rollback = ActionFor(nameof(PackageBrokerPolicyActions.RollbackLegacyPackageBrokerPolicyMigration)); + ManagedAction migrate = ActionFor(nameof(PackageBrokerPolicyActions.MigrateLegacyPackageBrokerPolicy)); + ManagedAction commit = ActionFor(nameof(PackageBrokerPolicyActions.CommitLegacyPackageBrokerPolicyMigration)); + + Assert.Equal(Execute.deferred, ensure.Execute); + Assert.Equal(Execute.rollback, rollback.Execute); + Assert.Equal(Execute.deferred, migrate.Execute); + Assert.Equal(Execute.commit, commit.Execute); + Assert.False(ensure.Impersonate); + Assert.False(rollback.Impersonate); + Assert.False(migrate.Impersonate); + Assert.False(commit.Impersonate); + Assert.Equal(Return.ignore, rollback.Return); + Assert.Equal(Return.check, ensure.Return); + Assert.Equal(Return.check, migrate.Return); + Assert.Equal(Return.check, commit.Return); + Assert.Equal(When.Before, rollback.When); + Assert.Equal(When.After, migrate.When); + Assert.Equal(When.After, commit.When); + Assert.Equal(migrate.Id, rollback.Step.ToString()); + Assert.Equal(ensure.Id, migrate.Step.ToString()); + Assert.Contains("createProgramDataDirectory", ensure.Step.ToString()); + Assert.Equal(migrate.Id, commit.Step.ToString()); + Assert.Equal(Condition.NOT_BeingRemoved.ToString(), ensure.Condition.ToString()); + Assert.Equal(Condition.NOT_BeingRemoved.ToString(), migrate.Condition.ToString()); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public void EventLogSourceUsesNativeMsiRegistryLifecycle(bool win64) + { + RegValue value = Program.CreateEventLogSourceRegistryValue(win64); + + Assert.Equal(RegistryHive.LocalMachine, value.Root); + Assert.Equal( + @"SYSTEM\CurrentControlSet\Services\EventLog\Application\Devolutions Agent", + value.Key); + Assert.Equal("EventMessageFile", value.Name); + Assert.Equal("[INSTALLDIR]DevolutionsAgent.exe", value.Value); + Assert.Equal(win64, value.Win64); + Assert.Equal(RegistryKeyAction.createAndRemoveOnUninstall, value.RegistryKeyAction); + Assert.False(value.ForceCreateOnInstall); + Assert.False(value.ForceDeleteOnUninstall); + Assert.Contains("Type=string", value.AttributesDefinition); + } + + private static ManagedAction ActionFor(string methodName) => + Assert.IsAssignableFrom( + AgentActions.Actions.Single( + action => action is ManagedAction managed && managed.MethodName == methodName)); + + private static PackageBrokerPolicyActions.PinnedPath PinFile(string path, uint access) => + PackageBrokerPolicyActions.PinPathWithoutReparse( + path, + leafIsDirectory: false, + allowMissingLeaf: false, + leafAccess: access); + + private static FileSecurity Security(string sddl) + { + RawSecurityDescriptor descriptor = new(sddl); + byte[] binary = new byte[descriptor.BinaryLength]; + descriptor.GetBinaryForm(binary, 0); + FileSecurity security = new(); + security.SetSecurityDescriptorBinaryForm(binary); + return security; + } + + private static DirectorySecurity DirectorySecurity(string sddl) + { + RawSecurityDescriptor descriptor = new(sddl); + byte[] binary = new byte[descriptor.BinaryLength]; + descriptor.GetBinaryForm(binary, 0); + DirectorySecurity security = new(); + security.SetSecurityDescriptorBinaryForm(binary); + return security; + } + + [DllImport("kernel32", EntryPoint = "CreateHardLinkW", CharSet = CharSet.Unicode, SetLastError = true)] + [return: MarshalAs(UnmanagedType.Bool)] + private static extern bool CreateHardLink(string fileName, string existingFileName, IntPtr securityAttributes); + + private sealed class TempDirectory : IDisposable + { + internal TempDirectory() + { + Path = System.IO.Path.Combine( + System.IO.Path.GetTempPath(), + $"DevolutionsAgentInstallerTests-{Guid.NewGuid():N}"); + Directory.CreateDirectory(Path); + } + + internal string Path { get; } + + public void Dispose() + { + if (Directory.Exists(Path)) + { + Directory.Delete(Path, recursive: true); + } + } + } +} From e6993486730a9054685c96bc9722cce4c9605013 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Tue, 8 Sep 2026 16:26:55 -0400 Subject: [PATCH 3/9] fix(agent-installer): address review feedback Build the installer test reference through its actual SDK target path without invoking MSI authoring. Ignore unrelated audit entries when comparing directory security while preserving owner, group, protected DACL, and access validation. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../DevolutionsAgent.Installer.Tests.csproj | 13 +--------- .../PackageBrokerInstallerTests.cs | 24 +++++++++++++++++++ .../Actions/PackageBrokerPolicyActions.cs | 13 ++++++++-- 3 files changed, 36 insertions(+), 14 deletions(-) diff --git a/package/AgentWindowsManaged.Tests/DevolutionsAgent.Installer.Tests.csproj b/package/AgentWindowsManaged.Tests/DevolutionsAgent.Installer.Tests.csproj index 8ee8be266..e54f1240d 100644 --- a/package/AgentWindowsManaged.Tests/DevolutionsAgent.Installer.Tests.csproj +++ b/package/AgentWindowsManaged.Tests/DevolutionsAgent.Installer.Tests.csproj @@ -16,17 +16,6 @@ - + - - - - - - diff --git a/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs b/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs index 0e05951b7..219f5728d 100644 --- a/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs +++ b/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs @@ -266,6 +266,30 @@ public void SecureDirectoryCreationAppliesDescriptorAtCreation() sddl); } + [Fact] + public void SecurityDescriptorComparisonIgnoresAuditRules() + { + const string expected = "O:SYG:SYD:P(A;OICI;FA;;;SY)(A;OICI;FA;;;BA)"; + DirectorySecurity actual = + DirectorySecurity($"{expected}S:(AU;SA;FA;;;WD)"); + + PackageBrokerPolicyActions.VerifySecurityDescriptor(actual, expected); + } + + [Theory] + [InlineData("O:BAG:SYD:P(A;OICI;FA;;;SY)(A;OICI;FA;;;BA)")] + [InlineData("O:SYG:SYD:P(A;OICI;FA;;;SY)")] + [InlineData("O:SYG:SYD:AI(A;OICI;FA;;;SY)(A;OICI;FA;;;BA)")] + public void SecurityDescriptorComparisonRejectsOwnerDaclOrProtectionChanges(string actualSddl) + { + const string expected = "O:SYG:SYD:P(A;OICI;FA;;;SY)(A;OICI;FA;;;BA)"; + + Assert.Throws( + () => PackageBrokerPolicyActions.VerifySecurityDescriptor( + DirectorySecurity(actualSddl), + expected)); + } + [Fact] public void MigrationActionsUseDeferredRollbackCommitSequence() { diff --git a/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs b/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs index 7633d6e34..e28ea3e66 100644 --- a/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs +++ b/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs @@ -921,9 +921,18 @@ private static void SetFileSecurity(string path, string sddl) internal static void VerifySecurityDescriptor(FileSystemSecurity actual, string expectedSddl) { + if (!actual.AreAccessRulesProtected) + { + throw new InvalidOperationException("new directory DACL inheritance is not protected"); + } + + const AccessControlSections sections = + AccessControlSections.Owner | + AccessControlSections.Group | + AccessControlSections.Access; RawSecurityDescriptor expected = new(expectedSddl); - string expectedCanonical = expected.GetSddlForm(AccessControlSections.All); - string actualCanonical = actual.GetSecurityDescriptorSddlForm(AccessControlSections.All); + string expectedCanonical = expected.GetSddlForm(sections); + string actualCanonical = actual.GetSecurityDescriptorSddlForm(sections); if (!string.Equals(actualCanonical, expectedCanonical, StringComparison.Ordinal)) { throw new InvalidOperationException("new directory security does not match its creation descriptor"); From e6a9e3aecb4ad62ad5be973406be3b6ac2b932ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Tue, 8 Sep 2026 16:49:53 -0400 Subject: [PATCH 4/9] fix(agent-installer): harden migration cleanup Reject NULL and empty DACLs before trusting legacy sources or policy ancestors. Reacquire delete access after retiring the migration marker and preserve both copies when source cleanup cannot safely proceed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../PackageBrokerInstallerTests.cs | 84 ++++++++++- .../Actions/PackageBrokerPolicyActions.cs | 141 ++++++++++++++---- 2 files changed, 190 insertions(+), 35 deletions(-) diff --git a/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs b/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs index 219f5728d..bad0921bb 100644 --- a/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs +++ b/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs @@ -82,6 +82,29 @@ public void LegacySourceRejectsUntrustedOwner() Assert.Contains("untrusted owner", diagnostic); } + [Theory] + [InlineData("O:SYG:SY")] + [InlineData("O:SYG:SYD:P")] + public void LegacySourceRejectsNullOrEmptyDacl(string sddl) + { + Assert.False( + PackageBrokerPolicyActions.TryVerifyLegacyPolicySourceSecurity( + Security(sddl), + out string diagnostic)); + Assert.Contains("DACL", diagnostic); + } + + [Theory] + [InlineData("O:SYG:SY")] + [InlineData("O:SYG:SYD:P")] + public void TrustedAncestorRejectsNullOrEmptyDacl(string sddl) + { + InvalidOperationException error = Assert.Throws( + () => PackageBrokerPolicyActions.VerifyTrustedDirectorySecurity( + DirectorySecurity(sddl))); + Assert.Contains("DACL", error.Message); + } + [Fact] public void StrictConfigWithoutPolicyPathAllowsSourceCleanup() { @@ -199,14 +222,71 @@ public void HandleTargetedDeletionDeletesPinnedFile() using (PackageBrokerPolicyActions.PinnedPath pinned = PinFile( path, - WinAPI.DELETE | WinAPI.FILE_READ_ATTRIBUTES)) + WinAPI.GENERIC_READ | WinAPI.DELETE | WinAPI.FILE_READ_ATTRIBUTES)) { - PackageBrokerPolicyActions.DeleteFileByHandle(pinned.Leaf); + string identity = PackageBrokerPolicyActions.FileIdentity(pinned.Leaf); + string digest = PackageBrokerPolicyActions.FileContentDigest(pinned.Leaf); + Assert.True( + PackageBrokerPolicyActions.DeleteFileIfIdentityAndDigestMatch( + pinned.Leaf, + identity, + digest)); } Assert.False(File.Exists(path)); } + [Fact] + public void ChangedIdentityIsPreservedByDeleteBinding() + { + using TempDirectory temp = new(); + string path = Path.Combine(temp.Path, "source.json"); + File.WriteAllText(path, "original"); + string identity; + string digest; + using (PackageBrokerPolicyActions.PinnedPath source = PinFile(path, WinAPI.GENERIC_READ)) + { + identity = PackageBrokerPolicyActions.FileIdentity(source.Leaf); + digest = PackageBrokerPolicyActions.FileContentDigest(source.Leaf); + } + + File.Delete(path); + File.WriteAllText(path, "replacement"); + using PackageBrokerPolicyActions.PinnedPath replacement = PinFile( + path, + WinAPI.GENERIC_READ | WinAPI.DELETE | WinAPI.FILE_READ_ATTRIBUTES); + Assert.False( + PackageBrokerPolicyActions.DeleteFileIfIdentityAndDigestMatch( + replacement.Leaf, + identity, + digest)); + Assert.True(File.Exists(path)); + } + + [Fact] + public void UnavailableDeleteHandlePreservesLegacySource() + { + using TempDirectory temp = new(); + string path = Path.Combine(temp.Path, "source.json"); + File.WriteAllText(path, "{}"); + using PackageBrokerPolicyActions.PinnedPath source = PinFile(path, WinAPI.GENERIC_READ); + PackageBrokerPolicyActions.MigrationRecord record = new( + PackageBrokerPolicyActions.FileIdentity(source.Leaf), + PackageBrokerPolicyActions.FileContentDigest(source.Leaf), + "destination", + "digest"); + using FileStream blocker = new(path, FileMode.Open, FileAccess.Read, FileShare.Read); + string diagnostic = null; + + Assert.False( + PackageBrokerPolicyActions.TryDeleteLegacyPolicySource( + message => diagnostic = message, + path, + record)); + Assert.True(File.Exists(path)); + Assert.Contains("preserving both copies", diagnostic); + } + [Fact] public void HardLinkedFileIsRejected() { diff --git a/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs b/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs index e28ea3e66..e6bc415bb 100644 --- a/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs +++ b/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs @@ -240,33 +240,37 @@ public static ActionResult CommitLegacyPackageBrokerPolicyMigration(Session sess VerifyPackageBrokerSecurity(SecurityFromHandle(markerPath.Leaf, isDirectory: false)); MigrationRecord record = ReadMigrationMarker(markerPath.Leaf); - using PinnedPath source = PinPathWithoutReparse( + bool sourceChanged; + bool removeSource; + using (PinnedPath source = PinPathWithoutReparse( sourcePath, leafIsDirectory: false, allowMissingLeaf: true, - leafAccess: WinAPI.GENERIC_READ | WinAPI.DELETE | WinAPI.FILE_READ_ATTRIBUTES | WinAPI.READ_CONTROL); - bool sourceChanged = - source.Leaf == null || - !FileIdentityAndDigestMatch(source.Leaf, record.SourceIdentity, record.SourceDigest); - bool removeSource = !sourceChanged; - if (removeSource && - IsLegacyPackageBrokerPolicyExplicitlyConfigured( - sourcePath, - source.Leaf, - out string configuredDiagnostic)) - { - session.Log( - $"preserving the configured legacy package broker policy during commit: {configuredDiagnostic}"); - removeSource = false; - } - if (removeSource && - !TryVerifyLegacyPolicySourceSecurity( - SecurityFromHandle(source.Leaf, isDirectory: false), - out string sourceSecurityDiagnostic)) + leafAccess: WinAPI.GENERIC_READ | WinAPI.FILE_READ_ATTRIBUTES | WinAPI.READ_CONTROL)) { - session.Log( - $"preserving the legacy package broker policy during commit: {sourceSecurityDiagnostic}"); - removeSource = false; + sourceChanged = + source.Leaf == null || + !FileIdentityAndDigestMatch(source.Leaf, record.SourceIdentity, record.SourceDigest); + removeSource = !sourceChanged; + if (removeSource && + IsLegacyPackageBrokerPolicyExplicitlyConfigured( + sourcePath, + source.Leaf, + out string configuredDiagnostic)) + { + session.Log( + $"preserving the configured legacy package broker policy during commit: {configuredDiagnostic}"); + removeSource = false; + } + if (removeSource && + !TryVerifyLegacyPolicySourceSecurity( + SecurityFromHandle(source.Leaf, isDirectory: false), + out string sourceSecurityDiagnostic)) + { + session.Log( + $"preserving the legacy package broker policy during commit: {sourceSecurityDiagnostic}"); + removeSource = false; + } } VerifyPackageBrokerSecurity(SecurityFromHandle(markerPath.Leaf, isDirectory: false)); @@ -282,15 +286,7 @@ public static ActionResult CommitLegacyPackageBrokerPolicyMigration(Session sess return ActionResult.Success; } - try - { - DeleteFileByHandle(source.Leaf); - } - catch (Exception error) - { - session.Log( - $"failed to remove the migrated legacy package broker policy; preserving both copies: {error}"); - } + TryDeleteLegacyPolicySource(session, sourcePath, record); return ActionResult.Success; } @@ -393,6 +389,7 @@ internal static void EnsureSecureDirectoryTree(string programData, string target internal static void VerifyPackageBrokerSecurity(FileSystemSecurity security) { + VerifyDaclPresentAndNonEmpty(security, "package broker path"); SecurityIdentifier system = new(WellKnownSidType.LocalSystemSid, null); SecurityIdentifier administrators = new(WellKnownSidType.BuiltinAdministratorsSid, null); SecurityIdentifier owner = (SecurityIdentifier)security.GetOwner(typeof(SecurityIdentifier)); @@ -669,6 +666,20 @@ internal static void DeleteFileByHandle(SafeFileHandle handle) } } + internal static bool DeleteFileIfIdentityAndDigestMatch( + SafeFileHandle handle, + string expectedIdentity, + string expectedDigest) + { + if (!FileIdentityAndDigestMatch(handle, expectedIdentity, expectedDigest)) + { + return false; + } + + DeleteFileByHandle(handle); + return true; + } + internal static MigrationRecord ReadMigrationMarkerJson(string markerJson) { JObject document = JObject.Parse(markerJson); @@ -803,7 +814,7 @@ private static void VerifyLegacyPolicySourceSecurity(FileSystemSecurity security "unsafe write or tamper"); } - private static void VerifyTrustedDirectorySecurity(FileSystemSecurity security) + internal static void VerifyTrustedDirectorySecurity(FileSystemSecurity security) { const FileSystemRights tamperRights = FileSystemRights.Delete | @@ -824,6 +835,7 @@ private static void VerifyTrustedOwnerAndNoUnsafeGrants( string subject, string accessDescription) { + VerifyDaclPresentAndNonEmpty(security, subject); SecurityIdentifier system = new(WellKnownSidType.LocalSystemSid, null); SecurityIdentifier administrators = new(WellKnownSidType.BuiltinAdministratorsSid, null); SecurityIdentifier trustedInstaller = @@ -857,6 +869,21 @@ private static void VerifyTrustedOwnerAndNoUnsafeGrants( } } + private static void VerifyDaclPresentAndNonEmpty(FileSystemSecurity security, string subject) + { + RawSecurityDescriptor descriptor = + new(security.GetSecurityDescriptorBinaryForm(), 0); + if (!descriptor.ControlFlags.HasFlag(ControlFlags.DiscretionaryAclPresent) || + descriptor.DiscretionaryAcl == null) + { + throw new InvalidOperationException($"{subject} has a NULL DACL granting full control to everyone"); + } + if (descriptor.DiscretionaryAcl.Count == 0) + { + throw new InvalidOperationException($"{subject} has an empty DACL with no trusted access entries"); + } + } + internal static void CreateDirectoryWithSecurity(string path, string sddl) { const uint sdRevision = 1; @@ -1117,6 +1144,54 @@ private static void TryDeleteTemporaryFile(Session session, string path) } } + internal static bool TryDeleteLegacyPolicySource( + Action log, + string sourcePath, + MigrationRecord record) + { + try + { + using PinnedPath source = PinPathWithoutReparse( + sourcePath, + leafIsDirectory: false, + allowMissingLeaf: true, + leafAccess: WinAPI.GENERIC_READ | WinAPI.DELETE | WinAPI.FILE_READ_ATTRIBUTES | WinAPI.READ_CONTROL); + if (source.Leaf == null) + { + log("legacy package broker policy disappeared before cleanup; preserving the migrated copy"); + return false; + } + if (!FileIdentityAndDigestMatch(source.Leaf, record.SourceIdentity, record.SourceDigest)) + { + log("legacy package broker policy changed before cleanup; preserving both copies"); + return false; + } + if (!TryVerifyLegacyPolicySourceSecurity( + SecurityFromHandle(source.Leaf, isDirectory: false), + out string sourceSecurityDiagnostic)) + { + log($"legacy package broker policy became unsafe before cleanup: {sourceSecurityDiagnostic}"); + return false; + } + + return DeleteFileIfIdentityAndDigestMatch( + source.Leaf, + record.SourceIdentity, + record.SourceDigest); + } + catch (Exception error) + { + log($"failed to remove the migrated legacy package broker policy; preserving both copies: {error}"); + return false; + } + } + + private static bool TryDeleteLegacyPolicySource( + Session session, + string sourcePath, + MigrationRecord record) => + TryDeleteLegacyPolicySource(session.Log, sourcePath, record); + internal sealed class PinnedPath : IDisposable { private readonly IReadOnlyList handles; From 5201d7e71aaaa6f428c486c49bbad49b64296970 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Tue, 8 Sep 2026 17:09:37 -0400 Subject: [PATCH 5/9] fix(agent-installer): make commit cleanup best-effort Prevent commit-phase marker or source cleanup failures from turning a successful migration into an installed-but-failed MSI result. Keep setup and migration checked while logging every commit cleanup error. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../PackageBrokerInstallerTests.cs | 19 ++- .../Actions/AgentActions.cs | 2 +- .../Actions/PackageBrokerPolicyActions.cs | 130 ++++++++++-------- 3 files changed, 88 insertions(+), 63 deletions(-) diff --git a/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs b/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs index bad0921bb..599857cc0 100644 --- a/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs +++ b/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs @@ -1,5 +1,6 @@ using DevolutionsAgent; using DevolutionsAgent.Actions; +using Microsoft.Deployment.WindowsInstaller; using System; using System.Diagnostics; using System.IO; @@ -389,7 +390,7 @@ public void MigrationActionsUseDeferredRollbackCommitSequence() Assert.Equal(Return.ignore, rollback.Return); Assert.Equal(Return.check, ensure.Return); Assert.Equal(Return.check, migrate.Return); - Assert.Equal(Return.check, commit.Return); + Assert.Equal(Return.ignore, commit.Return); Assert.Equal(When.Before, rollback.When); Assert.Equal(When.After, migrate.When); Assert.Equal(When.After, commit.When); @@ -401,6 +402,22 @@ public void MigrationActionsUseDeferredRollbackCommitSequence() Assert.Equal(Condition.NOT_BeingRemoved.ToString(), migrate.Condition.ToString()); } + [Theory] + [InlineData("marker inspection")] + [InlineData("marker deletion")] + [InlineData("source cleanup")] + public void CommitCleanupFailuresRemainSuccessful(string stage) + { + string diagnostic = null; + + ActionResult result = PackageBrokerPolicyActions.RunBestEffortCommit( + message => diagnostic = message, + () => throw new IOException(stage)); + + Assert.Equal(ActionResult.Success, result); + Assert.Contains(stage, diagnostic); + } + [Theory] [InlineData(true)] [InlineData(false)] diff --git a/package/AgentWindowsManaged/Actions/AgentActions.cs b/package/AgentWindowsManaged/Actions/AgentActions.cs index 429fc5425..3c9f98c3d 100644 --- a/package/AgentWindowsManaged/Actions/AgentActions.cs +++ b/package/AgentWindowsManaged/Actions/AgentActions.cs @@ -192,7 +192,7 @@ internal static class AgentActions private static readonly ElevatedManagedAction commitLegacyPackageBrokerPolicyMigration = new( new Id($"CA.{nameof(commitLegacyPackageBrokerPolicyMigration)}"), PackageBrokerPolicyActions.CommitLegacyPackageBrokerPolicyMigration, - Return.check, + Return.ignore, When.After, new Step(migrateLegacyPackageBrokerPolicy.Id), Condition.NOT_BeingRemoved, Sequence.InstallExecuteSequence) diff --git a/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs b/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs index e6bc415bb..440657d46 100644 --- a/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs +++ b/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs @@ -220,81 +220,89 @@ public static ActionResult RollbackLegacyPackageBrokerPolicyMigration(Session se } [CustomAction] - public static ActionResult CommitLegacyPackageBrokerPolicyMigration(Session session) + public static ActionResult CommitLegacyPackageBrokerPolicyMigration(Session session) => + RunBestEffortCommit( + session.Log, + () => CommitLegacyPackageBrokerPolicyMigrationCore(session)); + + internal static ActionResult RunBestEffortCommit(Action log, Action commit) + { + try + { + commit(); + } + catch (Exception error) + { + log($"failed to commit legacy package broker policy migration: {error}"); + } + + return ActionResult.Success; + } + + private static void CommitLegacyPackageBrokerPolicyMigrationCore(Session session) { string marker = MigrationMarkerPath(session); string sourcePath = LegacyPolicyPath; - - try + using PinnedPath markerPath = PinPathWithoutReparse( + marker, + leafIsDirectory: false, + allowMissingLeaf: true, + leafAccess: WinAPI.GENERIC_READ | WinAPI.DELETE | WinAPI.FILE_READ_ATTRIBUTES | WinAPI.READ_CONTROL); + if (markerPath.Leaf == null) { - using PinnedPath markerPath = PinPathWithoutReparse( - marker, - leafIsDirectory: false, - allowMissingLeaf: true, - leafAccess: WinAPI.GENERIC_READ | WinAPI.DELETE | WinAPI.FILE_READ_ATTRIBUTES | WinAPI.READ_CONTROL); - if (markerPath.Leaf == null) - { - return ActionResult.Success; - } + return; + } - VerifyPackageBrokerSecurity(SecurityFromHandle(markerPath.Leaf, isDirectory: false)); - MigrationRecord record = ReadMigrationMarker(markerPath.Leaf); + VerifyPackageBrokerSecurity(SecurityFromHandle(markerPath.Leaf, isDirectory: false)); + MigrationRecord record = ReadMigrationMarker(markerPath.Leaf); - bool sourceChanged; - bool removeSource; - using (PinnedPath source = PinPathWithoutReparse( - sourcePath, - leafIsDirectory: false, - allowMissingLeaf: true, - leafAccess: WinAPI.GENERIC_READ | WinAPI.FILE_READ_ATTRIBUTES | WinAPI.READ_CONTROL)) + bool sourceChanged; + bool removeSource; + using (PinnedPath source = PinPathWithoutReparse( + sourcePath, + leafIsDirectory: false, + allowMissingLeaf: true, + leafAccess: WinAPI.GENERIC_READ | WinAPI.FILE_READ_ATTRIBUTES | WinAPI.READ_CONTROL)) + { + sourceChanged = + source.Leaf == null || + !FileIdentityAndDigestMatch(source.Leaf, record.SourceIdentity, record.SourceDigest); + removeSource = !sourceChanged; + if (removeSource && + IsLegacyPackageBrokerPolicyExplicitlyConfigured( + sourcePath, + source.Leaf, + out string configuredDiagnostic)) { - sourceChanged = - source.Leaf == null || - !FileIdentityAndDigestMatch(source.Leaf, record.SourceIdentity, record.SourceDigest); - removeSource = !sourceChanged; - if (removeSource && - IsLegacyPackageBrokerPolicyExplicitlyConfigured( - sourcePath, - source.Leaf, - out string configuredDiagnostic)) - { - session.Log( - $"preserving the configured legacy package broker policy during commit: {configuredDiagnostic}"); - removeSource = false; - } - if (removeSource && - !TryVerifyLegacyPolicySourceSecurity( - SecurityFromHandle(source.Leaf, isDirectory: false), - out string sourceSecurityDiagnostic)) - { - session.Log( - $"preserving the legacy package broker policy during commit: {sourceSecurityDiagnostic}"); - removeSource = false; - } + session.Log( + $"preserving the configured legacy package broker policy during commit: {configuredDiagnostic}"); + removeSource = false; } - - VerifyPackageBrokerSecurity(SecurityFromHandle(markerPath.Leaf, isDirectory: false)); - DeleteFileByHandle(markerPath.Leaf); - - if (!removeSource) + if (removeSource && + !TryVerifyLegacyPolicySourceSecurity( + SecurityFromHandle(source.Leaf, isDirectory: false), + out string sourceSecurityDiagnostic)) { - if (sourceChanged) - { - session.Log( - "legacy package broker policy changed after migration; preserving the current source"); - } - return ActionResult.Success; + session.Log( + $"preserving the legacy package broker policy during commit: {sourceSecurityDiagnostic}"); + removeSource = false; } + } - TryDeleteLegacyPolicySource(session, sourcePath, record); + VerifyPackageBrokerSecurity(SecurityFromHandle(markerPath.Leaf, isDirectory: false)); + DeleteFileByHandle(markerPath.Leaf); - return ActionResult.Success; - } - catch (Exception error) + if (!removeSource) { - session.Log($"failed to commit legacy package broker policy migration: {error}"); - return ActionResult.Failure; + if (sourceChanged) + { + session.Log( + "legacy package broker policy changed after migration; preserving the current source"); + } + return; } + + TryDeleteLegacyPolicySource(session, sourcePath, record); } internal static void EnsureSecureDirectoryTree(string programData, string target) From 1966fc8e88d218b047de98e40bc2a198aa7eb540 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Tue, 8 Sep 2026 17:30:21 -0400 Subject: [PATCH 6/9] fix(agent-installer): preserve migration collisions Publish migrated policies with native no-replace semantics. Treat only destination-exists races as safe skips, retire the exact marker, and clean only the bound temporary while preserving external destinations and legacy sources. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../PackageBrokerInstallerTests.cs | 66 ++++++++++++++++ .../Actions/PackageBrokerPolicyActions.cs | 77 ++++++++++++++++--- package/AgentWindowsManaged/Actions/WinAPI.cs | 1 + 3 files changed, 133 insertions(+), 11 deletions(-) diff --git a/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs b/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs index 599857cc0..a6ef60333 100644 --- a/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs +++ b/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs @@ -2,6 +2,7 @@ using DevolutionsAgent.Actions; using Microsoft.Deployment.WindowsInstaller; using System; +using System.ComponentModel; using System.Diagnostics; using System.IO; using System.Linq; @@ -288,6 +289,71 @@ public void UnavailableDeleteHandlePreservesLegacySource() Assert.Contains("preserving both copies", diagnostic); } + [Fact] + public void NoReplaceMovePreservesCollisionAndCleansOnlyBoundTemporary() + { + using TempDirectory temp = new(); + string source = Path.Combine(temp.Path, "legacy.json"); + string temporary = Path.Combine(temp.Path, "migration.tmp"); + string destination = Path.Combine(temp.Path, "managed.json"); + File.WriteAllText(source, "legacy"); + File.WriteAllText(temporary, "migrated"); + File.WriteAllText(destination, "external"); + string temporaryIdentity; + string temporaryDigest; + using (PackageBrokerPolicyActions.PinnedPath pinned = PinFile(temporary, WinAPI.GENERIC_READ)) + { + temporaryIdentity = PackageBrokerPolicyActions.FileIdentity(pinned.Leaf); + temporaryDigest = PackageBrokerPolicyActions.FileContentDigest(pinned.Leaf); + } + + Assert.Equal( + PackageBrokerPolicyActions.NoReplaceMoveResult.DestinationExists, + PackageBrokerPolicyActions.MoveFileNoReplace(temporary, destination)); + Assert.Equal("legacy", File.ReadAllText(source)); + Assert.Equal("external", File.ReadAllText(destination)); + + using (PackageBrokerPolicyActions.PinnedPath pinned = PinFile( + temporary, + WinAPI.GENERIC_READ | WinAPI.DELETE | WinAPI.FILE_READ_ATTRIBUTES)) + { + Assert.True( + PackageBrokerPolicyActions.DeleteFileIfIdentityAndDigestMatch( + pinned.Leaf, + temporaryIdentity, + temporaryDigest)); + } + Assert.False(File.Exists(temporary)); + Assert.Equal("external", File.ReadAllText(destination)); + } + + [Fact] + public void NoReplaceMovePublishesWhenDestinationIsMissing() + { + using TempDirectory temp = new(); + string temporary = Path.Combine(temp.Path, "migration.tmp"); + string destination = Path.Combine(temp.Path, "managed.json"); + File.WriteAllText(temporary, "migrated"); + + Assert.Equal( + PackageBrokerPolicyActions.NoReplaceMoveResult.Moved, + PackageBrokerPolicyActions.MoveFileNoReplace(temporary, destination)); + Assert.False(File.Exists(temporary)); + Assert.Equal("migrated", File.ReadAllText(destination)); + } + + [Fact] + public void NoReplaceMovePropagatesUnrelatedErrors() + { + using TempDirectory temp = new(); + string missing = Path.Combine(temp.Path, "missing.tmp"); + string destination = Path.Combine(temp.Path, "managed.json"); + + Assert.Throws( + () => PackageBrokerPolicyActions.MoveFileNoReplace(missing, destination)); + Assert.False(File.Exists(destination)); + } + [Fact] public void HardLinkedFileIsRejected() { diff --git a/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs b/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs index 440657d46..f380a451d 100644 --- a/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs +++ b/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs @@ -72,6 +72,7 @@ public static ActionResult MigrateLegacyPackageBrokerPolicy(Session session) $".package-broker-policy.migration-{Guid.NewGuid():N}.tmp"); string marker = MigrationMarkerPath(session); bool migrationStarted = false; + MigrationRecord? migrationRecord = null; try { @@ -120,7 +121,6 @@ public static ActionResult MigrateLegacyPackageBrokerPolicy(Session session) } SetFileSecurity(temporary, Includes.PROGRAM_DATA_PACKAGE_BROKER_FILE_SDDL); - MigrationRecord record; using (PinnedPath temporaryPath = PinPathWithoutReparse( temporary, leafIsDirectory: false, @@ -128,15 +128,24 @@ public static ActionResult MigrateLegacyPackageBrokerPolicy(Session session) leafAccess: WinAPI.GENERIC_READ | WinAPI.FILE_READ_ATTRIBUTES | WinAPI.READ_CONTROL)) { VerifyPackageBrokerSecurity(SecurityFromHandle(temporaryPath.Leaf, isDirectory: false)); - record = new MigrationRecord( + migrationRecord = new MigrationRecord( sourceIdentity, sourceDigest, FileIdentity(temporaryPath.Leaf), FileContentDigest(temporaryPath.Leaf)); } - WriteMigrationMarker(marker, record); - File.Move(temporary, destination); + MigrationRecord record = migrationRecord.Value; + using PinnedPath markerPath = WriteMigrationMarker(marker, record); + if (MoveFileNoReplace(temporary, destination) == NoReplaceMoveResult.DestinationExists) + { + VerifyPackageBrokerSecurity(SecurityFromHandle(markerPath.Leaf, isDirectory: false)); + DeleteFileByHandle(markerPath.Leaf); + session.Log( + $"package broker policy appeared at {destination} during migration; " + + "the external destination and legacy source were preserved"); + return ActionResult.Success; + } using PinnedPath migratedPath = PinPathWithoutReparse( destination, @@ -168,7 +177,11 @@ public static ActionResult MigrateLegacyPackageBrokerPolicy(Session session) } finally { - TryDeleteTemporaryFile(session, temporary); + TryDeleteTemporaryFile( + session, + temporary, + migrationRecord?.DestinationIdentity, + migrationRecord?.DestinationDigest); } } @@ -710,7 +723,23 @@ private static string MigrationMarkerPath(Session session) => ProgramDataPackageBrokerDirectory, $".legacy-policy-migration-{session.Get(AgentProperties.installId)}.marker"); - private static void WriteMigrationMarker(string marker, MigrationRecord record) + internal static NoReplaceMoveResult MoveFileNoReplace(string source, string destination) + { + if (WinAPI.MoveFileEx(source, destination, 0)) + { + return NoReplaceMoveResult.Moved; + } + + int error = Marshal.GetLastWin32Error(); + if (error == WinAPI.ERROR_FILE_EXISTS || error == WinAPI.ERROR_ALREADY_EXISTS) + { + return NoReplaceMoveResult.DestinationExists; + } + + throw new Win32Exception(error, $"failed to move {source} to {destination} without replacement"); + } + + private static PinnedPath WriteMigrationMarker(string marker, MigrationRecord record) { using (FileStream markerFile = new(marker, FileMode.CreateNew, FileAccess.ReadWrite, FileShare.None)) { @@ -720,13 +749,22 @@ private static void WriteMigrationMarker(string marker, MigrationRecord record) } SetFileSecurity(marker, Includes.PROGRAM_DATA_PACKAGE_BROKER_FILE_SDDL); - using PinnedPath markerPath = PinPathWithoutReparse( + PinnedPath markerPath = PinPathWithoutReparse( marker, leafIsDirectory: false, allowMissingLeaf: false, - leafAccess: WinAPI.GENERIC_READ | WinAPI.FILE_READ_ATTRIBUTES | WinAPI.READ_CONTROL); - VerifyPackageBrokerSecurity(SecurityFromHandle(markerPath.Leaf, isDirectory: false)); - _ = ReadMigrationMarker(markerPath.Leaf); + leafAccess: WinAPI.GENERIC_READ | WinAPI.DELETE | WinAPI.FILE_READ_ATTRIBUTES | WinAPI.READ_CONTROL); + try + { + VerifyPackageBrokerSecurity(SecurityFromHandle(markerPath.Leaf, isDirectory: false)); + _ = ReadMigrationMarker(markerPath.Leaf); + return markerPath; + } + catch + { + markerPath.Dispose(); + throw; + } } private static MigrationRecord ReadMigrationMarker(SafeFileHandle marker) @@ -1129,7 +1167,11 @@ private static void LogLegacyYamlMigrationRequired(Session session, string desti } } - private static void TryDeleteTemporaryFile(Session session, string path) + private static void TryDeleteTemporaryFile( + Session session, + string path, + string expectedIdentity, + string expectedDigest) { try { @@ -1144,6 +1186,13 @@ private static void TryDeleteTemporaryFile(Session session, string path) } VerifyPackageBrokerSecurity(SecurityFromHandle(temporary.Leaf, isDirectory: false)); + if (expectedIdentity != null && + !FileIdentityAndDigestMatch(temporary.Leaf, expectedIdentity, expectedDigest)) + { + session.Log( + $"package broker policy migration temporary path {path} was replaced; preserving the current file"); + return; + } DeleteFileByHandle(temporary.Leaf); } catch (Exception error) @@ -1249,4 +1298,10 @@ internal string ToJson() => ["DestinationDigest"] = DestinationDigest, }.ToString(Formatting.None, Array.Empty()); } + + internal enum NoReplaceMoveResult + { + Moved, + DestinationExists, + } } diff --git a/package/AgentWindowsManaged/Actions/WinAPI.cs b/package/AgentWindowsManaged/Actions/WinAPI.cs index df7bf11fe..de27bbf8a 100644 --- a/package/AgentWindowsManaged/Actions/WinAPI.cs +++ b/package/AgentWindowsManaged/Actions/WinAPI.cs @@ -9,6 +9,7 @@ internal static class WinAPI { internal static uint CREATE_ALWAYS = 2; internal const int ERROR_ALREADY_EXISTS = 183; + internal const int ERROR_FILE_EXISTS = 80; internal const int ERROR_FILE_NOT_FOUND = 2; internal const int ERROR_INSUFFICIENT_BUFFER = 122; internal const int ERROR_PATH_NOT_FOUND = 3; From 8e2f5dd23adeebb2f877ae7a4cdaa723354dc1eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Tue, 8 Sep 2026 17:41:46 -0400 Subject: [PATCH 7/9] fix(agent-installer): read bound migration temporary Open migration temporary files with read access before recomputing their identity and digest during cleanup. Preserve mutated or replaced temporary paths while deleting only the exact bound file. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../PackageBrokerInstallerTests.cs | 40 +++++++++++++++++-- .../Actions/PackageBrokerPolicyActions.cs | 13 +++--- 2 files changed, 45 insertions(+), 8 deletions(-) diff --git a/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs b/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs index a6ef60333..e5a45e12a 100644 --- a/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs +++ b/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs @@ -313,9 +313,8 @@ public void NoReplaceMovePreservesCollisionAndCleansOnlyBoundTemporary() Assert.Equal("legacy", File.ReadAllText(source)); Assert.Equal("external", File.ReadAllText(destination)); - using (PackageBrokerPolicyActions.PinnedPath pinned = PinFile( - temporary, - WinAPI.GENERIC_READ | WinAPI.DELETE | WinAPI.FILE_READ_ATTRIBUTES)) + using (PackageBrokerPolicyActions.PinnedPath pinned = + PackageBrokerPolicyActions.PinTemporaryForCleanup(temporary, allowMissing: false)) { Assert.True( PackageBrokerPolicyActions.DeleteFileIfIdentityAndDigestMatch( @@ -327,6 +326,41 @@ public void NoReplaceMovePreservesCollisionAndCleansOnlyBoundTemporary() Assert.Equal("external", File.ReadAllText(destination)); } + [Fact] + public void TemporaryCleanupHandleSupportsDigestBindingAndPreservesMutation() + { + using TempDirectory temp = new(); + string path = Path.Combine(temp.Path, "migration.tmp"); + File.WriteAllText(path, "migrated"); + string identity; + string digest; + using (PackageBrokerPolicyActions.PinnedPath original = PinFile(path, WinAPI.GENERIC_READ)) + { + identity = PackageBrokerPolicyActions.FileIdentity(original.Leaf); + digest = PackageBrokerPolicyActions.FileContentDigest(original.Leaf); + } + + using (PackageBrokerPolicyActions.PinnedPath cleanup = + PackageBrokerPolicyActions.PinTemporaryForCleanup(path, allowMissing: false)) + { + Assert.True( + PackageBrokerPolicyActions.FileIdentityAndDigestMatch( + cleanup.Leaf, + identity, + digest)); + } + + File.WriteAllText(path, "mutated"); + using PackageBrokerPolicyActions.PinnedPath mutated = + PackageBrokerPolicyActions.PinTemporaryForCleanup(path, allowMissing: false); + Assert.False( + PackageBrokerPolicyActions.FileIdentityAndDigestMatch( + mutated.Leaf, + identity, + digest)); + Assert.True(File.Exists(path)); + } + [Fact] public void NoReplaceMovePublishesWhenDestinationIsMissing() { diff --git a/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs b/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs index f380a451d..36b8f4ff8 100644 --- a/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs +++ b/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs @@ -1175,11 +1175,7 @@ private static void TryDeleteTemporaryFile( { try { - using PinnedPath temporary = PinPathWithoutReparse( - path, - leafIsDirectory: false, - allowMissingLeaf: true, - leafAccess: WinAPI.DELETE | WinAPI.FILE_READ_ATTRIBUTES | WinAPI.READ_CONTROL); + using PinnedPath temporary = PinTemporaryForCleanup(path, allowMissing: true); if (temporary.Leaf == null) { return; @@ -1201,6 +1197,13 @@ private static void TryDeleteTemporaryFile( } } + internal static PinnedPath PinTemporaryForCleanup(string path, bool allowMissing) => + PinPathWithoutReparse( + path, + leafIsDirectory: false, + allowMissingLeaf: allowMissing, + leafAccess: WinAPI.GENERIC_READ | WinAPI.DELETE | WinAPI.FILE_READ_ATTRIBUTES | WinAPI.READ_CONTROL); + internal static bool TryDeleteLegacyPolicySource( Action log, string sourcePath, From 0e87ab80505749a7ea38ffb4cd03be58b2991022 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Tue, 8 Sep 2026 18:04:35 -0400 Subject: [PATCH 8/9] fix(agent-installer): probe legacy YAML safely Allocate native security buffers with checked lengths and replace elevated YAML existence checks with local, no-reparse, attributes-only probes. Keep YAML content untouched while reporting unsafe paths safely. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../PackageBrokerInstallerTests.cs | 83 +++++++++++++++++-- .../Actions/PackageBrokerPolicyActions.cs | 46 ++++++++-- 2 files changed, 115 insertions(+), 14 deletions(-) diff --git a/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs b/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs index e5a45e12a..9fef58b30 100644 --- a/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs +++ b/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs @@ -388,6 +388,66 @@ public void NoReplaceMovePropagatesUnrelatedErrors() Assert.False(File.Exists(destination)); } + [Fact] + public void SecurityDescriptorAllocationUsesCheckedLength() + { + Assert.Equal(256, PackageBrokerPolicyActions.AllocateSecurityDescriptorBuffer(256).Length); + Assert.Throws( + () => PackageBrokerPolicyActions.AllocateSecurityDescriptorBuffer(uint.MaxValue)); + } + + [Fact] + public void LegacyYamlProbeDistinguishesFileMissingAndDirectory() + { + using TempDirectory temp = new(); + string file = Path.Combine(temp.Path, "policy.yaml"); + string missing = Path.Combine(temp.Path, "missing.yaml"); + string directory = Directory.CreateDirectory(Path.Combine(temp.Path, "directory.yaml")).FullName; + File.WriteAllText(file, "not read"); + + Assert.True(PackageBrokerPolicyActions.TryProbePinnedOrdinaryFile(file, out string fileDiagnostic)); + Assert.Null(fileDiagnostic); + Assert.False(PackageBrokerPolicyActions.TryProbePinnedOrdinaryFile(missing, out string missingDiagnostic)); + Assert.Null(missingDiagnostic); + Assert.False(PackageBrokerPolicyActions.TryProbePinnedOrdinaryFile(directory, out string directoryDiagnostic)); + Assert.Contains("could not safely inspect", directoryDiagnostic); + } + + [Theory] + [InlineData(false)] + [InlineData(true)] + public void LegacyYamlProbeRejectsJunctionWithoutFollowingTarget(bool dangling) + { + using TempDirectory temp = new(); + string target = Directory.CreateDirectory(Path.Combine(temp.Path, "target")).FullName; + string sentinel = Path.Combine(target, "sentinel"); + File.WriteAllText(sentinel, "untouched"); + string link = Path.Combine(temp.Path, "policy.yaml"); + CreateDirectoryJunction(link, target); + if (dangling) + { + File.Delete(sentinel); + Directory.Delete(target); + } + + Assert.False(PackageBrokerPolicyActions.TryProbePinnedOrdinaryFile(link, out string diagnostic)); + Assert.Contains("could not safely inspect", diagnostic); + if (!dangling) + { + Assert.Equal("untouched", File.ReadAllText(sentinel)); + } + Directory.Delete(link); + } + + [Fact] + public void LegacyYamlProbeRejectsRemotePathBeforeAccess() + { + string remote = $@"\\127.0.0.1\missing-{Guid.NewGuid():N}\policy.yaml"; + + Assert.False(PackageBrokerPolicyActions.TryProbePinnedOrdinaryFile(remote, out string diagnostic)); + Assert.Contains("refusing to inspect remote", diagnostic); + } + [Fact] public void HardLinkedFileIsRejected() { @@ -409,15 +469,7 @@ public void DirectoryReparsePointIsRejectedWithoutTouchingTarget() using TempDirectory temp = new(); string target = Directory.CreateDirectory(Path.Combine(temp.Path, "target")).FullName; string link = Path.Combine(temp.Path, "link"); - using Process process = Process.Start(new ProcessStartInfo - { - FileName = "cmd.exe", - Arguments = $"/d /c mklink /J \"{link}\" \"{target}\"", - CreateNoWindow = true, - UseShellExecute = false, - }); - process.WaitForExit(); - Assert.Equal(0, process.ExitCode); + CreateDirectoryJunction(link, target); Assert.Throws(() => { @@ -432,6 +484,19 @@ public void DirectoryReparsePointIsRejectedWithoutTouchingTarget() Directory.Delete(link); } + private static void CreateDirectoryJunction(string link, string target) + { + using Process process = Process.Start(new ProcessStartInfo + { + FileName = "cmd.exe", + Arguments = $"/d /c mklink /J \"{link}\" \"{target}\"", + CreateNoWindow = true, + UseShellExecute = false, + }); + process.WaitForExit(); + Assert.Equal(0, process.ExitCode); + } + [Fact] public void SecureDirectoryCreationAppliesDescriptorAtCreation() { diff --git a/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs b/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs index 36b8f4ff8..9989cf265 100644 --- a/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs +++ b/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs @@ -22,6 +22,10 @@ namespace DevolutionsAgent.Actions; public static class PackageBrokerPolicyActions { + // Generic access bits may survive ACL conversion without expansion to specific rights. + private const FileSystemRights GenericWrite = (FileSystemRights)0x40000000; + private const FileSystemRights GenericAll = (FileSystemRights)0x10000000; + private static string ProgramDataDirectory => Path.Combine( Environment.GetFolderPath(Environment.SpecialFolder.CommonApplicationData), "Devolutions", @@ -851,8 +855,8 @@ private static void VerifyLegacyPolicySourceSecurity(FileSystemSecurity security FileSystemRights.DeleteSubdirectoriesAndFiles | FileSystemRights.ChangePermissions | FileSystemRights.TakeOwnership | - (FileSystemRights)0x40000000 | - (FileSystemRights)0x10000000; + GenericWrite | + GenericAll; VerifyTrustedOwnerAndNoUnsafeGrants( security, unsafeRights, @@ -867,7 +871,7 @@ internal static void VerifyTrustedDirectorySecurity(FileSystemSecurity security) FileSystemRights.DeleteSubdirectoriesAndFiles | FileSystemRights.ChangePermissions | FileSystemRights.TakeOwnership | - (FileSystemRights)0x10000000; + GenericAll; VerifyTrustedOwnerAndNoUnsafeGrants( security, tamperRights, @@ -1127,7 +1131,7 @@ private static FileSystemSecurity SecurityFromHandle(SafeFileHandle handle, bool throw new Win32Exception(error, "failed to query pinned path security descriptor size"); } - byte[] descriptor = new byte[requiredSize]; + byte[] descriptor = AllocateSecurityDescriptorBuffer(requiredSize); if (!WinAPI.GetKernelObjectSecurity( handle, information, @@ -1158,12 +1162,44 @@ private static void LogLegacyYamlMigrationRequired(Session session, string desti foreach (string extension in new[] { "yaml", "yml" }) { string legacyYaml = Path.Combine(ProgramDataDirectory, $"package-broker-policy.{extension}"); - if (File.Exists(legacyYaml)) + if (TryProbePinnedOrdinaryFile(legacyYaml, out string diagnostic)) { session.Log( $"legacy YAML package broker policy remains untouched at {legacyYaml}; " + $"validate and migrate it manually to strict JSON at {destination}"); } + else if (diagnostic != null) + { + session.Log(diagnostic); + } + } + } + + internal static byte[] AllocateSecurityDescriptorBuffer(uint requiredSize) => + new byte[checked((int)requiredSize)]; + + internal static bool TryProbePinnedOrdinaryFile(string path, out string diagnostic) + { + diagnostic = null; + if (path.StartsWith(@"\\", StringComparison.Ordinal)) + { + diagnostic = $"refusing to inspect remote legacy policy path {path}"; + return false; + } + + try + { + using PinnedPath pinned = PinPathWithoutReparse( + path, + leafIsDirectory: false, + allowMissingLeaf: true, + leafAccess: WinAPI.FILE_READ_ATTRIBUTES); + return pinned.Leaf != null; + } + catch (Exception error) + { + diagnostic = $"could not safely inspect legacy policy path {path}: {error.Message}"; + return false; } } From 3578b3065651c150a9e3dd549b1288634337c84a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Tue, 8 Sep 2026 19:24:15 -0400 Subject: [PATCH 9/9] fix(agent-installer): restrict configured policy paths Accept only fully qualified local DOS or volume-GUID JSON paths before probing configured policies. Reject remote, device, relative, traversal, and alternate-stream shapes without target access. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../PackageBrokerInstallerTests.cs | 53 +++++++++++++++ .../Actions/PackageBrokerPolicyActions.cs | 67 ++++++++++++++++++- package/AgentWindowsManaged/Actions/WinAPI.cs | 6 ++ 3 files changed, 124 insertions(+), 2 deletions(-) diff --git a/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs b/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs index 9fef58b30..3e7c19b01 100644 --- a/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs +++ b/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs @@ -133,6 +133,41 @@ public void StrictConfigReturnsAbsoluteJsonPolicyPath() Assert.Equal(path, configuredPath); } + [Theory] + [InlineData(@"\\server\share\policy.json")] + [InlineData(@"\policy.json")] + [InlineData(@"C:policy.json")] + [InlineData(@"\\?\UNC\server\share\policy.json")] + [InlineData(@"\??\UNC\server\share\policy.json")] + [InlineData(@"\\?\GLOBALROOT\Device\HarddiskVolumeShadowCopy1\policy.json")] + [InlineData(@"\\.\C:\policy.json")] + [InlineData(@"\Device\HarddiskVolume1\policy.json")] + [InlineData(@"C:\policy.json:stream")] + [InlineData(@"C:\folder\..\policy.json")] + [InlineData(@"C:\folder\\policy.json")] + [InlineData(@"C:\policy.yaml")] + public void ConfiguredPolicyPathRejectsUnsafeOrRemoteShapesBeforeProbe(string path) + { + Assert.False( + PackageBrokerPolicyActions.TryValidateConfiguredLocalPolicyPath( + path, + out string diagnostic)); + Assert.False(string.IsNullOrWhiteSpace(diagnostic)); + } + + [Fact] + public void ConfiguredPolicyPathAcceptsLocalVolumeGuid() + { + string volumeRoot = GetSystemVolumeGuidRoot(); + string path = $"{volumeRoot}Devolutions\\PackageBroker\\policy.json"; + + Assert.True( + PackageBrokerPolicyActions.TryValidateConfiguredLocalPolicyPath( + path, + out string diagnostic), + diagnostic); + } + [Theory] [InlineData("""{"PackageBroker":{"PolicyPath":"C:\\policy.json",},}""")] [InlineData("""{"PackageBroker":{/*comment*/"PolicyPath":"C:\\policy.json"}}""")] @@ -497,6 +532,24 @@ private static void CreateDirectoryJunction(string link, string target) Assert.Equal(0, process.ExitCode); } + private static string GetSystemVolumeGuidRoot() + { + using Process process = Process.Start(new ProcessStartInfo + { + FileName = "mountvol.exe", + Arguments = @"C:\ /L", + CreateNoWindow = true, + RedirectStandardOutput = true, + UseShellExecute = false, + }); + string output = process.StandardOutput.ReadToEnd().Trim(); + process.WaitForExit(); + Assert.Equal(0, process.ExitCode); + Assert.StartsWith(@"\\?\Volume{", output, StringComparison.OrdinalIgnoreCase); + Assert.EndsWith(@"\", output); + return output; + } + [Fact] public void SecureDirectoryCreationAppliesDescriptorAtCreation() { diff --git a/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs b/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs index 9989cf265..8c1cb948c 100644 --- a/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs +++ b/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs @@ -521,9 +521,8 @@ internal static bool TryReadConfiguredPolicyPath( } configuredPath = token.Value(); - if (!Path.IsPathRooted(configuredPath)) + if (!TryValidateConfiguredLocalPolicyPath(configuredPath, out diagnostic)) { - diagnostic = "PackageBroker.PolicyPath is not absolute"; return false; } return true; @@ -537,6 +536,70 @@ error is JsonException || } } + internal static bool TryValidateConfiguredLocalPolicyPath(string path, out string diagnostic) + { + diagnostic = null; + string root; + string relative; + const string volumePrefix = @"\\?\Volume{"; + if (path.StartsWith(volumePrefix, StringComparison.OrdinalIgnoreCase)) + { + int volumeEnd = path.IndexOf(@"}\", volumePrefix.Length, StringComparison.Ordinal); + if (volumeEnd < 0 || + !Guid.TryParseExact( + path.Substring(volumePrefix.Length, volumeEnd - volumePrefix.Length), + "D", + out _)) + { + diagnostic = "PackageBroker.PolicyPath has an invalid local volume GUID"; + return false; + } + root = path.Substring(0, volumeEnd + 2); + relative = path.Substring(volumeEnd + 2); + } + else + { + if (path.Length < 3 || + !char.IsLetter(path[0]) || + path[1] != ':' || + path[2] != '\\') + { + diagnostic = "PackageBroker.PolicyPath is not a fully qualified local path"; + return false; + } + root = path.Substring(0, 3); + relative = path.Substring(3); + } + + if (string.IsNullOrEmpty(relative) || + relative.EndsWith(@"\", StringComparison.Ordinal) || + relative.Contains('/') || + relative.Contains(':') || + relative.Split('\\').Any(component => + string.IsNullOrEmpty(component) || + component == "." || + component == "..")) + { + diagnostic = "PackageBroker.PolicyPath has an unsafe local path shape"; + return false; + } + if (!string.Equals(Path.GetExtension(relative), ".json", StringComparison.OrdinalIgnoreCase)) + { + diagnostic = "PackageBroker.PolicyPath must name a JSON file"; + return false; + } + + uint driveType = WinAPI.GetDriveType(root); + if (driveType == WinAPI.DRIVE_UNKNOWN || + driveType == WinAPI.DRIVE_NO_ROOT_DIR || + driveType == WinAPI.DRIVE_REMOTE) + { + diagnostic = "PackageBroker.PolicyPath does not use an available local volume"; + return false; + } + return true; + } + internal static bool ContainsNonStrictJsonSyntax(string json) { bool inString = false; diff --git a/package/AgentWindowsManaged/Actions/WinAPI.cs b/package/AgentWindowsManaged/Actions/WinAPI.cs index de27bbf8a..8268e0526 100644 --- a/package/AgentWindowsManaged/Actions/WinAPI.cs +++ b/package/AgentWindowsManaged/Actions/WinAPI.cs @@ -37,6 +37,9 @@ internal static class WinAPI internal const uint GENERIC_READ = 0x80000000; internal static uint GENERIC_WRITE = 0x40000000; + internal const uint DRIVE_NO_ROOT_DIR = 1; + internal const uint DRIVE_REMOTE = 4; + internal const uint DRIVE_UNKNOWN = 0; internal const uint OPEN_EXISTING = 3; internal const uint READ_CONTROL = 0x00020000; @@ -306,6 +309,9 @@ internal static extern bool GetFileInformationByHandle( SafeFileHandle hFile, out ByHandleFileInformation lpFileInformation); + [DllImport("kernel32", EntryPoint = "GetDriveTypeW", CharSet = CharSet.Unicode)] + internal static extern uint GetDriveType([MarshalAs(UnmanagedType.LPWStr)] string rootPathName); + [DllImport("kernel32", SetLastError = true)] [return: MarshalAs(UnmanagedType.Bool)] internal static extern bool SetFileInformationByHandle(