From 389629347f588fddc274265b09a34f038241eb71 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 01/10] 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 0e7de11085bce4437934e9bbf0e61d2b0e9ea96c 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 02/10] 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 3b47c853115303852db44297a817e597e942bf84 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 03/10] 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 5d60d81ecb54b532082921726eb1f2d64c44415e 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 04/10] 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 96410fc39b0cd414cbdb191afb868147f9759867 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 05/10] 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 745546c41183c3cadb4cba668b22d153a164d234 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 06/10] 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 fa7c9ab6c5b4cd3a9fee533410be92ea11bd2f8f 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 07/10] 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 d6918a29ef451cc67da88b77906790382907ed62 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 08/10] 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 71e1ab1af14c243a65eeedc0837df973a269a6f5 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 09/10] 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( From 1a71fcc4b9deaa2822519e9a39634ebc8f976399 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Wed, 16 Sep 2026 01:25:18 +0900 Subject: [PATCH 10/10] fix(agent-installer): convert legacy policies Convert trusted retained JSON with the official policy contract validator. Preserve compatible versions and publisher content, and retain protected originals and identity-bound evidence for rollback and downgrade recovery. Guard commit cleanup with verified publication and preserve policies when source, destination, or managed-authority identity changes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- Cargo.lock | 1 + crates/now-package-broker/Cargo.toml | 3 +- .../src/installer_policy_migration.rs | 64 +++ crates/now-package-broker/src/lib.rs | 2 + .../src/policy_store/mod.rs | 2 +- .../src/policy_store/validation.rs | 2 +- devolutions-agent/src/main.rs | 22 + docs/agent-policy-migration.md | 44 ++ .../PackageBrokerInstallerTests.cs | 300 +++++++++++++- .../Actions/AgentActions.cs | 4 +- .../Actions/PackageBrokerPolicyActions.cs | 382 +++++++++++++++--- testsuite/Cargo.toml | 5 + testsuite/tests/installer_policy_migration.rs | 111 +++++ 13 files changed, 886 insertions(+), 56 deletions(-) create mode 100644 crates/now-package-broker/src/installer_policy_migration.rs create mode 100644 docs/agent-policy-migration.md create mode 100644 testsuite/tests/installer_policy_migration.rs diff --git a/Cargo.lock b/Cargo.lock index 69d79e1b8..f5f7d2f41 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7631,6 +7631,7 @@ dependencies = [ "network-scanner", "network-scanner-proto", "nonempty", + "now-package-broker", "picky", "proxy-socks", "quinn", diff --git a/crates/now-package-broker/Cargo.toml b/crates/now-package-broker/Cargo.toml index daf063a88..71252d065 100644 --- a/crates/now-package-broker/Cargo.toml +++ b/crates/now-package-broker/Cargo.toml @@ -37,7 +37,8 @@ now-policy-server-template = "0.5" parking_lot = "0.12" regex = "1" semver = "1" -serde_json = "1" +serde = { version = "1", features = ["derive"] } +serde_json = { version = "1", features = ["raw_value"] } sha2 = "0.10" tokio = { version = "1.52", features = ["net", "io-util", "rt", "macros", "parking_lot", "fs", "sync", "time"] } tokio-util = "0.7" diff --git a/crates/now-package-broker/src/installer_policy_migration.rs b/crates/now-package-broker/src/installer_policy_migration.rs new file mode 100644 index 000000000..cda1b06fd --- /dev/null +++ b/crates/now-package-broker/src/installer_policy_migration.rs @@ -0,0 +1,64 @@ +//! Installer-only document conversion; callers retain responsibility for source trust and publication. + +use anyhow::{Context as _, bail}; +use serde::Deserialize; +use serde_json::value::RawValue; + +const LEGACY_SCHEMA: &str = "https://devolutions.net/schemas/now-policy.schema.1.0.json"; +pub const MAX_DOCUMENT_BYTES: u64 = 1024 * 1024; + +#[derive(Deserialize)] +#[serde(rename_all = "PascalCase", deny_unknown_fields)] +struct LegacyDocument<'a> { + #[serde(rename = "$schema")] + schema: String, + policy_version: now_policy::PolicyFormatVersion, + #[serde(borrow)] + policy_type: &'a RawValue, + #[serde(borrow)] + metadata: &'a RawValue, + #[serde(borrow)] + enforcement: &'a RawValue, + #[serde(borrow)] + rules: &'a RawValue, +} + +/// Convert a committed legacy document without rewriting its publisher-authored values. +/// +/// # Errors +/// +/// Rejects oversized, ambiguous, unsupported or invalid documents. +pub fn convert_document(input: &str) -> anyhow::Result { + if input.len() as u64 > MAX_DOCUMENT_BYTES { + bail!("policy exceeds the installer migration size limit"); + } + // Parse typed text, not a Value: duplicate fields must not be collapsed. + let output = if now_policy::schema::parse_policy_json(input).is_ok() { + input.to_owned() + } else { + let legacy: LegacyDocument<'_> = + serde_json::from_str(input).context("invalid or unsupported legacy policy document")?; + if legacy.schema != LEGACY_SCHEMA { + bail!("unsupported legacy policy schema"); + } + // Keep raw values: reserializing typed metadata would normalize timestamps, + // optional fields and sets, changing the publisher's original content. + format!( + "{{\"PolicyFormatVersion\":{},\"PolicyType\":{},\"Metadata\":{},\"Enforcement\":{},\"Rules\":{}}}", + serde_json::to_string(&legacy.policy_version)?, + legacy.policy_type, + legacy.metadata, + legacy.enforcement, + legacy.rules, + ) + }; + let policy = now_policy::schema::parse_policy_json(&output).map_err(anyhow::Error::msg)?; + let validation = crate::policy_store::validation::validate_committed_policy(&policy); + if !validation.is_valid { + bail!( + "converted policy failed authoritative validation: {:?}", + validation.findings + ); + } + Ok(output) +} diff --git a/crates/now-package-broker/src/lib.rs b/crates/now-package-broker/src/lib.rs index e1542dda4..d9617bd8f 100644 --- a/crates/now-package-broker/src/lib.rs +++ b/crates/now-package-broker/src/lib.rs @@ -16,6 +16,8 @@ pub mod event_channel; #[cfg(windows)] pub mod executor; #[cfg(windows)] +pub mod installer_policy_migration; +#[cfg(windows)] pub mod operation_tracker; #[cfg(windows)] pub mod pipe; diff --git a/crates/now-package-broker/src/policy_store/mod.rs b/crates/now-package-broker/src/policy_store/mod.rs index b79c1db2a..7ed272fe5 100644 --- a/crates/now-package-broker/src/policy_store/mod.rs +++ b/crates/now-package-broker/src/policy_store/mod.rs @@ -15,7 +15,7 @@ use now_policy_api::{ }; mod receipt; -mod validation; +pub(crate) mod validation; mod windows; #[derive(Clone, Copy, Debug)] diff --git a/crates/now-package-broker/src/policy_store/validation.rs b/crates/now-package-broker/src/policy_store/validation.rs index 230bac6ee..af32ee3d9 100644 --- a/crates/now-package-broker/src/policy_store/validation.rs +++ b/crates/now-package-broker/src/policy_store/validation.rs @@ -111,7 +111,7 @@ pub(super) fn validate_draft(raw: &serde_json::Value) -> PolicyValidationResult } } } -pub(super) fn validate_committed_policy(policy: &now_policy::PolicyDocument) -> PolicyValidationResult { +pub(crate) fn validate_committed_policy(policy: &now_policy::PolicyDocument) -> PolicyValidationResult { let raw = serde_json::to_value(policy.to_draft()).expect("committed policy draft serializes"); validate_draft(&raw) } diff --git a/devolutions-agent/src/main.rs b/devolutions-agent/src/main.rs index f8c9a048f..07b858937 100644 --- a/devolutions-agent/src/main.rs +++ b/devolutions-agent/src/main.rs @@ -207,6 +207,28 @@ fn parse_up_command_args_with_reader(args: &[String], mut stdin_read } fn main() { + #[cfg(windows)] + if env::args().nth(1).as_deref() == Some("installer-policy-convert") { + use std::io::{Read as _, Write as _}; + + let result = (|| -> Result<()> { + if env::args().count() != 2 { + bail!("installer-policy-convert accepts only policy JSON on stdin"); + } + let mut input = String::new(); + io::stdin() + .take(now_package_broker::installer_policy_migration::MAX_DOCUMENT_BYTES + 1) + .read_to_string(&mut input)?; + let output = now_package_broker::installer_policy_migration::convert_document(&input)?; + io::stdout().lock().write_all(output.as_bytes())?; + Ok(()) + })(); + if let Err(error) = result { + eprintln!("{error:#}"); + std::process::exit(1); + } + return; + } let mut controller = Controller::new(SERVICE_NAME, DISPLAY_NAME, DESCRIPTION); if let Some(cmd) = env::args().nth(1) { diff --git a/docs/agent-policy-migration.md b/docs/agent-policy-migration.md new file mode 100644 index 000000000..3d6a5f2ad --- /dev/null +++ b/docs/agent-policy-migration.md @@ -0,0 +1,44 @@ +# Agent policy format migration + +Upgrade with the Windows MSI to migrate an eligible legacy JSON policy from `%ProgramData%\Devolutions\Agent\package-broker-policy.json` to managed PackageBroker storage. +The installer retains the source through a no-follow handle, checks its owner and write permissions, and converts it before publication. +Unsafe paths, untrusted sources and YAML/YML files remain untouched and require administrator remediation. +An existing managed policy is never overwritten, including one that appears during migration. + +Conversion accepts only the old committed-document shape: `$schema` must equal `https://devolutions.net/schemas/now-policy.schema.1.0.json`, and `PolicyVersion` must be a canonical, supported `1.minor.patch` version. +It removes `$schema` and renames `PolicyVersion` to `PolicyFormatVersion`, preserving the version rather than relabeling a compatible document as `1.0.0`. +All other JSON values retain their raw representation, including policy identity, publisher, revision, timestamps, validity, rules and order. +Mixed identities, duplicate or unknown fields, malformed JSON and unsupported versions fail conversion. +The official new-contract parser and the broker's committed-policy validator must both accept the result; failure aborts migration and preserves the source. +The ordinary broker reader does not accept either legacy identity field. + +## Recovery and downgrade + +For a converted policy, the installer keeps the legacy source and a protected `.legacy-policy-migration-.marker.original` backup in `%ProgramData%\Devolutions\PackageBroker`. +Its protected marker records the source identity, SHA-256 digest and security descriptor, backup identity, converted-file identity and digest, and migration-owned managed-authority identity. +Neither commit nor rollback deletes the original backup. +The backup is recovery evidence, not an active policy. + +Rollback removes only the unchanged migration-owned destination and authority marker, after verifying the retained legacy source and backup. +This restores legacy-path selection for an older Agent without leaving a new-format policy that it cannot parse. +Changed files, replacement authority markers and unverifiable evidence are preserved for manual recovery instead of being overwritten or deleted. +A repeated invocation leaves an existing destination alone; an interrupted invocation with the same install ID can undo its owned authority marker and retry. +An incomplete backup or marker, an unrelated managed-authority marker, or a different transaction's evidence requires manual recovery. +Do not remove the last verified original or a runtime authority marker merely to make installation succeed. + +Before a later downgrade, stop the Agent and archive the active managed policy and recovery evidence. +The preserved legacy policy represents the state at migration, not subsequent policy edits. +Have an administrator verify that policy before restoring an old Agent, and resolve managed-path selection explicitly; retaining the backup does not automatically reverse later policy changes. + +## Portable, manual and import installations + +These entry points do not perform MSI migration. +A legacy document remains **Invalid**, not Missing and not a default policy; requests containing legacy identity fields are rejected. +An explicitly configured legacy `PackageBroker.PolicyPath` also remains Invalid until the administrator changes the configured document or selects the validated managed policy. + +Preserve the original bytes and permissions before remediation. +On a trusted local copy, verify the canonical old schema and compatible version, remove `$schema`, and rename `PolicyVersion` without changing metadata or rules. +Use the official contract and broker validation to check the result before replacing an active policy through its supported administrative workflow. +Do not use a parser that discards duplicate keys, silently drops unknown fields or substitutes defaults. +The internal `installer-policy-convert` Agent command accepts JSON on stdin and emits validated JSON on stdout, but does not establish source trust or publish files; it is not a general import or automatic recovery API. +YAML/YML needs a separate administrator-reviewed conversion to strict JSON. diff --git a/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs b/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs index 3e7c19b01..cbf0839af 100644 --- a/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs +++ b/package/AgentWindowsManaged.Tests/PackageBrokerInstallerTests.cs @@ -20,6 +20,293 @@ namespace DevolutionsAgent.Installer.Tests; public sealed class PackageBrokerInstallerTests { + private const string CurrentPolicy = + """{"PolicyFormatVersion":"1.7.3","PolicyType":"PackageBrokerPolicy","Metadata":{"Id":"policy-a","Publisher":"Test","Revision":17,"PublishedAt":"2026-01-01T00:00:00Z"},"Enforcement":{"DefaultDecision":"Deny","RulePrecedence":"PriorityThenDeny"},"Rules":[]}"""; + private static string LegacyPolicy => CurrentPolicy.Replace( + "\"PolicyFormatVersion\":", + "\"$schema\":\"https://devolutions.net/schemas/now-policy.schema.1.0.json\",\"PolicyVersion\":"); + + [SystemFact] + public void TransactionTestsRunAsLocalSystem() + { + Assert.Equal("S-1-5-18", WindowsIdentity.GetCurrent().User.Value); + } + + [SystemFact] + public void InstalledAgentConverterUsesAuthoritativeContractBeforePublication() + { + using TempDirectory temp = new(); + Directory.SetAccessControl(temp.Path, + DirectorySecurity(DevolutionsAgent.Resources.Includes.PROGRAM_DATA_PACKAGE_BROKER_SDDL)); + string builtAgent = Environment.GetEnvironmentVariable("DEVOLUTIONS_AGENT_MIGRATION_TEST_EXE") ?? + Path.GetFullPath(Path.Combine(AppDomain.CurrentDomain.BaseDirectory, + @"..\..\..\..\..\target\debug\devolutions-agent.exe")); + string staged = Path.Combine(temp.Path, DevolutionsAgent.Resources.Includes.EXECUTABLE_NAME); + File.Copy(builtAgent, staged); + File.SetAccessControl(staged, Security(DevolutionsAgent.Resources.Includes.PROGRAM_DATA_PACKAGE_BROKER_FILE_SDDL)); + Assert.Equal(CurrentPolicy, Encoding.UTF8.GetString( + PackageBrokerPolicyActions.ConvertWithInstalledAgent(temp.Path, Encoding.UTF8.GetBytes(LegacyPolicy)))); + Assert.Equal(CurrentPolicy, Encoding.UTF8.GetString( + PackageBrokerPolicyActions.ConvertWithInstalledAgent(temp.Path, Encoding.UTF8.GetBytes(CurrentPolicy)))); + Assert.Throws(() => + PackageBrokerPolicyActions.ConvertWithInstalledAgent(temp.Path, Encoding.UTF8.GetBytes("{}"))); + } + + [Theory] + [InlineData(".yaml")] + [InlineData(".yml")] + public void MigrationNeverReadsOrConvertsYaml(string extension) + { + using TempDirectory temp = new(); + string source = Path.Combine(temp.Path, "policy" + extension); + string destination = Path.Combine(temp.Path, "managed.json"); + File.WriteAllText(source, "PolicyVersion: 1.0.0"); + Assert.Equal(ActionResult.Success, PackageBrokerPolicyActions.MigrateLegacyPolicy( + _ => { }, source, destination, destination + ".marker", + _ => throw new Exception("converter must not run"))); + Assert.Equal("PolicyVersion: 1.0.0", File.ReadAllText(source)); + Assert.False(File.Exists(destination)); + } + + [Fact] + public void MigrationPreservesUntrustedSourceWithoutCallingConverter() + { + using TempDirectory temp = new(); + string source = Path.Combine(temp.Path, "policy.json"); + string destination = Path.Combine(temp.Path, "managed.json"); + File.WriteAllText(source, LegacyPolicy); + FileSecurity security = new FileInfo(source).GetAccessControl(); + security.AddAccessRule(new FileSystemAccessRule( + new SecurityIdentifier(WellKnownSidType.WorldSid, null), + FileSystemRights.WriteData, AccessControlType.Allow)); + File.SetAccessControl(source, security); + Assert.Equal(ActionResult.Success, PackageBrokerPolicyActions.MigrateLegacyPolicy( + _ => { }, source, destination, destination + ".marker", + _ => throw new Exception("converter must not run"))); + Assert.Equal(LegacyPolicy, File.ReadAllText(source)); + Assert.False(File.Exists(destination)); + } + + [Fact] + public void MigrationPreservesReparseSourceWithoutCallingConverter() + { + using TempDirectory temp = new(); + string target = Directory.CreateDirectory(Path.Combine(temp.Path, "target")).FullName; + string sentinel = Path.Combine(target, "untouched"); + File.WriteAllText(sentinel, LegacyPolicy); + string source = Path.Combine(temp.Path, "policy.json"); + string destination = Path.Combine(temp.Path, "managed.json"); + CreateDirectoryJunction(source, target); + Assert.Equal(ActionResult.Success, PackageBrokerPolicyActions.MigrateLegacyPolicy( + _ => { }, source, destination, destination + ".marker", + _ => throw new Exception("converter must not run"))); + Assert.Equal(LegacyPolicy, File.ReadAllText(sentinel)); + Assert.False(File.Exists(destination)); + Directory.Delete(source); + } + + [SystemFact] + public void ConvertedMigrationCommitAndRepeatPreserveOriginalAndEvidence() + { + using TempDirectory temp = new(); + string source = Path.Combine(temp.Path, "policy.json"); + string destination = Path.Combine(temp.Path, "managed.json"); + string marker = destination + ".marker"; + WriteTrusted(source, LegacyPolicy); + Assert.Equal(ActionResult.Success, MigrateFixture(source, destination, marker)); + Assert.Equal(CurrentPolicy, File.ReadAllText(destination)); + Assert.Equal(LegacyPolicy, File.ReadAllText(marker + ".original")); + PackageBrokerPolicyActions.MigrationRecord record = + PackageBrokerPolicyActions.ReadMigrationMarkerJson(File.ReadAllText(marker)); + using (PackageBrokerPolicyActions.PinnedPath pinned = PinFile(source, WinAPI.GENERIC_READ)) + { + Assert.True(PackageBrokerPolicyActions.FileIdentityAndDigestMatch( + pinned.Leaf, record.SourceIdentity, record.SourceDigest)); + } + Assert.NotNull(record.SourceSecurity); + Assert.NotNull(record.BackupIdentity); + Assert.NotNull(record.AuthorityIdentity); + PackageBrokerPolicyActions.VerifyPackageBrokerSecurity(new FileInfo(marker + ".original").GetAccessControl()); + PackageBrokerPolicyActions.CommitLegacyPolicy(_ => { }, source, destination, marker); + PackageBrokerPolicyActions.CommitLegacyPolicy(_ => { }, source, destination, marker); + Assert.Equal(ActionResult.Success, PackageBrokerPolicyActions.MigrateLegacyPolicy( + _ => { }, source, destination, marker, _ => throw new Exception("existing destination"))); + Assert.Equal(LegacyPolicy, File.ReadAllText(source)); + Assert.True(File.Exists(marker)); + Assert.True(File.Exists(marker + ".original")); + } + + [SystemFact] + public void RollbackRestoresLegacyArbitrationAndMigrationCanRepeat() + { + using TempDirectory temp = new(); + string source = Path.Combine(temp.Path, "policy.json"); + string destination = Path.Combine(temp.Path, "managed.json"); + string marker = destination + ".marker"; + WriteTrusted(source, LegacyPolicy); + Assert.Equal(ActionResult.Success, MigrateFixture(source, destination, marker)); + Assert.Equal(ActionResult.Success, PackageBrokerPolicyActions.RollbackLegacyPolicy( + _ => { }, source, destination, marker)); + Assert.False(File.Exists(destination)); + Assert.False(File.Exists(Path.Combine(temp.Path, ".package-broker-managed-authority.v1"))); + Assert.Equal(LegacyPolicy, File.ReadAllText(source)); + Assert.Equal(LegacyPolicy, File.ReadAllText(marker + ".original")); + Assert.Equal(ActionResult.Success, PackageBrokerPolicyActions.RollbackLegacyPolicy( + _ => { }, source, destination, marker)); + Assert.Equal(ActionResult.Success, MigrateFixture(source, destination, marker)); + Assert.Equal(CurrentPolicy, File.ReadAllText(destination)); + } + + [SystemFact] + public void InterruptedPublicationRecoversOnlyOwnedAuthority() + { + using TempDirectory temp = new(); + string source = Path.Combine(temp.Path, "policy.json"); + string destination = Path.Combine(temp.Path, "managed.json"); + string marker = destination + ".marker"; + WriteTrusted(source, LegacyPolicy); + Assert.Equal(ActionResult.Success, MigrateFixture(source, destination, marker)); + File.Delete(destination); + Assert.Equal(ActionResult.Success, MigrateFixture(source, destination, marker)); + Assert.Equal(CurrentPolicy, File.ReadAllText(destination)); + Assert.Equal(LegacyPolicy, File.ReadAllText(source)); + } + + [SystemFact] + public void InvalidLegacyPolicyFailsUpgradeAndPreservesSource() + { + using TempDirectory temp = new(); + string source = Path.Combine(temp.Path, "policy.json"); + string destination = Path.Combine(temp.Path, "managed.json"); + WriteTrusted(source, "{}"); + Assert.Equal(ActionResult.Failure, PackageBrokerPolicyActions.MigrateLegacyPolicy( + _ => { }, source, destination, destination + ".marker", + _ => throw new InvalidOperationException("invalid legacy policy"))); + Assert.Equal("{}", File.ReadAllText(source)); + Assert.False(File.Exists(destination)); + } + + [SystemFact] + public void ExistingNewDestinationAndPublicationCollisionArePreserved() + { + using TempDirectory temp = new(); + string source = Path.Combine(temp.Path, "policy.json"); + string destination = Path.Combine(temp.Path, "managed.json"); + WriteTrusted(source, LegacyPolicy); + Assert.Equal(ActionResult.Success, PackageBrokerPolicyActions.MigrateLegacyPolicy( + _ => { }, source, destination, destination + ".marker", + _ => + { + WriteTrusted(destination, CurrentPolicy); + return Encoding.UTF8.GetBytes(CurrentPolicy); + })); + PackageBrokerPolicyActions.RollbackLegacyPolicy(_ => { }, source, destination, destination + ".marker"); + Assert.Equal(CurrentPolicy, File.ReadAllText(destination)); + Assert.Equal(ActionResult.Success, PackageBrokerPolicyActions.MigrateLegacyPolicy( + _ => { }, source, destination, destination + ".marker", + _ => throw new Exception("existing destination must not be converted"))); + Assert.Equal(LegacyPolicy, File.ReadAllText(source)); + } + + [SystemFact] + public void ChangedSourcePreventsDestructiveRollback() + { + using TempDirectory temp = new(); + string source = Path.Combine(temp.Path, "policy.json"); + string destination = Path.Combine(temp.Path, "managed.json"); + string marker = destination + ".marker"; + WriteTrusted(source, LegacyPolicy); + Assert.Equal(ActionResult.Success, MigrateFixture(source, destination, marker)); + File.WriteAllText(source, "changed"); + PackageBrokerPolicyActions.RollbackLegacyPolicy(_ => { }, source, destination, marker); + Assert.Equal(CurrentPolicy, File.ReadAllText(destination)); + Assert.Equal("changed", File.ReadAllText(source)); + Assert.Equal(LegacyPolicy, File.ReadAllText(marker + ".original")); + } + + [SystemFact] + public void PreexistingAuthorityPreventsLegacyResurrection() + { + using TempDirectory temp = new(); + string source = Path.Combine(temp.Path, "policy.json"); + string destination = Path.Combine(temp.Path, "managed.json"); + WriteTrusted(source, LegacyPolicy); + WriteTrusted(Path.Combine(temp.Path, ".package-broker-managed-authority.v1"), ""); + Assert.Equal(ActionResult.Failure, MigrateFixture(source, destination, destination + ".marker")); + Assert.False(File.Exists(destination)); + Assert.Equal(LegacyPolicy, File.ReadAllText(source)); + } + + [SystemFact] + public void AuthorityCollisionCannotCommitSourceDeletion() + { + using TempDirectory temp = new(); + string source = Path.Combine(temp.Path, "policy.json"); + string destination = Path.Combine(temp.Path, "managed.json"); + string marker = destination + ".marker"; + WriteTrusted(source, CurrentPolicy); + Assert.Equal(ActionResult.Failure, PackageBrokerPolicyActions.MigrateLegacyPolicy( + _ => { }, source, destination, marker, input => + { + WriteTrusted(Path.Combine(temp.Path, ".package-broker-managed-authority.v1"), ""); + return input; + })); + PackageBrokerPolicyActions.CommitLegacyPolicy(_ => { }, source, destination, marker); + Assert.Equal(CurrentPolicy, File.ReadAllText(source)); + Assert.False(File.Exists(destination)); + Assert.True(File.Exists(marker)); + } + + [SystemFact] + public void DestinationCollisionCannotCommitSourceDeletion() + { + using TempDirectory temp = new(); + string source = Path.Combine(temp.Path, "policy.json"); + string destination = Path.Combine(temp.Path, "managed.json"); + string marker = destination + ".marker"; + WriteTrusted(source, CurrentPolicy); + Assert.Equal(ActionResult.Success, PackageBrokerPolicyActions.MigrateLegacyPolicy( + _ => { }, source, destination, marker, input => + { + WriteTrusted(destination, CurrentPolicy); + return input; + })); + PackageBrokerPolicyActions.CommitLegacyPolicy(_ => { }, source, destination, marker); + Assert.Equal(CurrentPolicy, File.ReadAllText(source)); + Assert.Equal(CurrentPolicy, File.ReadAllText(destination)); + Assert.True(File.Exists(marker)); + } + + [SystemFact] + public void UnchangedInputRollbackPreservesLastSurvivingPolicy() + { + using TempDirectory temp = new(); + string source = Path.Combine(temp.Path, "policy.json"); + string destination = Path.Combine(temp.Path, "managed.json"); + string marker = destination + ".marker"; + WriteTrusted(source, CurrentPolicy); + Assert.Equal(ActionResult.Success, PackageBrokerPolicyActions.MigrateLegacyPolicy( + _ => { }, source, destination, marker, input => input)); + File.Delete(source); + PackageBrokerPolicyActions.RollbackLegacyPolicy(_ => { }, source, destination, marker); + Assert.Equal(CurrentPolicy, File.ReadAllText(destination)); + Assert.True(File.Exists(marker)); + Assert.True(File.Exists(Path.Combine(temp.Path, ".package-broker-managed-authority.v1"))); + } + + private static ActionResult MigrateFixture(string source, string destination, string marker) => + PackageBrokerPolicyActions.MigrateLegacyPolicy(_ => { }, source, destination, marker, input => + { + Assert.Equal(LegacyPolicy, Encoding.UTF8.GetString(input)); + return Encoding.UTF8.GetBytes(CurrentPolicy); + }); + + private static void WriteTrusted(string path, string content) + { + File.WriteAllText(path, content); + File.SetAccessControl(path, Security(DevolutionsAgent.Resources.Includes.PROGRAM_DATA_PACKAGE_BROKER_FILE_SDDL)); + } + [Fact] public void DedicatedPolicyAclAcceptsOnlySystemAndAdministrators() { @@ -32,6 +319,17 @@ public void DedicatedPolicyAclAcceptsOnlySystemAndAdministrators() DevolutionsAgent.Resources.Includes.PROGRAM_DATA_PACKAGE_BROKER_SDDL); } + public sealed class SystemFactAttribute : FactAttribute + { + public SystemFactAttribute() + { + if (!WindowsIdentity.GetCurrent().IsSystem) + { + Skip = "Requires LocalSystem, matching the MSI custom-action token and SYSTEM-owned backup ACL"; + } + } + } + [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)")] @@ -613,7 +911,7 @@ public void MigrationActionsUseDeferredRollbackCommitSequence() 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.Equal(Step.InstallFiles.ToString(), 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()); diff --git a/package/AgentWindowsManaged/Actions/AgentActions.cs b/package/AgentWindowsManaged/Actions/AgentActions.cs index 3c9f98c3d..4da16f2ec 100644 --- a/package/AgentWindowsManaged/Actions/AgentActions.cs +++ b/package/AgentWindowsManaged/Actions/AgentActions.cs @@ -167,13 +167,13 @@ internal static class AgentActions new Id($"CA.{nameof(migrateLegacyPackageBrokerPolicy)}"), PackageBrokerPolicyActions.MigrateLegacyPackageBrokerPolicy, Return.check, - When.After, new Step(ensureProgramDataPackageBrokerDirectory.Id), + When.After, Step.InstallFiles, Condition.NOT_BeingRemoved, Sequence.InstallExecuteSequence) { Execute = Execute.deferred, Impersonate = false, - UsesProperties = UseProperties(new[] { AgentProperties.installId }), + UsesProperties = $"{UseProperties(new[] { AgentProperties.installId })},{AgentProperties.InstallDir}", }; private static readonly ElevatedManagedAction rollbackLegacyPackageBrokerPolicyMigration = new( diff --git a/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs b/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs index 8c1cb948c..f1b40e35b 100644 --- a/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs +++ b/package/AgentWindowsManaged/Actions/PackageBrokerPolicyActions.cs @@ -7,6 +7,7 @@ using System; using System.Collections.Generic; using System.ComponentModel; +using System.Diagnostics; using System.IO; using System.Linq; using System.Runtime.CompilerServices; @@ -15,6 +16,7 @@ using System.Security.Cryptography; using System.Security.Principal; using System.Text; +using System.Threading.Tasks; [assembly: InternalsVisibleTo("DevolutionsAgent.Installer.Tests")] @@ -42,12 +44,91 @@ public static class PackageBrokerPolicyActions private static string LegacyPolicyPath => Path.Combine(ProgramDataDirectory, "package-broker-policy.json"); + private static string AuthorityMarkerPath(string destination) => + Path.Combine(Path.GetDirectoryName(destination), ".package-broker-managed-authority.v1"); + private static uint PackageBrokerSecurityInformation => WinAPI.OWNER_SECURITY_INFORMATION | WinAPI.GROUP_SECURITY_INFORMATION | WinAPI.DACL_SECURITY_INFORMATION | WinAPI.PROTECTED_DACL_SECURITY_INFORMATION; + internal static byte[] ConvertWithInstalledAgent(string installDirectory, byte[] input) + { + string executable = Path.Combine(installDirectory, Includes.EXECUTABLE_NAME); + if (!TryValidateLocalFilePath(executable, ".exe", out string diagnostic)) + { + throw new InvalidOperationException($"unsafe installed Agent path: {diagnostic}"); + } + using PinnedPath agent = PinPathWithoutReparse( + executable, false, false, WinAPI.GENERIC_READ | WinAPI.READ_CONTROL, + verifyTrustedAncestors: true); + VerifyLegacyPolicySourceSecurity(SecurityFromHandle(agent.Leaf, false)); + using Process process = new() + { + StartInfo = new ProcessStartInfo + { + FileName = executable, + Arguments = "installer-policy-convert", + UseShellExecute = false, + CreateNoWindow = true, + RedirectStandardInput = true, + RedirectStandardOutput = true, + RedirectStandardError = true, + StandardOutputEncoding = new UTF8Encoding(false, true), + StandardErrorEncoding = new UTF8Encoding(false, true), + } + }; + process.Start(); + Task output = process.StandardOutput.ReadToEndAsync(); + Task error = process.StandardError.ReadToEndAsync(); + Task write = Task.Run(async () => + { + await process.StandardInput.BaseStream.WriteAsync(input, 0, input.Length); + process.StandardInput.Close(); + }); + if (!Task.WaitAll(new Task[] { output, error, write }, 30000) || !process.WaitForExit(30000)) + { + process.Kill(); + throw new InvalidOperationException("installed Agent policy converter timed out"); + } + if (process.ExitCode != 0 || output.Result.Length == 0) + { + throw new InvalidOperationException($"installed Agent rejected policy conversion: {error.Result}"); + } + return new UTF8Encoding(false, true).GetBytes(output.Result); + } + + internal static string PreserveLegacyBackup(string path, byte[] original, string digest) + { + using (PinnedPath existing = PinPathWithoutReparse( + path, false, true, WinAPI.GENERIC_READ | WinAPI.READ_CONTROL)) + { + if (existing.Leaf != null) + { + VerifyPackageBrokerSecurity(SecurityFromHandle(existing.Leaf, false)); + if (FileContentDigest(existing.Leaf) != digest) + { + throw new InvalidOperationException("legacy backup collision; preserving both originals"); + } + return FileIdentity(existing.Leaf); + } + } + using (FileStream backup = new(path, FileMode.CreateNew, FileAccess.Write, FileShare.None)) + { + backup.Write(original, 0, original.Length); + backup.Flush(true); + } + SetFileSecurity(path, Includes.PROGRAM_DATA_PACKAGE_BROKER_FILE_SDDL); + using PinnedPath pinned = PinPathWithoutReparse(path, false, false, WinAPI.GENERIC_READ | WinAPI.READ_CONTROL); + VerifyPackageBrokerSecurity(SecurityFromHandle(pinned.Leaf, false)); + if (FileContentDigest(pinned.Leaf) != digest) + { + throw new InvalidOperationException("legacy backup digest changed"); + } + return FileIdentity(pinned.Leaf); + } + [CustomAction] public static ActionResult EnsureProgramDataPackageBrokerDirectory(Session session) { @@ -69,18 +150,35 @@ public static ActionResult EnsureProgramDataPackageBrokerDirectory(Session sessi [CustomAction] public static ActionResult MigrateLegacyPackageBrokerPolicy(Session session) { - string destination = DestinationPolicyPath; - string sourcePath = LegacyPolicyPath; + LogLegacyYamlMigrationRequired(session, DestinationPolicyPath); + return MigrateLegacyPolicy( + session.Log, + LegacyPolicyPath, + DestinationPolicyPath, + MigrationMarkerPath(session), + input => ConvertWithInstalledAgent(session.CustomActionData[AgentProperties.InstallDir], input)); + } + + internal static ActionResult MigrateLegacyPolicy( + Action log, + string sourcePath, + string destination, + string marker, + Func convert) + { string temporary = Path.Combine( - ProgramDataPackageBrokerDirectory, + Path.GetDirectoryName(destination), $".package-broker-policy.migration-{Guid.NewGuid():N}.tmp"); - string marker = MigrationMarkerPath(session); + string authorityTemporary = temporary + ".authority"; bool migrationStarted = false; MigrationRecord? migrationRecord = null; try { - LogLegacyYamlMigrationRequired(session, destination); + if (!sourcePath.EndsWith(".json", StringComparison.OrdinalIgnoreCase)) + { + throw new InvalidOperationException("only a legacy JSON policy is eligible for migration"); + } using PinnedPath destinationPath = PinPathWithoutReparse( destination, leafIsDirectory: false, @@ -89,10 +187,28 @@ public static ActionResult MigrateLegacyPackageBrokerPolicy(Session session) if (destinationPath.Leaf != null) { VerifyPackageBrokerSecurity(SecurityFromHandle(destinationPath.Leaf, isDirectory: false)); - session.Log($"package broker policy already exists at {destination}; legacy migration skipped"); + log($"package broker policy already exists at {destination}; legacy migration skipped"); return ActionResult.Success; } + bool authorityPresent; + using (PinnedPath authority = PinPathWithoutReparse( + AuthorityMarkerPath(destination), false, true, WinAPI.GENERIC_READ | WinAPI.READ_CONTROL)) + { + authorityPresent = authority.Leaf != null; + } + if (authorityPresent) + { + RollbackLegacyPolicy(log, sourcePath, destination, marker); + using PinnedPath authority = PinPathWithoutReparse( + AuthorityMarkerPath(destination), false, true, WinAPI.GENERIC_READ | WinAPI.READ_CONTROL); + if (authority.Leaf != null) + { + log("managed policy authority already exists; preserving legacy policy for manual recovery"); + return ActionResult.Failure; + } + } + using PinnedPath source = PinPathWithoutReparse( sourcePath, leafIsDirectory: false, @@ -107,7 +223,7 @@ public static ActionResult MigrateLegacyPackageBrokerPolicy(Session session) SecurityFromHandle(source.Leaf, isDirectory: false), out string sourceSecurityDiagnostic)) { - session.Log( + 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."); @@ -116,11 +232,49 @@ public static ActionResult MigrateLegacyPackageBrokerPolicy(Session session) string sourceIdentity = FileIdentity(source.Leaf); string sourceDigest = FileContentDigest(source.Leaf); + string sourceSecurity = SecurityFromHandle(source.Leaf, isDirectory: false) + .GetSecurityDescriptorSddlForm(AccessControlSections.Owner | AccessControlSections.Group | AccessControlSections.Access); + byte[] original; migrationStarted = true; using (FileStream sourceStream = OpenPinnedFileStream(source.Leaf)) + { + if (sourceStream.Length > 1024 * 1024) + { + throw new InvalidOperationException("legacy policy exceeds the migration size limit"); + } + using MemoryStream content = new(); + sourceStream.CopyTo(content); + original = content.ToArray(); + } + // A conversion failure must abort the upgrade, not activate Missing/default. + byte[] converted = convert(original); + bool preserveSource = !original.SequenceEqual(converted); + string backupIdentity = null; + if (preserveSource) + { + backupIdentity = PreserveLegacyBackup(marker + ".original", original, sourceDigest); + } + string authorityIdentity = PreserveLegacyBackup( + authorityTemporary, Array.Empty(), EmptyContentDigest); + using (PinnedPath previousMarker = PinPathWithoutReparse( + marker, false, true, WinAPI.GENERIC_READ | WinAPI.DELETE | WinAPI.READ_CONTROL)) + { + if (previousMarker.Leaf != null) + { + VerifyPackageBrokerSecurity(SecurityFromHandle(previousMarker.Leaf, false)); + MigrationRecord previous = ReadMigrationMarker(previousMarker.Leaf); + if (!FileIdentityAndDigestMatch(source.Leaf, previous.SourceIdentity, previous.SourceDigest) || + previous.SourceSecurity != sourceSecurity || + previous.BackupIdentity != backupIdentity) + { + throw new InvalidOperationException("interrupted migration evidence changed; manual recovery required"); + } + DeleteFileByHandle(previousMarker.Leaf); + } + } using (FileStream target = new(temporary, FileMode.CreateNew, FileAccess.ReadWrite, FileShare.None)) { - sourceStream.CopyTo(target); + target.Write(converted, 0, converted.Length); target.Flush(true); } @@ -136,16 +290,23 @@ public static ActionResult MigrateLegacyPackageBrokerPolicy(Session session) sourceIdentity, sourceDigest, FileIdentity(temporaryPath.Leaf), - FileContentDigest(temporaryPath.Leaf)); + FileContentDigest(temporaryPath.Leaf), + sourceSecurity, + backupIdentity, + authorityIdentity); } MigrationRecord record = migrationRecord.Value; using PinnedPath markerPath = WriteMigrationMarker(marker, record); + if (MoveFileNoReplace(authorityTemporary, AuthorityMarkerPath(destination)) == + NoReplaceMoveResult.DestinationExists) + { + throw new InvalidOperationException( + "managed policy authority appeared during migration; preserving source and recovery evidence"); + } if (MoveFileNoReplace(temporary, destination) == NoReplaceMoveResult.DestinationExists) { - VerifyPackageBrokerSecurity(SecurityFromHandle(markerPath.Leaf, isDirectory: false)); - DeleteFileByHandle(markerPath.Leaf); - session.Log( + log( $"package broker policy appeared at {destination} during migration; " + "the external destination and legacy source were preserved"); return ActionResult.Success; @@ -165,36 +326,38 @@ public static ActionResult MigrateLegacyPackageBrokerPolicy(Session session) throw new InvalidOperationException("migrated package broker policy identity changed unexpectedly"); } - session.Log($"migrated legacy package broker policy from {sourcePath} to {destination}"); + log($"migrated legacy package broker policy from {sourcePath} to {destination}"); return ActionResult.Success; } catch (Exception error) { if (!migrationStarted) { - session.Log( + 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}"); + log($"failed to migrate legacy package broker policy; source preserved, manual remediation required: {error}"); return ActionResult.Failure; } finally { TryDeleteTemporaryFile( - session, + log, temporary, migrationRecord?.DestinationIdentity, migrationRecord?.DestinationDigest); + TryDeleteTemporaryFile(log, authorityTemporary, migrationRecord?.AuthorityIdentity, EmptyContentDigest); } } [CustomAction] public static ActionResult RollbackLegacyPackageBrokerPolicyMigration(Session session) - { - string marker = MigrationMarkerPath(session); - string destination = DestinationPolicyPath; + => RollbackLegacyPolicy(session.Log, LegacyPolicyPath, DestinationPolicyPath, MigrationMarkerPath(session)); + internal static ActionResult RollbackLegacyPolicy( + Action log, string sourcePath, string destination, string marker) + { try { using PinnedPath markerPath = PinPathWithoutReparse( @@ -209,28 +372,75 @@ public static ActionResult RollbackLegacyPackageBrokerPolicyMigration(Session se VerifyPackageBrokerSecurity(SecurityFromHandle(markerPath.Leaf, isDirectory: false)); MigrationRecord record = ReadMigrationMarker(markerPath.Leaf); + using PinnedPath source = PinPathWithoutReparse( + sourcePath, false, true, WinAPI.GENERIC_READ | WinAPI.READ_CONTROL); + using PinnedPath backup = record.BackupIdentity == null ? null : PinPathWithoutReparse( + marker + ".original", false, false, WinAPI.GENERIC_READ | WinAPI.READ_CONTROL); + if (source.Leaf == null || + !FileIdentityAndDigestMatch(source.Leaf, record.SourceIdentity, record.SourceDigest)) + { + throw new InvalidOperationException("legacy rollback source changed; preserving all copies for manual recovery"); + } + FileSystemSecurity sourceSecurity = SecurityFromHandle(source.Leaf, false); + VerifyLegacyPolicySourceSecurity(sourceSecurity); + if (record.SourceSecurity != null && record.SourceSecurity != sourceSecurity.GetSecurityDescriptorSddlForm( + AccessControlSections.Owner | AccessControlSections.Group | AccessControlSections.Access)) + { + throw new InvalidOperationException("legacy rollback source security changed; manual recovery required"); + } + if (record.BackupIdentity != null) + { + VerifyPackageBrokerSecurity(SecurityFromHandle(backup.Leaf, false)); + if (!FileIdentityAndDigestMatch(backup.Leaf, record.BackupIdentity, record.SourceDigest)) + { + throw new InvalidOperationException("legacy rollback evidence changed; preserving all copies for manual recovery"); + } + } 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 && + bool destinationOwned = destinationPath.Leaf == null || FileIdentityAndDigestMatch( destinationPath.Leaf, record.DestinationIdentity, - record.DestinationDigest)) + record.DestinationDigest); + if (!destinationOwned) + { + log("managed policy changed after migration; preserving destination and authority during rollback"); + return ActionResult.Success; + } + using PinnedPath authority = record.AuthorityIdentity == null ? null : PinPathWithoutReparse( + AuthorityMarkerPath(destination), false, true, WinAPI.GENERIC_READ | WinAPI.DELETE | WinAPI.READ_CONTROL); + if (authority?.Leaf != null) + { + VerifyPackageBrokerSecurity(SecurityFromHandle(authority.Leaf, false)); + if (!FileIdentityAndDigestMatch(authority.Leaf, record.AuthorityIdentity, EmptyContentDigest)) + { + throw new InvalidOperationException("managed authority changed; manual rollback required"); + } + } + if (destinationPath.Leaf != null) { VerifyPackageBrokerSecurity(SecurityFromHandle(destinationPath.Leaf, isDirectory: false)); DeleteFileByHandle(destinationPath.Leaf); } + if (authority?.Leaf != null) + { + DeleteFileByHandle(authority.Leaf); + } - VerifyPackageBrokerSecurity(SecurityFromHandle(markerPath.Leaf, isDirectory: false)); - DeleteFileByHandle(markerPath.Leaf); + if (record.BackupIdentity == null) + { + VerifyPackageBrokerSecurity(SecurityFromHandle(markerPath.Leaf, isDirectory: false)); + DeleteFileByHandle(markerPath.Leaf); + } } catch (Exception error) { - session.Log($"failed to roll back legacy package broker policy migration: {error}"); + log($"failed to roll back legacy package broker policy migration: {error}"); } return ActionResult.Success; @@ -240,7 +450,7 @@ public static ActionResult RollbackLegacyPackageBrokerPolicyMigration(Session se public static ActionResult CommitLegacyPackageBrokerPolicyMigration(Session session) => RunBestEffortCommit( session.Log, - () => CommitLegacyPackageBrokerPolicyMigrationCore(session)); + () => CommitLegacyPolicy(session.Log, LegacyPolicyPath, DestinationPolicyPath, MigrationMarkerPath(session))); internal static ActionResult RunBestEffortCommit(Action log, Action commit) { @@ -256,10 +466,8 @@ internal static ActionResult RunBestEffortCommit(Action log, Action comm return ActionResult.Success; } - private static void CommitLegacyPackageBrokerPolicyMigrationCore(Session session) + internal static void CommitLegacyPolicy(Action log, string sourcePath, string destination, string marker) { - string marker = MigrationMarkerPath(session); - string sourcePath = LegacyPolicyPath; using PinnedPath markerPath = PinPathWithoutReparse( marker, leafIsDirectory: false, @@ -272,6 +480,30 @@ private static void CommitLegacyPackageBrokerPolicyMigrationCore(Session session VerifyPackageBrokerSecurity(SecurityFromHandle(markerPath.Leaf, isDirectory: false)); MigrationRecord record = ReadMigrationMarker(markerPath.Leaf); + if (record.BackupIdentity != null) + { + log("preserving the legacy policy, protected original and migration evidence for Agent downgrade recovery"); + return; + } + using PinnedPath published = PinPathWithoutReparse( + destination, false, true, WinAPI.GENERIC_READ | WinAPI.READ_CONTROL); + if (published.Leaf == null || + !FileIdentityAndDigestMatch(published.Leaf, record.DestinationIdentity, record.DestinationDigest)) + { + log("migration did not publish the current managed policy; preserving source and evidence"); + return; + } + VerifyPackageBrokerSecurity(SecurityFromHandle(published.Leaf, false)); + using PinnedPath authority = record.AuthorityIdentity == null ? null : PinPathWithoutReparse( + AuthorityMarkerPath(destination), false, false, WinAPI.GENERIC_READ | WinAPI.READ_CONTROL); + if (authority != null) + { + VerifyPackageBrokerSecurity(SecurityFromHandle(authority.Leaf, false)); + if (!FileIdentityAndDigestMatch(authority.Leaf, record.AuthorityIdentity, EmptyContentDigest)) + { + throw new InvalidOperationException("managed authority changed before migration commit"); + } + } bool sourceChanged; bool removeSource; @@ -291,7 +523,7 @@ private static void CommitLegacyPackageBrokerPolicyMigrationCore(Session session source.Leaf, out string configuredDiagnostic)) { - session.Log( + log( $"preserving the configured legacy package broker policy during commit: {configuredDiagnostic}"); removeSource = false; } @@ -300,7 +532,7 @@ private static void CommitLegacyPackageBrokerPolicyMigrationCore(Session session SecurityFromHandle(source.Leaf, isDirectory: false), out string sourceSecurityDiagnostic)) { - session.Log( + log( $"preserving the legacy package broker policy during commit: {sourceSecurityDiagnostic}"); removeSource = false; } @@ -313,13 +545,13 @@ private static void CommitLegacyPackageBrokerPolicyMigrationCore(Session session { if (sourceChanged) { - session.Log( + log( "legacy package broker policy changed after migration; preserving the current source"); } return; } - TryDeleteLegacyPolicySource(session, sourcePath, record); + TryDeleteLegacyPolicySource(log, sourcePath, record); } internal static void EnsureSecureDirectoryTree(string programData, string target) @@ -536,7 +768,10 @@ error is JsonException || } } - internal static bool TryValidateConfiguredLocalPolicyPath(string path, out string diagnostic) + internal static bool TryValidateConfiguredLocalPolicyPath(string path, out string diagnostic) => + TryValidateLocalFilePath(path, ".json", out diagnostic); + + private static bool TryValidateLocalFilePath(string path, string extension, out string diagnostic) { diagnostic = null; string root; @@ -583,9 +818,9 @@ internal static bool TryValidateConfiguredLocalPolicyPath(string path, out strin diagnostic = "PackageBroker.PolicyPath has an unsafe local path shape"; return false; } - if (!string.Equals(Path.GetExtension(relative), ".json", StringComparison.OrdinalIgnoreCase)) + if (!string.Equals(Path.GetExtension(relative), extension, StringComparison.OrdinalIgnoreCase)) { - diagnostic = "PackageBroker.PolicyPath must name a JSON file"; + diagnostic = $"local path must name a {extension} file"; return false; } @@ -657,7 +892,8 @@ internal static PinnedPath PinPathWithoutReparse( string path, bool leafIsDirectory, bool allowMissingLeaf, - uint leafAccess) + uint leafAccess, + bool verifyTrustedAncestors = false) { string fullPath = Path.GetFullPath(path); string root = Path.GetPathRoot(fullPath); @@ -681,11 +917,15 @@ internal static PinnedPath PinPathWithoutReparse( SafeFileHandle ancestorHandle = OpenPathWithoutReparse( ancestor, isDirectory: true, - WinAPI.FILE_READ_ATTRIBUTES, + WinAPI.FILE_READ_ATTRIBUTES | (verifyTrustedAncestors ? WinAPI.READ_CONTROL : 0), WinAPI.FILE_SHARE_READ | WinAPI.FILE_SHARE_WRITE, allowMissing: false); - VerifyResolvedPath(ancestorHandle, ancestor); handles.Add(ancestorHandle); + VerifyResolvedPath(ancestorHandle, ancestor); + if (verifyTrustedAncestors) + { + VerifyTrustedDirectorySecurity(SecurityFromHandle(ancestorHandle, true)); + } } uint shareMode = (leafAccess & WinAPI.GENERIC_READ) != 0 @@ -770,7 +1010,27 @@ internal static bool DeleteFileIfIdentityAndDigestMatch( internal static MigrationRecord ReadMigrationMarkerJson(string markerJson) { - JObject document = JObject.Parse(markerJson); + if (ContainsNonStrictJsonSyntax(markerJson)) + { + throw new InvalidOperationException("package broker migration marker is not strict JSON"); + } + using JsonTextReader reader = new(new StringReader(markerJson)) { DateParseHandling = DateParseHandling.None }; + JObject document = JObject.Load(reader, new JsonLoadSettings + { + DuplicatePropertyNameHandling = DuplicatePropertyNameHandling.Error, + CommentHandling = CommentHandling.Load, + }); + string[] fields = + { + "SourceIdentity", "SourceDigest", "DestinationIdentity", "DestinationDigest", + "SourceSecurity", "BackupIdentity", "AuthorityIdentity", + }; + if (reader.Read() || document.Properties().Any(property => + !fields.Contains(property.Name) || + (property.Value.Type != JTokenType.String && property.Value.Type != JTokenType.Null))) + { + throw new InvalidOperationException("package broker migration marker has unexpected content"); + } string sourceIdentity = document.Value("SourceIdentity"); string sourceDigest = document.Value("SourceDigest"); string destinationIdentity = document.Value("DestinationIdentity"); @@ -782,7 +1042,14 @@ internal static MigrationRecord ReadMigrationMarkerJson(string markerJson) { throw new InvalidOperationException("package broker migration marker is incomplete"); } - return new MigrationRecord(sourceIdentity, sourceDigest, destinationIdentity, destinationDigest); + if (document.Value("BackupIdentity") != null && + string.IsNullOrEmpty(document.Value("SourceSecurity"))) + { + throw new InvalidOperationException("converted policy backup has no source security evidence"); + } + return new MigrationRecord(sourceIdentity, sourceDigest, destinationIdentity, destinationDigest, + document.Value("SourceSecurity"), document.Value("BackupIdentity"), + document.Value("AuthorityIdentity")); } private static string MigrationMarkerPath(Session session) => @@ -806,6 +1073,15 @@ internal static NoReplaceMoveResult MoveFileNoReplace(string source, string dest throw new Win32Exception(error, $"failed to move {source} to {destination} without replacement"); } + private static string EmptyContentDigest + { + get + { + using SHA256 sha256 = SHA256.Create(); + return Convert.ToBase64String(sha256.ComputeHash(Array.Empty())); + } + } + private static PinnedPath WriteMigrationMarker(string marker, MigrationRecord record) { using (FileStream markerFile = new(marker, FileMode.CreateNew, FileAccess.ReadWrite, FileShare.None)) @@ -1267,7 +1543,7 @@ internal static bool TryProbePinnedOrdinaryFile(string path, out string diagnost } private static void TryDeleteTemporaryFile( - Session session, + Action log, string path, string expectedIdentity, string expectedDigest) @@ -1284,7 +1560,7 @@ private static void TryDeleteTemporaryFile( if (expectedIdentity != null && !FileIdentityAndDigestMatch(temporary.Leaf, expectedIdentity, expectedDigest)) { - session.Log( + log( $"package broker policy migration temporary path {path} was replaced; preserving the current file"); return; } @@ -1292,7 +1568,7 @@ private static void TryDeleteTemporaryFile( } catch (Exception error) { - session.Log($"failed to remove package broker policy migration temporary file {path}: {error}"); + log($"failed to remove package broker policy migration temporary file {path}: {error}"); } } @@ -1345,12 +1621,6 @@ internal static bool TryDeleteLegacyPolicySource( } } - private static bool TryDeleteLegacyPolicySource( - Session session, - string sourcePath, - MigrationRecord record) => - TryDeleteLegacyPolicySource(session.Log, sourcePath, record); - internal sealed class PinnedPath : IDisposable { private readonly IReadOnlyList handles; @@ -1378,18 +1648,27 @@ internal MigrationRecord( string sourceIdentity, string sourceDigest, string destinationIdentity, - string destinationDigest) + string destinationDigest, + string sourceSecurity = null, + string backupIdentity = null, + string authorityIdentity = null) { SourceIdentity = sourceIdentity; SourceDigest = sourceDigest; DestinationIdentity = destinationIdentity; DestinationDigest = destinationDigest; + SourceSecurity = sourceSecurity; + BackupIdentity = backupIdentity; + AuthorityIdentity = authorityIdentity; } internal string SourceIdentity { get; } internal string SourceDigest { get; } internal string DestinationIdentity { get; } internal string DestinationDigest { get; } + internal string SourceSecurity { get; } + internal string BackupIdentity { get; } + internal string AuthorityIdentity { get; } internal string ToJson() => new JObject @@ -1398,6 +1677,9 @@ internal string ToJson() => ["SourceDigest"] = SourceDigest, ["DestinationIdentity"] = DestinationIdentity, ["DestinationDigest"] = DestinationDigest, + ["SourceSecurity"] = SourceSecurity, + ["BackupIdentity"] = BackupIdentity, + ["AuthorityIdentity"] = AuthorityIdentity, }.ToString(Formatting.None, Array.Empty()); } diff --git a/testsuite/Cargo.toml b/testsuite/Cargo.toml index 1cc45390a..9b534bef8 100644 --- a/testsuite/Cargo.toml +++ b/testsuite/Cargo.toml @@ -15,6 +15,10 @@ name = "integration_tests" path = "tests/main.rs" harness = true +[[test]] +name = "installer_policy_migration" +path = "tests/installer_policy_migration.rs" + [dependencies] anyhow = "1.0" assert_cmd = "2.2" @@ -65,6 +69,7 @@ uuid = { version = "1", features = ["v4"] } sysevent-syslog.path = "../crates/sysevent-syslog" [target.'cfg(windows)'.dev-dependencies] +now-package-broker.path = "../crates/now-package-broker" sysevent-winevent.path = "../crates/sysevent-winevent" [lints] diff --git a/testsuite/tests/installer_policy_migration.rs b/testsuite/tests/installer_policy_migration.rs new file mode 100644 index 000000000..ea942f535 --- /dev/null +++ b/testsuite/tests/installer_policy_migration.rs @@ -0,0 +1,111 @@ +#![cfg(windows)] + +use now_package_broker::installer_policy_migration::convert_document; +use rstest::rstest; + +const LEGACY: &str = r#"{ + "$schema":"https://devolutions.net/schemas/now-policy.schema.1.0.json", + "PolicyVersion":"1.0.0", + "PolicyType":"PackageBrokerPolicy", + "Metadata":{"Id":"policy-a","Publisher":"Test\u0020Publisher","Revision":17,"PublishedAt":"2026-01-01T09:00:00.000+09:00","ValidFrom":"2025-01-01T00:00:00Z","ValidUntil":"2027-01-01T00:00:00Z"}, + "Enforcement":{"DefaultDecision":"Deny","RulePrecedence":"PriorityThenDeny"}, + "Rules":[{"Id":"z","Priority":2,"Decision":"Deny","Match":{"Managers":["Winget"]}},{"Id":"a","Priority":1,"Decision":"Allow","Match":{"Managers":["Winget"]}}] +}"#; + +#[rstest] +#[case("1.0.0")] +#[case("1.7.3")] +#[case("1.18446744073709551615.0")] +fn preserves_version_and_raw_content(#[case] version: &str) { + let input = LEGACY.replace("1.0.0", version); + let output = convert_document(&input).expect("convert compatible legacy policy"); + let before: serde_json::Value = serde_json::from_str(&input).expect("parse legacy fixture"); + let after: serde_json::Value = serde_json::from_str(&output).expect("parse converted policy"); + assert!(after.get("$schema").is_none()); + assert!(after.get("PolicyVersion").is_none()); + assert_eq!(after["PolicyFormatVersion"], version); + for field in ["PolicyType", "Metadata", "Enforcement", "Rules"] { + assert_eq!(before[field], after[field]); + } + for line in input.lines().filter(|line| { + ["\"Metadata\":", "\"Enforcement\":", "\"Rules\":"] + .iter() + .any(|prefix| line.trim_start().starts_with(prefix)) + }) { + assert!(output.contains(line.trim().trim_end_matches(','))); + } + assert_eq!(convert_document(&output).expect("validate current policy"), output); +} + +#[rstest] +#[case("0.9.0")] +#[case("2.0.0")] +#[case("1.01.0")] +#[case("01.0.0")] +#[case("1.0")] +#[case("1.0.0-beta")] +#[case("1.0.0+build")] +#[case("1.0.0 ")] +#[case("1.18446744073709551616.0")] +fn rejects_incompatible_or_noncanonical_versions(#[case] version: &str) { + assert!(convert_document(&LEGACY.replace("1.0.0", version)).is_err()); +} + +#[rstest] +#[case("\"PolicyFormatVersion\":\"1.0.0\",")] +#[case("\"PolicyVersion\":\"1.0.0\",")] +#[case("\"Policy\\u0056ersion\":\"1.0.0\",")] +#[case("\"$schema\":\"https://devolutions.net/schemas/now-policy.schema.1.0.json\",")] +#[case("\"policyVersion\":\"1.0.0\",")] +#[case("\"Id\":\"ambiguous\",")] +#[case("\"Unknown\":true,")] +fn rejects_mixed_duplicate_and_unknown_identity(#[case] extra: &str) { + assert!(convert_document(&LEGACY.replacen('{', &format!("{{{extra}"), 1)).is_err()); +} + +#[rstest] +#[case("https://example.com/schema")] +#[case("https://devolutions.net/schemas/now-policy-draft.schema.1.0.json")] +fn rejects_wrong_schema(#[case] schema: &str) { + assert!( + convert_document(&LEGACY.replace("https://devolutions.net/schemas/now-policy.schema.1.0.json", schema)) + .is_err() + ); +} + +#[rstest] +#[case("\"Revision\":17", "\"Revision\":0")] +#[case("\"Revision\":17", "\"Revision\":17,\"Revision\":18")] +#[case("\"Publisher\":", "\"Unknown\":true,\"Publisher\":")] +#[case("\"Id\":\"a\"", "\"Id\":\"z\"")] +#[case("\"ValidUntil\":\"2027", "\"ValidUntil\":\"2024")] +#[case("\"PublishedAt\":\"2026", "\"PublishedAt\":\"invalid")] +#[case("\"Priority\":2", "\"Priority\":2,\"Priority\":3")] +#[case("\"Priority\":2", "\"Priority\":2147483648")] +#[case("\"DefaultDecision\":", "\"Unknown\":true,\"DefaultDecision\":")] +fn rejects_malformed_and_semantically_invalid_content(#[case] from: &str, #[case] to: &str) { + assert!(convert_document(&LEGACY.replace(from, to)).is_err()); +} + +#[rstest] +#[case("\"PolicyFormatVersion\":\"1.7.3\",")] +#[case("\"PolicyFormat\\u0056ersion\":\"1.0.0\",")] +#[case("\"PolicyVersion\":\"1.0.0\",")] +#[case("\"$schema\":\"https://devolutions.net/schemas/now-policy.schema.1.0.json\",")] +fn current_documents_do_not_accept_ambiguous_identity(#[case] extra: &str) { + let current = convert_document(LEGACY).expect("convert legacy fixture"); + assert!(convert_document(¤t.replacen('{', &format!("{{{extra}"), 1)).is_err()); +} + +#[test] +fn rejects_non_json_and_oversized_documents() { + for input in [ + "PolicyVersion: 1.0.0".to_owned(), + format!("{LEGACY} {{}}"), + LEGACY.replacen('{', "{/* comment */", 1), + LEGACY.replace("\"Rules\":", "'Rules':"), + " ".repeat(1024 * 1024 + 1), + ] { + assert!(convert_document(&input).is_err()); + } +}