mirror of
https://github.com/rustdesk/rustdesk.git
synced 2026-09-14 16:31:03 +03:00
refactor(linux): make the exe-check degrade decision pure and tested
Address review feedback on #16088: - Extract the privilege-dependent decision into a pure `should_defer_exe_check_to_uid_gate(err, server_is_unprivileged)` so both branches are unit-testable, and add a matrix test asserting a root server stays fail-closed while only an unprivileged server degrades on a permission error (a non-permission error never degrades). - Reword the comment: a root server does not "always" read the peer's exe (a dropped CAP_SYS_PTRACE or cross-namespace peer can still fail); the invariant that matters is that a root server stays fail-closed regardless. No behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Lq6xFoeEmjcuKwRx1GfdQ2
This commit is contained in:
@@ -479,18 +479,29 @@ fn peer_exe_read_permission_denied(err: &anyhow::Error) -> bool {
|
|||||||
.is_some_and(|io_err| io_err.kind() == std::io::ErrorKind::PermissionDenied)
|
.is_some_and(|io_err| io_err.kind() == std::io::ErrorKind::PermissionDenied)
|
||||||
}
|
}
|
||||||
|
|
||||||
// A root IPC server can always read a same-user-namespace peer's /proc/<pid>/exe, so a permission
|
// The degrade is confined to a NON-root server. In a standard install the IPC server is the root
|
||||||
// error there is genuinely anomalous (e.g. a cross-namespace peer) and must stay fail-closed. Only a
|
// service, which normally can read a same-user-namespace peer's /proc/<pid>/exe; a root server that
|
||||||
// NON-root server (the non-systemd case where the service could not register as root and runs as the
|
// nonetheless fails the read (e.g. dropped CAP_SYS_PTRACE, or a cross-namespace peer) is anomalous
|
||||||
// active user) cannot introspect a peer; there the executable check has no information to act on, so
|
// and stays fail-closed. Only when the server itself is unprivileged (the non-systemd case where the
|
||||||
// deferring to the uid gate is both the best achievable and the pre-1.4.7 behavior — and it keeps the
|
// service could not register as root and runs as the active user) can it not introspect ANY peer;
|
||||||
// hardened root-service path untouched.
|
// there the executable check has no information to act on, so deferring to the uid gate is both the
|
||||||
|
// best achievable and the pre-1.4.7 behavior, while the hardened root-service path is untouched.
|
||||||
#[cfg(target_os = "linux")]
|
#[cfg(target_os = "linux")]
|
||||||
#[inline]
|
#[inline]
|
||||||
fn ipc_server_is_unprivileged() -> bool {
|
fn ipc_server_is_unprivileged() -> bool {
|
||||||
unsafe { libc::geteuid() != 0 }
|
unsafe { libc::geteuid() != 0 }
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// The privilege-dependent authorization decision, pure so both branches are unit-testable: defer to
|
||||||
|
// the uid gate only when an unprivileged server hit a permission error introspecting the peer. A root
|
||||||
|
// server (`server_is_unprivileged == false`) always stays fail-closed, and a non-permission error
|
||||||
|
// (e.g. the peer vanished) never degrades.
|
||||||
|
#[cfg(target_os = "linux")]
|
||||||
|
#[inline]
|
||||||
|
fn should_defer_exe_check_to_uid_gate(err: &anyhow::Error, server_is_unprivileged: bool) -> bool {
|
||||||
|
server_is_unprivileged && peer_exe_read_permission_denied(err)
|
||||||
|
}
|
||||||
|
|
||||||
#[cfg(any(target_os = "linux", target_os = "macos", target_os = "windows"))]
|
#[cfg(any(target_os = "linux", target_os = "macos", target_os = "windows"))]
|
||||||
#[inline]
|
#[inline]
|
||||||
fn ensure_peer_executable_matches_current_by_pid(peer_pid: u32, postfix: &str) -> ResultType<()> {
|
fn ensure_peer_executable_matches_current_by_pid(peer_pid: u32, postfix: &str) -> ResultType<()> {
|
||||||
@@ -498,7 +509,7 @@ fn ensure_peer_executable_matches_current_by_pid(peer_pid: u32, postfix: &str) -
|
|||||||
Ok(peer_exe) => peer_exe,
|
Ok(peer_exe) => peer_exe,
|
||||||
Err(err) => {
|
Err(err) => {
|
||||||
#[cfg(target_os = "linux")]
|
#[cfg(target_os = "linux")]
|
||||||
if peer_exe_read_permission_denied(&err) && ipc_server_is_unprivileged() {
|
if should_defer_exe_check_to_uid_gate(&err, ipc_server_is_unprivileged()) {
|
||||||
log::warn!(
|
log::warn!(
|
||||||
"Peer executable link not introspectable on ipc channel '{}' by an unprivileged server (peer_pid={}): {}; identity unavailable, deferring to the uid gate",
|
"Peer executable link not introspectable on ipc channel '{}' by an unprivileged server (peer_pid={}): {}; identity unavailable, deferring to the uid gate",
|
||||||
postfix,
|
postfix,
|
||||||
@@ -1078,8 +1089,7 @@ mod tests {
|
|||||||
#[test]
|
#[test]
|
||||||
fn test_peer_exe_read_permission_denied_classification() {
|
fn test_peer_exe_read_permission_denied_classification() {
|
||||||
// EACCES/EPERM reading /proc/<pid>/exe (both map to `PermissionDenied`) means the peer is
|
// EACCES/EPERM reading /proc/<pid>/exe (both map to `PermissionDenied`) means the peer is
|
||||||
// not introspectable and must be classified as such even after `context()` is attached, so
|
// not introspectable and must be classified as such even after `context()` is attached.
|
||||||
// the caller degrades to the uid gate instead of rejecting a legitimate peer.
|
|
||||||
for errno in [hbb_common::libc::EACCES, hbb_common::libc::EPERM] {
|
for errno in [hbb_common::libc::EACCES, hbb_common::libc::EPERM] {
|
||||||
let err = hbb_common::anyhow::Error::new(std::io::Error::from_raw_os_error(errno))
|
let err = hbb_common::anyhow::Error::new(std::io::Error::from_raw_os_error(errno))
|
||||||
.context("Failed to read peer executable link '/proc/1/exe'");
|
.context("Failed to read peer executable link '/proc/1/exe'");
|
||||||
@@ -1088,7 +1098,7 @@ mod tests {
|
|||||||
"errno {errno} must classify as permission denied"
|
"errno {errno} must classify as permission denied"
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
// A vanished peer (NotFound) or a non-io error must NOT degrade to allow.
|
// A vanished peer (NotFound) or a non-io error must NOT classify as permission denied.
|
||||||
let not_found =
|
let not_found =
|
||||||
hbb_common::anyhow::Error::new(std::io::Error::from(std::io::ErrorKind::NotFound))
|
hbb_common::anyhow::Error::new(std::io::Error::from(std::io::ErrorKind::NotFound))
|
||||||
.context("Failed to read peer executable link '/proc/1/exe'");
|
.context("Failed to read peer executable link '/proc/1/exe'");
|
||||||
@@ -1097,4 +1107,32 @@ mod tests {
|
|||||||
&hbb_common::anyhow::anyhow!("canonicalize failure text")
|
&hbb_common::anyhow::anyhow!("canonicalize failure text")
|
||||||
));
|
));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[cfg(target_os = "linux")]
|
||||||
|
#[test]
|
||||||
|
fn test_should_defer_exe_check_to_uid_gate_matrix() {
|
||||||
|
let eacces = || {
|
||||||
|
hbb_common::anyhow::Error::new(std::io::Error::from_raw_os_error(
|
||||||
|
hbb_common::libc::EACCES,
|
||||||
|
))
|
||||||
|
.context("Failed to read peer executable link '/proc/1/exe'")
|
||||||
|
};
|
||||||
|
let not_found = || {
|
||||||
|
hbb_common::anyhow::Error::new(std::io::Error::from(std::io::ErrorKind::NotFound))
|
||||||
|
.context("Failed to read peer executable link '/proc/1/exe'")
|
||||||
|
};
|
||||||
|
// Only an unprivileged server that hit a permission error defers to the uid gate.
|
||||||
|
assert!(super::should_defer_exe_check_to_uid_gate(&eacces(), true));
|
||||||
|
// A root server stays fail-closed even on a permission error.
|
||||||
|
assert!(!super::should_defer_exe_check_to_uid_gate(&eacces(), false));
|
||||||
|
// A non-permission error never degrades, regardless of privilege.
|
||||||
|
assert!(!super::should_defer_exe_check_to_uid_gate(
|
||||||
|
¬_found(),
|
||||||
|
true
|
||||||
|
));
|
||||||
|
assert!(!super::should_defer_exe_check_to_uid_gate(
|
||||||
|
¬_found(),
|
||||||
|
false
|
||||||
|
));
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user