From 9b6532c4c13822fce989b19d62d4ed758e8435ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Mon, 7 Sep 2026 18:53:56 -0400 Subject: [PATCH 01/10] fix(agent): retain broker caller identity Retain the named-pipe connector process and reject recycled or exited process identities before authorizing requests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- crates/now-package-broker/src/auth.rs | 149 ++++++++++++++++++++++++- crates/win-api-wrappers/src/process.rs | 36 +++++- 2 files changed, 175 insertions(+), 10 deletions(-) diff --git a/crates/now-package-broker/src/auth.rs b/crates/now-package-broker/src/auth.rs index c5126dcb0..b2b944d5c 100644 --- a/crates/now-package-broker/src/auth.rs +++ b/crates/now-package-broker/src/auth.rs @@ -1,6 +1,8 @@ //! Package broker pipe client authentication. use std::path::{Path, PathBuf}; +use std::sync::Arc; +use std::time::SystemTime; use anyhow::{Context as _, bail}; use devolutions_agent_shared::windows::code_signing::validate_devolutions_authenticode_signature; @@ -11,13 +13,26 @@ 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::Foundation::{WAIT_OBJECT_0, WAIT_TIMEOUT}; use windows::Win32::Security::{TOKEN_DUPLICATE, TOKEN_QUERY, WinBuiltinAdministratorsSid}; use windows::Win32::Storage::FileSystem::FILE_ID_INFO; -use windows::Win32::System::Threading::PROCESS_QUERY_LIMITED_INFORMATION; +use windows::Win32::System::Threading::{PROCESS_ACCESS_RIGHTS, PROCESS_QUERY_LIMITED_INFORMATION}; + +const PROCESS_SYNCHRONIZE: PROCESS_ACCESS_RIGHTS = PROCESS_ACCESS_RIGHTS(0x0010_0000); +const PROCESS_IDENTITY_ACCESS: PROCESS_ACCESS_RIGHTS = + PROCESS_ACCESS_RIGHTS(PROCESS_QUERY_LIMITED_INFORMATION.0 | PROCESS_SYNCHRONIZE.0); + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +struct ProcessInstanceIdentity { + process_id: u32, + creation_time: SystemTime, +} #[derive(Clone, Debug)] pub(crate) struct PipeClient { process_id: u32, + process_creation_time: SystemTime, + process: Option>, executable_path: PathBuf, /// Security identifier of the pipe client process token user, captured at connect. user_sid: Sid, @@ -35,12 +50,46 @@ impl PipeClient { /// unauthenticated work a connection flood can trigger. pub(crate) fn from_connected_pipe(server: &NamedPipeServer) -> anyhow::Result { let process_id = connected_pipe_client_process_id(server).context("failed to query pipe client process id")?; - Self::from_process_id(process_id) + let process = Arc::new( + Process::get_by_pid(process_id, PROCESS_IDENTITY_ACCESS) + .with_context(|| format!("failed to open pipe client process {process_id}"))?, + ); + Self::ensure_process_active(process_id, &process)?; + let process_instance = process_instance_identity(process_id, &process)?; + let client = Self::from_process(process_instance, Arc::clone(&process))?; + let confirmed_process_id = + connected_pipe_client_process_id(server).context("failed to confirm pipe client process id")?; + if confirmed_process_id != process_id { + bail!("pipe client process changed while its identity was captured"); + } + Self::ensure_process_active(process_id, &process)?; + let confirmation = Process::get_by_pid(process_id, PROCESS_IDENTITY_ACCESS) + .with_context(|| format!("failed to reopen pipe client process {process_id}"))?; + ensure_same_process_instance(process_instance, process_instance_identity(process_id, &confirmation)?)?; + Ok(client) + } + + fn ensure_process_active(process_id: u32, process: &Process) -> anyhow::Result<()> { + match process + .wait(Some(0)) + .with_context(|| format!("failed to query pipe client process {process_id} state"))? + { + WAIT_TIMEOUT => Ok(()), + WAIT_OBJECT_0 => bail!("pipe client process {process_id} exited while its identity was captured"), + status => bail!("unexpected wait status {status:?} for pipe client process {process_id}"), + } } fn from_process_id(process_id: u32) -> anyhow::Result { - let process = Process::get_by_pid(process_id, PROCESS_QUERY_LIMITED_INFORMATION) - .with_context(|| format!("failed to open pipe client process {process_id}"))?; + let process = Arc::new( + Process::get_by_pid(process_id, PROCESS_IDENTITY_ACCESS) + .with_context(|| format!("failed to open pipe client process {process_id}"))?, + ); + Self::from_process(process_instance_identity(process_id, &process)?, process) + } + + fn from_process(process_instance: ProcessInstanceIdentity, process: Arc) -> anyhow::Result { + let process_id = process_instance.process_id; let executable_path = process .exe_path() .with_context(|| format!("failed to query pipe client process {process_id} executable path"))?; @@ -62,6 +111,8 @@ impl PipeClient { Ok(Self { process_id, + process_creation_time: process_instance.creation_time, + process: Some(process), executable_path, user_sid, is_elevated, @@ -124,6 +175,7 @@ impl PipeClient { } pub(crate) fn validate_connection(&self, skip_signature_validation: bool) -> anyhow::Result<()> { + self.validate_process_instance()?; if signature_validation_skipped(skip_signature_validation) { warn!("DEBUG MODE: Skipping package broker client signature validation"); return Ok(()); @@ -133,6 +185,7 @@ impl PipeClient { debug!( process_id = self.process_id, + process_creation_time = ?self.process_creation_time, executable = %self.executable_path.display(), certificate_thumbprint = %thumbprint, "Package broker pipe client authenticated" @@ -141,6 +194,21 @@ impl PipeClient { Ok(()) } + fn validate_process_instance(&self) -> anyhow::Result<()> { + let Some(process) = &self.process else { + return Ok(()); + }; + + Self::ensure_process_active(self.process_id, process)?; + ensure_same_process_instance( + ProcessInstanceIdentity { + process_id: self.process_id, + creation_time: self.process_creation_time, + }, + process_instance_identity(self.process_id, process)?, + ) + } + /// 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, @@ -231,6 +299,25 @@ fn connected_pipe_client_process_id(server: &NamedPipeServer) -> anyhow::Result< Ok(process_id) } +fn process_instance_identity(process_id: u32, process: &Process) -> anyhow::Result { + Ok(ProcessInstanceIdentity { + process_id, + creation_time: process + .creation_time() + .with_context(|| format!("failed to query pipe client process {process_id} creation time"))?, + }) +} + +fn ensure_same_process_instance( + expected: ProcessInstanceIdentity, + actual: ProcessInstanceIdentity, +) -> anyhow::Result<()> { + if expected != actual { + bail!("pipe client process instance changed while its identity was captured"); + } + Ok(()) +} + /// 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")?; @@ -305,6 +392,8 @@ mod tests { fn system_client() -> PipeClient { PipeClient { process_id: 0, + process_creation_time: SystemTime::UNIX_EPOCH, + process: None, executable_path: PathBuf::new(), user_sid: system_sid(), is_elevated: true, @@ -312,6 +401,56 @@ mod tests { } } + #[test] + fn mismatched_process_creation_time_is_rejected() { + let expected = ProcessInstanceIdentity { + process_id: 42, + creation_time: SystemTime::UNIX_EPOCH, + }; + let actual = ProcessInstanceIdentity { + process_id: 42, + creation_time: SystemTime::UNIX_EPOCH + std::time::Duration::from_nanos(100), + }; + + ensure_same_process_instance(expected, actual) + .expect_err("a recycled PID with a different creation time must be rejected"); + } + + #[test] + fn exited_process_cannot_supply_executable_identity() { + let mut child = std::process::Command::new("powershell.exe") + .args(["-NoProfile", "-Command", "exit 0"]) + .spawn() + .expect("start short-lived child"); + let process_id = child.id(); + let process = Process::get_by_pid(process_id, PROCESS_IDENTITY_ACCESS).expect("open child while it is running"); + child.wait().expect("wait for child"); + + let error = PipeClient::ensure_process_active(process_id, &process) + .expect_err("an exited process cannot authenticate a connected pipe client"); + assert!(error.to_string().contains("exited while its identity was captured")); + } + + #[test] + fn pipe_client_retains_process_handle_and_rejects_exit() { + let mut child = std::process::Command::new("powershell.exe") + .args(["-NoProfile", "-Command", "Start-Sleep -Seconds 30"]) + .spawn() + .expect("start child"); + let client = PipeClient::from_process_id(child.id()).expect("capture child identity"); + assert!( + client.process.is_some(), + "the exact authenticated process handle must be retained" + ); + + child.kill().expect("terminate child"); + child.wait().expect("wait for child"); + + client + .validate_process_instance() + .expect_err("an inherited pipe cannot outlive the authenticated process"); + } + #[cfg(not(feature = "dev-skip-broker-signature"))] fn client_user_sid() -> Sid { system_client().user_sid @@ -398,6 +537,8 @@ mod tests { // even though the configuration requests skipping it. let client = PipeClient { process_id: std::process::id(), + process_creation_time: SystemTime::UNIX_EPOCH, + process: None, executable_path: std::env::current_exe().expect("current test executable path"), user_sid: client_user_sid(), is_elevated: false, diff --git a/crates/win-api-wrappers/src/process.rs b/crates/win-api-wrappers/src/process.rs index 06dc69f5c..e4a791992 100644 --- a/crates/win-api-wrappers/src/process.rs +++ b/crates/win-api-wrappers/src/process.rs @@ -3,13 +3,14 @@ use std::ffi::{OsString, c_void}; use std::fmt::Debug; use std::os::windows::ffi::OsStringExt; use std::path::{Path, PathBuf}; +use std::time::{Duration, SystemTime}; use std::{ptr, slice}; use anyhow::{Context, Result, bail}; use tracing::{error, warn}; use windows::Win32::Foundation::{ - E_INVALIDARG, ERROR_INCORRECT_SIZE, ERROR_NO_MORE_FILES, FreeLibrary, HANDLE, HMODULE, HWND, LPARAM, MAX_PATH, - WAIT_EVENT, WAIT_FAILED, WPARAM, + E_INVALIDARG, ERROR_INCORRECT_SIZE, ERROR_NO_MORE_FILES, FILETIME, FreeLibrary, HANDLE, HMODULE, HWND, LPARAM, + MAX_PATH, WAIT_EVENT, WAIT_FAILED, WPARAM, }; use windows::Win32::Security::{TOKEN_ACCESS_MASK, TOKEN_ADJUST_PRIVILEGES, TOKEN_QUERY}; use windows::Win32::System::Com::{COINIT_APARTMENTTHREADED, COINIT_DISABLE_OLE1DDE}; @@ -24,10 +25,11 @@ use windows::Win32::System::LibraryLoader::{ use windows::Win32::System::RemoteDesktop::ProcessIdToSessionId; use windows::Win32::System::Threading::{ CREATE_UNICODE_ENVIRONMENT, CreateProcessAsUserW, CreateRemoteThread, EXTENDED_STARTUPINFO_PRESENT, - GetCurrentProcess, GetCurrentProcessId, GetExitCodeProcess, INFINITE, LPPROC_THREAD_ATTRIBUTE_LIST, - LPTHREAD_START_ROUTINE, OpenProcess, OpenProcessToken, PEB, PROCESS_ACCESS_RIGHTS, PROCESS_BASIC_INFORMATION, - PROCESS_CREATION_FLAGS, PROCESS_INFORMATION, PROCESS_NAME_WIN32, PROCESS_TERMINATE, QueryFullProcessImageNameW, - STARTUPINFOEXW, STARTUPINFOW, STARTUPINFOW_FLAGS, TerminateProcess, WaitForSingleObject, + GetCurrentProcess, GetCurrentProcessId, GetExitCodeProcess, GetProcessTimes, INFINITE, + LPPROC_THREAD_ATTRIBUTE_LIST, LPTHREAD_START_ROUTINE, OpenProcess, OpenProcessToken, PEB, PROCESS_ACCESS_RIGHTS, + PROCESS_BASIC_INFORMATION, PROCESS_CREATION_FLAGS, PROCESS_INFORMATION, PROCESS_NAME_WIN32, PROCESS_TERMINATE, + QueryFullProcessImageNameW, STARTUPINFOEXW, STARTUPINFOW, STARTUPINFOW_FLAGS, TerminateProcess, + WaitForSingleObject, }; use windows::Win32::UI::Shell::{SEE_MASK_NOCLOSEPROCESS, SHELLEXECUTEINFOW, ShellExecuteExW}; use windows::Win32::UI::WindowsAndMessaging::{ @@ -196,6 +198,28 @@ impl Process { Ok(Token::from(handle)) } + pub fn creation_time(&self) -> Result { + let mut creation = FILETIME::default(); + let mut exit = FILETIME::default(); + let mut kernel = FILETIME::default(); + let mut user = FILETIME::default(); + + // SAFETY: All output pointers are valid for the duration of the call. + unsafe { GetProcessTimes(self.handle.raw(), &mut creation, &mut exit, &mut kernel, &mut user) }?; + + const WINDOWS_TO_UNIX_EPOCH_SECONDS: u64 = 11_644_473_600; + const TICKS_PER_SECOND: u64 = 10_000_000; + + let ticks = (u64::from(creation.dwHighDateTime) << 32) | u64::from(creation.dwLowDateTime); + let seconds = ticks / TICKS_PER_SECOND; + let unix_seconds = seconds + .checked_sub(WINDOWS_TO_UNIX_EPOCH_SECONDS) + .context("process creation time predates the Unix epoch")?; + let nanos = u32::try_from((ticks % TICKS_PER_SECOND) * 100).expect("FILETIME subsecond value fits in u32"); + + Ok(SystemTime::UNIX_EPOCH + Duration::new(unix_seconds, nanos)) + } + pub fn wait(&self, timeout_ms: Option) -> Result { // SAFETY: No preconditions. let status = unsafe { WaitForSingleObject(self.handle.raw(), timeout_ms.unwrap_or(INFINITE)) }; From c25a0635bbc59131e223e148f03806e46fad227d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Mon, 7 Sep 2026 18:56:27 -0400 Subject: [PATCH 02/10] fix(agent): bind broker caller to its image Resolve the connector's kernel-backed main image, reject non-local image paths, and retain the exact mapped file object for subsequent authentication. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- crates/now-package-broker/src/auth.rs | 139 ++++++++++++++++++++----- crates/win-api-wrappers/src/process.rs | 78 +++++++++++++- crates/win-api-wrappers/src/undoc.rs | 30 ++++++ 3 files changed, 222 insertions(+), 25 deletions(-) diff --git a/crates/now-package-broker/src/auth.rs b/crates/now-package-broker/src/auth.rs index b2b944d5c..a748e3ec4 100644 --- a/crates/now-package-broker/src/auth.rs +++ b/crates/now-package-broker/src/auth.rs @@ -1,5 +1,8 @@ //! Package broker pipe client authentication. +use std::ffi::OsString; +use std::fs::{File, OpenOptions}; +use std::os::windows::fs::OpenOptionsExt as _; use std::path::{Path, PathBuf}; use std::sync::Arc; use std::time::SystemTime; @@ -13,14 +16,17 @@ 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::Foundation::{WAIT_OBJECT_0, WAIT_TIMEOUT}; +use windows::Win32::Foundation::{GENERIC_READ, WAIT_OBJECT_0, WAIT_TIMEOUT}; use windows::Win32::Security::{TOKEN_DUPLICATE, TOKEN_QUERY, WinBuiltinAdministratorsSid}; -use windows::Win32::Storage::FileSystem::FILE_ID_INFO; -use windows::Win32::System::Threading::{PROCESS_ACCESS_RIGHTS, PROCESS_QUERY_LIMITED_INFORMATION}; +use windows::Win32::Storage::FileSystem::{FILE_EXECUTE, FILE_ID_INFO, FILE_READ_ATTRIBUTES, FILE_SHARE_READ}; +use windows::Win32::System::Threading::{ + PROCESS_ACCESS_RIGHTS, PROCESS_QUERY_INFORMATION, PROCESS_QUERY_LIMITED_INFORMATION, PROCESS_VM_READ, +}; const PROCESS_SYNCHRONIZE: PROCESS_ACCESS_RIGHTS = PROCESS_ACCESS_RIGHTS(0x0010_0000); -const PROCESS_IDENTITY_ACCESS: PROCESS_ACCESS_RIGHTS = - PROCESS_ACCESS_RIGHTS(PROCESS_QUERY_LIMITED_INFORMATION.0 | PROCESS_SYNCHRONIZE.0); +const PROCESS_IDENTITY_ACCESS: PROCESS_ACCESS_RIGHTS = PROCESS_ACCESS_RIGHTS( + PROCESS_QUERY_INFORMATION.0 | PROCESS_QUERY_LIMITED_INFORMATION.0 | PROCESS_VM_READ.0 | PROCESS_SYNCHRONIZE.0, +); #[derive(Clone, Copy, Debug, PartialEq, Eq)] struct ProcessInstanceIdentity { @@ -34,6 +40,7 @@ pub(crate) struct PipeClient { process_creation_time: SystemTime, process: Option>, executable_path: PathBuf, + executable_file: Option>, /// Security identifier of the pipe client process token user, captured at connect. user_sid: Sid, /// Actual connected process token elevation, captured at connect. @@ -90,12 +97,27 @@ impl PipeClient { fn from_process(process_instance: ProcessInstanceIdentity, process: Arc) -> anyhow::Result { let process_id = process_instance.process_id; - let executable_path = process - .exe_path() - .with_context(|| format!("failed to query pipe client process {process_id} executable path"))?; let token = process .token(TOKEN_QUERY | TOKEN_DUPLICATE) .with_context(|| format!("failed to open pipe client process {process_id} token"))?; + let executable_path = process + .exe_path() + .with_context(|| format!("failed to query pipe client process {process_id} executable path"))?; + let (_image_address, mapped_executable_path) = process + .main_image_mapped_path() + .with_context(|| format!("failed to query pipe client process {process_id} mapped executable image"))?; + if !is_supported_local_image_path(&mapped_executable_path) { + bail!("pipe client process {process_id} mapped executable is not on a supported local volume"); + } + let executable_file = Arc::new(open_native_executable_file(&mapped_executable_path).with_context(|| { + format!( + "failed to retain pipe client process {process_id} mapped executable '{}'", + executable_path.display() + ) + })?); + process.verify_image_file_mapping(&executable_file).with_context(|| { + format!("pipe client process {process_id} mapped executable file does not match its image") + })?; let user_sid = token .sid_and_attributes() .with_context(|| format!("failed to query pipe client process {process_id} token user"))? @@ -108,12 +130,16 @@ impl PipeClient { let is_administrator = token .is_member(&administrators_sid) .with_context(|| format!("failed to query pipe client process {process_id} Administrators membership"))?; + process + .verify_image_file_mapping(&executable_file) + .with_context(|| format!("pipe client process {process_id} executable image changed during capture"))?; Ok(Self { process_id, process_creation_time: process_instance.creation_time, process: Some(process), executable_path, + executable_file: Some(executable_file), user_sid, is_elevated, is_administrator, @@ -251,12 +277,16 @@ impl PipeClient { bail!("request client executable path is not absolute"); } - let actual_id = file_id(&self.executable_path).with_context(|| { - format!( - "failed to query pipe client executable '{}' file identity", - self.executable_path.display() - ) - })?; + let actual_id = if let Some(executable_file) = &self.executable_file { + file_id_from_handle(executable_file).context("failed to query retained pipe client executable identity")? + } else { + file_id(&self.executable_path).with_context(|| { + format!( + "failed to query pipe client executable '{}' file identity", + self.executable_path.display() + ) + })? + }; let requested_id = file_id(requested_path).with_context(|| { format!("failed to query request client executable '{requested_executable_path}' file identity") })?; @@ -318,6 +348,25 @@ fn ensure_same_process_instance( Ok(()) } +fn open_executable_file(path: &Path) -> anyhow::Result { + OpenOptions::new() + .access_mode(GENERIC_READ.0 | FILE_READ_ATTRIBUTES.0 | FILE_EXECUTE.0) + .share_mode(FILE_SHARE_READ.0) + .open(path) + .context("failed to open executable without write or delete sharing") +} + +fn open_native_executable_file(native_path: &Path) -> anyhow::Result { + let mut global_root_path = OsString::from(r"\\?\GLOBALROOT"); + global_root_path.push(native_path.as_os_str()); + open_executable_file(Path::new(&global_root_path)) +} + +fn is_supported_local_image_path(path: &Path) -> bool { + let path = path.as_os_str().to_string_lossy().to_ascii_lowercase(); + path.starts_with(r"\device\harddiskvolume") || path.starts_with(r"\device\volume{") +} + /// 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")?; @@ -327,22 +376,23 @@ fn resolve_account_sid(account_name: &str) -> anyhow::Result { /// Queries the volume serial number and 128-bit file ID uniquely identifying the file. fn file_id(path: &Path) -> anyhow::Result { - use std::os::windows::fs::OpenOptionsExt as _; - use std::os::windows::io::AsRawHandle as _; - - use windows::Win32::Foundation::HANDLE; - use windows::Win32::Storage::FileSystem::{ - FILE_READ_ATTRIBUTES, FILE_SHARE_DELETE, FILE_SHARE_READ, FILE_SHARE_WRITE, FileIdInfo, - GetFileInformationByHandleEx, - }; + use windows::Win32::Storage::FileSystem::{FILE_SHARE_DELETE, FILE_SHARE_WRITE}; - let file = std::fs::OpenOptions::new() + let file = OpenOptions::new() .access_mode(FILE_READ_ATTRIBUTES.0) .share_mode((FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE).0) .open(path)?; - let mut info = FILE_ID_INFO::default(); + file_id_from_handle(&file) +} + +fn file_id_from_handle(file: &File) -> anyhow::Result { + use std::os::windows::io::AsRawHandle as _; + + use windows::Win32::Foundation::HANDLE; + use windows::Win32::Storage::FileSystem::{FileIdInfo, GetFileInformationByHandleEx}; + let mut info = FILE_ID_INFO::default(); let info_size = u32::try_from(size_of::()).expect("FILE_ID_INFO size fits in u32"); // SAFETY: `file` is an open file handle, and the output pointer points to a @@ -395,6 +445,7 @@ mod tests { process_creation_time: SystemTime::UNIX_EPOCH, process: None, executable_path: PathBuf::new(), + executable_file: None, user_sid: system_sid(), is_elevated: true, is_administrator: true, @@ -521,6 +572,45 @@ mod tests { assert!(!same_file(&exe_id, &temp_id)); } + #[test] + fn process_image_file_mapping_accepts_the_main_image_and_rejects_a_signed_substitute() { + let process = Process::get_by_pid(std::process::id(), PROCESS_IDENTITY_ACCESS).expect("open current process"); + let executable = + open_executable_file(&std::env::current_exe().expect("current executable")).expect("open current image"); + process + .verify_image_file_mapping(&executable) + .expect("current executable must match its process image"); + + let Some(windows_dir) = std::env::var_os("WINDIR") else { + return; + }; + let signed_substitute = open_executable_file(&PathBuf::from(windows_dir).join(r"System32\cmd.exe")) + .expect("open signed substitute"); + process + .verify_image_file_mapping(&signed_substitute) + .expect_err("a different signed image mapping must not substitute for the main executable"); + } + + #[test] + fn mapped_image_path_rejects_network_and_non_volume_devices() { + assert!(is_supported_local_image_path(Path::new( + r"\Device\HarddiskVolume3\Program Files\Devolutions\client.exe" + ))); + assert!(is_supported_local_image_path(Path::new( + r"\Device\Volume{01234567-89ab-cdef-0123-456789abcdef}\client.exe" + ))); + + for path in [ + r"\Device\Mup\server\share\client.exe", + r"\Device\LanmanRedirector\server\share\client.exe", + r"\Device\WebDavRedirector\server\share\client.exe", + r"\??\UNC\server\share\client.exe", + r"\\server\share\client.exe", + ] { + assert!(!is_supported_local_image_path(Path::new(path)), "{path}"); + } + } + #[cfg(not(feature = "dev-skip-broker-signature"))] mod shipping_build { use super::*; @@ -540,6 +630,7 @@ mod tests { process_creation_time: SystemTime::UNIX_EPOCH, process: None, executable_path: std::env::current_exe().expect("current test executable path"), + executable_file: None, user_sid: client_user_sid(), is_elevated: false, is_administrator: false, diff --git a/crates/win-api-wrappers/src/process.rs b/crates/win-api-wrappers/src/process.rs index e4a791992..9ea36da84 100644 --- a/crates/win-api-wrappers/src/process.rs +++ b/crates/win-api-wrappers/src/process.rs @@ -1,7 +1,9 @@ use std::collections::HashMap; use std::ffi::{OsString, c_void}; use std::fmt::Debug; +use std::fs::File; use std::os::windows::ffi::OsStringExt; +use std::os::windows::io::AsRawHandle as _; use std::path::{Path, PathBuf}; use std::time::{Duration, SystemTime}; use std::{ptr, slice}; @@ -22,6 +24,7 @@ use windows::Win32::System::Environment::{CreateEnvironmentBlock, DestroyEnviron use windows::Win32::System::LibraryLoader::{ GET_MODULE_HANDLE_EX_FLAG_FROM_ADDRESS, GetModuleFileNameW, GetModuleHandleExW, GetProcAddress, }; +use windows::Win32::System::ProcessStatus::K32GetMappedFileNameW; use windows::Win32::System::RemoteDesktop::ProcessIdToSessionId; use windows::Win32::System::Threading::{ CREATE_UNICODE_ENVIRONMENT, CreateProcessAsUserW, CreateRemoteThread, EXTENDED_STARTUPINFO_PRESENT, @@ -43,7 +46,10 @@ use crate::security::attributes::SecurityAttributes; use crate::security::privilege::{self, ScopedPrivileges}; use crate::thread::Thread; use crate::token::Token; -use crate::undoc::{NtQueryInformationProcess, ProcessBasicInformation, RTL_USER_PROCESS_PARAMETERS}; +use crate::undoc::{ + NtQueryInformationProcess, ProcessBasicInformation, ProcessImageFileMapping, ProcessImageInformation, + RTL_USER_PROCESS_PARAMETERS, SECTION_IMAGE_INFORMATION, +}; use crate::utils::{Allocation, AnsiString, ComContext, CommandLine, WideString, u32size_of}; #[derive(Debug)] @@ -105,6 +111,76 @@ impl Process { Ok(OsString::from_wide(&path).into()) } + /// Returns an address and candidate native path for the process's main image. + /// + /// `ProcessImageInformation` comes from the kernel image section rather than the + /// target-controlled PEB or loader module list. + /// Callers must confirm the opened candidate with [`Process::verify_image_file_mapping`]. + pub fn main_image_mapped_path(&self) -> Result<(usize, PathBuf)> { + let mut image = SECTION_IMAGE_INFORMATION::default(); + + // SAFETY: `image` is a writable buffer with the exact native structure size. + unsafe { + NtQueryInformationProcess( + self.handle.raw(), + ProcessImageInformation, + (&raw mut image).cast(), + u32size_of::(), + None, + ) + }?; + + let image_address = image.TransferAddress as usize; + if image_address == 0 { + bail!(Error::NullPointer("SECTION_IMAGE_INFORMATION::TransferAddress")); + } + + let mut capacity = MAX_PATH as usize; + loop { + let mut path = vec![0u16; capacity]; + // SAFETY: `image_address` is inside the kernel-reported main image section, + // and `path` is a writable UTF-16 output buffer. + let length = unsafe { + K32GetMappedFileNameW(self.handle.raw(), image_address as *const c_void, path.as_mut_slice()) + }; + if length == 0 { + bail!(Error::last_error()); + } + if usize::try_from(length).expect("u32 fits in usize") < path.len() { + path.truncate(length as usize); + return Ok((image_address, OsString::from_wide(&path).into())); + } + capacity = capacity + .checked_mul(2) + .filter(|capacity| u16::try_from(*capacity).is_ok()) + .context("main image mapped path is too long")?; + } + } + + /// Verifies that `file` shares the section-object pointer used by this process's main image. + /// + /// `ProcessImageFileMapping` compares the kernel file objects' section pointers. + /// It identifies the backing file object but does not attest the bytes originally mapped + /// into the image section. + /// The file handle is an input buffer despite `NtQueryInformationProcess`'s generic output-buffer signature. + /// The handle must include `SYNCHRONIZE | FILE_EXECUTE` access. + pub fn verify_image_file_mapping(&self, file: &File) -> Result<()> { + let mut file_handle = HANDLE(file.as_raw_handle()); + + // SAFETY: ProcessImageFileMapping reads one valid HANDLE-sized input value. + unsafe { + NtQueryInformationProcess( + self.handle.raw(), + ProcessImageFileMapping, + (&raw mut file_handle).cast(), + u32size_of::(), + None, + ) + }?; + + Ok(()) + } + pub fn inject_dll(&self, path: &Path) -> Result<()> { let path = WideString::from(path).0.expect("WideString::from failed"); diff --git a/crates/win-api-wrappers/src/undoc.rs b/crates/win-api-wrappers/src/undoc.rs index 99d3680f4..66a7c6df6 100644 --- a/crates/win-api-wrappers/src/undoc.rs +++ b/crates/win-api-wrappers/src/undoc.rs @@ -171,6 +171,36 @@ pub const LOGON32_PROVIDER_VIRTUAL: LOGON32_PROVIDER = LOGON32_PROVIDER(4u32); /// Actually 68, we are generous pub const SECURITY_MAX_SID_SIZE: u32 = 256; +/// Kernel-backed image-section information for the process's main executable. +pub const ProcessImageInformation: PROCESSINFOCLASS = PROCESSINFOCLASS(37); + +/// Compares an input `SYNCHRONIZE | FILE_EXECUTE` file handle with the process image file. +pub const ProcessImageFileMapping: PROCESSINFOCLASS = PROCESSINFOCLASS(44); + +#[repr(C)] +#[derive(Clone, Copy, Debug, Default)] +#[allow(non_snake_case)] +pub struct SECTION_IMAGE_INFORMATION { + pub TransferAddress: *mut c_void, + pub ZeroBits: u32, + pub MaximumStackSize: usize, + pub CommittedStackSize: usize, + pub SubSystemType: u32, + pub SubSystemVersion: u32, + pub OperatingSystemVersion: u32, + pub ImageCharacteristics: u16, + pub DllCharacteristics: u16, + pub Machine: u16, + pub ImageContainsCode: u8, + pub ImageFlags: u8, + pub LoaderFlags: u32, + pub ImageFileSize: u32, + pub CheckSum: u32, +} + +const _: () = + assert!(size_of::() == if cfg!(target_pointer_width = "64") { 64 } else { 48 }); + #[repr(transparent)] #[derive(PartialEq, Eq, Copy, Clone, Default)] pub struct PROCESSINFOCLASS(i32); From 27fd94e617889cd2f866369419328843ce510309 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Mon, 7 Sep 2026 19:02:29 -0400 Subject: [PATCH 03/10] fix(agent): verify retained caller provenance Verify Authenticode, trusted writers, and reparse-safe ancestors through the retained process-image file object so path swaps and writable images fail closed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../src/windows/code_signing.rs | 21 +- crates/now-package-broker/src/auth.rs | 377 +++++++++++++++++- crates/now-package-broker/src/pipe.rs | 3 +- .../now-package-broker/src/policy_security.rs | 99 +++-- crates/win-api-wrappers/src/security/crypt.rs | 186 ++++++++- 5 files changed, 629 insertions(+), 57 deletions(-) diff --git a/crates/devolutions-agent-shared/src/windows/code_signing.rs b/crates/devolutions-agent-shared/src/windows/code_signing.rs index 4d58e5356..e7f1b8bd0 100644 --- a/crates/devolutions-agent-shared/src/windows/code_signing.rs +++ b/crates/devolutions-agent-shared/src/windows/code_signing.rs @@ -1,9 +1,12 @@ //! Windows code-signing validation helpers. +use std::fs::File; use std::path::Path; use anyhow::{Context as _, bail}; -use win_api_wrappers::security::crypt::{AuthenticodeSignatureStatus, authenticode_status}; +use win_api_wrappers::security::crypt::{ + AuthenticodeSignatureStatus, WinVerifyTrustResult, authenticode_status, authenticode_status_for_file, +}; /// List of allowed thumbprints for Devolutions code signing certificates. pub const DEVOLUTIONS_CERT_THUMBPRINTS: &[&str] = &[ @@ -50,7 +53,21 @@ pub fn is_devolutions_certificate_thumbprint(calculated_thumbprint: &[u8; 20]) - } pub fn validate_devolutions_authenticode_signature(path: &Path) -> anyhow::Result { - let wintrust_result = authenticode_status(path).with_context(|| { + let wintrust_result = authenticode_status(path); + validate_devolutions_authenticode_result(path, wintrust_result) +} + +/// Validate the exact retained executable object identified by `path`. +pub fn validate_devolutions_authenticode_signature_for_file(path: &Path, file: &File) -> anyhow::Result { + let wintrust_result = authenticode_status_for_file(path, file); + validate_devolutions_authenticode_result(path, wintrust_result) +} + +fn validate_devolutions_authenticode_result( + path: &Path, + wintrust_result: anyhow::Result, +) -> anyhow::Result { + let wintrust_result = wintrust_result.with_context(|| { format!( "failed to read authenticode signature for executable '{}'", path.display() diff --git a/crates/now-package-broker/src/auth.rs b/crates/now-package-broker/src/auth.rs index a748e3ec4..07fb2dc1e 100644 --- a/crates/now-package-broker/src/auth.rs +++ b/crates/now-package-broker/src/auth.rs @@ -1,4 +1,9 @@ //! Package broker pipe client authentication. +//! +//! The broker binds an approved executable's retained file object to the connector's main +//! image section, verifies its current signature and trusted-writer path, and separately +//! requires an elevated Administrators token for policy replacement. +//! These checks do not attest runtime memory integrity or historical file permissions. use std::ffi::OsString; use std::fs::{File, OpenOptions}; @@ -8,7 +13,7 @@ use std::sync::Arc; use std::time::SystemTime; use anyhow::{Context as _, bail}; -use devolutions_agent_shared::windows::code_signing::validate_devolutions_authenticode_signature; +use devolutions_agent_shared::windows::code_signing::validate_devolutions_authenticode_signature_for_file; use now_policy_api::{CancelRequest, ClientContext, PackageRequest, StatusRequest}; use tokio::net::windows::named_pipe::NamedPipeServer; use tracing::{debug, warn}; @@ -23,6 +28,8 @@ use windows::Win32::System::Threading::{ PROCESS_ACCESS_RIGHTS, PROCESS_QUERY_INFORMATION, PROCESS_QUERY_LIMITED_INFORMATION, PROCESS_VM_READ, }; +use crate::policy_security::RetainedExecutableSecurity; + const PROCESS_SYNCHRONIZE: PROCESS_ACCESS_RIGHTS = PROCESS_ACCESS_RIGHTS(0x0010_0000); const PROCESS_IDENTITY_ACCESS: PROCESS_ACCESS_RIGHTS = PROCESS_ACCESS_RIGHTS( PROCESS_QUERY_INFORMATION.0 | PROCESS_QUERY_LIMITED_INFORMATION.0 | PROCESS_VM_READ.0 | PROCESS_SYNCHRONIZE.0, @@ -41,6 +48,7 @@ pub(crate) struct PipeClient { process: Option>, executable_path: PathBuf, executable_file: Option>, + executable_security: Option>, /// Security identifier of the pipe client process token user, captured at connect. user_sid: Sid, /// Actual connected process token elevation, captured at connect. @@ -52,10 +60,12 @@ pub(crate) struct PipeClient { impl PipeClient { /// Captures the identity of the process on the other end of a connected pipe instance. /// - /// Deliberately limited to fast, local syscalls (no account-name resolution, which may - /// hit a domain controller), because it runs before any signature gate and is therefore - /// unauthenticated work a connection flood can trigger. - pub(crate) fn from_connected_pipe(server: &NamedPipeServer) -> anyhow::Result { + /// This unauthenticated capture performs blocking process and local-filesystem checks. + /// Account-name resolution remains deferred because it may contact a domain controller. + pub(crate) fn from_connected_pipe( + server: &NamedPipeServer, + skip_signature_validation: bool, + ) -> anyhow::Result { let process_id = connected_pipe_client_process_id(server).context("failed to query pipe client process id")?; let process = Arc::new( Process::get_by_pid(process_id, PROCESS_IDENTITY_ACCESS) @@ -63,7 +73,11 @@ impl PipeClient { ); Self::ensure_process_active(process_id, &process)?; let process_instance = process_instance_identity(process_id, &process)?; - let client = Self::from_process(process_instance, Arc::clone(&process))?; + let client = Self::from_process( + process_instance, + Arc::clone(&process), + !signature_validation_skipped(skip_signature_validation), + )?; let confirmed_process_id = connected_pipe_client_process_id(server).context("failed to confirm pipe client process id")?; if confirmed_process_id != process_id { @@ -92,10 +106,23 @@ impl PipeClient { Process::get_by_pid(process_id, PROCESS_IDENTITY_ACCESS) .with_context(|| format!("failed to open pipe client process {process_id}"))?, ); - Self::from_process(process_instance_identity(process_id, &process)?, process) + Self::from_process(process_instance_identity(process_id, &process)?, process, false) + } + + #[cfg(test)] + fn from_process_id_with_security(process_id: u32) -> anyhow::Result { + let process = Arc::new( + Process::get_by_pid(process_id, PROCESS_IDENTITY_ACCESS) + .with_context(|| format!("failed to open pipe client process {process_id}"))?, + ); + Self::from_process(process_instance_identity(process_id, &process)?, process, true) } - fn from_process(process_instance: ProcessInstanceIdentity, process: Arc) -> anyhow::Result { + fn from_process( + process_instance: ProcessInstanceIdentity, + process: Arc, + enforce_executable_security: bool, + ) -> anyhow::Result { let process_id = process_instance.process_id; let token = process .token(TOKEN_QUERY | TOKEN_DUPLICATE) @@ -118,6 +145,21 @@ impl PipeClient { process.verify_image_file_mapping(&executable_file).with_context(|| { format!("pipe client process {process_id} mapped executable file does not match its image") })?; + let executable_security = enforce_executable_security + .then(|| { + crate::policy_security::verify_retained_executable_security( + &executable_file, + "package broker pipe client executable", + ) + .with_context(|| { + format!( + "pipe client process {process_id} executable '{}' failed trusted-writer security validation", + executable_path.display() + ) + }) + }) + .transpose()? + .map(Arc::new); let user_sid = token .sid_and_attributes() .with_context(|| format!("failed to query pipe client process {process_id} token user"))? @@ -140,6 +182,7 @@ impl PipeClient { process: Some(process), executable_path, executable_file: Some(executable_file), + executable_security, user_sid, is_elevated, is_administrator, @@ -207,7 +250,14 @@ impl PipeClient { return Ok(()); } - let thumbprint = validate_devolutions_authenticode_signature(&self.executable_path)?; + self.executable_security + .as_ref() + .context("pipe client executable trusted-writer security guard is not retained")?; + let executable_file = self + .executable_file + .as_deref() + .context("pipe client executable handle is not retained")?; + let thumbprint = validate_devolutions_authenticode_signature_for_file(&self.executable_path, executable_file)?; debug!( process_id = self.process_id, @@ -446,6 +496,7 @@ mod tests { process: None, executable_path: PathBuf::new(), executable_file: None, + executable_security: None, user_sid: system_sid(), is_elevated: true, is_administrator: true, @@ -611,6 +662,313 @@ mod tests { } } + #[test] + fn retained_executable_handle_rejects_junction_retarget_to_signed_file() { + use win_api_wrappers::security::crypt::{ + AuthenticodeSignatureStatus, authenticode_status, authenticode_status_for_file, + }; + + let Some(windows_dir) = std::env::var_os("WINDIR") else { + return; + }; + let signed_source = PathBuf::from(windows_dir).join(r"System32\cmd.exe"); + let root = tempfile::tempdir().expect("create retarget test directory"); + let unsigned_dir = root.path().join("unsigned"); + let signed_dir = root.path().join("signed"); + std::fs::create_dir(&unsigned_dir).expect("create unsigned directory"); + std::fs::create_dir(&signed_dir).expect("create signed directory"); + let unsigned_file = unsigned_dir.join("client.exe"); + let signed_file = signed_dir.join("client.exe"); + std::fs::copy(std::env::current_exe().expect("current executable"), &unsigned_file) + .expect("copy unsigned test executable"); + std::fs::copy(signed_source, &signed_file).expect("copy signed system executable"); + + let Ok(signed_status) = authenticode_status(&signed_file) else { + return; + }; + if !matches!(signed_status.status, AuthenticodeSignatureStatus::Valid) { + return; + } + if authenticode_status(&unsigned_file) + .is_ok_and(|status| matches!(status.status, AuthenticodeSignatureStatus::Valid)) + { + return; + } + + let junction = root.path().join("client-dir"); + create_directory_junction(&junction, &unsigned_dir); + let aliased_file = junction.join("client.exe"); + let retained_unsigned = open_executable_file(&unsigned_file).expect("retain unsigned executable"); + + std::fs::remove_dir(&junction).expect("remove original junction"); + create_directory_junction(&junction, &signed_dir); + assert!( + authenticode_status(&aliased_file) + .is_ok_and(|status| matches!(status.status, AuthenticodeSignatureStatus::Valid)), + "retargeted path should resolve to the signed control file" + ); + + let retained_status = authenticode_status_for_file(&aliased_file, &retained_unsigned); + assert!( + match retained_status { + Ok(status) => !matches!(status.status, AuthenticodeSignatureStatus::Valid), + Err(_) => true, + }, + "signature verification must reject the retained unsigned object despite the retargeted signed path" + ); + + drop(retained_unsigned); + std::fs::remove_dir(&junction).expect("remove retargeted junction"); + } + + #[test] + fn process_capture_uses_the_running_image_instead_of_a_signed_path_replacement() { + let Some(windows_dir) = std::env::var_os("WINDIR") else { + return; + }; + let root = tempfile::tempdir().expect("create process image test directory"); + let launch_path = root.path().join("client.exe"); + let mapped_path = root.path().join("mapped-client.exe"); + std::fs::copy(std::env::current_exe().expect("current test executable"), &launch_path) + .expect("copy unsigned process image"); + let mut child = std::process::Command::new(&launch_path) + .args(["--exact", "auth::tests::process_reimaging_child", "--ignored"]) + .spawn() + .expect("start unsigned copied executable"); + let process = Process::get_by_pid(child.id(), PROCESS_IDENTITY_ACCESS).expect("open child process"); + let reported_launch_path = process.exe_path().expect("query reported process path"); + assert!(crate::policy_security::windows_paths_equal( + &reported_launch_path, + &launch_path + )); + + std::fs::rename(&launch_path, &mapped_path).expect("rename the running mapped executable"); + std::fs::copy(PathBuf::from(&windows_dir).join(r"System32\cmd.exe"), &launch_path) + .expect("place signed executable at cached launch path"); + + let path_status = win_api_wrappers::security::crypt::authenticode_status(&reported_launch_path) + .expect("verify signed path replacement"); + assert!(matches!( + path_status.status, + win_api_wrappers::security::crypt::AuthenticodeSignatureStatus::Valid + )); + let (_, mapped_native_path) = process + .main_image_mapped_path() + .expect("locate the running image section"); + let mapped_candidate = + open_native_executable_file(&mapped_native_path).expect("open the running image candidate"); + process + .verify_image_file_mapping(&mapped_candidate) + .expect("renamed running image must match its process"); + let signed_replacement = open_executable_file(&launch_path).expect("open signed path replacement"); + process + .verify_image_file_mapping(&signed_replacement) + .expect_err("signed replacement must not match the process image file mapping"); + let security_error = PipeClient::from_process_id_with_security(child.id()) + .expect_err("a user-writable process image must fail trusted-writer security"); + assert!( + security_error + .to_string() + .contains("trusted-writer security validation"), + "unexpected security error: {security_error:#}" + ); + + let client = PipeClient::from_process_id(child.id()).expect("capture section-backed process image identity"); + let retained_id = file_id_from_handle(client.executable_file.as_deref().expect("retained executable")) + .expect("query retained executable identity"); + + assert!(same_file( + &retained_id, + &file_id(&mapped_path).expect("query running mapped executable identity") + )); + assert!(!same_file( + &retained_id, + &file_id(&launch_path).expect("query signed replacement executable identity") + )); + let retained_status = win_api_wrappers::security::crypt::authenticode_status_for_file( + &client.executable_path, + client.executable_file.as_deref().expect("retained executable"), + ); + assert!( + match retained_status { + Ok(status) => !matches!( + status.status, + win_api_wrappers::security::crypt::AuthenticodeSignatureStatus::Valid + ), + Err(_) => true, + }, + "section-backed verification must reject the unsigned mapped image" + ); + + child.kill().expect("terminate child"); + child.wait().expect("wait for child"); + drop(client); + } + + #[test] + fn same_stream_signed_rewrite_passes_class_44_but_fails_caller_security() { + use std::ffi::c_void; + use std::io::{Seek as _, SeekFrom, Write as _}; + use std::os::windows::io::AsRawHandle as _; + + use win_api_wrappers::handle::Handle; + use windows::Win32::Foundation::{HANDLE, NTSTATUS}; + use windows::Win32::Storage::FileSystem::{FILE_SHARE_DELETE, FILE_SHARE_WRITE}; + use windows::Win32::System::Threading::{GetCurrentProcess, PROCESS_ALL_ACCESS}; + + const SECTION_ALL_ACCESS: u32 = 0x000F_001F; + const PAGE_READONLY: u32 = 0x02; + const SEC_IMAGE: u32 = 0x0100_0000; + + #[link(name = "ntdll")] + unsafe extern "system" { + fn NtCreateSection( + section_handle: *mut HANDLE, + desired_access: u32, + object_attributes: *const c_void, + maximum_size: *const i64, + section_page_protection: u32, + allocation_attributes: u32, + file_handle: HANDLE, + ) -> NTSTATUS; + fn NtCreateProcessEx( + process_handle: *mut HANDLE, + desired_access: u32, + object_attributes: *const c_void, + parent_process: HANDLE, + flags: u32, + section_handle: HANDLE, + debug_port: HANDLE, + exception_port: HANDLE, + job_member_level: u32, + ) -> NTSTATUS; + } + + let Some(windows_dir) = std::env::var_os("WINDIR") else { + return; + }; + let root = tempfile::tempdir().expect("create same-stream test directory"); + let image_path = root.path().join("client.exe"); + let signed_bytes = + std::fs::read(PathBuf::from(windows_dir).join(r"System32\cmd.exe")).expect("read signed control image"); + let mut unsigned_bytes = signed_bytes.clone(); + let dos_stub_byte = unsigned_bytes + .get_mut(0x40) + .expect("signed control image must contain a DOS stub"); + *dos_stub_byte ^= 1; + std::fs::write(&image_path, &unsigned_bytes).expect("write tampered process image"); + assert!( + !win_api_wrappers::security::crypt::authenticode_status(&image_path).is_ok_and(|status| matches!( + status.status, + win_api_wrappers::security::crypt::AuthenticodeSignatureStatus::Valid + )), + "the image used to create the process must not retain a valid signature" + ); + + let mut writer = OpenOptions::new() + .read(true) + .write(true) + .share_mode((FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE).0) + .open(&image_path) + .expect("open image stream for the herpaderping control"); + + let mut section_handle = HANDLE::default(); + // SAFETY: All optional pointers are null, `writer` supplies a live file handle, + // and the returned section handle is wrapped immediately. + unsafe { + NtCreateSection( + &mut section_handle, + SECTION_ALL_ACCESS, + std::ptr::null(), + std::ptr::null(), + PAGE_READONLY, + SEC_IMAGE, + HANDLE(writer.as_raw_handle()), + ) + } + .ok() + .expect("create image section from the unsigned stream"); + // SAFETY: NtCreateSection returned an owned section handle. + let section = unsafe { Handle::new_owned(section_handle) }.expect("retain image section"); + + let mut process_handle = HANDLE::default(); + // SAFETY: GetCurrentProcess has no preconditions and returns a pseudo handle. + let parent_process = unsafe { GetCurrentProcess() }; + // SAFETY: `section` is a live SEC_IMAGE section and the current-process pseudo + // handle is valid. The returned process handle is wrapped immediately. + unsafe { + NtCreateProcessEx( + &mut process_handle, + PROCESS_ALL_ACCESS.0, + std::ptr::null(), + parent_process, + 0, + section.raw(), + HANDLE::default(), + HANDLE::default(), + 0, + ) + } + .ok() + .expect("create process from the unsigned image section"); + // SAFETY: NtCreateProcessEx returned an owned process handle. + let process = Process::from(unsafe { Handle::new_owned(process_handle) }.expect("retain created process")); + drop(section); + + writer.seek(SeekFrom::Start(0)).expect("rewind image stream"); + writer.write_all(&signed_bytes).expect("write signed replacement bytes"); + writer.sync_all().expect("flush signed replacement bytes"); + drop(writer); + + let (_, mapped_native_path) = process + .main_image_mapped_path() + .expect("locate the created process image section"); + let candidate = open_native_executable_file(&mapped_native_path).expect("open same-stream candidate"); + process + .verify_image_file_mapping(&candidate) + .expect("class 44 must still match the same rewritten file object"); + let path_status = win_api_wrappers::security::crypt::authenticode_status_for_file(&image_path, &candidate) + .expect("verify signed rewritten stream"); + assert!(matches!( + path_status.status, + win_api_wrappers::security::crypt::AuthenticodeSignatureStatus::Valid + )); + + let error = crate::policy_security::verify_retained_executable_security( + &candidate, + "package broker pipe client executable", + ) + .expect_err("trusted-writer security must reject a user-writable rewritten image"); + let message = error.to_string(); + assert!( + message.contains("is not a trusted principal") || message.contains("DACL grants write access"), + "unexpected security error: {error:#}" + ); + } + + #[test] + #[ignore = "helper process for the process-reimaging regression"] + fn process_reimaging_child() { + std::thread::sleep(std::time::Duration::from_secs(30)); + } + + fn create_directory_junction(link: &Path, target: &Path) { + let status = std::process::Command::new("cmd") + .args(["/C", "mklink", "/J"]) + .arg(link) + .arg(target) + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status() + .expect("spawn mklink"); + assert!( + status.success(), + "failed to create junction {} -> {}", + link.display(), + target.display() + ); + } + #[cfg(not(feature = "dev-skip-broker-signature"))] mod shipping_build { use super::*; @@ -631,6 +989,7 @@ mod tests { process: None, executable_path: std::env::current_exe().expect("current test executable path"), executable_file: None, + executable_security: None, user_sid: client_user_sid(), is_elevated: false, is_administrator: false, diff --git a/crates/now-package-broker/src/pipe.rs b/crates/now-package-broker/src/pipe.rs index 1cb6d6ca7..65c2624ce 100644 --- a/crates/now-package-broker/src/pipe.rs +++ b/crates/now-package-broker/src/pipe.rs @@ -78,7 +78,8 @@ pub async fn run_pipe_server(state: Arc, shutdown: CancellationToke let serve = async move { // Capture the client identity off the accept loop so a slow // lookup cannot stall accepting other connections. - let client = match PipeClient::from_connected_pipe(&server) { + let client = + match PipeClient::from_connected_pipe(&server, state.skip_signature_validation) { Ok(client) => client, Err(error) => { warn!(%error, "Rejected named pipe client"); diff --git a/crates/now-package-broker/src/policy_security.rs b/crates/now-package-broker/src/policy_security.rs index a9f38e915..86a623504 100644 --- a/crates/now-package-broker/src/policy_security.rs +++ b/crates/now-package-broker/src/policy_security.rs @@ -282,6 +282,7 @@ pub(crate) fn security_state_digest(file: &File) -> anyhow::Result<[u8; 32]> { #[derive(Debug)] pub(crate) struct VerifiedExecutable { _file: File, + _ancestor_handles: Vec, path: PathBuf, } @@ -296,6 +297,39 @@ impl VerifiedExecutable { } } +/// Security guard for an executable file already retained by its caller. +/// +/// The caller must keep both its file handle and this guard alive. +/// The file handle binds later checks to the same object and denies new writers, while the +/// guard pins each verified ancestor against rename or reparse-point substitution. +#[derive(Debug)] +pub(crate) struct RetainedExecutableSecurity { + _ancestor_handles: Vec, + path: PathBuf, +} + +/// Verify trusted-writer security for an already-retained executable and pin its ancestors. +pub(crate) fn verify_retained_executable_security( + file: &File, + subject: &str, +) -> anyhow::Result { + let path = final_path_from_handle(file).with_context(|| format!("failed to resolve final path of {subject}"))?; + + verify_handle_security( + file, + subject, + TrustedWriters::AdminOrTrustedInstaller, + WRITE_ACCESS_MASK, + )?; + + let ancestor_handles = retain_executable_ancestor_directories(&path, subject)?; + + Ok(RetainedExecutableSecurity { + _ancestor_handles: ancestor_handles, + path, + }) +} + /// Verify that a resolved package-manager executable which will be launched with an /// elevated or machine-scope token cannot be tampered with by untrusted principals. /// @@ -346,24 +380,12 @@ pub(crate) fn verify_elevated_executable_security( .open(path) .with_context(|| format!("failed to open {subject}"))?; - // Resolve the path from the handle itself: if `path` traversed a reparse point - // (symlink, junction, ...), this yields the real target, which is the very object - // pinned by the guard handle. - let final_path = - final_path_from_handle(&file).with_context(|| format!("failed to resolve final path of {subject}"))?; - - verify_handle_security( - &file, - &subject, - TrustedWriters::AdminOrTrustedInstaller, - WRITE_ACCESS_MASK, - )?; - - verify_ancestor_directories(&final_path, &subject)?; + let security = verify_retained_executable_security(&file, &subject)?; Ok(Some(VerifiedExecutable { _file: file, - path: final_path, + _ancestor_handles: security._ancestor_handles, + path: security.path, })) } @@ -544,14 +566,45 @@ fn parse_app_exec_alias(buffer: &[u8]) -> Option { /// file when the image is finally loaded. Create rights higher up are harmless (and are /// granted to unprivileged users on stock drive roots), since they cannot redirect an /// existing path component. -fn verify_ancestor_directories(path: &Path, subject: &str) -> anyhow::Result<()> { - verify_directory_chain( - path.parent(), - subject, - TrustedWriters::AdminOrTrustedInstaller, - TrustedWriters::AdminOrTrustedInstaller, - false, - ) +fn retain_executable_ancestor_directories(path: &Path, subject: &str) -> anyhow::Result> { + let mut handles = Vec::new(); + let mut current = path.parent(); + let mut tamper_mask = PARENT_DIRECTORY_TAMPER_MASK; + + while let Some(dir) = current { + let dir_subject = format!("{subject} ancestor directory '{}'", dir.display()); + let handle = OpenOptions::new() + .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) + .open(dir) + .with_context(|| format!("failed to open {dir_subject}"))?; + + if is_reparse_point(&handle).with_context(|| format!("failed to inspect {dir_subject}"))? { + bail!("{dir_subject} is a reparse point"); + } + + let resolved = final_path_from_handle(&handle).with_context(|| format!("failed to resolve {dir_subject}"))?; + if !windows_paths_equal(&resolved, dir) { + bail!( + "{dir_subject} resolved to an unexpected location '{}'; refusing to trust a retargeted ancestor", + resolved.display() + ); + } + + verify_handle_security( + &handle, + &dir_subject, + TrustedWriters::AdminOrTrustedInstaller, + tamper_mask, + )?; + + handles.push(handle); + tamper_mask = DIRECTORY_TAMPER_MASK; + current = dir.parent(); + } + + Ok(handles) } fn verify_directory_chain( diff --git a/crates/win-api-wrappers/src/security/crypt.rs b/crates/win-api-wrappers/src/security/crypt.rs index 35ee61c59..4ef1d3171 100644 --- a/crates/win-api-wrappers/src/security/crypt.rs +++ b/crates/win-api-wrappers/src/security/crypt.rs @@ -1,9 +1,11 @@ use std::ffi::{OsString, c_void}; use std::fmt::Debug; use std::fs::File; +use std::io::{Seek as _, SeekFrom}; use std::os::windows::ffi::OsStringExt; use std::os::windows::io::AsRawHandle; use std::path::{Path, PathBuf}; +use std::rc::Rc; use anyhow::{Result, anyhow, bail}; use windows::Win32::Foundation::{ @@ -33,33 +35,82 @@ use crate::utils::{SafeWindowsString, WideString, nul_slice_wide_str, slice_from pub struct CatalogInfo { pub path: PathBuf, pub hash: Vec, + pub admin_context: Rc, } impl CatalogInfo { pub fn try_from_file(path: &Path) -> Result> { - let admin_ctx = CatalogAdminContext::try_new()?; + let file = File::open(path)?; + Self::try_from_file_handle(&file) + } + + /// Resolve catalog metadata from the exact retained file object. + pub fn try_from_file_handle(file: &File) -> Result> { + let admin_context = Rc::new(CatalogAdminContext::try_new()?); - let hash = admin_ctx.hash_file(path)?; + let hash = admin_context.hash_file_handle(file)?; - let catalog_path = admin_ctx.catalogs_for_hash(&hash).next(); + let catalog_path = { + let mut catalogs = admin_context.catalogs_for_hash(&hash); + catalogs.next() + }; Ok(catalog_path.map(|catalog_path| Self { hash, path: catalog_path, + admin_context, })) } } +fn wintrust_catalog_info( + catalog_info: &CatalogInfo, + catalog_path: &WideString, + member_path: &WideString, + member_tag: &WideString, + file: &File, +) -> WINTRUST_CATALOG_INFO { + WINTRUST_CATALOG_INFO { + cbStruct: u32size_of::(), + pcwszCatalogFilePath: catalog_path.as_pcwstr(), + pcwszMemberFilePath: member_path.as_pcwstr(), + pcwszMemberTag: member_tag.as_pcwstr(), + hMemberFile: HANDLE(file.as_raw_handle().cast()), + hCatAdmin: catalog_info.admin_context.handle.0 as isize, + ..Default::default() + } +} + +fn wintrust_file_info(path: &WideString, file: &File) -> WINTRUST_FILE_INFO { + WINTRUST_FILE_INFO { + cbStruct: u32size_of::(), + pcwszFilePath: path.as_pcwstr(), + hFile: HANDLE(file.as_raw_handle().cast()), + ..Default::default() + } +} + /// https://learn.microsoft.com/en-us/windows/win32/seccrypto/example-c-program--verifying-the-signature-of-a-pe-file /// https://stackoverflow.com/questions/68215779/getting-winverifytrust-to-work-with-catalog-signed-files-such-as-cmd-exe /// https://github.com/dragokas/Verify-Signature-Cpp/blob/master/verify.cpp#L140 /// https://github.com/microsoft/Windows-classic-samples/blob/main/Samples/Security/CodeSigning/cpp/codesigning.cpp pub fn win_verify_trust(path: &Path, catalog_info: Option) -> Result { + let file = File::open(path)?; + win_verify_trust_for_file(path, &file, catalog_info) +} + +/// Verify the exact retained file object; `path` must identify that object and is retained +/// as WinTrust subject metadata. +pub fn win_verify_trust_for_file( + path: &Path, + file: &File, + catalog_info: Option, +) -> Result { let path = WideString::from(path); - let catalog_info = catalog_info.map(|c| { + let catalog_strings = catalog_info.as_ref().map(|catalog_info| { ( - WideString::from(&c.path), - WideString::from(base16ct::upper::encode_string(&c.hash)), + WideString::from(&catalog_info.path), + WideString::from(base16ct::upper::encode_string(&catalog_info.hash)), ) }); @@ -68,19 +119,16 @@ pub fn win_verify_trust(path: &Path, catalog_info: Option) -> Resul File(WINTRUST_FILE_INFO), } - let mut wintrust_info = match &catalog_info { - Some((catalog_info_path, catalog_info_member)) => WintrustInfo::Catalog(WINTRUST_CATALOG_INFO { - cbStruct: u32size_of::(), - pcwszCatalogFilePath: catalog_info_path.as_pcwstr(), - pcwszMemberFilePath: path.as_pcwstr(), - pcwszMemberTag: catalog_info_member.as_pcwstr(), - ..Default::default() - }), - None => WintrustInfo::File(WINTRUST_FILE_INFO { - cbStruct: u32size_of::(), - pcwszFilePath: path.as_pcwstr(), - ..Default::default() - }), + let mut wintrust_info = match (&catalog_info, &catalog_strings) { + (Some(catalog_info), Some((catalog_path, member_tag))) => WintrustInfo::Catalog(wintrust_catalog_info( + catalog_info, + catalog_path, + &path, + member_tag, + file, + )), + (None, None) => WintrustInfo::File(wintrust_file_info(&path, file)), + _ => unreachable!("catalog info and derived strings are created together"), }; let mut win_trust_data = WINTRUST_DATA { @@ -135,9 +183,14 @@ pub struct WinVerifyTrustResult { } pub fn authenticode_status(path: &Path) -> Result { - let catalog_info = CatalogInfo::try_from_file(path)?; + let file = File::open(path)?; + authenticode_status_for_file(path, &file) +} - win_verify_trust(path, catalog_info) +/// Read Authenticode status from the exact retained file object identified by `path`. +pub fn authenticode_status_for_file(path: &Path, file: &File) -> Result { + let catalog_info = CatalogInfo::try_from_file_handle(file)?; + win_verify_trust_for_file(path, file, catalog_info) } pub struct CatalogAdminContext { @@ -164,11 +217,18 @@ impl CatalogAdminContext { } pub fn hash_file(&self, path: &Path) -> Result> { + let file = File::open(path)?; + self.hash_file_handle(&file) + } + + /// Hash the exact retained file object and reset its cursor to offset zero. + pub fn hash_file_handle(&self, file: &File) -> Result> { // The output has a variable size. // Therefore, we must call CryptCATAdminCalcHashFromFileHandle2 once with a zero-size, and check for the ERROR_INSUFFICIENT_BUFFER status. // At this point, we call CryptCATAdminCalcHashFromFileHandle2 again with a buffer of the correct size. - let file = File::open(path)?; + let mut cursor = file; + cursor.seek(SeekFrom::Start(0))?; let mut required_size = 0u32; // SAFETY: `hFile` must not be NULL and must be a valid file pointer. The `file` is not dropped so it should be valid. @@ -200,6 +260,7 @@ impl CatalogAdminContext { debug_assert_eq!(allocated_length, required_size); hash.truncate(required_size as usize); + cursor.seek(SeekFrom::Start(0))?; Ok(hash) } @@ -216,6 +277,87 @@ impl Drop for CatalogAdminContext { } } +#[cfg(test)] +mod tests { + use std::io::{Seek as _, SeekFrom}; + + use super::*; + + #[test] + #[cfg_attr(miri, ignore)] + fn catalog_hash_uses_retained_handle_and_resets_position() { + let executable = std::env::current_exe().expect("current executable"); + let mut file = File::open(executable).expect("open current executable"); + file.seek(SeekFrom::Start(17)).expect("move retained handle position"); + let context = CatalogAdminContext::try_new().expect("create catalog context"); + + let hash = context + .hash_file_handle(&file) + .expect("hash retained executable handle"); + + assert!(!hash.is_empty()); + assert_eq!(file.stream_position().expect("query retained handle position"), 0); + } + + #[test] + #[cfg_attr(miri, ignore)] + fn catalog_wintrust_info_carries_live_sha256_context_and_member_handle() { + let executable = std::env::current_exe().expect("current executable"); + let file = File::open(&executable).expect("open current executable"); + let admin_context = Rc::new(CatalogAdminContext::try_new().expect("create SHA-256 catalog context")); + let catalog_info = CatalogInfo { + path: PathBuf::from(r"C:\test\catalog.cat"), + hash: vec![0xAB; 32], + admin_context, + }; + let catalog_path = WideString::from(&catalog_info.path); + let member_path = WideString::from(&executable); + let member_tag = WideString::from(base16ct::upper::encode_string(&catalog_info.hash)); + + let native = wintrust_catalog_info(&catalog_info, &catalog_path, &member_path, &member_tag, &file); + + assert_eq!(native.hCatAdmin, catalog_info.admin_context.handle.0 as isize); + assert_eq!(native.hMemberFile, HANDLE(file.as_raw_handle().cast())); + } + + #[test] + #[cfg_attr(miri, ignore)] + fn embedded_wintrust_info_carries_retained_file_handle() { + let executable = std::env::current_exe().expect("current executable"); + let file = File::open(&executable).expect("open current executable"); + let path = WideString::from(&executable); + + let native = wintrust_file_info(&path, &file); + + assert_eq!(native.hFile, HANDLE(file.as_raw_handle().cast())); + } + + #[test] + fn catalog_backed_signature_uses_retained_context_when_available() { + let Some(windows_dir) = std::env::var_os("WINDIR") else { + return; + }; + let candidates = [ + PathBuf::from(&windows_dir).join(r"System32\cmd.exe"), + PathBuf::from(&windows_dir).join(r"System32\WindowsPowerShell\v1.0\powershell.exe"), + PathBuf::from(&windows_dir).join(r"System32\drivers\acpi.sys"), + ]; + + for path in candidates { + let Ok(file) = File::open(&path) else { + continue; + }; + let Ok(Some(catalog_info)) = CatalogInfo::try_from_file_handle(&file) else { + continue; + }; + let result = + win_verify_trust_for_file(&path, &file, Some(catalog_info)).expect("verify catalog-backed system file"); + assert!(matches!(result.status, AuthenticodeSignatureStatus::Valid)); + return; + } + } +} + pub struct CatalogIterator<'a> { admin_ctx: &'a CatalogAdminContext, cur: Option, From eedbb26a597258ff1018f76ec4d919be8f4e1e51 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Mon, 7 Sep 2026 19:03:14 -0400 Subject: [PATCH 04/10] fix(agent): bound caller identity capture Run blocking caller-image checks outside the accept loop while retaining the connection permit until abandoned work actually completes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- crates/now-package-broker/src/pipe.rs | 91 +++++++++++++++++++++++---- 1 file changed, 78 insertions(+), 13 deletions(-) diff --git a/crates/now-package-broker/src/pipe.rs b/crates/now-package-broker/src/pipe.rs index 65c2624ce..a8d46f824 100644 --- a/crates/now-package-broker/src/pipe.rs +++ b/crates/now-package-broker/src/pipe.rs @@ -7,7 +7,8 @@ use std::sync::Arc; use anyhow::Context as _; use tokio::net::windows::named_pipe::{NamedPipeServer, ServerOptions}; -use tokio::sync::Semaphore; +use tokio::sync::{OwnedSemaphorePermit, Semaphore}; +use tokio::task::JoinHandle; use tokio_util::sync::CancellationToken; use tracing::{error, info, warn}; use win_api_wrappers::identity::sid::Sid; @@ -72,20 +73,26 @@ pub async fn run_pipe_server(state: Arc, shutdown: CancellationToke Ok(()) => { let state = Arc::clone(&state); tokio::spawn(async move { - // The permit is held for the lifetime of the connection task. - let _permit = permit; - let serve = async move { - // Capture the client identity off the accept loop so a slow - // lookup cannot stall accepting other connections. - let client = - match PipeClient::from_connected_pipe(&server, state.skip_signature_validation) { - Ok(client) => client, - Err(error) => { - warn!(%error, "Rejected named pipe client"); - return; - } + // Keep blocking unauthenticated capture off the accept loop and + // retain the connection slot until the work actually completes. + let skip_signature_validation = state.skip_signature_validation; + let capture = spawn_bounded_capture(permit, move || { + let client = PipeClient::from_connected_pipe(&server, skip_signature_validation); + (server, client) + }); + let (_permit, server, client) = match capture.await { + Ok((permit, (server, Ok(client)))) => (permit, server, client), + Ok((_permit, (_server, Err(error)))) => { + warn!(%error, "Rejected named pipe client"); + return; + } + Err(error) => { + error!(%error, "Named pipe client identity capture task failed"); + return; + } }; + info!("Client connected to named pipe"); let router = build_router_for_client(state, client); serve_connection(server, router).await; @@ -112,6 +119,14 @@ pub async fn run_pipe_server(state: Arc, shutdown: CancellationToke } } +fn spawn_bounded_capture(permit: OwnedSemaphorePermit, capture: F) -> JoinHandle<(OwnedSemaphorePermit, T)> +where + T: Send + 'static, + F: FnOnce() -> T + Send + 'static, +{ + tokio::task::spawn_blocking(move || (permit, capture())) +} + fn create_pipe_instance(pipe_name: &str, first_instance: bool) -> anyhow::Result { let security_attributes = build_pipe_security_attributes().context("failed to build pipe security attributes")?; @@ -175,3 +190,53 @@ fn build_pipe_security_attributes() -> anyhow::Result Date: Mon, 7 Sep 2026 19:06:35 -0400 Subject: [PATCH 05/10] test(agent): align caller security coverage Keep test-only process helpers out of production builds and accept expected fail-closed outcomes from locally modified WindowsApps security. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- crates/now-package-broker/src/auth.rs | 1 + crates/now-package-broker/src/policy_security.rs | 15 ++++++++++----- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/crates/now-package-broker/src/auth.rs b/crates/now-package-broker/src/auth.rs index 07fb2dc1e..16d4b4d9b 100644 --- a/crates/now-package-broker/src/auth.rs +++ b/crates/now-package-broker/src/auth.rs @@ -101,6 +101,7 @@ impl PipeClient { } } + #[cfg(test)] fn from_process_id(process_id: u32) -> anyhow::Result { let process = Arc::new( Process::get_by_pid(process_id, PROCESS_IDENTITY_ACCESS) diff --git a/crates/now-package-broker/src/policy_security.rs b/crates/now-package-broker/src/policy_security.rs index 86a623504..3693dc32c 100644 --- a/crates/now-package-broker/src/policy_security.rs +++ b/crates/now-package-broker/src/policy_security.rs @@ -1170,9 +1170,10 @@ mod tests { } #[test] - fn winget_app_exec_alias_passes_elevated_verification() { - // Opportunistic end-to-end check: the alias itself cannot be opened for read, - // so verification must transparently target the real WindowsApps binary. + fn winget_app_exec_alias_uses_resolved_target_security() { + // Opportunistic end-to-end check: the alias itself cannot be opened for read, so + // verification must transparently target the real WindowsApps binary. A locally + // modified WindowsApps ACL is expected to fail the same strict security check. let Some(local_app_data) = std::env::var_os("LOCALAPPDATA") else { return; }; @@ -1188,10 +1189,14 @@ mod tests { } // Non-elevated test runs cannot open `Program Files\WindowsApps` ancestors for // READ_CONTROL; the agent service (SYSTEM) can. Everything up to the ancestor - // walk — alias resolution and file-level verification — must have succeeded. + // walk must have succeeded unless the resolved target itself has an insecure + // owner or write grant. Err(error) => { + let message = error.to_string(); assert!( - error.to_string().contains("ancestor directory"), + message.contains("ancestor directory") + || message.contains("owner") + || message.contains("DACL grants write access"), "unexpected error: {error:#}" ); } From 888ad887827aa93de8e22c150f6c873e67f5e7ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Mon, 7 Sep 2026 19:30:08 -0400 Subject: [PATCH 06/10] docs(agent): clarify caller identity guarantees State which handles establish identity, how capture is bounded, and which runtime and historical guarantees remain outside this layer. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../src/windows/code_signing.rs | 2 +- crates/now-package-broker/src/auth.rs | 13 +++++++++---- .../now-package-broker/src/policy_security.rs | 9 ++++++--- crates/win-api-wrappers/src/process.rs | 18 +++++++++--------- crates/win-api-wrappers/src/security/crypt.rs | 6 +++--- 5 files changed, 28 insertions(+), 20 deletions(-) diff --git a/crates/devolutions-agent-shared/src/windows/code_signing.rs b/crates/devolutions-agent-shared/src/windows/code_signing.rs index e7f1b8bd0..d55443de2 100644 --- a/crates/devolutions-agent-shared/src/windows/code_signing.rs +++ b/crates/devolutions-agent-shared/src/windows/code_signing.rs @@ -57,7 +57,7 @@ pub fn validate_devolutions_authenticode_signature(path: &Path) -> anyhow::Resul validate_devolutions_authenticode_result(path, wintrust_result) } -/// Validate the exact retained executable object identified by `path`. +/// Validate the signature of `file`; `path` only supplies subject metadata and error context. pub fn validate_devolutions_authenticode_signature_for_file(path: &Path, file: &File) -> anyhow::Result { let wintrust_result = authenticode_status_for_file(path, file); validate_devolutions_authenticode_result(path, wintrust_result) diff --git a/crates/now-package-broker/src/auth.rs b/crates/now-package-broker/src/auth.rs index 16d4b4d9b..a76a40a19 100644 --- a/crates/now-package-broker/src/auth.rs +++ b/crates/now-package-broker/src/auth.rs @@ -1,8 +1,8 @@ //! Package broker pipe client authentication. //! -//! The broker binds an approved executable's retained file object to the connector's main -//! image section, verifies its current signature and trusted-writer path, and separately -//! requires an elevated Administrators token for policy replacement. +//! The broker pins the pipe client process instance to the retained file object backing its +//! main image, then verifies that object's current signature and trusted-writer security. +//! Policy replacement additionally requires an elevated Administrators token. //! These checks do not attest runtime memory integrity or historical file permissions. use std::ffi::OsString; @@ -60,7 +60,10 @@ pub(crate) struct PipeClient { impl PipeClient { /// Captures the identity of the process on the other end of a connected pipe instance. /// - /// This unauthenticated capture performs blocking process and local-filesystem checks. + /// Retains the process and main-image file handles, then rechecks the client process ID + /// and process instance so a PID reused during capture is rejected. + /// This unauthenticated capture blocks on process and local-filesystem work, so callers + /// must keep it off the accept loop and bound it with a connection permit. /// Account-name resolution remains deferred because it may contact a domain controller. pub(crate) fn from_connected_pipe( server: &NamedPipeServer, @@ -413,6 +416,8 @@ fn open_native_executable_file(native_path: &Path) -> anyhow::Result { open_executable_file(Path::new(&global_root_path)) } +/// Accept only local volume devices because remote providers cannot satisfy local +/// trusted-writer and ancestor-pinning guarantees. fn is_supported_local_image_path(path: &Path) -> bool { let path = path.as_os_str().to_string_lossy().to_ascii_lowercase(); path.starts_with(r"\device\harddiskvolume") || path.starts_with(r"\device\volume{") diff --git a/crates/now-package-broker/src/policy_security.rs b/crates/now-package-broker/src/policy_security.rs index 3693dc32c..2e3e056d5 100644 --- a/crates/now-package-broker/src/policy_security.rs +++ b/crates/now-package-broker/src/policy_security.rs @@ -299,9 +299,10 @@ impl VerifiedExecutable { /// Security guard for an executable file already retained by its caller. /// -/// The caller must keep both its file handle and this guard alive. -/// The file handle binds later checks to the same object and denies new writers, while the -/// guard pins each verified ancestor against rename or reparse-point substitution. +/// The caller must open the file without write or delete sharing, then keep both that handle +/// and this guard alive. +/// The handle binds later checks to the same object and blocks new writers, while the guard +/// pins each verified ancestor against rename or reparse-point substitution. #[derive(Debug)] pub(crate) struct RetainedExecutableSecurity { _ancestor_handles: Vec, @@ -566,6 +567,8 @@ fn parse_app_exec_alias(buffer: &[u8]) -> Option { /// file when the image is finally loaded. Create rights higher up are harmless (and are /// granted to unprivileged users on stock drive roots), since they cannot redirect an /// existing path component. +/// 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> { let mut handles = Vec::new(); let mut current = path.parent(); diff --git a/crates/win-api-wrappers/src/process.rs b/crates/win-api-wrappers/src/process.rs index 9ea36da84..9696edb80 100644 --- a/crates/win-api-wrappers/src/process.rs +++ b/crates/win-api-wrappers/src/process.rs @@ -111,10 +111,11 @@ impl Process { Ok(OsString::from_wide(&path).into()) } - /// Returns an address and candidate native path for the process's main image. + /// Returns the main image's transfer address and a candidate native path for it. /// - /// `ProcessImageInformation` comes from the kernel image section rather than the - /// target-controlled PEB or loader module list. + /// The address comes from `ProcessImageInformation`, which reports the kernel image + /// section rather than the target-controlled PEB or loader module list. + /// The path identifies the file currently mapped at that address. /// Callers must confirm the opened candidate with [`Process::verify_image_file_mapping`]. pub fn main_image_mapped_path(&self) -> Result<(usize, PathBuf)> { let mut image = SECTION_IMAGE_INFORMATION::default(); @@ -157,13 +158,12 @@ impl Process { } } - /// Verifies that `file` shares the section-object pointer used by this process's main image. + /// Verifies that `file` is the same kernel file object backing this process's main image. /// - /// `ProcessImageFileMapping` compares the kernel file objects' section pointers. - /// It identifies the backing file object but does not attest the bytes originally mapped - /// into the image section. - /// The file handle is an input buffer despite `NtQueryInformationProcess`'s generic output-buffer signature. - /// The handle must include `SYNCHRONIZE | FILE_EXECUTE` access. + /// `ProcessImageFileMapping` compares section-object pointers, but does not attest the + /// bytes originally mapped into the image section. + /// The handle is an input despite `NtQueryInformationProcess`'s generic output-buffer + /// signature, and must grant `SYNCHRONIZE | FILE_EXECUTE`. pub fn verify_image_file_mapping(&self, file: &File) -> Result<()> { let mut file_handle = HANDLE(file.as_raw_handle()); diff --git a/crates/win-api-wrappers/src/security/crypt.rs b/crates/win-api-wrappers/src/security/crypt.rs index 4ef1d3171..50973b169 100644 --- a/crates/win-api-wrappers/src/security/crypt.rs +++ b/crates/win-api-wrappers/src/security/crypt.rs @@ -99,8 +99,8 @@ pub fn win_verify_trust(path: &Path, catalog_info: Option) -> Resul win_verify_trust_for_file(path, &file, catalog_info) } -/// Verify the exact retained file object; `path` must identify that object and is retained -/// as WinTrust subject metadata. +/// Verify `file` itself. +/// `path` is passed to WinTrust as subject metadata and must not be relied on for identity. pub fn win_verify_trust_for_file( path: &Path, file: &File, @@ -187,7 +187,7 @@ pub fn authenticode_status(path: &Path) -> Result { authenticode_status_for_file(path, &file) } -/// Read Authenticode status from the exact retained file object identified by `path`. +/// Read the Authenticode status of `file`; `path` only supplies WinTrust subject metadata. pub fn authenticode_status_for_file(path: &Path, file: &File) -> Result { let catalog_info = CatalogInfo::try_from_file_handle(file)?; win_verify_trust_for_file(path, file, catalog_info) From c9542571e047992a0b52a01c5eea3d9e69458d52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Mon, 7 Sep 2026 19:34:03 -0400 Subject: [PATCH 07/10] fix(agent): preserve executable path compatibility Keep existing package-manager support for secured reparse-backed layouts while applying stricter retained ancestor pinning only to broker callers. Bind the WinGet regression to the resolved target and document the residual pre-capture identity interval. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- crates/now-package-broker/src/auth.rs | 10 +++-- .../now-package-broker/src/policy_security.rs | 41 ++++++++++++++----- 2 files changed, 36 insertions(+), 15 deletions(-) diff --git a/crates/now-package-broker/src/auth.rs b/crates/now-package-broker/src/auth.rs index a76a40a19..f123f8de5 100644 --- a/crates/now-package-broker/src/auth.rs +++ b/crates/now-package-broker/src/auth.rs @@ -1,9 +1,10 @@ //! Package broker pipe client authentication. //! -//! The broker pins the pipe client process instance to the retained file object backing its -//! main image, then verifies that object's current signature and trusted-writer security. +//! After opening the pipe-reported process ID, the broker retains that process and the file +//! object backing its main image, then verifies the file's signature and trusted-writer security. //! Policy replacement additionally requires an elevated Administrators token. //! These checks do not attest runtime memory integrity or historical file permissions. +//! They also cannot prove which process instance owned the pipe before the initial process open. use std::ffi::OsString; use std::fs::{File, OpenOptions}; @@ -60,8 +61,9 @@ pub(crate) struct PipeClient { impl PipeClient { /// Captures the identity of the process on the other end of a connected pipe instance. /// - /// Retains the process and main-image file handles, then rechecks the client process ID - /// and process instance so a PID reused during capture is rejected. + /// Retains the process and main-image file handles, then rechecks the reported process ID, + /// retained process liveness, and creation time during capture. + /// This cannot close the interval before the initial process handle is opened. /// This unauthenticated capture blocks on process and local-filesystem work, so callers /// must keep it off the accept loop and bound it with a connection permit. /// Account-name resolution remains deferred because it may contact a domain controller. diff --git a/crates/now-package-broker/src/policy_security.rs b/crates/now-package-broker/src/policy_security.rs index 2e3e056d5..73441e0d7 100644 --- a/crates/now-package-broker/src/policy_security.rs +++ b/crates/now-package-broker/src/policy_security.rs @@ -282,7 +282,6 @@ pub(crate) fn security_state_digest(file: &File) -> anyhow::Result<[u8; 32]> { #[derive(Debug)] pub(crate) struct VerifiedExecutable { _file: File, - _ancestor_handles: Vec, path: PathBuf, } @@ -306,7 +305,6 @@ impl VerifiedExecutable { #[derive(Debug)] pub(crate) struct RetainedExecutableSecurity { _ancestor_handles: Vec, - path: PathBuf, } /// Verify trusted-writer security for an already-retained executable and pin its ancestors. @@ -327,7 +325,6 @@ pub(crate) fn verify_retained_executable_security( Ok(RetainedExecutableSecurity { _ancestor_handles: ancestor_handles, - path, }) } @@ -356,8 +353,6 @@ pub(crate) fn verify_elevated_executable_security( return Ok(None); } - let subject = format!("elevated package-manager executable '{}'", path.display()); - // App execution aliases (Microsoft Store shims such as the per-user `winget.exe`) // are reparse points that cannot be opened for read, so they cannot be verified or // pinned directly. `CreateProcess` resolves them internally, but the broker must @@ -372,6 +367,7 @@ pub(crate) fn verify_elevated_executable_security( None => None, }; let path = alias_target.as_deref().unwrap_or(path); + let subject = format!("elevated package-manager executable '{}'", path.display()); // Share only read access: while this handle is alive the file cannot be opened for // write or delete (rename), and this open fails if such a handle already exists. @@ -381,12 +377,32 @@ pub(crate) fn verify_elevated_executable_security( .open(path) .with_context(|| format!("failed to open {subject}"))?; - let security = verify_retained_executable_security(&file, &subject)?; + // Resolve the path from the handle itself: if `path` traversed a reparse point + // (symlink, junction, ...), this yields the real target, which is the very object + // pinned by the guard handle. + let final_path = + final_path_from_handle(&file).with_context(|| format!("failed to resolve final path of {subject}"))?; + + verify_handle_security( + &file, + &subject, + TrustedWriters::AdminOrTrustedInstaller, + WRITE_ACCESS_MASK, + )?; + + // Preserve compatibility for existing package-manager installations under secured + // junctions or mount points. The stricter caller guard below rejects and pins reparses. + verify_directory_chain( + final_path.parent(), + &subject, + TrustedWriters::AdminOrTrustedInstaller, + TrustedWriters::AdminOrTrustedInstaller, + false, + )?; Ok(Some(VerifiedExecutable { _file: file, - _ancestor_handles: security._ancestor_handles, - path: security.path, + path: final_path, })) } @@ -1184,6 +1200,8 @@ mod tests { if !alias.exists() { return; } + let resolved = resolve_app_exec_alias(&alias).expect("winget alias must resolve"); + let resolved_target = resolved.target.display().to_string(); match verify_elevated_executable_security(&alias, true) { Ok(guard) => { @@ -1197,9 +1215,10 @@ mod tests { Err(error) => { let message = error.to_string(); assert!( - message.contains("ancestor directory") - || message.contains("owner") - || message.contains("DACL grants write access"), + message.contains(&resolved_target) + && (message.contains("ancestor directory") + || message.contains("owner") + || message.contains("DACL grants write access")), "unexpected error: {error:#}" ); } From 0ced18694236b3ad4148a688ebb08043639b12d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Mon, 7 Sep 2026 19:40:34 -0400 Subject: [PATCH 08/10] fix(agent): isolate caller provenance checks Keep package-manager path compatibility and existing path-based signature behavior while applying retained handles, pinned ancestors, and full diagnostics only to broker callers. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- crates/now-package-broker/src/pipe.rs | 4 +- crates/win-api-wrappers/src/security/crypt.rs | 113 +++++++++++++----- 2 files changed, 82 insertions(+), 35 deletions(-) diff --git a/crates/now-package-broker/src/pipe.rs b/crates/now-package-broker/src/pipe.rs index a8d46f824..8cbab17b8 100644 --- a/crates/now-package-broker/src/pipe.rs +++ b/crates/now-package-broker/src/pipe.rs @@ -84,11 +84,11 @@ pub async fn run_pipe_server(state: Arc, shutdown: CancellationToke let (_permit, server, client) = match capture.await { Ok((permit, (server, Ok(client)))) => (permit, server, client), Ok((_permit, (_server, Err(error)))) => { - warn!(%error, "Rejected named pipe client"); + warn!(error = format!("{error:#}"), "Rejected named pipe client"); return; } Err(error) => { - error!(%error, "Named pipe client identity capture task failed"); + error!(error = format!("{error:#}"), "Named pipe client identity capture task failed"); return; } }; diff --git a/crates/win-api-wrappers/src/security/crypt.rs b/crates/win-api-wrappers/src/security/crypt.rs index 50973b169..e423a7282 100644 --- a/crates/win-api-wrappers/src/security/crypt.rs +++ b/crates/win-api-wrappers/src/security/crypt.rs @@ -5,7 +5,6 @@ use std::io::{Seek as _, SeekFrom}; use std::os::windows::ffi::OsStringExt; use std::os::windows::io::AsRawHandle; use std::path::{Path, PathBuf}; -use std::rc::Rc; use anyhow::{Result, anyhow, bail}; use windows::Win32::Foundation::{ @@ -23,9 +22,10 @@ use windows::Win32::Security::Cryptography::{ }; use windows::Win32::Security::WinTrust::{ CRYPT_PROVIDER_CERT, CRYPT_PROVIDER_DATA, CRYPT_PROVIDER_SGNR, WINTRUST_ACTION_GENERIC_VERIFY_V2, - WINTRUST_CATALOG_INFO, WINTRUST_DATA, WINTRUST_DATA_0, WINTRUST_FILE_INFO, WTD_CACHE_ONLY_URL_RETRIEVAL, - WTD_CHOICE_CATALOG, WTD_CHOICE_FILE, WTD_DISABLE_MD2_MD4, WTD_REVOKE_WHOLECHAIN, WTD_STATEACTION_CLOSE, - WTD_STATEACTION_VERIFY, WTD_UI_NONE, WTD_USE_DEFAULT_OSVER_CHECK, WTHelperProvDataFromStateData, WinVerifyTrustEx, + WINTRUST_CATALOG_INFO, WINTRUST_DATA, WINTRUST_DATA_0, WINTRUST_DATA_UNION_CHOICE, WINTRUST_FILE_INFO, + WTD_CACHE_ONLY_URL_RETRIEVAL, WTD_CHOICE_CATALOG, WTD_CHOICE_FILE, WTD_DISABLE_MD2_MD4, WTD_REVOKE_WHOLECHAIN, + WTD_STATEACTION_CLOSE, WTD_STATEACTION_VERIFY, WTD_UI_NONE, WTD_USE_DEFAULT_OSVER_CHECK, + WTHelperProvDataFromStateData, WinVerifyTrustEx, }; use windows::core::HRESULT; @@ -35,21 +35,31 @@ use crate::utils::{SafeWindowsString, WideString, nul_slice_wide_str, slice_from pub struct CatalogInfo { pub path: PathBuf, pub hash: Vec, - pub admin_context: Rc, } impl CatalogInfo { pub fn try_from_file(path: &Path) -> Result> { - let file = File::open(path)?; - Self::try_from_file_handle(&file) + let admin_context = CatalogAdminContext::try_new()?; + let hash = admin_context.hash_file(path)?; + let catalog_path = admin_context.catalogs_for_hash(&hash).next(); + + Ok(catalog_path.map(|catalog_path| Self { + hash, + path: catalog_path, + })) } +} - /// Resolve catalog metadata from the exact retained file object. - pub fn try_from_file_handle(file: &File) -> Result> { - let admin_context = Rc::new(CatalogAdminContext::try_new()?); +struct RetainedCatalogInfo { + path: PathBuf, + hash: Vec, + admin_context: CatalogAdminContext, +} +impl RetainedCatalogInfo { + fn try_from_file(file: &File) -> Result> { + let admin_context = CatalogAdminContext::try_new()?; let hash = admin_context.hash_file_handle(file)?; - let catalog_path = { let mut catalogs = admin_context.catalogs_for_hash(&hash); catalogs.next() @@ -64,7 +74,7 @@ impl CatalogInfo { } fn wintrust_catalog_info( - catalog_info: &CatalogInfo, + catalog_info: &RetainedCatalogInfo, catalog_path: &WideString, member_path: &WideString, member_tag: &WideString, @@ -95,16 +105,52 @@ fn wintrust_file_info(path: &WideString, file: &File) -> WINTRUST_FILE_INFO { /// https://github.com/dragokas/Verify-Signature-Cpp/blob/master/verify.cpp#L140 /// https://github.com/microsoft/Windows-classic-samples/blob/main/Samples/Security/CodeSigning/cpp/codesigning.cpp pub fn win_verify_trust(path: &Path, catalog_info: Option) -> Result { - let file = File::open(path)?; - win_verify_trust_for_file(path, &file, catalog_info) + let path = WideString::from(path); + let catalog_info = catalog_info.map(|catalog| { + ( + WideString::from(&catalog.path), + WideString::from(base16ct::upper::encode_string(&catalog.hash)), + ) + }); + + enum WintrustInfo { + Catalog(WINTRUST_CATALOG_INFO), + File(WINTRUST_FILE_INFO), + } + + let mut wintrust_info = match &catalog_info { + Some((catalog_path, member_tag)) => WintrustInfo::Catalog(WINTRUST_CATALOG_INFO { + cbStruct: u32size_of::(), + pcwszCatalogFilePath: catalog_path.as_pcwstr(), + pcwszMemberFilePath: path.as_pcwstr(), + pcwszMemberTag: member_tag.as_pcwstr(), + ..Default::default() + }), + None => WintrustInfo::File(WINTRUST_FILE_INFO { + cbStruct: u32size_of::(), + pcwszFilePath: path.as_pcwstr(), + ..Default::default() + }), + }; + + let (choice, data) = match &mut wintrust_info { + WintrustInfo::Catalog(info) => (WTD_CHOICE_CATALOG, WINTRUST_DATA_0 { pCatalog: info }), + WintrustInfo::File(info) => (WTD_CHOICE_FILE, WINTRUST_DATA_0 { pFile: info }), + }; + run_win_verify_trust(choice, data) } /// Verify `file` itself. /// `path` is passed to WinTrust as subject metadata and must not be relied on for identity. -pub fn win_verify_trust_for_file( +pub fn win_verify_trust_for_file(path: &Path, file: &File) -> Result { + let catalog_info = RetainedCatalogInfo::try_from_file(file)?; + win_verify_trust_for_file_with_catalog(path, file, catalog_info) +} + +fn win_verify_trust_for_file_with_catalog( path: &Path, file: &File, - catalog_info: Option, + catalog_info: Option, ) -> Result { let path = WideString::from(path); let catalog_strings = catalog_info.as_ref().map(|catalog_info| { @@ -131,19 +177,21 @@ pub fn win_verify_trust_for_file( _ => unreachable!("catalog info and derived strings are created together"), }; + let (choice, data) = match &mut wintrust_info { + WintrustInfo::Catalog(info) => (WTD_CHOICE_CATALOG, WINTRUST_DATA_0 { pCatalog: info }), + WintrustInfo::File(info) => (WTD_CHOICE_FILE, WINTRUST_DATA_0 { pFile: info }), + }; + run_win_verify_trust(choice, data) +} + +fn run_win_verify_trust(choice: WINTRUST_DATA_UNION_CHOICE, data: WINTRUST_DATA_0) -> Result { let mut win_trust_data = WINTRUST_DATA { cbStruct: u32size_of::(), dwUIChoice: WTD_UI_NONE, fdwRevocationChecks: WTD_REVOKE_WHOLECHAIN, - dwUnionChoice: match &wintrust_info { - WintrustInfo::Catalog(_) => WTD_CHOICE_CATALOG, - WintrustInfo::File(_) => WTD_CHOICE_FILE, - }, + dwUnionChoice: choice, dwStateAction: WTD_STATEACTION_VERIFY, - Anonymous: match &mut wintrust_info { - WintrustInfo::Catalog(x) => WINTRUST_DATA_0 { pCatalog: x }, - WintrustInfo::File(x) => WINTRUST_DATA_0 { pFile: x }, - }, + Anonymous: data, dwProvFlags: WTD_USE_DEFAULT_OSVER_CHECK | WTD_DISABLE_MD2_MD4 | WTD_CACHE_ONLY_URL_RETRIEVAL, ..Default::default() }; @@ -183,14 +231,13 @@ pub struct WinVerifyTrustResult { } pub fn authenticode_status(path: &Path) -> Result { - let file = File::open(path)?; - authenticode_status_for_file(path, &file) + let catalog_info = CatalogInfo::try_from_file(path)?; + win_verify_trust(path, catalog_info) } /// Read the Authenticode status of `file`; `path` only supplies WinTrust subject metadata. pub fn authenticode_status_for_file(path: &Path, file: &File) -> Result { - let catalog_info = CatalogInfo::try_from_file_handle(file)?; - win_verify_trust_for_file(path, file, catalog_info) + win_verify_trust_for_file(path, file) } pub struct CatalogAdminContext { @@ -304,8 +351,8 @@ mod tests { fn catalog_wintrust_info_carries_live_sha256_context_and_member_handle() { let executable = std::env::current_exe().expect("current executable"); let file = File::open(&executable).expect("open current executable"); - let admin_context = Rc::new(CatalogAdminContext::try_new().expect("create SHA-256 catalog context")); - let catalog_info = CatalogInfo { + let admin_context = CatalogAdminContext::try_new().expect("create SHA-256 catalog context"); + let catalog_info = RetainedCatalogInfo { path: PathBuf::from(r"C:\test\catalog.cat"), hash: vec![0xAB; 32], admin_context, @@ -347,11 +394,11 @@ mod tests { let Ok(file) = File::open(&path) else { continue; }; - let Ok(Some(catalog_info)) = CatalogInfo::try_from_file_handle(&file) else { + let Ok(Some(catalog_info)) = RetainedCatalogInfo::try_from_file(&file) else { continue; }; - let result = - win_verify_trust_for_file(&path, &file, Some(catalog_info)).expect("verify catalog-backed system file"); + let result = win_verify_trust_for_file_with_catalog(&path, &file, Some(catalog_info)) + .expect("verify catalog-backed system file"); assert!(matches!(result.status, AuthenticodeSignatureStatus::Valid)); return; } From 66af85d585cffbfc53e3a8a03975edfc58d48c26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Mon, 7 Sep 2026 21:23:16 -0400 Subject: [PATCH 09/10] fix(agent): enforce caller path security in dev Keep trusted-writer and ancestor validation active when only Authenticode is skipped, and support retained callers on local volume-GUID paths. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- crates/now-package-broker/src/auth.rs | 55 +++++++++----- crates/now-package-broker/src/pipe.rs | 26 +++---- .../now-package-broker/src/policy_security.rs | 72 +++++++++++++++++-- 3 files changed, 117 insertions(+), 36 deletions(-) diff --git a/crates/now-package-broker/src/auth.rs b/crates/now-package-broker/src/auth.rs index f123f8de5..d891f6351 100644 --- a/crates/now-package-broker/src/auth.rs +++ b/crates/now-package-broker/src/auth.rs @@ -42,6 +42,13 @@ struct ProcessInstanceIdentity { creation_time: SystemTime, } +#[derive(Clone, Copy)] +enum ExecutableSecurityMode { + Enforce, + #[cfg(test)] + Skip, +} + #[derive(Clone, Debug)] pub(crate) struct PipeClient { process_id: u32, @@ -67,10 +74,7 @@ impl PipeClient { /// This unauthenticated capture blocks on process and local-filesystem work, so callers /// must keep it off the accept loop and bound it with a connection permit. /// Account-name resolution remains deferred because it may contact a domain controller. - pub(crate) fn from_connected_pipe( - server: &NamedPipeServer, - skip_signature_validation: bool, - ) -> anyhow::Result { + pub(crate) fn from_connected_pipe(server: &NamedPipeServer) -> anyhow::Result { let process_id = connected_pipe_client_process_id(server).context("failed to query pipe client process id")?; let process = Arc::new( Process::get_by_pid(process_id, PROCESS_IDENTITY_ACCESS) @@ -78,11 +82,7 @@ impl PipeClient { ); Self::ensure_process_active(process_id, &process)?; let process_instance = process_instance_identity(process_id, &process)?; - let client = Self::from_process( - process_instance, - Arc::clone(&process), - !signature_validation_skipped(skip_signature_validation), - )?; + let client = Self::from_process(process_instance, Arc::clone(&process), ExecutableSecurityMode::Enforce)?; let confirmed_process_id = connected_pipe_client_process_id(server).context("failed to confirm pipe client process id")?; if confirmed_process_id != process_id { @@ -112,7 +112,11 @@ impl PipeClient { Process::get_by_pid(process_id, PROCESS_IDENTITY_ACCESS) .with_context(|| format!("failed to open pipe client process {process_id}"))?, ); - Self::from_process(process_instance_identity(process_id, &process)?, process, false) + Self::from_process( + process_instance_identity(process_id, &process)?, + process, + ExecutableSecurityMode::Skip, + ) } #[cfg(test)] @@ -121,13 +125,17 @@ impl PipeClient { Process::get_by_pid(process_id, PROCESS_IDENTITY_ACCESS) .with_context(|| format!("failed to open pipe client process {process_id}"))?, ); - Self::from_process(process_instance_identity(process_id, &process)?, process, true) + Self::from_process( + process_instance_identity(process_id, &process)?, + process, + ExecutableSecurityMode::Enforce, + ) } fn from_process( process_instance: ProcessInstanceIdentity, process: Arc, - enforce_executable_security: bool, + executable_security_mode: ExecutableSecurityMode, ) -> anyhow::Result { let process_id = process_instance.process_id; let token = process @@ -151,8 +159,8 @@ impl PipeClient { process.verify_image_file_mapping(&executable_file).with_context(|| { format!("pipe client process {process_id} mapped executable file does not match its image") })?; - let executable_security = enforce_executable_security - .then(|| { + let executable_security = match executable_security_mode { + ExecutableSecurityMode::Enforce => Some(Arc::new( crate::policy_security::verify_retained_executable_security( &executable_file, "package broker pipe client executable", @@ -162,10 +170,11 @@ impl PipeClient { "pipe client process {process_id} executable '{}' failed trusted-writer security validation", executable_path.display() ) - }) - }) - .transpose()? - .map(Arc::new); + })?, + )), + #[cfg(test)] + ExecutableSecurityMode::Skip => None, + }; let user_sid = token .sid_and_attributes() .with_context(|| format!("failed to query pipe client process {process_id} token user"))? @@ -1011,6 +1020,16 @@ mod tests { mod dev_build { use super::*; + #[test] + fn signature_bypass_does_not_disable_trusted_writer_security() { + let error = PipeClient::from_process_id_with_security(std::process::id()) + .expect_err("the user-writable test executable path must be rejected"); + assert!( + error.to_string().contains("trusted-writer security validation"), + "unexpected security error: {error:#}" + ); + } + #[test] fn signature_validation_is_skipped_only_when_requested() { assert!(signature_validation_skipped(true)); diff --git a/crates/now-package-broker/src/pipe.rs b/crates/now-package-broker/src/pipe.rs index 8cbab17b8..6a4c51b00 100644 --- a/crates/now-package-broker/src/pipe.rs +++ b/crates/now-package-broker/src/pipe.rs @@ -76,21 +76,23 @@ pub async fn run_pipe_server(state: Arc, shutdown: CancellationToke let serve = async move { // Keep blocking unauthenticated capture off the accept loop and // retain the connection slot until the work actually completes. - let skip_signature_validation = state.skip_signature_validation; let capture = spawn_bounded_capture(permit, move || { - let client = PipeClient::from_connected_pipe(&server, skip_signature_validation); - (server, client) + let client = PipeClient::from_connected_pipe(&server); + (server, client) }); let (_permit, server, client) = match capture.await { - Ok((permit, (server, Ok(client)))) => (permit, server, client), - Ok((_permit, (_server, Err(error)))) => { - warn!(error = format!("{error:#}"), "Rejected named pipe client"); - return; - } - Err(error) => { - error!(error = format!("{error:#}"), "Named pipe client identity capture task failed"); - return; - } + Ok((permit, (server, Ok(client)))) => (permit, server, client), + Ok((_permit, (_server, Err(error)))) => { + warn!(error = format!("{error:#}"), "Rejected named pipe client"); + return; + } + Err(error) => { + error!( + error = format!("{error:#}"), + "Named pipe client identity capture task failed" + ); + return; + } }; info!("Client connected to named pipe"); diff --git a/crates/now-package-broker/src/policy_security.rs b/crates/now-package-broker/src/policy_security.rs index 73441e0d7..d4a68cf38 100644 --- a/crates/now-package-broker/src/policy_security.rs +++ b/crates/now-package-broker/src/policy_security.rs @@ -38,7 +38,9 @@ use std::path::{Path, PathBuf}; use anyhow::{Context as _, bail}; use sha2::{Digest as _, Sha256}; -use windows::Win32::Foundation::{ERROR_SUCCESS, GENERIC_ALL, GENERIC_WRITE, HANDLE, HLOCAL, LocalFree}; +use windows::Win32::Foundation::{ + ERROR_PATH_NOT_FOUND, ERROR_SUCCESS, GENERIC_ALL, GENERIC_WRITE, HANDLE, HLOCAL, LocalFree, +}; use windows::Win32::Globalization::{CSTR_EQUAL, CompareStringOrdinal}; use windows::Win32::Security::Authorization::{ConvertSidToStringSidW, GetSecurityInfo, SE_FILE_OBJECT}; use windows::Win32::Security::{ @@ -50,8 +52,8 @@ 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_WRITE_ATTRIBUTES, FILE_WRITE_DATA, FILE_WRITE_EA, - FileAttributeTagInfo, GetFileInformationByHandleEx, GetFinalPathNameByHandleW, READ_CONTROL, WRITE_DAC, - WRITE_OWNER, + FileAttributeTagInfo, GETFINALPATHNAMEBYHANDLE_FLAGS, GetFileInformationByHandleEx, GetFinalPathNameByHandleW, + READ_CONTROL, VOLUME_NAME_GUID, WRITE_DAC, WRITE_OWNER, }; use windows::core::PWSTR; @@ -79,6 +81,8 @@ const DIRECTORY_TAMPER_MASK: u32 = FILE_DELETE_CHILD.0 /* delete or rename child /// Additional rights that allow retargeting an ancestor that is itself a reparse point. const REPARSE_POINT_TAMPER_MASK: u32 = FILE_WRITE_DATA.0 | GENERIC_WRITE.0; +const VOLUME_GUID_FINAL_PATH_FLAGS: GETFINALPATHNAMEBYHANDLE_FLAGS = + GETFINALPATHNAMEBYHANDLE_FLAGS(FILE_NAME_NORMALIZED.0 | VOLUME_NAME_GUID.0); /// Access rights on the directory hosting a verified executable that allow tampering with /// its execution. On top of [`DIRECTORY_TAMPER_MASK`], create rights are rejected: a @@ -679,14 +683,30 @@ fn is_reparse_point(file: &File) -> anyhow::Result { /// Resolve the normalized final path of an open file from its handle. fn final_path_from_handle(file: &File) -> anyhow::Result { let handle = HANDLE(file.as_raw_handle()); + match final_path_name(handle, FILE_NAME_NORMALIZED) { + Ok(path) => Ok(final_path_from_wide(&path, false)), + Err(error) if should_retry_final_path_with_volume_guid(&error) => { + let path = final_path_name(handle, VOLUME_GUID_FINAL_PATH_FLAGS) + .context("GetFinalPathNameByHandleW failed for volume GUID path")?; + Ok(final_path_from_wide(&path, true)) + } + Err(error) => Err(error).context("GetFinalPathNameByHandleW failed"), + } +} + +fn should_retry_final_path_with_volume_guid(error: &windows::core::Error) -> bool { + error.code() == ERROR_PATH_NOT_FOUND.to_hresult() +} + +fn final_path_name(handle: HANDLE, flags: GETFINALPATHNAMEBYHANDLE_FLAGS) -> windows::core::Result> { let mut buffer = vec![0u16; 512]; loop { // SAFETY: `handle` is a valid open file handle and `buffer` is a live mutable slice. - let len = unsafe { GetFinalPathNameByHandleW(handle, &mut buffer, FILE_NAME_NORMALIZED) }; + let len = unsafe { GetFinalPathNameByHandleW(handle, &mut buffer, flags) }; if len == 0 { - return Err(windows::core::Error::from_win32()).context("GetFinalPathNameByHandleW failed"); + return Err(windows::core::Error::from_win32()); } let len = usize::try_from(len).expect("u32 fits in usize on Windows"); @@ -695,13 +715,21 @@ fn final_path_from_handle(file: &File) -> anyhow::Result { // otherwise it is the required buffer size (including the null terminator). if len < buffer.len() { buffer.truncate(len); - return Ok(dos_path_from_wide(&buffer)); + return Ok(buffer); } buffer.resize(len, 0); } } +fn final_path_from_wide(wide: &[u16], preserve_verbatim_prefix: bool) -> PathBuf { + if preserve_verbatim_prefix { + PathBuf::from(OsString::from_wide(wide)) + } else { + dos_path_from_wide(wide) + } +} + /// Convert a possibly `\\?\`-prefixed wide path to its plain Win32 form. /// /// The resolved path is embedded into generated batch scripts and passed to @@ -928,6 +956,38 @@ mod tests { use super::*; + #[test] + fn final_path_retries_only_when_dos_volume_resolution_is_unavailable() { + let path_not_found = windows::core::Error::from_hresult(ERROR_PATH_NOT_FOUND.to_hresult()); + let access_denied = + windows::core::Error::from_hresult(windows::Win32::Foundation::ERROR_ACCESS_DENIED.to_hresult()); + + assert!(should_retry_final_path_with_volume_guid(&path_not_found)); + assert!(!should_retry_final_path_with_volume_guid(&access_denied)); + assert_eq!(VOLUME_GUID_FINAL_PATH_FLAGS.0, 1); + } + + #[test] + fn volume_guid_final_path_preserves_verbatim_prefix_and_root() { + let raw = r"\\?\Volume{01234567-89ab-cdef-0123-456789abcdef}\Program Files\Client\client.exe"; + let wide: Vec = raw.encode_utf16().collect(); + let path = final_path_from_wide(&wide, true); + let root = Path::new(r"\\?\Volume{01234567-89ab-cdef-0123-456789abcdef}\"); + + assert_eq!(path, Path::new(raw)); + assert!(path.starts_with(root)); + assert_eq!(path.ancestors().last(), Some(root)); + } + + #[test] + fn final_path_resolves_current_executable_on_mounted_volume() { + let executable = std::env::current_exe().expect("current executable"); + let file = File::open(&executable).expect("open current executable"); + let resolved = final_path_from_handle(&file).expect("resolve current executable path"); + + assert!(windows_paths_equal(&resolved, &executable)); + } + /// SDDL-backed security descriptor together with its extracted owner and DACL pointers. struct SddlDescriptor { _descriptor: OwnedSecurityDescriptor, From f19fee3c5fbf2e96022e9d42b51ffbc5dabbb0d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Beno=C3=AEt=20CORTIER?= Date: Mon, 7 Sep 2026 22:31:24 -0400 Subject: [PATCH 10/10] test(agent): stage policy tester securely Run the LocalSystem policy tester from an atomically protected ProgramData leaf so production caller ACL checks remain enabled in CI. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- crates/agent-policy-tester/run-as-system.ps1 | 86 +++++++++++++++++++- 1 file changed, 84 insertions(+), 2 deletions(-) diff --git a/crates/agent-policy-tester/run-as-system.ps1 b/crates/agent-policy-tester/run-as-system.ps1 index 10b988fb4..8520d10ec 100644 --- a/crates/agent-policy-tester/run-as-system.ps1 +++ b/crates/agent-policy-tester/run-as-system.ps1 @@ -4,13 +4,95 @@ $workspacePath = (Resolve-Path (Join-Path $PSScriptRoot "../..")).Path $testerPath = Join-Path $workspacePath "target/debug/agent-policy-tester.exe" $agentPath = Join-Path $workspacePath "target/debug/devolutions-agent.exe" $outputPath = Join-Path $PSScriptRoot "agent-policy-tester.out" +$stagingPath = Join-Path $env:ProgramData "dgw-agent-policy-tester-$([guid]::NewGuid().ToString('N'))" +$stagedTesterPath = Join-Path $stagingPath "agent-policy-tester.exe" try { - & $testerPath $agentPath 2>&1 | Out-File $outputPath + Set-Content -LiteralPath $outputPath -Value "" + Add-Type -TypeDefinition @' +using System; +using System.ComponentModel; +using System.Runtime.InteropServices; + +public static class AgentPolicyTesterNativeDirectory +{ + [StructLayout(LayoutKind.Sequential)] + private struct SecurityAttributes + { + internal int Length; + internal IntPtr SecurityDescriptor; + internal int InheritHandle; + } + + [DllImport("kernel32.dll", CharSet = CharSet.Unicode, SetLastError = true)] + private static extern bool CreateDirectoryW(string path, ref SecurityAttributes securityAttributes); + + public static void Create(string path, byte[] securityDescriptor) + { + GCHandle pinnedDescriptor = GCHandle.Alloc(securityDescriptor, GCHandleType.Pinned); + try + { + SecurityAttributes attributes = new SecurityAttributes + { + Length = Marshal.SizeOf(), + SecurityDescriptor = pinnedDescriptor.AddrOfPinnedObject(), + InheritHandle = 0, + }; + if (!CreateDirectoryW(path, ref attributes)) + { + throw new Win32Exception(Marshal.GetLastWin32Error()); + } + } + finally + { + pinnedDescriptor.Free(); + } + } +} +'@ + $directorySecurity = [System.Security.AccessControl.DirectorySecurity]::new() + $directorySecurity.SetSecurityDescriptorSddlForm( + 'O:SYG:SYD:P(A;OICI;FA;;;SY)(A;OICI;FA;;;BA)' + ) + [AgentPolicyTesterNativeDirectory]::Create( + $stagingPath, + $directorySecurity.GetSecurityDescriptorBinaryForm() + ) + if (Get-ChildItem -LiteralPath $stagingPath -Force) { + throw "The atomically protected staged tester directory was not empty" + } + + Copy-Item -LiteralPath $testerPath -Destination $stagedTesterPath + & icacls.exe $stagedTesterPath /setowner '*S-1-5-18' 2>&1 | Out-File $outputPath -Append + if ($LASTEXITCODE -ne 0) { + throw "Failed to set the staged tester owner" + } + & icacls.exe $stagedTesterPath /inheritance:r /grant:r '*S-1-5-18:(F)' '*S-1-5-32-544:(F)' 2>&1 | + Out-File $outputPath -Append + if ($LASTEXITCODE -ne 0) { + throw "Failed to protect the staged tester executable" + } + + "Staged policy tester at $stagedTesterPath" | Out-File $outputPath -Append + Get-Acl -LiteralPath $stagingPath | Format-List Owner, Sddl | Out-File $outputPath -Append + Get-Acl -LiteralPath $stagedTesterPath | Format-List Owner, Sddl | Out-File $outputPath -Append + & $stagedTesterPath $agentPath 2>&1 | Out-File $outputPath -Append $exitCode = $LASTEXITCODE } catch { $_ | Out-File $outputPath -Append - exit 1 + $exitCode = 1 +} finally { + for ($attempt = 0; $attempt -lt 20 -and (Test-Path -LiteralPath $stagingPath); $attempt++) { + try { + Remove-Item -LiteralPath $stagingPath -Recurse -Force + } catch { + if ($attempt -eq 19) { + "Failed to remove $stagingPath after 20 attempts: $_" | Out-File $outputPath -Append + } else { + Start-Sleep -Milliseconds 250 + } + } + } } exit $exitCode