Fix/windows ipc server auth (#16755)

* fix(ipc): authenticate Windows named pipe servers

Restrict main and service pipe instance creation and authenticate the
connected server before exchanging IPC data. Open clients with data access
that excludes FILE_CREATE_PIPE_INSTANCE.

Require SYSTEM ownership for service channels. For main IPC, also accept a
same-user server with a restricted DACL, matching session and executable,
and sufficient elevation. This includes the UI hosting main IPC while the
service is unavailable, without relying on installation or service config.

Elevated clients reject non-elevated servers, and SYSTEM clients reject
user-owned pipes. New clients reject old permissive listeners until those
processes restart on the fixed version.

Add native Windows regression tests for instance-creation permissions,
DACL rejection before application data, and normal client communication.

Signed-off-by: fufesou <linlong1266@gmail.com>

* refactor(ipc): clarify Windows pipe security policy

Signed-off-by: fufesou <linlong1266@gmail.com>

* test(ipc): bound the Windows server rejection test

Signed-off-by: fufesou <linlong1266@gmail.com>

* test(ipc): bound the complete Windows client communication test

Signed-off-by: fufesou <linlong1266@gmail.com>

* fix(windows): keep portable GUI alive on IPC startup failure

Only attempt GUI fallback hosting when the main IPC pipe is absent. Bind its listener before marking hosting active or starting rendezvous, and return on bind failure without terminating the GUI.

Preserve IPC authentication and explicit server-process behavior. Cover rejected listeners, bind collisions, and standalone IPC communication with Windows regression tests.

Signed-off-by: fufesou <linlong1266@gmail.com>

* refactor(windows): simplify portable IPC startup fallback

Rely on exclusive listener creation instead of a separate pipe-presence probe. Keep GUI bind failures recoverable and require a successful bind before starting host services.

Retain the subprocess regression for bind-failure process survival, removing redundant GUI tests and their helper parameters and delay.

Signed-off-by: fufesou <linlong1266@gmail.com>

---------

Signed-off-by: fufesou <linlong1266@gmail.com>
This commit is contained in:
fufesou authored and GitHub committed 2026-11-05 22:35:07 +08:00
1 parent 7b5540bd61
commit 83e508cfdf
6 files changed
+449 -19

No files matched your search

+1
View File
@@ -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",
+25 -8
View File
@@ -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<Incoming> {
{
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<Incoming> {
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<u32> {
}
pub async fn connect(ms_timeout: u64, postfix: &str) -> ResultType<ConnectionTmpl<ConnClient>> {
#[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());
+1 -1
View File
@@ -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)
}
+203
View File
@@ -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<SecurityAttributes> {
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<ConnectionClient> {
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<ConnectionClient> {
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<bool> {
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::<ACCESS_ALLOWED_ACE>()
{
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<String> {
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;
+137
View File
@@ -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<T>(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");
}
+82 -10
View File
@@ -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"
);
}
}