diff --git a/DRM_CAPTURE_SECURITY.md b/DRM_CAPTURE_SECURITY.md index 0b82da39b..f0c4d448d 100644 --- a/DRM_CAPTURE_SECURITY.md +++ b/DRM_CAPTURE_SECURITY.md @@ -85,14 +85,15 @@ unprivileged one presents) but reuses RustDesk's own hardened IPC. - **Separate opt-in package.** A `--drm` build ships as a distinctly named `rustdesk-unattended-wayland` package (Conflicts/Replaces `rustdesk`), so enabling consent-free capture is an explicit install choice. -- **Bundled library, no capabilities.** The package installs `libdrmtap.so.0` - under `/usr/lib/rustdesk/` and registers that directory with the dynamic - linker so the in-process `dlopen("libdrmtap.so.0")` resolves: - - ```bash - # /etc/ld.so.conf.d/rustdesk-unattended-wayland.conf contains /usr/lib/rustdesk - ldconfig - ``` +- **Bundled library, no capabilities.** The package installs the versioned + `libdrmtap.so.0..` plus a `libdrmtap.so.0` soname symlink under + `/usr/lib/rustdesk/`, and the in-process `dlopen` names that absolute path + (`/usr/lib/rustdesk/libdrmtap.so.0`). The package deliberately does **not** + register the directory with the dynamic linker: no + `/etc/ld.so.conf.d/` drop-in and no `ldconfig` trigger are shipped, so a + private library cannot shadow a system one for unrelated binaries + (Debian Policy 10.2). The bare-soname lookups remain only as a fallback for a + development build reached through `LD_LIBRARY_PATH`. There is no `setcap`, no `rustdesk-capture` group, and no privileged binary: the capture runs inside the root `--service`, which already holds the @@ -111,9 +112,11 @@ unprivileged one presents) but reuses RustDesk's own hardened IPC. ## Auditing ```bash -# the bundled capture library — no capabilities are set on it -ls -l /usr/lib/rustdesk/libdrmtap.so.0 -cat /etc/ld.so.conf.d/rustdesk-unattended-wayland.conf # expect: /usr/lib/rustdesk +# the bundled capture library and its soname symlink — no capabilities are set on either +ls -l /usr/lib/rustdesk/libdrmtap.so.0* +# it must be exactly one real object plus the symlink: a second file with the same soname +# left beside it can become the one the symlink resolves to +ls /etc/ld.so.conf.d/ | grep -i rustdesk # expect: no output (none is shipped) # confirm no privileged helper is present (there should be none) getcap -r /usr/lib/rustdesk 2>/dev/null # expect: no output ``` diff --git a/libs/scrap/src/common/drmtap_dl.rs b/libs/scrap/src/common/drmtap_dl.rs index 8591c60a1..b7006b4ee 100644 --- a/libs/scrap/src/common/drmtap_dl.rs +++ b/libs/scrap/src/common/drmtap_dl.rs @@ -219,6 +219,18 @@ impl DrmtapLib { let (lib, name) = LIB_NAMES .iter() .find_map(|n| Library::new(n).ok().map(|l| (l, *n)))?; + // Resolve what we ACTUALLY opened, for the absolute candidate only. That name is a soname + // symlink, so a second file declaring the same soname beside the packaged one (an upgrade + // leftover, a hand-built .so) can end up being the one it points at, and then the path we + // asked for tells the reader nothing about which library is loaded. ONLY for an absolute + // name: `dlopen` does not search the process CWD for a bare soname (it uses DT_RUNPATH, + // LD_LIBRARY_PATH, the ld.so cache, the default dirs), while `canonicalize` resolves a + // relative name against the CWD, so canonicalizing a fallback could name a same-named file + // the loader never touched. For those we keep logging the plain name. + let real = std::path::Path::new(name) + .is_absolute() + .then(|| std::fs::canonicalize(name).ok()) + .flatten(); // every symbol is required; a missing one means an incompatible .so, // so bail to None and let the caller fall back to PipeWire. let version: FnVersion = *lib.get(b"drmtap_version").ok()?; @@ -236,7 +248,12 @@ impl DrmtapLib { ); return None; } - log::info!("libdrmtap loaded: {name} (v{major}.{minor}.{patch})"); + match real.as_ref().map(|p| p.display().to_string()) { + Some(p) if p != name => { + log::info!("libdrmtap loaded: {name} -> {p} (v{major}.{minor}.{patch})") + } + _ => log::info!("libdrmtap loaded: {name} (v{major}.{minor}.{patch})"), + } let open: FnOpen = *lib.get(b"drmtap_open").ok()?; let close: FnClose = *lib.get(b"drmtap_close").ok()?; let list_displays: FnListDisplays = *lib.get(b"drmtap_list_displays").ok()?; @@ -256,6 +273,28 @@ impl DrmtapLib { lib.get(b"drmtap_convert_dmabuf").ok().map(|s| *s); let render_node: Option = lib.get(b"drmtap_render_node").ok().map(|s| *s); + // A library can REPORT a version whose symbols it does not actually have, and that is not + // hypothetical: the multi-GPU accessors landed after an earlier build had already stamped + // itself 0.4.15, so a stale copy of that build keeps claiming 0.4.15 while lacking them. + // From in here that is indistinguishable from a repointed soname symlink, and it degrades + // SILENTLY (the service stops naming the exporting GPU, so the converter is left guessing). + // Say it out loud and name the file we really loaded, because the version alone lies. + if (minor, patch) >= (4, 15) && (render_node.is_none() || list_devices.is_none()) { + log::warn!( + "libdrmtap at {} reports v{major}.{minor}.{patch} but is missing {}: it is a stale \ + or pre-release build. Look for another libdrmtap.so.0* beside it (an upgrade \ + leftover or a hand-built copy); if that directory is in the linker search path, \ + ldconfig points the soname at whichever one it prefers. Multi-GPU display \ + enumeration and exporting-GPU selection stay disabled.", + real.as_ref() + .map_or_else(|| name.to_owned(), |p| p.display().to_string()), + match (render_node.is_none(), list_devices.is_none()) { + (true, true) => "drmtap_render_node and drmtap_list_devices", + (true, false) => "drmtap_render_node", + _ => "drmtap_list_devices", + } + ); + } Some(DrmtapLib { _lib: lib, open, diff --git a/src/server/drm_capturer.rs b/src/server/drm_capturer.rs index aefc02f7a..d83115339 100644 --- a/src/server/drm_capturer.rs +++ b/src/server/drm_capturer.rs @@ -316,8 +316,9 @@ async fn recv_thread( // Bind the converter to the GPU that EXPORTS this display's scanout, which the service // named in the display list. Auto-selection can land on a different GPU on a multi-GPU // host, and importing a scanout across vendors can fail on an incompatible tiling - // modifier. Empty (an older service or a device with no render node) means auto-select, - // exactly as before. Every display of one device carries the same node, so a display + // modifier. Empty means the service could not name it (an older service, or a device with no + // render node of its own): auto-select then, but only where there is nothing to pick wrong, per + // the ambiguity check below. Every display of one device carries the same node, so a display // index that does not resolve still gets the right answer from the first entry. let render_node = displays .get(display.max(0) as usize)