diff --git a/Cargo.toml b/Cargo.toml index 4388fb062..53bb7d831 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -142,6 +142,7 @@ windows = { version = "0.61", features = [ "Win32_System_Pipes", "Win32_System_Registry", "Win32_System_SystemInformation", + "Win32_System_SystemServices", "Win32_System_Threading", "Win32_UI_Shell", "Win32_UI_WindowsAndMessaging", diff --git a/src/ipc.rs b/src/ipc.rs index 44a1c8d51..cf60c1d61 100644 --- a/src/ipc.rs +++ b/src/ipc.rs @@ -1,5 +1,8 @@ #[path = "ipc/auth.rs"] mod ipc_auth; +#[cfg(windows)] +#[path = "ipc/windows.rs"] +mod ipc_windows; #[cfg(any(target_os = "linux", target_os = "macos"))] #[path = "ipc/fs.rs"] mod ipc_fs; @@ -59,8 +62,8 @@ use ipc_auth::{active_uid, authorize_service_scoped_ipc_connection}; use ipc_auth::authorize_user_server_process; #[cfg(windows)] use ipc_auth::{ - authorize_windows_main_ipc_connection, portable_service_listener_security_attributes, - should_allow_everyone_create_on_windows, + authorize_windows_main_ipc_connection, is_windows_main_or_service_ipc, + portable_service_listener_security_attributes, }; #[cfg(target_os = "linux")] pub(crate) use ipc_auth::{ @@ -611,7 +614,12 @@ fn legacy_hot_measured() -> bool { #[tokio::main(flavor = "current_thread")] pub async fn start(postfix: &str) -> ResultType<()> { - let mut incoming = new_listener(postfix).await?; + let incoming = new_listener(postfix).await?; + start_with_listener(postfix, incoming).await; + Ok(()) +} + +pub(crate) async fn start_with_listener(postfix: &str, mut incoming: Incoming) { loop { if let Some(result) = incoming.next().await { match result { @@ -715,8 +723,8 @@ pub async fn new_listener(postfix: &str) -> ResultType { { if postfix == "_portable_service" { portable_service_listener_security_attributes() - } else if should_allow_everyone_create_on_windows(postfix) { - SecurityAttributes::allow_everyone_create() + } else if is_windows_main_or_service_ipc(postfix) { + ipc_windows::listener_security_attributes() } else { Ok(SecurityAttributes::empty()) } @@ -731,9 +739,8 @@ pub async fn new_listener(postfix: &str) -> ResultType { Err(err) => { log::error!("Failed to set ipc{} security: {}", postfix, err); #[cfg(windows)] - if postfix == "_portable_service" { - // Fail closed for `_portable_service` when SDDL construction fails. - // This endpoint is security-critical and must not start with default ACLs. + if postfix == "_portable_service" || is_windows_main_or_service_ipc(postfix) { + // Protected endpoints must not start with default ACLs. return Err(err.into()); } } @@ -1535,6 +1542,16 @@ fn user_main_ipc_server_uid() -> ResultType { } pub async fn connect(ms_timeout: u64, postfix: &str) -> ResultType> { + #[cfg(windows)] + if is_windows_main_or_service_ipc(postfix) { + return Ok(ConnectionTmpl::new( + timeout( + ms_timeout, + ipc_windows::connect(&Config::ipc_path(postfix), postfix), + ) + .await??, + )); + } #[cfg(any(target_os = "linux", target_os = "macos"))] { let use_user_main_ipc = USE_USER_MAIN_IPC.with(|use_user_main| use_user_main.get()); diff --git a/src/ipc/auth.rs b/src/ipc/auth.rs index 18f75cd49..06a9597e4 100644 --- a/src/ipc/auth.rs +++ b/src/ipc/auth.rs @@ -30,7 +30,7 @@ use windows::Win32::{Foundation::HANDLE, System::Pipes::GetNamedPipeClientProces #[cfg(windows)] #[inline] -pub(crate) fn should_allow_everyone_create_on_windows(postfix: &str) -> bool { +pub(crate) fn is_windows_main_or_service_ipc(postfix: &str) -> bool { postfix.is_empty() || hbb_common::config::is_service_ipc_postfix(postfix) } diff --git a/src/ipc/windows.rs b/src/ipc/windows.rs new file mode 100644 index 000000000..1bb36d359 --- /dev/null +++ b/src/ipc/windows.rs @@ -0,0 +1,203 @@ +use crate::platform::windows as platform; +use hbb_common::{anyhow::Context, bail, config, tokio::time::sleep, ResultType}; +use parity_tokio_ipc::{ConnectionClient, SecurityAttributes}; +use std::{ + ffi::c_void, + fs::OpenOptions, + io, + os::windows::{ + fs::OpenOptionsExt, + io::{AsRawHandle, IntoRawHandle}, + }, + time::Duration, +}; +use windows::{ + core::PWSTR, + Win32::{ + Foundation::{LocalFree, ERROR_PIPE_BUSY, HANDLE, HLOCAL}, + Security::{ + Authorization::{ConvertSidToStringSidW, GetSecurityInfo, SE_FILE_OBJECT}, + EqualSid, GetAce, IsWellKnownSid, WinLocalSystemSid, ACCESS_ALLOWED_ACE, ACE_HEADER, + ACL, DACL_SECURITY_INFORMATION, OWNER_SECURITY_INFORMATION, PSECURITY_DESCRIPTOR, PSID, + }, + Storage::FileSystem::{ + FILE_FLAG_OVERLAPPED, FILE_GENERIC_READ, FILE_WRITE_DATA, SECURITY_IDENTIFICATION, + }, + System::{ + Pipes::{GetNamedPipeServerProcessId, GetNamedPipeServerSessionId}, + SystemServices::ACCESS_ALLOWED_ACE_TYPE, + }, + }, +}; + +// FILE_GENERIC_WRITE includes FILE_CREATE_PIPE_INSTANCE, so clients request only data writes. +// https://learn.microsoft.com/en-us/windows/win32/ipc/named-pipe-security-and-access-rights +const CLIENT_ACCESS: u32 = FILE_GENERIC_READ.0 | FILE_WRITE_DATA.0; +const PIPE_BUSY_RETRY: Duration = Duration::from_millis(50); +const AUTH_LOG_INTERVAL: Duration = Duration::from_secs(5); +const SYSTEM_SID: &str = "S-1-5-18"; + +struct LocalAllocation(*mut c_void); + +impl Drop for LocalAllocation { + fn drop(&mut self) { + unsafe { LocalFree(Some(HLOCAL(self.0))) }; + } +} + +/// Restricts creation of main/service pipe instances to SYSTEM and the owner. +/// +/// Windows checks the existing pipe's DACL when CreateNamedPipe opens another +/// server instance of the same name. It requires FILE_CREATE_PIPE_INSTANCE, +/// even if the caller supplies a different security descriptor. +/// +/// The ACL checks identities, so another process running as the owner may still +/// create instances. It also cannot stop an attacker from creating the name first. +/// Clients must still authenticate the server in connect(); name squatting can +/// still deny service. +pub(super) fn listener_security_attributes() -> io::Result { + let owner = platform::current_process_user_sid_string() + .map_err(|err| io::Error::new(io::ErrorKind::Other, err))?; + // SDDL: each (A;;rights;;;sid) is an allow rule. + // O:{owner}: use the current process user as owner for client authentication. + // D:P: protect the DACL from inherited access rules. + // GA grants full access to SY (LocalSystem) and {owner}. + // WD (Everyone) gets CLIENT_ACCESS: data read/write and READ_CONTROL to inspect + // the owner/DACL, without FILE_CREATE_PIPE_INSTANCE or WRITE_DAC. + // https://learn.microsoft.com/en-us/windows/win32/secauthz/security-descriptor-string-format + // https://learn.microsoft.com/en-us/windows/win32/secauthz/ace-strings + // https://learn.microsoft.com/en-us/windows/win32/secauthz/sid-strings + let sddl = format!("O:{owner}D:P(A;;GA;;;SY)(A;;GA;;;{owner})(A;;0x{CLIENT_ACCESS:x};;;WD)"); + SecurityAttributes::from_sddl(&sddl) +} + +fn open_pipe(path: &str) -> io::Result { + let file = OpenOptions::new() + .access_mode(CLIENT_ACCESS) + .share_mode(0) + .custom_flags(FILE_FLAG_OVERLAPPED.0) + .security_qos_flags(SECURITY_IDENTIFICATION.0) + .open(path)?; + // Tokio takes ownership of the handle, including when registration fails. + unsafe { ConnectionClient::from_raw_handle(file.into_raw_handle()) } +} + +/// Authenticates a main/service pipe before exposing it for application data. +/// All accepted pipes need a restricted DACL. Service pipes require SYSTEM +/// ownership; main IPC also accepts same-user servers with a matching session +/// and executable, and no lower elevation than the client. +/// The caller must enforce the timeout, including retries while all instances are busy. +pub(super) async fn connect(path: &str, postfix: &str) -> ResultType { + let client = loop { + match open_pipe(path) { + Ok(client) => break client, + Err(error) if error.raw_os_error() == Some(ERROR_PIPE_BUSY.0 as i32) => { + sleep(PIPE_BUSY_RETRY).await; + } + Err(error) => return Err(error.into()), + } + }; + // The UI can own the main pipe while an installed service is unavailable. + if let Err(error) = ensure_server(&client, config::is_service_ipc_postfix(postfix)) { + hbb_common::throttled_log!( + AUTH_LOG_INTERVAL, + warn, + "Rejected untrusted Windows IPC server at '{}': {}", + path, + error + ); + return Err(error); + } + Ok(client) +} + +fn ensure_server(client: &ConnectionClient, require_system: bool) -> ResultType<()> { + let pipe = HANDLE(client.as_raw_handle()); + let mut pid = 0; + unsafe { GetNamedPipeServerProcessId(pipe, &mut pid) } + .context("Failed to resolve IPC server process")?; + if pid == 0 { + bail!("IPC server process is unavailable"); + } + if ensure_pipe_security(pipe, require_system)? { + return Ok(()); + } + let mut session = 0; + unsafe { GetNamedPipeServerSessionId(pipe, &mut session) } + .context("Failed to resolve IPC server session")?; + if platform::get_current_process_session_id() != Some(session) { + bail!("IPC server belongs to a different session"); + } + super::ipc_auth::ensure_peer_executable_matches_current_by_pid_opt(Some(pid), "")?; + if platform::is_elevated(None)? && !platform::is_elevated(Some(pid))? { + bail!("IPC server is less privileged than the client"); + } + Ok(()) +} + +fn ensure_pipe_security(pipe: HANDLE, require_system: bool) -> ResultType { + let mut owner = PSID::default(); + let mut dacl = std::ptr::null_mut(); + let mut descriptor = PSECURITY_DESCRIPTOR::default(); + unsafe { + GetSecurityInfo( + pipe, + SE_FILE_OBJECT, + OWNER_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION, + Some(&mut owner), + None, + Some(&mut dacl), + None, + Some(&mut descriptor), + ) + .ok() + .context("Failed to read IPC server security descriptor")?; + } + let _descriptor = LocalAllocation(descriptor.0); + if owner.0.is_null() || dacl.is_null() { + bail!("IPC server has no owner or restricted DACL"); + } + let owner_string = sid_string(owner)?; + let system = owner_string == SYSTEM_SID; + if !system && (require_system || owner_string != platform::current_process_user_sid_string()?) { + bail!("IPC server is not owned by the expected identity"); + } + // SYSTEM ownership alone is insufficient: a legacy DACL allows rogue extra instances. + // Together these checks authenticate installed pipes without opening SYSTEM process tokens. + ensure_restricted_dacl(dacl, owner)?; + Ok(system) +} + +fn ensure_restricted_dacl(dacl: *const ACL, owner: PSID) -> ResultType<()> { + for index in 0..unsafe { (*dacl).AceCount } { + let mut ace = std::ptr::null_mut(); + unsafe { GetAce(dacl, index.into(), &mut ace) } + .context("Failed to read IPC server access rule")?; + let header = unsafe { &*(ace as *const ACE_HEADER) }; + if header.AceType != (ACCESS_ALLOWED_ACE_TYPE as u8) + || usize::from(header.AceSize) < std::mem::size_of::() + { + bail!("Unsupported IPC server access rule"); + } + let ace = unsafe { &*(ace as *const ACCESS_ALLOWED_ACE) }; + let sid = PSID(std::ptr::addr_of!(ace.SidStart) as *mut c_void); + let trusted = unsafe { + EqualSid(sid, owner).is_ok() || IsWellKnownSid(sid, WinLocalSystemSid).as_bool() + }; + if !trusted && ace.Mask & !CLIENT_ACCESS != 0 { + bail!("IPC server grants untrusted users more than client access"); + } + } + Ok(()) +} + +fn sid_string(sid: PSID) -> ResultType { + let mut string = PWSTR::null(); + unsafe { ConvertSidToStringSidW(sid, &mut string) }?; + let _string = LocalAllocation(string.0.cast()); + Ok(unsafe { string.to_string() }?) +} + +#[cfg(test)] +#[path = "windows_tests.rs"] +mod tests; diff --git a/src/ipc/windows_tests.rs b/src/ipc/windows_tests.rs new file mode 100644 index 000000000..293e490e6 --- /dev/null +++ b/src/ipc/windows_tests.rs @@ -0,0 +1,137 @@ +use base::platform::windows::RAIIHandle; +use hbb_common::tokio::{ + self, + io::{AsyncReadExt, AsyncWriteExt}, + net::windows::named_pipe::ServerOptions, + time::{timeout, Duration}, +}; +use parity_tokio_ipc::{Endpoint, Incoming, SecurityAttributes}; +use windows::Win32::{ + Foundation::{ERROR_ACCESS_DENIED, HANDLE}, + Security::{ + CreateRestrictedToken, CreateWellKnownSid, ImpersonateLoggedOnUser, RevertToSelf, + WinWorldSid, DISABLE_MAX_PRIVILEGE, PSID, SECURITY_MAX_SID_SIZE, SID_AND_ATTRIBUTES, + TOKEN_DUPLICATE, TOKEN_QUERY, + }, + System::Threading::{GetCurrentProcess, OpenProcessToken}, +}; + +const TEST_TIMEOUT: Duration = Duration::from_secs(2); +const MESSAGE: &[u8] = b"ipc-regression"; + +fn listener(attributes: SecurityAttributes) -> (String, Incoming) { + let path = parity_tokio_ipc::dummy_endpoint(); + let mut endpoint = Endpoint::new(path.clone()); + endpoint.set_security_attributes(attributes); + (path, endpoint.incoming().unwrap()) +} + +fn as_untrusted_client(operation: impl FnOnce() -> T) -> T { + unsafe { + let mut token = HANDLE::default(); + OpenProcessToken( + GetCurrentProcess(), + TOKEN_DUPLICATE | TOKEN_QUERY, + &mut token, + ) + .unwrap(); + let _token = RAIIHandle(token.0.cast()); + let mut sid = [0u8; SECURITY_MAX_SID_SIZE as usize]; + let mut size = sid.len() as u32; + let sid = PSID(sid.as_mut_ptr().cast()); + CreateWellKnownSid(WinWorldSid, None, Some(sid), &mut size).unwrap(); + let mut restricted = HANDLE::default(); + let restrictions = [SID_AND_ATTRIBUTES { + Sid: sid, + Attributes: 0, + }]; + CreateRestrictedToken( + token, + DISABLE_MAX_PRIVILEGE, + None, + None, + Some(&restrictions), + &mut restricted, + ) + .unwrap(); + let _restricted = RAIIHandle(restricted.0.cast()); + ImpersonateLoggedOnUser(restricted).unwrap(); + let result = std::panic::catch_unwind(std::panic::AssertUnwindSafe(operation)); + RevertToSelf().unwrap(); + match result { + Ok(result) => result, + Err(panic) => std::panic::resume_unwind(panic), + } + } +} + +#[tokio::test] +async fn windows_ipc_rejects_additional_untrusted_instances() { + let (path, _incoming) = listener(super::listener_security_attributes().unwrap()); + let result = as_untrusted_client(|| ServerOptions::new().create(&path)); + assert_eq!( + result + .expect_err("untrusted clients must not create server instances") + .raw_os_error(), + Some(ERROR_ACCESS_DENIED.0 as i32), + ); +} + +#[tokio::test] +async fn windows_ipc_rejects_untrusted_server_before_sending() { + timeout(TEST_TIMEOUT, async { + { + let (path, _incoming) = listener(super::listener_security_attributes().unwrap()); + let result = super::connect(&path, crate::POSTFIX_SERVICE).await; + assert_eq!( + result.is_ok(), + crate::platform::windows::is_root(), + "restrictive permissions alone must not authenticate a non-SYSTEM owner" + ); + } + for postfix in [crate::POSTFIX_SERVICE, ""] { + let owner = crate::platform::windows::current_process_user_sid_string().unwrap(); + let attributes = + SecurityAttributes::from_sddl(&format!("O:{owner}D:P(A;;GA;;;WD)")).unwrap(); + let (path, mut incoming) = listener(attributes); + let result = super::connect(&path, postfix).await; + let error = + result.expect_err("a rogue server must not receive a usable client connection"); + if postfix.is_empty() { + assert_eq!( + error.to_string(), + "IPC server grants untrusted users more than client access" + ); + } + let mut server = incoming.next().await.unwrap().unwrap(); + let mut received = [0u8; 1]; + match server.read(&mut received).await { + Ok(length) => { + assert_eq!(length, 0, "authentication must not send application data") + } + Err(error) => assert_eq!(error.kind(), std::io::ErrorKind::BrokenPipe), + } + } + }) + .await + .expect("Windows IPC server rejection test timed out"); +} + +#[tokio::test] +async fn windows_ipc_allows_client_communication() { + timeout(TEST_TIMEOUT, async { + let (path, mut incoming) = listener(super::listener_security_attributes().unwrap()); + let mut client = as_untrusted_client(|| super::open_pipe(&path)).unwrap(); + let mut server = incoming.next().await.unwrap().unwrap(); + client.write_all(MESSAGE).await.unwrap(); + let mut received = vec![0u8; MESSAGE.len()]; + server.read_exact(&mut received).await.unwrap(); + server.write_all(&received).await.unwrap(); + let mut reply = vec![0u8; MESSAGE.len()]; + client.read_exact(&mut reply).await.unwrap(); + assert_eq!(reply, MESSAGE); + assert!(super::connect(&path, "").await.is_ok()); + }) + .await + .expect("Windows IPC client communication test timed out"); +} diff --git a/src/server.rs b/src/server.rs index 8e4c56cba..b839a6921 100644 --- a/src/server.rs +++ b/src/server.rs @@ -676,17 +676,26 @@ pub async fn start_server(is_server: bool, no_server: bool) { }); if is_server { - crate::common::set_server_running(true); - std::thread::spawn(move || { - if let Err(err) = crate::ipc::start("") { - log::error!("Failed to start ipc: {}", err); - if crate::is_server() { - log::error!("ipc is occupied by another process, try kill it"); - std::thread::spawn(stop_main_window_process).join().ok(); - } - std::process::exit(-1); + #[cfg(windows)] + if !crate::is_server() { + if let Err(err) = start_gui_ipc().await { + log::error!("Local hosting unavailable: failed to start GUI IPC listener: {err:#}"); + return; } - }); + } + crate::common::set_server_running(true); + if !cfg!(windows) || crate::is_server() { + std::thread::spawn(move || { + if let Err(err) = crate::ipc::start("") { + log::error!("Failed to start ipc: {}", err); + if crate::is_server() { + log::error!("ipc is occupied by another process, try kill it"); + std::thread::spawn(stop_main_window_process).join().ok(); + } + std::process::exit(-1); + } + }); + } // Warm the DRM availability cache before any client connects, so the first connection does // not race a cold `_drm` probe and ship an empty display list ("No displays" + retry). // X11 is skipped -- probing there makes the root service open DRM readers for a path this @@ -766,6 +775,13 @@ pub async fn start_server(is_server: bool, no_server: bool) { } } +#[cfg(windows)] +async fn start_gui_ipc() -> ResultType<()> { + let incoming = crate::ipc::new_listener("").await?; + tokio::spawn(crate::ipc::start_with_listener("", incoming)); + Ok(()) +} + #[cfg(target_os = "macos")] #[tokio::main(flavor = "current_thread")] pub async fn start_ipc_url_server() { @@ -974,3 +990,59 @@ pub async fn stop_main_window_process() { } } } + +#[cfg(all(test, windows))] +mod windows_gui_startup_tests { + use super::*; + use parity_tokio_ipc::{Endpoint, SecurityAttributes}; + use std::{os::windows::io::AsRawHandle, process::Command}; + use windows::Win32::{ + Foundation::{HANDLE, WAIT_OBJECT_0}, + System::Threading::WaitForSingleObject, + }; + + const CHILD_TEST: &str = "RUSTDESK_TEST_GUI_IPC_CHILD"; + const CHILD_TIMEOUT_MS: u32 = 10_000; + + #[test] + fn windows_gui_survives_ipc_bind_failure() { + if std::env::var_os(CHILD_TEST).is_some() { + *hbb_common::config::APP_NAME.write().unwrap() = + format!("RustDesk-ipc-test-{}", std::process::id()); + let runtime = tokio::runtime::Builder::new_current_thread() + .enable_all() + .build() + .unwrap(); + let _incoming = runtime.block_on(async { + let mut endpoint = Endpoint::new(Config::ipc_path("")); + endpoint + .set_security_attributes(SecurityAttributes::allow_everyone_create().unwrap()); + endpoint.incoming().unwrap() + }); + start_server(true, false); + assert!(!crate::common::is_server_running()); + return; + } + + let mut child = Command::new(std::env::current_exe().unwrap()) + .args([ + "--exact", + "server::windows_gui_startup_tests::windows_gui_survives_ipc_bind_failure", + "--nocapture", + ]) + .env(CHILD_TEST, "1") + .spawn() + .unwrap(); + let result = + unsafe { WaitForSingleObject(HANDLE(child.as_raw_handle()), CHILD_TIMEOUT_MS) }; + if result != WAIT_OBJECT_0 { + child.kill().unwrap(); + child.wait().unwrap(); + panic!("GUI IPC startup test timed out or failed to wait: {result:?}"); + } + assert!( + child.wait().unwrap().success(), + "GUI process did not survive" + ); + } +}