refactor: fix soundness hole in Handle API (#998)

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()?;
 

crates/devolutions-pedm-shell-ext/src/lib_win.rs

@@ -179,7 +179,7 @@ fn find_main_explorer(session: u32) -> Option<u32> {
     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()

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;
 

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 }
     }
 }
 

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<Application> {
-    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()?;
 

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,26 +23,69 @@ 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<Self, crate::Error> {
+        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<Self, crate::Error> {
+        // 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<Self, crate::Error> {
+        // 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
     }
 
     pub fn leak(&mut self) {
         self.owned = false;
     }
 
-    pub fn try_clone(&self) -> Result<Self> {
+    pub fn try_clone(&self) -> anyhow::Result<Self> {
         // 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<HANDLE> 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<Self, Self::Error> {
-        let handle = Handle {
+    fn try_from(value: &BorrowedHandle<'_>) -> anyhow::Result<Self, Self::Error> {
+        let handle = Self {
             raw: HANDLE(value.as_raw_handle().cast()),
             owned: false,
         };
 
-        Self::try_clone(&handle)
+        handle.try_clone()
     }
 }
 
 impl TryFrom<BorrowedHandle<'_>> for Handle {
     type Error = anyhow::Error;
 
-    fn try_from(value: BorrowedHandle<'_>) -> Result<Self, Self::Error> {
+    fn try_from(value: BorrowedHandle<'_>) -> anyhow::Result<Self, Self::Error> {
         Self::try_from(&value)
     }
 }
 
 impl From<OwnedHandle> 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,
+        }
     }
 }
 

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<Handle> 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<Self> {
-        // SAFETY: No preconditions. Handle is closed with RAII wrapper.
+    pub fn get_by_pid(pid: u32, desired_access: PROCESS_ACCESS_RIGHTS) -> Result<Self> {
+        // 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<Self> {
-        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<PathBuf> {
@@ -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<Allocation<'_>> {
@@ -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<Token> {
-        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<u32>) -> Result<WAIT_EVENT> {
@@ -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,
     })

crates/win-api-wrappers/src/security/privilege.rs

@@ -93,7 +93,7 @@ pub fn find_token_with_privilege(privilege: LUID) -> Result<Option<Token>> {
     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) {