diff --git a/crates/devolutions-pedm-hook/src/hook.rs b/crates/devolutions-pedm-hook/src/hook.rs index 310bb1637..504120a52 100644 --- a/crates/devolutions-pedm-hook/src/hook.rs +++ b/crates/devolutions-pedm-hook/src/hook.rs @@ -123,8 +123,8 @@ fn rai_launch_admin_process_handler( let proc_info = proc_info.map_err(|x| win_api_wrappers::raw::core::Error::from_hresult(HRESULT(x.win32_error as _)))?; - let mut process = Process::try_get_by_pid(proc_info.process_id, PROCESS_ALL_ACCESS)?; - let mut thread = Thread::try_get_by_id(proc_info.thread_id, THREAD_ALL_ACCESS)?; + let mut process = Process::get_by_pid(proc_info.process_id, PROCESS_ALL_ACCESS)?; + let mut thread = Thread::get_by_id(proc_info.thread_id, THREAD_ALL_ACCESS)?; thread.resume()?; diff --git a/crates/devolutions-pedm-shell-ext/src/lib_win.rs b/crates/devolutions-pedm-shell-ext/src/lib_win.rs index 14f2dd3f7..259352788 100644 --- a/crates/devolutions-pedm-shell-ext/src/lib_win.rs +++ b/crates/devolutions-pedm-shell-ext/src/lib_win.rs @@ -179,7 +179,7 @@ fn find_main_explorer(session: u32) -> Option { let snapshot = Snapshot::new(TH32CS_SNAPPROCESS, None).ok()?; snapshot.process_ids().find_map(|pid| { - let proc = Process::try_get_by_pid(pid, PROCESS_QUERY_INFORMATION | PROCESS_VM_READ).ok()?; + let proc = Process::get_by_pid(pid, PROCESS_QUERY_INFORMATION | PROCESS_VM_READ).ok()?; if !(proc .exe_path() diff --git a/crates/devolutions-pedm/src/api/launch.rs b/crates/devolutions-pedm/src/api/launch.rs index ab09f0241..a93f09c2b 100644 --- a/crates/devolutions-pedm/src/api/launch.rs +++ b/crates/devolutions-pedm/src/api/launch.rs @@ -105,7 +105,7 @@ pub(crate) async fn post_launch( .and_then(|x| x.parent_pid) .unwrap_or(named_pipe_info.pipe_process_id); - let process = Process::try_get_by_pid(parent_pid, PROCESS_QUERY_INFORMATION | PROCESS_CREATE_PROCESS)?; + let process = Process::get_by_pid(parent_pid, PROCESS_QUERY_INFORMATION | PROCESS_CREATE_PROCESS)?; let caller_sid = named_pipe_info.token.sid_and_attributes()?.sid; diff --git a/crates/devolutions-pedm/src/api/mod.rs b/crates/devolutions-pedm/src/api/mod.rs index e4a5d624c..f4075782e 100644 --- a/crates/devolutions-pedm/src/api/mod.rs +++ b/crates/devolutions-pedm/src/api/mod.rs @@ -61,9 +61,9 @@ struct RawNamedPipeConnectInfo { impl Connected<&NamedPipeServer> for RawNamedPipeConnectInfo { fn connect_info(target: &NamedPipeServer) -> Self { - Self { - handle: Handle::new(HANDLE(target.as_raw_handle().cast()), false), - } + let handle = HANDLE(target.as_raw_handle().cast()); + let handle = Handle::new_borrowed(handle).expect("the handled held by NamedPipeServer is valid"); + Self { handle } } } diff --git a/crates/devolutions-pedm/src/policy.rs b/crates/devolutions-pedm/src/policy.rs index 53bf184d4..769bee15d 100644 --- a/crates/devolutions-pedm/src/policy.rs +++ b/crates/devolutions-pedm/src/policy.rs @@ -518,7 +518,7 @@ pub(crate) fn application_from_path( } pub(crate) fn application_from_process(pid: u32) -> Result { - let process = Process::try_get_by_pid(pid, PROCESS_QUERY_INFORMATION | PROCESS_VM_READ)?; + let process = Process::get_by_pid(pid, PROCESS_QUERY_INFORMATION | PROCESS_VM_READ)?; let path = process.exe_path()?; diff --git a/crates/win-api-wrappers/src/handle.rs b/crates/win-api-wrappers/src/handle.rs index 789be3111..4f8a8f05c 100644 --- a/crates/win-api-wrappers/src/handle.rs +++ b/crates/win-api-wrappers/src/handle.rs @@ -1,11 +1,16 @@ use std::fmt::Debug; use std::os::windows::io::{AsRawHandle, BorrowedHandle, IntoRawHandle, OwnedHandle}; -use anyhow::Result; - +use windows::Win32::Foundation::E_HANDLE; use windows::Win32::Foundation::{CloseHandle, DuplicateHandle, DUPLICATE_SAME_ACCESS, HANDLE}; use windows::Win32::System::Threading::GetCurrentProcess; +// TODO: Use/implement AsHandle and AsRawHandle as appropriate + +/// A wrapper around a Windows [`HANDLE`]. +/// +/// Whenever possible, you should use [`BorrowedHandle`] or [`OwnedHandle`] instead. +/// Those are safer to use. #[derive(Debug, Clone)] pub struct Handle { raw: HANDLE, @@ -18,16 +23,58 @@ unsafe impl Send for Handle {} // SAFETY: A `HANDLE` is simply an integer, no dereferencing is done. unsafe impl Sync for Handle {} +/// The `Drop` implementation is assuming we constructed the `Handle` object in +/// a sane way to call `CloseHandle`, but there is no way for us to verify that +/// the handle is actually owned outside of the callsite. Conceptually, calling +/// `Handle::new_owned(handle)` or `Handle::new(handle, true)` is like calling the +/// unsafe function `CloseHandle` and thus must inherit its safety preconditions. impl Handle { - pub fn new(handle: HANDLE, owned: bool) -> Self { - Self { raw: handle, owned } + /// Wraps a Windows [`HANDLE`]. + /// + /// # Safety + /// + /// When `owned` is `true`: + /// + /// - `handle` is a valid handle to an open object. + /// - `handle` is not a pseudohandle. + /// - The caller is actually responsible for closing the `HANDLE` when the value goes out of scope. + /// + /// When `owned` is `false`: no outstanding precondition. + pub unsafe fn new(handle: HANDLE, owned: bool) -> Result { + if handle.is_invalid() || handle.0.is_null() { + return Err(crate::Error::from_hresult(E_HANDLE)); + } + + Ok(Self { raw: handle, owned }) + } + + /// Wraps an owned Windows [`HANDLE`]. + /// + /// # Safety + /// + /// - `handle` is a valid handle to an open object. + /// - `handle` is not a pseudohandle. + /// - The caller is actually responsible for closing the `HANDLE` when the value goes out of scope. + pub unsafe fn new_owned(handle: HANDLE) -> Result { + // SAFETY: Same preconditions as the called function. + unsafe { Self::new(handle, true) } + } +} + +impl Handle { + /// Wraps a borrowed Windows [`HANDLE`]. + /// + /// Always use this when knowing statically that the handle is never owned. + pub fn new_borrowed(handle: HANDLE) -> Result { + // SAFETY: It’s safe to wrap a non-owning Handle as we’ll not call `CloseHandle` on it. + unsafe { Self::new(handle, false) } } pub fn raw(&self) -> HANDLE { self.raw } - pub fn as_raw_ref(&self) -> &HANDLE { + pub fn raw_as_ref(&self) -> &HANDLE { &self.raw } @@ -35,9 +82,10 @@ impl Handle { self.owned = false; } - pub fn try_clone(&self) -> Result { + pub fn try_clone(&self) -> anyhow::Result { // SAFETY: No preconditions. Always a valid handle. let current_process = unsafe { GetCurrentProcess() }; + let mut duplicated = HANDLE::default(); // SAFETY: `current_process` is valid. No preconditions. Returned handle is closed with its RAII wrapper. @@ -53,49 +101,52 @@ impl Handle { )?; } - Ok(duplicated.into()) + // SAFETY: The duplicated handle is owned by us. + let handle = unsafe { Self::new_owned(duplicated)? }; + + Ok(handle) } } impl Drop for Handle { fn drop(&mut self) { if self.owned { - // SAFETY: No preconditions and handle is assumed to be valid if owned (we assume it is not a pseudohandle). + // SAFETY: `self.raw` is a valid handle to an open object by construction. + // It’s also safe to close it ourselves when `self.owned` is true per contract. let _ = unsafe { CloseHandle(self.raw) }; } } } -impl From for Handle { - fn from(value: HANDLE) -> Self { - Self::new(value, true) - } -} +// TODO: make this return a borrowed `Handle`. impl TryFrom<&BorrowedHandle<'_>> for Handle { type Error = anyhow::Error; - fn try_from(value: &BorrowedHandle<'_>) -> Result { - let handle = Handle { + fn try_from(value: &BorrowedHandle<'_>) -> anyhow::Result { + let handle = Self { raw: HANDLE(value.as_raw_handle().cast()), owned: false, }; - Self::try_clone(&handle) + handle.try_clone() } } impl TryFrom> for Handle { type Error = anyhow::Error; - fn try_from(value: BorrowedHandle<'_>) -> Result { + fn try_from(value: BorrowedHandle<'_>) -> anyhow::Result { Self::try_from(&value) } } impl From for Handle { fn from(handle: OwnedHandle) -> Self { - Self::from(HANDLE(handle.into_raw_handle().cast())) + Self { + raw: HANDLE(handle.into_raw_handle().cast()), + owned: true, + } } } diff --git a/crates/win-api-wrappers/src/process.rs b/crates/win-api-wrappers/src/process.rs index de4f6bf3b..0f0da89a0 100644 --- a/crates/win-api-wrappers/src/process.rs +++ b/crates/win-api-wrappers/src/process.rs @@ -17,7 +17,7 @@ use crate::utils::{serialize_environment, Allocation, AnsiString, CommandLine, W use crate::Error; use windows::core::PCWSTR; use windows::Win32::Foundation::{ - FreeLibrary, ERROR_INCORRECT_SIZE, E_HANDLE, HANDLE, HMODULE, MAX_PATH, WAIT_EVENT, WAIT_FAILED, + FreeLibrary, ERROR_INCORRECT_SIZE, HANDLE, HMODULE, MAX_PATH, WAIT_EVENT, WAIT_FAILED, }; use windows::Win32::Security::TOKEN_ACCESS_MASK; use windows::Win32::System::Com::{COINIT_APARTMENTTHREADED, COINIT_DISABLE_OLE1DDE}; @@ -39,23 +39,24 @@ use super::utils::{size_of_u32, ComContext}; #[derive(Debug)] pub struct Process { + // Handle is closed with RAII wrapper. pub handle: Handle, } +impl From for Process { + fn from(handle: Handle) -> Self { + Self { handle } + } +} + impl Process { - pub fn try_get_by_pid(pid: u32, desired_access: PROCESS_ACCESS_RIGHTS) -> Result { - // SAFETY: No preconditions. Handle is closed with RAII wrapper. + pub fn get_by_pid(pid: u32, desired_access: PROCESS_ACCESS_RIGHTS) -> Result { + // SAFETY: No preconditions. let handle = unsafe { OpenProcess(desired_access, false, pid) }?; + // SAFETY: The handle is owned by us, we opened the process above. + let handle = unsafe { Handle::new_owned(handle)? }; - Ok(Self { handle: handle.into() }) - } - - pub fn try_with_handle(handle: HANDLE) -> Result { - if handle.is_invalid() { - bail!(Error::from_hresult(E_HANDLE)) - } else { - Ok(Self { handle: handle.into() }) - } + Ok(Self { handle }) } pub fn exe_path(&self) -> Result { @@ -129,7 +130,6 @@ impl Process { let mut thread_id: u32 = 0; // SAFETY: We assume `start_address` points to a valid and executable memory address. - // No preconditions. let handle = unsafe { CreateRemoteThread( self.handle.raw(), @@ -139,10 +139,13 @@ impl Process { parameter, 0, Some(&mut thread_id), - ) + )? }; - Thread::try_with_handle(handle?) + // SAFETY: The handle is owned by us, we opened the ressource above. + let handle = unsafe { Handle::new_owned(handle) }?; + + Ok(Thread::from(handle)) } pub fn allocate(&self, size: usize) -> Result> { @@ -163,19 +166,23 @@ impl Process { } pub fn current_process() -> Self { - Self { - // SAFETY: `GetCurrentProcess()` has no preconditions and always returns a valid handle. - handle: Handle::new(unsafe { GetCurrentProcess() }, false), - } + // SAFETY: `GetCurrentProcess()` has no preconditions and always returns a valid handle. + let handle = unsafe { GetCurrentProcess() }; + let handle = Handle::new_borrowed(handle).expect("always valid"); + + Self { handle } } pub fn token(&self, desired_access: TOKEN_ACCESS_MASK) -> Result { - let mut handle = Default::default(); + let mut handle = HANDLE::default(); // SAFETY: No preconditions. Returned handle will be closed with its RAII wrapper. unsafe { OpenProcessToken(self.handle.raw(), desired_access, &mut handle) }?; - Token::try_with_handle(handle) + // SAFETY: We own the handle. + let handle = unsafe { Handle::new_owned(handle)? }; + + Ok(Token::from(handle)) } pub fn wait(&self, timeout_ms: Option) -> Result { @@ -330,7 +337,10 @@ pub fn shell_execute( // SAFETY: Must be called with COM context initialized. unsafe { ShellExecuteExW(&mut exec_info) }?; - Process::try_with_handle(exec_info.hProcess) + // SAFETY: We are responsibles for closing the handle. + let handle = unsafe { Handle::new_owned(exec_info.hProcess)? }; + + Ok(Process::from(handle)) } pub struct Peb<'a> { @@ -590,9 +600,15 @@ pub fn create_process_as_user( ) }?; + // SAFETY: The handle is owned by us, we opened the ressource above. + let process = unsafe { Handle::new_owned(raw_process_information.hProcess).map(Process::from)? }; + + // SAFETY: The handle is owned by us, we opened the ressource above. + let thread = unsafe { Handle::new_owned(raw_process_information.hThread).map(Thread::from)? }; + Ok(ProcessInformation { - process: Process::try_with_handle(raw_process_information.hProcess)?, - thread: Thread::try_with_handle(raw_process_information.hThread)?, + process, + thread, process_id: raw_process_information.dwProcessId, thread_id: raw_process_information.dwThreadId, }) diff --git a/crates/win-api-wrappers/src/security/privilege.rs b/crates/win-api-wrappers/src/security/privilege.rs index 6c6b7c2d0..497e608e5 100644 --- a/crates/win-api-wrappers/src/security/privilege.rs +++ b/crates/win-api-wrappers/src/security/privilege.rs @@ -93,7 +93,7 @@ pub fn find_token_with_privilege(privilege: LUID) -> Result> { let snapshot = Snapshot::new(TH32CS_SNAPPROCESS, None)?; Ok(snapshot.process_ids().find_map(|pid| { - let proc = Process::try_get_by_pid(pid, PROCESS_QUERY_INFORMATION).ok()?; + let proc = Process::get_by_pid(pid, PROCESS_QUERY_INFORMATION).ok()?; let token = proc.token(TOKEN_ALL_ACCESS).ok()?; if token.privileges().ok()?.0.iter().any(|p| p.Luid == privilege) { diff --git a/crates/win-api-wrappers/src/thread.rs b/crates/win-api-wrappers/src/thread.rs index 95c9ff195..cae17f537 100644 --- a/crates/win-api-wrappers/src/thread.rs +++ b/crates/win-api-wrappers/src/thread.rs @@ -8,7 +8,7 @@ use crate::handle::{Handle, HandleWrapper}; use crate::process::Process; use crate::token::Token; use crate::Error; -use windows::Win32::Foundation::{E_HANDLE, HANDLE, WAIT_OBJECT_0}; +use windows::Win32::Foundation::{HANDLE, WAIT_OBJECT_0}; use windows::Win32::Security::TOKEN_ACCESS_MASK; use windows::Win32::System::Threading::{ DeleteProcThreadAttributeList, GetCurrentThread, InitializeProcThreadAttributeList, OpenThread, OpenThreadToken, @@ -22,32 +22,34 @@ pub struct Thread { pub handle: Handle, } -impl Thread { - pub fn try_with_handle(handle: HANDLE) -> Result { - if handle.is_invalid() { - bail!(Error::from_hresult(E_HANDLE)) - } else { - Ok(Self { handle: handle.into() }) - } +impl From for Thread { + fn from(handle: Handle) -> Self { + Self { handle } } +} - pub fn try_get_by_id(id: u32, desired_access: THREAD_ACCESS_RIGHTS) -> Result { +impl Thread { + pub fn get_by_id(id: u32, desired_access: THREAD_ACCESS_RIGHTS) -> Result { // SAFETY: No preconditions. - let handle = unsafe { OpenThread(desired_access, false, id) }?; + let handle = unsafe { OpenThread(desired_access, false, id)? }; + // SAFETY: The handle is owned by us, we opened the ressource above. + let handle = unsafe { Handle::new_owned(handle)? }; - Self::try_with_handle(handle) + Ok(Self::from(handle)) } pub fn current() -> Self { - Self { - // SAFETY: No preconditions. Returns a pseudohandle, thus not owning it. - handle: Handle::new(unsafe { GetCurrentThread() }, false), - } + // SAFETY: No preconditions. Returns a pseudohandle, thus not owning it. + let handle = unsafe { GetCurrentThread() }; + let handle = Handle::new_borrowed(handle).expect("always valid"); + + Self::from(handle) } pub fn join(&self, timeout_ms: Option) -> Result<()> { // SAFETY: No preconditions. let result = unsafe { WaitForSingleObject(self.handle.raw(), timeout_ms.unwrap_or(INFINITE)) }; + match result { WAIT_OBJECT_0 => Ok(()), _ => bail!(Error::last_error()), @@ -73,12 +75,15 @@ impl Thread { } pub fn token(&self, desired_access: TOKEN_ACCESS_MASK, open_as_self: bool) -> Result { - let mut handle = Default::default(); + let mut handle = HANDLE::default(); // SAFETY: Returned handle must be closed, which is done in its RAII wrapper. unsafe { OpenThreadToken(self.handle.raw(), desired_access, open_as_self, &mut handle) }?; - Token::try_with_handle(handle) + // SAFETY: We own the handle. + let handle = unsafe { Handle::new_owned(handle)? }; + + Ok(Token::from(handle)) } } @@ -159,7 +164,7 @@ impl ThreadAttributeType<'_> { pub fn value(&self) -> *const c_void { match self { - ThreadAttributeType::ParentProcess(p) => p.handle.as_raw_ref() as *const _ as *const c_void, + ThreadAttributeType::ParentProcess(p) => p.handle.raw_as_ref() as *const _ as *const c_void, ThreadAttributeType::ExtendedFlags(v) => &v as *const _ as *const c_void, ThreadAttributeType::HandleList(h) => h.as_ptr().cast(), } diff --git a/crates/win-api-wrappers/src/token.rs b/crates/win-api-wrappers/src/token.rs index e7cceca25..987603203 100644 --- a/crates/win-api-wrappers/src/token.rs +++ b/crates/win-api-wrappers/src/token.rs @@ -27,8 +27,8 @@ use crate::undoc::{ use crate::utils::WideString; use crate::{create_impersonation_context, Error}; use windows::Win32::Foundation::{ - ERROR_ALREADY_EXISTS, ERROR_INVALID_SECURITY_DESCR, ERROR_INVALID_VARIANT, ERROR_NO_TOKEN, ERROR_SUCCESS, E_HANDLE, - HANDLE, LUID, + ERROR_ALREADY_EXISTS, ERROR_INVALID_SECURITY_DESCR, ERROR_INVALID_VARIANT, ERROR_NO_TOKEN, ERROR_SUCCESS, HANDLE, + LUID, }; use windows::Win32::Security::Authentication::Identity::EXTENDED_NAME_FORMAT; use windows::Win32::Security::{ @@ -51,18 +51,16 @@ pub struct Token { handle: Handle, } -impl Token { - pub fn try_with_handle(handle: HANDLE) -> Result { - if handle.is_invalid() { - bail!(Error::from_hresult(E_HANDLE)) - } else { - Ok(Self { handle: handle.into() }) - } +impl From for Token { + fn from(handle: Handle) -> Self { + Self { handle } } +} +impl Token { pub fn current_process_token() -> Self { Self { - handle: Handle::new(HANDLE(-4isize as *mut c_void), false), + handle: Handle::new_borrowed(HANDLE(-4isize as *mut c_void)).expect("always valid"), } } @@ -140,7 +138,10 @@ impl Token { )?; } - Ok(Self { handle: handle.into() }) + // SAFETY: We create the token, and thus own it. + let handle = unsafe { Handle::new_owned(handle)? }; + + Ok(Self::from(handle)) } pub fn duplicate( @@ -150,7 +151,7 @@ impl Token { impersonation_level: SECURITY_IMPERSONATION_LEVEL, token_type: TOKEN_TYPE, ) -> Result { - let mut handle = Default::default(); + let mut handle = HANDLE::default(); // SAFETY: Returned `handle` must be closed, which it is in its RAII wrapper. unsafe { @@ -164,7 +165,10 @@ impl Token { ) }?; - Self::try_with_handle(handle) + // SAFETY: We own the handle. + let handle = unsafe { Handle::new_owned(handle)? }; + + Ok(Self::from(handle)) } pub fn duplicate_impersonation(&self) -> Result { @@ -264,7 +268,11 @@ impl Token { } pub fn linked_token(&self) -> Result { - Self::try_with_handle(self.information_raw::(windows::Win32::Security::TokenLinkedToken)?) + let handle = self.information_raw::(windows::Win32::Security::TokenLinkedToken)?; + // SAFETY: We are responsible for closing the linked token. + let handle = unsafe { Handle::new_owned(handle)? }; + + Ok(Self::from(handle)) } pub fn username(&self, format: EXTENDED_NAME_FORMAT) -> Result { @@ -307,7 +315,10 @@ impl Token { ) }?; - Token::try_with_handle(raw_token) + // SAFETY: We own the handle. + let handle = unsafe { Handle::new_owned(raw_token)? }; + + Ok(Token::from(handle)) } pub fn statistics(&self) -> Result { diff --git a/crates/win-api-wrappers/src/utils.rs b/crates/win-api-wrappers/src/utils.rs index 0ac3eaf15..b991dffbd 100644 --- a/crates/win-api-wrappers/src/utils.rs +++ b/crates/win-api-wrappers/src/utils.rs @@ -507,10 +507,10 @@ impl Snapshot { pub fn new(flags: CREATE_TOOLHELP_SNAPSHOT_FLAGS, process_id: Option) -> Result { // SAFETY: No preconditions. Flags or process ID cannot create scenarios where unsafe behavior happens. let handle = unsafe { CreateToolhelp32Snapshot(flags, process_id.unwrap_or(0))? }; + // SAFETY: We created the handle just above and are responsibles for closing it ourselves. + let handle = unsafe { Handle::new_owned(handle)? }; - Ok(Self { - handle: Handle::new(handle, true), - }) + Ok(Self { handle }) } pub fn process_ids(&self) -> ProcessIdIterator<'_> { @@ -638,7 +638,13 @@ impl Pipe { ) }?; - Ok((Self { handle: rx.into() }, Self { handle: tx.into() })) + // SAFETY: We created the ressource above and are thus owning it. + let rx = unsafe { Handle::new_owned(rx)? }; + + // SAFETY: We created the ressource above and are thus owning it. + let tx = unsafe { Handle::new_owned(tx)? }; + + Ok((Self { handle: rx }, Self { handle: tx })) } /// Peeks the contents of the pipe in `data`, while returning the amount of bytes available on the pipe.