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 diff --git a/crates/devolutions-agent-shared/src/windows/code_signing.rs b/crates/devolutions-agent-shared/src/windows/code_signing.rs index 4d58e5356..d55443de2 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 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) +} + +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 c5126dcb0..d891f6351 100644 --- a/crates/now-package-broker/src/auth.rs +++ b/crates/now-package-broker/src/auth.rs @@ -1,9 +1,20 @@ //! Package broker pipe client authentication. - +//! +//! 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}; +use std::os::windows::fs::OpenOptionsExt as _; 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; +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}; @@ -11,14 +22,41 @@ 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::{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_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, +}; + +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, +); + +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +struct ProcessInstanceIdentity { + process_id: u32, + creation_time: SystemTime, +} + +#[derive(Clone, Copy)] +enum ExecutableSecurityMode { + Enforce, + #[cfg(test)] + Skip, +} #[derive(Clone, Debug)] pub(crate) struct PipeClient { process_id: u32, + process_creation_time: SystemTime, + 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. @@ -30,23 +68,113 @@ 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. + /// 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. 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), 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 { + 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}"), + } } + #[cfg(test)] 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 executable_path = process - .exe_path() - .with_context(|| format!("failed to query pipe client process {process_id} executable path"))?; + 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, + ExecutableSecurityMode::Skip, + ) + } + + #[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, + ExecutableSecurityMode::Enforce, + ) + } + + fn from_process( + process_instance: ProcessInstanceIdentity, + process: Arc, + executable_security_mode: ExecutableSecurityMode, + ) -> anyhow::Result { + let process_id = process_instance.process_id; 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 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", + ) + .with_context(|| { + format!( + "pipe client process {process_id} executable '{}' failed trusted-writer security validation", + executable_path.display() + ) + })?, + )), + #[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"))? @@ -59,10 +187,17 @@ 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), + executable_security, user_sid, is_elevated, is_administrator, @@ -124,15 +259,24 @@ 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(()); } - 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, + process_creation_time = ?self.process_creation_time, executable = %self.executable_path.display(), certificate_thumbprint = %thumbprint, "Package broker pipe client authenticated" @@ -141,6 +285,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, @@ -183,12 +342,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") })?; @@ -231,6 +394,46 @@ 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(()) +} + +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)) +} + +/// 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{") +} + /// 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")?; @@ -240,22 +443,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 @@ -305,13 +509,67 @@ mod tests { fn system_client() -> PipeClient { PipeClient { process_id: 0, + process_creation_time: SystemTime::UNIX_EPOCH, + process: None, executable_path: PathBuf::new(), + executable_file: None, + executable_security: None, user_sid: system_sid(), is_elevated: true, is_administrator: true, } } + #[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 @@ -382,6 +640,352 @@ 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}"); + } + } + + #[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::*; @@ -398,7 +1002,11 @@ 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"), + executable_file: None, + executable_security: None, user_sid: client_user_sid(), is_elevated: false, is_administrator: false, @@ -412,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 1cb6d6ca7..6a4c51b00 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,19 +73,28 @@ 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) { - Ok(client) => client, + // Keep blocking unauthenticated capture off the accept loop and + // retain the connection slot until the work actually completes. + let capture = spawn_bounded_capture(permit, move || { + 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) => { - warn!(%error, "Rejected named pipe client"); + error!( + error = format!("{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; @@ -111,6 +121,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")?; @@ -174,3 +192,53 @@ fn build_pipe_security_attributes() -> anyhow::Result, +} + +/// 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, + }) +} + /// 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. /// @@ -321,8 +357,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 @@ -337,6 +371,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. @@ -359,7 +394,15 @@ pub(crate) fn verify_elevated_executable_security( WRITE_ACCESS_MASK, )?; - verify_ancestor_directories(&final_path, &subject)?; + // 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, @@ -544,14 +587,47 @@ 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, - ) +/// 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(); + 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( @@ -607,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"); @@ -623,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 @@ -856,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, @@ -1117,9 +1249,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; }; @@ -1127,6 +1260,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) => { @@ -1135,10 +1270,15 @@ 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(&resolved_target) + && (message.contains("ancestor directory") + || message.contains("owner") + || message.contains("DACL grants write access")), "unexpected error: {error:#}" ); } diff --git a/crates/win-api-wrappers/src/process.rs b/crates/win-api-wrappers/src/process.rs index 06dc69f5c..9696edb80 100644 --- a/crates/win-api-wrappers/src/process.rs +++ b/crates/win-api-wrappers/src/process.rs @@ -1,15 +1,18 @@ 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}; 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}; @@ -21,13 +24,15 @@ 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, - 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::{ @@ -41,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)] @@ -103,6 +111,76 @@ impl Process { Ok(OsString::from_wide(&path).into()) } + /// Returns the main image's transfer address and a candidate native path for it. + /// + /// 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(); + + // 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` is the same kernel file object backing this process's main image. + /// + /// `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()); + + // 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"); @@ -196,6 +274,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)) }; diff --git a/crates/win-api-wrappers/src/security/crypt.rs b/crates/win-api-wrappers/src/security/crypt.rs index 35ee61c59..e423a7282 100644 --- a/crates/win-api-wrappers/src/security/crypt.rs +++ b/crates/win-api-wrappers/src/security/crypt.rs @@ -1,6 +1,7 @@ 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}; @@ -21,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; @@ -37,29 +39,77 @@ pub struct CatalogInfo { impl CatalogInfo { pub fn try_from_file(path: &Path) -> Result> { - let admin_ctx = CatalogAdminContext::try_new()?; + let admin_context = CatalogAdminContext::try_new()?; + let hash = admin_context.hash_file(path)?; + let catalog_path = admin_context.catalogs_for_hash(&hash).next(); - let hash = admin_ctx.hash_file(path)?; + Ok(catalog_path.map(|catalog_path| Self { + hash, + path: catalog_path, + })) + } +} + +struct RetainedCatalogInfo { + path: PathBuf, + hash: Vec, + admin_context: CatalogAdminContext, +} - let catalog_path = admin_ctx.catalogs_for_hash(&hash).next(); +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() + }; Ok(catalog_path.map(|catalog_path| Self { hash, path: catalog_path, + admin_context, })) } } +fn wintrust_catalog_info( + catalog_info: &RetainedCatalogInfo, + 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 path = WideString::from(path); - let catalog_info = catalog_info.map(|c| { + let catalog_info = catalog_info.map(|catalog| { ( - WideString::from(&c.path), - WideString::from(base16ct::upper::encode_string(&c.hash)), + WideString::from(&catalog.path), + WideString::from(base16ct::upper::encode_string(&catalog.hash)), ) }); @@ -69,11 +119,11 @@ pub fn win_verify_trust(path: &Path, catalog_info: Option) -> Resul } let mut wintrust_info = match &catalog_info { - Some((catalog_info_path, catalog_info_member)) => WintrustInfo::Catalog(WINTRUST_CATALOG_INFO { + Some((catalog_path, member_tag)) => WintrustInfo::Catalog(WINTRUST_CATALOG_INFO { cbStruct: u32size_of::(), - pcwszCatalogFilePath: catalog_info_path.as_pcwstr(), + pcwszCatalogFilePath: catalog_path.as_pcwstr(), pcwszMemberFilePath: path.as_pcwstr(), - pcwszMemberTag: catalog_info_member.as_pcwstr(), + pcwszMemberTag: member_tag.as_pcwstr(), ..Default::default() }), None => WintrustInfo::File(WINTRUST_FILE_INFO { @@ -83,19 +133,65 @@ pub fn win_verify_trust(path: &Path, catalog_info: Option) -> Resul }), }; + 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(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, +) -> Result { + let path = WideString::from(path); + let catalog_strings = catalog_info.as_ref().map(|catalog_info| { + ( + WideString::from(&catalog_info.path), + WideString::from(base16ct::upper::encode_string(&catalog_info.hash)), + ) + }); + + enum WintrustInfo { + Catalog(WINTRUST_CATALOG_INFO), + File(WINTRUST_FILE_INFO), + } + + 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 (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() }; @@ -136,10 +232,14 @@ pub struct WinVerifyTrustResult { pub fn authenticode_status(path: &Path) -> Result { 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 { + win_verify_trust_for_file(path, file) +} + pub struct CatalogAdminContext { pub handle: HANDLE, } @@ -164,11 +264,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 +307,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 +324,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 = 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, + }; + 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)) = RetainedCatalogInfo::try_from_file(&file) else { + continue; + }; + 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; + } + } +} + pub struct CatalogIterator<'a> { admin_ctx: &'a CatalogAdminContext, cur: Option, 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);