fix(agent): authenticate and select the WTS session by SID

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]

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<Sid> {
+    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<PathBuf> {
@@ -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()
+        );
     }
 }

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<String>,
     /// Optional shell command to run after the main command (`cmd.exe /S /C`).
     pub post_command: Option<String>,
-    /// 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<ExecutionOutput> {
         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(),

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<ProcessStartedCallback>,
@@ -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")?;
 

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> {
     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<u32> {
-    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<u32> {
             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.

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<StatusResponse, ErrorResponse> {
@@ -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<ExecutionResponse, ErrorResponse> {
+    async fn execute(&self, request: PackageRequest, user_sid: &Sid) -> Result<ExecutionResponse, ErrorResponse> {
         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<StatusResponse, ErrorResponse> {
-        self.status_for_client(request, String::new()).await
-    }
-}
-
-impl BrokerState {
     async fn status_for_client(
         &self,
         request: StatusRequest,