From 6c2b51a621272a86cd9e8e2af4f340517e8e34b3 Mon Sep 17 00:00:00 2001 From: David Knaack Date: Thu, 21 Apr 2022 20:59:00 +0200 Subject: [PATCH] refactor(windows): replace winapi with windows (#3690) --- Cargo.lock | 21 ++++- Cargo.toml | 11 ++- src/modules/utils/directory_nix.rs | 13 +-- src/modules/utils/directory_win.rs | 147 ++++++++++++++--------------- 4 files changed, 107 insertions(+), 85 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index c6199c5e..ad14c1aa 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1947,7 +1947,7 @@ dependencies = [ "urlencoding", "versions", "which", - "winapi", + "windows 0.35.0", "winres", "yaml-rust", ] @@ -2320,9 +2320,9 @@ dependencies = [ [[package]] name = "vtparse" -version = "0.6.0" +version = "0.6.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8f41c9314c4dde1f43dd0c46c67bb5ae73850ce11eebaf7d8b912e178bda5401" +checksum = "36ce903972602c84dd48f488cdce39edcba03a93b7ca67b146ae862568f48c5c" dependencies = [ "utf8parse", ] @@ -2408,6 +2408,19 @@ dependencies = [ "windows_x86_64_msvc 0.24.0", ] +[[package]] +name = "windows" +version = "0.35.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "08746b4b7ac95f708b3cccceb97b7f9a21a8916dd47fc99b0e6aaf7208f26fd7" +dependencies = [ + "windows_aarch64_msvc", + "windows_i686_gnu 0.35.0", + "windows_i686_msvc 0.35.0", + "windows_x86_64_gnu 0.35.0", + "windows_x86_64_msvc 0.35.0", +] + [[package]] name = "windows-sys" version = "0.35.0" @@ -2491,7 +2504,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "007a0353840b23e0c6dc73e5b962ff58ed7f6bc9ceff3ce7fe6fbad8d496edf4" dependencies = [ "strum", - "windows", + "windows 0.24.0", "xml-rs", ] diff --git a/Cargo.toml b/Cargo.toml index d9c292d6..a470023a 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -92,9 +92,18 @@ optional = true features = ["preserve_order", "indexmap"] [target.'cfg(windows)'.dependencies] -winapi = { version = "0.3.9", features = ["winuser", "securitybaseapi", "processthreadsapi", "handleapi", "impl-default"] } deelevate = "0.2.0" +[target.'cfg(windows)'.dependencies.windows] +version = "0.35.0" +features = [ + "Win32_Foundation", + "Win32_UI_Shell", + "Win32_Security", + "Win32_System_Threading", + "Win32_Storage_FileSystem", +] + [target.'cfg(not(windows))'.dependencies] nix = "0.23.1" diff --git a/src/modules/utils/directory_nix.rs b/src/modules/utils/directory_nix.rs index 2e048914..920d72cb 100644 --- a/src/modules/utils/directory_nix.rs +++ b/src/modules/utils/directory_nix.rs @@ -13,8 +13,9 @@ use std::path::Path; /// 2a) (not implemented on macOS) one of the supplementary groups of the current user is the /// directory group owner and whether it has write access /// 3) 'others' part of the access mask has the write access -pub fn is_write_allowed(folder_path: &Path) -> Result { - let meta = fs::metadata(folder_path).map_err(|_| "Unable to stat() directory")?; +pub fn is_write_allowed(folder_path: &Path) -> Result { + let meta = + fs::metadata(folder_path).map_err(|e| format!("Unable to stat() directory: {e:?}"))?; let perms = meta.permissions().mode(); let euid = Uid::effective(); @@ -54,9 +55,9 @@ mod tests { #[ignore] fn read_only_test() { assert_eq!(is_write_allowed(Path::new("/etc")), Ok(false)); - assert_eq!( - is_write_allowed(Path::new("/i_dont_exist")), - Err("Unable to stat() directory") - ); + assert!(match is_write_allowed(Path::new("/i_dont_exist")) { + Ok(_) => false, + Err(e) => e.starts_with("Unable to stat() directory"), + }); } } diff --git a/src/modules/utils/directory_win.rs b/src/modules/utils/directory_win.rs index 53253c7d..f7de2a85 100644 --- a/src/modules/utils/directory_win.rs +++ b/src/modules/utils/directory_win.rs @@ -1,29 +1,32 @@ -extern crate winapi; +use std::{ffi::c_void, mem, os::windows::ffi::OsStrExt, path::Path}; -use std::mem; -use std::os::windows::ffi::OsStrExt; -use std::path::Path; - -use winapi::shared::minwindef::{BOOL, DWORD}; -use winapi::um::handleapi; -use winapi::um::processthreadsapi; -use winapi::um::securitybaseapi; -use winapi::um::winnt::{ - SecurityImpersonation, BOOLEAN, DACL_SECURITY_INFORMATION, FILE_ALL_ACCESS, - FILE_GENERIC_EXECUTE, FILE_GENERIC_READ, FILE_GENERIC_WRITE, GENERIC_MAPPING, - GROUP_SECURITY_INFORMATION, HANDLE, LPCWSTR, OWNER_SECURITY_INFORMATION, PRIVILEGE_SET, - STANDARD_RIGHTS_READ, TOKEN_DUPLICATE, TOKEN_IMPERSONATE, TOKEN_QUERY, +use windows::{ + core::PCWSTR, + Win32::{ + Foundation::{CloseHandle, ERROR_INSUFFICIENT_BUFFER, HANDLE}, + Security::{ + AccessCheck, DuplicateToken, GetFileSecurityW, MapGenericMask, SecurityImpersonation, + DACL_SECURITY_INFORMATION, GENERIC_MAPPING, GROUP_SECURITY_INFORMATION, + OWNER_SECURITY_INFORMATION, PRIVILEGE_SET, PSECURITY_DESCRIPTOR, TOKEN_DUPLICATE, + TOKEN_IMPERSONATE, TOKEN_QUERY, TOKEN_READ_CONTROL, + }, + Storage::FileSystem::{ + FILE_ALL_ACCESS, FILE_GENERIC_EXECUTE, FILE_GENERIC_READ, FILE_GENERIC_WRITE, + }, + System::Threading::{GetCurrentProcess, OpenProcessToken}, + UI::Shell::PathIsNetworkPathW, + }, }; - /// Checks if the current user has write access right to the `folder_path` /// /// First, the function extracts DACL from the given directory and then calls `AccessCheck` against /// the current process access token and directory's security descriptor. /// Does not work for network drives and always returns true -pub fn is_write_allowed(folder_path: &Path) -> std::result::Result { - let folder_name: Vec = folder_path.as_os_str().encode_wide().chain([0]).collect(); +pub fn is_write_allowed(folder_path: &Path) -> std::result::Result { + let wpath_vec: Vec = folder_path.as_os_str().encode_wide().chain([0]).collect(); + let wpath = PCWSTR(wpath_vec.as_ptr()); - if is_network_path(&folder_name) { + if unsafe { PathIsNetworkPathW(wpath) }.as_bool() { log::info!( "Directory '{:?}' is a network drive, unable to check write permissions. See #1506 for details", folder_path @@ -31,79 +34,84 @@ pub fn is_write_allowed(folder_path: &Path) -> std::result::Result (), + result => return Err(format!("GetFileSecurityW returned unexpected return value when asked for the security descriptor size: {:?}", result)), } - let mut buf: Vec = Vec::with_capacity(length as usize); + let mut buf = vec![0u8; length as usize]; + let psecurity_descriptor = PSECURITY_DESCRIPTOR(buf.as_mut_ptr() as *mut c_void); let rc = unsafe { - securitybaseapi::GetFileSecurityW( - folder_name.as_ptr(), - OWNER_SECURITY_INFORMATION | GROUP_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION, - buf.as_mut_ptr().cast::(), + GetFileSecurityW( + wpath, + (OWNER_SECURITY_INFORMATION | GROUP_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION).0, + psecurity_descriptor, length, &mut length, ) }; - if rc != 1 { - return Err("GetFileSecurityW failed to retrieve the security descriptor"); + if let Err(e) = rc.ok() { + return Err(format!( + "GetFileSecurityW failed to retrieve the security descriptor: {e:?}" + )); } - let mut token: HANDLE = 0 as HANDLE; + let mut token = HANDLE::default(); let rc = unsafe { - processthreadsapi::OpenProcessToken( - processthreadsapi::GetCurrentProcess(), - TOKEN_IMPERSONATE | TOKEN_QUERY | TOKEN_DUPLICATE | STANDARD_RIGHTS_READ, + OpenProcessToken( + GetCurrentProcess(), + TOKEN_IMPERSONATE | TOKEN_QUERY | TOKEN_DUPLICATE | TOKEN_READ_CONTROL, &mut token, ) }; - if rc != 1 { - return Err("OpenProcessToken failed to retrieve current process' security token"); + if let Err(e) = rc.ok() { + return Err(format!( + "OpenProcessToken failed to retrieve current process' security token: {e:?}" + )); } - let mut impersonated_token: HANDLE = 0 as HANDLE; - let rc = unsafe { - securitybaseapi::DuplicateToken(token, SecurityImpersonation, &mut impersonated_token) - }; - if rc != 1 { - unsafe { handleapi::CloseHandle(token) }; - return Err("DuplicateToken failed"); + let mut impersonated_token = HANDLE::default(); + let rc = unsafe { DuplicateToken(token, SecurityImpersonation, &mut impersonated_token) }; + + if let Err(e) = rc.ok() { + unsafe { CloseHandle(token) }; + return Err(format!("DuplicateToken failed: {e:?}")); } - let mut mapping: GENERIC_MAPPING = GENERIC_MAPPING { - GenericRead: FILE_GENERIC_READ, - GenericWrite: FILE_GENERIC_WRITE, - GenericExecute: FILE_GENERIC_EXECUTE, - GenericAll: FILE_ALL_ACCESS, + let mapping = GENERIC_MAPPING { + GenericRead: FILE_GENERIC_READ.0, + GenericWrite: FILE_GENERIC_WRITE.0, + GenericExecute: FILE_GENERIC_EXECUTE.0, + GenericAll: FILE_ALL_ACCESS.0, }; let mut priviledges: PRIVILEGE_SET = PRIVILEGE_SET::default(); - let mut priv_size = mem::size_of::() as DWORD; - let mut granted_access: DWORD = 0; - let mut access_rights: DWORD = FILE_GENERIC_WRITE; - let mut result: BOOL = 0; - unsafe { securitybaseapi::MapGenericMask(&mut access_rights, &mut mapping) }; + let mut priv_size = mem::size_of::() as _; + let mut granted_access = 0; + let mut access_rights = FILE_GENERIC_WRITE; + let mut result = 0; + unsafe { MapGenericMask(&mut access_rights.0, &mapping) }; let rc = unsafe { - securitybaseapi::AccessCheck( - buf.as_mut_ptr().cast::(), + AccessCheck( + psecurity_descriptor, impersonated_token, - access_rights, - &mut mapping, + access_rights.0, + &mapping, &mut priviledges, &mut priv_size, &mut granted_access, @@ -111,22 +119,13 @@ pub fn is_write_allowed(folder_path: &Path) -> std::result::Result BOOLEAN; -} - -fn is_network_path(folder_path: &[u16]) -> bool { - unsafe { PathIsNetworkPathW(folder_path.as_ptr()) == 1 } -}