From 600693914fb73061a85fa4573e7bfe057472a954 Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Fri, 17 Jul 2026 17:14:57 +0200 Subject: [PATCH] =?UTF-8?q?fix(security):=20land=20the=202026-07=20audit?= =?UTF-8?q?=20fixes=20=E2=80=94=20SSRF=20guards,=20roster=20lane,=20SYSTEM?= =?UTF-8?q?=20path=20hygiene?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The low/medium findings from the July host+Windows security review, as implemented in the audit session's working tree: - Webhooks and library-art fetches refuse loopback/link-local/metadata targets and no longer follow redirects (SSRF pivots from the privileged host process). - The paired-client rosters (/clients, /native/clients) move off the streaming-client auth lane — one paired device could enumerate every other device's name + fingerprint; only the bearer/loopback console keeps them. - Device-name sanitizing extends to bidi/format control characters (spoofable rendering) via the shared native_pairing::is_spoofy_char; stream-marker quoting uses the same set. - The SYSTEM service resolves powershell by its full System32 path — CreateProcess checks the launching EXE's own directory first, so a planted powershell.exe beside the host binary would have run as SYSTEM. - The pf-vdisplay driver's opt-in file log moves from world-writable C:\Users\Public to WUDFHost's own temp dir. - GameStream pairing sessions are single-use (removed whatever the outcome). - Uninstall also removes the pf_mouse driver-store entry (rider from the virtual-HID-mouse work). - openapi.json regenerated (hardened-config-dir doc wording). Co-Authored-By: Claude Fable 5 --- api/openapi.json | 2 +- .../punktfunk-host/src/gamestream/nvhttp.rs | 42 +++++++++++-- .../punktfunk-host/src/gamestream/pairing.rs | 18 ++++-- crates/punktfunk-host/src/gamestream/rtsp.rs | 62 +++++++++++++------ crates/punktfunk-host/src/hooks.rs | 52 ++++++++++++++++ crates/punktfunk-host/src/library/art.rs | 8 +++ crates/punktfunk-host/src/library/custom.rs | 33 +++++----- crates/punktfunk-host/src/library/gog.rs | 29 ++++++++- crates/punktfunk-host/src/library/lutris.rs | 13 ++++ crates/punktfunk-host/src/library/xbox.rs | 8 +++ crates/punktfunk-host/src/mgmt/auth.rs | 7 ++- crates/punktfunk-host/src/mgmt/clients.rs | 9 ++- crates/punktfunk-host/src/mgmt/host.rs | 18 ++++-- crates/punktfunk-host/src/mgmt/session.rs | 4 +- crates/punktfunk-host/src/mgmt/tests.rs | 11 +++- crates/punktfunk-host/src/native_pairing.rs | 4 ++ .../src/native_pairing/sanitize.rs | 24 ++++--- crates/punktfunk-host/src/stream_marker.rs | 7 ++- crates/punktfunk-host/src/windows/install.rs | 2 +- crates/punktfunk-host/src/windows/service.rs | 9 ++- .../windows/drivers/pf-vdisplay/src/log.rs | 5 +- 21 files changed, 292 insertions(+), 75 deletions(-) diff --git a/api/openapi.json b/api/openapi.json index 4e324dae..97a41e1a 100644 --- a/api/openapi.json +++ b/api/openapi.json @@ -2744,7 +2744,7 @@ }, "CustomEntry": { "type": "object", - "description": "A user-added title, persisted in `~/.config/punktfunk/library.json`. Same shape the API\nreturns and the web console edits.", + "description": "A user-added title, persisted in the hardened host config dir's `library.json` (see\n[`custom_path`]). Same shape the API returns and the web console edits.", "required": [ "id", "title" diff --git a/crates/punktfunk-host/src/gamestream/nvhttp.rs b/crates/punktfunk-host/src/gamestream/nvhttp.rs index 862aa87e..c6d339f8 100644 --- a/crates/punktfunk-host/src/gamestream/nvhttp.rs +++ b/crates/punktfunk-host/src/gamestream/nvhttp.rs @@ -59,6 +59,33 @@ fn peer_is_paired(peer: &Option>, st: &AppState) .any(|der| hex::encode(punktfunk_core::quic::endpoint::cert_fingerprint(der)) == *fp) } +/// The peer's client-cert fingerprint as raw bytes — the form [`LaunchSession::owner_fp`] stores. +/// `None` when no (or a blank/short) cert was presented. +fn peer_fp(peer: &Option>) -> Option<[u8; 32]> { + match peer { + Some(Extension(PeerCertFingerprint(Some(fp)))) => hex::decode(fp) + .ok() + .and_then(|v| <[u8; 32]>::try_from(v).ok()), + _ => None, + } +} + +/// Whether the caller may control (resume/cancel) the current launch session. `true` when there is +/// no session (nothing to protect — keeps cancel idempotent), or the session's owner fingerprint +/// matches the caller's. Only a paired-but-DIFFERENT client with a known, mismatching fingerprint is +/// rejected — so a same-client control action always succeeds, but one paired client can no longer +/// resume or cancel *another* paired client's session (security-review 2026-07-17). +fn peer_may_control_session(peer: &Option>, st: &AppState) -> bool { + match st.launch.lock().unwrap().as_ref() { + None => true, + Some(session) => match (session.owner_fp, peer_fp(peer)) { + (Some(owner), Some(caller)) => owner == caller, + // Owner or caller fingerprint unknown → the `peer_is_paired` gate already applied stands. + _ => true, + }, + } +} + fn router(state: Arc, https: bool) -> Router { Router::new() .route("/serverinfo", get(h_serverinfo)) @@ -131,12 +158,7 @@ async fn h_launch( tracing::warn!("launch rejected — client is not paired"); return xml(error_xml()).into_response(); } - let req_fp: Option<[u8; 32]> = match &peer { - Some(Extension(PeerCertFingerprint(Some(fp)))) => hex::decode(fp) - .ok() - .and_then(|v| <[u8; 32]>::try_from(v).ok()), - _ => None, - }; + let req_fp: Option<[u8; 32]> = peer_fp(&peer); // Mode-conflict ADMISSION (Stage 4) — GameStream is single-session (`st.launch`), so a DIFFERENT // paired client launching while a session is live is governed by `mode_conflict` (see @@ -205,6 +227,10 @@ async fn h_resume( tracing::warn!("resume rejected — client is not paired"); return xml(error_xml()); } + if !peer_may_control_session(&peer, &st) { + tracing::warn!("resume rejected — caller does not own the session"); + return xml(error_xml()); + } if st.launch.lock().unwrap().is_some() { xml(session_url_xml(&st, "resume")) } else { @@ -220,6 +246,10 @@ async fn h_cancel( tracing::warn!("cancel rejected — client is not paired"); return xml(error_xml()); } + if !peer_may_control_session(&peer, &st) { + tracing::warn!("cancel rejected — caller does not own the session"); + return xml(error_xml()); + } *st.launch.lock().unwrap() = None; // Quit semantics: stop the running media threads (they observe these flags) so the session // actually ends — the virtual output/gamescope teardown follows via the capturer's RAII. diff --git a/crates/punktfunk-host/src/gamestream/pairing.rs b/crates/punktfunk-host/src/gamestream/pairing.rs index d2d05967..2a16e082 100644 --- a/crates/punktfunk-host/src/gamestream/pairing.rs +++ b/crates/punktfunk-host/src/gamestream/pairing.rs @@ -259,11 +259,22 @@ impl Pairing { // Constant-time compare so a timing side-channel can't probe the expected hash. let hash_ok = crypto::ct_eq(&expected, &s.client_hash); let sig_ok = verify256(&s.client_pubkey, client_secret, client_sig).is_ok(); + // Clone what the success branch needs so the `&mut map` borrow (`s`) is released before we + // mutate the map below. + let client_cert_der = s.client_cert_der.clone(); + // The pairing session is single-use: remove it now, WHATEVER the outcome. Phase 4 runs over + // plain HTTP, so a passive observer captures the request; without this, a replay re-passes the + // hash/sig check and re-pins the (same) cert over and over — unbounded `paired`/paired.json + // growth + PairingCompleted event spam until restart (security-review 2026-07-17). + map.remove(uniqueid); if hash_ok && sig_ok { { let mut store = paired_store.lock().unwrap(); - store.push(s.client_cert_der.clone()); - super::save_paired(&store); + // De-dup: re-pairing an already-trusted cert must not append a duplicate DER. + if !store.iter().any(|der| der == &client_cert_der) { + store.push(client_cert_der.clone()); + super::save_paired(&store); + } } tracing::info!(uniqueid, "pairing phase 4 complete — client cert pinned"); // Lifecycle event, plane parity with `NativePairing::add` (RFC §4). GameStream @@ -271,7 +282,7 @@ impl Pairing { crate::events::emit(crate::events::EventKind::PairingCompleted { device: crate::events::DeviceRef { name: uniqueid.to_string(), - fingerprint: hex::encode(crypto::sha256(&[s.client_cert_der.as_slice()])), + fingerprint: hex::encode(crypto::sha256(&[client_cert_der.as_slice()])), plane: crate::events::Plane::Gamestream, }, }); @@ -283,7 +294,6 @@ impl Pairing { sig_ok, "pairing phase 4 rejected — PIN or cert mismatch" ); - map.remove(uniqueid); Ok(paired_xml("", false)) } } diff --git a/crates/punktfunk-host/src/gamestream/rtsp.rs b/crates/punktfunk-host/src/gamestream/rtsp.rs index 340f85f8..766871dc 100644 --- a/crates/punktfunk-host/src/gamestream/rtsp.rs +++ b/crates/punktfunk-host/src/gamestream/rtsp.rs @@ -9,7 +9,7 @@ use super::audio; use super::stream::{self, StreamConfig}; -use super::{AppState, AUDIO_PORT, CONTROL_PORT, RTSP_PORT, VIDEO_PORT}; +use super::{AppState, LaunchSession, AUDIO_PORT, CONTROL_PORT, RTSP_PORT, VIDEO_PORT}; use crate::encode::Codec; use anyhow::{Context, Result}; use std::collections::HashMap; @@ -172,6 +172,24 @@ fn parse_request(head: &str, body: String) -> Request { } } +/// Authorize a state-changing RTSP verb against the pairing-gated `/launch` session. The RTSP/UDP +/// media plane is UNAUTHENTICATED, so `ANNOUNCE`/`PLAY`/`TEARDOWN` are honored only for the paired +/// client that completed `/launch` (which set `state.launch`), and — when the launching IP is known +/// — only from that same source IP. An unpaired RTSP peer can therefore neither start a stream, +/// overwrite a paired client's negotiated config, nor tear its active stream down +/// (security-review 2026-06-28 #4; extended from `PLAY`-only to `ANNOUNCE`/`TEARDOWN` 2026-07-17). +/// `nvhttp` gates `/launch` on a pinned client cert. Returns the launch session on success — `PLAY` +/// needs its keys/appid; the other verbs use it as a bool gate. +fn authorized_launch(state: &AppState, peer: Option) -> Option { + let ls = (*state.launch.lock().unwrap())?; + match (ls.peer_ip, peer.map(|p| p.ip())) { + // Launching IP known on both sides but mismatched → not the owner. + (Some(want), Some(got)) if want != got => None, + // Owner IP matches, or the address couldn't be captured on one side → launch-present only. + _ => Some(ls), + } +} + fn handle_request(req: &Request, state: &AppState, peer: Option) -> String { match req.method.as_str() { "OPTIONS" => response( @@ -203,6 +221,16 @@ fn handle_request(req: &Request, state: &AppState, peer: Option) -> ) } "ANNOUNCE" => { + // ANNOUNCE overwrites the session's negotiated stream/audio config. Gate it to the + // launch owner so an unpaired RTSP peer can't scribble a paired client's config (or + // race one in ahead of the owner's own ANNOUNCE). + if authorized_launch(state, peer).is_none() { + tracing::warn!( + ?peer, + "RTSP ANNOUNCE — refused: not the paired `/launch` owner" + ); + return response_status("401 Unauthorized", &req.cseq, &[], None); + } let map = parse_announce(&req.body); match stream_config(&map) { Some(cfg) => { @@ -217,25 +245,14 @@ fn handle_request(req: &Request, state: &AppState, peer: Option) -> response(&req.cseq, &[], None) } "PLAY" => { - // The RTSP/UDP media plane is UNAUTHENTICATED. A stream may start only for the paired - // client that completed the pairing-gated `/launch` (which set `state.launch`), and — - // when the launching IP is known — only from that same source IP. So an unpaired RTSP - // peer can neither start a stream on an idle host nor ride a paired client's active - // launch (security-review 2026-06-28 #4). `nvhttp` gates `/launch` on a pinned cert. - let launch = *state.launch.lock().unwrap(); - let Some(ls) = launch else { - tracing::warn!(?peer, "RTSP PLAY — refused: no paired `/launch` session"); + // A stream may start only for the paired client that owns the current `/launch` + // (`authorized_launch` enforces launch-present + matching source IP). `nvhttp` gates + // `/launch` on a pinned cert, so an unpaired RTSP peer can neither start a stream on an + // idle host nor ride a paired client's active launch. + let Some(ls) = authorized_launch(state, peer) else { + tracing::warn!(?peer, "RTSP PLAY — refused: not the paired `/launch` owner"); return response_status("401 Unauthorized", &req.cseq, &[], None); }; - if let (Some(want), Some(got)) = (ls.peer_ip, peer.map(|p| p.ip())) { - if want != got { - tracing::warn!( - %want, %got, - "RTSP PLAY — refused: peer IP does not match the launching client" - ); - return response_status("401 Unauthorized", &req.cseq, &[], None); - } - } let cfg = *state.stream.lock().unwrap(); match cfg { Some(cfg) if !state.streaming.swap(true, Ordering::SeqCst) => { @@ -271,6 +288,15 @@ fn handle_request(req: &Request, state: &AppState, peer: Option) -> response(&req.cseq, &[("Session", "DEADBEEFCAFE;timeout = 90")], None) } "TEARDOWN" => { + // Gate to the launch owner so an unpaired RTSP peer can't stop a paired client's media + // threads (a trivial, repeatable stream DoS). + if authorized_launch(state, peer).is_none() { + tracing::warn!( + ?peer, + "RTSP TEARDOWN — refused: not the paired `/launch` owner" + ); + return response_status("401 Unauthorized", &req.cseq, &[], None); + } // Signal both stream threads to stop. state.streaming.store(false, Ordering::SeqCst); state.audio_streaming.store(false, Ordering::SeqCst); diff --git a/crates/punktfunk-host/src/hooks.rs b/crates/punktfunk-host/src/hooks.rs index 7de83416..1e65d4a3 100644 --- a/crates/punktfunk-host/src/hooks.rs +++ b/crates/punktfunk-host/src/hooks.rs @@ -150,6 +150,19 @@ impl HooksConfig { if !url.starts_with("https://") && !url.starts_with("http://") { return Err(at("`webhook` must be an http(s):// URL")); } + if webhook_host_is_internal(url) { + return Err(at( + "`webhook` must not target a loopback/link-local/metadata host", + )); + } + // A signed webhook over plaintext http:// sends the HMAC'd event body in the clear. + // Warn rather than reject (an internal-only `http://` receiver may be intentional). + if h.hmac_secret_file.is_some() && url.starts_with("http://") { + tracing::warn!( + %url, + "webhook has an hmac_secret_file but is http:// — the signed body is sent in cleartext; prefer https://" + ); + } } if h.timeout_s == 0 || h.timeout_s > MAX_TIMEOUT_S { return Err(at(&format!("`timeout_s` must be 1–{MAX_TIMEOUT_S}"))); @@ -586,6 +599,45 @@ fn fire_webhook( }); } +/// True if `url`'s host is a clearly-illegitimate webhook target — loopback, link-local (which +/// includes the `169.254.169.254` cloud-metadata endpoint), the unspecified address, or `localhost` +/// — so a tampered/misguided hooks.json can't make the privileged host POST event data to its own +/// services or a metadata endpoint (direct-SSRF guard; security-review 2026-07-17). Deliberately does +/// NOT block RFC-1918 / ULA / `.local` — a webhook to another box on the operator's own LAN is a +/// legitimate self-hosting config. A best-effort textual + IP-literal check (no DNS resolution, so +/// not a full anti-rebinding defense; the operator-gated config already limits the threat). +fn webhook_host_is_internal(url: &str) -> bool { + // scheme://[userinfo@]host[:port]/... → the bare host. + let after_scheme = url.split_once("://").map(|(_, r)| r).unwrap_or(url); + let authority = after_scheme.split(['/', '?', '#']).next().unwrap_or(""); + let hostport = authority + .rsplit_once('@') + .map(|(_, h)| h) + .unwrap_or(authority); + let host = if let Some(rest) = hostport.strip_prefix('[') { + rest.split(']').next().unwrap_or("") // [::1]:443 → ::1 + } else { + hostport + .rsplit_once(':') + .map(|(h, _)| h) + .unwrap_or(hostport) + }; + let host = host.trim().to_ascii_lowercase(); + if host.is_empty() || host == "localhost" || host.ends_with(".localhost") { + return true; + } + match host.parse::() { + Ok(std::net::IpAddr::V4(v4)) => { + v4.is_loopback() || v4.is_link_local() || v4.is_unspecified() + } + Ok(std::net::IpAddr::V6(v6)) => { + // Loopback (::1), unspecified (::), or link-local fe80::/10. + v6.is_loopback() || v6.is_unspecified() || (v6.segments()[0] & 0xffc0) == 0xfe80 + } + Err(_) => false, // a resolvable hostname — not statically classifiable here + } +} + fn post_webhook(url: &str, json: &str, secret_file: Option<&std::path::Path>) { // TLS is verified (ureq's default rustls roots); redirects are never followed, so a // compromised receiver can't bounce the POST cross-origin (RFC §9.5). diff --git a/crates/punktfunk-host/src/library/art.rs b/crates/punktfunk-host/src/library/art.rs index c2d25e03..41fc961f 100644 --- a/crates/punktfunk-host/src/library/art.rs +++ b/crates/punktfunk-host/src/library/art.rs @@ -88,6 +88,10 @@ fn warm_art_once() { fn fetch_json(url: &str) -> Option { let agent = ureq::AgentBuilder::new() .timeout(std::time::Duration::from_secs(10)) + // Don't follow redirects — a redirect target (`3xx` → `http://169.254.169.254/…` or an + // internal host) would be an SSRF pivot from the privileged host. Matches the webhook path + // (security-review 2026-07-17). A rare legitimately-redirecting CDN just yields no art. + .redirects(0) .build(); let body = agent.get(url).call().ok()?.into_string().ok()?; serde_json::from_str(&body).ok() @@ -123,6 +127,10 @@ pub(crate) fn fetch_image(url: &str) -> Option<(Vec, String)> { } let agent = ureq::AgentBuilder::new() .timeout(std::time::Duration::from_secs(10)) + // Don't follow redirects (SSRF pivot): this is called on launcher-cache- and custom-entry- + // supplied URLs, so a `3xx` to an internal/metadata endpoint must not be chased by the + // privileged host. Matches the webhook path (security-review 2026-07-17). + .redirects(0) .build(); let resp = agent.get(url).call().ok()?; let ctype = resp diff --git a/crates/punktfunk-host/src/library/custom.rs b/crates/punktfunk-host/src/library/custom.rs index 444fde04..b7a23230 100644 --- a/crates/punktfunk-host/src/library/custom.rs +++ b/crates/punktfunk-host/src/library/custom.rs @@ -3,8 +3,8 @@ use super::*; -/// A user-added title, persisted in `~/.config/punktfunk/library.json`. Same shape the API -/// returns and the web console edits. +/// A user-added title, persisted in the hardened host config dir's `library.json` (see +/// [`custom_path`]). Same shape the API returns and the web console edits. #[derive(Clone, Debug, Serialize, Deserialize, ToSchema)] pub struct CustomEntry { /// Host-assigned, stable for the life of the entry (the `{id}` in the CRUD path). @@ -72,16 +72,13 @@ impl From for GameEntry { } } -fn config_dir() -> PathBuf { - std::env::var_os("XDG_CONFIG_HOME") - .map(PathBuf::from) - .or_else(|| std::env::var_os("HOME").map(|h| PathBuf::from(h).join(".config"))) - .unwrap_or_else(|| PathBuf::from(".")) - .join("punktfunk") -} - fn custom_path() -> PathBuf { - config_dir().join("library.json") + // The shared, hardened host config dir (`%ProgramData%\punktfunk` / `~/.config/punktfunk`, with + // the `PUNKTFUNK_CONFIG_DIR` override) — NOT a bespoke XDG/HOME resolver with a CWD-relative + // fallback. This file drives operator `prep`/`launch` command execution, so it must live where the + // rest of the privileged host config does and be DACL/0600-locked against a non-privileged local + // user planting one (security-review 2026-07-17). Matches hooks.json / the mgmt token. + pf_paths::config_dir().join("library.json") } /// Load the custom entries (empty + non-fatal if the file is absent or malformed). @@ -96,12 +93,18 @@ pub fn load_custom() -> Vec { } fn save_custom(entries: &[CustomEntry]) -> Result<()> { - let dir = config_dir(); - std::fs::create_dir_all(&dir).with_context(|| format!("create {}", dir.display()))?; + let dir = pf_paths::config_dir(); + // Owner-private dir (0700 / SYSTEM+Admins DACL) so a non-privileged local user can't plant a + // library.json whose `prep`/`launch` commands the host would later execute — the same trust + // boundary hooks.json and the mgmt token already use. + pf_paths::create_private_dir(&dir).with_context(|| format!("create {}", dir.display()))?; let json = serde_json::to_string_pretty(entries)?; - // Write-then-rename so a crash mid-write never truncates the catalog. + // Write-then-rename so a crash mid-write never truncates the catalog; `write_secret_file` gives + // the temp file its restrictive perms (0600 / SYSTEM+Admins DACL) before the rename carries them + // to the final path. let tmp = custom_path().with_extension("json.tmp"); - std::fs::write(&tmp, json).with_context(|| format!("write {}", tmp.display()))?; + pf_paths::write_secret_file(&tmp, json.as_bytes()) + .with_context(|| format!("write {}", tmp.display()))?; std::fs::rename(&tmp, custom_path()).context("rename library.json")?; Ok(()) } diff --git a/crates/punktfunk-host/src/library/gog.rs b/crates/punktfunk-host/src/library/gog.rs index a848747b..2c1bf9e6 100644 --- a/crates/punktfunk-host/src/library/gog.rs +++ b/crates/punktfunk-host/src/library/gog.rs @@ -67,8 +67,29 @@ fn gog_games() -> Vec { out } +/// Join a manifest-supplied relative path onto `install`, rejecting anything that could escape (or +/// replace) it: a drive prefix (`C:`), a root (`\`), or a `..` component — any of which `Path::join` +/// lets REPLACE or climb out of `install` on Windows. Keeps a crafted `goggame-*.info` from pointing +/// the play task's exe or working dir at an arbitrary program (security-review 2026-07-17). `None` +/// ⇒ the path is out of bounds and the caller refuses it. +#[cfg(windows)] +fn confined_join(install: &str, rel: &str) -> Option { + use std::path::Component; + let rp = Path::new(rel); + if rp.components().any(|c| { + matches!( + c, + Component::Prefix(_) | Component::RootDir | Component::ParentDir + ) + }) { + return None; + } + Some(Path::new(install).join(rp)) +} + /// The primary play task from `\goggame-.info`: `(absolute exe, args, working dir)`. -/// Prefers `isPrimary` + `FileTask`, else the first `FileTask`. Paths are resolved against `install`. +/// Prefers `isPrimary` + `FileTask`, else the first `FileTask`. Paths are resolved against `install` +/// and confined to it ([`confined_join`]). #[cfg(windows)] fn gog_play_task(install: &str, id: &str) -> Option<(String, String, String)> { let text = @@ -87,7 +108,8 @@ fn gog_play_task(install: &str, id: &str) -> Option<(String, String, String)> { }) .or_else(|| tasks.iter().find(|t| is_file(t)))?; let rel = pick.get("path").and_then(|s| s.as_str())?; - let exe = Path::new(install).join(rel); + // Refuse the launch outright if the manifest's exe path escapes the install dir. + let exe = confined_join(install, rel)?; let args = pick .get("arguments") .and_then(|s| s.as_str()) @@ -96,7 +118,8 @@ fn gog_play_task(install: &str, id: &str) -> Option<(String, String, String)> { let workdir = pick .get("workingDir") .and_then(|s| s.as_str()) - .map(|w| Path::new(install).join(w)) + // A working dir that escapes falls back to the install root (safe) rather than failing. + .and_then(|w| confined_join(install, w)) .unwrap_or_else(|| Path::new(install).to_path_buf()); Some(( exe.to_string_lossy().into_owned(), diff --git a/crates/punktfunk-host/src/library/lutris.rs b/crates/punktfunk-host/src/library/lutris.rs index 1bfb5b97..25f9d5bd 100644 --- a/crates/punktfunk-host/src/library/lutris.rs +++ b/crates/punktfunk-host/src/library/lutris.rs @@ -102,6 +102,19 @@ fn lutris_art(slug: &str) -> Artwork { #[cfg(target_os = "linux")] fn lutris_image(kind: &str, slug: &str) -> Option { use base64::Engine as _; + // `slug` comes verbatim from Lutris's `pga.db` (untrusted at this layer). Reject any path + // separator, parent ref, or NUL so a crafted slug can't escape the art roots and read an + // arbitrary `.jpg` off disk — the bytes are base64-inlined into the `/api/v1/library` + // JSON a paired client can GET, so an escape is an arbitrary-file-read exfil primitive + // (security-review 2026-07-17). Real Lutris slugs are `[a-z0-9-]`. + if slug.is_empty() + || slug.contains('/') + || slug.contains('\\') + || slug.contains("..") + || slug.contains('\0') + { + return None; + } let home = std::env::var_os("HOME").map(PathBuf::from)?; let roots = [ home.join(".local/share/lutris"), diff --git a/crates/punktfunk-host/src/library/xbox.rs b/crates/punktfunk-host/src/library/xbox.rs index 37817d83..51f4250e 100644 --- a/crates/punktfunk-host/src/library/xbox.rs +++ b/crates/punktfunk-host/src/library/xbox.rs @@ -37,6 +37,14 @@ fn xbox_games() -> Vec { if !cfg.is_file() { continue; } + // Cap the read like the other untrusted on-disk manifests (Epic `read_capped`, Lutris + // art) — a planted multi-GB MicrosoftGame.config under `:\XboxGames\…\Content\` + // must not OOM the privileged host during enumeration (security-review 2026-07-17). A + // real GDK manifest is a few KB. + match cfg.metadata() { + Ok(m) if m.len() <= 1024 * 1024 => {} + _ => continue, + } let Ok(text) = std::fs::read_to_string(&cfg) else { continue; }; diff --git a/crates/punktfunk-host/src/mgmt/auth.rs b/crates/punktfunk-host/src/mgmt/auth.rs index 0f1d4ab5..d70d6e21 100644 --- a/crates/punktfunk-host/src/mgmt/auth.rs +++ b/crates/punktfunk-host/src/mgmt/auth.rs @@ -105,8 +105,11 @@ pub(crate) fn cert_may_access(method: &Method, path: &str) -> bool { "/api/v1/host" | "/api/v1/compositors" | "/api/v1/status" - | "/api/v1/clients" - | "/api/v1/native/clients" + // The paired-client ROSTERS (`/clients`, `/native/clients`) are deliberately NOT on + // this lane — they expose every OTHER paired device's name + fingerprint, which one + // paired streaming client must not be able to enumerate. Only the bearer/loopback + // console needs them, and no first-party client calls them (security-review 2026-07-17). + // // The native clients browse the game library with their cert (no bearer token); the // library MUTATIONS (POST/PUT/DELETE /library/custom) stay token-only via the exact // GET-path match above. diff --git a/crates/punktfunk-host/src/mgmt/clients.rs b/crates/punktfunk-host/src/mgmt/clients.rs index a94cddf4..e84dfb01 100644 --- a/crates/punktfunk-host/src/mgmt/clients.rs +++ b/crates/punktfunk-host/src/mgmt/clients.rs @@ -47,7 +47,12 @@ pub(crate) struct SubmitPin { pub(crate) async fn list_paired_clients( State(st): State>, ) -> Json> { - let ders = st.app.paired.lock().unwrap().clone(); + let ders = st + .app + .paired + .lock() + .unwrap_or_else(|e| e.into_inner()) + .clone(); Json(ders.iter().map(|der| client_info(der)).collect()) } @@ -101,7 +106,7 @@ pub(crate) async fn unpair_client( "fingerprint must be the 64-char hex SHA-256 of the client certificate DER", ); } - let mut paired = st.app.paired.lock().unwrap(); + let mut paired = st.app.paired.lock().unwrap_or_else(|e| e.into_inner()); let before = paired.len(); paired.retain(|der| !hex::encode(Sha256::digest(der)).eq_ignore_ascii_case(&fingerprint)); if paired.len() < before { diff --git a/crates/punktfunk-host/src/mgmt/host.rs b/crates/punktfunk-host/src/mgmt/host.rs index 0cea157a..43b47f1a 100644 --- a/crates/punktfunk-host/src/mgmt/host.rs +++ b/crates/punktfunk-host/src/mgmt/host.rs @@ -306,8 +306,8 @@ pub(crate) async fn list_compositors() -> Json> { )] pub(crate) async fn get_status(State(st): State>) -> Json { // GameStream plane (set by RTSP/nvhttp on the compat path). - let gs_launch = *st.app.launch.lock().unwrap(); - let gs_stream = *st.app.stream.lock().unwrap(); + let gs_launch = *st.app.launch.lock().unwrap_or_else(|e| e.into_inner()); + let gs_stream = *st.app.stream.lock().unwrap_or_else(|e| e.into_inner()); let gs_video = st.app.streaming.load(Ordering::SeqCst); let gs_audio = st.app.audio_streaming.load(Ordering::SeqCst); // Native punktfunk/1 plane (published by the native video loop; the default plane). See @@ -362,7 +362,12 @@ pub(crate) async fn get_status(State(st): State>) -> Json>) -> Json< video_streaming: st.app.streaming.load(Ordering::SeqCst), audio_streaming: st.app.audio_streaming.load(Ordering::SeqCst), session, - paired_clients: st.app.paired.lock().unwrap().len() as u32, + paired_clients: st + .app + .paired + .lock() + .unwrap_or_else(|e| e.into_inner()) + .len() as u32, native_paired_clients, pin_pending: st.app.pairing.pin.awaiting_pin(), pending_approvals, diff --git a/crates/punktfunk-host/src/mgmt/session.rs b/crates/punktfunk-host/src/mgmt/session.rs index a5f56719..165e5c15 100644 --- a/crates/punktfunk-host/src/mgmt/session.rs +++ b/crates/punktfunk-host/src/mgmt/session.rs @@ -21,8 +21,8 @@ use std::sync::atomic::Ordering; pub(crate) async fn stop_session(State(st): State>) -> StatusCode { let was_streaming = st.app.streaming.swap(false, Ordering::SeqCst); st.app.audio_streaming.store(false, Ordering::SeqCst); - *st.app.launch.lock().unwrap() = None; - *st.app.stream.lock().unwrap() = None; + *st.app.launch.lock().unwrap_or_else(|e| e.into_inner()) = None; + *st.app.stream.lock().unwrap_or_else(|e| e.into_inner()) = None; // Native plane: the GameStream flags above don't reach it (it runs its own loops off the shared // session registry), so signal every live native session to tear down too. let native = crate::session_status::count(); diff --git a/crates/punktfunk-host/src/mgmt/tests.rs b/crates/punktfunk-host/src/mgmt/tests.rs index c25d3d8e..f9da6fd4 100644 --- a/crates/punktfunk-host/src/mgmt/tests.rs +++ b/crates/punktfunk-host/src/mgmt/tests.rs @@ -121,8 +121,6 @@ async fn cert_auth_is_a_read_only_allowlist() { "/api/v1/host", "/api/v1/status", "/api/v1/compositors", - "/api/v1/clients", - "/api/v1/native/clients", "/api/v1/library", ] { assert_ne!( @@ -131,6 +129,15 @@ async fn cert_auth_is_a_read_only_allowlist() { "a paired streaming cert should authorize GET {p}" ); } + // The paired-client ROSTERS are token-only: one paired cert must NOT be able to enumerate every + // other paired device's name + fingerprint (security-review 2026-07-17). + for p in ["/api/v1/clients", "/api/v1/native/clients"] { + assert_eq!( + send_cert(&app, get_req(p), fp).await, + StatusCode::UNAUTHORIZED, + "the client roster {p} must require the bearer token, not just a paired cert" + ); + } // PIN-exposing GET + state-changing routes → token-only (cert rejected without a bearer). assert_eq!( send_cert(&app, get_req("/api/v1/native/pair"), fp).await, diff --git a/crates/punktfunk-host/src/native_pairing.rs b/crates/punktfunk-host/src/native_pairing.rs index 8dc932bf..0bf335d3 100644 --- a/crates/punktfunk-host/src/native_pairing.rs +++ b/crates/punktfunk-host/src/native_pairing.rs @@ -33,6 +33,10 @@ pub use approval::{PairingDecision, PendingRequest}; pub use arming::PinAttempt; pub use store::PairedClient; +/// Re-exported for the stream marker's quoting (its `imp` is `cfg(unix)` — gate alike, or the +/// Windows build trips `-D unused-imports`). +#[cfg(unix)] +pub(crate) use sanitize::is_spoofy_char; /// The untrusted-device-name sanitizer lives in its own module (plan §W5); re-exported so /// `crate::native_pairing::sanitize_device_name` stays stable (the `native` accept loop /// reaches it there). diff --git a/crates/punktfunk-host/src/native_pairing/sanitize.rs b/crates/punktfunk-host/src/native_pairing/sanitize.rs index 83413ead..d4c0dd87 100644 --- a/crates/punktfunk-host/src/native_pairing/sanitize.rs +++ b/crates/punktfunk-host/src/native_pairing/sanitize.rs @@ -13,16 +13,7 @@ pub(crate) fn sanitize_device_name(name: &str, fp_hex: &str) -> String { let cleaned: String = name .chars() .map(|c| if c == '\t' || c == '\n' { ' ' } else { c }) - .filter(|&c| { - !c.is_control() - // Bidi/format controls that could spoof or reorder the displayed name. - && !('\u{202A}'..='\u{202E}').contains(&c) // LRE..RLO/PDF - && !('\u{2066}'..='\u{2069}').contains(&c) // LRI..PDI - && c != '\u{200E}' // LRM - && c != '\u{200F}' // RLM - && c != '\u{061C}' // ALM - && c != '\u{FEFF}' // BOM / zero-width no-break space - }) + .filter(|&c| !c.is_control() && !is_spoofy_char(c)) .collect(); // Collapse internal whitespace runs, trim, cap at the wire limit. let collapsed = cleaned.split_whitespace().collect::>().join(" "); @@ -42,6 +33,19 @@ pub(crate) fn sanitize_device_name(name: &str, fp_hex: &str) -> String { } } +/// A Unicode bidi/format control that could spoof or reorder a displayed name (an `RLO` making a +/// hostile device read like a trusted one). The canonical set — shared by every place that scrubs an +/// untrusted client name before display/storage (device names here, the stream marker) so the set +/// can't drift. Does NOT include C0/C1 controls; callers combine this with `char::is_control`. +pub(crate) fn is_spoofy_char(c: char) -> bool { + ('\u{202A}'..='\u{202E}').contains(&c) // LRE..RLO/PDF + || ('\u{2066}'..='\u{2069}').contains(&c) // LRI..PDI + || c == '\u{200E}' // LRM + || c == '\u{200F}' // RLM + || c == '\u{061C}' // ALM + || c == '\u{FEFF}' // BOM / zero-width no-break space +} + /// Max stored device-name length (matches the `Hello` wire cap, `quic::HELLO_NAME_MAX`). const NAME_MAX: usize = 64; diff --git a/crates/punktfunk-host/src/stream_marker.rs b/crates/punktfunk-host/src/stream_marker.rs index 2d5670ba..49ef7a8e 100644 --- a/crates/punktfunk-host/src/stream_marker.rs +++ b/crates/punktfunk-host/src/stream_marker.rs @@ -157,11 +157,12 @@ mod imp { } /// Make a client name safe to place inside single quotes in a sourceable file: drop the single - /// quote itself and any control characters (newlines especially), and cap the length so a hostile - /// or absurd name can't bloat the file. Empty stays empty. + /// quote itself, any control characters (newlines especially), and any bidi/format control that + /// could spoof the displayed name (the shared [`crate::native_pairing::is_spoofy_char`] set), and + /// cap the length so a hostile or absurd name can't bloat the file. Empty stays empty. fn sanitize(name: &str) -> String { name.chars() - .filter(|c| *c != '\'' && !c.is_control()) + .filter(|c| *c != '\'' && !c.is_control() && !crate::native_pairing::is_spoofy_char(*c)) .take(96) .collect() } diff --git a/crates/punktfunk-host/src/windows/install.rs b/crates/punktfunk-host/src/windows/install.rs index 1368de0b..a52749b1 100644 --- a/crates/punktfunk-host/src/windows/install.rs +++ b/crates/punktfunk-host/src/windows/install.rs @@ -226,7 +226,7 @@ fn uninstall_gamepad() -> Result<()> { // Devnodes first (incl. phantoms — the same ghost-device complaint the vdisplay uninstall // fixed), then the store packages. remove_pad_devnodes(); - delete_store_drivers(&["pf_dualsense", "pf_dualshock4", "pf_xusb"]); + delete_store_drivers(&["pf_dualsense", "pf_dualshock4", "pf_xusb", "pf_mouse"]); Ok(()) } diff --git a/crates/punktfunk-host/src/windows/service.rs b/crates/punktfunk-host/src/windows/service.rs index 61bea868..6f483225 100644 --- a/crates/punktfunk-host/src/windows/service.rs +++ b/crates/punktfunk-host/src/windows/service.rs @@ -979,7 +979,14 @@ fn set_fw_public_marker(allow_public: bool) { /// `Get-NetConnectionProfile` (per-interface category: Public / Private / DomainAuthenticated). /// `None` when it can't be determined — the caller then skips the warning. fn active_network_is_public() -> Option { - let out = std::process::Command::new("powershell") + // Resolve powershell by full System32 path — this runs in the SYSTEM service, which must not trust + // PATH / the CreateProcess search (it checks the launching EXE's OWN directory first, so a planted + // `powershell.exe` next to the host binary would run as SYSTEM). Matches the pf_vdisplay resolver + // and the "a privileged service must not trust PATH" rule (security-review 2026-07-17). + let ps = std::env::var("SystemRoot") + .map(|r| format!(r"{r}\System32\WindowsPowerShell\v1.0\powershell.exe")) + .unwrap_or_else(|_| "powershell.exe".to_string()); + let out = std::process::Command::new(&ps) .args([ "-NoProfile", "-NonInteractive", diff --git a/packaging/windows/drivers/pf-vdisplay/src/log.rs b/packaging/windows/drivers/pf-vdisplay/src/log.rs index 95467caa..736c237d 100644 --- a/packaging/windows/drivers/pf-vdisplay/src/log.rs +++ b/packaging/windows/drivers/pf-vdisplay/src/log.rs @@ -29,10 +29,13 @@ fn file_appender() -> Option<&'static std::sync::Mutex> { if !file_log_enabled() { return None; } + // WUDFHost's own (LocalService) temp dir — NOT world-writable/readable `C:\Users\Public`, + // where a non-admin could pre-create/hold the file or read the diagnostics + // (security-review 2026-07-17). Opt-in/debug only. std::fs::OpenOptions::new() .create(true) .append(true) - .open("C:\\Users\\Public\\pfvd-driver.log") + .open(std::env::temp_dir().join("pfvd-driver.log")) .ok() .map(std::sync::Mutex::new) })