From 1447ca2a562d9d1532a73ea46a0bf8ce8c13ec5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Tue, 6 Oct 2026 01:54:23 +0900 Subject: [PATCH 01/11] fix(agent): tighten elevated package broker operations Elevated broker operations run with the user's administrator token without a UAC prompt, so the broker now limits what they can select regardless of the policy: - Requests with an explicit user scope, typed or passed through custom parameters, run with the standard token even when elevation is requested; policy rules match that elevation. - Kill-before-operation process names without an extension get .exe appended. Names with another extension, wildcards, paths, or quotes are rejected. Processes are only terminated in the requester's session, and only the requester's own processes for elevated operations. - Elevated operations reject a custom install location that non-administrators can write, including through inherited permissions, or that goes through a reparse point. - The broker no longer uninstalls the Devolutions Agent. - The audit mode policy warning now states that elevated requests run without a UAC prompt while audit mode is enabled. Audit mode cannot override these restrictions. A sample policy rule shows how to protect other critical software from elevated uninstall. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- crates/agent-policy-tester/src/windows.rs | 4 +- .../samples/deny-risky-options.policy.json | 23 +- ...nget-endpoint-agent-uninstall.request.json | 31 ++ .../samples/scenarios/baseline.scenarios.json | 8 + .../src/command_builder/bun.rs | 9 +- .../src/command_builder/cargo.rs | 7 +- .../src/command_builder/dotnet.rs | 2 +- .../src/command_builder/npm.rs | 16 +- .../src/command_builder/pip.rs | 7 +- .../src/command_builder/scoop.rs | 9 +- .../src/command_builder/vcpkg.rs | 7 +- .../src/evaluator/builtin_rules.rs | 120 ++++++++ .../src/evaluator/identifier.rs | 9 + .../src/evaluator/matching.rs | 19 ++ .../now-package-broker/src/evaluator/mod.rs | 109 +++++++- .../now-package-broker/src/evaluator/tests.rs | 94 +++++++ crates/now-package-broker/src/executor/mod.rs | 15 +- .../src/executor/windows/mod.rs | 143 ++++++++-- .../src/executor/windows/user_env.rs | 7 +- .../now-package-broker/src/policy_security.rs | 264 +++++++++++++++++- .../src/policy_store/mod.rs | 2 +- .../src/policy_store/validation.rs | 33 ++- crates/now-package-broker/src/server/mod.rs | 184 +++++++++++- 23 files changed, 1045 insertions(+), 77 deletions(-) create mode 100644 crates/now-package-broker/src/assets/samples/requests/winget-endpoint-agent-uninstall.request.json create mode 100644 crates/now-package-broker/src/evaluator/builtin_rules.rs diff --git a/crates/agent-policy-tester/src/windows.rs b/crates/agent-policy-tester/src/windows.rs index 20fc2574de..487ab5872f 100644 --- a/crates/agent-policy-tester/src/windows.rs +++ b/crates/agent-policy-tester/src/windows.rs @@ -723,7 +723,7 @@ async fn validate_policy_by_pipe(pipe_name: &str, draft: &Value) -> anyhow::Resu let validation = validation_response.json()?["Validation"].clone(); ensure!(validation["IsValid"] == true, "policy validation failed"); ensure!( - validation["ValidatorVersion"] == "now-package-broker-policy-validator/11", + validation["ValidatorVersion"] == "now-package-broker-policy-validator/12", "unexpected validator contract" ); ensure!( @@ -1037,7 +1037,7 @@ async fn assert_invalid_draft(pipe_name: &str, draft: Value, expected_path: &str ensure!( invalid_validation.get("CanonicalDraft").is_none() && invalid_validation.get("ValidationReceipt").is_none() - && invalid_validation["ValidatorVersion"] == "now-package-broker-policy-validator/11", + && invalid_validation["ValidatorVersion"] == "now-package-broker-policy-validator/12", "invalid draft returned a canonical draft, receipt, or wrong validator version" ); ensure!( diff --git a/crates/now-package-broker/src/assets/samples/deny-risky-options.policy.json b/crates/now-package-broker/src/assets/samples/deny-risky-options.policy.json index d68db9aa90..fc9bed64c6 100644 --- a/crates/now-package-broker/src/assets/samples/deny-risky-options.policy.json +++ b/crates/now-package-broker/src/assets/samples/deny-risky-options.policy.json @@ -3,7 +3,7 @@ "Metadata": { "Id": "contoso.desktop.deny-risky-options", "Publisher": "Contoso IT", - "Revision": 2, + "Revision": 3, "PublishedAt": "2026-05-05T00:00:00Z", "Description": "Default-allow policy that blocks risky broker request options." }, @@ -65,6 +65,27 @@ "winget-fonts" ] } + }, + { + "Id": "deny.elevated-uninstall-critical-software", + "Priority": 60, + "Decision": "Deny", + "Reason": "Endpoint protection, VPN and device management agents cannot be removed through the broker.", + "Match": { + "Operations": [ + "Uninstall" + ], + "ExecutionElevation": [ + "Elevated" + ], + "PackageIdentifiers": { + "Patterns": [ + "Contoso.EndpointProtection*", + "Contoso.Vpn*", + "Contoso.DeviceManagement*" + ] + } + } } ] } diff --git a/crates/now-package-broker/src/assets/samples/requests/winget-endpoint-agent-uninstall.request.json b/crates/now-package-broker/src/assets/samples/requests/winget-endpoint-agent-uninstall.request.json new file mode 100644 index 0000000000..19ac2f5f1f --- /dev/null +++ b/crates/now-package-broker/src/assets/samples/requests/winget-endpoint-agent-uninstall.request.json @@ -0,0 +1,31 @@ +{ + "RequestKind": "PackageRequest", + "RequestVersion": "1.0", + "RequestId": "req-winget-endpoint-agent-uninstall", + "CreatedAt": "2026-05-05T12:00:00Z", + "Operation": "Uninstall", + "Manager": "Winget", + "Source": { + "Name": "winget", + "Url": "https://cdn.winget.microsoft.com/cache" + }, + "Package": { + "Id": "Contoso.EndpointProtection.Agent" + }, + "Options": { + "Scope": "Machine", + "Interactive": false, + "SkipHashCheck": false, + "PreRelease": false, + "CustomParameters": [], + "KillBeforeOperation": [] + }, + "Client": { + "RequestedElevation": "Elevated", + "EffectiveUser": "CONTOSO\\alice", + "ClientVersion": "3.2.0", + "Transport": "HttpNamedPipe", + "ClientExecutablePath": "C:\\Program Files\\Devolutions\\NOW\\now-client.exe" + }, + "IncludeCommandPreview": true +} diff --git a/crates/now-package-broker/src/assets/samples/scenarios/baseline.scenarios.json b/crates/now-package-broker/src/assets/samples/scenarios/baseline.scenarios.json index 9e8ee611ea..e05e97d9d9 100644 --- a/crates/now-package-broker/src/assets/samples/scenarios/baseline.scenarios.json +++ b/crates/now-package-broker/src/assets/samples/scenarios/baseline.scenarios.json @@ -33,6 +33,14 @@ "ExpectedDecision": "Allow", "ExpectedRuleId": "", "Tags": ["baseline", "winget", "default-allow"] + }, + { + "Id": "baseline.default-allow.critical-software-uninstall.deny", + "Policy": "deny-risky-options.policy.json", + "Request": "requests/winget-endpoint-agent-uninstall.request.json", + "ExpectedDecision": "Deny", + "ExpectedRuleId": "deny.elevated-uninstall-critical-software", + "Tags": ["baseline", "winget", "default-allow", "uninstall"] } ] } \ No newline at end of file diff --git a/crates/now-package-broker/src/command_builder/bun.rs b/crates/now-package-broker/src/command_builder/bun.rs index bb81617e00..9355ef62cd 100644 --- a/crates/now-package-broker/src/command_builder/bun.rs +++ b/crates/now-package-broker/src/command_builder/bun.rs @@ -42,14 +42,14 @@ pub fn build_bun_command(request: &PackageRequest) -> anyhow::Result } fn validate_bun_request(request: &PackageRequest) -> anyhow::Result<()> { - if request.client.requested_elevation == Elevation::Elevated { - bail!("elevated Bun package operations are not supported by the broker"); - } - if request.options.scope == Some(Scope::Machine) { bail!("machine-scope Bun package operations are not supported by the broker"); } + if crate::evaluator::effective_execution_elevation(request) == Elevation::Elevated { + bail!("elevated Bun package operations are not supported by the broker"); + } + if !request.source.name.eq_ignore_ascii_case(BUN_SOURCE_NAME) || request .source @@ -265,6 +265,7 @@ mod tests { fn bun_rejects_elevated_execution() { let mut request = make_request(); request.client.requested_elevation = Elevation::Elevated; + request.options.scope = None; let error = build_bun_command(&request).expect_err("elevation should fail"); diff --git a/crates/now-package-broker/src/command_builder/cargo.rs b/crates/now-package-broker/src/command_builder/cargo.rs index 3a2cba4db0..5d08e53217 100644 --- a/crates/now-package-broker/src/command_builder/cargo.rs +++ b/crates/now-package-broker/src/command_builder/cargo.rs @@ -61,12 +61,12 @@ pub fn build_cargo_command(request: &PackageRequest) -> anyhow::Result anyhow::Result<()> { - if request.client.requested_elevation == Elevation::Elevated { - bail!("cargo elevated operations are not supported by the broker"); - } if request.options.scope == Some(Scope::Machine) { bail!("cargo machine-scope operations are not supported by the broker"); } + if crate::evaluator::effective_execution_elevation(request) == Elevation::Elevated { + bail!("cargo elevated operations are not supported by the broker"); + } if !request.source.name.eq_ignore_ascii_case(CRATES_IO_SOURCE_NAME) { bail!("cargo package source must be crates.io"); } @@ -247,6 +247,7 @@ mod tests { fn elevated_cargo_operations_are_rejected() { let mut request = make_request(); request.client.requested_elevation = Elevation::Elevated; + request.options.scope = None; let error = build_cargo_command(&request).expect_err("elevated cargo should fail"); diff --git a/crates/now-package-broker/src/command_builder/dotnet.rs b/crates/now-package-broker/src/command_builder/dotnet.rs index 2990058dcd..6bbaec607d 100644 --- a/crates/now-package-broker/src/command_builder/dotnet.rs +++ b/crates/now-package-broker/src/command_builder/dotnet.rs @@ -64,7 +64,7 @@ pub fn build_dotnet_command(request: &PackageRequest) -> anyhow::Result anyhow::Result<()> { - if request.client.requested_elevation == now_policy_api::Elevation::Elevated + if crate::evaluator::effective_execution_elevation(request) == now_policy_api::Elevation::Elevated && !trusted_dotnet_executable_is_program_files_path() { bail!("elevated .NET tool operations require the trusted Program Files dotnet.exe path"); diff --git a/crates/now-package-broker/src/command_builder/npm.rs b/crates/now-package-broker/src/command_builder/npm.rs index 5b475b4914..a8224a5a7f 100644 --- a/crates/now-package-broker/src/command_builder/npm.rs +++ b/crates/now-package-broker/src/command_builder/npm.rs @@ -43,12 +43,12 @@ pub fn build_npm_command(request: &PackageRequest) -> anyhow::Result } fn validate_npm_request(request: &PackageRequest) -> anyhow::Result<()> { - if request.client.requested_elevation == Elevation::Elevated { - bail!("npm elevated operations are not supported by the broker"); - } if request.options.scope == Some(Scope::Machine) { bail!("npm machine-scope operations are not supported by the broker"); } + if crate::evaluator::effective_execution_elevation(request) == Elevation::Elevated { + bail!("npm elevated operations are not supported by the broker"); + } if request.source.url.is_some() { bail!("npm package sources with URLs are not supported by the broker"); } @@ -323,12 +323,22 @@ mod tests { fn elevated_requests_are_rejected() { let mut request = make_request(); request.client.requested_elevation = Elevation::Elevated; + request.options.scope = None; let error = build_npm_command(&request).expect_err("elevated npm should fail"); assert!(error.to_string().contains("elevated")); } + #[test] + fn elevated_user_scope_requests_run_with_the_standard_token() { + let mut request = make_request(); + request.client.requested_elevation = Elevation::Elevated; + request.options.scope = Some(Scope::User); + + build_npm_command(&request).expect("elevated user-scope npm runs with the standard token"); + } + #[test] fn machine_scope_requests_are_rejected() { let mut request = make_request(); diff --git a/crates/now-package-broker/src/command_builder/pip.rs b/crates/now-package-broker/src/command_builder/pip.rs index ec4888a821..2edd09eab8 100644 --- a/crates/now-package-broker/src/command_builder/pip.rs +++ b/crates/now-package-broker/src/command_builder/pip.rs @@ -52,12 +52,12 @@ pub fn build_pip_command(request: &PackageRequest) -> anyhow::Result } fn validate_pip_request(request: &PackageRequest) -> anyhow::Result<()> { - if request.client.requested_elevation == Elevation::Elevated { - bail!("pip elevated operations are not supported by the broker"); - } if request.options.scope == Some(Scope::Machine) { bail!("pip machine-scope operations are not supported by the broker"); } + if crate::evaluator::effective_execution_elevation(request) == Elevation::Elevated { + bail!("pip elevated operations are not supported by the broker"); + } if request.source.url.is_some() { bail!("pip package sources with urls are not supported by the broker"); } @@ -274,6 +274,7 @@ mod tests { fn pip_rejects_elevated_operations() { let mut request = make_request(); request.client.requested_elevation = Elevation::Elevated; + request.options.scope = None; let error = build_pip_command(&request).expect_err("elevated pip should fail"); diff --git a/crates/now-package-broker/src/command_builder/scoop.rs b/crates/now-package-broker/src/command_builder/scoop.rs index 219e4bfaca..ee0dfcbce4 100644 --- a/crates/now-package-broker/src/command_builder/scoop.rs +++ b/crates/now-package-broker/src/command_builder/scoop.rs @@ -76,14 +76,14 @@ pub fn build_scoop_command(request: &PackageRequest) -> anyhow::Result anyhow::Result<()> { - if request.client.requested_elevation == Elevation::Elevated { + if request.options.scope == Some(Scope::Machine) { + bail!("Scoop machine/global scope is not supported by the broker"); + } + if crate::evaluator::effective_execution_elevation(request) == Elevation::Elevated { bail!( "Scoop elevated execution is not supported by the broker because scoop.ps1 is resolved from the target user profile" ); } - if request.options.scope == Some(Scope::Machine) { - bail!("Scoop machine/global scope is not supported by the broker"); - } if request.source.name.trim().is_empty() { bail!("Scoop package source name is required"); } @@ -352,6 +352,7 @@ mod tests { fn elevated_execution_is_rejected() { let mut request = make_request(); request.client.requested_elevation = Elevation::Elevated; + request.options.scope = None; let error = build_scoop_command(&request).expect_err("elevated Scoop should fail"); diff --git a/crates/now-package-broker/src/command_builder/vcpkg.rs b/crates/now-package-broker/src/command_builder/vcpkg.rs index 3f868391c4..8d3b79b727 100644 --- a/crates/now-package-broker/src/command_builder/vcpkg.rs +++ b/crates/now-package-broker/src/command_builder/vcpkg.rs @@ -34,12 +34,12 @@ pub fn build_vcpkg_command(request: &PackageRequest) -> anyhow::Result anyhow::Result<()> { - if request.client.requested_elevation == Elevation::Elevated { - bail!("vcpkg elevated operations are not supported by the broker"); - } if request.options.scope == Some(Scope::Machine) { bail!("vcpkg machine-scope operations are not supported by the broker"); } + if crate::evaluator::effective_execution_elevation(request) == Elevation::Elevated { + bail!("vcpkg elevated operations are not supported by the broker"); + } if request.source.url.is_some() { bail!("vcpkg package sources with URLs are not supported by the broker"); } @@ -269,6 +269,7 @@ mod tests { fn elevated_requests_are_rejected() { let mut request = make_request(); request.client.requested_elevation = Elevation::Elevated; + request.options.scope = None; let error = build_vcpkg_command(&request).expect_err("elevated request should fail"); diff --git a/crates/now-package-broker/src/evaluator/builtin_rules.rs b/crates/now-package-broker/src/evaluator/builtin_rules.rs new file mode 100644 index 0000000000..c8f0993998 --- /dev/null +++ b/crates/now-package-broker/src/evaluator/builtin_rules.rs @@ -0,0 +1,120 @@ +//! Built-in request restrictions that apply before, and regardless of, the policy. + +use std::borrow::Cow; + +use now_policy_api::{ManagerName, Operation, PackageRequest}; + +use super::RequestFlags; +use super::identifier::selects_identifier; + +/// Packages the broker never uninstalls, by package manager and identifier. +/// +/// The Devolutions Agent hosts the broker, so removing it through the broker would remove the +/// policy enforcement with it. +/// To protect another package, add an entry for each package manager that distributes it, using +/// the identifier that manager uses for the package. +/// Entries are compared like Deny rule identifiers: letter case is ignored and decorated +/// identifiers (such as versioned specifiers) match. +const PROTECTED_PACKAGES: &[(ManagerName, &str)] = &[ + // https://github.com/microsoft/winget-pkgs/tree/master/manifests/d/Devolutions/Agent + (ManagerName::Winget, "Devolutions.Agent"), + // `package/chocolatey/devoagent/devoagent.template.nuspec` + (ManagerName::Chocolatey, "devo-agent"), +]; + +/// Whether the request uninstalls a [`PROTECTED_PACKAGES`] entry, directly or as the previous +/// version replaced by an install or update. +pub(super) fn uninstalls_protected_package(request: &PackageRequest, flags: &RequestFlags) -> bool { + let removes_package = request.operation == Operation::Uninstall || flags.has_uninstall_previous; + removes_package + && PROTECTED_PACKAGES.iter().any(|(manager, identifier)| { + *manager == request.manager && selects_identifier(request.manager, &request.package.id.0, identifier) + }) +} + +/// Kill-before-operation entry as passed to `taskkill /IM`: `.exe` is appended to a name without +/// an extension, as clients may send bare process names (`chrome`). +/// +/// Other names are returned unchanged; [`is_acceptable_kill_process_name`] rejects any other +/// extension. +pub(crate) fn normalize_kill_process_name(name: &str) -> Cow<'_, str> { + if name.is_empty() || name.contains('.') { + Cow::Borrowed(name) + } else { + Cow::Owned(format!("{name}.exe")) + } +} + +/// Whether `name` is a plain executable image name that `taskkill /IM` matches literally. +/// +/// `/IM` accepts `*` wildcards, so names must not contain wildcard characters, path separators, +/// drive or stream separators, quotes, or control characters, and must end in `.exe`. +/// Apply [`normalize_kill_process_name`] first. +pub(crate) fn is_acceptable_kill_process_name(name: &str) -> bool { + const EXTENSION: &str = ".exe"; + + let has_executable_extension = name.len() > EXTENSION.len() + && name + .get(name.len() - EXTENSION.len()..) + .is_some_and(|extension| extension.eq_ignore_ascii_case(EXTENSION)); + + has_executable_extension + && !name + .chars() + .any(|character| character.is_control() || matches!(character, '*' | '?' | '\\' | '/' | ':' | '"' | '\'')) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn plain_executable_names_are_acceptable_kill_targets() { + for name in ["Code.exe", "notepad.EXE", "My App.exe", "app-1.2.exe"] { + assert!(is_acceptable_kill_process_name(name), "{name}"); + } + } + + #[test] + fn wildcard_path_and_malformed_kill_targets_are_rejected() { + for name in [ + "", + ".exe", + "*", + "*.exe", + "a?.exe", + "notepad.exe ", + r"C:\Windows\notepad.exe", + "dir/notepad.exe", + "C:notepad.exe", + "\"notepad.exe\"", + "note'pad.exe", + "note\npad.exe", + "note\u{0}pad.exe", + "notepad.ex", + "chrome.bat", + "chrome.", + ] { + assert!( + !is_acceptable_kill_process_name(&normalize_kill_process_name(name)), + "{name:?}" + ); + } + } + + #[test] + fn names_without_an_extension_get_exe_appended() { + assert_eq!(normalize_kill_process_name("chrome"), "chrome.exe"); + assert_eq!(normalize_kill_process_name("Chrome.EXE"), "Chrome.EXE"); + assert_eq!(normalize_kill_process_name("chrome.bat"), "chrome.bat"); + assert_eq!(normalize_kill_process_name(""), ""); + assert!(is_acceptable_kill_process_name(&normalize_kill_process_name("chrome"))); + assert!(is_acceptable_kill_process_name(&normalize_kill_process_name( + "Chrome.EXE" + ))); + assert!(!is_acceptable_kill_process_name(&normalize_kill_process_name("*"))); + assert!(!is_acceptable_kill_process_name(&normalize_kill_process_name( + r"dir\chrome" + ))); + } +} diff --git a/crates/now-package-broker/src/evaluator/identifier.rs b/crates/now-package-broker/src/evaluator/identifier.rs index 372c89b930..22dde7e843 100644 --- a/crates/now-package-broker/src/evaluator/identifier.rs +++ b/crates/now-package-broker/src/evaluator/identifier.rs @@ -120,6 +120,15 @@ pub(super) fn embedded_package_names(manager: ManagerName, identifier: &str) -> } } +/// Whether the requested `value` selects the package `identifier`, with the fail-closed +/// comparison Deny rules use (letter case ignored, decorated identifiers included). +pub(super) fn selects_identifier(manager: ManagerName, value: &str, identifier: &str) -> bool { + let target = identifier_key(manager, identifier, Decision::Deny); + std::iter::once(value) + .chain(embedded_package_names(manager, value)) + .any(|candidate| identifier_key(manager, candidate, Decision::Deny) == target) +} + /// Strip a `#ref` Git reference and an `@version` suffix, keeping a leading `@` scope. fn strip_version_suffix(specifier: &str) -> &str { let specifier = specifier diff --git a/crates/now-package-broker/src/evaluator/matching.rs b/crates/now-package-broker/src/evaluator/matching.rs index 0974397d05..9dea23591f 100644 --- a/crates/now-package-broker/src/evaluator/matching.rs +++ b/crates/now-package-broker/src/evaluator/matching.rs @@ -423,6 +423,25 @@ mod tests { assert!(rule_matches(&rule, &request, &flags, Some("1.2.3"))); } + #[test] + fn elevated_user_scope_request_uses_standard_execution_privilege() { + let mut request = request(); + request.client.requested_elevation = api::Elevation::Elevated; + request.options.scope = Some(api::Scope::User); + let flags = RequestFlags::from_request(&request); + let standard = rule(PolicyMatch { + execution_elevation: BTreeSet::from([Elevation::Standard]), + ..Default::default() + }); + let elevated = rule(PolicyMatch { + execution_elevation: BTreeSet::from([Elevation::Elevated]), + ..Default::default() + }); + + assert!(rule_matches(&standard, &request, &flags, Some("1.2.3"))); + assert!(!rule_matches(&elevated, &request, &flags, Some("1.2.3"))); + } + #[test] fn absent_scope_and_architecture_use_deterministic_manager_defaults() { use api::{Architecture as A, ManagerName as M, Operation as O, Scope as S}; diff --git a/crates/now-package-broker/src/evaluator/mod.rs b/crates/now-package-broker/src/evaluator/mod.rs index f0d05af9c6..016107d8a0 100644 --- a/crates/now-package-broker/src/evaluator/mod.rs +++ b/crates/now-package-broker/src/evaluator/mod.rs @@ -1,7 +1,8 @@ //! Policy evaluation engine. //! //! Implements the broker flow described in the package broker policies spec: -//! 1. Deny requests whose custom install location is not a plain local drive path +//! 1. Deny requests whose custom install location is not a plain local drive path, whose +//! kill-before-operation entries are not plain `.exe` image names, or that uninstall a protected package //! 2. Match enabled rules against request //! 3. Sort by priority (lowest wins), deny wins on tie //! 4. Fall back to `enforcement.defaultDecision` @@ -30,10 +31,40 @@ //! the caller pass arbitrary arguments to an installer running with administrator privileges. //! For elevated WinGet rules, set `AllowCustomParameters` to `false`, or list `--override*` and //! `--custom*` in `DeniedCustomParameters` (WinGet has no short forms for these options). +//! +//! # Security model +//! +//! Standard operations run with the user's own filtered token, so the user could run the same +//! package manager command directly. +//! For them, the policy is governance, not a security boundary, unless App Control or AppLocker +//! also blocks running the package manager directly. +//! +//! Elevated operations run with the user's linked administrator token. +//! Each Elevated Allow rule removes the UAC prompt for the operations it matches, for any code +//! running in that administrator's session, so the broker restricts what an elevated operation +//! can select regardless of the policy: +//! - An explicit user scope, typed or passed through custom parameters, always runs with the standard +//! token (see `effective_execution_elevation`); policy matching and execution use the same elevation. +//! - Kill-before-operation entries must be plain `.exe` image names; process names without an +//! extension get `.exe` appended. +//! `taskkill` only targets the requester's session, and the requester's own processes when elevated. +//! - An elevated custom install location must be on a local disk, contain no reparse point, and not be +//! writable by principals other than SYSTEM, Administrators and TrustedInstaller, including through +//! inheritable ACEs. +//! The executor checks the nearest existing folder before running the package manager. +//! An install location selected by opaque installer arguments (WinGet `--override` and `--custom`) is +//! not checked. +//! - The broker never uninstalls the Devolutions Agent, which hosts it. +//! WinGet may also select installed programs by their `ARP\...` identifiers, which this protection +//! does not cover; add Deny rules for other critical software and for such identifiers. +//! - None of these restrictions depend on the policy, so audit mode cannot override them. +//! Audit mode allows every other request, including elevated ones the rules deny, and the policy +//! validator warns about this whenever audit mode is enabled. use now_policy::{Decision, PolicyDocument}; use now_policy_api::{Elevation, PackageRequest, Scope}; +mod builtin_rules; mod constraints; mod custom_options; mod identifier; @@ -42,6 +73,7 @@ mod matching; mod version; mod wildcard; +pub(crate) use builtin_rules::{is_acceptable_kill_process_name, normalize_kill_process_name}; pub(crate) use wildcard::has_powershell_wildcard_syntax; #[cfg(test)] @@ -124,6 +156,33 @@ pub(crate) fn has_unacceptable_install_location(request: &PackageRequest) -> boo RequestFlags::from_request(request).has_unacceptable_install_location } +/// Normalized custom install location the package manager will use, typed or passed through +/// custom parameters, or `None` when there is none or it is unknown. +pub(crate) fn custom_install_location(request: &PackageRequest) -> Option { + RequestFlags::from_request(request).custom_install_location +} + +/// Whether a kill-before-operation entry, after [`normalize_kill_process_name`], is not an +/// [`is_acceptable_kill_process_name`]. +/// +/// The server rejects such requests before policy evaluation so audit mode cannot override the rejection; +/// [`evaluate`] also denies them for direct callers. +pub(crate) fn has_unacceptable_kill_process_name(request: &PackageRequest) -> bool { + request + .options + .kill_before_operation + .iter() + .any(|process| !is_acceptable_kill_process_name(&normalize_kill_process_name(&process.0))) +} + +/// Whether the request uninstalls a package the broker protects, such as the Devolutions Agent. +/// +/// The server rejects such requests before policy evaluation so audit mode cannot override the rejection; +/// [`evaluate`] also denies them for direct callers. +pub(crate) fn uninstalls_protected_package(request: &PackageRequest) -> bool { + builtin_rules::uninstalls_protected_package(request, &RequestFlags::from_request(request)) +} + /// Evaluate a parsed request against a parsed policy document. /// /// Both the policy and request should have already been deserialized into typed structs. @@ -140,6 +199,23 @@ pub fn evaluate(policy: &PolicyDocument, request: &PackageRequest) -> PolicyDeci }; } + if has_unacceptable_kill_process_name(request) { + return PolicyDecision { + decision: Decision::Deny, + rule_id: "".to_owned(), + reason: "Kill-before-operation entries must be process names without an extension or ending in .exe, without wildcards, path separators, quotes, or control characters." + .to_owned(), + }; + } + + if builtin_rules::uninstalls_protected_package(request, &flags) { + return PolicyDecision { + decision: Decision::Deny, + rule_id: "".to_owned(), + reason: "The package broker does not uninstall the Devolutions Agent.".to_owned(), + }; + } + let requested_version = version::requested_version(request); let mut matched_rules: Vec<(&str, u32, Decision, &str)> = Vec::new(); @@ -210,10 +286,33 @@ pub(crate) fn is_powershell_manager(manager: now_policy_api::ManagerName) -> boo ) } +/// Elevation of the token that executes `request`. +/// +/// Policy matching, request validation, command building and the executor all use this value, +/// so a request is always evaluated with the elevation it runs with. pub(crate) fn effective_execution_elevation(request: &PackageRequest) -> Elevation { - if request.options.scope == Some(Scope::Machine) || request.client.requested_elevation == Elevation::Elevated { - Elevation::Elevated - } else { - Elevation::Standard + let custom_scope = || { + let custom = custom_options::custom_options(request.manager, &request.options.custom_parameters); + custom_options::resolve(None, &custom.scopes).flatten() + }; + // A user scope passed only through custom parameters lowers the elevation like a typed one; + // a machine scope there does not raise it. + let scope = request + .options + .scope + .or_else(|| custom_scope().filter(|scope| *scope == Scope::User)); + effective_elevation(request.client.requested_elevation, scope) +} + +/// Elevation of the execution token for a requested elevation and an explicitly requested scope. +/// +/// Machine scope requires the elevated token. +/// User scope runs with the standard token even when elevation is requested, so the +/// administrator token never writes into the user profile. +pub(crate) fn effective_elevation(requested: Elevation, scope: Option) -> Elevation { + match scope { + Some(Scope::Machine) => Elevation::Elevated, + Some(Scope::User) => Elevation::Standard, + None => requested, } } diff --git a/crates/now-package-broker/src/evaluator/tests.rs b/crates/now-package-broker/src/evaluator/tests.rs index 4a769166d4..83a8e39b61 100644 --- a/crates/now-package-broker/src/evaluator/tests.rs +++ b/crates/now-package-broker/src/evaluator/tests.rs @@ -719,3 +719,97 @@ fn recommended_denied_custom_parameters_block_winget_installer_arguments() { request.options.custom_parameters = vec![api::CustomParameterString("--silent".to_owned())]; assert_eq!(evaluate(&policy, &request).rule_id, "allow-winget"); } + +#[test] +fn effective_elevation_lowers_explicit_user_scope_and_raises_machine_scope() { + use api::{Elevation as E, Scope as S}; + + assert_eq!(super::effective_elevation(E::Elevated, Some(S::User)), E::Standard); + assert_eq!(super::effective_elevation(E::Elevated, None), E::Elevated); + assert_eq!(super::effective_elevation(E::Elevated, Some(S::Machine)), E::Elevated); + assert_eq!(super::effective_elevation(E::Standard, Some(S::Machine)), E::Elevated); + assert_eq!(super::effective_elevation(E::Standard, Some(S::User)), E::Standard); + assert_eq!(super::effective_elevation(E::Standard, None), E::Standard); +} + +#[test] +fn unacceptable_kill_process_names_are_denied_before_rules() { + let policy = make_policy(Decision::Allow, Vec::new()); + + for name in ["*", "Code*.exe", r"C:\Tools\Code.exe", "Code.bat"] { + let mut request = make_request(api::Operation::Install, "Microsoft.VisualStudioCode"); + request.options.kill_before_operation = vec![api::ProcessName(name.to_owned())]; + let result = evaluate(&policy, &request); + assert_eq!(result.decision, Decision::Deny, "{name}"); + assert_eq!(result.rule_id, "", "{name}"); + } + + let mut request = make_request(api::Operation::Install, "Microsoft.VisualStudioCode"); + request.options.kill_before_operation = vec![ + api::ProcessName("Code.exe".to_owned()), + api::ProcessName("chrome".to_owned()), + ]; + assert_eq!(evaluate(&policy, &request).decision, Decision::Allow); +} + +#[test] +fn agent_uninstall_is_denied_before_rules() { + let policy = make_policy(Decision::Allow, Vec::new()); + + let request = make_request(api::Operation::Uninstall, "devolutions.agent"); + let result = evaluate(&policy, &request); + assert_eq!(result.decision, Decision::Deny); + assert_eq!(result.rule_id, ""); + + let mut request = make_request(api::Operation::Update, "Devolutions.Agent"); + request.options.uninstall_previous = true; + assert_eq!(evaluate(&policy, &request).decision, Decision::Deny); + + let mut request = make_request(api::Operation::Update, "Devolutions.Agent"); + request.options.custom_parameters = vec![api::CustomParameterString("--uninstall-previous".to_owned())]; + assert_eq!(evaluate(&policy, &request).decision, Decision::Deny); + + let mut request = make_request(api::Operation::Uninstall, "DEVO-AGENT"); + request.manager = api::ManagerName::Chocolatey; + request.source.name = "chocolatey".to_owned(); + assert_eq!(evaluate(&policy, &request).decision, Decision::Deny); + + // Installs and updates that keep the previous version, and other packages, are left to the policy. + let request = make_request(api::Operation::Update, "Devolutions.Agent"); + assert_eq!(evaluate(&policy, &request).decision, Decision::Allow); + let request = make_request(api::Operation::Uninstall, "Devolutions.Agent.Extra"); + assert_eq!(evaluate(&policy, &request).decision, Decision::Allow); + let mut request = make_request(api::Operation::Uninstall, "Devolutions.Agent"); + request.manager = api::ManagerName::Chocolatey; + request.source.name = "chocolatey".to_owned(); + assert_eq!(evaluate(&policy, &request).decision, Decision::Allow); +} + +#[test] +fn custom_parameter_user_scope_lowers_execution_elevation() { + let custom = |values: &[&str]| { + values + .iter() + .map(|value| api::CustomParameterString((*value).to_owned())) + .collect::>() + }; + + let mut request = make_request(api::Operation::Install, "Contoso.Tool"); + request.options.custom_parameters = custom(&["--scope", "user"]); + assert_eq!(super::effective_execution_elevation(&request), api::Elevation::Standard); + + // A custom machine scope does not raise a standard request, and a typed scope takes precedence. + request.client.requested_elevation = api::Elevation::Standard; + request.options.custom_parameters = custom(&["--scope", "machine"]); + assert_eq!(super::effective_execution_elevation(&request), api::Elevation::Standard); + + request.client.requested_elevation = api::Elevation::Elevated; + request.options.scope = Some(api::Scope::Machine); + request.options.custom_parameters = custom(&["--scope", "user"]); + assert_eq!(super::effective_execution_elevation(&request), api::Elevation::Elevated); + + // Conflicting custom scopes leave the requested elevation unchanged. + request.options.scope = None; + request.options.custom_parameters = custom(&["--scope", "user", "--scope", "machine"]); + assert_eq!(super::effective_execution_elevation(&request), api::Elevation::Elevated); +} diff --git a/crates/now-package-broker/src/executor/mod.rs b/crates/now-package-broker/src/executor/mod.rs index 88519475bf..308622c6fb 100644 --- a/crates/now-package-broker/src/executor/mod.rs +++ b/crates/now-package-broker/src/executor/mod.rs @@ -2,6 +2,8 @@ //! //! Handles running commands under the specified user identity. +use std::path::PathBuf; + use async_trait::async_trait; use chrono::{DateTime, Utc}; use now_policy_api::{Elevation, ManagerName, Scope}; @@ -50,10 +52,14 @@ pub struct ExecutionContext { /// Session selection uses this SID so distinct accounts sharing the same name /// (e.g. `MACHINE\alice` vs `DOMAIN\alice`) cannot be confused with one another. pub user_sid: Sid, - /// Requested elevation level. + /// Execution elevation, as decided by `evaluator::effective_execution_elevation`. pub elevation: Elevation, /// Installation scope (machine scope requires elevation). pub scope: Option, + /// Normalized custom install location, when the request selects a known one. + /// + /// Elevated execution requires it to be writable by administrators only. + pub custom_install_location: Option, /// When true, capture the main command's combined stdout+stderr. /// /// Since API v0.3 this only controls whether output frames are pushed on the @@ -73,6 +79,13 @@ pub struct ExecutionContext { pub event_sink: Option, } +impl ExecutionContext { + /// Whether the plan runs with the elevated (linked) token. + pub fn requires_elevation(&self) -> bool { + crate::evaluator::effective_elevation(self.elevation, self.scope) == Elevation::Elevated + } +} + /// Marker error reported by executors when an operation was terminated because its /// cancellation was requested through [`ExecutionContext::cancel_token`]. #[derive(Debug)] diff --git a/crates/now-package-broker/src/executor/windows/mod.rs b/crates/now-package-broker/src/executor/windows/mod.rs index 1d4657be89..1e97d52c74 100644 --- a/crates/now-package-broker/src/executor/windows/mod.rs +++ b/crates/now-package-broker/src/executor/windows/mod.rs @@ -9,7 +9,7 @@ use std::path::{Path, PathBuf}; use anyhow::{Context as _, bail}; use async_trait::async_trait; use devolutions_agent_shared::temp_file::{BATCH_UTF8_PREAMBLE, POWERSHELL_UTF8_ENCODING_PREAMBLE, TmpFileGuard}; -use now_policy_api::{Elevation, ManagerName, Scope}; +use now_policy_api::ManagerName; use tracing::{debug, info, warn}; use win_api_wrappers::identity::sid::Sid; use win_api_wrappers::process::Process; @@ -68,7 +68,7 @@ impl CommandExecutor for WindowsExecutor { ctx: &ExecutionContext, process_started: Option, ) -> anyhow::Result { - let requires_elevation = ctx.elevation == Elevation::Elevated || ctx.scope == Some(Scope::Machine); + let requires_elevation = ctx.requires_elevation(); reject_unsupported_vcpkg_elevation(ctx)?; // SECURITY: Defense in depth — the broker server already rejects such @@ -210,7 +210,7 @@ fn execute_as_system( ctx: &ExecutionContext, process_started: Option, ) -> anyhow::Result { - let requires_elevation = ctx.elevation == Elevation::Elevated || ctx.scope == Some(Scope::Machine); + let requires_elevation = ctx.requires_elevation(); info!( effective_user = %ctx.effective_user, @@ -340,7 +340,7 @@ fn run_plan( return Err(anyhow::Error::new(OperationCanceled)); } - let requires_elevation = ctx.elevation == Elevation::Elevated || ctx.scope == Some(Scope::Machine); + let requires_elevation = ctx.requires_elevation(); if requires_elevation && command_is_bun(&ctx.command) { bail!("elevated Bun package operations are not supported by the broker"); } @@ -359,15 +359,40 @@ fn run_plan( } } + // Defense in depth: the server rejects these names before policy evaluation. + if let Some(process_name) = ctx + .kill_processes + .iter() + .find(|name| !crate::evaluator::is_acceptable_kill_process_name(name)) + { + bail!("unacceptable kill-before-operation process name '{process_name}'"); + } + + let vars = win_api_wrappers::utils::environment_block(Some(token), false) + .context("failed to load user environment block")?; + let user_env = UserEnv::for_user(&vars, token)?; + + // SECURITY: An elevated install into a folder that non-administrators can write lets them + // replace what the administrator token installed. The guard keeps the ancestors of the + // verified folder from being renamed until the plan completes. + let _install_location_guard = match &ctx.custom_install_location { + Some(location) if requires_elevation => { + Some(policy_security::verify_elevated_install_location(&user_env, location)?) + } + _ => None, + }; + + // Elevated kills are limited to the requester's processes in the requester's session. + let kill_user = if requires_elevation && !ctx.kill_processes.is_empty() { + Some(account_display_name(&ctx.user_sid)?) + } else { + None + }; + // 1. Kill requested processes (best-effort; a missing process is not an error, // but a cancellation request must still be honored). for process_name in &ctx.kill_processes { - let kill_cmd = vec![ - trusted_system32_executable("taskkill.exe"), - "/F".to_owned(), - "/IM".to_owned(), - process_name.clone(), - ]; + let kill_cmd = kill_command(process_name, session_id, kill_user.as_deref()); match create_process( token, &kill_cmd, @@ -407,7 +432,7 @@ fn run_plan( } // 3. Main package-manager command. - let command = prepare_main_command(token, &ctx.command, requires_elevation)?; + let command = prepare_main_command_in(&ctx.command, None, Some(&user_env), requires_elevation)?; let output = create_process( token, command.args(), @@ -450,17 +475,6 @@ fn run_plan( Ok(output) } -fn prepare_main_command( - token: &Token, - command: &[String], - requires_elevation: bool, -) -> anyhow::Result { - let vars = win_api_wrappers::utils::environment_block(Some(token), false) - .context("failed to load user environment block")?; - let user_env = UserEnv::for_user(&vars, token)?; - prepare_main_command_in(command, None, Some(&user_env), requires_elevation) -} - fn prepare_main_command_in( command: &[String], temp_dir: Option<&Path>, @@ -498,10 +512,37 @@ fn prepare_main_command_in( Ok(PreparedCommand::raw(command)) } +/// `taskkill` command line terminating `process_name` in `session_id`, owned by `user` when given. +/// +/// `/IM` alone matches processes of every session and user the execution token can reach. +fn kill_command(process_name: &str, session_id: u32, user: Option<&str>) -> Vec { + let mut command = vec![ + trusted_system32_executable("taskkill.exe"), + "/F".to_owned(), + "/FI".to_owned(), + format!("SESSION eq {session_id}"), + ]; + if let Some(user) = user { + command.extend(["/FI".to_owned(), format!("USERNAME eq {user}")]); + } + command.extend(["/IM".to_owned(), process_name.to_owned()]); + command +} + +/// `DOMAIN\name` of the account identified by `sid`, as `taskkill` reports process owners. +fn account_display_name(sid: &Sid) -> anyhow::Result { + let account = sid + .lookup_account(None) + .with_context(|| format!("failed to look up the account name of '{sid}'"))?; + Ok(format!( + "{}\\{}", + account.domain_name.to_string_lossy(), + account.name.to_string_lossy() + )) +} + fn reject_unsupported_vcpkg_elevation(ctx: &ExecutionContext) -> anyhow::Result<()> { - if executable_is(&ctx.command, "vcpkg.exe") - && (ctx.elevation == Elevation::Elevated || ctx.scope == Some(Scope::Machine)) - { + if executable_is(&ctx.command, "vcpkg.exe") && ctx.requires_elevation() { bail!("vcpkg elevated or machine-scope operations are not supported by the broker"); } @@ -1370,10 +1411,11 @@ mod tests { use windows::Win32::Security::{NO_INHERITANCE, WinWorldSid}; use super::{ - BATCH_METACHARACTERS, POWERSHELL_UTF8_ENCODING_PREAMBLE, UserEnv, WindowsExecutor, append_batch_executable, - execute_as_current_user, prepare_bun_cmd_script, prepare_chocolatey_script_in_with_default_install_root, - prepare_main_command_in, prepare_shell_command_in, reject_unsupported_vcpkg_elevation, - resolve_trusted_chocolatey_executable, resolve_winget_executable, + BATCH_METACHARACTERS, POWERSHELL_UTF8_ENCODING_PREAMBLE, UserEnv, WindowsExecutor, account_display_name, + append_batch_executable, execute_as_current_user, kill_command, prepare_bun_cmd_script, + prepare_chocolatey_script_in_with_default_install_root, prepare_main_command_in, prepare_shell_command_in, + reject_unsupported_vcpkg_elevation, resolve_trusted_chocolatey_executable, resolve_winget_executable, + trusted_system32_executable, }; use crate::executor::{CommandExecutor as _, ExecutionContext}; @@ -2178,7 +2220,8 @@ mod tests { effective_user: "DOMAIN\\user".to_owned(), user_sid: Sid::from_well_known(WinWorldSid, None).expect("well-known Everyone SID"), elevation: Elevation::Elevated, - scope: Some(Scope::User), + scope: None, + custom_install_location: None, capture_output: false, cancel_token: tokio_util::sync::CancellationToken::new(), event_sink: None, @@ -2191,6 +2234,42 @@ mod tests { ctx.scope = Some(Scope::Machine); let error = reject_unsupported_vcpkg_elevation(&ctx).expect_err("machine-scope vcpkg should fail"); assert!(error.to_string().contains("machine-scope")); + + // Elevated user-scope requests run with the standard token. + ctx.elevation = Elevation::Elevated; + ctx.scope = Some(Scope::User); + assert!(!ctx.requires_elevation()); + reject_unsupported_vcpkg_elevation(&ctx).expect("elevated user-scope vcpkg runs with the standard token"); + } + + #[test] + fn kill_command_is_scoped_to_the_requester_session() { + let taskkill = trusted_system32_executable("taskkill.exe"); + + assert_eq!( + kill_command("Code.exe", 3, None), + [taskkill.as_str(), "/F", "/FI", "SESSION eq 3", "/IM", "Code.exe"] + ); + assert_eq!( + kill_command("Code.exe", 3, Some(r"CONTOSO\alice")), + [ + taskkill.as_str(), + "/F", + "/FI", + "SESSION eq 3", + "/FI", + r"USERNAME eq CONTOSO\alice", + "/IM", + "Code.exe" + ] + ); + } + + #[test] + fn account_display_name_uses_domain_and_name() { + let system = Sid::from_well_known(windows::Win32::Security::WinLocalSystemSid, None).expect("SYSTEM SID"); + let name = account_display_name(&system).expect("look up SYSTEM"); + assert!(name.ends_with(r"\SYSTEM"), "{name}"); } #[tokio::test] @@ -2203,7 +2282,8 @@ mod tests { effective_user: "DOMAIN\\user".to_owned(), user_sid: Sid::from_well_known(WinWorldSid, None).expect("well-known Everyone SID"), elevation: Elevation::Elevated, - scope: Some(Scope::User), + scope: None, + custom_install_location: None, capture_output: false, cancel_token: tokio_util::sync::CancellationToken::new(), event_sink: None, @@ -2233,6 +2313,7 @@ mod tests { user_sid: Sid::from_well_known(WinWorldSid, None).expect("well-known Everyone SID"), elevation: Elevation::Standard, scope: Some(Scope::User), + custom_install_location: None, capture_output: false, cancel_token: tokio_util::sync::CancellationToken::new(), event_sink: None, diff --git a/crates/now-package-broker/src/executor/windows/user_env.rs b/crates/now-package-broker/src/executor/windows/user_env.rs index c717bb6d1d..0fe467922b 100644 --- a/crates/now-package-broker/src/executor/windows/user_env.rs +++ b/crates/now-package-broker/src/executor/windows/user_env.rs @@ -9,7 +9,7 @@ use tracing::{debug, warn}; use win_api_wrappers::token::Token; use crate::impersonation::{ThreadImpersonation, impersonation_token}; -use crate::policy_security::{PathOpener, is_plain_local_drive_path}; +use crate::policy_security::{PathOpener, is_plain_local_drive_path, is_plain_local_drive_root}; /// Environment variables of the target user, with filesystem lookups performed as that user. /// @@ -95,7 +95,10 @@ impl<'a> UserEnv<'a> { impl PathOpener for UserEnv<'_> { fn open(&self, options: &OpenOptions, path: &Path) -> std::io::Result { - if !path.to_str().is_some_and(is_plain_local_drive_path) { + if !path + .to_str() + .is_some_and(|path| is_plain_local_drive_path(path) || is_plain_local_drive_root(path)) + { return Err(std::io::Error::new( std::io::ErrorKind::InvalidInput, format!("'{}' is not a plain local drive path", path.display()), diff --git a/crates/now-package-broker/src/policy_security.rs b/crates/now-package-broker/src/policy_security.rs index 78f4aa44b0..6be0ab6d1d 100644 --- a/crates/now-package-broker/src/policy_security.rs +++ b/crates/now-package-broker/src/policy_security.rs @@ -50,7 +50,7 @@ use windows::Win32::Security::Authorization::{ConvertSidToStringSidW, GetSecurit use windows::Win32::Security::{ ACCESS_ALLOWED_ACE, ACE_HEADER, ACL, DACL_SECURITY_INFORMATION, GetAce, GetLengthSid, INHERIT_ONLY_ACE, IsWellKnownSid, OWNER_SECURITY_INFORMATION, PSECURITY_DESCRIPTOR, PSID, TOKEN_QUERY, WinBuiltinAdministratorsSid, - WinLocalSystemSid, + WinCreatorOwnerSid, WinLocalSystemSid, }; use windows::Win32::Storage::FileSystem::{ DELETE, FILE_APPEND_DATA, FILE_ATTRIBUTE_REPARSE_POINT, FILE_ATTRIBUTE_TAG_INFO, FILE_DELETE_CHILD, @@ -117,6 +117,13 @@ enum TrustedWriters { /// it is a low-privilege shared service identity, and accepting it for elevated /// executables would open a privilege-escalation path. AdminOrTrustedInstaller, + /// SYSTEM, the built-in Administrators group, and `NT SERVICE\TrustedInstaller`, for an + /// elevated custom install location. + /// + /// Inheritable ACEs are checked too, since they grant their rights on the folders and files + /// the installer creates; `CREATOR OWNER` is accepted on inherit-only ACEs because the + /// administrator token creates those objects. + InstallLocation, } // ACE type constants from winnt.h (the Win32_System_SystemServices feature is not enabled). @@ -231,6 +238,11 @@ pub(crate) fn is_plain_local_drive_path(path: &str) -> bool { .any(|&byte| matches!(byte, b':' | b'/') || byte.is_ascii_control()) } +/// Accepts only a drive-letter root such as `C:\`. +pub(crate) fn is_plain_local_drive_root(path: &str) -> bool { + matches!(path.as_bytes(), [drive, b':', b'\\'] if drive.is_ascii_alphabetic()) +} + pub(crate) fn paths_match_case_insensitive(left: &Path, right: &Path) -> bool { windows_paths_equal(left, right) } @@ -818,6 +830,99 @@ pub(crate) fn pin_launch_executable( ) } +/// Rights on an elevated install location, directly or through inheritable ACEs, that let a +/// principal add, replace or delete what the installer puts there, or rewrite its security. +const INSTALL_LOCATION_TAMPER_MASK: u32 = PARENT_DIRECTORY_TAMPER_MASK | WRITE_ACCESS_MASK; + +/// Handles pinning a verified elevated install location and its ancestors against rename. +#[derive(Debug)] +pub(crate) struct VerifiedInstallLocation { + _handles: Vec, +} + +/// Verify that an elevated operation may install into `location`. +/// +/// The nearest existing folder, which is `location` itself when it exists, is opened through +/// `opener` so the path resolves as it does for the execution token. +/// That folder must be on a local disk volume, must not be a reparse point, and its owner and +/// DACL, including inheritable ACEs, must not let principals other than SYSTEM, Administrators +/// and TrustedInstaller write it. +/// Its ancestors must not be reparse points or be renamable by other principals. +/// The returned guard must be kept alive until the installation completes. +pub(crate) fn verify_elevated_install_location( + opener: &dyn PathOpener, + location: &Path, +) -> anyhow::Result { + // Delete sharing lets the installer replace an existing install folder; non-administrators + // cannot, since the DACL check below rejects delete rights for them. + let mut options = OpenOptions::new(); + options + .access_mode(FILE_READ_ATTRIBUTES.0 | READ_CONTROL.0) + .share_mode(FILE_SHARE_READ.0 | FILE_SHARE_WRITE.0 | FILE_SHARE_DELETE.0) + .custom_flags((FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OPEN_REPARSE_POINT).0); + + let mut folder = location; + let handle = loop { + match opener.open(&options, folder) { + Ok(handle) => break handle, + Err(error) if error.kind() == std::io::ErrorKind::NotFound => { + folder = folder.parent().with_context(|| { + format!( + "custom install location '{}' is not on an existing drive", + location.display() + ) + })?; + } + Err(error) => { + return Err(error) + .with_context(|| format!("failed to open install location folder '{}'", folder.display())); + } + } + }; + + let subject = format!("install location folder '{}'", folder.display()); + verify_local_volume_file(&handle).with_context(|| format!("failed to verify {subject}"))?; + if is_reparse_point(&handle).with_context(|| format!("failed to inspect {subject}"))? { + bail!("{subject} is a reparse point"); + } + if !handle + .metadata() + .with_context(|| format!("failed to inspect {subject}"))? + .is_dir() + { + bail!("{subject} is not a directory"); + } + let resolved = final_path_from_handle(&handle).with_context(|| format!("failed to resolve {subject}"))?; + if !windows_paths_equal(&resolved, folder) { + bail!("{subject} resolved to an unexpected location '{}'", resolved.display()); + } + + verify_handle_security( + &handle, + &subject, + TrustedWriters::InstallLocation, + INSTALL_LOCATION_TAMPER_MASK, + ) + .with_context(|| { + format!( + "custom install location '{}' is writable by non-administrators; \ + choose a location under Program Files or a folder restricted to administrators", + location.display() + ) + })?; + + let mut handles = + retain_ancestor_directories(folder.parent(), DIRECTORY_TAMPER_MASK, &subject).with_context(|| { + format!( + "custom install location '{}' is in a folder that non-administrators can redirect", + location.display() + ) + })?; + handles.push(handle); + + Ok(VerifiedInstallLocation { _handles: handles }) +} + /// Kind of Microsoft Store app execution alias found at a path. #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub(crate) enum AppExecAliasKind { @@ -1082,9 +1187,17 @@ fn parse_app_exec_alias(buffer: &[u8]) -> Option { /// Each ancestor must not itself be a reparse point and must resolve to its own path. /// The returned handles pin the verified chain and must be kept alive by the caller. fn retain_executable_ancestor_directories(path: &Path, subject: &str) -> anyhow::Result> { + retain_ancestor_directories(path.parent(), PARENT_DIRECTORY_TAMPER_MASK, subject) +} + +/// Verify and pin `first` and each of its ancestors, checking `first` against `first_mask` +/// and higher ancestors against [`DIRECTORY_TAMPER_MASK`]. +/// +/// See [`retain_executable_ancestor_directories`] for the rationale. +fn retain_ancestor_directories(first: Option<&Path>, first_mask: u32, subject: &str) -> anyhow::Result> { let mut handles = Vec::new(); - let mut current = path.parent(); - let mut tamper_mask = PARENT_DIRECTORY_TAMPER_MASK; + let mut current = first; + let mut tamper_mask = first_mask; while let Some(dir) = current { let dir_subject = format!("{subject} ancestor directory '{}'", dir.display()); @@ -1322,7 +1435,9 @@ unsafe fn verify_owner_and_dacl( // Inherit-only ACEs do not apply to the object itself, only to its children // (e.g. the `CREATOR OWNER` full-control template ACE on `Program Files`). - if u32::from(header.AceFlags) & INHERIT_ONLY_ACE.0 != 0 { + // An install location also passes them on to what the installer creates there. + let inherit_only = u32::from(header.AceFlags) & INHERIT_ONLY_ACE.0 != 0; + if inherit_only && trusted_writers != TrustedWriters::InstallLocation { continue; } @@ -1346,6 +1461,12 @@ unsafe fn verify_owner_and_dacl( let trustee = PSID(std::ptr::from_ref(&ace.SidStart).cast_mut().cast()); + // On objects created by the administrator token, `CREATOR OWNER` grants that owner. + // SAFETY: `SidStart` is the first DWORD of the trustee SID stored inline in the ACE. + if inherit_only && unsafe { IsWellKnownSid(trustee, WinCreatorOwnerSid) }.as_bool() { + continue; + } + // SAFETY: `SidStart` is the first DWORD of the trustee SID stored inline in the ACE. if !unsafe { is_trusted_sid(trustee, trusted_writers) } { // SAFETY: `trustee` points to a valid SID inside the ACE. @@ -1386,10 +1507,14 @@ unsafe fn is_trusted_sid(sid: PSID, trusted_writers: TrustedWriters) -> bool { // SAFETY: Per function contract, `sid` points to a valid SID. let sid_string = unsafe { sid_to_string(sid) }; + if sid_string.eq_ignore_ascii_case(TRUSTED_INSTALLER_SID) { + return true; + } + // Windows-protected executables (e.g. Store apps under `Program Files\WindowsApps`) // grant write access to TrustedInstaller and to process trust-label SIDs, which the // kernel only assigns to Windows-signed protected processes. - sid_string.eq_ignore_ascii_case(TRUSTED_INSTALLER_SID) || sid_string.starts_with(PROCESS_TRUST_SID_PREFIX) + trusted_writers == TrustedWriters::AdminOrTrustedInstaller && sid_string.starts_with(PROCESS_TRUST_SID_PREFIX) } /// Best-effort conversion of a SID to its string form for diagnostics. @@ -1781,6 +1906,135 @@ mod tests { ) } } + + fn verify_as_install_location(&self) -> anyhow::Result<()> { + // SAFETY: `owner` and `dacl` point into the owned security descriptor, which outlives + // this call. + unsafe { + verify_owner_and_dacl( + "test install location", + self.owner, + self.dacl, + TrustedWriters::InstallLocation, + INSTALL_LOCATION_TAMPER_MASK, + ) + } + } + } + + #[test] + fn program_files_like_install_location_is_accepted() { + // `Program Files` grants Users read and execute, and full control to the owner of new objects. + let sd = SddlDescriptor::parse(&format!( + "O:{TRUSTED_INSTALLER_SID}D:PAI(A;;FA;;;{TRUSTED_INSTALLER_SID})(A;CIIO;GA;;;{TRUSTED_INSTALLER_SID})\ + (A;;0x1301bf;;;SY)(A;OICIIO;GA;;;SY)(A;;0x1301bf;;;BA)(A;OICIIO;GA;;;BA)\ + (A;;0x1200a9;;;BU)(A;OICIIO;GXGR;;;BU)(A;OICIIO;GA;;;CO)(A;;0x1200a9;;;AC)(A;OICIIO;GXGR;;;AC)" + )); + sd.verify_as_install_location() + .expect("Program Files-like folder must be accepted"); + } + + #[test] + fn administrators_only_install_location_is_accepted() { + let sd = SddlDescriptor::parse("O:BAD:P(A;OICI;FA;;;SY)(A;OICI;FA;;;BA)(A;OICI;0x1200a9;;;BU)"); + sd.verify_as_install_location() + .expect("SYSTEM/Administrators-only folder must be accepted"); + } + + #[test] + fn inheritable_untrusted_write_ace_is_rejected_for_install_locations_only() { + // Users may modify what gets created inside the folder, but not the folder itself. + let sd = SddlDescriptor::parse("O:BAD:P(A;OICI;FA;;;SY)(A;OICI;FA;;;BA)(A;OICIIO;0x1301bf;;;BU)"); + sd.verify_as_executable() + .expect("inherit-only ACEs do not apply to an executable's ancestor itself"); + let error = sd.verify_as_install_location().unwrap_err(); + assert!(error.to_string().contains("S-1-5-32-545"), "unexpected error: {error}"); + } + + #[test] + fn untrusted_create_rights_are_rejected_for_install_locations() { + // Drive roots let Authenticated Users create subfolders. + for ace in [ + "(A;;0x4;;;AU)", + "(A;;0x2;;;AU)", + "(A;OICI;0x1301bf;;;BU)", + "(A;;GA;;;CO)", + ] { + let sd = SddlDescriptor::parse(&format!("O:BAD:P(A;OICI;FA;;;SY)(A;OICI;FA;;;BA){ace}")); + assert!(sd.verify_as_install_location().is_err(), "{ace}"); + } + } + + #[test] + fn untrusted_owner_is_rejected_for_install_locations() { + let sd = SddlDescriptor::parse("O:BUD:P(A;OICI;FA;;;SY)(A;OICI;FA;;;BA)"); + assert!(sd.verify_as_install_location().is_err()); + } + + #[test] + fn install_location_with_inheritable_users_write_ace_is_rejected() { + let temp = tempfile::tempdir().unwrap(); + let folder = temp.path().join("Contoso"); + std::fs::create_dir(&folder).unwrap(); + let users = Sid::from_well_known(windows::Win32::Security::WinBuiltinUsersSid, None).unwrap(); + let current_user = Process::current_process() + .token(TOKEN_QUERY) + .unwrap() + .sid_and_attributes() + .unwrap() + .sid; + set_security( + &folder, + None, + &[ + grant(GENERIC_ALL.0, current_user), + ExplicitAccess { + access_permissions: GENERIC_ALL.0, + access_mode: GRANT_ACCESS, + inheritance: windows::Win32::Security::SUB_CONTAINERS_AND_OBJECTS_INHERIT + | windows::Win32::Security::INHERIT_ONLY, + trustee: Trustee::Sid(users), + }, + ], + ) + .unwrap(); + + let location = canonical_dir(&folder).join("App"); + let error = verify_elevated_install_location(&ServiceOpener, &location).unwrap_err(); + assert!( + error.to_string().contains("is writable by non-administrators"), + "unexpected error: {error:#}" + ); + } + + #[test] + fn install_location_under_program_files_is_accepted() { + let program_files = std::env::var("ProgramFiles").unwrap_or_else(|_| r"C:\Program Files".to_owned()); + let location = Path::new(&program_files).join(format!("now-package-broker-test-{}", uuid::Uuid::new_v4())); + + verify_elevated_install_location(&ServiceOpener, &location.join("App")) + .expect("a new folder under Program Files must be accepted"); + } + + fn canonical_dir(path: &Path) -> PathBuf { + let canonical = path.canonicalize().unwrap(); + dos_path_from_wide(&canonical.as_os_str().encode_wide().collect::>()) + } + + #[test] + fn install_location_through_a_reparse_point_is_rejected() { + let temp = tempfile::tempdir().unwrap(); + let base = canonical_dir(temp.path()); + let target = base.join("target"); + std::fs::create_dir(&target).unwrap(); + let link = base.join("link"); + std::os::windows::fs::symlink_dir(&target, &link).unwrap(); + + let error = verify_elevated_install_location(&ServiceOpener, &link.join("App")).unwrap_err(); + assert!( + error.to_string().contains("reparse point"), + "unexpected error: {error:#}" + ); } #[test] diff --git a/crates/now-package-broker/src/policy_store/mod.rs b/crates/now-package-broker/src/policy_store/mod.rs index 2f9a8bdd78..e4b8d77ac8 100644 --- a/crates/now-package-broker/src/policy_store/mod.rs +++ b/crates/now-package-broker/src/policy_store/mod.rs @@ -1145,7 +1145,7 @@ mod storage_tests { assert_eq!(error.code, ErrorCode::ValidationFailed); let validation = store.validate_draft(&request.draft); - assert_eq!(validation.validator_version, "now-package-broker-policy-validator/11"); + assert_eq!(validation.validator_version, "now-package-broker-policy-validator/12"); let canonical = validation.canonical_draft.as_ref().expect("compatible draft"); let old_receipt = store diff --git a/crates/now-package-broker/src/policy_store/validation.rs b/crates/now-package-broker/src/policy_store/validation.rs index 60a8272f49..1c39e680af 100644 --- a/crates/now-package-broker/src/policy_store/validation.rs +++ b/crates/now-package-broker/src/policy_store/validation.rs @@ -12,7 +12,7 @@ use now_policy_api::{ use crate::evaluator; -pub(super) const VALIDATOR_VERSION: &str = "now-package-broker-policy-validator/11"; +pub(super) const VALIDATOR_VERSION: &str = "now-package-broker-policy-validator/12"; const MAX_RULES: usize = 1024; const MAX_RULE_PRIORITY: u32 = i32::MAX as u32; const MAX_FINDING_MESSAGE_CHARS: usize = 2048; @@ -343,10 +343,12 @@ fn semantic_checks(raw: &serde_json::Value, draft: &PolicyDraftDocument, finding return; } if draft.enforcement.audit_mode == Some(true) { + // Audit mode allows every request that passes the built-in restrictions, including + // elevated requests the rules would deny. findings.push(warning( PolicyFindingCode::AuditModeEnabled, "/Enforcement/AuditMode", - "audit mode is enabled; decisions are not enforced", + "audit mode does not enforce decisions; elevated requests run without a UAC prompt while it is enabled", )); } if draft.enforcement.default_decision == Decision::Allow { @@ -712,6 +714,33 @@ mod tests { json!({ "Id": id, "Priority": 1, "Decision": "Deny", "Match": match_value }) } + #[test] + fn audit_mode_warns_that_elevated_requests_run_without_a_prompt() { + let uac_warnings = |raw: &serde_json::Value| { + validate_draft(raw) + .findings + .iter() + .filter(|finding| { + finding.code == PolicyFindingCode::AuditModeEnabled + && finding.message.contains("without a UAC prompt") + }) + .count() + }; + + // Audit mode allows elevated requests even when every rule is limited to standard execution. + let mut raw = draft(); + raw["Enforcement"]["AuditMode"] = json!(true); + raw["Rules"] = json!([{ + "Id": "allow.standard", + "Priority": 1, + "Decision": "Allow", + "Match": { "Managers": ["Winget"], "ExecutionElevation": ["Standard"] } + }]); + assert_eq!(uac_warnings(&raw), 1); + + raw["Enforcement"]["AuditMode"] = json!(false); + assert_eq!(uac_warnings(&raw), 0); + } #[test] fn final_contract_is_canonical_and_uses_scalar_booleans() { let mut raw = draft(); diff --git a/crates/now-package-broker/src/server/mod.rs b/crates/now-package-broker/src/server/mod.rs index 0b52faa8c1..5cc5de4506 100644 --- a/crates/now-package-broker/src/server/mod.rs +++ b/crates/now-package-broker/src/server/mod.rs @@ -1,5 +1,6 @@ //! Runtime implementation of the shared NOW package broker server facade. +use std::borrow::Cow; use std::collections::HashMap; use std::fmt; use std::path::PathBuf; @@ -21,7 +22,7 @@ use now_policy_api::{ ExecutionResponseKind, HealthResponse, HealthResponseKind, HealthStatus, ManagerCapability, ManagerName, OperationStatus, OperationSubmission, PackageRequest, PolicyManagementResponse, PolicyReplacementRequest, PolicyReplacementResponse, PolicyResponse, PolicyResponseKind, PolicyValidationRequest, PolicyValidationResponse, - Scope, StatusRequest, StatusResponse, StatusResponseKind, Transport, + StatusRequest, StatusResponse, StatusResponseKind, Transport, }; use now_policy_server_template::{ MAX_POLICY_MANAGEMENT_BODY_BYTES, MAX_REQUEST_BODY_BYTES, PackageBrokerServer, SharedPackageBrokerServer, @@ -631,6 +632,7 @@ impl BrokerState { user_sid: user_sid.clone(), elevation: evaluator::effective_execution_elevation(&request), scope: request.options.scope, + custom_install_location: evaluator::custom_install_location(&request).map(PathBuf::from), capture_output: request.capture_output, cancel_token: tokio_util::sync::CancellationToken::new(), event_sink: None, @@ -790,19 +792,39 @@ impl BrokerState { )); } + let execution_elevation = evaluator::effective_execution_elevation(request); + if request.client.requested_elevation == Elevation::Elevated && execution_elevation == Elevation::Standard { + info!( + request_id = %request.request_id, + "Running elevated request with the standard token because it targets the user scope" + ); + } + + // SECURITY: The broker never removes the Devolutions Agent, which hosts it. Like the + // gates below, this is not bypassable by policy rules, `defaultDecision`, or audit mode. + if evaluator::uninstalls_protected_package(request) { + warn!( + request_id = %request.request_id, + package_id = %request.package.id, + "Rejecting request: the package is protected from uninstallation" + ); + return Err(error_response( + ErrorCode::ValidationFailed, + "the package broker does not uninstall the Devolutions Agent", + )); + } + // 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 // code execution and make the package allowlist moot, so they are only - // accepted for non-elevated execution (standard elevation and non-machine - // scope; machine scope also elevates the execution token). + // accepted when the execution token is standard (see + // `evaluator::effective_execution_elevation`; machine scope elevates it). // This gate is intentionally not bypassable by policy rules, `defaultDecision`, // or audit mode; revisit once the policy schema supports a content allowlist. let has_pre_post_commands = request.options.pre_operation_command.is_some() || request.options.post_operation_command.is_some(); - let requires_elevation = - request.client.requested_elevation == Elevation::Elevated || request.options.scope == Some(Scope::Machine); - if has_pre_post_commands && requires_elevation { + if has_pre_post_commands && execution_elevation == Elevation::Elevated { warn!( request_id = %request.request_id, "Rejecting request: pre/post operation commands are not allowed for elevated execution" @@ -813,6 +835,20 @@ impl BrokerState { )); } + // Kill-before-operation entries are passed to `taskkill /IM`, which accepts wildcards. + // Like the gates above, this is not bypassable by audit mode. + if evaluator::has_unacceptable_kill_process_name(request) { + warn!( + request_id = %request.request_id, + "Rejecting request: unacceptable kill-before-operation process name" + ); + return Err(error_response( + ErrorCode::ValidationFailed, + "kill-before-operation entries must be process names without an extension or ending in .exe, \ + without wildcards, path separators, quotes, or control characters", + )); + } + // A custom install location (typed or passed through WinGet custom parameters) must be a // single plain local drive path. Like the gates above, this is not bypassable by audit mode. if evaluator::has_unacceptable_install_location(request) { @@ -930,6 +966,11 @@ fn normalize_request(mut request: PackageRequest) -> PackageRequest { { request.options.custom_install_location = None; } + for process in &mut request.options.kill_before_operation { + if let Cow::Owned(name) = evaluator::normalize_kill_process_name(&process.0) { + process.0 = name; + } + } request } @@ -945,6 +986,7 @@ mod tests { use chrono::Utc; use now_policy::{PolicyEnforcement, PolicyFormatVersion, PolicyMetadata, ResourceId}; use now_policy_api as api; + use now_policy_api::Scope; use tower_service::Service as _; use super::*; @@ -1604,6 +1646,90 @@ mod tests { assert_eq!(error.code, ErrorCode::ValidationFailed); } + #[test] + fn elevated_user_scope_pre_post_commands_are_accepted() { + // Explicit user scope runs with the standard token even when elevation is requested. + let mut request = request(); + request.client.requested_elevation = Elevation::Elevated; + request.options.scope = Some(Scope::User); + request.options.pre_operation_command = Some("echo before".to_owned()); + request.options.post_operation_command = Some("echo after".to_owned()); + + let Ok(evaluated) = state().evaluate_request(&request) else { + panic!("expected elevated user-scope pre/post commands to be accepted"); + }; + assert!(evaluated.would_execute); + } + + #[test] + fn unacceptable_kill_process_names_are_rejected_even_under_permissive_policy() { + for name in [ + "*", + "Code*.exe", + "C?de.exe", + r"C:\Tools\Code.exe", + "dir/Code.exe", + "\"Code.exe\"", + "chrome.bat", + "*", + ] { + let mut request = request(); + request.options.kill_before_operation = vec![api::ProcessName(name.to_owned())]; + + let Err(error) = state().evaluate_request(&normalize_request(request)) else { + panic!("expected kill-before-operation entry {name:?} to be rejected"); + }; + assert_eq!(error.code, ErrorCode::ValidationFailed, "{name:?}"); + } + + for name in ["Code.exe", "Chrome.EXE"] { + let mut request = request(); + request.options.kill_before_operation = vec![api::ProcessName(name.to_owned())]; + assert!( + state().evaluate_request(&normalize_request(request)).is_ok(), + "{name:?}" + ); + } + } + + #[tokio::test] + async fn kill_process_names_without_an_extension_execute_with_exe_appended() { + let executor = Arc::new(CapturingExecutor::default()); + let state = state_with_executor(Arc::clone(&executor) as Arc); + let mut request = request(); + request.options.kill_before_operation = vec![ + api::ProcessName("chrome".to_owned()), + api::ProcessName("Code.EXE".to_owned()), + ]; + assert_eq!( + normalize_request(request.clone()) + .options + .kill_before_operation + .iter() + .map(|process| process.0.as_str()) + .collect::>(), + ["chrome.exe", "Code.EXE"] + ); + + let operation = submit_operation(&state, &request).await; + wait_for_status(&state, &request, &operation.operation_id).await; + + let contexts = executor.contexts.lock().expect("contexts lock"); + assert_eq!(contexts[0].kill_processes, ["chrome.exe", "Code.EXE"]); + } + + #[test] + fn agent_uninstall_is_rejected_even_under_permissive_policy() { + let mut request = request(); + request.operation = api::Operation::Uninstall; + request.package.id = api::PackageIdentifier("Devolutions.Agent".to_owned()); + + let Err(error) = state().evaluate_request(&request) else { + panic!("expected the Agent uninstall to be rejected"); + }; + assert_eq!(error.code, ErrorCode::ValidationFailed); + } + #[test] fn non_elevated_pre_post_commands_are_accepted() { let mut request = request(); @@ -1829,6 +1955,52 @@ mod tests { } } + /// Executor that records the execution context and completes instantly. + #[derive(Default)] + struct CapturingExecutor { + contexts: std::sync::Mutex>, + } + + #[async_trait] + impl CommandExecutor for CapturingExecutor { + async fn execute( + &self, + ctx: &ExecutionContext, + _process_started: Option, + ) -> anyhow::Result { + self.contexts.lock().expect("contexts lock").push(ctx.clone()); + Ok(ExecutionOutput { + exit_code: 0, + stdout: String::new(), + started_at: Some(Utc::now()), + }) + } + } + + #[tokio::test] + async fn elevated_user_scope_request_executes_with_the_standard_token() { + let executor = Arc::new(CapturingExecutor::default()); + let state = state_with_executor(Arc::clone(&executor) as Arc); + let mut request = request(); + request.client.requested_elevation = Elevation::Elevated; + request.options.scope = Some(Scope::User); + request.options.custom_install_location = Some(r"c:\Tools\Contoso".to_owned()); + + let operation = submit_operation(&state, &request).await; + wait_for_status(&state, &request, &operation.operation_id).await; + + let contexts = executor.contexts.lock().expect("contexts lock"); + let [context] = contexts.as_slice() else { + panic!("expected one execution, got {}", contexts.len()); + }; + assert_eq!(context.elevation, Elevation::Standard); + assert!(!context.requires_elevation()); + assert_eq!( + context.custom_install_location.as_deref(), + Some(std::path::Path::new(r"C:\Tools\Contoso")) + ); + } + fn state_with_executor(executor: Arc) -> BrokerState { BrokerState { policy_store: PolicyStore::for_tests(Some(permissive_policy())), From a77663d5da0f6c1128ec0e8ba07e837f50a5c579 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Tue, 6 Oct 2026 15:19:27 +0900 Subject: [PATCH 02/11] fix(agent): run broker plans in the client session with a matching token Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- crates/now-package-broker/src/auth.rs | 14 +++ .../now-package-broker/src/evaluator/mod.rs | 5 +- crates/now-package-broker/src/executor/mod.rs | 5 ++ .../src/executor/windows/mod.rs | 89 +++++++++++++++---- .../src/executor/windows/token.rs | 31 +++++++ crates/now-package-broker/src/server/mod.rs | 23 +++-- 6 files changed, 142 insertions(+), 25 deletions(-) diff --git a/crates/now-package-broker/src/auth.rs b/crates/now-package-broker/src/auth.rs index 87a0cd0e96..6fe592c380 100644 --- a/crates/now-package-broker/src/auth.rs +++ b/crates/now-package-broker/src/auth.rs @@ -60,6 +60,8 @@ pub(crate) struct PipeClient { executable_security: Option>, /// Security identifier of the pipe client process token user, captured at connect. user_sid: Sid, + /// Session of the pipe client process token, captured at connect. + session_id: u32, /// Actual connected process token elevation, captured at connect. is_elevated: bool, /// Enabled built-in Administrators membership, captured at connect. @@ -183,6 +185,9 @@ impl PipeClient { .sid_and_attributes() .with_context(|| format!("failed to query pipe client process {process_id} token user"))? .sid; + let session_id = token + .session_id() + .with_context(|| format!("failed to query pipe client process {process_id} token session"))?; let is_elevated = token .is_elevated() .with_context(|| format!("failed to query pipe client process {process_id} token elevation"))?; @@ -207,6 +212,7 @@ impl PipeClient { executable_file: Some(executable_file), executable_security, user_sid, + session_id, is_elevated, is_administrator, impersonation_token: Some(impersonation_token), @@ -231,6 +237,11 @@ impl PipeClient { &self.user_sid } + /// Session of the authenticated pipe client, captured at connect. + pub(crate) fn session_id(&self) -> u32 { + self.session_id + } + pub(crate) fn executable_path(&self) -> &Path { &self.executable_path } @@ -551,6 +562,7 @@ mod tests { executable_file: None, executable_security: None, user_sid: system_sid(), + session_id: 1, is_elevated: true, is_administrator: true, impersonation_token: None, @@ -1251,6 +1263,7 @@ mod tests { executable_file: Some(Arc::new(executable_file)), executable_security: Some(Arc::new(RetainedExecutableSecurity::unchecked_for_tests())), user_sid: client_user_sid(), + session_id: 1, is_elevated: false, is_administrator: false, impersonation_token: None, @@ -1276,6 +1289,7 @@ mod tests { executable_file: None, executable_security: None, user_sid: client_user_sid(), + session_id: 1, is_elevated: false, is_administrator: false, impersonation_token: None, diff --git a/crates/now-package-broker/src/evaluator/mod.rs b/crates/now-package-broker/src/evaluator/mod.rs index 016107d8a0..31c6a937f7 100644 --- a/crates/now-package-broker/src/evaluator/mod.rs +++ b/crates/now-package-broker/src/evaluator/mod.rs @@ -45,9 +45,12 @@ //! can select regardless of the policy: //! - An explicit user scope, typed or passed through custom parameters, always runs with the standard //! token (see `effective_execution_elevation`); policy matching and execution use the same elevation. +//! The executor refuses to run a standard plan with an elevated token, such as the full token of an +//! administrator when UAC is disabled. //! - Kill-before-operation entries must be plain `.exe` image names; process names without an //! extension get `.exe` appended. -//! `taskkill` only targets the requester's session, and the requester's own processes when elevated. +//! `taskkill` only targets the session of the authenticated client, and the requester's own processes +//! when elevated. //! - An elevated custom install location must be on a local disk, contain no reparse point, and not be //! writable by principals other than SYSTEM, Administrators and TrustedInstaller, including through //! inheritable ACEs. diff --git a/crates/now-package-broker/src/executor/mod.rs b/crates/now-package-broker/src/executor/mod.rs index 308622c6fb..cd3296908e 100644 --- a/crates/now-package-broker/src/executor/mod.rs +++ b/crates/now-package-broker/src/executor/mod.rs @@ -52,6 +52,10 @@ pub struct ExecutionContext { /// Session selection uses this SID so distinct accounts sharing the same name /// (e.g. `MACHINE\alice` vs `DOMAIN\alice`) cannot be confused with one another. pub user_sid: Sid, + /// Session of the authenticated pipe client, captured from its token at connect. + /// + /// The plan runs in this session, and kill-before-operation only targets processes in it. + pub session_id: u32, /// Execution elevation, as decided by `evaluator::effective_execution_elevation`. pub elevation: Elevation, /// Installation scope (machine scope requires elevation). @@ -158,6 +162,7 @@ impl CommandExecutor for DryRunExecutor { info!( effective_user = %ctx.effective_user, user_sid = %ctx.user_sid, + session_id = ctx.session_id, kill_processes = ?ctx.kill_processes, has_pre_command = ctx.pre_command.is_some(), command_len = ctx.command.len(), diff --git a/crates/now-package-broker/src/executor/windows/mod.rs b/crates/now-package-broker/src/executor/windows/mod.rs index 1e97d52c74..d4e5e64065 100644 --- a/crates/now-package-broker/src/executor/windows/mod.rs +++ b/crates/now-package-broker/src/executor/windows/mod.rs @@ -32,7 +32,7 @@ mod user_env; use privileges::SharedPrivileges; use process::{OutputCapture, create_process}; -use token::{detect_running_as_system, find_user_session, get_elevated_token}; +use token::{detect_running_as_system, find_user_session, get_elevated_token, user_session_token}; use user_env::UserEnv; /// Windows command executor using `win-api-wrappers` safe abstractions. @@ -202,7 +202,7 @@ fn path_contains_executable(env: &UserEnv<'_>, names: &[&str]) -> bool { /// Execute a command in the context of the target user's session (SYSTEM mode). /// /// Steps: -/// 1. Find the user's active session (and its token) by matching the session token SID. +/// 1. Query the user token of the pipe client's session, which must belong to the client user. /// 2. If elevated execution is requested, obtain the linked elevated token. /// 3. Set the token session ID and create the process. /// 4. Wait for the process to exit and return the exit code. @@ -230,10 +230,11 @@ fn execute_as_system( ]) .context("failed to enable privileges required for SYSTEM-mode execution")?; - debug!("All privileges enabled, finding user session"); + debug!("All privileges enabled, querying the pipe client session"); - let (session_id, user_token) = find_user_session(&ctx.user_sid).context( - "failed to find an active logon session for the target user; \ + let session_id = ctx.session_id; + let user_token = user_session_token(session_id, &ctx.user_sid).context( + "failed to use the pipe client session; \ user-scope and interactive operations require the user to be logged on", )?; @@ -317,6 +318,13 @@ fn execute_as_current_user( } let session_id = token.session_id().context("failed to query token session ID")?; + if session_id != ctx.session_id { + bail!( + "pipe client session {} does not match broker process session {session_id}; \ + execution is only supported in the broker session when the broker is not running as SYSTEM", + ctx.session_id, + ); + } let output = run_plan(&token, ctx, session_id, process_started)?; @@ -345,18 +353,25 @@ fn run_plan( bail!("elevated Bun package operations are not supported by the broker"); } + // SECURITY: The request was evaluated for the elevation `requires_elevation` reports, but the + // token actually running the plan is what matters (e.g. a broker launched from an elevated + // shell, or a full session token when UAC is disabled), so query the token itself. + // Running a standard plan elevated would apply standard-execution policy rules, and skip the + // elevated safeguards, while the administrator token runs without a UAC prompt. + let token_is_elevated = token + .is_elevated() + .context("failed to query execution token elevation")?; + if token_is_elevated && !requires_elevation { + bail!( + "the execution token is elevated but the request was evaluated for standard execution; \ + standard execution requires a non-elevated user token" + ); + } + // SECURITY: Pre/post commands are raw strings whose content is not governed by - // the policy, so they must never run elevated. The request flags are already - // checked upstream, but the token actually running the plan is what matters - // (e.g. a broker launched from an elevated shell, or a full session token when - // UAC is disabled), so query the token itself right before running. - if ctx.pre_command.is_some() || ctx.post_command.is_some() { - let is_elevated = token - .is_elevated() - .context("failed to query execution token elevation")?; - if is_elevated { - bail!("pre/post operation commands are only allowed for non-elevated execution"); - } + // the policy, so they must never run elevated. + if token_is_elevated && (ctx.pre_command.is_some() || ctx.post_command.is_some()) { + bail!("pre/post operation commands are only allowed for non-elevated execution"); } // Defense in depth: the server rejects these names before policy evaluation. @@ -2219,6 +2234,7 @@ mod tests { post_command: None, effective_user: "DOMAIN\\user".to_owned(), user_sid: Sid::from_well_known(WinWorldSid, None).expect("well-known Everyone SID"), + session_id: 1, elevation: Elevation::Elevated, scope: None, custom_install_location: None, @@ -2281,6 +2297,7 @@ mod tests { post_command: None, effective_user: "DOMAIN\\user".to_owned(), user_sid: Sid::from_well_known(WinWorldSid, None).expect("well-known Everyone SID"), + session_id: 1, elevation: Elevation::Elevated, scope: None, custom_install_location: None, @@ -2311,6 +2328,7 @@ mod tests { effective_user: "DOMAIN\\other".to_owned(), // The Everyone (World) SID never matches the test process user SID. user_sid: Sid::from_well_known(WinWorldSid, None).expect("well-known Everyone SID"), + session_id: 1, elevation: Elevation::Standard, scope: Some(Scope::User), custom_install_location: None, @@ -2323,6 +2341,45 @@ mod tests { assert!(error.to_string().contains("does not match broker process user SID")); } + #[test] + fn non_system_mode_rejects_client_session_different_from_broker_session() { + let token = win_api_wrappers::process::Process::current_process() + .token(windows::Win32::Security::TOKEN_QUERY) + .expect("open current process token"); + let ctx = ExecutionContext { + kill_processes: vec!["notepad.exe".to_owned()], + pre_command: None, + command: vec![ + r"C:\Windows\System32\cmd.exe".to_owned(), + "/C".to_owned(), + "exit".to_owned(), + ], + post_command: None, + effective_user: "DOMAIN\\user".to_owned(), + user_sid: token.sid_and_attributes().expect("query token user SID").sid, + session_id: token.session_id().expect("query token session ID") + 1, + elevation: Elevation::Standard, + scope: Some(Scope::User), + custom_install_location: None, + capture_output: false, + cancel_token: tokio_util::sync::CancellationToken::new(), + event_sink: None, + }; + + let error = execute_as_current_user(&ctx, None).expect_err("mismatched client session should fail"); + assert!( + error.to_string().contains("does not match broker process session"), + "{error:#}" + ); + } + + #[test] + fn session_zero_is_never_used_for_execution() { + let sid = Sid::from_well_known(WinWorldSid, None).expect("well-known Everyone SID"); + let error = super::user_session_token(0, &sid).expect_err("session 0 must be rejected"); + assert!(error.to_string().contains("session 0"), "{error:#}"); + } + #[test] fn pip_command_resolves_python_without_shell_wrapper() { let temp_dir = tempfile::tempdir().expect("create temp dir"); diff --git a/crates/now-package-broker/src/executor/windows/token.rs b/crates/now-package-broker/src/executor/windows/token.rs index 52edef939e..1f9f5a2844 100644 --- a/crates/now-package-broker/src/executor/windows/token.rs +++ b/crates/now-package-broker/src/executor/windows/token.rs @@ -73,6 +73,37 @@ pub(super) fn find_user_session(user_sid: &Sid) -> anyhow::Result<(u32, Token)> bail!("no active session found for user SID '{user_sid}'") } +/// Return the user token of the active session `session_id`, requiring it to belong to `user_sid`. +/// +/// Execution uses the session of the authenticated pipe client rather than any session of the +/// same user, so a user logged on several times only affects the requesting session. +/// The caller must have the SeTcb privilege enabled (required by `WTSQueryUserToken`). +pub(super) fn user_session_token(session_id: u32, user_sid: &Sid) -> anyhow::Result { + if session_id == 0 { + bail!("the pipe client runs in session 0, which has no interactive user"); + } + + let sessions = wts::get_sessions().context("failed to enumerate WTS sessions")?; + let is_active = sessions + .iter() + .any(|session| session.session_id == session_id && session.state == wts::WTSConnectState::Active); + if !is_active { + bail!("pipe client session {session_id} is not an active session"); + } + + let token = Token::for_session(session_id) + .with_context(|| format!("failed to query the user token of session {session_id}"))?; + let session_user = token + .sid_and_attributes() + .with_context(|| format!("failed to query the user of session {session_id}"))? + .sid; + if session_user != *user_sid { + bail!("session {session_id} belongs to '{session_user}', not to the pipe client user '{user_sid}'"); + } + + Ok(token) +} + /// Attempt to obtain an elevated (linked) token from a filtered/limited token. /// /// On UAC-enabled systems with split tokens, the standard user token has a linked diff --git a/crates/now-package-broker/src/server/mod.rs b/crates/now-package-broker/src/server/mod.rs index 5cc5de4506..9164685f9f 100644 --- a/crates/now-package-broker/src/server/mod.rs +++ b/crates/now-package-broker/src/server/mod.rs @@ -492,7 +492,9 @@ impl PackageBrokerServer for BrokerConnection { error_response(ErrorCode::Unauthorized, "pipe client authentication failed") })?; - self.state.execute(request, self.client.user_sid()).await + self.state + .execute(request, self.client.user_sid(), self.client.session_id()) + .await } async fn status(&self, request: StatusRequest) -> Result { @@ -612,7 +614,12 @@ impl BrokerState { }) } - async fn execute(&self, request: PackageRequest, user_sid: &Sid) -> Result { + async fn execute( + &self, + request: PackageRequest, + user_sid: &Sid, + session_id: u32, + ) -> Result { let request = normalize_request(request); let evaluated = self.evaluate_request(&request)?; let operation = if evaluated.would_execute { @@ -630,6 +637,7 @@ impl BrokerState { post_command: request.options.post_operation_command.clone(), effective_user: request.client.effective_user.clone(), user_sid: user_sid.clone(), + session_id, elevation: evaluator::effective_execution_elevation(&request), scope: request.options.scope, custom_install_location: evaluator::custom_install_location(&request).map(PathBuf::from), @@ -2033,14 +2041,13 @@ mod tests { async fn submit_operation(state: &BrokerState, request: &PackageRequest) -> OperationSubmission { // Use the real test-process user SID: the per-operation event pipe ACL only // admits the requesting client user (plus SYSTEM/Administrators). - let user_sid = win_api_wrappers::process::Process::current_process() + let token = win_api_wrappers::process::Process::current_process() .token(windows::Win32::Security::TOKEN_QUERY) - .expect("open current process token") - .sid_and_attributes() - .expect("query token user SID") - .sid; + .expect("open current process token"); + let user_sid = token.sid_and_attributes().expect("query token user SID").sid; + let session_id = token.session_id().expect("query token session ID"); let response = state - .execute(request.clone(), &user_sid) + .execute(request.clone(), &user_sid, session_id) .await .expect("execute request accepted"); response.operation.expect("operation submitted") From a4a02310da38af7f9da89bbec09f3c5e404d7ec3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Tue, 6 Oct 2026 15:36:48 +0900 Subject: [PATCH 03/11] fix(agent): keep supplied install locations verified and pinned Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../samples/deny-risky-options.policy.json | 19 +++++++++++ ...winget-endpoint-agent-replace.request.json | 33 +++++++++++++++++++ .../samples/scenarios/baseline.scenarios.json | 8 +++++ .../src/evaluator/constraints.rs | 1 + .../now-package-broker/src/evaluator/mod.rs | 26 +++++++++++---- .../now-package-broker/src/evaluator/tests.rs | 23 +++++++++++++ .../now-package-broker/src/policy_security.rs | 6 ++-- 7 files changed, 106 insertions(+), 10 deletions(-) create mode 100644 crates/now-package-broker/src/assets/samples/requests/winget-endpoint-agent-replace.request.json diff --git a/crates/now-package-broker/src/assets/samples/deny-risky-options.policy.json b/crates/now-package-broker/src/assets/samples/deny-risky-options.policy.json index fc9bed64c6..6daed4ab6d 100644 --- a/crates/now-package-broker/src/assets/samples/deny-risky-options.policy.json +++ b/crates/now-package-broker/src/assets/samples/deny-risky-options.policy.json @@ -86,6 +86,25 @@ ] } } + }, + { + "Id": "deny.elevated-replace-critical-software", + "Priority": 60, + "Decision": "Deny", + "Reason": "Updates that remove the previous version of critical software first cannot run through the broker.", + "Match": { + "HasUninstallPrevious": true, + "ExecutionElevation": [ + "Elevated" + ], + "PackageIdentifiers": { + "Patterns": [ + "Contoso.EndpointProtection*", + "Contoso.Vpn*", + "Contoso.DeviceManagement*" + ] + } + } } ] } diff --git a/crates/now-package-broker/src/assets/samples/requests/winget-endpoint-agent-replace.request.json b/crates/now-package-broker/src/assets/samples/requests/winget-endpoint-agent-replace.request.json new file mode 100644 index 0000000000..b7abfc5b99 --- /dev/null +++ b/crates/now-package-broker/src/assets/samples/requests/winget-endpoint-agent-replace.request.json @@ -0,0 +1,33 @@ +{ + "RequestKind": "PackageRequest", + "RequestVersion": "1.0", + "RequestId": "req-winget-endpoint-agent-replace", + "CreatedAt": "2026-05-05T12:00:00Z", + "Operation": "Update", + "Manager": "Winget", + "Source": { + "Name": "winget", + "Url": "https://cdn.winget.microsoft.com/cache" + }, + "Package": { + "Id": "Contoso.EndpointProtection.Agent", + "Version": "2.0.0" + }, + "Options": { + "Scope": "Machine", + "Interactive": false, + "SkipHashCheck": false, + "PreRelease": false, + "CustomParameters": [], + "KillBeforeOperation": [], + "UninstallPrevious": true + }, + "Client": { + "RequestedElevation": "Elevated", + "EffectiveUser": "CONTOSO\\alice", + "ClientVersion": "3.2.0", + "Transport": "HttpNamedPipe", + "ClientExecutablePath": "C:\\Program Files\\Devolutions\\NOW\\now-client.exe" + }, + "IncludeCommandPreview": true +} diff --git a/crates/now-package-broker/src/assets/samples/scenarios/baseline.scenarios.json b/crates/now-package-broker/src/assets/samples/scenarios/baseline.scenarios.json index e05e97d9d9..37aa57480c 100644 --- a/crates/now-package-broker/src/assets/samples/scenarios/baseline.scenarios.json +++ b/crates/now-package-broker/src/assets/samples/scenarios/baseline.scenarios.json @@ -41,6 +41,14 @@ "ExpectedDecision": "Deny", "ExpectedRuleId": "deny.elevated-uninstall-critical-software", "Tags": ["baseline", "winget", "default-allow", "uninstall"] + }, + { + "Id": "baseline.default-allow.critical-software-replace.deny", + "Policy": "deny-risky-options.policy.json", + "Request": "requests/winget-endpoint-agent-replace.request.json", + "ExpectedDecision": "Deny", + "ExpectedRuleId": "deny.elevated-replace-critical-software", + "Tags": ["baseline", "winget", "default-allow", "uninstall"] } ] } \ No newline at end of file diff --git a/crates/now-package-broker/src/evaluator/constraints.rs b/crates/now-package-broker/src/evaluator/constraints.rs index b04b2e6f6c..f3fce2154b 100644 --- a/crates/now-package-broker/src/evaluator/constraints.rs +++ b/crates/now-package-broker/src/evaluator/constraints.rs @@ -143,6 +143,7 @@ mod tests { has_uninstall_previous: false, no_upgrade: false, custom_install_location: None, + supplied_install_location: None, custom_parameters: Vec::new(), } } diff --git a/crates/now-package-broker/src/evaluator/mod.rs b/crates/now-package-broker/src/evaluator/mod.rs index 31c6a937f7..efb27be9f0 100644 --- a/crates/now-package-broker/src/evaluator/mod.rs +++ b/crates/now-package-broker/src/evaluator/mod.rs @@ -54,9 +54,10 @@ //! - An elevated custom install location must be on a local disk, contain no reparse point, and not be //! writable by principals other than SYSTEM, Administrators and TrustedInstaller, including through //! inheritable ACEs. -//! The executor checks the nearest existing folder before running the package manager. -//! An install location selected by opaque installer arguments (WinGet `--override` and `--custom`) is -//! not checked. +//! The executor checks the nearest existing folder before running the package manager and keeps it +//! from being renamed until the operation completes. +//! A supplied location is checked even with opaque installer arguments (WinGet `--override` and +//! `--custom`), but a location selected only inside those arguments is not. //! - The broker never uninstalls the Devolutions Agent, which hosts it. //! WinGet may also select installed programs by their `ARP\...` identifiers, which this protection //! does not cover; add Deny rules for other critical software and for such identifiers. @@ -107,6 +108,9 @@ struct RequestFlags { no_upgrade: bool, /// Normalized custom install location, or `None` when it is absent, unknown or unacceptable. custom_install_location: Option, + /// Normalized supplied install location, typed or passed through custom parameters, even when + /// installer arguments may override it. + supplied_install_location: Option, custom_parameters: Vec, } @@ -128,6 +132,8 @@ impl RequestFlags { _ => Some(None), }; + let supplied_install_location = supplied_location.clone().flatten(); + Self { interactive: request.options.interactive || custom.interactive, skip_hash_check: request.options.skip_hash_check || custom.skip_hash_check, @@ -140,7 +146,10 @@ impl RequestFlags { has_uninstall_previous: request.options.uninstall_previous || custom.uninstall_previous, no_upgrade: request.options.no_upgrade || custom.no_upgrade, // Installer arguments may override a supplied location. - custom_install_location: supplied_location.flatten().filter(|_| !custom.installer_arguments), + custom_install_location: supplied_install_location + .clone() + .filter(|_| !custom.installer_arguments), + supplied_install_location, custom_parameters: request .options .custom_parameters @@ -159,10 +168,13 @@ pub(crate) fn has_unacceptable_install_location(request: &PackageRequest) -> boo RequestFlags::from_request(request).has_unacceptable_install_location } -/// Normalized custom install location the package manager will use, typed or passed through -/// custom parameters, or `None` when there is none or it is unknown. +/// Normalized install location the request supplies, typed or passed through custom parameters, +/// or `None` when there is none. +/// +/// Unlike policy matching, this keeps a supplied location even when opaque installer arguments +/// may override it, because the package manager still receives it. pub(crate) fn custom_install_location(request: &PackageRequest) -> Option { - RequestFlags::from_request(request).custom_install_location + RequestFlags::from_request(request).supplied_install_location } /// Whether a kill-before-operation entry, after [`normalize_kill_process_name`], is not an diff --git a/crates/now-package-broker/src/evaluator/tests.rs b/crates/now-package-broker/src/evaluator/tests.rs index 83a8e39b61..0d4b562500 100644 --- a/crates/now-package-broker/src/evaluator/tests.rs +++ b/crates/now-package-broker/src/evaluator/tests.rs @@ -813,3 +813,26 @@ fn custom_parameter_user_scope_lowers_execution_elevation() { request.options.custom_parameters = custom(&["--scope", "user", "--scope", "machine"]); assert_eq!(super::effective_execution_elevation(&request), api::Elevation::Elevated); } + +#[test] +fn supplied_install_location_is_kept_for_security_checks_when_installer_arguments_are_present() { + let mut request = make_request(api::Operation::Install, "Contoso.Tool"); + request.options.custom_install_location = Some(r"c:\Users\alice\Downloads\App".to_owned()); + request.options.custom_parameters = vec![api::CustomParameterString("--custom=/quiet".to_owned())]; + assert_eq!( + super::custom_install_location(&request).as_deref(), + Some(r"C:\Users\alice\Downloads\App") + ); + + request.options.custom_install_location = None; + request.options.custom_parameters = vec![ + api::CustomParameterString("--location".to_owned()), + api::CustomParameterString(r"C:\Tools\App".to_owned()), + api::CustomParameterString("--override".to_owned()), + api::CustomParameterString("/S".to_owned()), + ]; + assert_eq!( + super::custom_install_location(&request).as_deref(), + Some(r"C:\Tools\App") + ); +} diff --git a/crates/now-package-broker/src/policy_security.rs b/crates/now-package-broker/src/policy_security.rs index 6be0ab6d1d..efcf6f0425 100644 --- a/crates/now-package-broker/src/policy_security.rs +++ b/crates/now-package-broker/src/policy_security.rs @@ -853,12 +853,12 @@ pub(crate) fn verify_elevated_install_location( opener: &dyn PathOpener, location: &Path, ) -> anyhow::Result { - // Delete sharing lets the installer replace an existing install folder; non-administrators - // cannot, since the DACL check below rejects delete rights for them. + // Delete sharing is not granted: a renamed verified folder could be replaced by a junction + // while the guard is held, so the installer cannot rename or delete it during the operation. let mut options = OpenOptions::new(); options .access_mode(FILE_READ_ATTRIBUTES.0 | READ_CONTROL.0) - .share_mode(FILE_SHARE_READ.0 | FILE_SHARE_WRITE.0 | FILE_SHARE_DELETE.0) + .share_mode(FILE_SHARE_READ.0 | FILE_SHARE_WRITE.0) .custom_flags((FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OPEN_REPARSE_POINT).0); let mut folder = location; From 40184ed82ddeaa272561c6b830737da6f8a6efe2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Tue, 6 Oct 2026 15:56:15 +0900 Subject: [PATCH 04/11] fix(agent): protect the installed Agent from WinGet ARP uninstalls Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../src/evaluator/builtin_rules.rs | 26 ++++++++++++-- .../now-package-broker/src/evaluator/mod.rs | 30 ++++++++++++---- .../now-package-broker/src/evaluator/tests.rs | 34 +++++++++++++++++++ crates/now-package-broker/src/server/mod.rs | 28 ++++++++++++++- 4 files changed, 108 insertions(+), 10 deletions(-) diff --git a/crates/now-package-broker/src/evaluator/builtin_rules.rs b/crates/now-package-broker/src/evaluator/builtin_rules.rs index c8f0993998..88a441d06c 100644 --- a/crates/now-package-broker/src/evaluator/builtin_rules.rs +++ b/crates/now-package-broker/src/evaluator/builtin_rules.rs @@ -2,7 +2,8 @@ use std::borrow::Cow; -use now_policy_api::{ManagerName, Operation, PackageRequest}; +use now_policy_api::{ManagerName, PackageRequest}; +use uuid::Uuid; use super::RequestFlags; use super::identifier::selects_identifier; @@ -25,13 +26,32 @@ const PROTECTED_PACKAGES: &[(ManagerName, &str)] = &[ /// Whether the request uninstalls a [`PROTECTED_PACKAGES`] entry, directly or as the previous /// version replaced by an install or update. pub(super) fn uninstalls_protected_package(request: &PackageRequest, flags: &RequestFlags) -> bool { - let removes_package = request.operation == Operation::Uninstall || flags.has_uninstall_previous; - removes_package + flags.removes_package(request) && PROTECTED_PACKAGES.iter().any(|(manager, identifier)| { *manager == request.manager && selects_identifier(request.manager, &request.package.id.0, identifier) }) } +/// Whether a WinGet request that removes a package references the MSI `product_code`, in its +/// package identifier or custom parameters. +/// +/// WinGet selects installed programs by `ARP\\\{ProductCode}` identifiers, +/// and the product code of an MSI changes with each version, so the caller resolves it from the +/// installed product. +pub(super) fn uninstalls_product_code(request: &PackageRequest, flags: &RequestFlags, product_code: Uuid) -> bool { + let product_code = product_code.hyphenated().to_string(); + let references_product_code = |value: &str| value.to_ascii_lowercase().contains(&product_code); + + request.manager == ManagerName::Winget + && flags.removes_package(request) + && (references_product_code(&request.package.id.0) + || request + .options + .custom_parameters + .iter() + .any(|parameter| references_product_code(¶meter.0))) +} + /// Kill-before-operation entry as passed to `taskkill /IM`: `.exe` is appended to a name without /// an extension, as clients may send bare process names (`chrome`). /// diff --git a/crates/now-package-broker/src/evaluator/mod.rs b/crates/now-package-broker/src/evaluator/mod.rs index efb27be9f0..0672effce8 100644 --- a/crates/now-package-broker/src/evaluator/mod.rs +++ b/crates/now-package-broker/src/evaluator/mod.rs @@ -58,9 +58,9 @@ //! from being renamed until the operation completes. //! A supplied location is checked even with opaque installer arguments (WinGet `--override` and //! `--custom`), but a location selected only inside those arguments is not. -//! - The broker never uninstalls the Devolutions Agent, which hosts it. -//! WinGet may also select installed programs by their `ARP\...` identifiers, which this protection -//! does not cover; add Deny rules for other critical software and for such identifiers. +//! - The broker never uninstalls the Devolutions Agent, which hosts it, whether it is selected by its +//! package identifier or, for WinGet, by the `ARP\...` identifier of its installed MSI product. +//! Add Deny rules to protect other critical software, including its `ARP\...` identifiers. //! - None of these restrictions depend on the policy, so audit mode cannot override them. //! Audit mode allows every other request, including elevated ones the rules deny, and the policy //! validator warns about this whenever audit mode is enabled. @@ -160,6 +160,14 @@ impl RequestFlags { } } +impl RequestFlags { + /// Whether the request removes an installed package, directly or as the previous version + /// replaced by an install or update. + fn removes_package(&self, request: &PackageRequest) -> bool { + request.operation == now_policy_api::Operation::Uninstall || self.has_uninstall_previous + } +} + /// Whether the request supplies a custom install location that is not a single plain local drive path. /// /// The server rejects such requests before policy evaluation so audit mode cannot override the rejection; @@ -192,10 +200,20 @@ pub(crate) fn has_unacceptable_kill_process_name(request: &PackageRequest) -> bo /// Whether the request uninstalls a package the broker protects, such as the Devolutions Agent. /// +/// `agent_product_code` is the MSI product code of the installed Agent, when known, so WinGet +/// requests selecting the Agent by its `ARP\...` identifier are covered too. /// The server rejects such requests before policy evaluation so audit mode cannot override the rejection; -/// [`evaluate`] also denies them for direct callers. -pub(crate) fn uninstalls_protected_package(request: &PackageRequest) -> bool { - builtin_rules::uninstalls_protected_package(request, &RequestFlags::from_request(request)) +/// [`evaluate`] also denies them for direct callers, by package identifier only. +pub(crate) fn uninstalls_protected_package(request: &PackageRequest, agent_product_code: Option) -> bool { + let flags = RequestFlags::from_request(request); + builtin_rules::uninstalls_protected_package(request, &flags) + || agent_product_code.is_some_and(|code| builtin_rules::uninstalls_product_code(request, &flags, code)) +} + +/// Whether the request removes an installed package, directly or as the previous version replaced +/// by an install or update. +pub(crate) fn removes_package(request: &PackageRequest) -> bool { + RequestFlags::from_request(request).removes_package(request) } /// Evaluate a parsed request against a parsed policy document. diff --git a/crates/now-package-broker/src/evaluator/tests.rs b/crates/now-package-broker/src/evaluator/tests.rs index 0d4b562500..c0c6d34997 100644 --- a/crates/now-package-broker/src/evaluator/tests.rs +++ b/crates/now-package-broker/src/evaluator/tests.rs @@ -836,3 +836,37 @@ fn supplied_install_location_is_kept_for_security_checks_when_installer_argument Some(r"C:\Tools\App") ); } + +#[test] +fn agent_uninstall_by_installed_product_code_is_detected() { + let product_code = uuid::uuid!("{0A1B2C3D-4E5F-4A6B-8C7D-9E0F1A2B3C4D}"); + let protects = |request: &PackageRequest| super::uninstalls_protected_package(request, Some(product_code)); + + let request = make_request( + api::Operation::Uninstall, + r"ARP\Machine\X64\{0A1B2C3D-4E5F-4A6B-8C7D-9E0F1A2B3C4D}", + ); + assert!(protects(&request)); + assert!(!super::uninstalls_protected_package(&request, None)); + + let mut request = make_request(api::Operation::Uninstall, "Contoso.Tool"); + request.options.custom_parameters = vec![ + api::CustomParameterString("--product-code".to_owned()), + api::CustomParameterString("{0a1b2c3d-4e5f-4a6b-8c7d-9e0f1a2b3c4d}".to_owned()), + ]; + assert!(protects(&request)); + + let mut request = make_request( + api::Operation::Update, + r"ARP\Machine\X64\{0A1B2C3D-4E5F-4A6B-8C7D-9E0F1A2B3C4D}", + ); + assert!(!protects(&request)); + request.options.uninstall_previous = true; + assert!(protects(&request)); + + let request = make_request( + api::Operation::Uninstall, + r"ARP\Machine\X64\{FFFFFFFF-4E5F-4A6B-8C7D-9E0F1A2B3C4D}", + ); + assert!(!protects(&request)); +} diff --git a/crates/now-package-broker/src/server/mod.rs b/crates/now-package-broker/src/server/mod.rs index 9164685f9f..f2c303d9b9 100644 --- a/crates/now-package-broker/src/server/mod.rs +++ b/crates/now-package-broker/src/server/mod.rs @@ -810,7 +810,7 @@ impl BrokerState { // SECURITY: The broker never removes the Devolutions Agent, which hosts it. Like the // gates below, this is not bypassable by policy rules, `defaultDecision`, or audit mode. - if evaluator::uninstalls_protected_package(request) { + if evaluator::uninstalls_protected_package(request, installed_agent_product_code(request)?) { warn!( request_id = %request.request_id, package_id = %request.package.id, @@ -965,6 +965,32 @@ impl PackageRequestClientOwner for PackageRequest { /// /// An empty or whitespace-only custom install location means "not set", so every component /// (policy evaluation, the command builders and operation tracking) sees `None`. +/// MSI product code of the installed Devolutions Agent, for requests that remove a package. +/// +/// Fails closed: a request that removes a package is rejected when the product code cannot be read. +#[expect( + clippy::result_large_err, + reason = "the shared API contract requires ErrorResponse values" +)] +fn installed_agent_product_code(request: &PackageRequest) -> Result, ErrorResponse> { + if !evaluator::removes_package(request) { + return Ok(None); + } + + devolutions_agent_shared::windows::registry::get_product_code(devolutions_agent_shared::windows::AGENT_UPDATE_CODE) + .map_err(|error| { + warn!( + request_id = %request.request_id, + error = format!("{error:#}"), + "Rejecting request: failed to read the installed Devolutions Agent product code" + ); + error_response( + ErrorCode::ValidationFailed, + "the package broker cannot verify that the request does not uninstall the Devolutions Agent", + ) + }) +} + fn normalize_request(mut request: PackageRequest) -> PackageRequest { if request .options From be61f906e0402db990e09491761e25720103dc5c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Tue, 6 Oct 2026 16:25:29 +0900 Subject: [PATCH 05/11] fix(agent): pin elevated install roots and clarify full-token errors Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../now-package-broker/src/evaluator/mod.rs | 10 ++-- .../src/executor/windows/mod.rs | 5 +- .../now-package-broker/src/policy_security.rs | 56 +++++++++++++++---- crates/now-package-broker/src/server/mod.rs | 9 +-- 4 files changed, 60 insertions(+), 20 deletions(-) diff --git a/crates/now-package-broker/src/evaluator/mod.rs b/crates/now-package-broker/src/evaluator/mod.rs index 0672effce8..ab22c32caf 100644 --- a/crates/now-package-broker/src/evaluator/mod.rs +++ b/crates/now-package-broker/src/evaluator/mod.rs @@ -45,8 +45,8 @@ //! can select regardless of the policy: //! - An explicit user scope, typed or passed through custom parameters, always runs with the standard //! token (see `effective_execution_elevation`); policy matching and execution use the same elevation. -//! The executor refuses to run a standard plan with an elevated token, such as the full token of an -//! administrator when UAC is disabled. +//! The executor refuses to run a standard plan with an elevated token: a full administrator token +//! without UAC (UAC disabled, or the built-in Administrator account) cannot use the broker. //! - Kill-before-operation entries must be plain `.exe` image names; process names without an //! extension get `.exe` appended. //! `taskkill` only targets the session of the authenticated client, and the requester's own processes @@ -54,8 +54,10 @@ //! - An elevated custom install location must be on a local disk, contain no reparse point, and not be //! writable by principals other than SYSTEM, Administrators and TrustedInstaller, including through //! inheritable ACEs. -//! The executor checks the nearest existing folder before running the package manager and keeps it -//! from being renamed until the operation completes. +//! The executor checks the nearest existing folder before running the package manager and holds it +//! open without delete sharing until the operation completes. +//! Installers that delete or rename their own existing install root, such as rename-and-swap +//! upgrades, fail there; installing into a new folder, for example under Program Files, is not affected. //! A supplied location is checked even with opaque installer arguments (WinGet `--override` and //! `--custom`), but a location selected only inside those arguments is not. //! - The broker never uninstalls the Devolutions Agent, which hosts it, whether it is selected by its diff --git a/crates/now-package-broker/src/executor/windows/mod.rs b/crates/now-package-broker/src/executor/windows/mod.rs index d4e5e64065..f08a146e70 100644 --- a/crates/now-package-broker/src/executor/windows/mod.rs +++ b/crates/now-package-broker/src/executor/windows/mod.rs @@ -363,8 +363,9 @@ fn run_plan( .context("failed to query execution token elevation")?; if token_is_elevated && !requires_elevation { bail!( - "the execution token is elevated but the request was evaluated for standard execution; \ - standard execution requires a non-elevated user token" + "the package broker can't run requests for a full administrator token without UAC \ + (User Account Control is disabled, the account is the built-in Administrator, \ + or the broker runs elevated outside service mode)" ); } diff --git a/crates/now-package-broker/src/policy_security.rs b/crates/now-package-broker/src/policy_security.rs index efcf6f0425..c905c975e1 100644 --- a/crates/now-package-broker/src/policy_security.rs +++ b/crates/now-package-broker/src/policy_security.rs @@ -54,9 +54,9 @@ use windows::Win32::Security::{ }; use windows::Win32::Storage::FileSystem::{ DELETE, FILE_APPEND_DATA, FILE_ATTRIBUTE_REPARSE_POINT, FILE_ATTRIBUTE_TAG_INFO, FILE_DELETE_CHILD, - FILE_FLAG_BACKUP_SEMANTICS, FILE_FLAG_OPEN_REPARSE_POINT, FILE_NAME_NORMALIZED, FILE_READ_ATTRIBUTES, - FILE_SHARE_DELETE, FILE_SHARE_READ, FILE_SHARE_WRITE, FILE_TRAVERSE, FILE_WRITE_ATTRIBUTES, FILE_WRITE_DATA, - FILE_WRITE_EA, FileAttributeTagInfo, GETFINALPATHNAMEBYHANDLE_FLAGS, GetFileInformationByHandleEx, + FILE_FLAG_BACKUP_SEMANTICS, FILE_FLAG_OPEN_REPARSE_POINT, FILE_LIST_DIRECTORY, FILE_NAME_NORMALIZED, + FILE_READ_ATTRIBUTES, FILE_SHARE_DELETE, FILE_SHARE_READ, FILE_SHARE_WRITE, FILE_TRAVERSE, FILE_WRITE_ATTRIBUTES, + FILE_WRITE_DATA, FILE_WRITE_EA, FileAttributeTagInfo, GETFINALPATHNAMEBYHANDLE_FLAGS, GetFileInformationByHandleEx, GetFinalPathNameByHandleW, READ_CONTROL, VOLUME_NAME_GUID, VOLUME_NAME_NT, WRITE_DAC, WRITE_OWNER, }; use windows::core::PWSTR; @@ -840,6 +840,19 @@ pub(crate) struct VerifiedInstallLocation { _handles: Vec, } +/// Options that open an install location folder for verification and pin it without delete sharing. +/// +/// Share modes only constrain other opens when the handle has data access, so the folder is also +/// opened for listing. +fn install_location_pin_options() -> OpenOptions { + let mut options = OpenOptions::new(); + options + .access_mode(FILE_LIST_DIRECTORY.0 | FILE_READ_ATTRIBUTES.0 | READ_CONTROL.0) + .share_mode(FILE_SHARE_READ.0 | FILE_SHARE_WRITE.0) + .custom_flags((FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OPEN_REPARSE_POINT).0); + options +} + /// Verify that an elevated operation may install into `location`. /// /// The nearest existing folder, which is `location` itself when it exists, is opened through @@ -849,17 +862,16 @@ pub(crate) struct VerifiedInstallLocation { /// and TrustedInstaller write it. /// Its ancestors must not be reparse points or be renamable by other principals. /// The returned guard must be kept alive until the installation completes. +/// +/// The guard holds the folder without delete sharing: a renamed verified folder could be replaced +/// by a junction while the guard is held. +/// Installers can still create, modify and delete entries inside it, but installers that delete or +/// rename an existing install root (such as rename-and-swap upgrades) fail. pub(crate) fn verify_elevated_install_location( opener: &dyn PathOpener, location: &Path, ) -> anyhow::Result { - // Delete sharing is not granted: a renamed verified folder could be replaced by a junction - // while the guard is held, so the installer cannot rename or delete it during the operation. - let mut options = OpenOptions::new(); - options - .access_mode(FILE_READ_ATTRIBUTES.0 | READ_CONTROL.0) - .share_mode(FILE_SHARE_READ.0 | FILE_SHARE_WRITE.0) - .custom_flags((FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OPEN_REPARSE_POINT).0); + let options = install_location_pin_options(); let mut folder = location; let handle = loop { @@ -2016,6 +2028,30 @@ mod tests { .expect("a new folder under Program Files must be accepted"); } + #[test] + fn pinned_install_location_accepts_installer_writes_but_not_root_rename() { + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join("App"); + std::fs::create_dir(&root).unwrap(); + std::fs::write(root.join("old.dll"), b"v1").unwrap(); + + let pin = install_location_pin_options().open(&root).unwrap(); + + std::fs::write(root.join("app.exe"), b"v2").unwrap(); + std::fs::write(root.join("old.dll"), b"v2").unwrap(); + std::fs::create_dir(root.join("plugins")).unwrap(); + std::fs::write(root.join("plugins").join("plugin.dll"), b"v2").unwrap(); + std::fs::rename(root.join("old.dll"), root.join("old.dll.bak")).unwrap(); + std::fs::remove_file(root.join("old.dll.bak")).unwrap(); + assert!( + std::fs::rename(&root, temp.path().join("App.old")).is_err(), + "the pinned install root must not be renamed" + ); + + drop(pin); + std::fs::rename(&root, temp.path().join("App.old")).expect("rename succeeds once the pin is released"); + } + fn canonical_dir(path: &Path) -> PathBuf { let canonical = path.canonicalize().unwrap(); dos_path_from_wide(&canonical.as_os_str().encode_wide().collect::>()) diff --git a/crates/now-package-broker/src/server/mod.rs b/crates/now-package-broker/src/server/mod.rs index f2c303d9b9..cd1406e3de 100644 --- a/crates/now-package-broker/src/server/mod.rs +++ b/crates/now-package-broker/src/server/mod.rs @@ -961,10 +961,6 @@ impl PackageRequestClientOwner for PackageRequest { } } -/// Normalize equivalent request spellings before policy evaluation and command building. -/// -/// An empty or whitespace-only custom install location means "not set", so every component -/// (policy evaluation, the command builders and operation tracking) sees `None`. /// MSI product code of the installed Devolutions Agent, for requests that remove a package. /// /// Fails closed: a request that removes a package is rejected when the product code cannot be read. @@ -991,6 +987,11 @@ fn installed_agent_product_code(request: &PackageRequest) -> Result PackageRequest { if request .options From ba02bae39c3020888fabbbd24c264b1000bf010c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Tue, 6 Oct 2026 16:44:01 +0900 Subject: [PATCH 06/11] fix(agent): reload the environment after broker pre-commands Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../src/executor/windows/mod.rs | 89 ++++++++++++++----- 1 file changed, 69 insertions(+), 20 deletions(-) diff --git a/crates/now-package-broker/src/executor/windows/mod.rs b/crates/now-package-broker/src/executor/windows/mod.rs index f08a146e70..586f214af9 100644 --- a/crates/now-package-broker/src/executor/windows/mod.rs +++ b/crates/now-package-broker/src/executor/windows/mod.rs @@ -353,27 +353,10 @@ fn run_plan( bail!("elevated Bun package operations are not supported by the broker"); } - // SECURITY: The request was evaluated for the elevation `requires_elevation` reports, but the - // token actually running the plan is what matters (e.g. a broker launched from an elevated - // shell, or a full session token when UAC is disabled), so query the token itself. - // Running a standard plan elevated would apply standard-execution policy rules, and skip the - // elevated safeguards, while the administrator token runs without a UAC prompt. let token_is_elevated = token .is_elevated() .context("failed to query execution token elevation")?; - if token_is_elevated && !requires_elevation { - bail!( - "the package broker can't run requests for a full administrator token without UAC \ - (User Account Control is disabled, the account is the built-in Administrator, \ - or the broker runs elevated outside service mode)" - ); - } - - // SECURITY: Pre/post commands are raw strings whose content is not governed by - // the policy, so they must never run elevated. - if token_is_elevated && (ctx.pre_command.is_some() || ctx.post_command.is_some()) { - bail!("pre/post operation commands are only allowed for non-elevated execution"); - } + check_execution_token_elevation(ctx, token_is_elevated)?; // Defense in depth: the server rejects these names before policy evaluation. if let Some(process_name) = ctx @@ -447,8 +430,12 @@ fn run_plan( } } - // 3. Main package-manager command. - let command = prepare_main_command_in(&ctx.command, None, Some(&user_env), requires_elevation)?; + // 3. Main package-manager command. The environment is reloaded because the pre-operation + // command may have updated persisted variables such as `PATH`. + let main_vars = win_api_wrappers::utils::environment_block(Some(token), false) + .context("failed to load user environment block")?; + let main_env = UserEnv::for_user(&main_vars, token)?; + let command = prepare_main_command_in(&ctx.command, None, Some(&main_env), requires_elevation)?; let output = create_process( token, command.args(), @@ -491,6 +478,30 @@ fn run_plan( Ok(output) } +/// Verify that the token running the plan has the elevation the request was evaluated for. +/// +/// The token actually running the plan is what matters (e.g. a broker launched from an elevated +/// shell, or a full session token when UAC is disabled), so the caller queries the token itself. +fn check_execution_token_elevation(ctx: &ExecutionContext, token_is_elevated: bool) -> anyhow::Result<()> { + // SECURITY: Running a standard plan elevated would apply standard-execution policy rules, and + // skip the elevated safeguards, while the administrator token runs without a UAC prompt. + if token_is_elevated && !ctx.requires_elevation() { + bail!( + "the package broker can't run requests for a full administrator token without UAC \ + (User Account Control is disabled, the account is the built-in Administrator, \ + or the broker runs elevated outside service mode)" + ); + } + + // SECURITY: Pre/post commands are raw strings whose content is not governed by + // the policy, so they must never run elevated. + if token_is_elevated && (ctx.pre_command.is_some() || ctx.post_command.is_some()) { + bail!("pre/post operation commands are only allowed for non-elevated execution"); + } + + Ok(()) +} + fn prepare_main_command_in( command: &[String], temp_dir: Option<&Path>, @@ -2374,6 +2385,44 @@ mod tests { ); } + #[test] + fn elevated_tokens_are_rejected_for_standard_plans() { + let mut ctx = ExecutionContext { + kill_processes: vec!["notepad.exe".to_owned()], + pre_command: None, + command: vec!["winget.exe".to_owned(), "install".to_owned()], + post_command: None, + effective_user: "DOMAIN\\user".to_owned(), + user_sid: Sid::from_well_known(WinWorldSid, None).expect("well-known Everyone SID"), + session_id: 1, + elevation: Elevation::Standard, + scope: None, + custom_install_location: None, + capture_output: false, + cancel_token: tokio_util::sync::CancellationToken::new(), + event_sink: None, + }; + + let error = super::check_execution_token_elevation(&ctx, true).expect_err("standard plan with elevated token"); + assert!( + error.to_string().contains("full administrator token without UAC"), + "{error:#}" + ); + super::check_execution_token_elevation(&ctx, false).expect("standard plan with standard token"); + + // An elevated user-scope request is a standard plan. + ctx.elevation = Elevation::Elevated; + ctx.scope = Some(Scope::User); + assert!(super::check_execution_token_elevation(&ctx, true).is_err()); + + ctx.scope = None; + super::check_execution_token_elevation(&ctx, true).expect("elevated plan with elevated token"); + + ctx.pre_command = Some("echo before".to_owned()); + let error = super::check_execution_token_elevation(&ctx, true).expect_err("elevated pre-command"); + assert!(error.to_string().contains("non-elevated"), "{error:#}"); + } + #[test] fn session_zero_is_never_used_for_execution() { let sid = Sid::from_well_known(WinWorldSid, None).expect("well-known Everyone SID"); From 38de2485eeaa4814acb728bbdac0b72c3b1ef41a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Thu, 15 Oct 2026 22:38:22 +0900 Subject: [PATCH 07/11] fix(agent): require an install root parent without untrusted create rights Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../now-package-broker/src/evaluator/mod.rs | 2 + .../now-package-broker/src/policy_security.rs | 76 +++++++++++++++++-- 2 files changed, 70 insertions(+), 8 deletions(-) diff --git a/crates/now-package-broker/src/evaluator/mod.rs b/crates/now-package-broker/src/evaluator/mod.rs index ab22c32caf..7b0dc978fc 100644 --- a/crates/now-package-broker/src/evaluator/mod.rs +++ b/crates/now-package-broker/src/evaluator/mod.rs @@ -58,6 +58,8 @@ //! open without delete sharing until the operation completes. //! Installers that delete or rename their own existing install root, such as rename-and-swap //! upgrades, fail there; installing into a new folder, for example under Program Files, is not affected. +//! An existing install root must also be in a folder where non-administrators cannot create entries, +//! so it stays protected if a canceled operation leaves installer processes running. //! A supplied location is checked even with opaque installer arguments (WinGet `--override` and //! `--custom`), but a location selected only inside those arguments is not. //! - The broker never uninstalls the Devolutions Agent, which hosts it, whether it is selected by its diff --git a/crates/now-package-broker/src/policy_security.rs b/crates/now-package-broker/src/policy_security.rs index c905c975e1..21beb8757b 100644 --- a/crates/now-package-broker/src/policy_security.rs +++ b/crates/now-package-broker/src/policy_security.rs @@ -861,12 +861,16 @@ fn install_location_pin_options() -> OpenOptions { /// DACL, including inheritable ACEs, must not let principals other than SYSTEM, Administrators /// and TrustedInstaller write it. /// Its ancestors must not be reparse points or be renamable by other principals. -/// The returned guard must be kept alive until the installation completes. +/// When `location` already exists, its parent must also not let other principals create entries: +/// an elevated installer may rename or delete its install root, and an untrusted principal could +/// then put a junction in its place. +/// This keeps the location safe without relying on the returned guard, which a canceled or timed-out +/// operation can release while installer processes it started are still running. /// -/// The guard holds the folder without delete sharing: a renamed verified folder could be replaced -/// by a junction while the guard is held. -/// Installers can still create, modify and delete entries inside it, but installers that delete or -/// rename an existing install root (such as rename-and-swap upgrades) fail. +/// The returned guard must be kept alive until the installation completes. +/// It holds the folder without delete sharing, so installers can still create, modify and delete +/// entries inside it, but installers that delete or rename an existing install root (such as +/// rename-and-swap upgrades) fail. pub(crate) fn verify_elevated_install_location( opener: &dyn PathOpener, location: &Path, @@ -874,10 +878,12 @@ pub(crate) fn verify_elevated_install_location( let options = install_location_pin_options(); let mut folder = location; + let mut location_exists = true; let handle = loop { match opener.open(&options, folder) { Ok(handle) => break handle, Err(error) if error.kind() == std::io::ErrorKind::NotFound => { + location_exists = false; folder = folder.parent().with_context(|| { format!( "custom install location '{}' is not on an existing drive", @@ -923,13 +929,27 @@ pub(crate) fn verify_elevated_install_location( ) })?; - let mut handles = - retain_ancestor_directories(folder.parent(), DIRECTORY_TAMPER_MASK, &subject).with_context(|| { + // A folder created by the installer below the verified folder is protected by the verified + // folder's own DACL, so only an existing install root needs a parent without create rights. + let parent_mask = if location_exists { + PARENT_DIRECTORY_TAMPER_MASK + } else { + DIRECTORY_TAMPER_MASK + }; + let mut handles = retain_ancestor_directories(folder.parent(), parent_mask, &subject).with_context(|| { + if location_exists { + format!( + "custom install location '{}' is in a folder where non-administrators can create or rename \ + entries; choose a location under Program Files or a folder restricted to administrators", + location.display() + ) + } else { format!( "custom install location '{}' is in a folder that non-administrators can redirect", location.display() ) - })?; + } + })?; handles.push(handle); Ok(VerifiedInstallLocation { _handles: handles }) @@ -2052,6 +2072,46 @@ mod tests { std::fs::rename(&root, temp.path().join("App.old")).expect("rename succeeds once the pin is released"); } + #[test] + fn existing_install_root_requires_a_parent_without_untrusted_create_rights() { + let temp = tempfile::tempdir().unwrap(); + let parent = canonical_dir(temp.path()).join("Parent"); + let root = parent.join("App"); + std::fs::create_dir_all(&root).unwrap(); + + let admins = Sid::from_well_known(WinBuiltinAdministratorsSid, None).unwrap(); + let system = Sid::from_well_known(WinLocalSystemSid, None).unwrap(); + let users = Sid::from_well_known(windows::Win32::Security::WinBuiltinUsersSid, None).unwrap(); + let admin_only = [grant(GENERIC_ALL.0, system), grant(GENERIC_ALL.0, admins.clone())]; + + // Setting the owner to Administrators requires an elevated token; skip otherwise. + // The DACL rules themselves are covered by the SDDL-based tests above. + if set_security(&root, Some(&admins), &admin_only).is_err() { + return; + } + // Users may create subfolders in the parent, as on a drive root. + let mut parent_entries = admin_only.to_vec(); + parent_entries.push(grant(FILE_APPEND_DATA.0 | FILE_TRAVERSE.0, users)); + set_security(&parent, Some(&admins), &parent_entries).unwrap(); + + // An existing install root could be renamed by the installer and replaced through the parent. + let error = verify_elevated_install_location(&ServiceOpener, &root).unwrap_err(); + assert!( + format!("{error:#}").contains("non-administrators can create or rename entries"), + "unexpected error: {error:#}" + ); + + // A new folder below the verified root does not depend on the parent's create rights, so the + // parent passes; the user-writable temporary directory above it is rejected instead. + let error = verify_elevated_install_location(&ServiceOpener, &root.join("New")).unwrap_err(); + let message = format!("{error:#}"); + assert!(message.contains("can redirect"), "unexpected error: {message}"); + assert!( + !message.contains(&format!("'{}'", parent.display())), + "unexpected error: {message}" + ); + } + fn canonical_dir(path: &Path) -> PathBuf { let canonical = path.canonicalize().unwrap(); dos_path_from_wide(&canonical.as_os_str().encode_wide().collect::>()) From 636022d349daf8ff2450c78d00774ce9ec37fdc1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Thu, 15 Oct 2026 22:48:17 +0900 Subject: [PATCH 08/11] fix(agent): check inheritable grants on an existing install root parent Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../now-package-broker/src/evaluator/mod.rs | 5 +- .../now-package-broker/src/policy_security.rs | 145 ++++++++++-------- 2 files changed, 88 insertions(+), 62 deletions(-) diff --git a/crates/now-package-broker/src/evaluator/mod.rs b/crates/now-package-broker/src/evaluator/mod.rs index 7b0dc978fc..ba43cc2cd2 100644 --- a/crates/now-package-broker/src/evaluator/mod.rs +++ b/crates/now-package-broker/src/evaluator/mod.rs @@ -58,8 +58,9 @@ //! open without delete sharing until the operation completes. //! Installers that delete or rename their own existing install root, such as rename-and-swap //! upgrades, fail there; installing into a new folder, for example under Program Files, is not affected. -//! An existing install root must also be in a folder where non-administrators cannot create entries, -//! so it stays protected if a canceled operation leaves installer processes running. +//! An existing install root must also be in a folder that passes the same check, so non-administrators +//! can neither create entries there nor inherit write access to a recreated root; the root then stays +//! protected if a canceled operation leaves installer processes running. //! A supplied location is checked even with opaque installer arguments (WinGet `--override` and //! `--custom`), but a location selected only inside those arguments is not. //! - The broker never uninstalls the Devolutions Agent, which hosts it, whether it is selected by its diff --git a/crates/now-package-broker/src/policy_security.rs b/crates/now-package-broker/src/policy_security.rs index 21beb8757b..6f07dc37b1 100644 --- a/crates/now-package-broker/src/policy_security.rs +++ b/crates/now-package-broker/src/policy_security.rs @@ -861,9 +861,9 @@ fn install_location_pin_options() -> OpenOptions { /// DACL, including inheritable ACEs, must not let principals other than SYSTEM, Administrators /// and TrustedInstaller write it. /// Its ancestors must not be reparse points or be renamable by other principals. -/// When `location` already exists, its parent must also not let other principals create entries: -/// an elevated installer may rename or delete its install root, and an untrusted principal could -/// then put a junction in its place. +/// When `location` already exists, its parent must also pass the same owner and DACL check, so an +/// elevated installer that renames, or deletes and recreates, its install root cannot let an +/// untrusted principal put a junction in its place or inherit write access to the new root. /// This keeps the location safe without relying on the returned guard, which a canceled or timed-out /// operation can release while installer processes it started are still running. /// @@ -930,26 +930,30 @@ pub(crate) fn verify_elevated_install_location( })?; // A folder created by the installer below the verified folder is protected by the verified - // folder's own DACL, so only an existing install root needs a parent without create rights. - let parent_mask = if location_exists { - PARENT_DIRECTORY_TAMPER_MASK + // folder's own DACL. An existing install root may be renamed, or deleted and recreated, by the + // installer, so its parent gets the same check: no untrusted create rights, and no write access + // passed on through inheritable ACEs. + let (parent_writers, parent_mask) = if location_exists { + (TrustedWriters::InstallLocation, INSTALL_LOCATION_TAMPER_MASK) } else { - DIRECTORY_TAMPER_MASK + (TrustedWriters::AdminOrTrustedInstaller, DIRECTORY_TAMPER_MASK) }; - let mut handles = retain_ancestor_directories(folder.parent(), parent_mask, &subject).with_context(|| { - if location_exists { - format!( - "custom install location '{}' is in a folder where non-administrators can create or rename \ - entries; choose a location under Program Files or a folder restricted to administrators", - location.display() - ) - } else { - format!( - "custom install location '{}' is in a folder that non-administrators can redirect", - location.display() - ) - } - })?; + let mut handles = retain_ancestor_directories(folder.parent(), parent_writers, parent_mask, &subject) + .with_context(|| { + if location_exists { + format!( + "custom install location '{}' is in a folder where non-administrators can create entries or \ + inherit write access; choose a location under Program Files or a folder restricted to \ + administrators", + location.display() + ) + } else { + format!( + "custom install location '{}' is in a folder that non-administrators can redirect", + location.display() + ) + } + })?; handles.push(handle); Ok(VerifiedInstallLocation { _handles: handles }) @@ -1219,16 +1223,27 @@ fn parse_app_exec_alias(buffer: &[u8]) -> Option { /// Each ancestor must not itself be a reparse point and must resolve to its own path. /// The returned handles pin the verified chain and must be kept alive by the caller. fn retain_executable_ancestor_directories(path: &Path, subject: &str) -> anyhow::Result> { - retain_ancestor_directories(path.parent(), PARENT_DIRECTORY_TAMPER_MASK, subject) + retain_ancestor_directories( + path.parent(), + TrustedWriters::AdminOrTrustedInstaller, + PARENT_DIRECTORY_TAMPER_MASK, + subject, + ) } /// Verify and pin `first` and each of its ancestors, checking `first` against `first_mask` /// and higher ancestors against [`DIRECTORY_TAMPER_MASK`]. /// /// See [`retain_executable_ancestor_directories`] for the rationale. -fn retain_ancestor_directories(first: Option<&Path>, first_mask: u32, subject: &str) -> anyhow::Result> { +fn retain_ancestor_directories( + first: Option<&Path>, + first_writers: TrustedWriters, + first_mask: u32, + subject: &str, +) -> anyhow::Result> { let mut handles = Vec::new(); let mut current = first; + let mut trusted_writers = first_writers; let mut tamper_mask = first_mask; while let Some(dir) = current { @@ -1252,14 +1267,10 @@ fn retain_ancestor_directories(first: Option<&Path>, first_mask: u32, subject: & ); } - verify_handle_security( - &handle, - &dir_subject, - TrustedWriters::AdminOrTrustedInstaller, - tamper_mask, - )?; + verify_handle_security(&handle, &dir_subject, trusted_writers, tamper_mask)?; handles.push(handle); + trusted_writers = TrustedWriters::AdminOrTrustedInstaller; tamper_mask = DIRECTORY_TAMPER_MASK; current = dir.parent(); } @@ -2073,43 +2084,57 @@ mod tests { } #[test] - fn existing_install_root_requires_a_parent_without_untrusted_create_rights() { - let temp = tempfile::tempdir().unwrap(); - let parent = canonical_dir(temp.path()).join("Parent"); - let root = parent.join("App"); - std::fs::create_dir_all(&root).unwrap(); - + fn existing_install_root_requires_a_parent_that_passes_the_install_location_check() { let admins = Sid::from_well_known(WinBuiltinAdministratorsSid, None).unwrap(); let system = Sid::from_well_known(WinLocalSystemSid, None).unwrap(); let users = Sid::from_well_known(windows::Win32::Security::WinBuiltinUsersSid, None).unwrap(); let admin_only = [grant(GENERIC_ALL.0, system), grant(GENERIC_ALL.0, admins.clone())]; - // Setting the owner to Administrators requires an elevated token; skip otherwise. - // The DACL rules themselves are covered by the SDDL-based tests above. - if set_security(&root, Some(&admins), &admin_only).is_err() { - return; + let parent_grants = [ + // Users may create subfolders in the parent, as on a drive root. + grant(FILE_APPEND_DATA.0 | FILE_TRAVERSE.0, users.clone()), + // Users get full control of what is created in the parent, such as a recreated root. + ExplicitAccess { + access_permissions: GENERIC_ALL.0, + access_mode: GRANT_ACCESS, + inheritance: windows::Win32::Security::SUB_CONTAINERS_AND_OBJECTS_INHERIT + | windows::Win32::Security::INHERIT_ONLY, + trustee: Trustee::Sid(users), + }, + ]; + + for parent_grant in parent_grants { + let temp = tempfile::tempdir().unwrap(); + let parent = canonical_dir(temp.path()).join("Parent"); + let root = parent.join("App"); + std::fs::create_dir_all(&root).unwrap(); + + // Setting the owner to Administrators requires an elevated token; skip otherwise. + // The DACL rules themselves are covered by the SDDL-based tests above. + if set_security(&root, Some(&admins), &admin_only).is_err() { + return; + } + let mut parent_entries = admin_only.to_vec(); + parent_entries.push(parent_grant); + set_security(&parent, Some(&admins), &parent_entries).unwrap(); + let parent_subject = format!("ancestor directory '{}' DACL grants write access", parent.display()); + + // The installer could rename, or delete and recreate, an existing install root. + let error = verify_elevated_install_location(&ServiceOpener, &root).unwrap_err(); + let message = format!("{error:#}"); + assert!( + message.contains("non-administrators can create entries or inherit write access"), + "unexpected error: {message}" + ); + assert!(message.contains(&parent_subject), "unexpected error: {message}"); + + // A new folder below the verified root depends on the root's DACL instead, so the parent + // passes; the user-writable temporary directory above it is rejected. + let error = verify_elevated_install_location(&ServiceOpener, &root.join("New")).unwrap_err(); + let message = format!("{error:#}"); + assert!(message.contains("can redirect"), "unexpected error: {message}"); + assert!(!message.contains(&parent_subject), "unexpected error: {message}"); } - // Users may create subfolders in the parent, as on a drive root. - let mut parent_entries = admin_only.to_vec(); - parent_entries.push(grant(FILE_APPEND_DATA.0 | FILE_TRAVERSE.0, users)); - set_security(&parent, Some(&admins), &parent_entries).unwrap(); - - // An existing install root could be renamed by the installer and replaced through the parent. - let error = verify_elevated_install_location(&ServiceOpener, &root).unwrap_err(); - assert!( - format!("{error:#}").contains("non-administrators can create or rename entries"), - "unexpected error: {error:#}" - ); - - // A new folder below the verified root does not depend on the parent's create rights, so the - // parent passes; the user-writable temporary directory above it is rejected instead. - let error = verify_elevated_install_location(&ServiceOpener, &root.join("New")).unwrap_err(); - let message = format!("{error:#}"); - assert!(message.contains("can redirect"), "unexpected error: {message}"); - assert!( - !message.contains(&format!("'{}'", parent.display())), - "unexpected error: {message}" - ); } fn canonical_dir(path: &Path) -> PathBuf { From 125c56cf7de329ace5c4e38311cd00fdd8fb67ec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Sat, 31 Oct 2026 00:26:55 +0900 Subject: [PATCH 09/11] fix(agent): lower elevation for implicit user package scope --- .../src/command_builder/bun.rs | 11 ++++-- .../src/command_builder/cargo.rs | 11 ++++-- .../src/command_builder/npm.rs | 11 ++++-- .../src/command_builder/pip.rs | 11 ++++-- .../src/command_builder/scoop.rs | 11 ++++-- .../src/command_builder/vcpkg.rs | 11 ++++-- .../src/evaluator/matching.rs | 2 +- .../now-package-broker/src/evaluator/mod.rs | 11 ++---- .../now-package-broker/src/evaluator/tests.rs | 38 +++++++++++++++++++ 9 files changed, 84 insertions(+), 33 deletions(-) diff --git a/crates/now-package-broker/src/command_builder/bun.rs b/crates/now-package-broker/src/command_builder/bun.rs index 9355ef62cd..6de0286711 100644 --- a/crates/now-package-broker/src/command_builder/bun.rs +++ b/crates/now-package-broker/src/command_builder/bun.rs @@ -262,14 +262,17 @@ mod tests { } #[test] - fn bun_rejects_elevated_execution() { + fn implicit_user_scope_lowers_requested_elevation() { + // oracle: regression Devolutions/devolutions-gateway#2036 discussion_r4379344015. let mut request = make_request(); request.client.requested_elevation = Elevation::Elevated; request.options.scope = None; - let error = build_bun_command(&request).expect_err("elevation should fail"); - - assert!(error.to_string().contains("elevated Bun")); + assert_eq!( + crate::evaluator::effective_execution_elevation(&request), + Elevation::Standard + ); + build_bun_command(&request).expect("implicit user scope runs with the standard token"); } #[test] diff --git a/crates/now-package-broker/src/command_builder/cargo.rs b/crates/now-package-broker/src/command_builder/cargo.rs index 5d08e53217..0ca0a2d11f 100644 --- a/crates/now-package-broker/src/command_builder/cargo.rs +++ b/crates/now-package-broker/src/command_builder/cargo.rs @@ -244,14 +244,17 @@ mod tests { } #[test] - fn elevated_cargo_operations_are_rejected() { + fn implicit_user_scope_lowers_requested_elevation() { + // oracle: regression Devolutions/devolutions-gateway#2036 discussion_r4379344015. let mut request = make_request(); request.client.requested_elevation = Elevation::Elevated; request.options.scope = None; - let error = build_cargo_command(&request).expect_err("elevated cargo should fail"); - - assert!(error.to_string().contains("elevated")); + assert_eq!( + crate::evaluator::effective_execution_elevation(&request), + Elevation::Standard + ); + build_cargo_command(&request).expect("implicit user scope runs with the standard token"); } #[test] diff --git a/crates/now-package-broker/src/command_builder/npm.rs b/crates/now-package-broker/src/command_builder/npm.rs index a8224a5a7f..b7b8447141 100644 --- a/crates/now-package-broker/src/command_builder/npm.rs +++ b/crates/now-package-broker/src/command_builder/npm.rs @@ -320,14 +320,17 @@ mod tests { } #[test] - fn elevated_requests_are_rejected() { + fn implicit_user_scope_lowers_requested_elevation() { + // oracle: regression Devolutions/devolutions-gateway#2036 discussion_r4379344015. let mut request = make_request(); request.client.requested_elevation = Elevation::Elevated; request.options.scope = None; - let error = build_npm_command(&request).expect_err("elevated npm should fail"); - - assert!(error.to_string().contains("elevated")); + assert_eq!( + crate::evaluator::effective_execution_elevation(&request), + Elevation::Standard + ); + build_npm_command(&request).expect("implicit user scope runs with the standard token"); } #[test] diff --git a/crates/now-package-broker/src/command_builder/pip.rs b/crates/now-package-broker/src/command_builder/pip.rs index 2edd09eab8..0ba59ef528 100644 --- a/crates/now-package-broker/src/command_builder/pip.rs +++ b/crates/now-package-broker/src/command_builder/pip.rs @@ -271,14 +271,17 @@ mod tests { } #[test] - fn pip_rejects_elevated_operations() { + fn implicit_user_scope_lowers_requested_elevation() { + // oracle: regression Devolutions/devolutions-gateway#2036 discussion_r4379344015. let mut request = make_request(); request.client.requested_elevation = Elevation::Elevated; request.options.scope = None; - let error = build_pip_command(&request).expect_err("elevated pip should fail"); - - assert!(error.to_string().contains("elevated")); + assert_eq!( + crate::evaluator::effective_execution_elevation(&request), + Elevation::Standard + ); + build_pip_command(&request).expect("implicit user scope runs with the standard token"); } #[test] diff --git a/crates/now-package-broker/src/command_builder/scoop.rs b/crates/now-package-broker/src/command_builder/scoop.rs index ee0dfcbce4..b9b3bac766 100644 --- a/crates/now-package-broker/src/command_builder/scoop.rs +++ b/crates/now-package-broker/src/command_builder/scoop.rs @@ -349,14 +349,17 @@ mod tests { } #[test] - fn elevated_execution_is_rejected() { + fn implicit_user_scope_lowers_requested_elevation() { + // oracle: regression Devolutions/devolutions-gateway#2036 discussion_r4379344015. let mut request = make_request(); request.client.requested_elevation = Elevation::Elevated; request.options.scope = None; - let error = build_scoop_command(&request).expect_err("elevated Scoop should fail"); - - assert!(error.to_string().contains("elevated execution")); + assert_eq!( + crate::evaluator::effective_execution_elevation(&request), + Elevation::Standard + ); + build_scoop_command(&request).expect("implicit user scope runs with the standard token"); } #[test] diff --git a/crates/now-package-broker/src/command_builder/vcpkg.rs b/crates/now-package-broker/src/command_builder/vcpkg.rs index 8d3b79b727..77af41ead0 100644 --- a/crates/now-package-broker/src/command_builder/vcpkg.rs +++ b/crates/now-package-broker/src/command_builder/vcpkg.rs @@ -266,14 +266,17 @@ mod tests { } #[test] - fn elevated_requests_are_rejected() { + fn implicit_user_scope_lowers_requested_elevation() { + // oracle: regression Devolutions/devolutions-gateway#2036 discussion_r4379344015. let mut request = make_request(); request.client.requested_elevation = Elevation::Elevated; request.options.scope = None; - let error = build_vcpkg_command(&request).expect_err("elevated request should fail"); - - assert!(error.to_string().contains("elevated")); + assert_eq!( + crate::evaluator::effective_execution_elevation(&request), + Elevation::Standard + ); + build_vcpkg_command(&request).expect("implicit user scope runs with the standard token"); } #[test] diff --git a/crates/now-package-broker/src/evaluator/matching.rs b/crates/now-package-broker/src/evaluator/matching.rs index 9dea23591f..ef7e7e466d 100644 --- a/crates/now-package-broker/src/evaluator/matching.rs +++ b/crates/now-package-broker/src/evaluator/matching.rs @@ -136,7 +136,7 @@ fn managers_match(manager: now_policy_api::ManagerName, allowed: &BTreeSet Option { +pub(super) fn effective_scope(request: &PackageRequest) -> Option { use now_policy_api::{ManagerName as M, Operation as O, Scope as S}; if matches!(request.manager, M::PowerShell | M::PowerShell7 | M::Pip) && request.operation == O::Uninstall { diff --git a/crates/now-package-broker/src/evaluator/mod.rs b/crates/now-package-broker/src/evaluator/mod.rs index ba43cc2cd2..93b8eb956d 100644 --- a/crates/now-package-broker/src/evaluator/mod.rs +++ b/crates/now-package-broker/src/evaluator/mod.rs @@ -43,7 +43,7 @@ //! Each Elevated Allow rule removes the UAC prompt for the operations it matches, for any code //! running in that administrator's session, so the broker restricts what an elevated operation //! can select regardless of the policy: -//! - An explicit user scope, typed or passed through custom parameters, always runs with the standard +//! - A user scope, explicit or defaulted by the manager, runs with the standard //! token (see `effective_execution_elevation`); policy matching and execution use the same elevation. //! The executor refuses to run a standard plan with an elevated token: a full administrator token //! without UAC (UAC disabled, or the built-in Administrator account) cannot use the broker. @@ -329,16 +329,11 @@ pub(crate) fn is_powershell_manager(manager: now_policy_api::ManagerName) -> boo /// Policy matching, request validation, command building and the executor all use this value, /// so a request is always evaluated with the elevation it runs with. pub(crate) fn effective_execution_elevation(request: &PackageRequest) -> Elevation { - let custom_scope = || { - let custom = custom_options::custom_options(request.manager, &request.options.custom_parameters); - custom_options::resolve(None, &custom.scopes).flatten() - }; - // A user scope passed only through custom parameters lowers the elevation like a typed one; - // a machine scope there does not raise it. + // An implicit user scope lowers elevation like a typed one; an implicit machine scope never raises it. let scope = request .options .scope - .or_else(|| custom_scope().filter(|scope| *scope == Scope::User)); + .or_else(|| matching::effective_scope(request).filter(|scope| *scope == Scope::User)); effective_elevation(request.client.requested_elevation, scope) } diff --git a/crates/now-package-broker/src/evaluator/tests.rs b/crates/now-package-broker/src/evaluator/tests.rs index c0c6d34997..6a56a30a18 100644 --- a/crates/now-package-broker/src/evaluator/tests.rs +++ b/crates/now-package-broker/src/evaluator/tests.rs @@ -732,6 +732,44 @@ fn effective_elevation_lowers_explicit_user_scope_and_raises_machine_scope() { assert_eq!(super::effective_elevation(E::Standard, None), E::Standard); } +#[test] +fn implicit_user_scope_matches_standard_elevation_policy() { + // oracle: regression Devolutions/devolutions-gateway#2036 discussion_r4379344015. + let policy = make_policy( + Decision::Deny, + vec![rule( + "allow-standard-user", + 10, + Decision::Allow, + PolicyMatch { + scopes: BTreeSet::from([now_policy::Scope::User]), + execution_elevation: BTreeSet::from([now_policy::Elevation::Standard]), + ..Default::default() + }, + )], + ); + + for (manager, source, operation) in [ + (api::ManagerName::Dotnet, "nuget.org", api::Operation::Install), + (api::ManagerName::Dotnet, "nuget.org", api::Operation::Update), + (api::ManagerName::Dotnet, "nuget.org", api::Operation::Uninstall), + (api::ManagerName::PowerShell, "PSGallery", api::Operation::Install), + (api::ManagerName::PowerShell, "PSGallery", api::Operation::Update), + (api::ManagerName::PowerShell7, "PSGallery", api::Operation::Install), + (api::ManagerName::PowerShell7, "PSGallery", api::Operation::Update), + ] { + let mut request = make_request(operation, "Contoso.Tool"); + request.manager = manager; + request.source.name = source.to_owned(); + + assert_eq!( + evaluate(&policy, &request).rule_id, + "allow-standard-user", + "{manager:?} {operation:?}" + ); + } +} + #[test] fn unacceptable_kill_process_names_are_denied_before_rules() { let policy = make_policy(Decision::Allow, Vec::new()); From 448cf41b91c996478267b3ce4cc0b5db7a53043a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Sat, 31 Oct 2026 02:58:59 +0900 Subject: [PATCH 10/11] test(agent): justify broker test expectations Tie regression oracles to reported defects and record measured faults for tests without an independent source. Verify rejection of receipts from the previous validator version without fixing the current version in tests. --- .../now-package-broker/src/evaluator/builtin_rules.rs | 3 +++ crates/now-package-broker/src/evaluator/matching.rs | 1 + crates/now-package-broker/src/evaluator/tests.rs | 5 +++++ crates/now-package-broker/src/executor/windows/mod.rs | 8 ++++++++ crates/now-package-broker/src/policy_security.rs | 8 ++++++++ crates/now-package-broker/src/policy_store/mod.rs | 11 ++++++----- .../now-package-broker/src/policy_store/validation.rs | 1 + crates/now-package-broker/src/server/mod.rs | 5 +++++ 8 files changed, 37 insertions(+), 5 deletions(-) diff --git a/crates/now-package-broker/src/evaluator/builtin_rules.rs b/crates/now-package-broker/src/evaluator/builtin_rules.rs index 88a441d06c..36b4e754c5 100644 --- a/crates/now-package-broker/src/evaluator/builtin_rules.rs +++ b/crates/now-package-broker/src/evaluator/builtin_rules.rs @@ -90,6 +90,7 @@ mod tests { #[test] fn plain_executable_names_are_acceptable_kill_targets() { + // oracle: mutant Rejecting plain executable names in the kill-target validator. for name in ["Code.exe", "notepad.EXE", "My App.exe", "app-1.2.exe"] { assert!(is_acceptable_kill_process_name(name), "{name}"); } @@ -97,6 +98,7 @@ mod tests { #[test] fn wildcard_path_and_malformed_kill_targets_are_rejected() { + // oracle: mutant Allowing wildcard executable names in kill targets. for name in [ "", ".exe", @@ -124,6 +126,7 @@ mod tests { #[test] fn names_without_an_extension_get_exe_appended() { + // oracle: mutant Leaving extensionless kill targets unnormalized. assert_eq!(normalize_kill_process_name("chrome"), "chrome.exe"); assert_eq!(normalize_kill_process_name("Chrome.EXE"), "Chrome.EXE"); assert_eq!(normalize_kill_process_name("chrome.bat"), "chrome.bat"); diff --git a/crates/now-package-broker/src/evaluator/matching.rs b/crates/now-package-broker/src/evaluator/matching.rs index ef7e7e466d..7822b508d3 100644 --- a/crates/now-package-broker/src/evaluator/matching.rs +++ b/crates/now-package-broker/src/evaluator/matching.rs @@ -425,6 +425,7 @@ mod tests { #[test] fn elevated_user_scope_request_uses_standard_execution_privilege() { + // oracle: mutant Matching elevated policy rules for typed user-scope requests. let mut request = request(); request.client.requested_elevation = api::Elevation::Elevated; request.options.scope = Some(api::Scope::User); diff --git a/crates/now-package-broker/src/evaluator/tests.rs b/crates/now-package-broker/src/evaluator/tests.rs index 6a56a30a18..4faf4eb0e4 100644 --- a/crates/now-package-broker/src/evaluator/tests.rs +++ b/crates/now-package-broker/src/evaluator/tests.rs @@ -772,6 +772,7 @@ fn implicit_user_scope_matches_standard_elevation_policy() { #[test] fn unacceptable_kill_process_names_are_denied_before_rules() { + // oracle: mutant Accepting wildcard kill targets before policy evaluation. let policy = make_policy(Decision::Allow, Vec::new()); for name in ["*", "Code*.exe", r"C:\Tools\Code.exe", "Code.bat"] { @@ -792,6 +793,7 @@ fn unacceptable_kill_process_names_are_denied_before_rules() { #[test] fn agent_uninstall_is_denied_before_rules() { + // oracle: mutant Disabling protection for named Agent packages. let policy = make_policy(Decision::Allow, Vec::new()); let request = make_request(api::Operation::Uninstall, "devolutions.agent"); @@ -825,6 +827,7 @@ fn agent_uninstall_is_denied_before_rules() { #[test] fn custom_parameter_user_scope_lowers_execution_elevation() { + // oracle: mutant Ignoring custom user scope or promoting elevation for custom machine scope. let custom = |values: &[&str]| { values .iter() @@ -854,6 +857,7 @@ fn custom_parameter_user_scope_lowers_execution_elevation() { #[test] fn supplied_install_location_is_kept_for_security_checks_when_installer_arguments_are_present() { + // oracle: regression Devolutions/devolutions-gateway#2036 discussion_r4192245600 let mut request = make_request(api::Operation::Install, "Contoso.Tool"); request.options.custom_install_location = Some(r"c:\Users\alice\Downloads\App".to_owned()); request.options.custom_parameters = vec![api::CustomParameterString("--custom=/quiet".to_owned())]; @@ -877,6 +881,7 @@ fn supplied_install_location_is_kept_for_security_checks_when_installer_argument #[test] fn agent_uninstall_by_installed_product_code_is_detected() { + // oracle: regression Devolutions/devolutions-gateway#2036 discussion_r4192380021 let product_code = uuid::uuid!("{0A1B2C3D-4E5F-4A6B-8C7D-9E0F1A2B3C4D}"); let protects = |request: &PackageRequest| super::uninstalls_protected_package(request, Some(product_code)); diff --git a/crates/now-package-broker/src/executor/windows/mod.rs b/crates/now-package-broker/src/executor/windows/mod.rs index 9fd669bc67..0b0b6fe2db 100644 --- a/crates/now-package-broker/src/executor/windows/mod.rs +++ b/crates/now-package-broker/src/executor/windows/mod.rs @@ -2284,6 +2284,7 @@ mod tests { #[test] fn vcpkg_elevated_execution_is_rejected() { + // oracle: mutant Running an elevated vcpkg plan or rejecting its demoted user-scope plan. let mut ctx = ExecutionContext { kill_processes: Vec::new(), pre_command: None, @@ -2321,6 +2322,7 @@ mod tests { #[test] fn kill_command_is_scoped_to_the_requester_session() { + // oracle: regression Devolutions/devolutions-gateway#2036 discussion_r4192110409 let taskkill = trusted_system32_executable("taskkill.exe"); assert_eq!( @@ -2344,6 +2346,7 @@ mod tests { #[test] fn account_display_name_uses_domain_and_name() { + // oracle: mutant Omitting the domain separator from the taskkill account filter. let system = Sid::from_well_known(windows::Win32::Security::WinLocalSystemSid, None).expect("SYSTEM SID"); let name = account_display_name(&system).expect("look up SYSTEM"); assert!(name.ends_with(r"\SYSTEM"), "{name}"); @@ -2351,6 +2354,7 @@ mod tests { #[tokio::test] async fn elevated_pre_post_commands_are_rejected_by_executor() { + // oracle: regression Devolutions/devolutions-gateway#1903 let ctx = ExecutionContext { kill_processes: Vec::new(), pre_command: Some("echo before".to_owned()), @@ -2377,6 +2381,7 @@ mod tests { #[test] fn non_system_mode_rejects_client_user_different_from_broker_user() { + // oracle: regression Devolutions/devolutions-gateway#1892 let ctx = ExecutionContext { kill_processes: Vec::new(), pre_command: None, @@ -2404,6 +2409,7 @@ mod tests { #[test] fn non_system_mode_rejects_client_session_different_from_broker_session() { + // oracle: regression Devolutions/devolutions-gateway#2036 discussion_r4192110409 let token = win_api_wrappers::process::Process::current_process() .token(windows::Win32::Security::TOKEN_QUERY) .expect("open current process token"); @@ -2436,6 +2442,7 @@ mod tests { #[test] fn elevated_tokens_are_rejected_for_standard_plans() { + // oracle: regression Devolutions/devolutions-gateway#2036 discussion_r4192110367 let mut ctx = ExecutionContext { kill_processes: vec!["notepad.exe".to_owned()], pre_command: None, @@ -2474,6 +2481,7 @@ mod tests { #[test] fn session_zero_is_never_used_for_execution() { + // oracle: mutant Using the service process token for a session-zero request. let sid = Sid::from_well_known(WinWorldSid, None).expect("well-known Everyone SID"); let error = super::user_session_token(0, &sid).expect_err("session 0 must be rejected"); assert!(error.to_string().contains("session 0"), "{error:#}"); diff --git a/crates/now-package-broker/src/policy_security.rs b/crates/now-package-broker/src/policy_security.rs index 5da2553ae4..8340afaed4 100644 --- a/crates/now-package-broker/src/policy_security.rs +++ b/crates/now-package-broker/src/policy_security.rs @@ -2028,6 +2028,7 @@ mod tests { #[test] fn inheritable_untrusted_write_ace_is_rejected_for_install_locations_only() { + // oracle: regression Devolutions/devolutions-gateway#2036 discussion_r4283749914 // Users may modify what gets created inside the folder, but not the folder itself. let sd = SddlDescriptor::parse("O:BAD:P(A;OICI;FA;;;SY)(A;OICI;FA;;;BA)(A;OICIIO;0x1301bf;;;BU)"); sd.verify_as_executable() @@ -2038,6 +2039,7 @@ mod tests { #[test] fn untrusted_create_rights_are_rejected_for_install_locations() { + // oracle: regression Devolutions/devolutions-gateway#2036 discussion_r4283609524 // Drive roots let Authenticated Users create subfolders. for ace in [ "(A;;0x4;;;AU)", @@ -2052,12 +2054,14 @@ mod tests { #[test] fn untrusted_owner_is_rejected_for_install_locations() { + // oracle: mutant Accepting an untrusted owner for an install location. let sd = SddlDescriptor::parse("O:BUD:P(A;OICI;FA;;;SY)(A;OICI;FA;;;BA)"); assert!(sd.verify_as_install_location().is_err()); } #[test] fn install_location_with_inheritable_users_write_ace_is_rejected() { + // oracle: regression Devolutions/devolutions-gateway#2036 discussion_r4283749914 let temp = tempfile::tempdir().unwrap(); let folder = temp.path().join("Contoso"); std::fs::create_dir(&folder).unwrap(); @@ -2094,6 +2098,7 @@ mod tests { #[test] fn install_location_under_program_files_is_accepted() { + // oracle: mutant Rejecting an install path with more than one missing ancestor under Program Files. let program_files = std::env::var("ProgramFiles").unwrap_or_else(|_| r"C:\Program Files".to_owned()); let location = Path::new(&program_files).join(format!("now-package-broker-test-{}", uuid::Uuid::new_v4())); @@ -2103,6 +2108,7 @@ mod tests { #[test] fn pinned_install_location_accepts_installer_writes_but_not_root_rename() { + // oracle: regression Devolutions/devolutions-gateway#2036 discussion_r4192245640 let temp = tempfile::tempdir().unwrap(); let root = temp.path().join("App"); std::fs::create_dir(&root).unwrap(); @@ -2127,6 +2133,7 @@ mod tests { #[test] fn existing_install_root_requires_a_parent_that_passes_the_install_location_check() { + // oracle: regression Devolutions/devolutions-gateway#2036 discussion_r4283609524 discussion_r4283749914 let admins = Sid::from_well_known(WinBuiltinAdministratorsSid, None).unwrap(); let system = Sid::from_well_known(WinLocalSystemSid, None).unwrap(); let users = Sid::from_well_known(windows::Win32::Security::WinBuiltinUsersSid, None).unwrap(); @@ -2186,6 +2193,7 @@ mod tests { #[test] fn install_location_through_a_reparse_point_is_rejected() { + // oracle: mutant Accepting a reparse point in an install path. let temp = tempfile::tempdir().unwrap(); let base = canonical_dir(temp.path()); let target = base.join("target"); diff --git a/crates/now-package-broker/src/policy_store/mod.rs b/crates/now-package-broker/src/policy_store/mod.rs index e4b8d77ac8..cfda7360ef 100644 --- a/crates/now-package-broker/src/policy_store/mod.rs +++ b/crates/now-package-broker/src/policy_store/mod.rs @@ -1145,12 +1145,13 @@ mod storage_tests { assert_eq!(error.code, ErrorCode::ValidationFailed); let validation = store.validate_draft(&request.draft); - assert_eq!(validation.validator_version, "now-package-broker-policy-validator/12"); let canonical = validation.canonical_draft.as_ref().expect("compatible draft"); - let old_receipt = - store - .receipt_key - .issue("now-package-broker-policy-validator/8", canonical, &validation.findings); + // oracle: mutant Reusing the previous validator version accepts stale validation receipts. + let old_receipt = store.receipt_key.issue( + "now-package-broker-policy-validator/11", + canonical, + &validation.findings, + ); request.validation_receipt = old_receipt; let error = store .replace_for_tests(request.clone()) diff --git a/crates/now-package-broker/src/policy_store/validation.rs b/crates/now-package-broker/src/policy_store/validation.rs index 1c39e680af..815a1d8942 100644 --- a/crates/now-package-broker/src/policy_store/validation.rs +++ b/crates/now-package-broker/src/policy_store/validation.rs @@ -716,6 +716,7 @@ mod tests { #[test] fn audit_mode_warns_that_elevated_requests_run_without_a_prompt() { + // oracle: mutant Omitting the audit-mode warning about elevated execution without UAC. let uac_warnings = |raw: &serde_json::Value| { validate_draft(raw) .findings diff --git a/crates/now-package-broker/src/server/mod.rs b/crates/now-package-broker/src/server/mod.rs index cd1406e3de..afa08d6d0f 100644 --- a/crates/now-package-broker/src/server/mod.rs +++ b/crates/now-package-broker/src/server/mod.rs @@ -1683,6 +1683,7 @@ mod tests { #[test] fn elevated_user_scope_pre_post_commands_are_accepted() { + // oracle: mutant Rejecting standard user-scope pre/post commands based on requested elevation. // Explicit user scope runs with the standard token even when elevation is requested. let mut request = request(); request.client.requested_elevation = Elevation::Elevated; @@ -1698,6 +1699,7 @@ mod tests { #[test] fn unacceptable_kill_process_names_are_rejected_even_under_permissive_policy() { + // oracle: mutant Allowing wildcard kill targets under a permissive policy. for name in [ "*", "Code*.exe", @@ -1729,6 +1731,7 @@ mod tests { #[tokio::test] async fn kill_process_names_without_an_extension_execute_with_exe_appended() { + // oracle: mutant Failing to normalize extensionless kill targets before execution. let executor = Arc::new(CapturingExecutor::default()); let state = state_with_executor(Arc::clone(&executor) as Arc); let mut request = request(); @@ -1755,6 +1758,7 @@ mod tests { #[test] fn agent_uninstall_is_rejected_even_under_permissive_policy() { + // oracle: mutant Allowing named Agent removal under a permissive policy. let mut request = request(); request.operation = api::Operation::Uninstall; request.package.id = api::PackageIdentifier("Devolutions.Agent".to_owned()); @@ -2014,6 +2018,7 @@ mod tests { #[tokio::test] async fn elevated_user_scope_request_executes_with_the_standard_token() { + // oracle: mutant Keeping elevated execution for an explicitly user-scoped request. let executor = Arc::new(CapturingExecutor::default()); let state = state_with_executor(Arc::clone(&executor) as Arc); let mut request = request(); From 906676380475b61d6339f57a8218a315c423428d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Sat, 31 Oct 2026 02:59:02 +0900 Subject: [PATCH 11/11] test(agent): prune redundant broker checks Remove four overlapping tests whose sampled faults remain detected by the remaining suite. Drop duplicate validator-version assertions and the unsupported unresolved-product-code expectation. --- crates/agent-policy-tester/src/windows.rs | 10 ++-------- .../src/command_builder/npm.rs | 9 --------- .../now-package-broker/src/evaluator/tests.rs | 13 ------------- .../now-package-broker/src/policy_security.rs | 19 ------------------- 4 files changed, 2 insertions(+), 49 deletions(-) diff --git a/crates/agent-policy-tester/src/windows.rs b/crates/agent-policy-tester/src/windows.rs index 487ab5872f..fb419ea40b 100644 --- a/crates/agent-policy-tester/src/windows.rs +++ b/crates/agent-policy-tester/src/windows.rs @@ -722,10 +722,6 @@ async fn validate_policy_by_pipe(pipe_name: &str, draft: &Value) -> anyhow::Resu ); let validation = validation_response.json()?["Validation"].clone(); ensure!(validation["IsValid"] == true, "policy validation failed"); - ensure!( - validation["ValidatorVersion"] == "now-package-broker-policy-validator/12", - "unexpected validator contract" - ); ensure!( validation["CanonicalDraft"]["PolicyFormatVersion"] == draft["PolicyFormatVersion"], "canonical draft changed the format version" @@ -1035,10 +1031,8 @@ async fn assert_invalid_draft(pipe_name: &str, draft: Value, expected_path: &str let invalid_validation = invalid_response.json()?["Validation"].clone(); ensure!(invalid_validation["IsValid"] == false, "invalid draft was accepted"); ensure!( - invalid_validation.get("CanonicalDraft").is_none() - && invalid_validation.get("ValidationReceipt").is_none() - && invalid_validation["ValidatorVersion"] == "now-package-broker-policy-validator/12", - "invalid draft returned a canonical draft, receipt, or wrong validator version" + invalid_validation.get("CanonicalDraft").is_none() && invalid_validation.get("ValidationReceipt").is_none(), + "invalid draft returned a canonical draft or receipt" ); ensure!( invalid_validation["Findings"] diff --git a/crates/now-package-broker/src/command_builder/npm.rs b/crates/now-package-broker/src/command_builder/npm.rs index b7b8447141..a20455a756 100644 --- a/crates/now-package-broker/src/command_builder/npm.rs +++ b/crates/now-package-broker/src/command_builder/npm.rs @@ -333,15 +333,6 @@ mod tests { build_npm_command(&request).expect("implicit user scope runs with the standard token"); } - #[test] - fn elevated_user_scope_requests_run_with_the_standard_token() { - let mut request = make_request(); - request.client.requested_elevation = Elevation::Elevated; - request.options.scope = Some(Scope::User); - - build_npm_command(&request).expect("elevated user-scope npm runs with the standard token"); - } - #[test] fn machine_scope_requests_are_rejected() { let mut request = make_request(); diff --git a/crates/now-package-broker/src/evaluator/tests.rs b/crates/now-package-broker/src/evaluator/tests.rs index 4faf4eb0e4..4e766343e5 100644 --- a/crates/now-package-broker/src/evaluator/tests.rs +++ b/crates/now-package-broker/src/evaluator/tests.rs @@ -720,18 +720,6 @@ fn recommended_denied_custom_parameters_block_winget_installer_arguments() { assert_eq!(evaluate(&policy, &request).rule_id, "allow-winget"); } -#[test] -fn effective_elevation_lowers_explicit_user_scope_and_raises_machine_scope() { - use api::{Elevation as E, Scope as S}; - - assert_eq!(super::effective_elevation(E::Elevated, Some(S::User)), E::Standard); - assert_eq!(super::effective_elevation(E::Elevated, None), E::Elevated); - assert_eq!(super::effective_elevation(E::Elevated, Some(S::Machine)), E::Elevated); - assert_eq!(super::effective_elevation(E::Standard, Some(S::Machine)), E::Elevated); - assert_eq!(super::effective_elevation(E::Standard, Some(S::User)), E::Standard); - assert_eq!(super::effective_elevation(E::Standard, None), E::Standard); -} - #[test] fn implicit_user_scope_matches_standard_elevation_policy() { // oracle: regression Devolutions/devolutions-gateway#2036 discussion_r4379344015. @@ -890,7 +878,6 @@ fn agent_uninstall_by_installed_product_code_is_detected() { r"ARP\Machine\X64\{0A1B2C3D-4E5F-4A6B-8C7D-9E0F1A2B3C4D}", ); assert!(protects(&request)); - assert!(!super::uninstalls_protected_package(&request, None)); let mut request = make_request(api::Operation::Uninstall, "Contoso.Tool"); request.options.custom_parameters = vec![ diff --git a/crates/now-package-broker/src/policy_security.rs b/crates/now-package-broker/src/policy_security.rs index 8340afaed4..2f7054234a 100644 --- a/crates/now-package-broker/src/policy_security.rs +++ b/crates/now-package-broker/src/policy_security.rs @@ -2007,25 +2007,6 @@ mod tests { } } - #[test] - fn program_files_like_install_location_is_accepted() { - // `Program Files` grants Users read and execute, and full control to the owner of new objects. - let sd = SddlDescriptor::parse(&format!( - "O:{TRUSTED_INSTALLER_SID}D:PAI(A;;FA;;;{TRUSTED_INSTALLER_SID})(A;CIIO;GA;;;{TRUSTED_INSTALLER_SID})\ - (A;;0x1301bf;;;SY)(A;OICIIO;GA;;;SY)(A;;0x1301bf;;;BA)(A;OICIIO;GA;;;BA)\ - (A;;0x1200a9;;;BU)(A;OICIIO;GXGR;;;BU)(A;OICIIO;GA;;;CO)(A;;0x1200a9;;;AC)(A;OICIIO;GXGR;;;AC)" - )); - sd.verify_as_install_location() - .expect("Program Files-like folder must be accepted"); - } - - #[test] - fn administrators_only_install_location_is_accepted() { - let sd = SddlDescriptor::parse("O:BAD:P(A;OICI;FA;;;SY)(A;OICI;FA;;;BA)(A;OICI;0x1200a9;;;BU)"); - sd.verify_as_install_location() - .expect("SYSTEM/Administrators-only folder must be accepted"); - } - #[test] fn inheritable_untrusted_write_ace_is_rejected_for_install_locations_only() { // oracle: regression Devolutions/devolutions-gateway#2036 discussion_r4283749914