fix(security): land the 2026-07 audit fixes — SSRF guards, roster lane, SYSTEM path hygiene
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 <noreply@anthropic.com>
This commit is contained in:
@@ -88,6 +88,10 @@ fn warm_art_once() {
|
||||
fn fetch_json(url: &str) -> Option<serde_json::Value> {
|
||||
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<u8>, 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
|
||||
|
||||
@@ -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<CustomEntry> 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<CustomEntry> {
|
||||
}
|
||||
|
||||
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(())
|
||||
}
|
||||
|
||||
@@ -67,8 +67,29 @@ fn gog_games() -> Vec<GameEntry> {
|
||||
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<PathBuf> {
|
||||
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 `<install>\goggame-<id>.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(),
|
||||
|
||||
@@ -102,6 +102,19 @@ fn lutris_art(slug: &str) -> Artwork {
|
||||
#[cfg(target_os = "linux")]
|
||||
fn lutris_image(kind: &str, slug: &str) -> Option<String> {
|
||||
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 `<slug>.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"),
|
||||
|
||||
@@ -37,6 +37,14 @@ fn xbox_games() -> Vec<GameEntry> {
|
||||
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 `<drive>:\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;
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user