From 59126cfb4dcf278565fadf84f36ff39e89d5f9ae Mon Sep 17 00:00:00 2001 From: unknown Date: Mon, 23 Oct 2023 03:34:36 -0700 Subject: [PATCH] Stronger permissions for mullvad directories Set read-only permissions for authenticated users and full-access for admins for relevant mullvad directories on creation. --- mullvad-nsis/include/mullvad-nsis.h | 2 + mullvad-nsis/src/lib.rs | 39 ++- mullvad-paths/Cargo.toml | 2 + mullvad-paths/src/cache.rs | 4 +- mullvad-paths/src/lib.rs | 22 +- mullvad-paths/src/logs.rs | 12 +- mullvad-paths/src/settings.rs | 10 +- mullvad-paths/src/windows.rs | 227 +++++++++++++++++- .../nsis-plugins/src/cleanup/cleaningops.cpp | 1 + windows/nsis-plugins/src/log/log.cpp | 19 +- windows/nsis-plugins/src/log/log.vcxproj | 22 +- 11 files changed, 324 insertions(+), 36 deletions(-) diff --git a/mullvad-nsis/include/mullvad-nsis.h b/mullvad-nsis/include/mullvad-nsis.h index 5c71a517f149..5d9f1ddd232c 100644 --- a/mullvad-nsis/include/mullvad-nsis.h +++ b/mullvad-nsis/include/mullvad-nsis.h @@ -21,4 +21,6 @@ extern "C" { /// the final null terminator. Status get_system_local_appdata(uint16_t *buffer, uintptr_t *buffer_size); +Status create_privileged_directory(const uint16_t* path); + } // extern "C" diff --git a/mullvad-nsis/src/lib.rs b/mullvad-nsis/src/lib.rs index 0ac85418e8a1..24e1e86f77bc 100644 --- a/mullvad-nsis/src/lib.rs +++ b/mullvad-nsis/src/lib.rs @@ -1,6 +1,12 @@ #![cfg(all(target_arch = "x86", target_os = "windows"))] -use std::{os::windows::ffi::OsStrExt, panic::UnwindSafe, ptr}; +use std::{ + ffi::OsString, + os::windows::ffi::{OsStrExt, OsStringExt}, + panic::UnwindSafe, + path::Path, + ptr, +}; #[repr(C)] pub enum Status { @@ -11,6 +17,35 @@ pub enum Status { Panic, } +/// Max path size allowed +const MAX_PATH_SIZE: isize = 32_767; + +/// SAFETY: path needs to be a windows path encoded as a string of u16 that terminates in 0 (two nul-bytes). +/// The string is also not allowed to be greater than `MAX_PATH_SIZE`. +#[no_mangle] +pub unsafe extern "C" fn create_privileged_directory(path: *const u16) -> Status { + catch_and_log_unwind(|| { + let mut i = 0; + // Calculate the length of the path by checking when the first u16 == 0 + let len = loop { + if *(path.offset(i)) == 0 { + break i; + } else if i >= MAX_PATH_SIZE { + return Status::InvalidArguments; + } + i += 1; + }; + let path = std::slice::from_raw_parts(path, len as usize); + let path = OsString::from_wide(path); + let path = Path::new(&path); + + match mullvad_paths::windows::create_privileged_directory(path) { + Ok(()) => Status::Ok, + Err(_) => Status::OsError, + } + }) +} + /// Writes the system's app data path into `buffer` when `Status::Ok` is returned. /// If `buffer` is `null`, or if the buffer is too small, `InsufficientBufferSize` /// is returned, and the required buffer size (in chars) is returned in `buffer_size`. @@ -53,6 +88,6 @@ pub unsafe extern "C" fn get_system_local_appdata( fn catch_and_log_unwind(func: impl FnOnce() -> Status + UnwindSafe) -> Status { match std::panic::catch_unwind(func) { Ok(status) => status, - Err(_error) => Status::Panic, + Err(_) => Status::Panic, } } diff --git a/mullvad-paths/Cargo.toml b/mullvad-paths/Cargo.toml index 78ddeefa9acc..cacd8526a2a0 100644 --- a/mullvad-paths/Cargo.toml +++ b/mullvad-paths/Cargo.toml @@ -23,10 +23,12 @@ workspace = true features = [ "Win32_Foundation", "Win32_Security", + "Win32_Security_Authorization", "Win32_Storage_FileSystem", "Win32_System_Com", "Win32_System_ProcessStatus", "Win32_System_SystemServices", "Win32_System_Threading", "Win32_UI_Shell", + "Win32_System_Memory", ] diff --git a/mullvad-paths/src/cache.rs b/mullvad-paths/src/cache.rs index 8d9f4bea4541..cbd110cd59a0 100644 --- a/mullvad-paths/src/cache.rs +++ b/mullvad-paths/src/cache.rs @@ -4,10 +4,12 @@ use std::{env, path::PathBuf}; /// Creates and returns the cache directory pointed to by `MULLVAD_CACHE_DIR`, or the default /// one if that variable is unset. pub fn cache_dir() -> Result { - #[cfg(not(target_os = "macos"))] + #[cfg(not(any(target_os = "macos", target_os = "windows")))] let permissions = None; #[cfg(target_os = "macos")] let permissions = Some(std::os::unix::fs::PermissionsExt::from_mode(0o755)); + #[cfg(target_os = "windows")] + let permissions = true; crate::create_and_return(get_cache_dir, permissions) } diff --git a/mullvad-paths/src/lib.rs b/mullvad-paths/src/lib.rs index e78b02bf2d39..cdbad6bc7b48 100644 --- a/mullvad-paths/src/lib.rs +++ b/mullvad-paths/src/lib.rs @@ -1,6 +1,11 @@ #![deny(rust_2018_idioms)] -use std::{fs, io, path::PathBuf}; +#[cfg(not(target_os = "windows"))] +use std::fs; +use std::{io, path::PathBuf}; + +#[cfg(windows)] +use crate::windows::create_dir_recursive; pub type Result = std::result::Result; @@ -20,6 +25,10 @@ pub enum Error { #[error(display = "Missing %ALLUSERSPROFILE% environment variable")] NoProgramDataDir, + #[cfg(windows)] + #[error(display = "Failed to create security attributes")] + GetSecurityAttributes(#[error(source)] io::Error), + #[cfg(all(windows, feature = "deduce-system-service"))] #[error(display = "Failed to deduce system service directory")] FailedToFindSystemServiceDir(#[error(source)] io::Error), @@ -42,6 +51,7 @@ fn get_allusersprofile_dir() -> Result { } } +#[cfg(not(target_os = "windows"))] fn create_and_return( dir_fn: fn() -> Result, permissions: Option, @@ -55,6 +65,16 @@ fn create_and_return( Ok(dir) } +#[cfg(windows)] +fn create_and_return( + dir_fn: fn() -> Result, + set_security_permissions: bool, +) -> Result { + let dir = dir_fn()?; + create_dir_recursive(&dir, set_security_permissions)?; + Ok(dir) +} + mod cache; pub use crate::cache::{cache_dir, get_cache_dir, get_default_cache_dir}; diff --git a/mullvad-paths/src/logs.rs b/mullvad-paths/src/logs.rs index 91f229f48994..d7f2c58f95d7 100644 --- a/mullvad-paths/src/logs.rs +++ b/mullvad-paths/src/logs.rs @@ -8,10 +8,14 @@ use std::{env, path::PathBuf}; /// one if that variable is unset. pub fn log_dir() -> Result { #[cfg(unix)] - let permissions = Some(PermissionsExt::from_mode(0o755)); - #[cfg(not(unix))] - let permissions = None; - crate::create_and_return(get_log_dir, permissions) + { + let permissions = Some(PermissionsExt::from_mode(0o755)); + crate::create_and_return(get_log_dir, permissions) + } + #[cfg(target_os = "windows")] + { + crate::create_and_return(get_log_dir, true) + } } /// Get the logging directory, but don't try to create it. diff --git a/mullvad-paths/src/settings.rs b/mullvad-paths/src/settings.rs index b62acf2254fd..ee611b4b0e2e 100644 --- a/mullvad-paths/src/settings.rs +++ b/mullvad-paths/src/settings.rs @@ -4,7 +4,15 @@ use std::{env, path::PathBuf}; /// Creates and returns the settings directory pointed to by `MULLVAD_SETTINGS_DIR`, or the default /// one if that variable is unset. pub fn settings_dir() -> Result { - crate::create_and_return(get_settings_dir, None) + #[cfg(not(target_os = "windows"))] + { + crate::create_and_return(get_settings_dir, None) + } + + #[cfg(target_os = "windows")] + { + crate::create_and_return(get_settings_dir, false) + } } fn get_settings_dir() -> Result { diff --git a/mullvad-paths/src/windows.rs b/mullvad-paths/src/windows.rs index 5ab42ae0ad40..e41f9faec9f7 100644 --- a/mullvad-paths/src/windows.rs +++ b/mullvad-paths/src/windows.rs @@ -1,22 +1,37 @@ +use crate::{Error, Result}; use once_cell::sync::OnceCell; -use std::{io, mem, path::PathBuf, ptr}; +use std::{ + ffi::OsStr, + io, mem, + os::windows::prelude::OsStrExt, + path::{Path, PathBuf}, + ptr, +}; use widestring::{WideCStr, WideCString}; use windows_sys::{ core::{GUID, PWSTR}, Win32::{ Foundation::{ - CloseHandle, ERROR_INSUFFICIENT_BUFFER, ERROR_SUCCESS, GENERIC_READ, HANDLE, - INVALID_HANDLE_VALUE, LUID, S_OK, + CloseHandle, ERROR_INSUFFICIENT_BUFFER, ERROR_SUCCESS, GENERIC_ALL, GENERIC_READ, + HANDLE, INVALID_HANDLE_VALUE, LUID, S_OK, }, Security::{ - AdjustTokenPrivileges, CreateWellKnownSid, EqualSid, GetTokenInformation, - ImpersonateSelf, LookupPrivilegeValueW, RevertToSelf, SecurityImpersonation, TokenUser, - WinLocalSystemSid, LUID_AND_ATTRIBUTES, SE_PRIVILEGE_ENABLED, TOKEN_ADJUST_PRIVILEGES, - TOKEN_DUPLICATE, TOKEN_IMPERSONATE, TOKEN_PRIVILEGES, TOKEN_QUERY, TOKEN_USER, + self, AdjustTokenPrivileges, + Authorization::{ + SetEntriesInAclW, SetNamedSecurityInfoW, EXPLICIT_ACCESS_W, NO_MULTIPLE_TRUSTEE, + SET_ACCESS, SE_FILE_OBJECT, TRUSTEE_IS_GROUP, TRUSTEE_IS_SID, TRUSTEE_W, + }, + CreateWellKnownSid, EqualSid, GetTokenInformation, ImpersonateSelf, + LookupPrivilegeValueW, RevertToSelf, SecurityImpersonation, TokenUser, + WinAuthenticatedUserSid, WinBuiltinAdministratorsSid, WinLocalSystemSid, + LUID_AND_ATTRIBUTES, NO_INHERITANCE, SE_PRIVILEGE_ENABLED, + SUB_CONTAINERS_AND_OBJECTS_INHERIT, TOKEN_ADJUST_PRIVILEGES, TOKEN_DUPLICATE, + TOKEN_IMPERSONATE, TOKEN_PRIVILEGES, TOKEN_QUERY, TOKEN_USER, }, Storage::FileSystem::MAX_SID_SIZE, System::{ Com::CoTaskMemFree, + Memory::LocalFree, ProcessStatus::EnumProcesses, Threading::{ GetCurrentThread, OpenProcess, OpenProcessToken, OpenThreadToken, @@ -41,6 +56,204 @@ impl Drop for Handle { } } +fn get_wide_str>(string: S) -> Vec { + let wide_string: Vec = string.as_ref() + .encode_wide() + // Add null terminator + .chain(std::iter::once(0)) + .collect(); + wide_string +} + +/// Recursively creates directories, if set_security_permissions is true it will set +/// file permissions corresponding to Authenticated Users - Read Only and Administrators - Full +/// Access. Only directories that do not already exist and the leaf directory will have their permissions set. +pub fn create_dir_recursive(path: &Path, set_security_permissions: bool) -> Result<()> { + if set_security_permissions { + create_dir_with_permissions_recursive(path) + } else { + std::fs::create_dir_all(path).map_err(|e| { + Error::CreateDirFailed( + format!("Could not create directory at {}", path.display()), + e, + ) + }) + } +} + +/// If directory at path already exists, set permissions for it. +/// If directory at path don't exist but parent does, create directory and set permissions. +/// If parent directory at path does not exist then recurse and create parent directory and set +/// permissions for it, then create child directory and set permissions. +/// This does not set permissions for parent directories that already exists. +fn create_dir_with_permissions_recursive(path: &Path) -> Result<()> { + // No directory to create + if path == Path::new("") { + return Ok(()); + } + + match std::fs::create_dir(path) { + Ok(()) => { + return set_security_permissions(path); + } + // Could not find parent directory, try creating parent + Err(e) if e.kind() == io::ErrorKind::NotFound => (), + // Directory already exists, set permissions + Err(e) if e.kind() == io::ErrorKind::AlreadyExists && path.is_dir() => { + return set_security_permissions(path); + } + Err(e) => { + return Err(Error::CreateDirFailed( + format!("Could not create directory at {}", path.display()), + e, + )) + } + } + + match path.parent() { + // Create parent directory + Some(parent) => create_dir_with_permissions_recursive(parent)?, + None => { + // Reached the top of the tree but when creating directories only got NotFound for some reason + return Err(Error::CreateDirFailed( + path.display().to_string(), + io::Error::new( + io::ErrorKind::Other, + "reached top of directory tree but could not create directory", + ), + )); + } + } + + std::fs::create_dir(path)?; + set_security_permissions(path) +} + +/// Recursively creates directories for the given path with permissions that give full access to admins and read only access to authenticated users. +/// If any of the directories already exist this will not return an error, instead it will apply the permissions and if successful return Ok(()). +pub fn create_privileged_directory(path: &Path) -> Result<()> { + create_dir_with_permissions_recursive(path) +} + +/// Sets security permissions for path such that admin has full ownership and access while authenticated users only have read access. +fn set_security_permissions(path: &Path) -> Result<()> { + let wide_path = get_wide_str(path); + let security_information = Security::DACL_SECURITY_INFORMATION + | Security::PROTECTED_DACL_SECURITY_INFORMATION + | Security::GROUP_SECURITY_INFORMATION + | Security::OWNER_SECURITY_INFORMATION; + + let mut admin_psid = [0u8; MAX_SID_SIZE as usize]; + let mut admin_psid_len = u32::try_from(admin_psid.len()).unwrap(); + if unsafe { + CreateWellKnownSid( + WinBuiltinAdministratorsSid, + ptr::null_mut(), + admin_psid.as_mut_ptr() as _, + &mut admin_psid_len, + ) + } == 0 + { + return Err(Error::SetDirPermissionFailed( + String::from("Could not create admin SID"), + io::Error::last_os_error(), + )); + } + + let trustee = TRUSTEE_W { + pMultipleTrustee: ptr::null_mut(), + MultipleTrusteeOperation: NO_MULTIPLE_TRUSTEE, + TrusteeForm: TRUSTEE_IS_SID, + TrusteeType: TRUSTEE_IS_GROUP, + ptstrName: admin_psid.as_mut_ptr() as *mut _, + }; + + let admin_ea = EXPLICIT_ACCESS_W { + grfAccessPermissions: GENERIC_ALL, + grfAccessMode: SET_ACCESS, + grfInheritance: NO_INHERITANCE | SUB_CONTAINERS_AND_OBJECTS_INHERIT, + Trustee: trustee, + }; + + let mut au_psid = [0u8; MAX_SID_SIZE as usize]; + let mut au_psid_len = u32::try_from(au_psid.len()).unwrap(); + if unsafe { + CreateWellKnownSid( + WinAuthenticatedUserSid, + ptr::null_mut(), + au_psid.as_mut_ptr() as _, + &mut au_psid_len, + ) + } == 0 + { + return Err(Error::SetDirPermissionFailed( + String::from("Could not create authenticated users SID"), + io::Error::last_os_error(), + )); + } + + let trustee = TRUSTEE_W { + pMultipleTrustee: ptr::null_mut(), + MultipleTrusteeOperation: NO_MULTIPLE_TRUSTEE, + TrusteeForm: TRUSTEE_IS_SID, + TrusteeType: TRUSTEE_IS_GROUP, + ptstrName: au_psid.as_mut_ptr() as *mut _, + }; + + let authenticated_users_ea = EXPLICIT_ACCESS_W { + grfAccessPermissions: GENERIC_READ, + grfAccessMode: SET_ACCESS, + grfInheritance: NO_INHERITANCE | SUB_CONTAINERS_AND_OBJECTS_INHERIT, + Trustee: trustee, + }; + + let ea_entries = [admin_ea, authenticated_users_ea]; + let mut new_dacl = ptr::null_mut(); + + let result = unsafe { + SetEntriesInAclW( + u32::try_from(ea_entries.len()).unwrap(), + ea_entries.as_ptr(), + ptr::null(), + &mut new_dacl, + ) + }; + if result != ERROR_SUCCESS { + return Err(Error::SetDirPermissionFailed( + String::from("SetEntriesInAclW failed"), + io::Error::from_raw_os_error( + i32::try_from(result).expect("result does not fit in i32"), + ), + )); + } + // new_dacl is now allocated and must be freed with FreeLocal + + let result = unsafe { + SetNamedSecurityInfoW( + wide_path.as_ptr(), + SE_FILE_OBJECT, + security_information, + admin_psid.as_mut_ptr() as *mut _, + admin_psid.as_mut_ptr() as *mut _, + new_dacl, + ptr::null(), + ) + }; + + unsafe { LocalFree(new_dacl as isize) }; + + if result != ERROR_SUCCESS { + Err(Error::SetDirPermissionFailed( + String::from("SetNamedSecurityInfoW failed"), + io::Error::from_raw_os_error( + i32::try_from(result).expect("result does not fit in i32"), + ), + )) + } else { + Ok(()) + } +} + /// Get local AppData path for the system service user. pub fn get_system_service_appdata() -> io::Result { static APPDATA_PATH: OnceCell = OnceCell::new(); diff --git a/windows/nsis-plugins/src/cleanup/cleaningops.cpp b/windows/nsis-plugins/src/cleanup/cleaningops.cpp index d95f275bca29..917b9356cf94 100644 --- a/windows/nsis-plugins/src/cleanup/cleaningops.cpp +++ b/windows/nsis-plugins/src/cleanup/cleaningops.cpp @@ -123,6 +123,7 @@ void MigrateCacheServiceUser() common::fs::Mkdir(newCacheDir); const auto localAppData = GetSystemUserLocalAppData(); + const auto oldCacheDir = std::filesystem::path(localAppData).append(L"Mullvad VPN"); common::fs::ScopedNativeFileSystem nativeFileSystem; diff --git a/windows/nsis-plugins/src/log/log.cpp b/windows/nsis-plugins/src/log/log.cpp index 69402401373b..80338b80fe8a 100644 --- a/windows/nsis-plugins/src/log/log.cpp +++ b/windows/nsis-plugins/src/log/log.cpp @@ -3,7 +3,6 @@ #include #include #include -#include #include #include #include @@ -13,6 +12,7 @@ #include #include #include +#include Logger *g_logger = nullptr; @@ -294,19 +294,12 @@ void __declspec(dllexport) NSISCALL SetLogTarget FOLDERID_ProgramData)); logpath.append(L"Mullvad VPN"); - if (FALSE == CreateDirectoryW(logpath.c_str(), nullptr)) - { - if (ERROR_ALREADY_EXISTS != GetLastError()) - { - std::wstringstream ss; - - ss << L"Cannot create folder: " - << L"\"" - << logpath - << L"\""; + auto logpath_wstring = logpath.wstring(); + const wchar_t* w_path = logpath_wstring.c_str(); - THROW_ERROR(common::string::ToAnsi(ss.str()).c_str()); - } + if (Status::Ok != create_privileged_directory(reinterpret_cast(w_path))) + { + THROW_ERROR("Failed to create log directory"); } logpath.append(logfile); diff --git a/windows/nsis-plugins/src/log/log.vcxproj b/windows/nsis-plugins/src/log/log.vcxproj index ca6349b7083a..7509919f195f 100644 --- a/windows/nsis-plugins/src/log/log.vcxproj +++ b/windows/nsis-plugins/src/log/log.vcxproj @@ -61,7 +61,7 @@ true WIN32;_DEBUG;LOG_EXPORTS;_WINDOWS;_USRDLL;%(PreprocessorDefinitions) true - $(ProjectDir)../../../../dist-assets/binaries/x86_64-pc-windows-msvc/;$(ProjectDir)../../../windows-libraries/src/ + $(ProjectDir)../../../../mullvad-nsis/include;$(ProjectDir)../../../../dist-assets/binaries/x86_64-pc-windows-msvc/;$(ProjectDir)../../../windows-libraries/src/ stdcpp20 MultiThreadedDebug @@ -69,11 +69,15 @@ Windows true false - $(ProjectDir)../../../../dist-assets/binaries/x86_64-pc-windows-msvc/nsis/;$(SolutionDir)bin\$(Platform)-$(Configuration)\ - version.lib;libcommon.lib;pluginapi-x86-unicode.lib;kernel32.lib;user32.lib;gdi32.lib;winspool.lib;comdlg32.lib;advapi32.lib;shell32.lib;ole32.lib;oleaut32.lib;uuid.lib;odbc32.lib;odbccp32.lib;%(AdditionalDependencies) + $(ProjectDir)../../../../target/i686-pc-windows-msvc/release;$(ProjectDir)../../../../dist-assets/binaries/x86_64-pc-windows-msvc/nsis/;$(SolutionDir)bin\$(Platform)-$(Configuration)\ + mullvad_nsis.lib;psapi.lib;version.lib;libcommon.lib;pluginapi-x86-unicode.lib;kernel32.lib;user32.lib;gdi32.lib;winspool.lib;comdlg32.lib;advapi32.lib;shell32.lib;ole32.lib;oleaut32.lib;uuid.lib;odbc32.lib;odbccp32.lib;%(AdditionalDependencies) libc.lib log.def + + cargo build --target i686-pc-windows-msvc --release -p mullvad-nsis + Build mullvad-nsis library + @@ -85,7 +89,7 @@ true WIN32;NDEBUG;LOG_EXPORTS;_WINDOWS;_USRDLL;%(PreprocessorDefinitions) true - $(ProjectDir)../../../../dist-assets/binaries/x86_64-pc-windows-msvc/;$(ProjectDir)../../../windows-libraries/src/ + $(ProjectDir)../../../../mullvad-nsis/include;$(ProjectDir)../../../../dist-assets/binaries/x86_64-pc-windows-msvc/;$(ProjectDir)../../../windows-libraries/src/ MultiThreaded stdcpp20 @@ -95,11 +99,15 @@ true true false - $(ProjectDir)../../../../dist-assets/binaries/x86_64-pc-windows-msvc/nsis/;$(SolutionDir)bin\$(Platform)-$(Configuration)\ - version.lib;libcommon.lib;pluginapi-x86-unicode.lib;kernel32.lib;user32.lib;gdi32.lib;winspool.lib;comdlg32.lib;advapi32.lib;shell32.lib;ole32.lib;oleaut32.lib;uuid.lib;odbc32.lib;odbccp32.lib;%(AdditionalDependencies) + $(ProjectDir)../../../../target/i686-pc-windows-msvc/release;$(ProjectDir)../../../../dist-assets/binaries/x86_64-pc-windows-msvc/nsis/;$(SolutionDir)bin\$(Platform)-$(Configuration)\ + mullvad_nsis.lib;psapi.lib;version.lib;libcommon.lib;pluginapi-x86-unicode.lib;kernel32.lib;user32.lib;gdi32.lib;winspool.lib;comdlg32.lib;advapi32.lib;shell32.lib;ole32.lib;oleaut32.lib;uuid.lib;odbc32.lib;odbccp32.lib;%(AdditionalDependencies) libc.lib log.def + + cargo build --target i686-pc-windows-msvc --release -p mullvad-nsis + Build mullvad-nsis library + @@ -122,4 +130,4 @@ - \ No newline at end of file +