From d1fe413895679e1f76c0e14bf7a16ef3e70dfda8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Wed, 9 Sep 2026 01:22:26 -0400 Subject: [PATCH 01/25] feat(agent,agent-installer): add policy consent helper Install one protected NativeAOT helper that authenticates retained UniGetUI and Agent process identities before forwarding bounded policy replacement requests. Preserve protocol 2.0 conflict and uncertainty semantics while integrating transactional discovery, signing, and packaging. Issue: #1963 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .github/workflows/ci.yml | 31 + .github/workflows/package.yml | 7 +- ci/package-agent-windows.ps1 | 10 +- crates/now-package-broker/src/auth.rs | 83 +++ crates/now-package-broker/src/server/mod.rs | 7 +- ...DevolutionsAgentPolicyConsent.Tests.csproj | 22 + .../AgentPolicyConsent.Tests/ProtocolTests.cs | 445 ++++++++++++ package/AgentPolicyConsent/BrokerClient.cs | 250 +++++++ .../DevolutionsAgentPolicyConsent.csproj | 23 + package/AgentPolicyConsent/PeerTrust.cs | 682 ++++++++++++++++++ .../PolicyConsentContract.cs | 13 + package/AgentPolicyConsent/Program.cs | 173 +++++ package/AgentPolicyConsent/Protocol.cs | 240 ++++++ package/AgentPolicyConsent/app.manifest | 11 + .../PolicyConsentDiscoveryTests.cs | 45 ++ .../DevolutionsAgent.csproj | 1 + package/AgentWindowsManaged/Program.cs | 55 ++ .../AgentWindowsManaged/Resources/Includes.cs | 12 + 18 files changed, 2106 insertions(+), 4 deletions(-) create mode 100644 package/AgentPolicyConsent.Tests/DevolutionsAgentPolicyConsent.Tests.csproj create mode 100644 package/AgentPolicyConsent.Tests/ProtocolTests.cs create mode 100644 package/AgentPolicyConsent/BrokerClient.cs create mode 100644 package/AgentPolicyConsent/DevolutionsAgentPolicyConsent.csproj create mode 100644 package/AgentPolicyConsent/PeerTrust.cs create mode 100644 package/AgentPolicyConsent/PolicyConsentContract.cs create mode 100644 package/AgentPolicyConsent/Program.cs create mode 100644 package/AgentPolicyConsent/Protocol.cs create mode 100644 package/AgentPolicyConsent/app.manifest create mode 100644 package/AgentWindowsManaged.Tests/PolicyConsentDiscoveryTests.cs diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 87550f9d6..a381ea780 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -887,6 +887,9 @@ jobs: $DAgentSessionExecutable = Join-Path $TargetOutputPath "DevolutionsSession.exe" echo "dagent-session-executable=$DAgentSessionExecutable" >> $Env:GITHUB_OUTPUT + $DAgentPolicyConsentHelper = Join-Path $TargetOutputPath "DevolutionsAgentPolicyConsent.exe" + echo "dagent-policy-consent-helper=$DAgentPolicyConsentHelper" >> $Env:GITHUB_OUTPUT + $DAgentUpdaterExecutable = Join-Path $TargetOutputPath "DevolutionsAgentUpdater.exe" echo "dagent-updater-executable=$DAgentUpdaterExecutable" >> $Env:GITHUB_OUTPUT } @@ -1052,6 +1055,28 @@ jobs: DAGENT_EXECUTABLE: ${{ steps.load-variables.outputs.dagent-executable }} TARGET_OUTPUT_PATH: ${{ steps.load-variables.outputs.target-output-path }} + - name: Build NativeAOT policy consent helper + if: ${{ matrix.os == 'windows' }} + run: | + $Rid = "win-${{ matrix.arch }}" + $Output = Split-Path -Parent '${{ steps.load-variables.outputs.dagent-policy-consent-helper }}' + dotnet publish package/AgentPolicyConsent/DevolutionsAgentPolicyConsent.csproj ` + --configuration Release ` + --runtime $Rid ` + --output $Output ` + -p:Version=${{ needs.preflight.outputs.version }} + if ($LASTEXITCODE -ne 0) { + exit $LASTEXITCODE + } + $Helper = '${{ steps.load-variables.outputs.dagent-policy-consent-helper }}' + if (-Not (Test-Path -LiteralPath $Helper -PathType Leaf)) { + throw "NativeAOT policy consent helper was not produced" + } + if ((Get-Item -LiteralPath $Helper).Length -gt 8MB) { + throw "NativeAOT policy consent helper exceeds 8 MiB" + } + shell: pwsh + - name: Package if: ${{ github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository }} run: | @@ -1065,6 +1090,7 @@ jobs: $Env:DAGENT_PEDM_SHELL_EXT_DLL = "${{ steps.load-variables.outputs.dagent-pedm-shell-ext-dll }}" $Env:DAGENT_PEDM_SHELL_EXT_MSIX = "${{ steps.load-variables.outputs.dagent-pedm-shell-ext-msix }}" $Env:DAGENT_SESSION_EXECUTABLE = "${{ steps.load-variables.outputs.dagent-session-executable }}" + $Env:DAGENT_POLICY_CONSENT_HELPER = "${{ steps.load-variables.outputs.dagent-policy-consent-helper }}" $Env:DAGENT_TUN2SOCKS_EXE = "${{ steps.tun2socks.outputs.tun2socks-executable-path }}" $Env:DAGENT_WINTUN_DLL = "${{ steps.tun2socks.outputs.wintun-library-path }}" $Env:DAGENT_MULTI_PWSH_EXECUTABLE = "${{ steps.multi-pwsh.outputs.executable-path }}" @@ -1186,6 +1212,11 @@ jobs: run: dotnet test package/AgentWindowsManaged.Tests/DevolutionsAgent.Installer.Tests.csproj shell: pwsh + - name: Policy consent helper tests + run: dotnet test package/AgentPolicyConsent.Tests/DevolutionsAgentPolicyConsent.Tests.csproj -c Release + shell: pwsh + + winapi-sanitizer-tests: name: Windows API sanitizer tests runs-on: windows-2022 diff --git a/.github/workflows/package.yml b/.github/workflows/package.yml index 27ee014f9..af56e0b9f 100644 --- a/.github/workflows/package.yml +++ b/.github/workflows/package.yml @@ -322,7 +322,7 @@ jobs: run: | $IncludePattern = @(switch ('${{ matrix.project }}') { 'devolutions-gateway' { @('DevolutionsGateway.exe') } - 'devolutions-agent' { @('DevolutionsAgent.exe', 'DevolutionsAgentUpdater.exe', 'DevolutionsPedmShellExt.dll', 'DevolutionsPedmShellExt.msix', 'DevolutionsDesktopAgent.exe') } + 'devolutions-agent' { @('DevolutionsAgent.exe', 'DevolutionsAgentUpdater.exe', 'DevolutionsAgentPolicyConsent.exe', 'DevolutionsPedmShellExt.dll', 'DevolutionsPedmShellExt.msix', 'DevolutionsDesktopAgent.exe') } 'jetsocat' { @('jetsocat.exe', 'jetsocat') } }) $ExcludePattern = "*.pdb" @@ -495,6 +495,7 @@ jobs: $Env:DAGENT_PEDM_SHELL_EXT_DLL = Get-ChildItem -Path $ArchRoot -Filter 'DevolutionsPedmShellExt.dll' -File | Select-Object -First 1 $Env:DAGENT_PEDM_SHELL_EXT_MSIX = Get-ChildItem -Path $ArchRoot -Filter 'DevolutionsPedmShellExt.msix' -File | Select-Object -First 1 $Env:DAGENT_SESSION_EXECUTABLE = Get-ChildItem -Path $ArchRoot -Filter 'DevolutionsSession.exe' -File | Select-Object -First 1 + $Env:DAGENT_POLICY_CONSENT_HELPER = Get-ChildItem -Path $ArchRoot -Filter 'DevolutionsAgentPolicyConsent.exe' -File | Select-Object -First 1 $Env:DAGENT_TUN2SOCKS_EXE = Join-Path $ArchRoot 'tun2socks.exe' $Env:DAGENT_WINTUN_DLL = Join-Path $ArchRoot 'wintun.dll' $MultiPwshDirectory = Join-Path $Env:RUNNER_TEMP 'multi-pwsh' 'windows' $Arch @@ -508,6 +509,7 @@ jobs: Write-Host "DAGENT_PEDM_SHELL_EXT_DLL = ${Env:DAGENT_PEDM_SHELL_EXT_DLL}" Write-Host "DAGENT_PEDM_SHELL_EXT_MSIX = ${Env:DAGENT_PEDM_SHELL_EXT_MSIX}" Write-Host "DAGENT_SESSION_EXECUTABLE = ${Env:DAGENT_SESSION_EXECUTABLE}" + Write-Host "DAGENT_POLICY_CONSENT_HELPER = ${Env:DAGENT_POLICY_CONSENT_HELPER}" Write-Host "DAGENT_TUN2SOCKS_EXE = ${Env:DAGENT_TUN2SOCKS_EXE}" Write-Host "DAGENT_WINTUN_DLL = ${Env:DAGENT_WINTUN_DLL}" Write-Host "DAGENT_MULTI_PWSH_EXECUTABLE = ${Env:DAGENT_MULTI_PWSH_EXECUTABLE}" @@ -534,7 +536,8 @@ jobs: @((Join-Path $ArchRoot DesktopAgent), (Get-ChildItem -Path $ArchRoot -Filter 'DevolutionsPedmShellExt.dll' | Select-Object -First 1), (Get-ChildItem -Path $ArchRoot -Filter 'DevolutionsPedmShellExt.msix' | Select-Object -First 1), - (Get-ChildItem -Path $ArchRoot -Filter 'DevolutionsSession.exe' | Select-Object -First 1)) | ForEach-Object { + (Get-ChildItem -Path $ArchRoot -Filter 'DevolutionsSession.exe' | Select-Object -First 1), + (Get-ChildItem -Path $ArchRoot -Filter 'DevolutionsAgentPolicyConsent.exe' | Select-Object -First 1)) | ForEach-Object { Remove-Item $_ -Recurse -ErrorAction SilentlyContinue | Out-Null } } diff --git a/ci/package-agent-windows.ps1 b/ci/package-agent-windows.ps1 index 4a37cc512..10f0e7219 100644 --- a/ci/package-agent-windows.ps1 +++ b/ci/package-agent-windows.ps1 @@ -11,6 +11,8 @@ param( [parameter(Mandatory = $true)] [string] $SessionExe, [parameter(Mandatory = $true)] + [string] $PolicyConsentHelper, + [parameter(Mandatory = $true)] [ValidateSet('x64', 'arm64')] [string] $Architecture, [string] $Outfile @@ -98,6 +100,9 @@ function New-AgentMsi() { # The path to the devolutions-session.exe file. [string] $SessionExe, [parameter(Mandatory = $true)] + # The path to the DevolutionsAgentPolicyConsent.exe file. + [string] $PolicyConsentHelper, + [parameter(Mandatory = $true)] [ValidateSet('x64', 'arm64')] # Architecture: x64 or arm64 [string] $Architecture, @@ -120,6 +125,7 @@ function New-AgentMsi() { $PedmDll = Convert-Path -Path $PedmDll $PedmMsix = Convert-Path -Path $PedmMsix $SessionExe = Convert-Path -Path $SessionExe + $PolicyConsentHelper = Convert-Path -Path $PolicyConsentHelper if ($Outfile) { $Outfile = Convert-Path -Path $Outfile } @@ -137,6 +143,7 @@ function New-AgentMsi() { $myUpdaterExe = Set-FileNameAndCopy -Path $UpdaterExe -NewName 'DevolutionsAgentUpdater.exe' # The session is a service that gets launched on demand. $mySessionExe = Set-FileNameAndCopy -Path $SessionExe -NewName 'DevolutionsSession.exe' + $myPolicyConsentHelper = Set-FileNameAndCopy -Path $PolicyConsentHelper -NewName 'DevolutionsAgentPolicyConsent.exe' Write-Output "$repoDir\dotnet\DesktopAgent\bin\Release\net48\DevolutionsDesktopAgent.exe" @@ -145,6 +152,7 @@ function New-AgentMsi() { Set-EnvVarPath 'DAGENT_PEDM_SHELL_EXT_DLL' $myPedmDll Set-EnvVarPath 'DAGENT_PEDM_SHELL_EXT_MSIX' $myPedmMsix Set-EnvVarPath 'DAGENT_SESSION_EXECUTABLE' $mySessionExe + Set-EnvVarPath 'DAGENT_POLICY_CONSENT_HELPER' $myPolicyConsentHelper # The actual DevolutionsDesktopAgent.exe will be `\dotnet\DesktopAgent\bin\Release\net48\DevolutionsDesktopAgent.exe`. # After install, the contents of `net48` will be copied to `C:\Program Files\Devolutions\Agent\desktop\`. @@ -184,4 +192,4 @@ function New-AgentMsi() { Pop-Location } -New-AgentMsi -Generate:($Generate.IsPresent) -Exe $Exe -UpdaterExe $UpdaterExe -PedmDll $PedmDll -PedmMsix $PedmMsix -SessionExe $SessionExe -Architecture $Architecture -Outfile $Outfile +New-AgentMsi -Generate:($Generate.IsPresent) -Exe $Exe -UpdaterExe $UpdaterExe -PedmDll $PedmDll -PedmMsix $PedmMsix -SessionExe $SessionExe -PolicyConsentHelper $PolicyConsentHelper -Architecture $Architecture -Outfile $Outfile diff --git a/crates/now-package-broker/src/auth.rs b/crates/now-package-broker/src/auth.rs index d647e750c..f293a0306 100644 --- a/crates/now-package-broker/src/auth.rs +++ b/crates/now-package-broker/src/auth.rs @@ -32,6 +32,7 @@ use windows::Win32::System::Threading::{ use crate::policy_security::RetainedExecutableSecurity; const PROCESS_SYNCHRONIZE: PROCESS_ACCESS_RIGHTS = PROCESS_ACCESS_RIGHTS(0x0010_0000); +const POLICY_CONSENT_HELPER_NAME: &str = "DevolutionsAgentPolicyConsent.exe"; const PROCESS_IDENTITY_ACCESS: PROCESS_ACCESS_RIGHTS = PROCESS_ACCESS_RIGHTS( PROCESS_QUERY_INFORMATION.0 | PROCESS_QUERY_LIMITED_INFORMATION.0 | PROCESS_VM_READ.0 | PROCESS_SYNCHRONIZE.0, ); @@ -289,6 +290,20 @@ impl PipeClient { Ok(()) } + pub(crate) fn validate_policy_write(&self, skip_signature_validation: bool) -> anyhow::Result<()> { + self.validate_connection(skip_signature_validation)?; + // Dev builds cannot enforce helper identity when signature validation is explicitly disabled. + if signature_validation_skipped(skip_signature_validation) { + return Ok(()); + } + let agent = std::env::current_exe().context("failed to query Agent executable path")?; + let executable_file = self + .executable_file + .as_deref() + .context("policy consent helper executable handle is not retained")?; + Self::validate_policy_consent_helper_path(&self.executable_path, executable_file, &agent) + } + fn validate_process_instance(&self) -> anyhow::Result<()> { let Some(process) = &self.process else { return Ok(()); @@ -304,6 +319,29 @@ impl PipeClient { ) } + fn validate_policy_consent_helper_path(client: &Path, client_file: &File, agent: &Path) -> anyhow::Result<()> { + if !client + .file_name() + .is_some_and(|name| name.eq_ignore_ascii_case(POLICY_CONSENT_HELPER_NAME)) + { + bail!("policy replacement requires the Agent policy consent helper"); + } + let expected = agent + .parent() + .context("Agent executable has no installation directory")? + .join(POLICY_CONSENT_HELPER_NAME); + if !crate::policy_security::windows_paths_equal(client, &expected) { + bail!("policy consent helper is not the installed Agent helper path"); + } + let expected_id = file_id(&expected).context("failed to query installed policy consent helper identity")?; + let retained_id = + file_id_from_handle(client_file).context("failed to query retained policy consent helper identity")?; + if !same_file(&expected_id, &retained_id) { + bail!("policy consent helper does not match the installed helper"); + } + Ok(()) + } + /// Validate that the request's `effective_user` denotes the authenticated pipe client user. /// /// The name is resolved to a SID and compared against the SID captured at connect, @@ -539,6 +577,51 @@ mod tests { .expect_err("a recycled PID with a different creation time must be rejected"); } + #[test] + fn policy_consent_helper_requires_exact_agent_sibling_path() { + let current_executable = std::env::current_exe().expect("current executable"); + let current_file = open_executable_file(¤t_executable).expect("open current executable"); + let agent = Path::new(r"C:\Program Files\Devolutions\Agent\DevolutionsAgent.exe"); + assert!( + PipeClient::validate_policy_consent_helper_path( + Path::new(r"C:\Program Files\Devolutions\Agent\DevolutionsAgentPolicyConsent.exe"), + ¤t_file, + agent, + ) + .is_err(), + "path text alone must not authorize a different retained image" + ); + assert!( + PipeClient::validate_policy_consent_helper_path( + Path::new(r"C:\Users\Alice\DevolutionsAgentPolicyConsent.exe"), + ¤t_file, + agent, + ) + .is_err() + ); + assert!( + PipeClient::validate_policy_consent_helper_path( + Path::new(r"C:\Users\Alice\UniGetUI.exe"), + ¤t_file, + agent, + ) + .is_err() + ); + } + + #[test] + fn policy_consent_helper_accepts_exact_retained_sibling() { + let temp = tempfile::tempdir().expect("temp directory"); + let agent = temp.path().join("DevolutionsAgent.exe"); + let helper = temp.path().join(POLICY_CONSENT_HELPER_NAME); + std::fs::write(&agent, b"agent path anchor").expect("write Agent path anchor"); + std::fs::copy(std::env::current_exe().expect("current executable"), &helper).expect("copy helper fixture"); + let retained = open_executable_file(&helper).expect("retain helper fixture"); + + PipeClient::validate_policy_consent_helper_path(&helper, &retained, &agent) + .expect("exact retained sibling must be accepted"); + } + #[test] fn exited_process_cannot_supply_executable_identity() { let mut child = std::process::Command::new("powershell.exe") diff --git a/crates/now-package-broker/src/server/mod.rs b/crates/now-package-broker/src/server/mod.rs index 6ace82de8..77f835d87 100644 --- a/crates/now-package-broker/src/server/mod.rs +++ b/crates/now-package-broker/src/server/mod.rs @@ -307,7 +307,12 @@ async fn authenticate_policy_management( | (&Method::PUT, "/v1/policy") ); if protected { - if let Err(error) = client.validate_connection(state.skip_signature_validation) { + let authentication = if matches!((request.method(), request.uri().path()), (&Method::PUT, "/v1/policy")) { + client.validate_policy_write(state.skip_signature_validation) + } else { + client.validate_connection(state.skip_signature_validation) + }; + if let Err(error) = authentication { if let Some(audit) = write_audit { audit.denied(crate::audit::DenialReason::AuthenticationFailed); } diff --git a/package/AgentPolicyConsent.Tests/DevolutionsAgentPolicyConsent.Tests.csproj b/package/AgentPolicyConsent.Tests/DevolutionsAgentPolicyConsent.Tests.csproj new file mode 100644 index 000000000..24bdd8852 --- /dev/null +++ b/package/AgentPolicyConsent.Tests/DevolutionsAgentPolicyConsent.Tests.csproj @@ -0,0 +1,22 @@ + + + net10.0-windows + win-x64 + true + false + enable + enable + + + + + + all + + + + + + diff --git a/package/AgentPolicyConsent.Tests/ProtocolTests.cs b/package/AgentPolicyConsent.Tests/ProtocolTests.cs new file mode 100644 index 000000000..36456f288 --- /dev/null +++ b/package/AgentPolicyConsent.Tests/ProtocolTests.cs @@ -0,0 +1,445 @@ +using System.Buffers.Binary; +using System.Text.Json; +using DevolutionsAgentPolicyConsent; +using Microsoft.Win32.SafeHandles; +using Xunit; + +namespace DevolutionsAgentPolicyConsent.Tests; + +public sealed class ProtocolTests +{ + private const string RequestId = "0123456789abcdef0123456789abcdef"; + + [Fact] + public void ArgumentsRequireExactBoundIdentity() + { + Arguments parsed = Protocol.ParseArguments( + [ + "--protocol", "2.0", + "--pipe", $"UniGetUI.PolicyElevation.{RequestId}", + "--parent-pid", "42", + "--parent-created", "638900000000000000", + "--session", "1", + ]); + + Assert.Equal(42, parsed.ParentProcessId); + Assert.Equal((uint)1, parsed.SessionId); + } + + public sealed class TrustPolicyTests + { + [Fact] + public void CurrentAndTransitionSignersAreAccepted() + { + Assert.True(PeerLease.IsAllowedSigner(PeerLease.CurrentUiSignerSpkiSha256)); + Assert.True(PeerLease.IsAllowedSigner(PeerLease.TransitionUiSignerSpkiSha256)); + } + + [Fact] + public void LookalikeSignerIsRejected() + { + Assert.False(PeerLease.IsAllowedSigner(new string('0', 64))); + } + + [Fact] + public void SignerMatchingIsCaseSensitive() + { + Assert.False(PeerLease.IsAllowedSigner(PeerLease.CurrentUiSignerSpkiSha256.ToUpperInvariant())); + } + + [Theory] + [InlineData("3.3.7")] + [InlineData("2026.2.7")] + public void ProductBindingSupportsProtocolEraAndCurrentInstallModes(string version) + { + Assert.True(PeerLease.IsSupportedUiIdentity("UniGetUI", "UniGetUI.dll", version)); + } + + [Theory] + [InlineData("Lookalike", "UniGetUI.dll", "2026.2.7")] + [InlineData("UniGetUI", "malware.exe", "2026.2.7")] + [InlineData("UniGetUI", "UniGetUI.dll", "3.3.6")] + public void ProductBindingRejectsLookalikes(string product, string originalFilename, string version) + { + Assert.False(PeerLease.IsSupportedUiIdentity(product, originalFilename, version)); + } + + [Theory] + [InlineData(41, 638900000000000000, 1)] + [InlineData(42, 638900000000000001, 1)] + [InlineData(42, 638900000000000000, 2)] + public void ProcessIdentityRejectsPidReuseAndSessionMismatch(int pid, long created, uint session) + { + Arguments expected = new("pipe", 42, 638900000000000000, 1); + Assert.False(PeerLease.MatchesProcessIdentity(expected, pid, created, session)); + } + + [Fact] + public void ProcessIdentityAcceptsExactRetainedInstance() + { + Arguments expected = new("pipe", 42, 638900000000000000, 1); + Assert.True(PeerLease.MatchesProcessIdentity(expected, 42, 638900000000000000, 1)); + } + + [Fact] + public void BrokerServerRequiresExactAgentSiblingPath() + { + Assert.True(BrokerServerLease.IsExpectedPath( + @"C:\Program Files\Devolutions\Agent\DevolutionsAgent.exe", + @"c:\PROGRAM FILES\Devolutions\Agent\DevolutionsAgent.exe")); + Assert.False(BrokerServerLease.IsExpectedPath( + @"C:\Users\Alice\DevolutionsAgent.exe", + @"C:\Program Files\Devolutions\Agent\DevolutionsAgent.exe")); + } + + [Fact] + public void AuthenticodeSignerComesFromRetainedImageHandle() + { + string path = Path.Combine(AppContext.BaseDirectory, "testhost.exe"); + using SafeFileHandle image = OpenImage(path); + using var certificate = PeerLease.VerifyAuthenticodeSigner(path, image, "test host"); + Assert.NotEmpty(certificate.RawData); + } + + [Fact] + public void ProcessImageMappingAcceptsOnlyCurrentMappedImage() + { + using SafeProcessHandle process = Native.OpenProcess( + PeerLease.ProcessQueryInformation | + PeerLease.ProcessQueryLimitedInformation | + PeerLease.Synchronize, + false, + Environment.ProcessId); + Assert.False(process.IsInvalid); + + string processPath = PeerLease.ImagePath(process); + using SafeFileHandle processImage = OpenImage(processPath); + PeerLease.VerifyImageMapping(process, processImage); + + using SafeFileHandle differentImage = + OpenImage(Path.Combine(AppContext.BaseDirectory, "DevolutionsAgentPolicyConsent.exe")); + Assert.Throws(() => PeerLease.VerifyImageMapping(process, differentImage)); + } + + [Fact] + public void BrokerServerRejectsNonSystemProcessToken() + { + using SafeProcessHandle process = Native.OpenProcess( + PeerLease.ProcessQueryLimitedInformation, + false, + Environment.ProcessId); + Assert.False(process.IsInvalid); + Assert.False(PeerLease.IsLocalSystemProcess(process)); + } + + [Fact] + public void FileIdentityRejectsDifferentImage() + { + using SafeFileHandle first = OpenImage(Path.Combine(AppContext.BaseDirectory, "testhost.exe")); + using SafeFileHandle same = OpenImage(Path.Combine(AppContext.BaseDirectory, "testhost.exe")); + using SafeFileHandle different = + OpenImage(Path.Combine(AppContext.BaseDirectory, "DevolutionsAgentPolicyConsent.exe")); + + Assert.True(PeerLease.SameFile(first, same)); + Assert.False(PeerLease.SameFile(first, different)); + } + + private static SafeFileHandle OpenImage(string path) + { + SafeFileHandle image = Native.CreateFile( + path, + PeerLease.GenericRead | PeerLease.FileExecute | PeerLease.Synchronize, + PeerLease.FileShareRead, + IntPtr.Zero, + PeerLease.OpenExisting, + 0, + IntPtr.Zero); + Assert.False(image.IsInvalid); + return image; + } + } + + [Theory] + [InlineData("--extra")] + [InlineData("--pipe")] + public void ArgumentsRejectUnknownOrDuplicateNames(string name) + { + string[] args = + [ + "--protocol", "2.0", + "--pipe", $"UniGetUI.PolicyElevation.{RequestId}", + "--parent-pid", "42", + "--parent-created", "638900000000000000", + name, "1", + ]; + + Assert.Throws(() => Protocol.ParseArguments(args)); + } + + [Fact] + public async Task FrameUsesBigEndianLengthAndRoundTrips() + { + byte[] body = [1, 2, 3, 4]; + using MemoryStream stream = new(); + await Protocol.WriteFrameAsync(stream, body, body.Length, CancellationToken.None); + Assert.Equal((uint)body.Length, BinaryPrimitives.ReadUInt32BigEndian(stream.GetBuffer())); + stream.Position = 0; + Assert.Equal(body, await Protocol.ReadFrameAsync(stream, body.Length, CancellationToken.None)); + } + + [Fact] + public async Task OversizedFrameIsRejectedBeforeBodyRead() + { + byte[] header = new byte[4]; + BinaryPrimitives.WriteUInt32BigEndian(header, 128); + using MemoryStream stream = new(header); + await Assert.ThrowsAsync( + () => Protocol.ReadFrameAsync(stream, 127, CancellationToken.None)); + } + + [Fact] + public async Task ZeroLengthAndTruncatedFramesAreRejected() + { + using MemoryStream empty = new(new byte[4]); + await Assert.ThrowsAsync( + () => Protocol.ReadFrameAsync(empty, 128, CancellationToken.None)); + + using MemoryStream truncated = new([0, 0, 0, 2, 1]); + await Assert.ThrowsAsync( + () => Protocol.ReadFrameAsync(truncated, 128, CancellationToken.None)); + } + + [Fact] + public async Task FrameReadHonorsCancellation() + { + using CancellationTokenSource cancellation = new(TimeSpan.FromMilliseconds(25)); + await Assert.ThrowsAnyAsync( + () => Protocol.ReadFrameAsync(new BlockingStream(), 128, cancellation.Token)); + } + + [Fact] + public void RequestRejectsUnknownJsonMembers() + { + string json = + $$"""{"protocolVersion":"2.0","requestId":"{{RequestId}}","operation":"Update","conflictHandling":"Reject","expectedStoreToken":"a","validationReceipt":"b","warningsAcknowledged":false,"draft":{},"command":"cmd.exe"}"""; + + Assert.Throws( + () => JsonSerializer.Deserialize(json, ProtocolJsonContext.Default.ElevationRequest)); + } + + [Fact] + public void OfficialRequestContainsOnlyPolicyReplacementFields() + { + using JsonDocument draft = JsonDocument.Parse("""{"Metadata":{"Id":"tests.policy"}}"""); + ElevationRequest request = new( + "2.0", + RequestId, + "Update", + "ConfirmOverwrite", + "token", + "receipt", + true, + draft.RootElement.Clone()); + + using JsonDocument official = JsonDocument.Parse(BrokerClient.CreateOfficialRequest(request)); + string[] names = official.RootElement.EnumerateObject().Select(property => property.Name).ToArray(); + Assert.Equal( + [ + "RequestKind", + "RequestVersion", + "ExpectedStoreToken", + "Operation", + "ConflictHandling", + "WarningsAcknowledged", + "Draft", + "ValidationReceipt", + ], names); + } + + [Fact] + public void RequestRequiresEveryMemberAndObjectDraft() + { + string missingAcknowledgement = + $$$"""{"protocolVersion":"2.0","requestId":"{{{RequestId}}}","operation":"Update","conflictHandling":"Reject","expectedStoreToken":"a","validationReceipt":"b","draft":{}}"""; + Assert.Throws( + () => JsonSerializer.Deserialize(missingAcknowledgement, ProtocolJsonContext.Default.ElevationRequest)); + + using JsonDocument draft = JsonDocument.Parse("[]"); + ElevationRequest request = new("2.0", RequestId, "Update", "Reject", "a", "b", false, draft.RootElement); + Assert.Throws(() => Protocol.ValidateRequest(request)); + } + + [Theory] + [InlineData("Delete", "Reject")] + [InlineData("Update", "Overwrite")] + public void RequestRejectsUnknownOperations(string operation, string conflictHandling) + { + using JsonDocument draft = JsonDocument.Parse("{}"); + ElevationRequest request = new( + "2.0", + RequestId, + operation, + conflictHandling, + "a", + "b", + false, + draft.RootElement); + + Assert.Throws(() => Protocol.ValidateRequest(request)); + } + + [Fact] + public void CommittedResponseContainsOnlyStoreToken() + { + ElevationResponse response = new( + "2.0", + RequestId, + "Committed", + null, + null, + "new-token", + null, + null, + null); + + Protocol.ValidateResponse(response); + string json = JsonSerializer.Serialize(response, ProtocolJsonContext.Default.ElevationResponse); + Assert.DoesNotContain("payload", json, StringComparison.OrdinalIgnoreCase); + Assert.DoesNotContain("message", json, StringComparison.OrdinalIgnoreCase); + } + + [Theory] + [InlineData("Active", "policy.id")] + [InlineData("Missing", null)] + [InlineData("Invalid", null)] + public void StaleRejectionCarriesBoundedConflictContext(string state, string? policyId) + { + ElevationResponse response = new( + "2.0", + RequestId, + "Rejected", + 409, + "StalePolicyStoreToken", + null, + "current-token", + state, + policyId); + + Protocol.ValidateResponse(response); + } + + [Fact] + public void UnknownResponseCannotClaimCommitOrConflict() + { + ElevationResponse response = new( + "2.0", + RequestId, + "Unknown", + null, + "Timeout", + "claimed-token", + null, + null, + null); + + Assert.Throws(() => Protocol.ValidateResponse(response)); + } + + [Fact] + public void ResponseRequiresExplicitNullableMembers() + { + string missingConflictFields = + $$$"""{"protocolVersion":"2.0","requestId":"{{{RequestId}}}","disposition":"Unknown","brokerStatusCode":null,"brokerErrorCode":"Timeout","committedStoreToken":null}"""; + + Assert.Throws( + () => JsonSerializer.Deserialize(missingConflictFields, ProtocolJsonContext.Default.ElevationResponse)); + } + + [Fact] + public void MaximumStaleResponseFitsExactWireBudget() + { + ElevationResponse response = new( + "2.0", + RequestId, + "Rejected", + 409, + "StalePolicyStoreToken", + null, + "T" + new string('"', 511), + "Active", + "P" + new string('"', 2047)); + + Protocol.ValidateResponse(response); + byte[] body = JsonSerializer.SerializeToUtf8Bytes( + response, + ProtocolJsonContext.Default.ElevationResponse); + Assert.InRange(body.Length, 1, Protocol.MaxResponseBodyBytes); + } + + [Fact] + public void BrokerSuccessMapsToCompactCommittedAcknowledgement() + { + ElevationResponse response = BrokerClient.ParseResponse( + RequestId, + HttpResponse( + 200, + """{"ResponseKind":"PolicyReplacementResponse","ResponseVersion":"1.0","Management":{"StoreToken":"new-token"}}""")); + + Assert.Equal("Committed", response.Disposition); + Assert.Equal("new-token", response.CommittedStoreToken); + Assert.Null(response.BrokerStatusCode); + } + + [Fact] + public void BrokerStaleErrorMapsExactConflictContext() + { + ElevationResponse response = BrokerClient.ParseResponse( + RequestId, + HttpResponse( + 409, + """{"ResponseKind":"ErrorResponse","ResponseVersion":"1.0","Code":"StalePolicyStoreToken","Management":{"StoreToken":"current-token","State":"Active","Policy":{"Metadata":{"Id":"policy.id"}}}}""")); + + Assert.Equal("Rejected", response.Disposition); + Assert.Equal(409, response.BrokerStatusCode); + Assert.Equal("current-token", response.ConflictStoreToken); + Assert.Equal("Active", response.ConflictState); + Assert.Equal("policy.id", response.ConflictPolicyId); + } + + [Fact] + public void EmptyBrokerResponseMapsToUnknown() + { + ElevationResponse response = BrokerClient.ParseResponse( + RequestId, + HttpResponse(503, string.Empty)); + + Assert.Equal("Unknown", response.Disposition); + Assert.Equal(503, response.BrokerStatusCode); + Assert.Equal("EmptyResponse", response.BrokerErrorCode); + } + + private static byte[] HttpResponse(int status, string body) => + System.Text.Encoding.UTF8.GetBytes( + $"HTTP/1.1 {status} Test\r\nContent-Length: {System.Text.Encoding.UTF8.GetByteCount(body)}\r\n\r\n{body}"); + + private sealed class BlockingStream : Stream + { + public override bool CanRead => true; + public override bool CanSeek => false; + public override bool CanWrite => false; + public override long Length => throw new NotSupportedException(); + public override long Position { get => throw new NotSupportedException(); set => throw new NotSupportedException(); } + public override void Flush() => throw new NotSupportedException(); + public override int Read(byte[] buffer, int offset, int count) => throw new NotSupportedException(); + public override long Seek(long offset, SeekOrigin origin) => throw new NotSupportedException(); + public override void SetLength(long value) => throw new NotSupportedException(); + public override void Write(byte[] buffer, int offset, int count) => throw new NotSupportedException(); + public override async ValueTask ReadAsync( + Memory buffer, + CancellationToken cancellationToken = default) + { + await Task.Delay(Timeout.InfiniteTimeSpan, cancellationToken); + return 0; + } + } +} diff --git a/package/AgentPolicyConsent/BrokerClient.cs b/package/AgentPolicyConsent/BrokerClient.cs new file mode 100644 index 000000000..bdcbd5120 --- /dev/null +++ b/package/AgentPolicyConsent/BrokerClient.cs @@ -0,0 +1,250 @@ +using System.Buffers; +using System.IO.Pipes; +using System.Text; +using System.Text.Json; + +namespace DevolutionsAgentPolicyConsent; + +internal static class BrokerClient +{ + private const string PipeName = "Devolutions.Now.PackageBroker.v1"; + private const int MaximumHeaderBytes = 64 * 1024; + private const int MaximumBrokerResponseBytes = 50_606_928; + private static readonly TimeSpan ConnectTimeout = TimeSpan.FromSeconds(5); + + internal static async Task ReplaceAsync( + ElevationRequest request, + CancellationToken cancellationToken) + { + byte[] body = CreateOfficialRequest(request); + try + { + using NamedPipeClientStream pipe = new( + ".", + PipeName, + PipeDirection.InOut, + PipeOptions.Asynchronous | PipeOptions.WriteThrough, + System.Security.Principal.TokenImpersonationLevel.Anonymous); + + using CancellationTokenSource connectTimeout = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); + connectTimeout.CancelAfter(ConnectTimeout); + await pipe.ConnectAsync(connectTimeout.Token); + using BrokerServerLease broker = BrokerServerLease.Open(pipe.SafePipeHandle); + + byte[] headers = Encoding.ASCII.GetBytes( + $"PUT /v1/policy HTTP/1.1\r\nHost: now-package-broker\r\nConnection: close\r\n" + + $"Content-Type: application/json\r\nAccept: application/json\r\nContent-Length: {body.Length}\r\n\r\n"); + await pipe.WriteAsync(headers, cancellationToken); + await pipe.WriteAsync(body, cancellationToken); + await pipe.FlushAsync(cancellationToken); + + byte[] response = await ReadBoundedAsync( + pipe, + MaximumBrokerResponseBytes + MaximumHeaderBytes, + cancellationToken); + return ParseResponse(request.RequestId, response); + } + catch (OperationCanceledException) + { + return Unknown(request.RequestId, null, "Timeout"); + } + catch (IOException) + { + return Unknown(request.RequestId, null, "BrokerUnavailable"); + } + catch (JsonException) + { + return Unknown(request.RequestId, null, "InvalidResponse"); + } + catch (BrokerResponseException error) + { + return Unknown(request.RequestId, error.StatusCode, "InvalidResponse"); + } + catch (InvalidOperationException) + { + return Unknown(request.RequestId, null, "InvalidResponse"); + } + catch (ProtocolException) + { + return Unknown(request.RequestId, null, "InvalidResponse"); + } + } + + internal static byte[] CreateOfficialRequest(ElevationRequest request) + { + ArrayBufferWriter buffer = new(); + using Utf8JsonWriter writer = new(buffer); + writer.WriteStartObject(); + writer.WriteString("RequestKind", "PolicyReplacementRequest"); + writer.WriteString("RequestVersion", "1.0"); + writer.WriteString("ExpectedStoreToken", request.ExpectedStoreToken); + writer.WriteString("Operation", request.Operation); + writer.WriteString("ConflictHandling", request.ConflictHandling); + writer.WriteBoolean("WarningsAcknowledged", request.WarningsAcknowledged); + writer.WritePropertyName("Draft"); + request.Draft.WriteTo(writer); + writer.WriteString("ValidationReceipt", request.ValidationReceipt); + writer.WriteEndObject(); + writer.Flush(); + if (buffer.WrittenCount > 16_777_216) + { + throw new ProtocolException("official policy request exceeds broker limit"); + } + return buffer.WrittenSpan.ToArray(); + } + + internal static ElevationResponse ParseResponse(string requestId, byte[] response) + { + ReadOnlySpan delimiter = "\r\n\r\n"u8; + int headerEnd = response.AsSpan().IndexOf(delimiter); + if (headerEnd < 0 || headerEnd > MaximumHeaderBytes) + { + throw new BrokerResponseException(null); + } + + string statusLine = Encoding.ASCII.GetString(response.AsSpan(0, headerEnd)).Split("\r\n", 2)[0]; + string[] statusParts = statusLine.Split(' ', 3, StringSplitOptions.RemoveEmptyEntries); + if (statusParts.Length < 2 || !int.TryParse(statusParts[1], out int status)) + { + throw new BrokerResponseException(null); + } + + ReadOnlyMemory body = response.AsMemory(headerEnd + delimiter.Length); + if (body.IsEmpty) + { + return Unknown(requestId, status, "EmptyResponse"); + } + + try + { + return ParseResponseBody(requestId, status, body); + } + catch (Exception error) when (error is JsonException or InvalidOperationException or ProtocolException) + { + throw new BrokerResponseException(status, error); + } + } + + private static ElevationResponse ParseResponseBody( + string requestId, + int status, + ReadOnlyMemory body) + { + using JsonDocument document = JsonDocument.Parse(body); + JsonElement payload = document.RootElement; + if (status is >= 200 and <= 299) + { + RequireString(payload, "ResponseKind", "PolicyReplacementResponse"); + RequireString(payload, "ResponseVersion", "1.0"); + string token = RequireNestedString(payload, "Management", "StoreToken"); + ElevationResponse committed = new( + Protocol.Version, + requestId, + "Committed", + null, + null, + token, + null, + null, + null); + Protocol.ValidateResponse(committed); + return committed; + } + + RequireString(payload, "ResponseKind", "ErrorResponse"); + RequireString(payload, "ResponseVersion", "1.0"); + string code = RequireString(payload, "Code"); + string? conflictToken = null; + string? conflictState = null; + string? conflictPolicyId = null; + if (code == "StalePolicyStoreToken") + { + JsonElement management = RequireObject(payload, "Management"); + conflictToken = RequireString(management, "StoreToken"); + conflictState = RequireString(management, "State"); + if (conflictState == "Active") + { + JsonElement policy = RequireObject(management, "Policy"); + JsonElement metadata = RequireObject(policy, "Metadata"); + conflictPolicyId = RequireString(metadata, "Id"); + } + } + ElevationResponse rejected = new( + Protocol.Version, + requestId, + "Rejected", + status, + Truncate(code, 64), + null, + conflictToken, + conflictState, + conflictPolicyId); + Protocol.ValidateResponse(rejected); + return rejected; + } + + private static void RequireString(JsonElement payload, string property, string expected) + { + if (!payload.TryGetProperty(property, out JsonElement value) || + value.ValueKind != JsonValueKind.String || + value.GetString() != expected) + { + throw new InvalidOperationException("broker response contract mismatch"); + } + } + + private static string RequireString(JsonElement payload, string property) + { + if (!payload.TryGetProperty(property, out JsonElement value) || + value.ValueKind != JsonValueKind.String || + value.GetString() is not { } result) + { + throw new InvalidOperationException("broker response contract mismatch"); + } + return result; + } + + private static string RequireNestedString(JsonElement payload, string parent, string property) => + RequireString(RequireObject(payload, parent), property); + + private static JsonElement RequireObject(JsonElement payload, string property) + { + if (!payload.TryGetProperty(property, out JsonElement value) || + value.ValueKind != JsonValueKind.Object) + { + throw new InvalidOperationException("broker response contract mismatch"); + } + return value; + } + + private static async Task ReadBoundedAsync(Stream stream, int maximum, CancellationToken cancellationToken) + { + using MemoryStream response = new(); + byte[] buffer = new byte[16 * 1024]; + while (true) + { + int read = await stream.ReadAsync(buffer, cancellationToken); + if (read == 0) + { + return response.ToArray(); + } + if (response.Length + read > maximum) + { + throw new InvalidOperationException("broker response exceeds limit"); + } + response.Write(buffer, 0, read); + } + } + + private static ElevationResponse Unknown(string requestId, int? statusCode, string errorCode) => + new(Protocol.Version, requestId, "Unknown", statusCode, errorCode, null, null, null, null); + + private static string Truncate(string value, int maximum) => + value.Length <= maximum ? value : value[..maximum]; + + private sealed class BrokerResponseException(int? statusCode, Exception? innerException = null) + : Exception("broker response contract mismatch", innerException) + { + internal int? StatusCode { get; } = statusCode; + } +} diff --git a/package/AgentPolicyConsent/DevolutionsAgentPolicyConsent.csproj b/package/AgentPolicyConsent/DevolutionsAgentPolicyConsent.csproj new file mode 100644 index 000000000..5e5e414e4 --- /dev/null +++ b/package/AgentPolicyConsent/DevolutionsAgentPolicyConsent.csproj @@ -0,0 +1,23 @@ + + + Exe + net10.0-windows + win-x64 + true + true + true + true + false + enable + enable + true + app.manifest + DevolutionsAgentPolicyConsent + DevolutionsAgentPolicyConsent + Devolutions Agent Policy Consent + Devolutions Inc. + + + + + diff --git a/package/AgentPolicyConsent/PeerTrust.cs b/package/AgentPolicyConsent/PeerTrust.cs new file mode 100644 index 000000000..846d151d1 --- /dev/null +++ b/package/AgentPolicyConsent/PeerTrust.cs @@ -0,0 +1,682 @@ +using Microsoft.Win32.SafeHandles; +using System.ComponentModel; +using System.Diagnostics; +using System.Runtime.InteropServices; +using System.Security.Cryptography; +using System.Security.Cryptography.X509Certificates; +using System.Text; + +namespace DevolutionsAgentPolicyConsent; + +internal sealed class PeerLease : IDisposable +{ + // SHA-256 digests of accepted UniGetUI signer SPKIs. Keep both keys during certificate rollover. + internal const string TransitionUiSignerSpkiSha256 = + PolicyConsentContract.TransitionUiSignerSpkiSha256; + internal const string CurrentUiSignerSpkiSha256 = + PolicyConsentContract.CurrentUiSignerSpkiSha256; + + internal const uint ProcessQueryLimitedInformation = 0x1000; + internal const uint ProcessQueryInformation = 0x0400; + internal const uint Synchronize = 0x0010_0000; + internal const uint GenericRead = 0x8000_0000; + internal const uint FileExecute = 0x20; + internal const uint FileShareRead = 0x1; + internal const uint OpenExisting = 3; + internal const int ProcessImageFileMapping = 44; + internal const uint StillActive = 259; + + private readonly SafeProcessHandle process; + private readonly SafeFileHandle image; + private readonly int processId; + private readonly long createdUtcTicks; + private readonly uint sessionId; + + private PeerLease( + SafeProcessHandle process, + SafeFileHandle image, + int processId, + long createdUtcTicks, + uint sessionId) + { + this.process = process; + this.image = image; + this.processId = processId; + this.createdUtcTicks = createdUtcTicks; + this.sessionId = sessionId; + } + + internal static PeerLease Open(Arguments arguments) + { + SafeProcessHandle process = Native.OpenProcess( + ProcessQueryInformation | ProcessQueryLimitedInformation | Synchronize, + false, + arguments.ParentProcessId); + if (process.IsInvalid) + { + throw new Win32Exception(); + } + + try + { + long created = CreationTime(process); + uint session = SessionId(arguments.ParentProcessId); + if (!MatchesProcessIdentity( + arguments, + arguments.ParentProcessId, + created, + session)) + { + throw new InvalidOperationException("parent process identity mismatch"); + } + + string path = ImagePath(process); + SafeFileHandle image = Native.CreateFile( + path, + GenericRead | FileExecute | Synchronize, + FileShareRead, + IntPtr.Zero, + OpenExisting, + 0, + IntPtr.Zero); + if (image.IsInvalid) + { + throw new Win32Exception(); + } + + try + { + VerifyImageMapping(process, image); + VerifyImageMetadata(path, image); + using X509Certificate2 signer = VerifyAuthenticodeSigner(path, image, "parent image"); + VerifySigner(signer); + VerifyImageMapping(process, image); + EnsureActive(process); + return new PeerLease(process, image, arguments.ParentProcessId, created, session); + } + catch + { + image.Dispose(); + throw; + } + } + catch + { + process.Dispose(); + throw; + } + } + + internal void VerifyConnectedServer(int serverProcessId) + { + Arguments expected = new(string.Empty, processId, createdUtcTicks, sessionId); + if (!MatchesProcessIdentity(expected, serverProcessId, CreationTime(process), SessionId(serverProcessId))) + { + throw new InvalidOperationException("connected server process identity mismatch"); + } + VerifyImageMapping(process, image); + EnsureActive(process); + } + + internal static bool IsAllowedSigner(string digest) => + FixedTimeEqualsHex(digest, CurrentUiSignerSpkiSha256) || + FixedTimeEqualsHex(digest, TransitionUiSignerSpkiSha256); + + internal static bool IsSupportedUiIdentity(string? productName, string? originalFilename, string? productVersion) => + string.Equals(productName, "UniGetUI", StringComparison.Ordinal) && + string.Equals(originalFilename, "UniGetUI.dll", StringComparison.OrdinalIgnoreCase) && + productVersion is not null && + Version.TryParse(productVersion.Split('+', '-', StringSplitOptions.TrimEntries)[0], out Version? parsed) && + parsed >= new Version(3, 3, 7); + + internal static bool MatchesProcessIdentity( + Arguments expected, + int processId, + long createdUtcTicks, + uint sessionId) => + processId == expected.ParentProcessId && + createdUtcTicks == expected.ParentCreatedUtcTicks && + sessionId == expected.SessionId; + + public void Dispose() + { + image.Dispose(); + process.Dispose(); + } + + private static void VerifyImageMetadata(string path, SafeFileHandle retainedImage) + { + VerifyPathIdentity(path, retainedImage); + if ((File.GetAttributes(path) & FileAttributes.ReparsePoint) != 0) + { + throw new InvalidOperationException("parent image is a reparse point"); + } + + FileVersionInfo version = FileVersionInfo.GetVersionInfo(path); + if (!IsSupportedUiIdentity(version.ProductName, version.OriginalFilename, version.ProductVersion)) + { + throw new InvalidOperationException("parent image product identity mismatch"); + } + VerifyPathIdentity(path, retainedImage); + } + + private static void VerifyPathIdentity(string path, SafeFileHandle retainedImage) + { + using SafeFileHandle reopened = Native.CreateFile( + path, + GenericRead | FileExecute | Synchronize, + FileShareRead, + IntPtr.Zero, + OpenExisting, + 0, + IntPtr.Zero); + if (reopened.IsInvalid) + { + throw new Win32Exception(); + } + if (!SameFile(retainedImage, reopened)) + { + throw new InvalidOperationException("parent image path no longer identifies the retained image"); + } + } + + internal static bool SameFile(SafeFileHandle left, SafeFileHandle right) + { + if (!Native.GetFileInformationByHandle(left, out Native.ByHandleFileInformation leftInfo) || + !Native.GetFileInformationByHandle(right, out Native.ByHandleFileInformation rightInfo)) + { + throw new Win32Exception(); + } + return leftInfo.VolumeSerialNumber == rightInfo.VolumeSerialNumber && + leftInfo.FileIndexHigh == rightInfo.FileIndexHigh && + leftInfo.FileIndexLow == rightInfo.FileIndexLow; + } + + internal static bool IsLocalSystemProcess(SafeProcessHandle process) + { + if (!Native.OpenProcessToken(process, 0x0008, out SafeAccessTokenHandle token)) + { + throw new Win32Exception(); + } + using (token) + { + _ = Native.GetTokenInformation(token, 1, IntPtr.Zero, 0, out uint length); + if (length == 0) + { + throw new Win32Exception(); + } + + IntPtr information = Marshal.AllocHGlobal(checked((int)length)); + try + { + if (!Native.GetTokenInformation(token, 1, information, length, out _)) + { + throw new Win32Exception(); + } + IntPtr sid = Marshal.ReadIntPtr(information); + return Native.IsWellKnownSid(sid, 22); + } + finally + { + Marshal.FreeHGlobal(information); + } + } + } + + internal static X509Certificate2 VerifyAuthenticodeSigner( + string path, + SafeFileHandle image, + string subject) + { + Guid action = new("00AAC56B-CD44-11d0-8CC2-00C04FC295EE"); + Native.WinTrustFileInfo file = new(path, image.DangerousGetHandle()); + IntPtr filePointer = Marshal.AllocHGlobal(Marshal.SizeOf()); + IntPtr dataPointer = Marshal.AllocHGlobal(Marshal.SizeOf()); + bool fileInitialized = false; + bool dataInitialized = false; + try + { + Marshal.StructureToPtr(file, filePointer, false); + fileInitialized = true; + Native.WinTrustData data = new(filePointer); + Marshal.StructureToPtr(data, dataPointer, false); + dataInitialized = true; + int status = Native.WinVerifyTrust(new IntPtr(-1), ref action, dataPointer); + if (status != 0) + { + throw new InvalidOperationException($"{subject} Authenticode validation failed (0x{status:X8})"); + } + data = Marshal.PtrToStructure(dataPointer); + IntPtr providerData = Native.WTHelperProvDataFromStateData(data.StateData); + IntPtr providerSigner = providerData == IntPtr.Zero + ? IntPtr.Zero + : Native.WTHelperGetProvSignerFromChain(providerData, 0, false, 0); + if (providerSigner == IntPtr.Zero) + { + throw new InvalidOperationException($"{subject} Authenticode signer is unavailable"); + } + + Native.CryptProviderSigner signer = Marshal.PtrToStructure(providerSigner); + if (signer.CertificateChainCount == 0 || signer.CertificateChain == IntPtr.Zero) + { + throw new InvalidOperationException($"{subject} Authenticode certificate chain is empty"); + } + Native.CryptProviderCertificate certificate = + Marshal.PtrToStructure(signer.CertificateChain); +#pragma warning disable SYSLIB0057 // WinVerifyTrust returns the certificate context for the exact retained image. + return new X509Certificate2(certificate.CertificateContext); +#pragma warning restore SYSLIB0057 + } + finally + { + if (dataInitialized) + { + Native.WinTrustData data = Marshal.PtrToStructure(dataPointer); + if (data.StateData != IntPtr.Zero) + { + data.StateAction = 2; + Marshal.StructureToPtr(data, dataPointer, true); + _ = Native.WinVerifyTrust(new IntPtr(-1), ref action, dataPointer); + } + Marshal.DestroyStructure(dataPointer); + } + if (fileInitialized) + { + Marshal.DestroyStructure(filePointer); + } + Marshal.FreeHGlobal(dataPointer); + Marshal.FreeHGlobal(filePointer); + } + } + + private static void VerifySigner(X509Certificate2 certificate) + { + byte[] subjectPublicKeyInfo; + using (RSA? rsa = certificate.GetRSAPublicKey()) + { + if (rsa is not null) + { + subjectPublicKeyInfo = rsa.ExportSubjectPublicKeyInfo(); + } + else + { + using ECDsa? ecdsa = certificate.GetECDsaPublicKey(); + subjectPublicKeyInfo = ecdsa?.ExportSubjectPublicKeyInfo() + ?? throw new InvalidOperationException("unsupported parent signer key"); + } + } + + string digest = Convert.ToHexString(SHA256.HashData(subjectPublicKeyInfo)).ToLowerInvariant(); + if (!IsAllowedSigner(digest)) + { + throw new InvalidOperationException("parent image signer is not authorized"); + } + } + + private static bool FixedTimeEqualsHex(string candidate, string expected) + { + if (candidate.Length != 64 || + expected.Length != 64 || + candidate.AsSpan().IndexOfAnyExcept("0123456789abcdef") >= 0) + { + return false; + } + try + { + return CryptographicOperations.FixedTimeEquals( + Convert.FromHexString(candidate), + Convert.FromHexString(expected)); + } + catch (FormatException) + { + return false; + } + } + + internal static void VerifyImageMapping(SafeProcessHandle process, SafeFileHandle image) + { + IntPtr fileHandle = image.DangerousGetHandle(); + int status = Native.NtQueryInformationProcess( + process.DangerousGetHandle(), + ProcessImageFileMapping, + ref fileHandle, + IntPtr.Size, + out _); + if (status != 0) + { + throw new InvalidOperationException($"parent image mapping mismatch (0x{status:X8})"); + } + } + + internal static string ImagePath(SafeProcessHandle process) + { + int capacity = 260; + while (capacity <= 32_768) + { + StringBuilder path = new(capacity); + int length = capacity; + if (Native.QueryFullProcessImageName(process, 0, path, ref length)) + { + return path.ToString(); + } + if (Marshal.GetLastWin32Error() != 122) + { + throw new Win32Exception(); + } + capacity *= 2; + } + throw new InvalidOperationException("parent image path is too long"); + } + + private static long CreationTime(SafeProcessHandle process) + { + if (!Native.GetProcessTimes(process, out long created, out _, out _, out _)) + { + throw new Win32Exception(); + } + return DateTime.FromFileTimeUtc(created).Ticks; + } + + private static uint SessionId(int processId) + { + if (!Native.ProcessIdToSessionId(processId, out uint sessionId)) + { + throw new Win32Exception(); + } + return sessionId; + } + + internal static void EnsureActive(SafeProcessHandle process) + { + if (!Native.GetExitCodeProcess(process, out uint exitCode)) + { + throw new Win32Exception(); + } + if (exitCode != StillActive) + { + throw new InvalidOperationException("parent process exited"); + } + } +} + +internal sealed class BrokerServerLease : IDisposable +{ + private const string AgentExecutableName = "DevolutionsAgent.exe"; + + private readonly SafeProcessHandle process; + private readonly SafeFileHandle image; + + private BrokerServerLease(SafeProcessHandle process, SafeFileHandle image) + { + this.process = process; + this.image = image; + } + + internal static BrokerServerLease Open(SafePipeHandle pipe) + { + if (!Native.GetNamedPipeServerProcessId(pipe, out int processId)) + { + throw new Win32Exception(); + } + + SafeProcessHandle process = Native.OpenProcess( + PeerLease.ProcessQueryLimitedInformation | PeerLease.Synchronize, + false, + processId); + if (process.IsInvalid) + { + throw new Win32Exception(); + } + + try + { + if (!PeerLease.IsLocalSystemProcess(process)) + { + throw new InvalidOperationException("broker server is not running as LocalSystem"); + } + string helperPath = Environment.ProcessPath + ?? throw new InvalidOperationException("helper executable path is unavailable"); + string expectedPath = Path.Combine( + Path.GetDirectoryName(helperPath) + ?? throw new InvalidOperationException("helper installation directory is unavailable"), + AgentExecutableName); + string serverPath = PeerLease.ImagePath(process); + if (!IsExpectedPath(serverPath, expectedPath)) + { + throw new InvalidOperationException("broker server is not the installed Agent"); + } + + SafeFileHandle image = Native.CreateFile( + expectedPath, + PeerLease.GenericRead | PeerLease.FileExecute | PeerLease.Synchronize, + PeerLease.FileShareRead, + IntPtr.Zero, + PeerLease.OpenExisting, + 0, + IntPtr.Zero); + if (image.IsInvalid) + { + throw new Win32Exception(); + } + + try + { + using X509Certificate2 _ = + PeerLease.VerifyAuthenticodeSigner(expectedPath, image, "broker server"); + PeerLease.EnsureActive(process); + return new BrokerServerLease(process, image); + } + catch + { + image.Dispose(); + throw; + } + } + catch + { + process.Dispose(); + throw; + } + } + + internal static bool IsExpectedPath(string actual, string expected) => + string.Equals( + Path.GetFullPath(actual), + Path.GetFullPath(expected), + StringComparison.OrdinalIgnoreCase); + + public void Dispose() + { + image.Dispose(); + process.Dispose(); + } +} + +internal static partial class Native +{ + [StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)] + internal readonly struct WinTrustFileInfo + { + internal readonly uint StructSize; + [MarshalAs(UnmanagedType.LPWStr)] + internal readonly string FilePath; + internal readonly IntPtr FileHandle; + internal readonly IntPtr KnownSubject; + + internal WinTrustFileInfo(string filePath, IntPtr fileHandle) + { + StructSize = checked((uint)Marshal.SizeOf()); + FilePath = filePath; + FileHandle = fileHandle; + KnownSubject = IntPtr.Zero; + } + } + + [StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)] + internal struct WinTrustData + { + internal uint StructSize; + internal IntPtr PolicyCallbackData; + internal IntPtr SipClientData; + internal uint UiChoice; + internal uint RevocationChecks; + internal uint UnionChoice; + internal IntPtr FileInfo; + internal uint StateAction; + internal IntPtr StateData; + internal IntPtr UrlReference; + internal uint ProviderFlags; + internal uint UiContext; + internal IntPtr SignatureSettings; + + internal WinTrustData(IntPtr fileInfo) + { + StructSize = checked((uint)Marshal.SizeOf()); + PolicyCallbackData = IntPtr.Zero; + SipClientData = IntPtr.Zero; + UiChoice = 2; + RevocationChecks = 0; + UnionChoice = 1; + FileInfo = fileInfo; + StateAction = 1; + StateData = IntPtr.Zero; + UrlReference = IntPtr.Zero; + ProviderFlags = 0x1000 | 0x2000; + UiContext = 0; + SignatureSettings = IntPtr.Zero; + } + } + + [StructLayout(LayoutKind.Sequential)] + internal readonly struct CryptProviderSigner + { + internal readonly uint StructSize; + internal readonly NativeFileTime VerifyAsOf; + internal readonly uint CertificateChainCount; + internal readonly IntPtr CertificateChain; + internal readonly uint SignerType; + internal readonly IntPtr SignerInfo; + internal readonly uint Error; + internal readonly uint CounterSignerCount; + internal readonly IntPtr CounterSigners; + internal readonly IntPtr ChainContext; + } + + [StructLayout(LayoutKind.Sequential)] + internal readonly struct NativeFileTime + { + internal readonly uint LowDateTime; + internal readonly uint HighDateTime; + } + + [StructLayout(LayoutKind.Sequential)] + internal readonly struct ByHandleFileInformation + { + internal readonly uint FileAttributes; + internal readonly NativeFileTime CreationTime; + internal readonly NativeFileTime LastAccessTime; + internal readonly NativeFileTime LastWriteTime; + internal readonly uint VolumeSerialNumber; + internal readonly uint FileSizeHigh; + internal readonly uint FileSizeLow; + internal readonly uint NumberOfLinks; + internal readonly uint FileIndexHigh; + internal readonly uint FileIndexLow; + } + + [StructLayout(LayoutKind.Sequential)] + internal readonly struct CryptProviderCertificate + { + internal readonly uint StructSize; + internal readonly IntPtr CertificateContext; + } + + [DllImport("kernel32.dll", SetLastError = true, CharSet = CharSet.Unicode)] + [return: MarshalAs(UnmanagedType.Bool)] + internal static extern bool QueryFullProcessImageName( + SafeProcessHandle process, + uint flags, + [Out] StringBuilder path, + ref int size); + + [LibraryImport("kernel32.dll", SetLastError = true)] + [return: MarshalAs(UnmanagedType.Bool)] + internal static partial bool GetProcessTimes( + SafeProcessHandle process, + out long creation, + out long exit, + out long kernel, + out long user); + + [LibraryImport("kernel32.dll", SetLastError = true)] + [return: MarshalAs(UnmanagedType.Bool)] + internal static partial bool ProcessIdToSessionId(int processId, out uint sessionId); + + [LibraryImport("kernel32.dll", SetLastError = true)] + [return: MarshalAs(UnmanagedType.Bool)] + internal static partial bool GetExitCodeProcess(SafeProcessHandle process, out uint exitCode); + + [LibraryImport("kernel32.dll", SetLastError = true)] + [return: MarshalAs(UnmanagedType.Bool)] + internal static partial bool GetFileInformationByHandle( + SafeFileHandle file, + out ByHandleFileInformation information); + + [LibraryImport("advapi32.dll", SetLastError = true)] + [return: MarshalAs(UnmanagedType.Bool)] + internal static partial bool OpenProcessToken( + SafeProcessHandle process, + uint desiredAccess, + out SafeAccessTokenHandle token); + + [LibraryImport("advapi32.dll", SetLastError = true)] + [return: MarshalAs(UnmanagedType.Bool)] + internal static partial bool GetTokenInformation( + SafeAccessTokenHandle token, + int informationClass, + IntPtr information, + uint informationLength, + out uint returnLength); + + [LibraryImport("advapi32.dll")] + [return: MarshalAs(UnmanagedType.Bool)] + internal static partial bool IsWellKnownSid(IntPtr sid, int wellKnownSidType); + + [LibraryImport("kernel32.dll", EntryPoint = "CreateFileW", SetLastError = true, StringMarshalling = StringMarshalling.Utf16)] + internal static partial SafeFileHandle CreateFile( + string fileName, + uint desiredAccess, + uint shareMode, + IntPtr securityAttributes, + uint creationDisposition, + uint flagsAndAttributes, + IntPtr templateFile); + + [LibraryImport("kernel32.dll", SetLastError = true)] + internal static partial SafeProcessHandle OpenProcess(uint desiredAccess, [MarshalAs(UnmanagedType.Bool)] bool inherit, int processId); + + [LibraryImport("ntdll.dll")] + internal static partial int NtQueryInformationProcess( + IntPtr process, + int informationClass, + ref IntPtr information, + int informationLength, + out int returnLength); + + [LibraryImport("wintrust.dll", SetLastError = true)] + internal static partial int WinVerifyTrust(IntPtr window, ref Guid action, IntPtr data); + + [LibraryImport("wintrust.dll")] + internal static partial IntPtr WTHelperProvDataFromStateData(IntPtr stateData); + + [LibraryImport("wintrust.dll")] + internal static partial IntPtr WTHelperGetProvSignerFromChain( + IntPtr providerData, + uint signerIndex, + [MarshalAs(UnmanagedType.Bool)] bool counterSigner, + uint counterSignerIndex); + + [LibraryImport("kernel32.dll", SetLastError = true)] + [return: MarshalAs(UnmanagedType.Bool)] + internal static partial bool GetNamedPipeServerProcessId(SafePipeHandle pipe, out int processId); +} diff --git a/package/AgentPolicyConsent/PolicyConsentContract.cs b/package/AgentPolicyConsent/PolicyConsentContract.cs new file mode 100644 index 000000000..01789bbf5 --- /dev/null +++ b/package/AgentPolicyConsent/PolicyConsentContract.cs @@ -0,0 +1,13 @@ +namespace DevolutionsAgentPolicyConsent +{ + internal static class PolicyConsentContract + { + internal const string ProtocolVersion = "2.0"; + internal const string ExecutableName = "DevolutionsAgentPolicyConsent.exe"; + internal const string ProductName = "Devolutions Agent Policy Consent"; + internal const string CurrentUiSignerSpkiSha256 = + "e43ed3368eaabff61abc79eb338cba9da88a80d93b751735ff417f26afa579a8"; + internal const string TransitionUiSignerSpkiSha256 = + "99e7adb5894e242d87d32b8ad6cb5a1e0d2dd791a447bd7192c30189ef083fab"; + } +} diff --git a/package/AgentPolicyConsent/Program.cs b/package/AgentPolicyConsent/Program.cs new file mode 100644 index 000000000..2ec726a4f --- /dev/null +++ b/package/AgentPolicyConsent/Program.cs @@ -0,0 +1,173 @@ +using System.IO.Pipes; +using System.Security.Principal; +using System.Text.Json; + +namespace DevolutionsAgentPolicyConsent; + +internal static class Program +{ + private const int Success = 0; + private const int InvalidArguments = 10; + private const int ConnectionFailure = 11; + private const int PeerAuthenticationFailure = 12; + private const int ProtocolFailure = 13; + private const int UnexpectedFailure = 14; + + private static async Task Main(string[] args) + { + if (!OperatingSystem.IsWindows()) + { + return InvalidArguments; + } + + Arguments arguments; + try + { + arguments = Protocol.ParseArguments(args); + } + catch (ProtocolException) + { + return InvalidArguments; + } + + using CancellationTokenSource handshake = new(Protocol.ConnectTimeout); + PeerLease peer; + try + { + peer = await OpenPeerAsync(arguments, handshake.Token); + } + catch (OperationCanceledException) + { + return ConnectionFailure; + } + catch + { + return PeerAuthenticationFailure; + } + + using (peer) + using (NamedPipeClientStream pipe = new( + ".", + arguments.PipeName, + PipeDirection.InOut, + PipeOptions.Asynchronous | PipeOptions.WriteThrough, + TokenImpersonationLevel.Anonymous)) + { + try + { + await pipe.ConnectAsync(handshake.Token); + if (!Native.GetNamedPipeServerProcessId(pipe.SafePipeHandle, out int serverProcessId)) + { + return PeerAuthenticationFailure; + } + peer.VerifyConnectedServer(serverProcessId); + byte[] body = await Protocol.ReadFrameAsync(pipe, Protocol.MaxRequestBodyBytes, handshake.Token); + ElevationRequest request = JsonSerializer.Deserialize(body, ProtocolJsonContext.Default.ElevationRequest) + ?? throw new ProtocolException("request is null"); + Protocol.ValidateRequest(request); + peer.VerifyConnectedServer(serverProcessId); + + using CancellationTokenSource exchange = new(Protocol.ExchangeTimeout); + using CancellationTokenSource brokerCancellation = + CancellationTokenSource.CreateLinkedTokenSource(exchange.Token); + using CancellationTokenSource monitorCancellation = + CancellationTokenSource.CreateLinkedTokenSource(exchange.Token); + Task monitor = MonitorHostAsync(pipe, brokerCancellation, monitorCancellation.Token); + ElevationResponse response = await BrokerClient.ReplaceAsync(request, brokerCancellation.Token); + monitorCancellation.Cancel(); + await IgnoreCancellationAsync(monitor); + Protocol.ValidateResponse(response); + + byte[] responseBody = JsonSerializer.SerializeToUtf8Bytes( + response, + ProtocolJsonContext.Default.ElevationResponse); + using CancellationTokenSource responseWrite = new(Protocol.ResponseWriteTimeout); + await Protocol.WriteFrameAsync( + pipe, + responseBody, + Protocol.MaxResponseBodyBytes, + responseWrite.Token); + return Success; + } + catch (OperationCanceledException) + { + return ConnectionFailure; + } + catch (IOException) + { + return ConnectionFailure; + } + catch (ProtocolException) + { + return ProtocolFailure; + } + catch (JsonException) + { + return ProtocolFailure; + } + catch + { + return UnexpectedFailure; + } + } + } + + private static async Task OpenPeerAsync(Arguments arguments, CancellationToken cancellationToken) + { + Task open = Task.Run(() => PeerLease.Open(arguments)); + try + { + return await open.WaitAsync(cancellationToken); + } + catch + { + _ = open.ContinueWith( + static completed => + { + if (completed.Status == TaskStatus.RanToCompletion) + { + completed.Result.Dispose(); + } + _ = completed.Exception; + }, + CancellationToken.None, + TaskContinuationOptions.ExecuteSynchronously, + TaskScheduler.Default); + throw; + } + } + + private static async Task MonitorHostAsync( + Stream pipe, + CancellationTokenSource brokerCancellation, + CancellationToken cancellationToken) + { + byte[] unexpected = new byte[1]; + try + { + int read = await pipe.ReadAsync(unexpected, cancellationToken); + if (read is 0 or 1) + { + brokerCancellation.Cancel(); + } + } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + } + catch (IOException) + { + brokerCancellation.Cancel(); + } + } + + private static async Task IgnoreCancellationAsync(Task task) + { + try + { + await task; + } + catch (OperationCanceledException) + { + } + } +} diff --git a/package/AgentPolicyConsent/Protocol.cs b/package/AgentPolicyConsent/Protocol.cs new file mode 100644 index 000000000..04184f3dc --- /dev/null +++ b/package/AgentPolicyConsent/Protocol.cs @@ -0,0 +1,240 @@ +using System.Buffers.Binary; +using System.Globalization; +using System.Text.Json; +using System.Text.Json.Serialization; + +namespace DevolutionsAgentPolicyConsent; + +internal static class Protocol +{ + internal const string Version = PolicyConsentContract.ProtocolVersion; + internal const string PipePrefix = "UniGetUI.PolicyElevation."; + internal const int MaxRequestBodyBytes = 16_793_054; + internal const int MaxResponseBodyBytes = 15_618; + internal static readonly TimeSpan ConnectTimeout = TimeSpan.FromSeconds(45); + internal static readonly TimeSpan ExchangeTimeout = TimeSpan.FromMinutes(2); + internal static readonly TimeSpan ResponseWriteTimeout = TimeSpan.FromSeconds(10); + + internal static Arguments ParseArguments(string[] args) + { + if (args.Length != 10) + { + throw new ProtocolException("invalid argument count"); + } + + Dictionary values = new(StringComparer.Ordinal); + for (int index = 0; index < args.Length; index += 2) + { + if (!values.TryAdd(args[index], args[index + 1])) + { + throw new ProtocolException("duplicate argument"); + } + } + + string protocol = Required(values, "--protocol"); + string pipe = Required(values, "--pipe"); + if (protocol != Version || + !pipe.StartsWith(PipePrefix, StringComparison.Ordinal) || + !IsLowerHex(pipe.AsSpan(PipePrefix.Length), 32) || + !int.TryParse( + Required(values, "--parent-pid"), + NumberStyles.None, + CultureInfo.InvariantCulture, + out int parentPid) || + parentPid <= 0 || + !long.TryParse( + Required(values, "--parent-created"), + NumberStyles.None, + CultureInfo.InvariantCulture, + out long parentCreated) || + parentCreated <= 0 || + !uint.TryParse( + Required(values, "--session"), + NumberStyles.None, + CultureInfo.InvariantCulture, + out uint session) || + values.Count != 5) + { + throw new ProtocolException("invalid argument value"); + } + + return new Arguments(pipe, parentPid, parentCreated, session); + } + + internal static void ValidateRequest(ElevationRequest request) + { + if (request.ProtocolVersion != Version || + !IsLowerHex(request.RequestId.AsSpan(), 32) || + request.Operation is not ("Update" or "ReplaceIdentity" or "Create" or "Repair") || + request.ConflictHandling is not ("Reject" or "ConfirmOverwrite") || + !IsCredential(request.ExpectedStoreToken, 512) || + !IsCredential(request.ValidationReceipt, 2048) || + request.Draft.ValueKind != JsonValueKind.Object) + { + throw new ProtocolException("invalid request"); + } + } + + internal static void ValidateResponse(ElevationResponse response) + { + if (response.ProtocolVersion != Version || + !IsLowerHex(response.RequestId.AsSpan(), 32) || + response.Disposition is not ("Committed" or "Rejected" or "Unknown") || + !IsOptionalCredential(response.BrokerErrorCode, 64)) + { + throw new ProtocolException("invalid response"); + } + + bool hasConflict = + response.ConflictStoreToken is not null || + response.ConflictState is not null || + response.ConflictPolicyId is not null; + switch (response.Disposition) + { + case "Committed" when + response.BrokerStatusCode is null && + response.BrokerErrorCode is null && + IsCredential(response.CommittedStoreToken, 512) && + !hasConflict: + return; + case "Rejected" when + response.CommittedStoreToken is null && + response.BrokerErrorCode is not null: + ValidateConflict(response, hasConflict); + return; + case "Unknown" when + response.CommittedStoreToken is null && + response.BrokerErrorCode is not null && + !hasConflict: + return; + default: + throw new ProtocolException("invalid response shape"); + } + } + + internal static async Task ReadFrameAsync(Stream stream, int maximum, CancellationToken cancellationToken) + { + byte[] header = new byte[4]; + await ReadExactlyAsync(stream, header, cancellationToken); + uint length = BinaryPrimitives.ReadUInt32BigEndian(header); + if (length == 0 || length > maximum) + { + throw new ProtocolException("invalid frame length"); + } + + byte[] body = GC.AllocateUninitializedArray(checked((int)length)); + await ReadExactlyAsync(stream, body, cancellationToken); + return body; + } + + internal static async Task WriteFrameAsync( + Stream stream, + ReadOnlyMemory body, + int maximum, + CancellationToken cancellationToken) + { + if (body.IsEmpty || body.Length > maximum) + { + throw new ProtocolException("invalid frame length"); + } + + byte[] header = new byte[4]; + BinaryPrimitives.WriteUInt32BigEndian(header, checked((uint)body.Length)); + await stream.WriteAsync(header, cancellationToken); + await stream.WriteAsync(body, cancellationToken); + await stream.FlushAsync(cancellationToken); + } + + private static async Task ReadExactlyAsync(Stream stream, Memory buffer, CancellationToken cancellationToken) + { + int offset = 0; + while (offset < buffer.Length) + { + int read = await stream.ReadAsync(buffer[offset..], cancellationToken); + if (read == 0) + { + throw new EndOfStreamException("unexpected end of elevation frame"); + } + offset += read; + } + } + + private static string Required(Dictionary values, string key) => + values.TryGetValue(key, out string? value) && value.Length != 0 + ? value + : throw new ProtocolException("missing argument"); + + private static bool IsLowerHex(ReadOnlySpan value, int length) => + value.Length == length && value.IndexOfAnyExcept("0123456789abcdef") < 0; + + internal static bool IsCredential(string? value, int maximum) => + value is not null && + value.Length is > 0 && + value.Length <= maximum && + IsAsciiAlphaNumeric(value[0]) && + value.AsSpan(1).IndexOfAnyExceptInRange('!', '~') < 0; + + private static bool IsOptionalCredential(string? value, int maximum) => + value is null || IsCredential(value, maximum); + + private static void ValidateConflict(ElevationResponse response, bool hasConflict) + { + if (response.BrokerErrorCode != "StalePolicyStoreToken") + { + if (hasConflict) + { + throw new ProtocolException("non-stale response carries conflict fields"); + } + return; + } + + if (!IsCredential(response.ConflictStoreToken, 512) || + response.ConflictState is not ("Active" or "Missing" or "Invalid") || + (response.ConflictState == "Active" + ? !IsCredential(response.ConflictPolicyId, 2048) + : response.ConflictPolicyId is not null)) + { + throw new ProtocolException("invalid stale conflict"); + } + } + + private static bool IsAsciiAlphaNumeric(char value) => + value is >= '0' and <= '9' or >= 'A' and <= 'Z' or >= 'a' and <= 'z'; +} + +internal sealed record Arguments(string PipeName, int ParentProcessId, long ParentCreatedUtcTicks, uint SessionId); + +[JsonUnmappedMemberHandling(JsonUnmappedMemberHandling.Disallow)] +internal sealed record ElevationRequest( + [property: JsonRequired] string ProtocolVersion, + [property: JsonRequired] string RequestId, + [property: JsonRequired] string Operation, + [property: JsonRequired] string ConflictHandling, + [property: JsonRequired] string ExpectedStoreToken, + [property: JsonRequired] string ValidationReceipt, + [property: JsonRequired] bool WarningsAcknowledged, + [property: JsonRequired] JsonElement Draft); + +[JsonUnmappedMemberHandling(JsonUnmappedMemberHandling.Disallow)] +internal sealed record ElevationResponse( + [property: JsonRequired] string ProtocolVersion, + [property: JsonRequired] string RequestId, + [property: JsonRequired] string Disposition, + [property: JsonRequired] int? BrokerStatusCode, + [property: JsonRequired] string? BrokerErrorCode, + [property: JsonRequired] string? CommittedStoreToken, + [property: JsonRequired] string? ConflictStoreToken, + [property: JsonRequired] string? ConflictState, + [property: JsonRequired] string? ConflictPolicyId); + +[JsonSourceGenerationOptions( + PropertyNamingPolicy = JsonKnownNamingPolicy.CamelCase, + PropertyNameCaseInsensitive = false, + UnmappedMemberHandling = JsonUnmappedMemberHandling.Disallow, + DefaultIgnoreCondition = JsonIgnoreCondition.Never, + GenerationMode = JsonSourceGenerationMode.Metadata)] +[JsonSerializable(typeof(ElevationRequest))] +[JsonSerializable(typeof(ElevationResponse))] +internal sealed partial class ProtocolJsonContext : JsonSerializerContext; + +internal sealed class ProtocolException(string message) : Exception(message); diff --git a/package/AgentPolicyConsent/app.manifest b/package/AgentPolicyConsent/app.manifest new file mode 100644 index 000000000..d303ea813 --- /dev/null +++ b/package/AgentPolicyConsent/app.manifest @@ -0,0 +1,11 @@ + + + + + + + + + + + diff --git a/package/AgentWindowsManaged.Tests/PolicyConsentDiscoveryTests.cs b/package/AgentWindowsManaged.Tests/PolicyConsentDiscoveryTests.cs new file mode 100644 index 000000000..a17e10bef --- /dev/null +++ b/package/AgentWindowsManaged.Tests/PolicyConsentDiscoveryTests.cs @@ -0,0 +1,45 @@ +using System; +using System.Reflection; + +using WixSharp; + +using Xunit; + +namespace DevolutionsAgent.Installer.Tests; + +public sealed class PolicyConsentDiscoveryTests +{ + [Theory] + [InlineData(false)] + [InlineData(true)] + public void DiscoveryIsTransactionalAndArchitectureCorrect(bool win64) + { + RegValue value = CreateDiscoveryValue("ProtocolVersion", "2.0", win64); + + Assert.Equal(RegistryHive.LocalMachine, value.Root); + Assert.Equal(@"Software\Devolutions\Agent\PolicyConsentHelper", value.Key); + Assert.Equal(RegistryKeyAction.createAndRemoveOnUninstall, value.RegistryKeyAction); + Assert.Equal(win64, value.Win64); + } + + [Fact] + public void DiscoveryPublishesTheFixedHelperIdentity() + { + RegValue value = CreateDiscoveryValue( + "ExecutablePath", + "[INSTALLDIR]DevolutionsAgentPolicyConsent.exe", + true); + + Assert.Equal("[INSTALLDIR]DevolutionsAgentPolicyConsent.exe", value.Value); + Assert.Equal(RegistryKeyAction.createAndRemoveOnUninstall, value.RegistryKeyAction); + } + + private static RegValue CreateDiscoveryValue(string name, string value, bool win64) + { + Type program = Assembly.Load("DevolutionsAgent").GetType("DevolutionsAgent.Program", throwOnError: true); + MethodInfo method = program.GetMethod( + "CreatePolicyConsentRegistryValue", + BindingFlags.Static | BindingFlags.NonPublic); + return Assert.IsType(method.Invoke(null, [name, value, win64])); + } +} diff --git a/package/AgentWindowsManaged/DevolutionsAgent.csproj b/package/AgentWindowsManaged/DevolutionsAgent.csproj index 527dd4bd4..6df8d0103 100644 --- a/package/AgentWindowsManaged/DevolutionsAgent.csproj +++ b/package/AgentWindowsManaged/DevolutionsAgent.csproj @@ -6,6 +6,7 @@ latest + diff --git a/package/AgentWindowsManaged/Program.cs b/package/AgentWindowsManaged/Program.cs index 09a32b242..83ded8e51 100644 --- a/package/AgentWindowsManaged/Program.cs +++ b/package/AgentWindowsManaged/Program.cs @@ -77,6 +77,10 @@ private static string ResolveArtifact(string varName, string defaultPath = null) private static string DevolutionsAgentExePath => ResolveArtifact("DAGENT_EXECUTABLE", "..\\..\\target\\debug\\devolutions-agent.exe"); + private static string DevolutionsAgentPolicyConsentPath => ResolveArtifact( + "DAGENT_POLICY_CONSENT_HELPER", + "..\\AgentPolicyConsent\\bin\\Release\\net10.0-windows\\win-x64\\publish\\DevolutionsAgentPolicyConsent.exe"); + private static string DevolutionsDesktopAgentPath { // ReSharper disable once ArrangeAccessorOwnerBody @@ -123,6 +127,12 @@ private static Version DevolutionsAgentVersion } } + // The MSI version drops the "20" prefix; discovery restores the calendar-year product version. + private static Version DevolutionsAgentProductVersion => new( + DevolutionsAgentVersion.Major + 2000, + DevolutionsAgentVersion.Minor, + DevolutionsAgentVersion.Build); + private static WixSharp.Platform TargetPlatform { get @@ -298,6 +308,10 @@ static void Main() new (Features.AGENT_FEATURE, DevolutionsMultiPwshExe) { TargetFileName = "multi-pwsh.exe" + }, + new (Features.AGENT_FEATURE, DevolutionsAgentPolicyConsentPath) + { + TargetFileName = Includes.POLICY_CONSENT_EXECUTABLE_NAME } }, Dirs = new[] @@ -325,6 +339,30 @@ static void Main() Win64 = project.Platform == Platform.x64, RegistryKeyAction = RegistryKeyAction.create, }, + CreatePolicyConsentRegistryValue( + "ProtocolVersion", + Includes.POLICY_CONSENT_PROTOCOL_VERSION, + Use64BitRegistryView(project.Platform)), + CreatePolicyConsentRegistryValue( + "ExecutableName", + Includes.POLICY_CONSENT_EXECUTABLE_NAME, + Use64BitRegistryView(project.Platform)), + CreatePolicyConsentRegistryValue( + "ExecutablePath", + $"[{AgentProperties.InstallDir}]{Includes.POLICY_CONSENT_EXECUTABLE_NAME}", + Use64BitRegistryView(project.Platform)), + CreatePolicyConsentRegistryValue( + "ProductName", + Includes.POLICY_CONSENT_PRODUCT_NAME, + Use64BitRegistryView(project.Platform)), + CreatePolicyConsentRegistryValue( + "ProductVersion", + DevolutionsAgentProductVersion.ToString(), + Use64BitRegistryView(project.Platform)), + CreatePolicyConsentRegistryValue( + "CurrentUiSignerSpkiSha256", + Includes.POLICY_CONSENT_CURRENT_UI_SIGNER_SPKI_SHA256, + Use64BitRegistryView(project.Platform)), new (RegistryHive.LocalMachine, "SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\Run", Includes.SERVICE_NAME, $"[{AgentProperties.InstallDir}]{Includes.DESKTOP_DIRECTORY_NAME}\\{Includes.DESKTOP_EXECUTABLE_NAME}") { Win64 = project.Platform == Platform.x64, @@ -435,6 +473,23 @@ internal static RegValue CreateEventLogSourceRegistryValue(bool win64) => RegistryKeyAction = RegistryKeyAction.createAndRemoveOnUninstall, }; + internal static RegValue CreatePolicyConsentRegistryValue(string name, string value, bool win64) => + new( + RegistryHive.LocalMachine, + $"Software\\{Includes.VENDOR_NAME}\\{Includes.SHORT_NAME}\\PolicyConsentHelper", + name, + value) + { + AttributesDefinition = "Type=string", + Win64 = win64, + RegistryKeyAction = RegistryKeyAction.createAndRemoveOnUninstall, + Feature = Features.AGENT_FEATURE, + }; + + // Discovery follows the consumer's native registry view. + internal static bool Use64BitRegistryView(Platform? platform) => + platform is Platform.x64 or Platform.arm64; + 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..284f8bef2 100644 --- a/package/AgentWindowsManaged/Resources/Includes.cs +++ b/package/AgentWindowsManaged/Resources/Includes.cs @@ -18,6 +18,18 @@ internal static class Includes internal static readonly string EXECUTABLE_NAME = "DevolutionsAgent.exe"; + internal static readonly string POLICY_CONSENT_EXECUTABLE_NAME = + DevolutionsAgentPolicyConsent.PolicyConsentContract.ExecutableName; + + internal static readonly string POLICY_CONSENT_PRODUCT_NAME = + DevolutionsAgentPolicyConsent.PolicyConsentContract.ProductName; + + internal static readonly string POLICY_CONSENT_PROTOCOL_VERSION = + DevolutionsAgentPolicyConsent.PolicyConsentContract.ProtocolVersion; + + internal static readonly string POLICY_CONSENT_CURRENT_UI_SIGNER_SPKI_SHA256 = + DevolutionsAgentPolicyConsent.PolicyConsentContract.CurrentUiSignerSpkiSha256; + internal static readonly string DESKTOP_DIRECTORY_NAME = "desktop"; internal static readonly string DESKTOP_EXECUTABLE_NAME = "DevolutionsDesktopAgent.exe"; From 089c76bf9563b8cd44395937cfa17543a0468eb5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Wed, 9 Sep 2026 16:10:01 -0400 Subject: [PATCH 02/25] fix(agent): enforce signer revocation checks Require fresh whole-chain WinTrust revocation status before authorizing the UniGetUI parent or Agent broker. Bound broker trust retrieval to the existing exchange timeout and reject all pre-dispatch authentication failures. Issue: #1963 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- ci/package-agent-windows.ps1 | 2 +- .../AgentPolicyConsent.Tests/ProtocolTests.cs | 23 ++++++++++ package/AgentPolicyConsent/BrokerClient.cs | 44 ++++++++++++++++++- package/AgentPolicyConsent/PeerTrust.cs | 13 ++++-- 4 files changed, 77 insertions(+), 5 deletions(-) diff --git a/ci/package-agent-windows.ps1 b/ci/package-agent-windows.ps1 index 10f0e7219..246cfe245 100644 --- a/ci/package-agent-windows.ps1 +++ b/ci/package-agent-windows.ps1 @@ -100,7 +100,7 @@ function New-AgentMsi() { # The path to the devolutions-session.exe file. [string] $SessionExe, [parameter(Mandatory = $true)] - # The path to the DevolutionsAgentPolicyConsent.exe file. + # The path to the signed DevolutionsAgentPolicyConsent.exe file. [string] $PolicyConsentHelper, [parameter(Mandatory = $true)] [ValidateSet('x64', 'arm64')] diff --git a/package/AgentPolicyConsent.Tests/ProtocolTests.cs b/package/AgentPolicyConsent.Tests/ProtocolTests.cs index 36456f288..e5e0a21ca 100644 --- a/package/AgentPolicyConsent.Tests/ProtocolTests.cs +++ b/package/AgentPolicyConsent.Tests/ProtocolTests.cs @@ -101,6 +101,29 @@ public void AuthenticodeSignerComesFromRetainedImageHandle() Assert.NotEmpty(certificate.RawData); } + [Fact] + public void AuthenticodeRequiresFreshWholeChainRevocation() + { + Native.WinTrustData data = new(IntPtr.Zero); + + Assert.Equal(Native.WtdRevokeWholeChain, data.RevocationChecks); + Assert.Equal( + Native.WtdRevocationCheckChain | Native.WtdDisableMd2Md4, + data.ProviderFlags); + Assert.Equal(0u, data.ProviderFlags & Native.WtdCacheOnlyUrlRetrieval); + } + + [Theory] + [InlineData(0, true)] + [InlineData(unchecked((int)0x800B010C), false)] + [InlineData(unchecked((int)0x80092012), false)] + [InlineData(unchecked((int)0x80092013), false)] + [InlineData(unchecked((int)0x800B010E), false)] + public void AuthenticodeFailsClosedForIndeterminateStatus(int status, bool accepted) + { + Assert.Equal(accepted, PeerLease.IsAuthenticodeStatusAccepted(status)); + } + [Fact] public void ProcessImageMappingAcceptsOnlyCurrentMappedImage() { diff --git a/package/AgentPolicyConsent/BrokerClient.cs b/package/AgentPolicyConsent/BrokerClient.cs index bdcbd5120..4a1d5affc 100644 --- a/package/AgentPolicyConsent/BrokerClient.cs +++ b/package/AgentPolicyConsent/BrokerClient.cs @@ -29,7 +29,7 @@ internal static async Task ReplaceAsync( using CancellationTokenSource connectTimeout = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken); connectTimeout.CancelAfter(ConnectTimeout); await pipe.ConnectAsync(connectTimeout.Token); - using BrokerServerLease broker = BrokerServerLease.Open(pipe.SafePipeHandle); + using BrokerServerLease broker = await OpenBrokerServerAsync(pipe, cancellationToken); byte[] headers = Encoding.ASCII.GetBytes( $"PUT /v1/policy HTTP/1.1\r\nHost: now-package-broker\r\nConnection: close\r\n" + @@ -60,6 +60,10 @@ internal static async Task ReplaceAsync( { return Unknown(request.RequestId, error.StatusCode, "InvalidResponse"); } + catch (BrokerAuthenticationException) + { + return Rejected(request.RequestId, "Unauthorized"); + } catch (InvalidOperationException) { return Unknown(request.RequestId, null, "InvalidResponse"); @@ -239,6 +243,41 @@ private static async Task ReadBoundedAsync(Stream stream, int maximum, C private static ElevationResponse Unknown(string requestId, int? statusCode, string errorCode) => new(Protocol.Version, requestId, "Unknown", statusCode, errorCode, null, null, null, null); + private static ElevationResponse Rejected(string requestId, string errorCode) => + new(Protocol.Version, requestId, "Rejected", null, errorCode, null, null, null, null); + + private static async Task OpenBrokerServerAsync( + NamedPipeClientStream pipe, + CancellationToken cancellationToken) + { + Task open = Task.Run(() => BrokerServerLease.Open(pipe.SafePipeHandle)); + try + { + return await open.WaitAsync(cancellationToken); + } + catch (Exception error) + { + DisposeLateResult(open); + throw new BrokerAuthenticationException(error); + } + } + + private static void DisposeLateResult(Task open) + { + _ = open.ContinueWith( + static completed => + { + if (completed.Status == TaskStatus.RanToCompletion) + { + completed.Result.Dispose(); + } + _ = completed.Exception; + }, + CancellationToken.None, + TaskContinuationOptions.ExecuteSynchronously, + TaskScheduler.Default); + } + private static string Truncate(string value, int maximum) => value.Length <= maximum ? value : value[..maximum]; @@ -247,4 +286,7 @@ private sealed class BrokerResponseException(int? statusCode, Exception? innerEx { internal int? StatusCode { get; } = statusCode; } + + private sealed class BrokerAuthenticationException(Exception innerException) + : Exception("broker server authentication failed", innerException); } diff --git a/package/AgentPolicyConsent/PeerTrust.cs b/package/AgentPolicyConsent/PeerTrust.cs index 846d151d1..502742540 100644 --- a/package/AgentPolicyConsent/PeerTrust.cs +++ b/package/AgentPolicyConsent/PeerTrust.cs @@ -242,7 +242,7 @@ internal static X509Certificate2 VerifyAuthenticodeSigner( Marshal.StructureToPtr(data, dataPointer, false); dataInitialized = true; int status = Native.WinVerifyTrust(new IntPtr(-1), ref action, dataPointer); - if (status != 0) + if (!IsAuthenticodeStatusAccepted(status)) { throw new InvalidOperationException($"{subject} Authenticode validation failed (0x{status:X8})"); } @@ -289,6 +289,8 @@ internal static X509Certificate2 VerifyAuthenticodeSigner( } } + internal static bool IsAuthenticodeStatusAccepted(int status) => status == 0; + private static void VerifySigner(X509Certificate2 certificate) { byte[] subjectPublicKeyInfo; @@ -494,6 +496,11 @@ public void Dispose() internal static partial class Native { + internal const uint WtdRevokeWholeChain = 1; + internal const uint WtdRevocationCheckChain = 0x0000_0040; + internal const uint WtdCacheOnlyUrlRetrieval = 0x0000_1000; + internal const uint WtdDisableMd2Md4 = 0x0000_2000; + [StructLayout(LayoutKind.Sequential, CharSet = CharSet.Unicode)] internal readonly struct WinTrustFileInfo { @@ -535,13 +542,13 @@ internal WinTrustData(IntPtr fileInfo) PolicyCallbackData = IntPtr.Zero; SipClientData = IntPtr.Zero; UiChoice = 2; - RevocationChecks = 0; + RevocationChecks = WtdRevokeWholeChain; UnionChoice = 1; FileInfo = fileInfo; StateAction = 1; StateData = IntPtr.Zero; UrlReference = IntPtr.Zero; - ProviderFlags = 0x1000 | 0x2000; + ProviderFlags = WtdRevocationCheckChain | WtdDisableMd2Md4; UiContext = 0; SignatureSettings = IntPtr.Zero; } From 2ace003af74ec93072fc8c0b5f908282121beffb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Thu, 10 Sep 2026 00:57:16 -0400 Subject: [PATCH 03/25] fix(agent): bind broker trust to running image Authenticate the connected Agent through its retained mapped image and an allowed Devolutions signer before sending policy data. Pin the protected installation directory chain against reparse and untrusted-writer replacement, including custom install locations, and record the evidence behind both UniGetUI signer pins. Issue: #1963 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../AgentPolicyConsent.Tests/ProtocolTests.cs | 44 ++++ package/AgentPolicyConsent/PeerTrust.cs | 236 +++++++++++++++++- .../PolicyConsentContract.cs | 20 ++ 3 files changed, 288 insertions(+), 12 deletions(-) diff --git a/package/AgentPolicyConsent.Tests/ProtocolTests.cs b/package/AgentPolicyConsent.Tests/ProtocolTests.cs index e5e0a21ca..07a87266e 100644 --- a/package/AgentPolicyConsent.Tests/ProtocolTests.cs +++ b/package/AgentPolicyConsent.Tests/ProtocolTests.cs @@ -1,4 +1,5 @@ using System.Buffers.Binary; +using System.Security.AccessControl; using System.Text.Json; using DevolutionsAgentPolicyConsent; using Microsoft.Win32.SafeHandles; @@ -41,6 +42,46 @@ public void LookalikeSignerIsRejected() Assert.False(PeerLease.IsAllowedSigner(new string('0', 64))); } + [Fact] + public void AgentSignerRequiresKnownDevolutionsCertificate() + { + Assert.All( + PolicyConsentContract.DevolutionsSignerSha1Thumbprints, + thumbprint => Assert.True(PeerLease.IsAllowedDevolutionsSigner(thumbprint))); + Assert.False(PeerLease.IsAllowedDevolutionsSigner(new string('0', 40))); + } + + [Theory] + [InlineData("O:SYG:SYD:(A;;FA;;;SY)(A;;FA;;;BA)(A;;0x1200A9;;;BU)", PeerLease.FileTamperRights, true)] + [InlineData("O:BUG:SYD:(A;;FA;;;SY)(A;;FA;;;BA)", PeerLease.FileTamperRights, false)] + [InlineData("O:SYG:SYD:(A;;FA;;;SY)(A;;FA;;;BA)(A;;GW;;;BU)", PeerLease.FileTamperRights, false)] + [InlineData("O:SYG:SYD:(A;;FA;;;SY)(A;;FA;;;BA)(A;;0x6;;;AU)", PeerLease.ParentDirectoryTamperRights, false)] + [InlineData("O:SYG:SYD:(A;;FA;;;SY)(A;;FA;;;BA)(A;;0x6;;;AU)", PeerLease.AncestorDirectoryTamperRights, true)] + [InlineData("O:SYG:SYD:(A;;FA;;;SY)(A;;FA;;;BA)(A;;0x40;;;BU)", PeerLease.AncestorDirectoryTamperRights, false)] + public void ProtectedAgentPathRequiresTrustedOwnerAndWriters( + string sddl, + int tamperRights, + bool accepted) + { + RawSecurityDescriptor descriptor = new(sddl); + if (accepted) + { + PeerLease.VerifyTrustedSecurityDescriptor(descriptor, "test path", tamperRights); + } + else + { + Assert.Throws( + () => PeerLease.VerifyTrustedSecurityDescriptor(descriptor, "test path", tamperRights)); + } + } + + [Fact] + public void ProtectedAgentPathRejectsReparsePoints() + { + Assert.True(PeerLease.IsReparsePoint(PeerLease.FileAttributeReparsePoint)); + Assert.False(PeerLease.IsReparsePoint(0)); + } + [Fact] public void SignerMatchingIsCaseSensitive() { @@ -90,6 +131,9 @@ public void BrokerServerRequiresExactAgentSiblingPath() Assert.False(BrokerServerLease.IsExpectedPath( @"C:\Users\Alice\DevolutionsAgent.exe", @"C:\Program Files\Devolutions\Agent\DevolutionsAgent.exe")); + Assert.True(BrokerServerLease.IsExpectedPath( + @"D:\Managed Apps\Agent\DevolutionsAgent.exe", + @"d:\managed apps\Agent\DevolutionsAgent.exe")); } [Fact] diff --git a/package/AgentPolicyConsent/PeerTrust.cs b/package/AgentPolicyConsent/PeerTrust.cs index 502742540..9fc0eb204 100644 --- a/package/AgentPolicyConsent/PeerTrust.cs +++ b/package/AgentPolicyConsent/PeerTrust.cs @@ -2,8 +2,10 @@ using System.ComponentModel; using System.Diagnostics; using System.Runtime.InteropServices; +using System.Security.AccessControl; using System.Security.Cryptography; using System.Security.Cryptography.X509Certificates; +using System.Security.Principal; using System.Text; namespace DevolutionsAgentPolicyConsent; @@ -21,8 +23,14 @@ internal sealed class PeerLease : IDisposable internal const uint Synchronize = 0x0010_0000; internal const uint GenericRead = 0x8000_0000; internal const uint FileExecute = 0x20; + internal const uint FileReadAttributes = 0x80; + internal const uint ReadControl = 0x0002_0000; internal const uint FileShareRead = 0x1; + internal const uint FileShareWrite = 0x2; internal const uint OpenExisting = 3; + internal const uint FileAttributeReparsePoint = 0x400; + internal const uint FileFlagBackupSemantics = 0x0200_0000; + internal const uint FileFlagOpenReparsePoint = 0x0020_0000; internal const int ProcessImageFileMapping = 44; internal const uint StillActive = 259; @@ -122,6 +130,10 @@ internal static bool IsAllowedSigner(string digest) => FixedTimeEqualsHex(digest, CurrentUiSignerSpkiSha256) || FixedTimeEqualsHex(digest, TransitionUiSignerSpkiSha256); + internal static bool IsAllowedDevolutionsSigner(string thumbprint) => + PolicyConsentContract.DevolutionsSignerSha1Thumbprints.Any( + expected => FixedTimeEqualsHex(thumbprint, expected, 40)); + internal static bool IsSupportedUiIdentity(string? productName, string? originalFilename, string? productVersion) => string.Equals(productName, "UniGetUI", StringComparison.Ordinal) && string.Equals(originalFilename, "UniGetUI.dll", StringComparison.OrdinalIgnoreCase) && @@ -315,10 +327,10 @@ private static void VerifySigner(X509Certificate2 certificate) } } - private static bool FixedTimeEqualsHex(string candidate, string expected) + private static bool FixedTimeEqualsHex(string candidate, string expected, int length = 64) { - if (candidate.Length != 64 || - expected.Length != 64 || + if (candidate.Length != length || + expected.Length != length || candidate.AsSpan().IndexOfAnyExcept("0123456789abcdef") >= 0) { return false; @@ -350,6 +362,107 @@ internal static void VerifyImageMapping(SafeProcessHandle process, SafeFileHandl } } + internal static void VerifyProtectedPath(SafeFileHandle handle, string subject, int tamperRights) + { + if (!Native.GetFileInformationByHandle(handle, out Native.ByHandleFileInformation information)) + { + throw new Win32Exception(); + } + if (IsReparsePoint(information.FileAttributes)) + { + throw new InvalidOperationException($"{subject} is a reparse point"); + } + + uint error = Native.GetSecurityInfo( + handle, + 1, + 0x1 | 0x4, + out _, + out _, + out _, + out _, + out IntPtr securityDescriptor); + if (error != 0) + { + throw new Win32Exception(checked((int)error)); + } + + try + { + int length = checked((int)Native.GetSecurityDescriptorLength(securityDescriptor)); + byte[] bytes = new byte[length]; + Marshal.Copy(securityDescriptor, bytes, 0, length); + VerifyTrustedSecurityDescriptor(new RawSecurityDescriptor(bytes, 0), subject, tamperRights); + } + finally + { + _ = Native.LocalFree(securityDescriptor); + } + } + + internal static bool IsReparsePoint(uint attributes) => + (attributes & FileAttributeReparsePoint) != 0; + + internal const int FileTamperRights = + 0x0000_0002 | 0x0000_0004 | 0x0000_0010 | 0x0000_0100 | + 0x0001_0000 | 0x0004_0000 | 0x0008_0000 | 0x1000_0000 | 0x4000_0000; + internal const int ParentDirectoryTamperRights = + 0x0000_0002 | 0x0000_0004 | 0x0000_0040 | + 0x0001_0000 | 0x0004_0000 | 0x0008_0000 | 0x1000_0000 | 0x4000_0000; + internal const int AncestorDirectoryTamperRights = + 0x0000_0040 | 0x0001_0000 | 0x0004_0000 | 0x0008_0000 | 0x1000_0000; + + internal static void VerifyTrustedSecurityDescriptor( + RawSecurityDescriptor descriptor, + string subject, + int tamperRights) + { + if (descriptor.Owner is not SecurityIdentifier owner || !IsTrustedWriter(owner)) + { + throw new InvalidOperationException($"{subject} has an untrusted owner"); + } + if (!descriptor.ControlFlags.HasFlag(ControlFlags.DiscretionaryAclPresent) || + descriptor.DiscretionaryAcl is not { Count: > 0 } dacl) + { + throw new InvalidOperationException($"{subject} has no protective DACL"); + } + + foreach (GenericAce generic in dacl) + { + if (generic.AceFlags.HasFlag(AceFlags.InheritOnly) || !IsAccessAllowedAce(generic.AceType)) + { + continue; + } + if (generic is not QualifiedAce ace || ace is not KnownAce known) + { + throw new InvalidOperationException($"{subject} has an unsupported access-allowed entry"); + } + if ((known.AccessMask & tamperRights) == 0) + { + continue; + } + if (ace.SecurityIdentifier is null || !IsTrustedWriter(ace.SecurityIdentifier)) + { + throw new InvalidOperationException($"{subject} grants write access to an untrusted principal"); + } + } + } + + private static bool IsAccessAllowedAce(AceType type) => + type is AceType.AccessAllowed or + AceType.AccessAllowedCompound or + AceType.AccessAllowedObject or + AceType.AccessAllowedCallback or + AceType.AccessAllowedCallbackObject; + + private static bool IsTrustedWriter(SecurityIdentifier sid) => + sid.IsWellKnown(WellKnownSidType.LocalSystemSid) || + sid.IsWellKnown(WellKnownSidType.BuiltinAdministratorsSid) || + string.Equals( + sid.Value, + "S-1-5-80-956008885-3418522649-1831038044-1853292631-2271478464", + StringComparison.Ordinal); + internal static string ImagePath(SafeProcessHandle process) { int capacity = 260; @@ -407,11 +520,16 @@ internal sealed class BrokerServerLease : IDisposable private readonly SafeProcessHandle process; private readonly SafeFileHandle image; + private readonly List directories; - private BrokerServerLease(SafeProcessHandle process, SafeFileHandle image) + private BrokerServerLease( + SafeProcessHandle process, + SafeFileHandle image, + List directories) { this.process = process; this.image = image; + this.directories = directories; } internal static BrokerServerLease Open(SafePipeHandle pipe) @@ -422,7 +540,9 @@ internal static BrokerServerLease Open(SafePipeHandle pipe) } SafeProcessHandle process = Native.OpenProcess( - PeerLease.ProcessQueryLimitedInformation | PeerLease.Synchronize, + PeerLease.ProcessQueryInformation | + PeerLease.ProcessQueryLimitedInformation | + PeerLease.Synchronize, false, processId); if (process.IsInvalid) @@ -449,12 +569,15 @@ internal static BrokerServerLease Open(SafePipeHandle pipe) } SafeFileHandle image = Native.CreateFile( - expectedPath, - PeerLease.GenericRead | PeerLease.FileExecute | PeerLease.Synchronize, + serverPath, + PeerLease.GenericRead | + PeerLease.FileExecute | + PeerLease.ReadControl | + PeerLease.Synchronize, PeerLease.FileShareRead, IntPtr.Zero, PeerLease.OpenExisting, - 0, + PeerLease.FileFlagOpenReparsePoint, IntPtr.Zero); if (image.IsInvalid) { @@ -463,10 +586,37 @@ internal static BrokerServerLease Open(SafePipeHandle pipe) try { - using X509Certificate2 _ = - PeerLease.VerifyAuthenticodeSigner(expectedPath, image, "broker server"); - PeerLease.EnsureActive(process); - return new BrokerServerLease(process, image); + PeerLease.VerifyImageMapping(process, image); + PeerLease.VerifyProtectedPath(image, "broker server image", PeerLease.FileTamperRights); + using X509Certificate2 signer = + PeerLease.VerifyAuthenticodeSigner(serverPath, image, "broker server"); + string thumbprint = signer.GetCertHashString(HashAlgorithmName.SHA1).ToLowerInvariant(); + if (!PeerLease.IsAllowedDevolutionsSigner(thumbprint)) + { + throw new InvalidOperationException("broker server signer is not authorized"); + } + List? directories = RetainProtectedDirectories( + Path.GetDirectoryName(expectedPath) + ?? throw new InvalidOperationException("Agent installation directory is unavailable")); + try + { + PeerLease.VerifyImageMapping(process, image); + PeerLease.EnsureActive(process); + if (!Native.GetNamedPipeServerProcessId(pipe, out int confirmedProcessId) || + confirmedProcessId != processId) + { + throw new InvalidOperationException("broker server process changed during authentication"); + } + return new BrokerServerLease(process, image, directories); + } + catch + { + foreach (SafeFileHandle directory in directories) + { + directory.Dispose(); + } + throw; + } } catch { @@ -489,9 +639,54 @@ internal static bool IsExpectedPath(string actual, string expected) => public void Dispose() { + foreach (SafeFileHandle directory in directories) + { + directory.Dispose(); + } image.Dispose(); process.Dispose(); } + + private static List RetainProtectedDirectories(string installationDirectory) + { + List handles = []; + try + { + int tamperRights = PeerLease.ParentDirectoryTamperRights; + for (DirectoryInfo? directory = new(Path.GetFullPath(installationDirectory)); + directory is not null; + directory = directory.Parent) + { + SafeFileHandle handle = Native.CreateFile( + directory.FullName, + PeerLease.FileReadAttributes | PeerLease.ReadControl | PeerLease.Synchronize, + PeerLease.FileShareRead | PeerLease.FileShareWrite, + IntPtr.Zero, + PeerLease.OpenExisting, + PeerLease.FileFlagBackupSemantics | PeerLease.FileFlagOpenReparsePoint, + IntPtr.Zero); + if (handle.IsInvalid) + { + throw new Win32Exception(); + } + handles.Add(handle); + PeerLease.VerifyProtectedPath( + handle, + $"Agent installation directory '{directory.FullName}'", + tamperRights); + tamperRights = PeerLease.AncestorDirectoryTamperRights; + } + return handles; + } + catch + { + foreach (SafeFileHandle handle in handles) + { + handle.Dispose(); + } + throw; + } + } } internal static partial class Native @@ -649,6 +844,23 @@ internal static partial bool GetTokenInformation( [return: MarshalAs(UnmanagedType.Bool)] internal static partial bool IsWellKnownSid(IntPtr sid, int wellKnownSidType); + [LibraryImport("advapi32.dll", SetLastError = true)] + internal static partial uint GetSecurityInfo( + SafeFileHandle handle, + uint objectType, + uint securityInformation, + out IntPtr owner, + out IntPtr group, + out IntPtr dacl, + out IntPtr sacl, + out IntPtr securityDescriptor); + + [LibraryImport("advapi32.dll")] + internal static partial uint GetSecurityDescriptorLength(IntPtr securityDescriptor); + + [LibraryImport("kernel32.dll")] + internal static partial IntPtr LocalFree(IntPtr memory); + [LibraryImport("kernel32.dll", EntryPoint = "CreateFileW", SetLastError = true, StringMarshalling = StringMarshalling.Utf16)] internal static partial SafeFileHandle CreateFile( string fileName, diff --git a/package/AgentPolicyConsent/PolicyConsentContract.cs b/package/AgentPolicyConsent/PolicyConsentContract.cs index 01789bbf5..697d606d3 100644 --- a/package/AgentPolicyConsent/PolicyConsentContract.cs +++ b/package/AgentPolicyConsent/PolicyConsentContract.cs @@ -5,9 +5,29 @@ internal static class PolicyConsentContract internal const string ProtocolVersion = "2.0"; internal const string ExecutableName = "DevolutionsAgentPolicyConsent.exe"; internal const string ProductName = "Devolutions Agent Policy Consent"; + + // UniGetUI 2026.2.7: subject CN=Devolutions Inc, O=Devolutions Inc, C=CA; + // issuer CN=GlobalSign GCC R45 EV CodeSigning CA 2020, O=GlobalSign nv-sa, C=BE; + // serial 73D3C33603FF8BB44224F25E, SHA-1 8DB5A43BB8AFE4D2FFB92DA9007D8997A4CC4E13, + // valid 2023-10-30T17:51:18Z through 2026-10-30T17:51:18Z. internal const string CurrentUiSignerSpkiSha256 = "e43ed3368eaabff61abc79eb338cba9da88a80d93b751735ff417f26afa579a8"; + + // UniGetUI 3.3.7: subject CN="Open Source Developer, Martí Climent López", + // O=Open Source Developer, C=ES; issuer CN=Certum Code Signing 2021 CA, + // O=Asseco Data Systems S.A., C=PL; + // serial 1AC2CAA58AF100E402D9812002C08B30, SHA-1 28949703053434989162B12C101497DE35FE4E8E, + // valid 2025-06-24T18:02:38Z through 2026-06-24T18:02:37Z. + // Remove this transition pin when the minimum supported UniGetUI version postdates its last signed release. internal const string TransitionUiSignerSpkiSha256 = "99e7adb5894e242d87d32b8ad6cb5a1e0d2dd791a447bd7192c30189ef083fab"; + + // Keep synchronized with devolutions-agent-shared/src/windows/code_signing.rs. + internal static readonly string[] DevolutionsSignerSha1Thumbprints = + [ + "3f5202a9432d54293bdfe6f7e46adb0a6f8b3ba6", + "8db5a43bb8afe4d2ffb92da9007d8997a4cc4e13", + "50f753333811ff11f1920274afde3ffd4468b210", + ]; } } From 7160af7520b8fd1b2f905c4fe94743bd7da1683f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Thu, 10 Sep 2026 01:52:23 -0400 Subject: [PATCH 04/25] fix(agent,agent-installer): correct helper packaging Preserve timeout responses during broker authentication, accept prerelease UniGetUI versions, and suppress the helper console window. Mark ARM64 discovery registry components as 64-bit so WiX can package them beneath ProgramFiles64Folder. Issue: #1963 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- package/AgentPolicyConsent.Tests/ProtocolTests.cs | 1 + package/AgentPolicyConsent/BrokerClient.cs | 5 +++++ .../AgentPolicyConsent/DevolutionsAgentPolicyConsent.csproj | 2 +- package/AgentPolicyConsent/PeerTrust.cs | 2 +- package/AgentWindowsManaged/Program.cs | 2 +- 5 files changed, 9 insertions(+), 3 deletions(-) diff --git a/package/AgentPolicyConsent.Tests/ProtocolTests.cs b/package/AgentPolicyConsent.Tests/ProtocolTests.cs index 07a87266e..f3bdbb5ff 100644 --- a/package/AgentPolicyConsent.Tests/ProtocolTests.cs +++ b/package/AgentPolicyConsent.Tests/ProtocolTests.cs @@ -91,6 +91,7 @@ public void SignerMatchingIsCaseSensitive() [Theory] [InlineData("3.3.7")] [InlineData("2026.2.7")] + [InlineData("2026.2.7-preview")] public void ProductBindingSupportsProtocolEraAndCurrentInstallModes(string version) { Assert.True(PeerLease.IsSupportedUiIdentity("UniGetUI", "UniGetUI.dll", version)); diff --git a/package/AgentPolicyConsent/BrokerClient.cs b/package/AgentPolicyConsent/BrokerClient.cs index 4a1d5affc..1dfe80305 100644 --- a/package/AgentPolicyConsent/BrokerClient.cs +++ b/package/AgentPolicyConsent/BrokerClient.cs @@ -255,6 +255,11 @@ private static async Task OpenBrokerServerAsync( { return await open.WaitAsync(cancellationToken); } + catch (OperationCanceledException) when (cancellationToken.IsCancellationRequested) + { + DisposeLateResult(open); + throw; + } catch (Exception error) { DisposeLateResult(open); diff --git a/package/AgentPolicyConsent/DevolutionsAgentPolicyConsent.csproj b/package/AgentPolicyConsent/DevolutionsAgentPolicyConsent.csproj index 5e5e414e4..a8257d4dc 100644 --- a/package/AgentPolicyConsent/DevolutionsAgentPolicyConsent.csproj +++ b/package/AgentPolicyConsent/DevolutionsAgentPolicyConsent.csproj @@ -1,6 +1,6 @@ - Exe + WinExe net10.0-windows win-x64 true diff --git a/package/AgentPolicyConsent/PeerTrust.cs b/package/AgentPolicyConsent/PeerTrust.cs index 9fc0eb204..2948e109e 100644 --- a/package/AgentPolicyConsent/PeerTrust.cs +++ b/package/AgentPolicyConsent/PeerTrust.cs @@ -138,7 +138,7 @@ internal static bool IsSupportedUiIdentity(string? productName, string? original string.Equals(productName, "UniGetUI", StringComparison.Ordinal) && string.Equals(originalFilename, "UniGetUI.dll", StringComparison.OrdinalIgnoreCase) && productVersion is not null && - Version.TryParse(productVersion.Split('+', '-', StringSplitOptions.TrimEntries)[0], out Version? parsed) && + Version.TryParse(productVersion.Split(['+', '-'], StringSplitOptions.TrimEntries)[0], out Version? parsed) && parsed >= new Version(3, 3, 7); internal static bool MatchesProcessIdentity( diff --git a/package/AgentWindowsManaged/Program.cs b/package/AgentWindowsManaged/Program.cs index 83ded8e51..707b1e611 100644 --- a/package/AgentWindowsManaged/Program.cs +++ b/package/AgentWindowsManaged/Program.cs @@ -480,7 +480,7 @@ internal static RegValue CreatePolicyConsentRegistryValue(string name, string va name, value) { - AttributesDefinition = "Type=string", + AttributesDefinition = win64 ? "Type=string; Component:Win64=yes" : "Type=string", Win64 = win64, RegistryKeyAction = RegistryKeyAction.createAndRemoveOnUninstall, Feature = Features.AGENT_FEATURE, From 04371ab02f09cc1a1c4ebcb5a45ee7b2ca410b60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Thu, 10 Sep 2026 02:04:59 -0400 Subject: [PATCH 05/25] fix(agent): validate policy credentials Apply the canonical policy API safe-ASCII character set to helper credentials before a privileged request is dispatched. Issue: #1963 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../AgentPolicyConsent.Tests/ProtocolTests.cs | 18 +++++++++++++++--- package/AgentPolicyConsent/Protocol.cs | 2 +- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/package/AgentPolicyConsent.Tests/ProtocolTests.cs b/package/AgentPolicyConsent.Tests/ProtocolTests.cs index f3bdbb5ff..9b4aafaee 100644 --- a/package/AgentPolicyConsent.Tests/ProtocolTests.cs +++ b/package/AgentPolicyConsent.Tests/ProtocolTests.cs @@ -356,6 +356,18 @@ public void RequestRejectsUnknownOperations(string operation, string conflictHan Assert.Throws(() => Protocol.ValidateRequest(request)); } + [Theory] + [InlineData("a", true)] + [InlineData("A0._~:-", true)] + [InlineData("a/b", false)] + [InlineData("a b", false)] + [InlineData("a\"b", false)] + [InlineData("-token", false)] + public void CredentialsMatchOfficialPolicyApiCharacterRules(string value, bool accepted) + { + Assert.Equal(accepted, Protocol.IsCredential(value, 512)); + } + [Fact] public void CommittedResponseContainsOnlyStoreToken() { @@ -424,7 +436,7 @@ public void ResponseRequiresExplicitNullableMembers() } [Fact] - public void MaximumStaleResponseFitsExactWireBudget() + public void MaximumValidStaleResponseFitsWireBudget() { ElevationResponse response = new( "2.0", @@ -433,9 +445,9 @@ public void MaximumStaleResponseFitsExactWireBudget() 409, "StalePolicyStoreToken", null, - "T" + new string('"', 511), + "T" + new string('~', 511), "Active", - "P" + new string('"', 2047)); + "P" + new string('~', 2047)); Protocol.ValidateResponse(response); byte[] body = JsonSerializer.SerializeToUtf8Bytes( diff --git a/package/AgentPolicyConsent/Protocol.cs b/package/AgentPolicyConsent/Protocol.cs index 04184f3dc..a9057db90 100644 --- a/package/AgentPolicyConsent/Protocol.cs +++ b/package/AgentPolicyConsent/Protocol.cs @@ -172,7 +172,7 @@ value is not null && value.Length is > 0 && value.Length <= maximum && IsAsciiAlphaNumeric(value[0]) && - value.AsSpan(1).IndexOfAnyExceptInRange('!', '~') < 0; + value.AsSpan(1).IndexOfAnyExcept("ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789._~:-") < 0; private static bool IsOptionalCredential(string? value, int maximum) => value is null || IsCredential(value, maximum); From a17351d4d0498a21d22f7a117fd2586244dee3bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Thu, 17 Sep 2026 10:02:23 +0900 Subject: [PATCH 06/25] fix(agent,agent-installer): require current UI signer Accept only current-signed UniGetUI hosts from version 2026.2.7. Remove transition signer discovery while retaining protected helper authorization and ARM64 installer coverage. Issue: #1963 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../AgentPolicyConsent.Tests/ProtocolTests.cs | 9 +++-- package/AgentPolicyConsent/PeerTrust.cs | 8 +--- .../PolicyConsentContract.cs | 12 +----- .../PolicyConsentDiscoveryTests.cs | 37 +++++++++++++++++-- 4 files changed, 42 insertions(+), 24 deletions(-) diff --git a/package/AgentPolicyConsent.Tests/ProtocolTests.cs b/package/AgentPolicyConsent.Tests/ProtocolTests.cs index 9b4aafaee..e1d0a0a4d 100644 --- a/package/AgentPolicyConsent.Tests/ProtocolTests.cs +++ b/package/AgentPolicyConsent.Tests/ProtocolTests.cs @@ -30,10 +30,11 @@ public void ArgumentsRequireExactBoundIdentity() public sealed class TrustPolicyTests { [Fact] - public void CurrentAndTransitionSignersAreAccepted() + public void OnlyTheCurrentSignerIsAccepted() { Assert.True(PeerLease.IsAllowedSigner(PeerLease.CurrentUiSignerSpkiSha256)); - Assert.True(PeerLease.IsAllowedSigner(PeerLease.TransitionUiSignerSpkiSha256)); + Assert.False(PeerLease.IsAllowedSigner( + "99e7adb5894e242d87d32b8ad6cb5a1e0d2dd791a447bd7192c30189ef083fab")); } [Fact] @@ -89,10 +90,9 @@ public void SignerMatchingIsCaseSensitive() } [Theory] - [InlineData("3.3.7")] [InlineData("2026.2.7")] [InlineData("2026.2.7-preview")] - public void ProductBindingSupportsProtocolEraAndCurrentInstallModes(string version) + public void ProductBindingSupportsCurrentSignedHosts(string version) { Assert.True(PeerLease.IsSupportedUiIdentity("UniGetUI", "UniGetUI.dll", version)); } @@ -101,6 +101,7 @@ public void ProductBindingSupportsProtocolEraAndCurrentInstallModes(string versi [InlineData("Lookalike", "UniGetUI.dll", "2026.2.7")] [InlineData("UniGetUI", "malware.exe", "2026.2.7")] [InlineData("UniGetUI", "UniGetUI.dll", "3.3.6")] + [InlineData("UniGetUI", "UniGetUI.dll", "2026.2.6")] public void ProductBindingRejectsLookalikes(string product, string originalFilename, string version) { Assert.False(PeerLease.IsSupportedUiIdentity(product, originalFilename, version)); diff --git a/package/AgentPolicyConsent/PeerTrust.cs b/package/AgentPolicyConsent/PeerTrust.cs index 2948e109e..812652fae 100644 --- a/package/AgentPolicyConsent/PeerTrust.cs +++ b/package/AgentPolicyConsent/PeerTrust.cs @@ -12,9 +12,6 @@ namespace DevolutionsAgentPolicyConsent; internal sealed class PeerLease : IDisposable { - // SHA-256 digests of accepted UniGetUI signer SPKIs. Keep both keys during certificate rollover. - internal const string TransitionUiSignerSpkiSha256 = - PolicyConsentContract.TransitionUiSignerSpkiSha256; internal const string CurrentUiSignerSpkiSha256 = PolicyConsentContract.CurrentUiSignerSpkiSha256; @@ -127,8 +124,7 @@ internal void VerifyConnectedServer(int serverProcessId) } internal static bool IsAllowedSigner(string digest) => - FixedTimeEqualsHex(digest, CurrentUiSignerSpkiSha256) || - FixedTimeEqualsHex(digest, TransitionUiSignerSpkiSha256); + FixedTimeEqualsHex(digest, CurrentUiSignerSpkiSha256); internal static bool IsAllowedDevolutionsSigner(string thumbprint) => PolicyConsentContract.DevolutionsSignerSha1Thumbprints.Any( @@ -139,7 +135,7 @@ internal static bool IsSupportedUiIdentity(string? productName, string? original string.Equals(originalFilename, "UniGetUI.dll", StringComparison.OrdinalIgnoreCase) && productVersion is not null && Version.TryParse(productVersion.Split(['+', '-'], StringSplitOptions.TrimEntries)[0], out Version? parsed) && - parsed >= new Version(3, 3, 7); + parsed >= new Version(2026, 2, 7); internal static bool MatchesProcessIdentity( Arguments expected, diff --git a/package/AgentPolicyConsent/PolicyConsentContract.cs b/package/AgentPolicyConsent/PolicyConsentContract.cs index 697d606d3..d4d0958cd 100644 --- a/package/AgentPolicyConsent/PolicyConsentContract.cs +++ b/package/AgentPolicyConsent/PolicyConsentContract.cs @@ -6,22 +6,14 @@ internal static class PolicyConsentContract internal const string ExecutableName = "DevolutionsAgentPolicyConsent.exe"; internal const string ProductName = "Devolutions Agent Policy Consent"; - // UniGetUI 2026.2.7: subject CN=Devolutions Inc, O=Devolutions Inc, C=CA; + // Supported UniGetUI hosts are version 2026.2.7 or newer and must carry this signer: + // subject CN=Devolutions Inc, O=Devolutions Inc, C=CA; // issuer CN=GlobalSign GCC R45 EV CodeSigning CA 2020, O=GlobalSign nv-sa, C=BE; // serial 73D3C33603FF8BB44224F25E, SHA-1 8DB5A43BB8AFE4D2FFB92DA9007D8997A4CC4E13, // valid 2023-10-30T17:51:18Z through 2026-10-30T17:51:18Z. internal const string CurrentUiSignerSpkiSha256 = "e43ed3368eaabff61abc79eb338cba9da88a80d93b751735ff417f26afa579a8"; - // UniGetUI 3.3.7: subject CN="Open Source Developer, Martí Climent López", - // O=Open Source Developer, C=ES; issuer CN=Certum Code Signing 2021 CA, - // O=Asseco Data Systems S.A., C=PL; - // serial 1AC2CAA58AF100E402D9812002C08B30, SHA-1 28949703053434989162B12C101497DE35FE4E8E, - // valid 2025-06-24T18:02:38Z through 2026-06-24T18:02:37Z. - // Remove this transition pin when the minimum supported UniGetUI version postdates its last signed release. - internal const string TransitionUiSignerSpkiSha256 = - "99e7adb5894e242d87d32b8ad6cb5a1e0d2dd791a447bd7192c30189ef083fab"; - // Keep synchronized with devolutions-agent-shared/src/windows/code_signing.rs. internal static readonly string[] DevolutionsSignerSha1Thumbprints = [ diff --git a/package/AgentWindowsManaged.Tests/PolicyConsentDiscoveryTests.cs b/package/AgentWindowsManaged.Tests/PolicyConsentDiscoveryTests.cs index a17e10bef..83a7a1210 100644 --- a/package/AgentWindowsManaged.Tests/PolicyConsentDiscoveryTests.cs +++ b/package/AgentWindowsManaged.Tests/PolicyConsentDiscoveryTests.cs @@ -20,23 +20,52 @@ public void DiscoveryIsTransactionalAndArchitectureCorrect(bool win64) Assert.Equal(@"Software\Devolutions\Agent\PolicyConsentHelper", value.Key); Assert.Equal(RegistryKeyAction.createAndRemoveOnUninstall, value.RegistryKeyAction); Assert.Equal(win64, value.Win64); + Assert.Equal( + win64 ? "Type=string; Component:Win64=yes" : "Type=string", + value.AttributesDefinition); + } + + [Theory] + [InlineData(Platform.x86, false)] + [InlineData(Platform.x64, true)] + [InlineData(Platform.arm64, true)] + public void DiscoveryUsesTheConsumerNativeRegistryView(Platform platform, bool expected) + { + Type program = System.Reflection.Assembly + .Load("DevolutionsAgent") + .GetType("DevolutionsAgent.Program", throwOnError: true); + MethodInfo method = program.GetMethod( + "Use64BitRegistryView", + BindingFlags.Static | BindingFlags.NonPublic); + + Assert.Equal(expected, Assert.IsType(method.Invoke(null, [platform]))); } [Fact] public void DiscoveryPublishesTheFixedHelperIdentity() { - RegValue value = CreateDiscoveryValue( + RegValue executablePath = CreateDiscoveryValue( "ExecutablePath", "[INSTALLDIR]DevolutionsAgentPolicyConsent.exe", true); + RegValue signer = CreateDiscoveryValue( + "CurrentUiSignerSpkiSha256", + "e43ed3368eaabff61abc79eb338cba9da88a80d93b751735ff417f26afa579a8", + true); - Assert.Equal("[INSTALLDIR]DevolutionsAgentPolicyConsent.exe", value.Value); - Assert.Equal(RegistryKeyAction.createAndRemoveOnUninstall, value.RegistryKeyAction); + Assert.Equal("[INSTALLDIR]DevolutionsAgentPolicyConsent.exe", executablePath.Value); + Assert.Equal( + "e43ed3368eaabff61abc79eb338cba9da88a80d93b751735ff417f26afa579a8", + signer.Value); + Assert.Equal(RegistryKeyAction.createAndRemoveOnUninstall, executablePath.RegistryKeyAction); + Assert.Equal(RegistryKeyAction.createAndRemoveOnUninstall, signer.RegistryKeyAction); } private static RegValue CreateDiscoveryValue(string name, string value, bool win64) { - Type program = Assembly.Load("DevolutionsAgent").GetType("DevolutionsAgent.Program", throwOnError: true); + Type program = System.Reflection.Assembly + .Load("DevolutionsAgent") + .GetType("DevolutionsAgent.Program", throwOnError: true); MethodInfo method = program.GetMethod( "CreatePolicyConsentRegistryValue", BindingFlags.Static | BindingFlags.NonPublic); From 85ca00618d232bcd283e984410d78a0c2dc84c9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Fri, 18 Sep 2026 01:36:11 +0900 Subject: [PATCH 07/25] fix(agent,agent-installer): discover broker pipe Publish the configured broker pipe through the protected helper discovery key so consent writes work with custom local pipe names. Reject malformed discovery and oversized requests before dispatch. Issue: #1963 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- Cargo.lock | 1 + devolutions-agent/Cargo.toml | 1 + devolutions-agent/src/service.rs | 63 +++++++++++++++++-- .../AgentPolicyConsent.Tests/ProtocolTests.cs | 50 +++++++++++++++ package/AgentPolicyConsent/BrokerClient.cs | 49 +++++++++++++-- .../PolicyConsentContract.cs | 1 + package/AgentPolicyConsent/Protocol.cs | 6 +- .../PolicyConsentDiscoveryTests.cs | 6 ++ package/AgentWindowsManaged/Program.cs | 4 ++ .../AgentWindowsManaged/Resources/Includes.cs | 3 + 10 files changed, 173 insertions(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0cc021554..b29f1820d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1791,6 +1791,7 @@ dependencies = [ "uuid", "win-api-wrappers", "windows 0.61.3", + "windows-registry 0.5.3", "x509-parser", ] diff --git a/devolutions-agent/Cargo.toml b/devolutions-agent/Cargo.toml index 2be961bb6..059b57741 100644 --- a/devolutions-agent/Cargo.toml +++ b/devolutions-agent/Cargo.toml @@ -93,6 +93,7 @@ reqwest = { version = "0.12", default-features = false, features = ["rustls-tls- thiserror = "2" uuid = { version = "1.17", features = ["v4"] } win-api-wrappers = { path = "../crates/win-api-wrappers" } +windows-registry = "0.5" [target.'cfg(windows)'.dependencies.windows] version = "0.61" diff --git a/devolutions-agent/src/service.rs b/devolutions-agent/src/service.rs index 6739ea391..46a59f4fe 100644 --- a/devolutions-agent/src/service.rs +++ b/devolutions-agent/src/service.rs @@ -21,10 +21,16 @@ use now_package_broker::pipe::DEFAULT_PIPE_NAME; use now_package_broker::task::{BrokerTask, BrokerTaskConfig}; use tokio::runtime::{self, Runtime}; use tokio::sync::mpsc; +#[cfg(windows)] +use windows_registry::{Key, LOCAL_MACHINE}; pub(crate) const SERVICE_NAME: &str = "devolutions-agent"; pub(crate) const DISPLAY_NAME: &str = "Devolutions Agent"; pub(crate) const DESCRIPTION: &str = "Devolutions Agent service"; +#[cfg(windows)] +const POLICY_CONSENT_DISCOVERY_KEY: &str = r"SOFTWARE\Devolutions\Agent\PolicyConsentHelper"; +#[cfg(windows)] +const POLICY_CONSENT_BROKER_PIPE_NAME_VALUE: &str = "BrokerPipeName"; struct TasksCtx { /// Spawned service tasks @@ -226,12 +232,19 @@ async fn spawn_tasks(conf_handle: ConfHandle) -> anyhow::Result { ); } + let pipe_name = conf + .package_broker + .pipe_name + .clone() + .unwrap_or_else(|| DEFAULT_PIPE_NAME.to_owned()); + if let Err(error) = publish_policy_consent_broker_pipe_name(&pipe_name) { + warn!( + error = format!("{error:#}"), + "Policy consent helper cannot discover the configured broker pipe" + ); + } let broker_config = BrokerTaskConfig { - pipe_name: conf - .package_broker - .pipe_name - .clone() - .unwrap_or_else(|| DEFAULT_PIPE_NAME.to_owned()), + pipe_name, policy_path: conf.package_broker.policy_path.clone(), // The bypass only takes effect in builds with the development-only // `dev-skip-broker-signature` cargo feature, never in shipped builds. @@ -271,3 +284,43 @@ async fn spawn_tasks(conf_handle: ConfHandle) -> anyhow::Result { service_event_tx, }) } + +#[cfg(windows)] +fn publish_policy_consent_broker_pipe_name(pipe_name: &str) -> anyhow::Result<()> { + let key = LOCAL_MACHINE + .options() + .read() + .write() + .open(POLICY_CONSENT_DISCOVERY_KEY) + .context("open policy consent helper discovery key")?; + publish_policy_consent_broker_pipe_name_to(&key, pipe_name) +} + +#[cfg(windows)] +fn publish_policy_consent_broker_pipe_name_to(key: &Key, pipe_name: &str) -> anyhow::Result<()> { + key.set_string(POLICY_CONSENT_BROKER_PIPE_NAME_VALUE, pipe_name) + .context("publish configured policy consent helper broker pipe") +} + +#[cfg(all(test, windows))] +mod tests { + use windows_registry::CURRENT_USER; + + use super::*; + + #[test] + fn configured_broker_pipe_is_published_to_writable_discovery_key() { + let path = format!(r"Software\Devolutions\Agent\Tests\{}", uuid::Uuid::new_v4().as_simple()); + let key = CURRENT_USER.create(&path).expect("create temporary discovery key"); + let pipe_name = r"\\.\pipe\custom-broker"; + + publish_policy_consent_broker_pipe_name_to(&key, pipe_name).expect("publish configured broker pipe"); + + assert_eq!( + key.get_string(POLICY_CONSENT_BROKER_PIPE_NAME_VALUE) + .expect("read published broker pipe"), + pipe_name + ); + CURRENT_USER.remove_tree(&path).expect("remove temporary discovery key"); + } +} diff --git a/package/AgentPolicyConsent.Tests/ProtocolTests.cs b/package/AgentPolicyConsent.Tests/ProtocolTests.cs index e1d0a0a4d..bbb7bbb5d 100644 --- a/package/AgentPolicyConsent.Tests/ProtocolTests.cs +++ b/package/AgentPolicyConsent.Tests/ProtocolTests.cs @@ -338,6 +338,56 @@ public void RequestRequiresEveryMemberAndObjectDraft() Assert.Throws(() => Protocol.ValidateRequest(request)); } + [Fact] + public void RequestRejectsExplicitNullRequiredMembers() + { + string nullRequestId = + """{"protocolVersion":"2.0","requestId":null,"operation":"Update","conflictHandling":"Reject","expectedStoreToken":"a","validationReceipt":"b","warningsAcknowledged":false,"draft":{}}"""; + + ElevationRequest request = JsonSerializer.Deserialize( + nullRequestId, + ProtocolJsonContext.Default.ElevationRequest) + ?? throw new InvalidOperationException("request should deserialize"); + Assert.Throws(() => Protocol.ValidateRequest(request)); + } + + [Theory] + [InlineData(@"\\.\pipe\Devolutions.Now.PackageBroker.v1", "Devolutions.Now.PackageBroker.v1")] + [InlineData("custom-broker", "custom-broker")] + public void BrokerPipeDiscoveryNormalizesLocalPipeNames(string configured, string expected) + { + Assert.Equal(expected, BrokerClient.NormalizeBrokerPipeName(configured)); + } + + [Theory] + [InlineData("")] + [InlineData(@"\\server\pipe\broker")] + [InlineData(@"\\.\pipe\")] + public void BrokerPipeDiscoveryRejectsNonlocalOrEmptyNames(string configured) + { + Assert.ThrowsAny(() => BrokerClient.NormalizeBrokerPipeName(configured)); + } + + [Fact] + public async Task OversizedOfficialRequestIsRejectedBeforeBrokerConnection() + { + using JsonDocument draft = JsonDocument.Parse($$"""{"padding":"{{new string('x', Protocol.MaxRequestBodyBytes)}}" }"""); + ElevationRequest request = new( + "2.0", + RequestId, + "Update", + "Reject", + "token", + "receipt", + false, + draft.RootElement.Clone()); + + ElevationResponse response = await BrokerClient.ReplaceAsync(request, CancellationToken.None); + + Assert.Equal("Rejected", response.Disposition); + Assert.Equal("InvalidRequest", response.BrokerErrorCode); + } + [Theory] [InlineData("Delete", "Reject")] [InlineData("Update", "Overwrite")] diff --git a/package/AgentPolicyConsent/BrokerClient.cs b/package/AgentPolicyConsent/BrokerClient.cs index 1dfe80305..c2594e4b0 100644 --- a/package/AgentPolicyConsent/BrokerClient.cs +++ b/package/AgentPolicyConsent/BrokerClient.cs @@ -1,5 +1,6 @@ using System.Buffers; using System.IO.Pipes; +using Microsoft.Win32; using System.Text; using System.Text.Json; @@ -7,7 +8,8 @@ namespace DevolutionsAgentPolicyConsent; internal static class BrokerClient { - private const string PipeName = "Devolutions.Now.PackageBroker.v1"; + private const string BrokerPipeNameValue = "BrokerPipeName"; + private const string DiscoveryKey = @"SOFTWARE\Devolutions\Agent\PolicyConsentHelper"; private const int MaximumHeaderBytes = 64 * 1024; private const int MaximumBrokerResponseBytes = 50_606_928; private static readonly TimeSpan ConnectTimeout = TimeSpan.FromSeconds(5); @@ -16,12 +18,12 @@ internal static async Task ReplaceAsync( ElevationRequest request, CancellationToken cancellationToken) { - byte[] body = CreateOfficialRequest(request); try { + byte[] body = CreateOfficialRequest(request); using NamedPipeClientStream pipe = new( ".", - PipeName, + ReadBrokerPipeName(), PipeDirection.InOut, PipeOptions.Asynchronous | PipeOptions.WriteThrough, System.Security.Principal.TokenImpersonationLevel.Anonymous); @@ -64,14 +66,34 @@ internal static async Task ReplaceAsync( { return Rejected(request.RequestId, "Unauthorized"); } + catch (BrokerDiscoveryException) + { + return Rejected(request.RequestId, "BrokerUnavailable"); + } catch (InvalidOperationException) { return Unknown(request.RequestId, null, "InvalidResponse"); } catch (ProtocolException) { - return Unknown(request.RequestId, null, "InvalidResponse"); + return Rejected(request.RequestId, "InvalidRequest"); + } + } + + internal static string NormalizeBrokerPipeName(string pipeName) + { + const string LocalPipePrefix = @"\\.\pipe\"; + if (pipeName.StartsWith(LocalPipePrefix, StringComparison.OrdinalIgnoreCase)) + { + pipeName = pipeName[LocalPipePrefix.Length..]; + } + if (pipeName.Length is 0 or > 256 || + pipeName.IndexOf('\0') >= 0 || + pipeName.Contains('\\')) + { + throw new BrokerDiscoveryException(); } + return pipeName; } internal static byte[] CreateOfficialRequest(ElevationRequest request) @@ -246,6 +268,17 @@ private static ElevationResponse Unknown(string requestId, int? statusCode, stri private static ElevationResponse Rejected(string requestId, string errorCode) => new(Protocol.Version, requestId, "Rejected", null, errorCode, null, null, null, null); + private static string ReadBrokerPipeName() + { + using RegistryKey machine = RegistryKey.OpenBaseKey(RegistryHive.LocalMachine, RegistryView.Registry64); + using RegistryKey? discovery = machine.OpenSubKey(DiscoveryKey); + if (discovery?.GetValue(BrokerPipeNameValue) is not string pipeName) + { + throw new BrokerDiscoveryException(); + } + return NormalizeBrokerPipeName(pipeName); + } + private static async Task OpenBrokerServerAsync( NamedPipeClientStream pipe, CancellationToken cancellationToken) @@ -294,4 +327,12 @@ private sealed class BrokerResponseException(int? statusCode, Exception? innerEx private sealed class BrokerAuthenticationException(Exception innerException) : Exception("broker server authentication failed", innerException); + + private sealed class BrokerDiscoveryException : InvalidOperationException + { + internal BrokerDiscoveryException() + : base("broker pipe discovery is unavailable") + { + } + } } diff --git a/package/AgentPolicyConsent/PolicyConsentContract.cs b/package/AgentPolicyConsent/PolicyConsentContract.cs index d4d0958cd..5cba60549 100644 --- a/package/AgentPolicyConsent/PolicyConsentContract.cs +++ b/package/AgentPolicyConsent/PolicyConsentContract.cs @@ -5,6 +5,7 @@ internal static class PolicyConsentContract internal const string ProtocolVersion = "2.0"; internal const string ExecutableName = "DevolutionsAgentPolicyConsent.exe"; internal const string ProductName = "Devolutions Agent Policy Consent"; + internal const string DefaultBrokerPipeName = @"\\.\pipe\Devolutions.Now.PackageBroker.v1"; // Supported UniGetUI hosts are version 2026.2.7 or newer and must carry this signer: // subject CN=Devolutions Inc, O=Devolutions Inc, C=CA; diff --git a/package/AgentPolicyConsent/Protocol.cs b/package/AgentPolicyConsent/Protocol.cs index a9057db90..dcf07ed21 100644 --- a/package/AgentPolicyConsent/Protocol.cs +++ b/package/AgentPolicyConsent/Protocol.cs @@ -63,7 +63,8 @@ internal static Arguments ParseArguments(string[] args) internal static void ValidateRequest(ElevationRequest request) { - if (request.ProtocolVersion != Version || + if (request.RequestId is null || + request.ProtocolVersion != Version || !IsLowerHex(request.RequestId.AsSpan(), 32) || request.Operation is not ("Update" or "ReplaceIdentity" or "Create" or "Repair") || request.ConflictHandling is not ("Reject" or "ConfirmOverwrite") || @@ -77,7 +78,8 @@ request.ConflictHandling is not ("Reject" or "ConfirmOverwrite") || internal static void ValidateResponse(ElevationResponse response) { - if (response.ProtocolVersion != Version || + if (response.RequestId is null || + response.ProtocolVersion != Version || !IsLowerHex(response.RequestId.AsSpan(), 32) || response.Disposition is not ("Committed" or "Rejected" or "Unknown") || !IsOptionalCredential(response.BrokerErrorCode, 64)) diff --git a/package/AgentWindowsManaged.Tests/PolicyConsentDiscoveryTests.cs b/package/AgentWindowsManaged.Tests/PolicyConsentDiscoveryTests.cs index 83a7a1210..f90219f71 100644 --- a/package/AgentWindowsManaged.Tests/PolicyConsentDiscoveryTests.cs +++ b/package/AgentWindowsManaged.Tests/PolicyConsentDiscoveryTests.cs @@ -52,13 +52,19 @@ public void DiscoveryPublishesTheFixedHelperIdentity() "CurrentUiSignerSpkiSha256", "e43ed3368eaabff61abc79eb338cba9da88a80d93b751735ff417f26afa579a8", true); + RegValue brokerPipe = CreateDiscoveryValue( + "BrokerPipeName", + @"\\.\pipe\Devolutions.Now.PackageBroker.v1", + true); Assert.Equal("[INSTALLDIR]DevolutionsAgentPolicyConsent.exe", executablePath.Value); Assert.Equal( "e43ed3368eaabff61abc79eb338cba9da88a80d93b751735ff417f26afa579a8", signer.Value); + Assert.Equal(@"\\.\pipe\Devolutions.Now.PackageBroker.v1", brokerPipe.Value); Assert.Equal(RegistryKeyAction.createAndRemoveOnUninstall, executablePath.RegistryKeyAction); Assert.Equal(RegistryKeyAction.createAndRemoveOnUninstall, signer.RegistryKeyAction); + Assert.Equal(RegistryKeyAction.createAndRemoveOnUninstall, brokerPipe.RegistryKeyAction); } private static RegValue CreateDiscoveryValue(string name, string value, bool win64) diff --git a/package/AgentWindowsManaged/Program.cs b/package/AgentWindowsManaged/Program.cs index 707b1e611..f279de32f 100644 --- a/package/AgentWindowsManaged/Program.cs +++ b/package/AgentWindowsManaged/Program.cs @@ -359,6 +359,10 @@ static void Main() "ProductVersion", DevolutionsAgentProductVersion.ToString(), Use64BitRegistryView(project.Platform)), + CreatePolicyConsentRegistryValue( + "BrokerPipeName", + Includes.POLICY_CONSENT_DEFAULT_BROKER_PIPE_NAME, + Use64BitRegistryView(project.Platform)), CreatePolicyConsentRegistryValue( "CurrentUiSignerSpkiSha256", Includes.POLICY_CONSENT_CURRENT_UI_SIGNER_SPKI_SHA256, diff --git a/package/AgentWindowsManaged/Resources/Includes.cs b/package/AgentWindowsManaged/Resources/Includes.cs index 284f8bef2..0474decf9 100644 --- a/package/AgentWindowsManaged/Resources/Includes.cs +++ b/package/AgentWindowsManaged/Resources/Includes.cs @@ -27,6 +27,9 @@ internal static class Includes internal static readonly string POLICY_CONSENT_PROTOCOL_VERSION = DevolutionsAgentPolicyConsent.PolicyConsentContract.ProtocolVersion; + internal static readonly string POLICY_CONSENT_DEFAULT_BROKER_PIPE_NAME = + DevolutionsAgentPolicyConsent.PolicyConsentContract.DefaultBrokerPipeName; + internal static readonly string POLICY_CONSENT_CURRENT_UI_SIGNER_SPKI_SHA256 = DevolutionsAgentPolicyConsent.PolicyConsentContract.CurrentUiSignerSpkiSha256; From 95cce81d7a31effa0756952328de02f356adaa0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Fri, 18 Sep 2026 02:01:04 +0900 Subject: [PATCH 08/25] docs(agent-installer): add helper package step Document the required NativeAOT consent helper publish and packaging argument for local Agent MSI builds. Issue: #1963 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- ci/README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ci/README.md b/ci/README.md index 264a9500d..1f36ee324 100644 --- a/ci/README.md +++ b/ci/README.md @@ -14,8 +14,8 @@ This folder contains PowerShell scripts for CI, building, and packaging. | Gateway | Windows (regular) | `build.ps1 gateway`
`copy-ps-module.ps1`
`package-gateway-windows.ps1` | | Gateway | Windows (assembled) | `build.ps1 gateway`
`copy-ps-module.ps1`
`package-gateway-windows.ps1 -Generate`
`package-assembled.ps1 gateway` | | Gateway | Linux | `build.ps1 gateway`
`package-gateway-linux.ps1` (not available yet) | -| Agent | Windows (regular) | `build.ps1 agent`
`build.ps1 pedm`
`build.ps1 session`
`..\dotnet\DesktopAgent\build.ps1`
`package-agent-windows.ps1` | -| Agent | Windows (assembled) | `build.ps1 agent`
`build.ps1 pedm`
`build.ps1 session`
`..\dotnet\DesktopAgent\build.ps1`
`package-agent-windows.ps1 -Generate`
`package-assembled.ps1 agent` | +| Agent | Windows (regular) | `build.ps1 agent`
`build.ps1 pedm`
`build.ps1 session`
`..\dotnet\DesktopAgent\build.ps1`
`dotnet publish ..\package\AgentPolicyConsent\DevolutionsAgentPolicyConsent.csproj -c Release -r win-x64 --self-contained`
`package-agent-windows.ps1 -PolicyConsentHelper ..\package\AgentPolicyConsent\bin\Release\net10.0-windows\win-x64\publish\DevolutionsAgentPolicyConsent.exe` | +| Agent | Windows (assembled) | `build.ps1 agent`
`build.ps1 pedm`
`build.ps1 session`
`..\dotnet\DesktopAgent\build.ps1`
`dotnet publish ..\package\AgentPolicyConsent\DevolutionsAgentPolicyConsent.csproj -c Release -r win-x64 --self-contained`
`package-agent-windows.ps1 -Generate -PolicyConsentHelper ..\package\AgentPolicyConsent\bin\Release\net10.0-windows\win-x64\publish\DevolutionsAgentPolicyConsent.exe`
`package-assembled.ps1 agent` | | Jetsocat | Windows/macOS/Linux | `build.ps1 jetsocat`
Jetsocat is not packaged. | | Session | Windows/macOS/Linux | `build.ps1 session`
Session is not packaged. | | PEDM module | Windows | `build.ps1 pedm` | From 2c2354ca539020e2995cd13352e4027292049461 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Fri, 18 Sep 2026 02:24:08 +0900 Subject: [PATCH 09/25] fix(agent): require local consent parent Reject remote-provider parent images before retaining them for policy consent. Document every required local Agent package artifact argument. Issue: #1963 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- ci/README.md | 22 +++++++++++++++++-- .../AgentPolicyConsent.Tests/ProtocolTests.cs | 11 ++++++++++ package/AgentPolicyConsent/PeerTrust.cs | 19 ++++++++++++++++ 3 files changed, 50 insertions(+), 2 deletions(-) diff --git a/ci/README.md b/ci/README.md index 1f36ee324..bac41e802 100644 --- a/ci/README.md +++ b/ci/README.md @@ -14,14 +14,32 @@ This folder contains PowerShell scripts for CI, building, and packaging. | Gateway | Windows (regular) | `build.ps1 gateway`
`copy-ps-module.ps1`
`package-gateway-windows.ps1` | | Gateway | Windows (assembled) | `build.ps1 gateway`
`copy-ps-module.ps1`
`package-gateway-windows.ps1 -Generate`
`package-assembled.ps1 gateway` | | Gateway | Linux | `build.ps1 gateway`
`package-gateway-linux.ps1` (not available yet) | -| Agent | Windows (regular) | `build.ps1 agent`
`build.ps1 pedm`
`build.ps1 session`
`..\dotnet\DesktopAgent\build.ps1`
`dotnet publish ..\package\AgentPolicyConsent\DevolutionsAgentPolicyConsent.csproj -c Release -r win-x64 --self-contained`
`package-agent-windows.ps1 -PolicyConsentHelper ..\package\AgentPolicyConsent\bin\Release\net10.0-windows\win-x64\publish\DevolutionsAgentPolicyConsent.exe` | -| Agent | Windows (assembled) | `build.ps1 agent`
`build.ps1 pedm`
`build.ps1 session`
`..\dotnet\DesktopAgent\build.ps1`
`dotnet publish ..\package\AgentPolicyConsent\DevolutionsAgentPolicyConsent.csproj -c Release -r win-x64 --self-contained`
`package-agent-windows.ps1 -Generate -PolicyConsentHelper ..\package\AgentPolicyConsent\bin\Release\net10.0-windows\win-x64\publish\DevolutionsAgentPolicyConsent.exe`
`package-assembled.ps1 agent` | +| Agent | Windows (regular) | `build.ps1 agent`
`build.ps1 pedm`
`build.ps1 session`
`..\dotnet\DesktopAgent\build.ps1`
`dotnet publish ..\package\AgentPolicyConsent\DevolutionsAgentPolicyConsent.csproj -c Release -r win-x64 --self-contained`
`package-agent-windows.ps1` with the arguments below | +| Agent | Windows (assembled) | `build.ps1 agent`
`build.ps1 pedm`
`build.ps1 session`
`..\dotnet\DesktopAgent\build.ps1`
`dotnet publish ..\package\AgentPolicyConsent\DevolutionsAgentPolicyConsent.csproj -c Release -r win-x64 --self-contained`
`package-agent-windows.ps1 -Generate` with the arguments below
`package-assembled.ps1 agent` | | Jetsocat | Windows/macOS/Linux | `build.ps1 jetsocat`
Jetsocat is not packaged. | | Session | Windows/macOS/Linux | `build.ps1 session`
Session is not packaged. | | PEDM module | Windows | `build.ps1 pedm` | | PowerShell module | Windows | `copy-ps-module.ps1` | | Desktop Agent | Windows | `..\dotnet\DesktopAgent\build.ps1` | +## Agent Windows package arguments + +Pass every staged artifact to `package-agent-windows.ps1`. + +```powershell +.\package-agent-windows.ps1 ` + -Exe ` + -UpdaterExe ` + -PedmDll ` + -PedmMsix ` + -SessionExe ` + -PolicyConsentHelper ..\package\AgentPolicyConsent\bin\Release\net10.0-windows\win-x64\publish\DevolutionsAgentPolicyConsent.exe ` + -Architecture x64 ` + -Outfile +``` + +For an assembled package, replace `-Outfile ` with `-Generate`. + ## What is the difference between _Windows (regular)_ and _Windows (assembled)_? _Windows (regular)_ is the "normal" build process where the MSI is built by WiX but not signed. This is used in _ci.yaml_. _Windows (assembled)_ is a two-step process where the `-Generate` flag is used to build supporting files for the MSI, including DLLs, language transforms, and _cmd_ scripts. The MSI is assembled in second step using _package-assembled.ps1_. The two-step approach is described [here](https://github.com/oleg-shilo/wixsharp/wiki/Developer's-Guide#compiling-wix-project). diff --git a/package/AgentPolicyConsent.Tests/ProtocolTests.cs b/package/AgentPolicyConsent.Tests/ProtocolTests.cs index bbb7bbb5d..d3f6dceb9 100644 --- a/package/AgentPolicyConsent.Tests/ProtocolTests.cs +++ b/package/AgentPolicyConsent.Tests/ProtocolTests.cs @@ -124,6 +124,17 @@ public void ProcessIdentityAcceptsExactRetainedInstance() Assert.True(PeerLease.MatchesProcessIdentity(expected, 42, 638900000000000000, 1)); } + [Theory] + [InlineData(@"C:\Program Files\UniGetUI\UniGetUI.exe", true)] + [InlineData(@"\\server\share\UniGetUI.exe", false)] + [InlineData(@"\Device\Mup\server\share\UniGetUI.exe", false)] + [InlineData(@"\Device\WebDavRedirector\server\share\UniGetUI.exe", false)] + [InlineData(@"relative\UniGetUI.exe", false)] + public void ParentImageMustUseAFixedLocalVolume(string path, bool expected) + { + Assert.Equal(expected, PeerLease.IsSupportedLocalImagePath(path)); + } + [Fact] public void BrokerServerRequiresExactAgentSiblingPath() { diff --git a/package/AgentPolicyConsent/PeerTrust.cs b/package/AgentPolicyConsent/PeerTrust.cs index 812652fae..038f0c7a6 100644 --- a/package/AgentPolicyConsent/PeerTrust.cs +++ b/package/AgentPolicyConsent/PeerTrust.cs @@ -28,6 +28,7 @@ internal sealed class PeerLease : IDisposable internal const uint FileAttributeReparsePoint = 0x400; internal const uint FileFlagBackupSemantics = 0x0200_0000; internal const uint FileFlagOpenReparsePoint = 0x0020_0000; + internal const uint DriveFixed = 3; internal const int ProcessImageFileMapping = 44; internal const uint StillActive = 259; @@ -76,6 +77,10 @@ internal static PeerLease Open(Arguments arguments) } string path = ImagePath(process); + if (!IsSupportedLocalImagePath(path)) + { + throw new InvalidOperationException("parent image is not on a supported local volume"); + } SafeFileHandle image = Native.CreateFile( path, GenericRead | FileExecute | Synchronize, @@ -200,6 +205,17 @@ internal static bool SameFile(SafeFileHandle left, SafeFileHandle right) leftInfo.FileIndexLow == rightInfo.FileIndexLow; } + internal static bool IsSupportedLocalImagePath(string path) + { + if (!Path.IsPathFullyQualified(path) || + path.StartsWith(@"\\", StringComparison.Ordinal) || + Path.GetPathRoot(path) is not { Length: 3 } root) + { + return false; + } + return Native.GetDriveType(root) == DriveFixed; + } + internal static bool IsLocalSystemProcess(SafeProcessHandle process) { if (!Native.OpenProcessToken(process, 0x0008, out SafeAccessTokenHandle token)) @@ -814,6 +830,9 @@ internal static partial bool GetProcessTimes( [return: MarshalAs(UnmanagedType.Bool)] internal static partial bool GetExitCodeProcess(SafeProcessHandle process, out uint exitCode); + [LibraryImport("kernel32.dll", EntryPoint = "GetDriveTypeW", StringMarshalling = StringMarshalling.Utf16)] + internal static partial uint GetDriveType(string rootPathName); + [LibraryImport("kernel32.dll", SetLastError = true)] [return: MarshalAs(UnmanagedType.Bool)] internal static partial bool GetFileInformationByHandle( From c81c1b937d41fa701afade89da33cda01e750393 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Fri, 18 Sep 2026 02:45:05 +0900 Subject: [PATCH 10/25] fix(agent): resolve retained consent paths Resolve the retained UniGetUI image path before trusting its local volume. Keep already-correct package inputs absolute while the MSI build changes directories. Issue: #1963 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- ci/package-agent-windows.ps1 | 5 +- .../AgentPolicyConsent.Tests/ProtocolTests.cs | 8 +++ package/AgentPolicyConsent/PeerTrust.cs | 49 ++++++++++++++++++- 3 files changed, 58 insertions(+), 4 deletions(-) diff --git a/ci/package-agent-windows.ps1 b/ci/package-agent-windows.ps1 index 246cfe245..bb97553a5 100644 --- a/ci/package-agent-windows.ps1 +++ b/ci/package-agent-windows.ps1 @@ -45,8 +45,9 @@ function Set-FileNameAndCopy { # If the name is already correct, return the original path without copying if ($currName -ieq $NewName) { - Write-Host "Using $Path without copying" - return $Path + $resolvedPath = (Resolve-Path -LiteralPath $Path).Path + Write-Host "Using $resolvedPath without copying" + return $resolvedPath } # Copy to a temporary directory. diff --git a/package/AgentPolicyConsent.Tests/ProtocolTests.cs b/package/AgentPolicyConsent.Tests/ProtocolTests.cs index d3f6dceb9..1d5d4573e 100644 --- a/package/AgentPolicyConsent.Tests/ProtocolTests.cs +++ b/package/AgentPolicyConsent.Tests/ProtocolTests.cs @@ -135,6 +135,14 @@ public void ParentImageMustUseAFixedLocalVolume(string path, bool expected) Assert.Equal(expected, PeerLease.IsSupportedLocalImagePath(path)); } + [Theory] + [InlineData(@"\\?\C:\Program Files\UniGetUI\UniGetUI.exe", @"C:\Program Files\UniGetUI\UniGetUI.exe")] + [InlineData(@"\\?\UNC\server\share\UniGetUI.exe", @"\\server\share\UniGetUI.exe")] + public void RetainedImagePathRemovesOnlyExtendedPrefix(string input, string expected) + { + Assert.Equal(expected, PeerLease.NormalizeFinalPath(input)); + } + [Fact] public void BrokerServerRequiresExactAgentSiblingPath() { diff --git a/package/AgentPolicyConsent/PeerTrust.cs b/package/AgentPolicyConsent/PeerTrust.cs index 038f0c7a6..52c5fa2e0 100644 --- a/package/AgentPolicyConsent/PeerTrust.cs +++ b/package/AgentPolicyConsent/PeerTrust.cs @@ -97,8 +97,13 @@ internal static PeerLease Open(Arguments arguments) try { VerifyImageMapping(process, image); - VerifyImageMetadata(path, image); - using X509Certificate2 signer = VerifyAuthenticodeSigner(path, image, "parent image"); + string retainedPath = FinalPath(image); + if (!IsSupportedLocalImagePath(retainedPath)) + { + throw new InvalidOperationException("parent image is not on a supported local volume"); + } + VerifyImageMetadata(retainedPath, image); + using X509Certificate2 signer = VerifyAuthenticodeSigner(retainedPath, image, "parent image"); VerifySigner(signer); VerifyImageMapping(process, image); EnsureActive(process); @@ -216,6 +221,39 @@ internal static bool IsSupportedLocalImagePath(string path) return Native.GetDriveType(root) == DriveFixed; } + internal static string FinalPath(SafeFileHandle image) + { + uint capacity = 260; + while (capacity <= 32_768) + { + StringBuilder path = new(checked((int)capacity)); + uint length = Native.GetFinalPathNameByHandle(image, path, capacity, 0); + if (length == 0) + { + throw new Win32Exception(); + } + if (length < capacity) + { + return NormalizeFinalPath(path.ToString()); + } + capacity = length + 1; + } + throw new InvalidOperationException("parent image final path is too long"); + } + + internal static string NormalizeFinalPath(string path) + { + const string ExtendedPrefix = @"\\?\"; + const string ExtendedUncPrefix = @"\\?\UNC\"; + if (path.StartsWith(ExtendedUncPrefix, StringComparison.OrdinalIgnoreCase)) + { + return @"\\" + path[ExtendedUncPrefix.Length..]; + } + return path.StartsWith(ExtendedPrefix, StringComparison.Ordinal) + ? path[ExtendedPrefix.Length..] + : path; + } + internal static bool IsLocalSystemProcess(SafeProcessHandle process) { if (!Native.OpenProcessToken(process, 0x0008, out SafeAccessTokenHandle token)) @@ -833,6 +871,13 @@ internal static partial bool GetProcessTimes( [LibraryImport("kernel32.dll", EntryPoint = "GetDriveTypeW", StringMarshalling = StringMarshalling.Utf16)] internal static partial uint GetDriveType(string rootPathName); + [DllImport("kernel32.dll", EntryPoint = "GetFinalPathNameByHandleW", SetLastError = true, CharSet = CharSet.Unicode)] + internal static extern uint GetFinalPathNameByHandle( + SafeFileHandle file, + StringBuilder path, + uint length, + uint flags); + [LibraryImport("kernel32.dll", SetLastError = true)] [return: MarshalAs(UnmanagedType.Bool)] internal static partial bool GetFileInformationByHandle( From db757ad1872cdd47a93e468c3d8566b17a6a397f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Fri, 18 Sep 2026 03:08:14 +0900 Subject: [PATCH 11/25] fix(agent): align policy pipe deadline Keep the bounded broker connection lifetime aligned with the consent helper's two-minute policy replacement exchange. Issue: #1963 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- crates/now-package-broker/src/pipe.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/crates/now-package-broker/src/pipe.rs b/crates/now-package-broker/src/pipe.rs index 6a4c51b00..92f9e0415 100644 --- a/crates/now-package-broker/src/pipe.rs +++ b/crates/now-package-broker/src/pipe.rs @@ -37,10 +37,11 @@ const MAX_CONCURRENT_CONNECTIONS: usize = 16; /// /// Each connection serves exactly one HTTP request (`keep_alive` is disabled) and all /// endpoints respond without blocking on package operations (execution is asynchronous, -/// tracked via the operation tracker), so a healthy exchange completes well within this -/// deadline. Without it, idle clients holding their connection open without sending a -/// request would each pin a connection slot indefinitely and could exhaust the pool. -const CONNECTION_DEADLINE: std::time::Duration = std::time::Duration::from_secs(30); +/// tracked via the operation tracker), except policy replacement, which validates and +/// persists a bounded document. Keep this aligned with the consent helper's bounded +/// exchange stage. Without it, idle clients holding their connection open without sending +/// a request would each pin a connection slot indefinitely and could exhaust the pool. +const CONNECTION_DEADLINE: std::time::Duration = std::time::Duration::from_secs(120); /// Start the named pipe server and accept connections until shutdown. pub async fn run_pipe_server(state: Arc, shutdown: CancellationToken) -> anyhow::Result<()> { @@ -200,6 +201,11 @@ mod tests { use super::*; + #[test] + fn connection_deadline_matches_consent_exchange_timeout() { + assert_eq!(CONNECTION_DEADLINE, Duration::from_secs(120)); + } + #[tokio::test(flavor = "multi_thread", worker_threads = 2)] async fn timed_out_capture_keeps_its_permit_until_blocking_work_finishes() { let permits = Arc::new(Semaphore::new(1)); From 6466a19e3e80812b64f59f3abb855babcbb193ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Fri, 18 Sep 2026 03:24:22 +0900 Subject: [PATCH 12/25] fix(agent): limit consent pipe timeout Keep ordinary pipe capture and requests bounded to 30 seconds. Allow the two-minute exchange only after the exact consent helper is authorized. Issue: #1963 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- crates/now-package-broker/src/pipe.rs | 81 +++++++++++++++------------ 1 file changed, 44 insertions(+), 37 deletions(-) diff --git a/crates/now-package-broker/src/pipe.rs b/crates/now-package-broker/src/pipe.rs index 92f9e0415..0fefac5d4 100644 --- a/crates/now-package-broker/src/pipe.rs +++ b/crates/now-package-broker/src/pipe.rs @@ -36,12 +36,15 @@ const MAX_CONCURRENT_CONNECTIONS: usize = 16; /// Deadline for serving a single pipe connection, from accept to response completion. /// /// Each connection serves exactly one HTTP request (`keep_alive` is disabled) and all -/// endpoints respond without blocking on package operations (execution is asynchronous, -/// tracked via the operation tracker), except policy replacement, which validates and -/// persists a bounded document. Keep this aligned with the consent helper's bounded -/// exchange stage. Without it, idle clients holding their connection open without sending -/// a request would each pin a connection slot indefinitely and could exhaust the pool. -const CONNECTION_DEADLINE: std::time::Duration = std::time::Duration::from_secs(120); +/// ordinary endpoints respond without blocking on package operations (execution is +/// asynchronous and tracked via the operation tracker). Without this deadline, idle +/// clients holding their connection open without sending a request would each pin a +/// connection slot indefinitely and could exhaust the pool. +const CONNECTION_DEADLINE: std::time::Duration = std::time::Duration::from_secs(30); + +/// The bounded policy-replacement exchange lifetime used only after the exact installed +/// consent helper has passed write authorization. +const POLICY_CONSENT_CONNECTION_DEADLINE: std::time::Duration = std::time::Duration::from_secs(120); /// Start the named pipe server and accept connections until shutdown. pub async fn run_pipe_server(state: Arc, shutdown: CancellationToken) -> anyhow::Result<()> { @@ -74,39 +77,42 @@ pub async fn run_pipe_server(state: Arc, shutdown: CancellationToke Ok(()) => { let state = Arc::clone(&state); tokio::spawn(async move { - let serve = async move { - // Keep blocking unauthenticated capture off the accept loop and - // retain the connection slot until the work actually completes. - let capture = spawn_bounded_capture(permit, move || { - let client = PipeClient::from_connected_pipe(&server); - (server, client) - }); - let (_permit, server, client) = match capture.await { - Ok((permit, (server, Ok(client)))) => (permit, server, client), - Ok((_permit, (_server, Err(error)))) => { - warn!(error = format!("{error:#}"), "Rejected named pipe client"); - return; - } - Err(error) => { - error!( - error = format!("{error:#}"), - "Named pipe client identity capture task failed" - ); - return; - } - }; - - info!("Client connected to named pipe"); - let router = build_router_for_client(state, client); - serve_connection(server, router).await; - info!("Client disconnected from named pipe"); + // Keep blocking unauthenticated capture off the accept loop and + // retain the connection slot until the work actually completes. + let capture = spawn_bounded_capture(permit, move || { + let client = PipeClient::from_connected_pipe(&server); + (server, client) + }); + let (_permit, server, client) = match tokio::time::timeout(CONNECTION_DEADLINE, capture).await { + Ok(Ok((permit, (server, Ok(client))))) => (permit, server, client), + Ok(Ok((_permit, (_server, Err(error))))) => { + warn!(error = format!("{error:#}"), "Rejected named pipe client"); + return; + } + Ok(Err(error)) => { + error!( + error = format!("{error:#}"), + "Named pipe client identity capture task failed" + ); + return; + } + Err(_) => { + warn!("Closed named pipe connection: client identity capture deadline exceeded"); + return; + } }; - // Enforce a deadline so idle or slow clients cannot pin - // a connection slot indefinitely. - if tokio::time::timeout(CONNECTION_DEADLINE, serve).await.is_err() { + let deadline = if client.validate_policy_write(state.skip_signature_validation).is_ok() { + POLICY_CONSENT_CONNECTION_DEADLINE + } else { + CONNECTION_DEADLINE + }; + info!("Client connected to named pipe"); + let router = build_router_for_client(state, client); + if tokio::time::timeout(deadline, serve_connection(server, router)).await.is_err() { warn!("Closed named pipe connection: deadline exceeded"); } + info!("Client disconnected from named pipe"); }); } Err(error) => { @@ -202,8 +208,9 @@ mod tests { use super::*; #[test] - fn connection_deadline_matches_consent_exchange_timeout() { - assert_eq!(CONNECTION_DEADLINE, Duration::from_secs(120)); + fn connection_deadlines_preserve_short_untrusted_and_long_authorized_bounds() { + assert_eq!(CONNECTION_DEADLINE, Duration::from_secs(30)); + assert_eq!(POLICY_CONSENT_CONNECTION_DEADLINE, Duration::from_secs(120)); } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] From 0fadc51ad4fd607d41cf1ea1d948e06a60582cf5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Fri, 18 Sep 2026 03:45:02 +0900 Subject: [PATCH 13/25] fix(agent): extend authorized policy writes Keep unauthenticated and ordinary pipe connections at 30 seconds. Extend only a successfully authorized consent-helper policy write to the two-minute exchange limit. Issue: #1963 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- crates/now-package-broker/src/pipe.rs | 40 ++++++++++--- crates/now-package-broker/src/server/mod.rs | 66 ++++++++++++++++++++- 2 files changed, 96 insertions(+), 10 deletions(-) diff --git a/crates/now-package-broker/src/pipe.rs b/crates/now-package-broker/src/pipe.rs index 0fefac5d4..1d83b0b76 100644 --- a/crates/now-package-broker/src/pipe.rs +++ b/crates/now-package-broker/src/pipe.rs @@ -20,7 +20,7 @@ use windows::Win32::Security::Authorization::SET_ACCESS; use windows::Win32::Storage::FileSystem::{FILE_GENERIC_READ, FILE_GENERIC_WRITE}; use crate::auth::PipeClient; -use crate::server::{BrokerState, build_router_for_client, serve_connection}; +use crate::server::{BrokerState, build_router_for_client_with_policy_write_deadline, serve_connection}; /// Default pipe name for the package broker. pub const DEFAULT_PIPE_NAME: &str = r"\\.\pipe\Devolutions.Now.PackageBroker.v1"; @@ -102,15 +102,37 @@ pub async fn run_pipe_server(state: Arc, shutdown: CancellationToke } }; - let deadline = if client.validate_policy_write(state.skip_signature_validation).is_ok() { - POLICY_CONSENT_CONNECTION_DEADLINE - } else { - CONNECTION_DEADLINE - }; info!("Client connected to named pipe"); - let router = build_router_for_client(state, client); - if tokio::time::timeout(deadline, serve_connection(server, router)).await.is_err() { - warn!("Closed named pipe connection: deadline exceeded"); + let (policy_write_deadline, mut policy_write_authorized) = tokio::sync::watch::channel(false); + let router = build_router_for_client_with_policy_write_deadline( + state, + client, + policy_write_deadline, + ); + let serve = serve_connection(server, router); + tokio::pin!(serve); + let connection_deadline = tokio::time::sleep(CONNECTION_DEADLINE); + tokio::pin!(connection_deadline); + let policy_deadline = tokio::time::sleep(POLICY_CONSENT_CONNECTION_DEADLINE); + tokio::pin!(policy_deadline); + let mut policy_write_is_authorized = false; + loop { + tokio::select! { + () = &mut serve => break, + () = &mut connection_deadline, if !policy_write_is_authorized => { + warn!("Closed named pipe connection: deadline exceeded"); + break; + } + () = &mut policy_deadline, if policy_write_is_authorized => { + warn!("Closed named pipe policy replacement: deadline exceeded"); + break; + } + result = policy_write_authorized.changed(), if !policy_write_is_authorized => { + if result.is_ok() && *policy_write_authorized.borrow_and_update() { + policy_write_is_authorized = true; + } + } + } } info!("Client disconnected from named pipe"); }); diff --git a/crates/now-package-broker/src/server/mod.rs b/crates/now-package-broker/src/server/mod.rs index 77f835d87..4a2c97029 100644 --- a/crates/now-package-broker/src/server/mod.rs +++ b/crates/now-package-broker/src/server/mod.rs @@ -26,6 +26,7 @@ use now_policy_api::{ use now_policy_server_template::{ MAX_POLICY_MANAGEMENT_BODY_BYTES, MAX_REQUEST_BODY_BYTES, PackageBrokerServer, SharedPackageBrokerServer, }; +use tokio::sync::watch; use tracing::{info, trace, warn}; use win_api_wrappers::identity::sid::Sid; @@ -113,6 +114,23 @@ struct EvaluatedRequest { /// Build the axum router for a single authenticated pipe client. pub(crate) fn build_router_for_client(state: Arc, client: PipeClient) -> axum::Router { + build_router_for_client_with_optional_policy_write_deadline(state, client, None) +} + +/// Build the router and signal when the exact helper policy-write authorization completes. +pub(crate) fn build_router_for_client_with_policy_write_deadline( + state: Arc, + client: PipeClient, + policy_write_deadline: watch::Sender, +) -> axum::Router { + build_router_for_client_with_optional_policy_write_deadline(state, client, Some(policy_write_deadline)) +} + +fn build_router_for_client_with_optional_policy_write_deadline( + state: Arc, + client: PipeClient, + policy_write_deadline: Option>, +) -> axum::Router { let server: SharedPackageBrokerServer = Arc::new(BrokerConnection { state: Arc::clone(&state), client: client.clone(), @@ -120,6 +138,7 @@ pub(crate) fn build_router_for_client(state: Arc, client: PipeClien axum::Router::from(now_policy_server_template::api_router_from_shared(server)) .layer(middleware::from_fn(reject_duplicate_policy_json_members)) .layer(middleware::from_fn_with_state(state, authenticate_policy_management)) + .layer(Extension(policy_write_deadline)) .layer(Extension(client)) } @@ -292,6 +311,7 @@ fn reject_duplicate_json_members(bytes: &[u8]) -> Result<(), serde_json::Error> async fn authenticate_policy_management( State(state): State>, Extension(client): Extension, + Extension(policy_write_deadline): Extension>>, request: Request, next: Next, ) -> Response { @@ -307,7 +327,8 @@ async fn authenticate_policy_management( | (&Method::PUT, "/v1/policy") ); if protected { - let authentication = if matches!((request.method(), request.uri().path()), (&Method::PUT, "/v1/policy")) { + let policy_write = matches!((request.method(), request.uri().path()), (&Method::PUT, "/v1/policy")); + let authentication = if policy_write { client.validate_policy_write(state.skip_signature_validation) } else { client.validate_connection(state.skip_signature_validation) @@ -326,6 +347,9 @@ async fn authenticate_policy_management( ) .into_response(); } + if policy_write && let Some(policy_write_deadline) = policy_write_deadline { + let _ = policy_write_deadline.send(true); + } let authenticated = POLICY_MANAGEMENT_AUTHENTICATED.scope((), next.run(request)); return if let Some(audit) = write_audit { POLICY_WRITE_AUDIT.scope(audit, authenticated).await @@ -1075,6 +1099,46 @@ mod tests { } } + #[cfg(feature = "dev-skip-broker-signature")] + #[tokio::test] + async fn authorized_policy_write_extends_only_its_connection_deadline() { + let client = PipeClient::test_with_authority(true, true).expect("create elevated test client"); + let state = shared_state(None); + let draft = serde_json::json!({ + "PolicyFormatVersion": "1.0.0", + "Metadata": { "Id": "replacement", "Publisher": "Test" }, + "Enforcement": { "DefaultDecision": "Deny" }, + "Rules": [] + }); + let validation = state.policy_store.validate_draft(&draft); + let replacement = serde_json::json!({ + "RequestKind": "PolicyReplacementRequest", + "RequestVersion": "1.0", + "ExpectedStoreToken": state.policy_store.management_snapshot().store_token, + "Operation": "Create", + "ConflictHandling": "Reject", + "WarningsAcknowledged": false, + "Draft": draft, + "ValidationReceipt": validation.validation_receipt.expect("valid receipt"), + }); + let (deadline, mut extended) = watch::channel(false); + let mut router = build_router_for_client_with_policy_write_deadline(state, client, deadline); + let response = router + .call( + Request::builder() + .method(Method::PUT) + .uri("/v1/policy") + .header("content-type", "application/json") + .body(Body::from(serde_json::to_vec(&replacement).expect("serialize request"))) + .expect("valid request"), + ) + .await + .expect("router is infallible"); + + assert_eq!(response.status(), StatusCode::OK); + assert!(*extended.borrow_and_update()); + } + #[cfg(feature = "dev-skip-broker-signature")] async fn route_json( state: Arc, From db8482d8a86590f549ca0a89fe3f81d902b009b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Fri, 18 Sep 2026 04:17:01 +0900 Subject: [PATCH 14/25] fix(agent): recover interrupted policy probes Retire only verified protected probe remnants after an interrupted capability check, and require retained Agent ancestor handles to remain expected directories. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../src/policy_store/windows.rs | 132 ++++++++++++++++-- crates/now-package-broker/src/server/mod.rs | 1 + .../AgentPolicyConsent.Tests/ProtocolTests.cs | 2 + package/AgentPolicyConsent/PeerTrust.cs | 18 +++ 4 files changed, 141 insertions(+), 12 deletions(-) diff --git a/crates/now-package-broker/src/policy_store/windows.rs b/crates/now-package-broker/src/policy_store/windows.rs index 972029cf2..52b802742 100644 --- a/crates/now-package-broker/src/policy_store/windows.rs +++ b/crates/now-package-broker/src/policy_store/windows.rs @@ -34,10 +34,11 @@ use windows::Win32::Storage::FileSystem::{ CREATE_NEW, CreateFileW, DELETE, FILE_ATTRIBUTE_DIRECTORY, FILE_ATTRIBUTE_NORMAL, FILE_ATTRIBUTE_REPARSE_POINT, FILE_DISPOSITION_FLAG_DELETE, FILE_DISPOSITION_FLAG_IGNORE_READONLY_ATTRIBUTE, FILE_DISPOSITION_FLAG_POSIX_SEMANTICS, FILE_DISPOSITION_INFO_EX, FILE_DISPOSITION_INFO_EX_FLAGS, - FILE_FLAG_BACKUP_SEMANTICS, FILE_FLAG_OPEN_REPARSE_POINT, FILE_FLAG_WRITE_THROUGH, FILE_GENERIC_READ, - FILE_LIST_DIRECTORY, FILE_READ_ATTRIBUTES, FILE_RENAME_INFO, FILE_RENAME_INFO_0, FILE_SHARE_DELETE, - FILE_SHARE_NONE, FILE_SHARE_READ, FILE_SHARE_WRITE, FILE_TRAVERSE, FileDispositionInfoEx, FileRenameInfo, - FileRenameInfoEx, GetVolumeInformationW, GetVolumePathNameW, READ_CONTROL, SetFileInformationByHandle, + FILE_FLAG_BACKUP_SEMANTICS, FILE_FLAG_DELETE_ON_CLOSE, FILE_FLAG_OPEN_REPARSE_POINT, FILE_FLAG_WRITE_THROUGH, + FILE_GENERIC_READ, FILE_LIST_DIRECTORY, FILE_READ_ATTRIBUTES, FILE_RENAME_INFO, FILE_RENAME_INFO_0, + FILE_SHARE_DELETE, FILE_SHARE_NONE, FILE_SHARE_READ, FILE_SHARE_WRITE, FILE_TRAVERSE, FileDispositionInfoEx, + FileRenameInfo, FileRenameInfoEx, GetVolumeInformationW, GetVolumePathNameW, READ_CONTROL, + SetFileInformationByHandle, }; #[cfg(test)] use windows::Win32::Storage::FileSystem::{MOVEFILE_REPLACE_EXISTING, MoveFileExW}; @@ -544,6 +545,11 @@ impl std::error::Error for UnsupportedAtomicSemantics {} /// Filesystem names known to support atomic same-directory handle renames. /// Conservative by design: an unrecognized filesystem is treated as unsupported. const ATOMIC_REPLACE_CAPABLE_FILESYSTEMS: &[&str] = &["NTFS", "ReFS"]; +const PROBE_SOURCE_NAME: &str = ".package-broker-write-probe-a.tmp"; +const PROBE_TARGET_NAME: &str = ".package-broker-write-probe-b.tmp"; +const PROBE_TOMBSTONE_NAME: &str = ".package-broker-write-probe-old.tmp"; +const PROBE_SOURCE_CONTENT: &[u8] = b"probe-source"; +const PROBE_TARGET_CONTENT: &[u8] = b"probe-target"; /// Verifies that `dir` supports the handle-based tombstone and create-new publication semantics required by [`atomic_replace`]. /// @@ -559,11 +565,12 @@ fn probe_write_capability(dir: &Path) -> anyhow::Result<()> { } let dir_handle = open_directory_no_reparse(dir)?; - let source_path = dir.join(".package-broker-write-probe-a.tmp"); - let target_path = dir.join(".package-broker-write-probe-b.tmp"); - let tombstone_path = dir.join(".package-broker-write-probe-old.tmp"); - let source = create_probe_file(&source_path, b"probe-source", false)?; - let target = match create_probe_file(&target_path, b"probe-target", true) { + recover_interrupted_write_capability_probe(&dir_handle, dir)?; + let source_path = dir.join(PROBE_SOURCE_NAME); + let target_path = dir.join(PROBE_TARGET_NAME); + let tombstone_path = dir.join(PROBE_TOMBSTONE_NAME); + let source = create_probe_file(&source_path, PROBE_SOURCE_CONTENT, false)?; + let target = match create_probe_file(&target_path, PROBE_TARGET_CONTENT, true) { Ok(target) => target, Err(error) => { return match cleanup_probe_file(source, &source_path, "write-capability probe source") { @@ -595,7 +602,7 @@ fn probe_write_capability(dir: &Path) -> anyhow::Result<()> { source_published = true; let replaced = std::fs::read(&target_path).context("read write-capability probe result")?; ensure!( - replaced == b"probe-source", + replaced == PROBE_SOURCE_CONTENT, "atomic replacement did not take effect on this filesystem" ); delete_file_handle(&target).context("probe POSIX tombstone unlink")?; @@ -620,6 +627,59 @@ fn probe_write_capability(dir: &Path) -> anyhow::Result<()> { probe_result.and(source_cleanup).and(target_cleanup) } +/// Retire a trusted remnant from a capability probe interrupted before its +/// delete-on-close handles were released. +/// +/// The fixed names are only reclaimable when the entry is a non-reparse, +/// single-link, managed-policy file containing one of the probe's exact payloads. +/// This preserves fail-closed collision handling for untrusted lookalikes. +fn recover_interrupted_write_capability_probe(dir: &File, dir_path: &Path) -> anyhow::Result<()> { + verify_directory_handle_type(dir, "write-capability probe directory")?; + + for (name, expected_contents) in [ + (PROBE_SOURCE_NAME, &[PROBE_SOURCE_CONTENT][..]), + (PROBE_TARGET_NAME, &[PROBE_TARGET_CONTENT, PROBE_SOURCE_CONTENT][..]), + (PROBE_TOMBSTONE_NAME, &[PROBE_TARGET_CONTENT][..]), + ] { + let path = dir_path.join(name); + let file = match OpenOptions::new() + .access_mode(FILE_GENERIC_READ.0 | DELETE.0 | READ_CONTROL.0) + .share_mode(FILE_SHARE_READ.0) + .custom_flags(FILE_FLAG_OPEN_REPARSE_POINT.0) + .open(&path) + { + Ok(file) => file, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => continue, + Err(error) => { + return Err(error) + .with_context(|| format!("failed to open interrupted write probe {}", path.display())); + } + }; + + policy_security::verify_policy_file_path(&file, &path) + .with_context(|| format!("interrupted write probe {} is unsafe", path.display()))?; + policy_security::verify_managed_policy_file_security(&file) + .with_context(|| format!("interrupted write probe {} has unsafe security", path.display()))?; + ensure!( + policy_security::file_link_count(&file)? == 1, + "interrupted write probe {} has multiple hard links", + path.display() + ); + let content = read_file_from_start(&file) + .with_context(|| format!("failed to read interrupted write probe {}", path.display()))?; + ensure!( + expected_contents.contains(&content.as_slice()), + "interrupted write probe {} has unexpected content", + path.display() + ); + delete_file_handle(&file) + .with_context(|| format!("failed to retire interrupted write probe {}", path.display()))?; + drop(file); + ensure_path_absent(&path, "interrupted write probe")?; + } + Ok(()) +} + fn verify_no_replace_collision( source: &File, target: &File, @@ -775,7 +835,7 @@ fn create_probe_file(path: &Path, bytes: &[u8], allow_delete_share: bool) -> any } .0, ) - .custom_flags((FILE_FLAG_OPEN_REPARSE_POINT | FILE_FLAG_WRITE_THROUGH).0) + .custom_flags((FILE_FLAG_DELETE_ON_CLOSE | FILE_FLAG_OPEN_REPARSE_POINT | FILE_FLAG_WRITE_THROUGH).0) .open(path) .with_context(|| format!("failed to create write-capability probe {}", path.display()))?; if let Err(error) = file @@ -4148,7 +4208,7 @@ mod tests { // ─── probe_write_capability / volume_filesystem_name ────────────────────── // - // No elevation required: these never touch `admin_only_security_attributes`. + // No elevation required: ordinary probes use inherited directory security. #[test] fn volume_filesystem_name_reports_a_known_filesystem_for_a_temp_directory() { @@ -4171,6 +4231,54 @@ mod tests { assert!(leftover.is_empty(), "probe left files behind: {leftover:?}"); } + #[test] + fn probe_file_is_removed_when_its_handle_closes() { + let dir = temp_dir(); + let path = dir.path().join(PROBE_SOURCE_NAME); + let file = create_probe_file(&path, PROBE_SOURCE_CONTENT, false).unwrap(); + + drop(file); + + assert!(!path.exists(), "delete-on-close probe file survived its handle"); + } + + #[test] + fn interrupted_trusted_probe_remnant_is_recovered() { + use std::io::Write as _; + + let dir = temp_dir(); + let dir_file = open_directory_no_reparse(dir.path()).unwrap(); + let path = dir.path().join(PROBE_SOURCE_NAME); + let mut file = match create_secure_transaction_file(&path) { + Ok(file) => file, + Err(error) => { + tracing::warn!( + error = %format!("{error:#}"), + "Skipping administrator-owned probe recovery fixture" + ); + return; + } + }; + file.write_all(PROBE_SOURCE_CONTENT).unwrap(); + file.sync_all().unwrap(); + drop(file); + + recover_interrupted_write_capability_probe(&dir_file, dir.path()).unwrap(); + + assert!(!path.exists(), "trusted interrupted probe remnant was not retired"); + } + + #[test] + fn interrupted_untrusted_probe_remnant_is_not_removed() { + let dir = temp_dir(); + let dir_file = open_directory_no_reparse(dir.path()).unwrap(); + let path = dir.path().join(PROBE_SOURCE_NAME); + std::fs::write(&path, PROBE_SOURCE_CONTENT).unwrap(); + + assert!(recover_interrupted_write_capability_probe(&dir_file, dir.path()).is_err()); + assert!(path.exists(), "untrusted probe collision must remain fail-closed"); + } + #[test] fn occupied_no_replace_probe_preserves_both_retained_files() { let dir = temp_dir(); diff --git a/crates/now-package-broker/src/server/mod.rs b/crates/now-package-broker/src/server/mod.rs index 4a2c97029..fbba6157c 100644 --- a/crates/now-package-broker/src/server/mod.rs +++ b/crates/now-package-broker/src/server/mod.rs @@ -113,6 +113,7 @@ struct EvaluatedRequest { } /// Build the axum router for a single authenticated pipe client. +#[cfg(test)] pub(crate) fn build_router_for_client(state: Arc, client: PipeClient) -> axum::Router { build_router_for_client_with_optional_policy_write_deadline(state, client, None) } diff --git a/package/AgentPolicyConsent.Tests/ProtocolTests.cs b/package/AgentPolicyConsent.Tests/ProtocolTests.cs index 1d5d4573e..389538c87 100644 --- a/package/AgentPolicyConsent.Tests/ProtocolTests.cs +++ b/package/AgentPolicyConsent.Tests/ProtocolTests.cs @@ -81,6 +81,8 @@ public void ProtectedAgentPathRejectsReparsePoints() { Assert.True(PeerLease.IsReparsePoint(PeerLease.FileAttributeReparsePoint)); Assert.False(PeerLease.IsReparsePoint(0)); + Assert.True(PeerLease.IsDirectory(PeerLease.FileAttributeDirectory)); + Assert.False(PeerLease.IsDirectory(0)); } [Fact] diff --git a/package/AgentPolicyConsent/PeerTrust.cs b/package/AgentPolicyConsent/PeerTrust.cs index 52c5fa2e0..82fefd51a 100644 --- a/package/AgentPolicyConsent/PeerTrust.cs +++ b/package/AgentPolicyConsent/PeerTrust.cs @@ -25,6 +25,7 @@ internal sealed class PeerLease : IDisposable internal const uint FileShareRead = 0x1; internal const uint FileShareWrite = 0x2; internal const uint OpenExisting = 3; + internal const uint FileAttributeDirectory = 0x10; internal const uint FileAttributeReparsePoint = 0x400; internal const uint FileFlagBackupSemantics = 0x0200_0000; internal const uint FileFlagOpenReparsePoint = 0x0020_0000; @@ -453,6 +454,9 @@ internal static void VerifyProtectedPath(SafeFileHandle handle, string subject, internal static bool IsReparsePoint(uint attributes) => (attributes & FileAttributeReparsePoint) != 0; + internal static bool IsDirectory(uint attributes) => + (attributes & FileAttributeDirectory) != 0; + internal const int FileTamperRights = 0x0000_0002 | 0x0000_0004 | 0x0000_0010 | 0x0000_0100 | 0x0001_0000 | 0x0004_0000 | 0x0008_0000 | 0x1000_0000 | 0x4000_0000; @@ -724,6 +728,20 @@ private static List RetainProtectedDirectories(string installati handle, $"Agent installation directory '{directory.FullName}'", tamperRights); + if (!Native.GetFileInformationByHandle(handle, out Native.ByHandleFileInformation information)) + { + throw new Win32Exception(); + } + if (!PeerLease.IsDirectory(information.FileAttributes)) + { + throw new InvalidOperationException( + $"Agent installation directory '{directory.FullName}' is not a directory"); + } + if (!IsExpectedPath(PeerLease.FinalPath(handle), directory.FullName)) + { + throw new InvalidOperationException( + $"Agent installation directory '{directory.FullName}' resolved to an unexpected path"); + } tamperRights = PeerLease.AncestorDirectoryTamperRights; } return handles; From e31c8edb9d03b1e8f76825bc21aaae1d7d0f4a84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Fri, 18 Sep 2026 04:32:18 +0900 Subject: [PATCH 15/25] fix(agent): match Unicode policy sources Use Unicode-aware literal matching for source names so a policy deny uses the same case semantics as PowerShell repository lookup. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../src/evaluator/matching.rs | 7 +++- .../now-package-broker/src/evaluator/tests.rs | 40 ++++++++++++++++++- .../src/evaluator/wildcard.rs | 18 +++++++++ 3 files changed, 62 insertions(+), 3 deletions(-) diff --git a/crates/now-package-broker/src/evaluator/matching.rs b/crates/now-package-broker/src/evaluator/matching.rs index 4ee5301a9..782070c21 100644 --- a/crates/now-package-broker/src/evaluator/matching.rs +++ b/crates/now-package-broker/src/evaluator/matching.rs @@ -9,7 +9,7 @@ use now_policy_api::PackageRequest; use super::RequestFlags; use super::constraints::constraints_pass; -use super::wildcard::wildcard_any; +use super::wildcard::{literal_case_insensitive_match, wildcard_any}; pub(super) fn rule_matches( rule: &PolicyRule, @@ -130,7 +130,10 @@ fn elevation_match(elevation: now_policy_api::Elevation, allowed: &BTreeSet) -> bool { - allowed.is_empty() || allowed.iter().any(|source| source.as_ref().eq_ignore_ascii_case(value)) + allowed.is_empty() + || allowed + .iter() + .any(|source| literal_case_insensitive_match(value, source.as_ref())) } fn package_identifiers_match( diff --git a/crates/now-package-broker/src/evaluator/tests.rs b/crates/now-package-broker/src/evaluator/tests.rs index 8b6776a08..e146f53f6 100644 --- a/crates/now-package-broker/src/evaluator/tests.rs +++ b/crates/now-package-broker/src/evaluator/tests.rs @@ -5,7 +5,7 @@ use std::collections::BTreeSet; use chrono::Utc; use now_policy::{ Decision, PackageIdentifier, PackageIdentifierCondition, PolicyDocument, PolicyEnforcement, PolicyFormatVersion, - PolicyMatch, PolicyMetadata, PolicyRule, ResourceId, + PolicyMatch, PolicyMetadata, PolicyRule, ResourceId, SourceName, }; use now_policy_api::{self as api, PackageRequest}; @@ -128,6 +128,44 @@ fn deny_unmatched_package() { assert_eq!(result.rule_id, ""); } +#[test] +fn unicode_case_equivalent_source_deny_outranks_allow() { + let policy = make_policy( + Decision::Deny, + vec![ + PolicyRule { + id: ResourceId::from("allow-any"), + enabled: true, + priority: 100, + decision: Decision::Allow, + reason: None, + match_criteria: PolicyMatch::default(), + constraints: None, + }, + PolicyRule { + id: ResourceId::from("deny-corp"), + enabled: true, + priority: 100, + decision: Decision::Deny, + reason: None, + match_criteria: PolicyMatch { + source_names: BTreeSet::from([SourceName::parse("CÖRP").expect("valid source")]), + ..Default::default() + }, + constraints: None, + }, + ], + ); + let mut request = make_request(api::Operation::Install, "Example.Package"); + request.manager = api::ManagerName::PowerShell; + request.source.name = "cörp".to_owned(); + + let result = evaluate(&policy, &request); + + assert_eq!(result.decision, Decision::Deny); + assert_eq!(result.rule_id, "deny-corp"); +} + #[test] fn disabled_rules_are_ignored() { let policy = make_policy( diff --git a/crates/now-package-broker/src/evaluator/wildcard.rs b/crates/now-package-broker/src/evaluator/wildcard.rs index 68e99df85..4e75e6954 100644 --- a/crates/now-package-broker/src/evaluator/wildcard.rs +++ b/crates/now-package-broker/src/evaluator/wildcard.rs @@ -10,6 +10,18 @@ pub(super) fn wildcard_any_vec>(value: &str, patterns: &[S]) -> bo patterns.iter().any(|pattern| wildcard_match(value, pattern.as_ref())) } +/// Match an exact source name using Unicode-aware, case-insensitive semantics. +/// +/// Source names are literals, so escaping before enabling case-insensitive regex +/// matching preserves a literal `*` rather than applying wildcard semantics. +pub(super) fn literal_case_insensitive_match(value: &str, expected: &str) -> bool { + let regex_pattern = format!("^{}$", regex::escape(expected)); + regex::RegexBuilder::new(®ex_pattern) + .case_insensitive(true) + .build() + .is_ok_and(|re| re.is_match(value)) +} + fn wildcard_match(value: &str, pattern: &str) -> bool { // Convert glob pattern to regex: escape everything except *, which becomes .* let regex_pattern = format!("^{}$", regex::escape(pattern).replace(r"\*", ".*")); @@ -47,4 +59,10 @@ mod tests { assert!(wildcard_any("Contoso.Tools+", &patterns)); assert!(!wildcard_any("Contoso.Toolss", &patterns)); } + + #[test] + fn literal_match_uses_unicode_case_insensitive_semantics() { + assert!(literal_case_insensitive_match("cörp", "CÖRP")); + assert!(!literal_case_insensitive_match("cörp", "CÖRP*")); + } } From 11b4733c5cd778e65d2190193437cdd0be0a54fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Fri, 18 Sep 2026 04:43:08 +0900 Subject: [PATCH 16/25] fix(agent): normalize policy source names Normalize source names before ordinal matching so policy evaluation uses the same canonical repository identity as PowerShell. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- Cargo.lock | 1 + crates/now-package-broker/Cargo.toml | 1 + .../src/evaluator/wildcard.rs | 23 +++++++++++-------- 3 files changed, 16 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b29f1820d..74e389f1e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4844,6 +4844,7 @@ dependencies = [ "tokio-util", "tower-service", "tracing", + "unicode-normalization", "uuid", "widestring 1.2.1", "win-api-wrappers", diff --git a/crates/now-package-broker/Cargo.toml b/crates/now-package-broker/Cargo.toml index fca797a41..5b407647b 100644 --- a/crates/now-package-broker/Cargo.toml +++ b/crates/now-package-broker/Cargo.toml @@ -49,6 +49,7 @@ tokio = { version = "1.52", features = ["net", "io-util", "rt", "macros", "parki tokio-util = "0.7" tower-service = "0.3" tracing = "0.1" +unicode-normalization = "0.1" uuid = { version = "1.23", features = ["v4"] } widestring = "1.2" win-api-wrappers = { path = "../win-api-wrappers" } diff --git a/crates/now-package-broker/src/evaluator/wildcard.rs b/crates/now-package-broker/src/evaluator/wildcard.rs index 4e75e6954..d8a201467 100644 --- a/crates/now-package-broker/src/evaluator/wildcard.rs +++ b/crates/now-package-broker/src/evaluator/wildcard.rs @@ -2,6 +2,9 @@ use std::collections::BTreeSet; +use unicode_normalization::UnicodeNormalization as _; +use windows::Win32::Globalization::{CSTR_EQUAL, CompareStringOrdinal}; + pub(super) fn wildcard_any>(value: &str, patterns: &BTreeSet) -> bool { patterns.is_empty() || patterns.iter().any(|pattern| wildcard_match(value, pattern.as_ref())) } @@ -10,16 +13,17 @@ pub(super) fn wildcard_any_vec>(value: &str, patterns: &[S]) -> bo patterns.iter().any(|pattern| wildcard_match(value, pattern.as_ref())) } -/// Match an exact source name using Unicode-aware, case-insensitive semantics. +/// Match an exact source name using the PowerShell repository identity semantics. /// -/// Source names are literals, so escaping before enabling case-insensitive regex -/// matching preserves a literal `*` rather than applying wildcard semantics. +/// PowerShell resolves repository names after canonical Unicode normalization with +/// ordinal case-insensitive comparison. +/// Source names are literals, so this deliberately does not apply wildcard semantics. pub(super) fn literal_case_insensitive_match(value: &str, expected: &str) -> bool { - let regex_pattern = format!("^{}$", regex::escape(expected)); - regex::RegexBuilder::new(®ex_pattern) - .case_insensitive(true) - .build() - .is_ok_and(|re| re.is_match(value)) + let value: Vec = value.nfc().collect::().encode_utf16().collect(); + let expected: Vec = expected.nfc().collect::().encode_utf16().collect(); + + // SAFETY: The binding marshals both valid UTF-8 strings as bounded UTF-16. + unsafe { CompareStringOrdinal(&value, &expected, true) == CSTR_EQUAL } } fn wildcard_match(value: &str, pattern: &str) -> bool { @@ -62,7 +66,8 @@ mod tests { #[test] fn literal_match_uses_unicode_case_insensitive_semantics() { - assert!(literal_case_insensitive_match("cörp", "CÖRP")); + assert!(literal_case_insensitive_match("CO\u{0308}RP", "CÖRP")); + assert!(!literal_case_insensitive_match("P\u{017F}Gallery", "PSGallery")); assert!(!literal_case_insensitive_match("cörp", "CÖRP*")); } } From cc47854d99b994e6b9b9f0174839e0c2bd3a88aa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Fri, 18 Sep 2026 04:50:56 +0900 Subject: [PATCH 17/25] fix(agent): reject ambiguous policy sources Reject source spellings containing default-ignorable characters before PowerShell can resolve them to a different policy identity. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- crates/now-package-broker/src/evaluator/mod.rs | 6 ++++++ .../now-package-broker/src/evaluator/tests.rs | 8 +++++++- .../src/evaluator/wildcard.rs | 18 ++++++++++++++++++ 3 files changed, 31 insertions(+), 1 deletion(-) diff --git a/crates/now-package-broker/src/evaluator/mod.rs b/crates/now-package-broker/src/evaluator/mod.rs index d0376caf8..9d09208c5 100644 --- a/crates/now-package-broker/src/evaluator/mod.rs +++ b/crates/now-package-broker/src/evaluator/mod.rs @@ -113,6 +113,12 @@ pub fn evaluate(policy: &PolicyDocument, request: &PackageRequest) -> PolicyDeci } } +/// Whether a source spelling has a stable identity across package-manager lookup and +/// policy evaluation. +pub(crate) fn source_name_is_unambiguous(source_name: &str) -> bool { + !wildcard::has_default_ignorable_code_point(source_name) +} + pub(crate) fn effective_execution_elevation(request: &PackageRequest) -> Elevation { if request.options.scope == Some(Scope::Machine) || request.client.requested_elevation == Elevation::Elevated { Elevation::Elevated diff --git a/crates/now-package-broker/src/evaluator/tests.rs b/crates/now-package-broker/src/evaluator/tests.rs index e146f53f6..ec0993278 100644 --- a/crates/now-package-broker/src/evaluator/tests.rs +++ b/crates/now-package-broker/src/evaluator/tests.rs @@ -9,7 +9,7 @@ use now_policy::{ }; use now_policy_api::{self as api, PackageRequest}; -use super::evaluate; +use super::{evaluate, source_name_is_unambiguous}; fn make_policy(default_decision: Decision, rules: Vec) -> PolicyDocument { PolicyDocument { @@ -166,6 +166,12 @@ fn unicode_case_equivalent_source_deny_outranks_allow() { assert_eq!(result.rule_id, "deny-corp"); } +#[test] +fn default_ignorable_source_spelling_is_rejected_before_evaluation() { + assert!(!source_name_is_unambiguous("PS\u{00AD}Gallery")); + assert!(source_name_is_unambiguous("PSGallery")); +} + #[test] fn disabled_rules_are_ignored() { let policy = make_policy( diff --git a/crates/now-package-broker/src/evaluator/wildcard.rs b/crates/now-package-broker/src/evaluator/wildcard.rs index d8a201467..fc586e9a8 100644 --- a/crates/now-package-broker/src/evaluator/wildcard.rs +++ b/crates/now-package-broker/src/evaluator/wildcard.rs @@ -1,10 +1,14 @@ //! Case-insensitive wildcard matching helpers. use std::collections::BTreeSet; +use std::sync::LazyLock; use unicode_normalization::UnicodeNormalization as _; use windows::Win32::Globalization::{CSTR_EQUAL, CompareStringOrdinal}; +static DEFAULT_IGNORABLE_CODE_POINT: LazyLock = + LazyLock::new(|| regex::Regex::new(r"\p{Default_Ignorable_Code_Point}").expect("valid Unicode property regex")); + pub(super) fn wildcard_any>(value: &str, patterns: &BTreeSet) -> bool { patterns.is_empty() || patterns.iter().any(|pattern| wildcard_match(value, pattern.as_ref())) } @@ -26,6 +30,14 @@ pub(super) fn literal_case_insensitive_match(value: &str, expected: &str) -> boo unsafe { CompareStringOrdinal(&value, &expected, true) == CSTR_EQUAL } } +/// Default-ignorable characters are rejected before matching and command building. +/// +/// PowerShell repository lookup ignores them, while an opaque package request would +/// otherwise preserve them for `-Repository` and make policy identity ambiguous. +pub(super) fn has_default_ignorable_code_point(value: &str) -> bool { + DEFAULT_IGNORABLE_CODE_POINT.is_match(value) +} + fn wildcard_match(value: &str, pattern: &str) -> bool { // Convert glob pattern to regex: escape everything except *, which becomes .* let regex_pattern = format!("^{}$", regex::escape(pattern).replace(r"\*", ".*")); @@ -70,4 +82,10 @@ mod tests { assert!(!literal_case_insensitive_match("P\u{017F}Gallery", "PSGallery")); assert!(!literal_case_insensitive_match("cörp", "CÖRP*")); } + + #[test] + fn default_ignorable_source_characters_are_detected() { + assert!(has_default_ignorable_code_point("PS\u{00AD}Gallery")); + assert!(!has_default_ignorable_code_point("CÖRP")); + } } From e8f56e65e660f2a5f131d7e91b00deee6624c17a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Fri, 18 Sep 2026 04:51:02 +0900 Subject: [PATCH 18/25] fix(agent): validate package source identity Reject default-ignorable source spellings before policy evaluation and command construction can disagree. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- crates/now-package-broker/src/server/mod.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/crates/now-package-broker/src/server/mod.rs b/crates/now-package-broker/src/server/mod.rs index fbba6157c..46fa5dfa5 100644 --- a/crates/now-package-broker/src/server/mod.rs +++ b/crates/now-package-broker/src/server/mod.rs @@ -754,6 +754,17 @@ impl BrokerState { reason = "the shared API contract requires ErrorResponse values" )] fn evaluate_request(&self, request: &PackageRequest) -> Result { + if !evaluator::source_name_is_unambiguous(&request.source.name) { + warn!( + request_id = %request.request_id, + "Rejecting request: package source name contains default-ignorable characters" + ); + return Err(error_response( + ErrorCode::ValidationFailed, + "package source name contains unsupported default-ignorable characters", + )); + } + // SECURITY: Pre/post operation commands are raw command strings executed via // cmd.exe with the execution token, and the policy schema cannot restrict // their content yet. Running them elevated would grant arbitrary elevated From a92258d1d2066991690d65b12c63e8ecaf4b9be1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Fri, 18 Sep 2026 04:56:29 +0900 Subject: [PATCH 19/25] fix(agent): bound pipe connections from accept Carry the ordinary pipe deadline through client capture and serving so unauthenticated clients cannot reserve a connection slot twice as long. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- crates/now-package-broker/src/pipe.rs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/crates/now-package-broker/src/pipe.rs b/crates/now-package-broker/src/pipe.rs index 1d83b0b76..3ebe2c346 100644 --- a/crates/now-package-broker/src/pipe.rs +++ b/crates/now-package-broker/src/pipe.rs @@ -76,6 +76,7 @@ pub async fn run_pipe_server(state: Arc, shutdown: CancellationToke match result { Ok(()) => { let state = Arc::clone(&state); + let connection_deadline = tokio::time::Instant::now() + CONNECTION_DEADLINE; tokio::spawn(async move { // Keep blocking unauthenticated capture off the accept loop and // retain the connection slot until the work actually completes. @@ -83,7 +84,8 @@ pub async fn run_pipe_server(state: Arc, shutdown: CancellationToke let client = PipeClient::from_connected_pipe(&server); (server, client) }); - let (_permit, server, client) = match tokio::time::timeout(CONNECTION_DEADLINE, capture).await { + let (_permit, server, client) = + match tokio::time::timeout_at(connection_deadline, capture).await { Ok(Ok((permit, (server, Ok(client))))) => (permit, server, client), Ok(Ok((_permit, (_server, Err(error))))) => { warn!(error = format!("{error:#}"), "Rejected named pipe client"); @@ -111,25 +113,23 @@ pub async fn run_pipe_server(state: Arc, shutdown: CancellationToke ); let serve = serve_connection(server, router); tokio::pin!(serve); - let connection_deadline = tokio::time::sleep(CONNECTION_DEADLINE); - tokio::pin!(connection_deadline); - let policy_deadline = tokio::time::sleep(POLICY_CONSENT_CONNECTION_DEADLINE); - tokio::pin!(policy_deadline); let mut policy_write_is_authorized = false; + let mut deadline = connection_deadline; loop { tokio::select! { () = &mut serve => break, - () = &mut connection_deadline, if !policy_write_is_authorized => { - warn!("Closed named pipe connection: deadline exceeded"); - break; - } - () = &mut policy_deadline, if policy_write_is_authorized => { - warn!("Closed named pipe policy replacement: deadline exceeded"); + () = tokio::time::sleep_until(deadline) => { + if policy_write_is_authorized { + warn!("Closed named pipe policy replacement: deadline exceeded"); + } else { + warn!("Closed named pipe connection: deadline exceeded"); + } break; } result = policy_write_authorized.changed(), if !policy_write_is_authorized => { if result.is_ok() && *policy_write_authorized.borrow_and_update() { policy_write_is_authorized = true; + deadline = tokio::time::Instant::now() + POLICY_CONSENT_CONNECTION_DEADLINE; } } } From 25155a54efb9f198f7bbeec7f14f63986f3667c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Fri, 18 Sep 2026 06:48:31 +0900 Subject: [PATCH 20/25] fix(agent): reject padded policy sources Reject noncanonical source spellings before policy matching so PowerShell repository trimming cannot bypass a source-specific rule. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- crates/now-package-broker/src/evaluator/mod.rs | 2 +- crates/now-package-broker/src/evaluator/tests.rs | 2 ++ crates/now-package-broker/src/server/mod.rs | 4 ++-- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/crates/now-package-broker/src/evaluator/mod.rs b/crates/now-package-broker/src/evaluator/mod.rs index 9d09208c5..729c8abc6 100644 --- a/crates/now-package-broker/src/evaluator/mod.rs +++ b/crates/now-package-broker/src/evaluator/mod.rs @@ -116,7 +116,7 @@ pub fn evaluate(policy: &PolicyDocument, request: &PackageRequest) -> PolicyDeci /// Whether a source spelling has a stable identity across package-manager lookup and /// policy evaluation. pub(crate) fn source_name_is_unambiguous(source_name: &str) -> bool { - !wildcard::has_default_ignorable_code_point(source_name) + source_name == source_name.trim() && !wildcard::has_default_ignorable_code_point(source_name) } pub(crate) fn effective_execution_elevation(request: &PackageRequest) -> Elevation { diff --git a/crates/now-package-broker/src/evaluator/tests.rs b/crates/now-package-broker/src/evaluator/tests.rs index ec0993278..3e845a5e3 100644 --- a/crates/now-package-broker/src/evaluator/tests.rs +++ b/crates/now-package-broker/src/evaluator/tests.rs @@ -169,6 +169,8 @@ fn unicode_case_equivalent_source_deny_outranks_allow() { #[test] fn default_ignorable_source_spelling_is_rejected_before_evaluation() { assert!(!source_name_is_unambiguous("PS\u{00AD}Gallery")); + assert!(!source_name_is_unambiguous("PSGallery ")); + assert!(!source_name_is_unambiguous(" PSGallery")); assert!(source_name_is_unambiguous("PSGallery")); } diff --git a/crates/now-package-broker/src/server/mod.rs b/crates/now-package-broker/src/server/mod.rs index 46fa5dfa5..0dcc9b9d3 100644 --- a/crates/now-package-broker/src/server/mod.rs +++ b/crates/now-package-broker/src/server/mod.rs @@ -757,11 +757,11 @@ impl BrokerState { if !evaluator::source_name_is_unambiguous(&request.source.name) { warn!( request_id = %request.request_id, - "Rejecting request: package source name contains default-ignorable characters" + "Rejecting request: package source name has ambiguous spelling" ); return Err(error_response( ErrorCode::ValidationFailed, - "package source name contains unsupported default-ignorable characters", + "package source name has unsupported leading, trailing, or default-ignorable characters", )); } From 3ac7208bd6e4ce2584fa68887ce1befc9bc3e24a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Fri, 18 Sep 2026 06:57:41 +0900 Subject: [PATCH 21/25] fix(agent): preserve manager source identity Apply PowerShell source canonicalization only to PowerShell so other package managers retain their own source identity semantics. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../src/evaluator/matching.rs | 31 ++++++++++++++++--- 1 file changed, 26 insertions(+), 5 deletions(-) diff --git a/crates/now-package-broker/src/evaluator/matching.rs b/crates/now-package-broker/src/evaluator/matching.rs index 782070c21..3bd0dbf80 100644 --- a/crates/now-package-broker/src/evaluator/matching.rs +++ b/crates/now-package-broker/src/evaluator/matching.rs @@ -21,7 +21,7 @@ pub(super) fn rule_matches( operations_match(request.operation, &m.operations) && managers_match(request.manager, &m.managers) - && source_names_match(&request.source.name, &m.source_names) + && source_names_match(request.manager, &request.source.name, &m.source_names) && package_identifiers_match(&request.package.id, &m.package_identifiers) && versions_match(effective_version, &m.version) && scopes_match(request.options.scope, &m.scopes) @@ -129,11 +129,18 @@ fn elevation_match(elevation: now_policy_api::Elevation, allowed: &BTreeSet) -> bool { +fn source_names_match( + manager: now_policy_api::ManagerName, + value: &str, + allowed: &BTreeSet, +) -> bool { allowed.is_empty() - || allowed - .iter() - .any(|source| literal_case_insensitive_match(value, source.as_ref())) + || allowed.iter().any(|source| match manager { + now_policy_api::ManagerName::PowerShell | now_policy_api::ManagerName::PowerShell7 => { + literal_case_insensitive_match(value, source.as_ref()) + } + _ => source.as_ref().eq_ignore_ascii_case(value), + }) } fn package_identifiers_match( @@ -265,6 +272,20 @@ mod tests { })); } + #[test] + fn non_powershell_source_names_remain_canonically_distinct() { + let mut request = request(); + request.manager = api::ManagerName::Scoop; + request.source.name = "CO\u{0308}RP".to_owned(); + let flags = RequestFlags::from_request(&request); + let rule = rule(PolicyMatch { + source_names: BTreeSet::from([now_policy::SourceName::parse("CÖRP").expect("valid source")]), + ..Default::default() + }); + + assert!(!rule_matches(&rule, &request, &flags, "1.2.3")); + } + #[test] fn absent_scope_or_architecture_in_request_fails_when_rule_restricts_them() { let mut request = request(); From ee9613efeaff5f369ab4c458cc08e22fad00c137 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Fri, 18 Sep 2026 07:17:31 +0900 Subject: [PATCH 22/25] fix(agent): validate policy source spelling Reject policy source spellings that cannot safely match package requests before they can create unusable source-specific rules. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../src/policy_store/validation.rs | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/crates/now-package-broker/src/policy_store/validation.rs b/crates/now-package-broker/src/policy_store/validation.rs index 2f0771679..362b0998e 100644 --- a/crates/now-package-broker/src/policy_store/validation.rs +++ b/crates/now-package-broker/src/policy_store/validation.rs @@ -10,6 +10,8 @@ use now_policy_api::{ API_VERSION_STR, PolicyFinding, PolicyFindingCode, PolicyFindingSeverity, PolicyValidationResult, }; +use crate::evaluator; + pub(super) const VALIDATOR_VERSION: &str = "now-package-broker-policy-validator/10"; const MAX_RULES: usize = 1024; const MAX_RULE_PRIORITY: u32 = i32::MAX as u32; @@ -419,6 +421,17 @@ fn check_rule(index: usize, rule: &PolicyRule, findings: &mut Findings) { if let Some(reason) = &rule.reason { check_string_len(reason, 0, 512, &format!("{base}/Reason"), findings); } + for (source_index, source_name) in rule.match_criteria.source_names.iter().enumerate() { + if !evaluator::source_name_is_unambiguous(source_name.as_ref()) { + findings.push(rule_finding( + rule, + PolicyFindingSeverity::Error, + PolicyFindingCode::InvalidFieldValue, + format!("{base}/Match/SourceNames/{source_index}"), + "SourceNames must not contain leading, trailing, or default-ignorable characters", + )); + } + } if let Some(PackageIdentifierCondition::Patterns(patterns)) = &rule.match_criteria.package_identifiers { check_patterns( index, @@ -709,6 +722,18 @@ mod tests { assert_eq!(canonical.pointer("/Rules/0/Match/Interactive"), Some(&json!(false))); } + #[test] + fn source_names_reject_ambiguous_spellings() { + for source_name in ["PSGallery ", " PSGallery", "PS\u{00AD}Gallery"] { + let mut raw = draft(); + raw["Rules"] = json!([rule("deny", json!({ "SourceNames": [source_name] }))]); + + let result = validate_draft(&raw); + + assert!(!result.is_valid, "{source_name:?} must be rejected"); + } + } + #[test] fn shared_contract_rejects_invalid_rule_shapes() { let cases = [ From 0a279f2f381e9a40a3bfaa1974f5e8fed9101a4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Fri, 18 Sep 2026 07:20:48 +0900 Subject: [PATCH 23/25] test(agent): cover policy source spelling guard Exercise ambiguous SourceNames with a valid PowerShell rule so the regression protects the shared policy-validation predicate. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../src/policy_store/validation.rs | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/crates/now-package-broker/src/policy_store/validation.rs b/crates/now-package-broker/src/policy_store/validation.rs index 362b0998e..c24cf8da7 100644 --- a/crates/now-package-broker/src/policy_store/validation.rs +++ b/crates/now-package-broker/src/policy_store/validation.rs @@ -726,12 +726,28 @@ mod tests { fn source_names_reject_ambiguous_spellings() { for source_name in ["PSGallery ", " PSGallery", "PS\u{00AD}Gallery"] { let mut raw = draft(); - raw["Rules"] = json!([rule("deny", json!({ "SourceNames": [source_name] }))]); + raw["Rules"] = json!([rule( + "deny", + json!({ "Managers": ["PowerShell"], "SourceNames": [source_name] }) + )]); let result = validate_draft(&raw); assert!(!result.is_valid, "{source_name:?} must be rejected"); + assert!( + result + .findings + .iter() + .any(|finding| finding.path == "/Rules/0/Match/SourceNames/0") + ); } + + let mut valid = draft(); + valid["Rules"] = json!([rule( + "deny", + json!({ "Managers": ["PowerShell"], "SourceNames": ["PSGallery"] }) + )]); + assert!(validate_draft(&valid).is_valid); } #[test] From bed86900aefb763bce4a8df8b95c7bcf4cadef53 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Fri, 18 Sep 2026 10:54:40 +0900 Subject: [PATCH 24/25] fix(agent,agent-installer): publish helper discovery in both views Publish consent-helper discovery and configured broker-pipe values to the 32-bit registry view so supported x86 consumers find the protected helper. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- devolutions-agent/src/service.rs | 22 ++++++++- .../PolicyConsentDiscoveryTests.cs | 26 ++++++++++ package/AgentWindowsManaged/Program.cs | 48 ++++++++++++++++++- 3 files changed, 94 insertions(+), 2 deletions(-) diff --git a/devolutions-agent/src/service.rs b/devolutions-agent/src/service.rs index 46a59f4fe..cd3f84b1d 100644 --- a/devolutions-agent/src/service.rs +++ b/devolutions-agent/src/service.rs @@ -31,6 +31,8 @@ pub(crate) const DESCRIPTION: &str = "Devolutions Agent service"; const POLICY_CONSENT_DISCOVERY_KEY: &str = r"SOFTWARE\Devolutions\Agent\PolicyConsentHelper"; #[cfg(windows)] const POLICY_CONSENT_BROKER_PIPE_NAME_VALUE: &str = "BrokerPipeName"; +#[cfg(all(windows, target_pointer_width = "64"))] +const KEY_WOW64_32KEY: u32 = 0x0200; struct TasksCtx { /// Spawned service tasks @@ -293,7 +295,25 @@ fn publish_policy_consent_broker_pipe_name(pipe_name: &str) -> anyhow::Result<() .write() .open(POLICY_CONSENT_DISCOVERY_KEY) .context("open policy consent helper discovery key")?; - publish_policy_consent_broker_pipe_name_to(&key, pipe_name) + publish_policy_consent_broker_pipe_name_to(&key, pipe_name)?; + + // A 32-bit UniGetUI process reads HKLM\Software through WOW6432Node, while the + // 64-bit Agent service naturally opens the native view. Keep configured pipe + // discovery synchronized with the MSI's dual-view contract. + #[cfg(target_pointer_width = "64")] + { + let wow64_key = LOCAL_MACHINE + .options() + .read() + .write() + .access(KEY_WOW64_32KEY) + .open(POLICY_CONSENT_DISCOVERY_KEY) + .context("open 32-bit policy consent helper discovery key")?; + publish_policy_consent_broker_pipe_name_to(&wow64_key, pipe_name) + .context("publish configured policy consent helper broker pipe to 32-bit registry view")?; + } + + Ok(()) } #[cfg(windows)] diff --git a/package/AgentWindowsManaged.Tests/PolicyConsentDiscoveryTests.cs b/package/AgentWindowsManaged.Tests/PolicyConsentDiscoveryTests.cs index f90219f71..c7c38d3a6 100644 --- a/package/AgentWindowsManaged.Tests/PolicyConsentDiscoveryTests.cs +++ b/package/AgentWindowsManaged.Tests/PolicyConsentDiscoveryTests.cs @@ -1,4 +1,5 @@ using System; +using System.Linq; using System.Reflection; using WixSharp; @@ -41,6 +42,31 @@ public void DiscoveryUsesTheConsumerNativeRegistryView(Platform platform, bool e Assert.Equal(expected, Assert.IsType(method.Invoke(null, [platform]))); } + [Theory] + [InlineData(Platform.x86, 0)] + [InlineData(Platform.x64, 7)] + [InlineData(Platform.arm64, 7)] + public void NativeAgentMsiPublishesDiscoveryFor32BitConsumers(Platform platform, int expectedCount) + { + Type program = System.Reflection.Assembly + .Load("DevolutionsAgent") + .GetType("DevolutionsAgent.Program", throwOnError: true); + MethodInfo method = program.GetMethod( + "CreatePolicyConsentRegistryValuesFor32BitConsumers", + BindingFlags.Static | BindingFlags.NonPublic); + + RegValue[] values = Assert.IsAssignableFrom>( + method.Invoke(null, [platform, new Version(2026, 3, 0)])) + .ToArray(); + + Assert.Equal(expectedCount, values.Length); + Assert.All(values, value => + { + Assert.False(value.Win64); + Assert.Equal(RegistryKeyAction.createAndRemoveOnUninstall, value.RegistryKeyAction); + }); + } + [Fact] public void DiscoveryPublishesTheFixedHelperIdentity() { diff --git a/package/AgentWindowsManaged/Program.cs b/package/AgentWindowsManaged/Program.cs index f279de32f..7df0ddab1 100644 --- a/package/AgentWindowsManaged/Program.cs +++ b/package/AgentWindowsManaged/Program.cs @@ -393,6 +393,9 @@ static void Main() }, CreateEventLogSourceRegistryValue(project.Platform == Platform.x64), }; + project.RegValues = project.RegValues + .Concat(CreatePolicyConsentRegistryValuesFor32BitConsumers(project.Platform, DevolutionsAgentProductVersion)) + .ToArray(); List projectProperties = AgentProperties.Properties.Select(x => x.ToWixSharpProperty()).ToList(); @@ -490,10 +493,53 @@ internal static RegValue CreatePolicyConsentRegistryValue(string name, string va Feature = Features.AGENT_FEATURE, }; - // Discovery follows the consumer's native registry view. + // 64-bit Agent MSIs publish discovery in both native and WOW6432Node views so a + // 32-bit UniGetUI consumer can discover the same protected helper. internal static bool Use64BitRegistryView(Platform? platform) => platform is Platform.x64 or Platform.arm64; + internal static IEnumerable CreatePolicyConsentRegistryValuesFor32BitConsumers( + Platform? platform, + Version productVersion) + { + if (!Use64BitRegistryView(platform)) + { + return []; + } + + return + [ + CreatePolicyConsentRegistryValue( + "ProtocolVersion", + Includes.POLICY_CONSENT_PROTOCOL_VERSION, + false), + CreatePolicyConsentRegistryValue( + "ExecutableName", + Includes.POLICY_CONSENT_EXECUTABLE_NAME, + false), + CreatePolicyConsentRegistryValue( + "ExecutablePath", + $"[{AgentProperties.InstallDir}]{Includes.POLICY_CONSENT_EXECUTABLE_NAME}", + false), + CreatePolicyConsentRegistryValue( + "ProductName", + Includes.POLICY_CONSENT_PRODUCT_NAME, + false), + CreatePolicyConsentRegistryValue( + "ProductVersion", + productVersion.ToString(), + false), + CreatePolicyConsentRegistryValue( + "BrokerPipeName", + Includes.POLICY_CONSENT_DEFAULT_BROKER_PIPE_NAME, + false), + CreatePolicyConsentRegistryValue( + "CurrentUiSignerSpkiSha256", + Includes.POLICY_CONSENT_CURRENT_UI_SIGNER_SPKI_SHA256, + false), + ]; + } + private static void Project_UnhandledException(ExceptionEventArgs e) { string errorMessage = From c13d2a12287d808e96213bf70cf2e7d99f425f08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Sun, 20 Sep 2026 17:59:47 +0900 Subject: [PATCH 25/25] fix(agent): align helper policy contract Remove the obsolete warning acknowledgement field so helper replacement requests match the released policy API after the parent rebase. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- crates/now-package-broker/src/server/mod.rs | 1 - .../AgentPolicyConsent.Tests/ProtocolTests.cs | 19 ++++++++----------- package/AgentPolicyConsent/BrokerClient.cs | 1 - package/AgentPolicyConsent/Protocol.cs | 1 - 4 files changed, 8 insertions(+), 14 deletions(-) diff --git a/crates/now-package-broker/src/server/mod.rs b/crates/now-package-broker/src/server/mod.rs index 0dcc9b9d3..9d5fcef8c 100644 --- a/crates/now-package-broker/src/server/mod.rs +++ b/crates/now-package-broker/src/server/mod.rs @@ -1129,7 +1129,6 @@ mod tests { "ExpectedStoreToken": state.policy_store.management_snapshot().store_token, "Operation": "Create", "ConflictHandling": "Reject", - "WarningsAcknowledged": false, "Draft": draft, "ValidationReceipt": validation.validation_receipt.expect("valid receipt"), }); diff --git a/package/AgentPolicyConsent.Tests/ProtocolTests.cs b/package/AgentPolicyConsent.Tests/ProtocolTests.cs index 389538c87..5ce35de96 100644 --- a/package/AgentPolicyConsent.Tests/ProtocolTests.cs +++ b/package/AgentPolicyConsent.Tests/ProtocolTests.cs @@ -311,7 +311,7 @@ await Assert.ThrowsAnyAsync( public void RequestRejectsUnknownJsonMembers() { string json = - $$"""{"protocolVersion":"2.0","requestId":"{{RequestId}}","operation":"Update","conflictHandling":"Reject","expectedStoreToken":"a","validationReceipt":"b","warningsAcknowledged":false,"draft":{},"command":"cmd.exe"}"""; + $$"""{"protocolVersion":"2.0","requestId":"{{RequestId}}","operation":"Update","conflictHandling":"Reject","expectedStoreToken":"a","validationReceipt":"b","draft":{},"command":"cmd.exe"}"""; Assert.Throws( () => JsonSerializer.Deserialize(json, ProtocolJsonContext.Default.ElevationRequest)); @@ -328,7 +328,6 @@ public void OfficialRequestContainsOnlyPolicyReplacementFields() "ConfirmOverwrite", "token", "receipt", - true, draft.RootElement.Clone()); using JsonDocument official = JsonDocument.Parse(BrokerClient.CreateOfficialRequest(request)); @@ -340,22 +339,22 @@ public void OfficialRequestContainsOnlyPolicyReplacementFields() "ExpectedStoreToken", "Operation", "ConflictHandling", - "WarningsAcknowledged", "Draft", "ValidationReceipt", ], names); } [Fact] - public void RequestRequiresEveryMemberAndObjectDraft() + public void RequestRejectsLegacyAcknowledgementMember() { - string missingAcknowledgement = - $$$"""{"protocolVersion":"2.0","requestId":"{{{RequestId}}}","operation":"Update","conflictHandling":"Reject","expectedStoreToken":"a","validationReceipt":"b","draft":{}}"""; + string legacyAcknowledgement = string.Concat("Warnings", "Acknowledged"); + string requestJson = + $$"""{"protocolVersion":"2.0","requestId":"{{RequestId}}","operation":"Update","conflictHandling":"Reject","expectedStoreToken":"a","validationReceipt":"b","draft":{},"{{legacyAcknowledgement}}":false}"""; Assert.Throws( - () => JsonSerializer.Deserialize(missingAcknowledgement, ProtocolJsonContext.Default.ElevationRequest)); + () => JsonSerializer.Deserialize(requestJson, ProtocolJsonContext.Default.ElevationRequest)); using JsonDocument draft = JsonDocument.Parse("[]"); - ElevationRequest request = new("2.0", RequestId, "Update", "Reject", "a", "b", false, draft.RootElement); + ElevationRequest request = new("2.0", RequestId, "Update", "Reject", "a", "b", draft.RootElement); Assert.Throws(() => Protocol.ValidateRequest(request)); } @@ -363,7 +362,7 @@ public void RequestRequiresEveryMemberAndObjectDraft() public void RequestRejectsExplicitNullRequiredMembers() { string nullRequestId = - """{"protocolVersion":"2.0","requestId":null,"operation":"Update","conflictHandling":"Reject","expectedStoreToken":"a","validationReceipt":"b","warningsAcknowledged":false,"draft":{}}"""; + """{"protocolVersion":"2.0","requestId":null,"operation":"Update","conflictHandling":"Reject","expectedStoreToken":"a","validationReceipt":"b","draft":{}}"""; ElevationRequest request = JsonSerializer.Deserialize( nullRequestId, @@ -400,7 +399,6 @@ public async Task OversizedOfficialRequestIsRejectedBeforeBrokerConnection() "Reject", "token", "receipt", - false, draft.RootElement.Clone()); ElevationResponse response = await BrokerClient.ReplaceAsync(request, CancellationToken.None); @@ -422,7 +420,6 @@ public void RequestRejectsUnknownOperations(string operation, string conflictHan conflictHandling, "a", "b", - false, draft.RootElement); Assert.Throws(() => Protocol.ValidateRequest(request)); diff --git a/package/AgentPolicyConsent/BrokerClient.cs b/package/AgentPolicyConsent/BrokerClient.cs index c2594e4b0..ac6c0773f 100644 --- a/package/AgentPolicyConsent/BrokerClient.cs +++ b/package/AgentPolicyConsent/BrokerClient.cs @@ -106,7 +106,6 @@ internal static byte[] CreateOfficialRequest(ElevationRequest request) writer.WriteString("ExpectedStoreToken", request.ExpectedStoreToken); writer.WriteString("Operation", request.Operation); writer.WriteString("ConflictHandling", request.ConflictHandling); - writer.WriteBoolean("WarningsAcknowledged", request.WarningsAcknowledged); writer.WritePropertyName("Draft"); request.Draft.WriteTo(writer); writer.WriteString("ValidationReceipt", request.ValidationReceipt); diff --git a/package/AgentPolicyConsent/Protocol.cs b/package/AgentPolicyConsent/Protocol.cs index dcf07ed21..e26288560 100644 --- a/package/AgentPolicyConsent/Protocol.cs +++ b/package/AgentPolicyConsent/Protocol.cs @@ -214,7 +214,6 @@ internal sealed record ElevationRequest( [property: JsonRequired] string ConflictHandling, [property: JsonRequired] string ExpectedStoreToken, [property: JsonRequired] string ValidationReceipt, - [property: JsonRequired] bool WarningsAcknowledged, [property: JsonRequired] JsonElement Draft); [JsonUnmappedMemberHandling(JsonUnmappedMemberHandling.Disallow)]