From 4ddec81f282818838cad24252ff03ed85d57ea8a Mon Sep 17 00:00:00 2001 From: Reactor Scram Date: Thu, 15 Aug 2024 17:36:18 -0500 Subject: [PATCH] fix(gui-client/windows): allow GUI to run as admin again (#6308) Closes #6305 too I couldn't find the ticket for this so I'm not sure which customers are affected. --------- Signed-off-by: Reactor Scram Co-authored-by: Thomas Eizinger --- rust/gui-client/docs/manual_testing.md | 16 ++- rust/gui-client/src-tauri/src/client.rs | 2 +- .../src-tauri/src/client/elevation.rs | 98 +++---------------- rust/headless-client/src/ipc_service.rs | 8 +- rust/headless-client/src/ipc_service/linux.rs | 11 ++- .../src/ipc_service/windows.rs | 74 +++++++++++++- website/src/components/Changelog/GUI.tsx | 3 + 7 files changed, 119 insertions(+), 93 deletions(-) diff --git a/rust/gui-client/docs/manual_testing.md b/rust/gui-client/docs/manual_testing.md index ec2a92583..96a922baa 100644 --- a/rust/gui-client/docs/manual_testing.md +++ b/rust/gui-client/docs/manual_testing.md @@ -34,9 +34,19 @@ If the client stops running while signed in, then the token may be stored in Win # Permissions -- [ ] Given a production exe, when you run it normally, then it will ask to escalate to Admin privileges ([#2751](https://github.com/firezone/firezone/issues/2751)) -- [ ] Given the client is running, when you authenticate in the browser, then the client will not ask for privileges again ([#2751](https://github.com/firezone/firezone/issues/2751)) -- (Running as an unprivileged user is not supported yet) ([#2751](https://github.com/firezone/firezone/issues/2751)) +## Linux Permissions + +- [ ] The IPC service with `run-debug` can NOT run as a normal user +- [ ] The IPC service with `run-debug` can run with `sudo` +- [ ] The GUI can run as a normal user +- [ ] The GUI can NOT run with `sudo` + +## Windows Permissions + +- [ ] The IPC service with `run-debug` can NOT run as a normal user +- [ ] The IPC service with `run-debug` can run as admin +- [ ] The GUI can run as a normal user +- [ ] The GUI can run as admin # Auth flow diff --git a/rust/gui-client/src-tauri/src/client.rs b/rust/gui-client/src-tauri/src/client.rs index e9d507928..93f6592fa 100644 --- a/rust/gui-client/src-tauri/src/client.rs +++ b/rust/gui-client/src-tauri/src/client.rs @@ -41,7 +41,7 @@ pub(crate) fn run() -> Result<()> { if cli.no_deep_links { return run_gui(cli); } - match elevation::is_normal_user() { + match elevation::gui_check() { // Our elevation is correct (not elevated), just run the GUI Ok(true) => run_gui(cli), Ok(false) => bail!("The GUI should run as a normal user, not elevated"), diff --git a/rust/gui-client/src-tauri/src/client/elevation.rs b/rust/gui-client/src-tauri/src/client/elevation.rs index 9f3d5ddc4..6e9feb788 100644 --- a/rust/gui-client/src-tauri/src/client/elevation.rs +++ b/rust/gui-client/src-tauri/src/client/elevation.rs @@ -1,18 +1,17 @@ -pub(crate) use imp::is_normal_user; +pub(crate) use platform::gui_check; #[cfg(target_os = "linux")] -mod imp { +mod platform { use crate::client::gui::Error; use anyhow::{Context as _, Result}; use firezone_headless_client::FIREZONE_GROUP; - /// Returns true if we're running without root privileges + /// Returns true if all permissions are correct for the GUI to run /// /// Everything that needs root / admin powers happens in the IPC services, /// so for security and practicality reasons the GUIs must be non-root. /// (In Linux by default a root GUI app barely works at all) - pub(crate) fn is_normal_user() -> Result { - // Must use `eprintln` here because `tracing` won't be initialized yet. + pub(crate) fn gui_check() -> Result { let user = std::env::var("USER").context("USER env var should be set")?; if user == "root" { return Ok(false); @@ -35,97 +34,26 @@ mod imp { } } -// Stub only -#[cfg(target_os = "macos")] -mod imp { - /// Placeholder for cargo check on macOS - pub(crate) fn is_normal_user() -> anyhow::Result { - Ok(true) - } -} - #[cfg(target_os = "windows")] -mod imp { +mod platform { use crate::client::gui::Error; - use anyhow::{Context as _, Result}; - use std::{ffi::c_void, mem::size_of}; - use windows::Win32::{ - Foundation::{CloseHandle, HANDLE}, - Security::{GetTokenInformation, TokenElevation, TOKEN_ELEVATION, TOKEN_QUERY}, - System::Threading::{GetCurrentProcess, OpenProcessToken}, - }; + use anyhow::Result; // Returns true on Windows /// - /// On Windows, checking for elevation is complicated, - /// so it just always returns true. The Windows GUI does work correctly even if - /// elevated, so we should warn users that it doesn't need elevation, but it's - /// not a show-stopper if they accidentally "Run as admin". + /// On Windows, some users will run as admin, and the GUI does work correctly, + /// unlike on Linux where most distros don't like to mix root GUI apps with X11 / Wayland. #[allow(clippy::unnecessary_wraps)] - pub(crate) fn is_normal_user() -> Result { - let token = ProcessToken::our_process().map_err(Error::Other)?; - let elevated = token.is_elevated().map_err(Error::Other)?; - Ok(!elevated) - } - - // https://stackoverflow.com/questions/8046097/how-to-check-if-a-process-has-the-administrative-rights/8196291#8196291 - struct ProcessToken { - inner: HANDLE, - } - - impl ProcessToken { - fn our_process() -> Result { - // SAFETY: Calling C APIs is unsafe - // `GetCurrentProcess` returns a pseudo-handle which does not need to be closed. - // Docs say nothing about thread safety. - let our_proc = unsafe { GetCurrentProcess() }; - let mut inner = HANDLE::default(); - // SAFETY: We just created `inner`, and moving a `HANDLE` is safe. - // We assume that if `OpenProcessToken` fails, we don't need to close the `HANDLE`. - // Docs say nothing about threads or safety: - unsafe { OpenProcessToken(our_proc, TOKEN_QUERY, &mut inner) } - .context("`OpenProcessToken` failed")?; - Ok(Self { inner }) - } - - fn is_elevated(&self) -> Result { - let mut elevation = TOKEN_ELEVATION::default(); - let token_elevation_sz = u32::try_from(size_of::()) - .expect("`TOKEN_ELEVATION` size should fit into a u32"); - let mut return_size = 0u32; - // SAFETY: Docs say nothing about threads or safety - // The type of `elevation` varies based on the 2nd parameter, but we hard-coded that. - // It should be fine. - unsafe { - GetTokenInformation( - self.inner, - TokenElevation, - Some(&mut elevation as *mut _ as *mut c_void), - token_elevation_sz, - &mut return_size as *mut _, - ) - }?; - Ok(elevation.TokenIsElevated == 1) - } - } - - impl Drop for ProcessToken { - fn drop(&mut self) { - // SAFETY: We got `inner` from `OpenProcessToken` and didn't mutate it after that. - // Closing a pseudo-handle is a harmless no-op, though this is a real handle. - // - // > The pseudo handle need not be closed when it is no longer needed. Calling the CloseHandle function with a pseudo handle has no effect. If the pseudo handle is duplicated by DuplicateHandle, the duplicate handle must be closed. - unsafe { CloseHandle(self.inner) }.expect("`CloseHandle` should always succeed"); - self.inner = HANDLE::default(); - } + pub(crate) fn gui_check() -> Result { + Ok(true) } } #[cfg(test)] mod tests { - // Make sure it doesn't crash + // Make sure it doesn't panic #[test] - fn is_normal_user() { - super::is_normal_user().unwrap(); + fn gui_check_no_panic() { + super::gui_check().ok(); } } diff --git a/rust/headless-client/src/ipc_service.rs b/rust/headless-client/src/ipc_service.rs index ccd6c0237..ac346a0cb 100644 --- a/rust/headless-client/src/ipc_service.rs +++ b/rust/headless-client/src/ipc_service.rs @@ -2,7 +2,7 @@ use crate::{ device_id, dns_control::DnsController, known_dirs, signals, CallbackHandler, CliCommon, ConnlibMsg, IpcServerMsg, }; -use anyhow::{Context as _, Result}; +use anyhow::{bail, Context as _, Result}; use clap::Parser; use connlib_client_shared::{keypair, ConnectArgs, LoginUrl, Session}; use firezone_bin_shared::{ @@ -106,6 +106,9 @@ fn run_debug_ipc_service(cli: Cli) -> Result<()> { git_version = firezone_bin_shared::GIT_VERSION, system_uptime_seconds = crate::uptime::get().map(|dur| dur.as_secs()), ); + if !platform::elevation_check()? { + bail!("IPC service failed its elevation check, try running as admin / root"); + } let rt = tokio::runtime::Runtime::new()?; let _guard = rt.enter(); let mut signals = signals::Terminate::new()?; @@ -124,6 +127,9 @@ fn run_smoke_test() -> Result<()> { #[cfg(debug_assertions)] fn run_smoke_test() -> Result<()> { crate::setup_stdout_logging()?; + if !platform::elevation_check()? { + bail!("IPC service failed its elevation check, try running as admin / root"); + } let rt = tokio::runtime::Runtime::new()?; let _guard = rt.enter(); let mut dns_controller = DnsController { diff --git a/rust/headless-client/src/ipc_service/linux.rs b/rust/headless-client/src/ipc_service/linux.rs index c583e8b04..1ad5e8544 100644 --- a/rust/headless-client/src/ipc_service/linux.rs +++ b/rust/headless-client/src/ipc_service/linux.rs @@ -7,8 +7,8 @@ use anyhow::{bail, Result}; /// Linux uses the CLI args from here, Windows does not pub(crate) fn run_ipc_service(cli: CliCommon) -> Result<()> { let _handle = super::setup_logging(cli.log_dir)?; - if !nix::unistd::getuid().is_root() { - anyhow::bail!("This is the IPC service binary, it's not meant to run interactively."); + if !elevation_check()? { + bail!("IPC service failed its elevation check, try running as admin / root"); } let rt = tokio::runtime::Runtime::new()?; let _guard = rt.enter(); @@ -17,6 +17,13 @@ pub(crate) fn run_ipc_service(cli: CliCommon) -> Result<()> { rt.block_on(super::ipc_listen(cli.dns_control, &mut signals)) } +/// Returns true if the IPC service can run properly +// Fallible on Windows +#[allow(clippy::unnecessary_wraps)] +pub(crate) fn elevation_check() -> Result { + Ok(nix::unistd::getuid().is_root()) +} + pub(crate) fn install_ipc_service() -> Result<()> { bail!("`install_ipc_service` not implemented and not needed on Linux") } diff --git a/rust/headless-client/src/ipc_service/windows.rs b/rust/headless-client/src/ipc_service/windows.rs index 4ec0c26bb..08c0b77c7 100644 --- a/rust/headless-client/src/ipc_service/windows.rs +++ b/rust/headless-client/src/ipc_service/windows.rs @@ -2,8 +2,18 @@ use crate::CliCommon; use anyhow::{bail, Context as _, Result}; use firezone_bin_shared::platform::DnsControlMethod; use futures::future::{self, Either}; -use std::{ffi::OsString, pin::pin, time::Duration}; +use std::{ + ffi::{c_void, OsString}, + mem::size_of, + pin::pin, + time::Duration, +}; use tokio::sync::mpsc; +use windows::Win32::{ + Foundation::{CloseHandle, HANDLE}, + Security::{GetTokenInformation, TokenElevation, TOKEN_ELEVATION, TOKEN_QUERY}, + System::Threading::{GetCurrentProcess, OpenProcessToken}, +}; use windows_service::{ service::{ ServiceAccess, ServiceControl, ServiceControlAccept, ServiceErrorControl, ServiceExitCode, @@ -16,6 +26,65 @@ use windows_service::{ const SERVICE_NAME: &str = "firezone_client_ipc"; const SERVICE_TYPE: ServiceType = ServiceType::OWN_PROCESS; +/// Returns true if the IPC service can run properly +pub(crate) fn elevation_check() -> Result { + let token = ProcessToken::our_process()?; + let elevated = token.is_elevated()?; + Ok(elevated) +} + +// https://stackoverflow.com/questions/8046097/how-to-check-if-a-process-has-the-administrative-rights/8196291#8196291 +struct ProcessToken { + inner: HANDLE, +} + +impl ProcessToken { + fn our_process() -> Result { + // SAFETY: Calling C APIs is unsafe + // `GetCurrentProcess` returns a pseudo-handle which does not need to be closed. + // Docs say nothing about thread safety. + let our_proc = unsafe { GetCurrentProcess() }; + let mut inner = HANDLE::default(); + // SAFETY: We just created `inner`, and moving a `HANDLE` is safe. + // We assume that if `OpenProcessToken` fails, we don't need to close the `HANDLE`. + // Docs say nothing about threads or safety: + unsafe { OpenProcessToken(our_proc, TOKEN_QUERY, &mut inner) } + .context("`OpenProcessToken` failed")?; + Ok(Self { inner }) + } + + fn is_elevated(&self) -> Result { + let mut elevation = TOKEN_ELEVATION::default(); + let token_elevation_sz = u32::try_from(size_of::()) + .expect("`TOKEN_ELEVATION` size should fit into a u32"); + let mut return_size = 0u32; + // SAFETY: Docs say nothing about threads or safety + // The type of `elevation` varies based on the 2nd parameter, but we hard-coded that. + // It should be fine. + unsafe { + GetTokenInformation( + self.inner, + TokenElevation, + Some(&mut elevation as *mut _ as *mut c_void), + token_elevation_sz, + &mut return_size as *mut _, + ) + }?; + Ok(elevation.TokenIsElevated == 1) + } +} + +impl Drop for ProcessToken { + fn drop(&mut self) { + // SAFETY: We got `inner` from `OpenProcessToken` and didn't mutate it after that. + // Closing a pseudo-handle is a harmless no-op, though this is a real handle. + // + // > The pseudo handle need not be closed when it is no longer needed. Calling the CloseHandle function with a pseudo handle has no effect. If the pseudo handle is duplicated by DuplicateHandle, the duplicate handle must be closed. + unsafe { CloseHandle(self.inner) }.expect("`CloseHandle` should always succeed"); + self.inner = HANDLE::default(); + } +} + pub(crate) fn install_ipc_service() -> Result<()> { let manager_access = ServiceManagerAccess::CONNECT | ServiceManagerAccess::CREATE_SERVICE; let service_manager = ServiceManager::local_computer(None::<&str>, manager_access)?; @@ -76,6 +145,9 @@ fn fallible_service_run( logging_handle: firezone_logging::file::Handle, ) -> Result<()> { tracing::info!(?arguments, "fallible_windows_service_run"); + if !elevation_check()? { + bail!("IPC service failed its elevation check, try running as admin / root"); + } let rt = tokio::runtime::Runtime::new()?; let (shutdown_tx, shutdown_rx) = mpsc::channel(1); diff --git a/website/src/components/Changelog/GUI.tsx b/website/src/components/Changelog/GUI.tsx index c67729efc..0db7aaa97 100644 --- a/website/src/components/Changelog/GUI.tsx +++ b/website/src/components/Changelog/GUI.tsx @@ -22,6 +22,9 @@ export default function GUI({ title }: { title: string }) { Fixes a bug where the "Clear Logs" button did not clear the IPC service logs. + + Fixes a bug where the GUI could not run if the user is Administrator + */}