From 694cfbfb9dd0c5e29f3583c248c7a0ae021b5e04 Mon Sep 17 00:00:00 2001 From: Vladyslav Nikonov Date: Mon, 27 Jul 2026 16:41:43 +0300 Subject: [PATCH 1/2] fix(agent): authenticate and select the WTS session by SID, not display name Previously the package broker matched the pipe client identity against the request effective_user by comparing DOMAIN\name strings, and an unqualified effective_user ignored the domain entirely. On a host with both MACHINE\alice and DOMAIN\alice, the broker could run the operation in the wrong user's session. The caller SID is now captured at connect, threaded through ExecutionContext, and both authentication and WTS session selection compare SIDs instead of name strings. Issue: DGW-418 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- Cargo.lock | 1 + devolutions-agent/Cargo.toml | 1 + devolutions-agent/src/broker/auth.rs | 82 ++++++++++++++----- devolutions-agent/src/broker/executor/mod.rs | 9 +- .../src/broker/executor/windows/mod.rs | 16 ++-- .../src/broker/executor/windows/token.rs | 47 ++++++----- devolutions-agent/src/broker/server/mod.rs | 15 ++-- 7 files changed, 111 insertions(+), 60 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8ea7655a4..9d6959700 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1707,6 +1707,7 @@ dependencies = [ "tracing", "url", "uuid", + "widestring 1.2.1", "win-api-wrappers", "windows 0.61.3", "x509-parser", diff --git a/devolutions-agent/Cargo.toml b/devolutions-agent/Cargo.toml index 463965127..fe2339c85 100644 --- a/devolutions-agent/Cargo.toml +++ b/devolutions-agent/Cargo.toml @@ -97,6 +97,7 @@ notify-debouncer-mini = "0.6" reqwest = { version = "0.12", default-features = false, features = ["rustls-tls-native-roots", "http2", "socks"] } thiserror = "2" uuid = { version = "1.17", features = ["v4"] } +widestring = "1.2" win-api-wrappers = { path = "../crates/win-api-wrappers" } [target.'cfg(windows)'.dependencies.windows] diff --git a/devolutions-agent/src/broker/auth.rs b/devolutions-agent/src/broker/auth.rs index 7f5fd6479..3bcf3877b 100644 --- a/devolutions-agent/src/broker/auth.rs +++ b/devolutions-agent/src/broker/auth.rs @@ -6,6 +6,9 @@ use anyhow::{Context as _, bail}; use now_policy_api::{ClientContext, PackageRequest, StatusRequest}; use tokio::net::windows::named_pipe::NamedPipeServer; use tracing::{debug, warn}; +use widestring::U16CString; +use win_api_wrappers::identity::account::lookup_account_by_name; +use win_api_wrappers::identity::sid::Sid; use win_api_wrappers::process::Process; use windows::Win32::Security::TOKEN_QUERY; use windows::Win32::System::Threading::PROCESS_QUERY_LIMITED_INFORMATION; @@ -21,6 +24,8 @@ pub(crate) struct PipeClient { #[derive(Clone, Debug)] struct ClientUser { + /// Security identifier of the pipe client process token user, captured at connect. + sid: Sid, domain: String, name: String, } @@ -43,6 +48,7 @@ impl PipeClient { .lookup_account(None) .with_context(|| format!("failed to resolve pipe client process {process_id} user"))?; let user = ClientUser { + sid, domain: account.domain_name.to_string_lossy(), name: account.name.to_string_lossy(), }; @@ -54,6 +60,11 @@ impl PipeClient { }) } + /// Security identifier of the authenticated pipe client user, captured at connect. + pub(crate) fn user_sid(&self) -> &Sid { + &self.user.sid + } + pub(crate) fn validate_request( &self, request: &PackageRequest, @@ -95,16 +106,26 @@ impl PipeClient { Ok(()) } + /// Validate that the request's `effective_user` denotes the authenticated pipe client user. + /// + /// The name is resolved to a SID and compared against the SID captured at connect, + /// so distinct accounts sharing the same name (e.g. `MACHINE\alice` vs `DOMAIN\alice`) + /// cannot be confused with one another. fn validate_effective_user(&self, effective_user: &str) -> anyhow::Result<()> { - if same_user(effective_user, &self.user) { + let requested_sid = resolve_account_sid(effective_user) + .with_context(|| format!("failed to resolve request effective_user '{effective_user}'"))?; + + if requested_sid == self.user.sid { return Ok(()); } bail!( - "pipe client user '{}\\{}' does not match request effective_user '{}'", + "pipe client user '{}\\{}' ({}) does not match request effective_user '{}' ({})", self.user.domain, self.user.name, - effective_user + self.user.sid, + effective_user, + requested_sid ) } @@ -155,12 +176,11 @@ fn connected_pipe_client_process_id(server: &NamedPipeServer) -> anyhow::Result< Ok(process_id) } -fn same_user(expected: &str, actual: &ClientUser) -> bool { - let Some((expected_domain, expected_name)) = expected.rsplit_once('\\') else { - return expected.eq_ignore_ascii_case(&actual.name); - }; - - expected_domain.eq_ignore_ascii_case(&actual.domain) && expected_name.eq_ignore_ascii_case(&actual.name) +/// Resolve an account name (`DOMAIN\user` or `user`) to its security identifier. +fn resolve_account_sid(account_name: &str) -> anyhow::Result { + let account_name = U16CString::from_str(account_name).context("account name contains an interior NUL character")?; + let account = lookup_account_by_name(&account_name).context("failed to look up account by name")?; + Ok(account.sid.clone()) } fn canonicalize_for_comparison(path: &Path) -> anyhow::Result { @@ -175,27 +195,51 @@ fn same_windows_path(left: &Path, right: &Path) -> bool { #[cfg(test)] mod tests { + use windows::Win32::Security::WinLocalSystemSid; + use super::*; - fn client_user() -> ClientUser { - ClientUser { - domain: "CONTOSO".to_owned(), - name: "alice".to_owned(), + fn system_sid() -> Sid { + Sid::from_well_known(WinLocalSystemSid, None).expect("well-known SYSTEM SID") + } + + fn system_client() -> PipeClient { + PipeClient { + process_id: 0, + executable_path: PathBuf::new(), + user: ClientUser { + sid: system_sid(), + domain: "NT AUTHORITY".to_owned(), + name: "SYSTEM".to_owned(), + }, } } #[test] - fn same_user_matches_domain_qualified_user() { - assert!(same_user("contoso\\ALICE", &client_user())); + fn resolve_account_sid_resolves_qualified_name() { + let sid = resolve_account_sid("NT AUTHORITY\\SYSTEM").expect("SYSTEM account should resolve"); + assert_eq!(sid, system_sid()); + } + + #[test] + fn resolve_account_sid_resolves_unqualified_name() { + let sid = resolve_account_sid("SYSTEM").expect("SYSTEM account should resolve"); + assert_eq!(sid, system_sid()); + } + + #[test] + fn resolve_account_sid_rejects_unknown_account() { + assert!(resolve_account_sid("no-such-domain\\no-such-user-a2f6").is_err()); } #[test] - fn same_user_matches_unqualified_user() { - assert!(same_user("ALICE", &client_user())); + fn validate_effective_user_accepts_matching_sid() { + assert!(system_client().validate_effective_user("NT AUTHORITY\\SYSTEM").is_ok()); } #[test] - fn same_user_rejects_wrong_domain() { - assert!(!same_user("FABRIKAM\\alice", &client_user())); + fn validate_effective_user_rejects_different_account() { + // "Everyone" resolves to a different SID than the SYSTEM caller. + assert!(system_client().validate_effective_user("Everyone").is_err()); } } diff --git a/devolutions-agent/src/broker/executor/mod.rs b/devolutions-agent/src/broker/executor/mod.rs index bb0143a98..c23c55bb6 100644 --- a/devolutions-agent/src/broker/executor/mod.rs +++ b/devolutions-agent/src/broker/executor/mod.rs @@ -6,6 +6,7 @@ use async_trait::async_trait; use chrono::{DateTime, Utc}; use now_policy_api::{Elevation, Scope}; use tracing::info; +use win_api_wrappers::identity::sid::Sid; mod output; @@ -31,8 +32,13 @@ pub struct ExecutionContext { pub command: Vec, /// Optional shell command to run after the main command (`cmd.exe /S /C`). pub post_command: Option, - /// Windows identity of the target user (e.g., `DOMAIN\username`). + /// Windows identity of the target user (e.g., `DOMAIN\username`), used for display and logging. pub effective_user: String, + /// Security identifier of the target user, captured from the authenticated pipe client. + /// + /// 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. pub elevation: Elevation, /// Installation scope (machine scope requires elevation). @@ -69,6 +75,7 @@ impl CommandExecutor for DryRunExecutor { ) -> anyhow::Result { info!( effective_user = %ctx.effective_user, + user_sid = %ctx.user_sid, kill_processes = ?ctx.kill_processes, has_pre_command = ctx.pre_command.is_some(), command_len = ctx.command.len(), diff --git a/devolutions-agent/src/broker/executor/windows/mod.rs b/devolutions-agent/src/broker/executor/windows/mod.rs index 4d87b0289..32dcbd3dc 100644 --- a/devolutions-agent/src/broker/executor/windows/mod.rs +++ b/devolutions-agent/src/broker/executor/windows/mod.rs @@ -86,11 +86,10 @@ impl CommandExecutor for WindowsExecutor { /// Execute a command in the context of the target user's session (SYSTEM mode). /// /// Steps: -/// 1. Find the user's active session via WTS enumeration. -/// 2. Get the session token. -/// 3. If elevated execution is requested, obtain the linked elevated token. -/// 4. Set the token session ID and create the process. -/// 5. Wait for the process to exit and return the exit code. +/// 1. Find the user's active session (and its token) by matching the session token SID. +/// 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. fn execute_as_system( ctx: &ExecutionContext, process_started: Option, @@ -123,17 +122,16 @@ fn execute_as_system( debug!("All privileges enabled, finding user session"); - let session_id = find_user_session(&ctx.effective_user).context("failed to find active session for user")?; + let (session_id, user_token) = + find_user_session(&ctx.user_sid).context("failed to find active session for user")?; info!( effective_user = %ctx.effective_user, + user_sid = %ctx.user_sid, session_id, "Found user session" ); - debug!(session_id, "Calling Token::for_session"); - let user_token = Token::for_session(session_id).context("failed to obtain user token for session")?; - debug!("Duplicating user token as primary"); let primary_token = token::duplicate_as_primary(&user_token).context("failed to duplicate token as primary")?; diff --git a/devolutions-agent/src/broker/executor/windows/token.rs b/devolutions-agent/src/broker/executor/windows/token.rs index f59e46b8c..52edef939 100644 --- a/devolutions-agent/src/broker/executor/windows/token.rs +++ b/devolutions-agent/src/broker/executor/windows/token.rs @@ -1,6 +1,7 @@ //! Token and session helpers for Windows execution. use anyhow::{Context as _, bail}; +use tracing::debug; use win_api_wrappers::identity::sid::Sid; use win_api_wrappers::process::Process; use win_api_wrappers::token::{Token, TokenElevationType}; @@ -30,17 +31,15 @@ pub(super) fn duplicate_as_primary(token: &Token) -> anyhow::Result { token.duplicate(TOKEN_ALL_ACCESS, None, SecurityImpersonation, TokenPrimary) } -/// Enumerate WTS sessions to find one belonging to `effective_user`. +/// Enumerate WTS sessions to find the active one whose user token belongs to `user_sid`. /// -/// `effective_user` can be `DOMAIN\user` or just `user`. -pub(super) fn find_user_session(effective_user: &str) -> anyhow::Result { - let (target_domain, target_username) = effective_user - .rsplit_once('\\') - .map_or((None, effective_user), |(domain, username)| { - (Some(domain.to_lowercase()), username) - }); - let target_username = target_username.to_lowercase(); - +/// Matching is performed on the session token user SID rather than on the +/// `DOMAIN\username` display strings, so distinct accounts sharing the same +/// name (e.g. `MACHINE\alice` vs `DOMAIN\alice`) cannot be confused. +/// +/// Returns the session ID together with the session user token. +/// The caller must have the SeTcb privilege enabled (required by `WTSQueryUserToken`). +pub(super) fn find_user_session(user_sid: &Sid) -> anyhow::Result<(u32, Token)> { let sessions = wts::get_sessions().context("failed to enumerate WTS sessions")?; for session in &sessions { @@ -51,21 +50,27 @@ pub(super) fn find_user_session(effective_user: &str) -> anyhow::Result { continue; } - if let Ok(session_user) = wts::get_session_user_name(session.session_id) - && session_user.to_lowercase() == target_username - { - if let Some(target_domain) = &target_domain { - let session_domain = wts::get_session_domain_name(session.session_id) - .with_context(|| format!("failed to query domain for session {}", session.session_id))?; - if !session_domain.eq_ignore_ascii_case(target_domain) { - continue; - } + // Sessions without a logged-in user (or otherwise unqueryable) are skipped. + let token = match Token::for_session(session.session_id) { + Ok(token) => token, + Err(error) => { + debug!(session_id = session.session_id, %error, "Skipping session: failed to query user token"); + continue; + } + }; + + match token.sid_and_attributes() { + Ok(sid_and_attributes) if sid_and_attributes.sid == *user_sid => { + return Ok((session.session_id, token)); + } + Ok(_) => {} + Err(error) => { + debug!(session_id = session.session_id, %error, "Skipping session: failed to query token user SID"); } - return Ok(session.session_id); } } - anyhow::bail!("no active session found for user '{effective_user}'") + bail!("no active session found for user SID '{user_sid}'") } /// Attempt to obtain an elevated (linked) token from a filtered/limited token. diff --git a/devolutions-agent/src/broker/server/mod.rs b/devolutions-agent/src/broker/server/mod.rs index 4c51fe115..b2b3c40d3 100644 --- a/devolutions-agent/src/broker/server/mod.rs +++ b/devolutions-agent/src/broker/server/mod.rs @@ -14,6 +14,7 @@ use now_policy_api::{ }; use now_policy_server_template::{MAX_REQUEST_BODY_BYTES, PackageBrokerServer, SharedPackageBrokerServer}; use tracing::{info, trace, warn}; +use win_api_wrappers::identity::sid::Sid; use crate::broker::auth::PipeClient; use crate::broker::command_builder::build_command; @@ -90,7 +91,7 @@ impl PackageBrokerServer for BrokerConnection { error_response(ErrorCode::Unauthorized, "pipe client authentication failed") })?; - self.state.execute(request).await + self.state.execute(request, self.client.user_sid()).await } async fn status(&self, request: StatusRequest) -> Result { @@ -106,8 +107,7 @@ impl PackageBrokerServer for BrokerConnection { } } -#[async_trait] -impl PackageBrokerServer for BrokerState { +impl BrokerState { async fn health(&self) -> HealthResponse { let policy_guard = self.policy.read().expect("policy lock poisoned"); let (status, policy_id) = match policy_guard.as_ref() { @@ -153,7 +153,7 @@ impl PackageBrokerServer for BrokerState { }) } - async fn execute(&self, request: PackageRequest) -> Result { + async fn execute(&self, request: PackageRequest, user_sid: &Sid) -> Result { let evaluated = self.evaluate_request(&request)?; let operation = if evaluated.would_execute { let generated_operation_id = new_operation_id()?; @@ -169,6 +169,7 @@ impl PackageBrokerServer for BrokerState { command: evaluated.command.clone(), post_command: request.options.post_operation_command.clone(), effective_user: request.client.effective_user.clone(), + user_sid: user_sid.clone(), elevation: request.client.requested_elevation, scope: request.options.scope, capture_output: request.capture_output, @@ -216,12 +217,6 @@ impl PackageBrokerServer for BrokerState { }) } - async fn status(&self, request: StatusRequest) -> Result { - self.status_for_client(request, String::new()).await - } -} - -impl BrokerState { async fn status_for_client( &self, request: StatusRequest, From 733cbf044389cde1f7307514b0223afbefe47105 Mon Sep 17 00:00:00 2001 From: Vladyslav Nikonov Date: Mon, 27 Jul 2026 19:24:23 +0300 Subject: [PATCH 2/2] test(agent): resolve well-known account names for localized Windows Broker auth tests hard-coded English display names (NT AUTHORITY\SYSTEM, Everyone), which fail on non-English Windows installations. Resolve the well-known SIDs back to the host-localized names before exercising the name-to-SID path. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- devolutions-agent/src/broker/auth.rs | 44 +++++++++++++++++++++++----- 1 file changed, 36 insertions(+), 8 deletions(-) diff --git a/devolutions-agent/src/broker/auth.rs b/devolutions-agent/src/broker/auth.rs index 3bcf3877b..5ec62861b 100644 --- a/devolutions-agent/src/broker/auth.rs +++ b/devolutions-agent/src/broker/auth.rs @@ -195,7 +195,7 @@ fn same_windows_path(left: &Path, right: &Path) -> bool { #[cfg(test)] mod tests { - use windows::Win32::Security::WinLocalSystemSid; + use windows::Win32::Security::{WinLocalSystemSid, WinWorldSid}; use super::*; @@ -203,27 +203,46 @@ mod tests { Sid::from_well_known(WinLocalSystemSid, None).expect("well-known SYSTEM SID") } + /// Host-localized (domain, name) for the LocalSystem account. + fn system_account_names() -> (String, String) { + let account = system_sid().lookup_account(None).expect("SYSTEM account lookup"); + (account.domain_name.to_string_lossy(), account.name.to_string_lossy()) + } + + /// Host-localized unqualified name for the Everyone (World) group. + fn everyone_account_name() -> String { + Sid::from_well_known(WinWorldSid, None) + .expect("well-known Everyone SID") + .lookup_account(None) + .expect("Everyone account lookup") + .name + .to_string_lossy() + } + fn system_client() -> PipeClient { + let (domain, name) = system_account_names(); PipeClient { process_id: 0, executable_path: PathBuf::new(), user: ClientUser { sid: system_sid(), - domain: "NT AUTHORITY".to_owned(), - name: "SYSTEM".to_owned(), + domain, + name, }, } } #[test] fn resolve_account_sid_resolves_qualified_name() { - let sid = resolve_account_sid("NT AUTHORITY\\SYSTEM").expect("SYSTEM account should resolve"); + let (domain, name) = system_account_names(); + let sid = resolve_account_sid(&format!("{domain}\\{name}")).expect("SYSTEM account should resolve"); assert_eq!(sid, system_sid()); } #[test] fn resolve_account_sid_resolves_unqualified_name() { - let sid = resolve_account_sid("SYSTEM").expect("SYSTEM account should resolve"); + let (_, name) = system_account_names(); + let sid = resolve_account_sid(&name).expect("SYSTEM account should resolve"); assert_eq!(sid, system_sid()); } @@ -234,12 +253,21 @@ mod tests { #[test] fn validate_effective_user_accepts_matching_sid() { - assert!(system_client().validate_effective_user("NT AUTHORITY\\SYSTEM").is_ok()); + let (domain, name) = system_account_names(); + assert!( + system_client() + .validate_effective_user(&format!("{domain}\\{name}")) + .is_ok() + ); } #[test] fn validate_effective_user_rejects_different_account() { - // "Everyone" resolves to a different SID than the SYSTEM caller. - assert!(system_client().validate_effective_user("Everyone").is_err()); + // The Everyone group resolves to a different SID than the SYSTEM caller. + assert!( + system_client() + .validate_effective_user(&everyone_account_name()) + .is_err() + ); } }