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..5ec62861b 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,79 @@ fn same_windows_path(left: &Path, right: &Path) -> bool { #[cfg(test)] mod tests { + use windows::Win32::Security::{WinLocalSystemSid, WinWorldSid}; + 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") + } + + /// 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, + name, + }, } } #[test] - fn same_user_matches_domain_qualified_user() { - assert!(same_user("contoso\\ALICE", &client_user())); + fn resolve_account_sid_resolves_qualified_name() { + 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 (_, name) = system_account_names(); + let sid = resolve_account_sid(&name).expect("SYSTEM account should resolve"); + assert_eq!(sid, system_sid()); } #[test] - fn same_user_matches_unqualified_user() { - assert!(same_user("ALICE", &client_user())); + fn resolve_account_sid_rejects_unknown_account() { + assert!(resolve_account_sid("no-such-domain\\no-such-user-a2f6").is_err()); } #[test] - fn same_user_rejects_wrong_domain() { - assert!(!same_user("FABRIKAM\\alice", &client_user())); + fn validate_effective_user_accepts_matching_sid() { + 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() { + // The Everyone group resolves to a different SID than the SYSTEM caller. + assert!( + system_client() + .validate_effective_user(&everyone_account_name()) + .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,