From d76a42e0e9f45797699ed76b8831899f42c1725f Mon Sep 17 00:00:00 2001 From: enricobuehler Date: Mon, 6 Jul 2026 07:28:51 +0200 Subject: [PATCH] docs(security): add 2026-07-05 whole-project posture audit Assessment of the overall security architecture, the state of the prior reviews' remediations, and the process/supply-chain controls. Flags the two web-console residuals (no brute-force throttle; cookie seal key from the login password) that the accompanying web-hardening change fixes. Co-Authored-By: Claude Opus 4.8 (1M context) --- design/security-posture-audit-2026-07-05.md | 202 ++++++++++++++++++++ 1 file changed, 202 insertions(+) create mode 100644 design/security-posture-audit-2026-07-05.md diff --git a/design/security-posture-audit-2026-07-05.md b/design/security-posture-audit-2026-07-05.md new file mode 100644 index 0000000..c382cac --- /dev/null +++ b/design/security-posture-audit-2026-07-05.md @@ -0,0 +1,202 @@ +# punktfunk — security posture audit (2026-07-05) + +> **Status:** AUDIT COMPLETE (2026-07-05). Whole-project **posture** audit — not a finding hunt +> like [`security-review.md`](security-review.md) (2026-06-21) and +> [`security-review-2026-06-28.md`](security-review-2026-06-28.md) (host follow-up), but an +> assessment of the overall security architecture, the state of the prior reviews' remediations, +> the delta landed since 2026-06-28 (display management Stages 0–8, WoL wake-until-up, +> `--data-port`, web display-config surface), and the process/supply-chain controls around the +> code. Method: single-reviewer read of the trust-model core (`quic.rs`, `crypto.rs`, +> `session.rs`, `native_pairing.rs`, `punktfunk1.rs`, `mgmt.rs`, `mgmt_token.rs`, the web BFF +> auth stack, `gamestream/{pairing,crypto,control}.rs`, `discovery.rs`, `wol.rs`), plus sweeps +> for TLS-bypass patterns, committed secrets, unsafe density, CI workflow hygiene, and the +> dependency/advisory pipeline. + +## Executive summary + +**The posture is strong — unusually so for a project of this size.** The core trust +architecture is sound and shows defense-in-depth discipline: a modern, mutually-authenticated +native plane (QUIC + rustls, fingerprint pinning with real `CertificateVerify` verification, +SPAKE2 pairing with cert-fingerprint identity binding); a management plane with a clean +authn/authz split (loopback-confined bearer for admin, deny-by-default read-only allowlist for +paired mTLS certs); a web console that never leaks the admin token to the browser and fails +closed when misconfigured; legacy GameStream compatibility correctly quarantined behind an +explicit opt-in; and a supply chain guarded by scheduled `cargo audit`, a tightly-justified +ignore list, a license allowlist, and an exact-pinned toolchain. The two prior reviews' 18 +findings are remediated or accepted-with-rationale, and the fixes are visible (and in several +cases regression-tested) in the code today. + +**No new high-severity issue was found.** The residual risk concentrates in the **web console's +password gate** (no brute-force throttling; cookie-sealing key derived from the login password) +and in **second-tier supply-chain gaps** (no advisory scanning for the Bun/Nitro stack; +tag-pinned third-party CI actions holding deploy secrets). All are bounded by the documented +threat model (trusted LAN / VPN, no WAN exposure) but are cheap to close and worth closing. + +## Remediation status (2026-07-05) + +All findings from this audit were fixed the same day, plus one Windows LPE gap (F-8) surfaced by a +reviewer during remediation. Verification notes are per-item. + +| # | Sev | Status | +|---|-----|--------| +| F-1 | Medium | **FIXED** — per-IP exponential-backoff login throttle (`web/server/util/loginThrottle.ts` + `login.post.ts`); 5 free attempts then 1s→5min backoff, global floor, size-capped map. Behaviorally tested (backoff, unlock, IP-independence, success-clear). | +| F-2 | Medium | **FIXED** — cookie-seal key now derived from the CSPRNG mgmt token (`sessionConfig` in `auth.ts`), not the login password; password-derivation kept only as a no-token dev fallback. A captured cookie is no longer an offline password oracle. | +| F-3 | Low | **FIXED** — accept-any-cert scoped to the loopback proxy hop via Bun per-request `tls` (`routes/api/[...].ts`, gated on `isLoopbackUrl`); process-wide `NODE_TLS_REJECT_UNAUTHORIZED=0` removed from all 4 deploy files + 3 docs. A non-loopback mgmt URL now verifies normally. | +| F-4 | Low | **FIXED** — `bun audit` job added to `audit.yml` (weekly + on `web/bun.lock` change), same fail-on-vulnerability stance as `cargo audit`. | +| F-5 | Low | **FIXED** — the three secret-touching third-party actions SHA-pinned with version comments: `appleboy/scp-action`, `appleboy/ssh-action` (deploy SSH key + registry token, `docker.yml`), `android-actions/setup-android` (signing keystore + Play SA, `android.yml`/`android-screenshots.yml`). GitHub-owned `actions/*` left on major tags (org controls the tag). | +| F-6 | Info | No change (accepted) — tray loopback accept-any-cert; spoofed-status-only, local-process out of scope. | +| F-7 | Info | No change (documented) — session-layer input replay; native plane rides replay-protected QUIC, GameStream covered by the trusted-LAN caveat. | +| **F-8** | **Medium** | **FIXED** — see below. `driver install` executed/trusted files from `--dir` with no check the dir was admin-only → local EoP if the payload is staged somewhere user-writable. Added a DACL/owner check (`ensure_admin_only_source` in `windows/install.rs`) that fails closed. Verified: Win32 FFI typechecks against `windows 0.62.2` for `x86_64-pc-windows-msvc`. | + +### F-8 (Medium) — driver install trusts a non-admin-writable source directory + +**Reported by the user during remediation; confirmed.** `punktfunk-host driver install --dir ` +runs **elevated** (the Inno `[Run]` section, or a manual admin invocation) and, from `--dir`, +**executes** `nefconc.exe`, trusts a `.cer` into the machine `Root`/`TrustedPublisher` stores, and +`pnputil /add-driver`s the package — with **no check that `--dir` is writable only by +administrators**. If the staging directory is writable by a non-admin, a local unprivileged user can +plant a malicious `nefconc.exe` (arbitrary code as the elevated installer → SYSTEM) or swap the +`.cer` (poisoning the machine trust store) before the elevated step consumes it — a local elevation +of privilege. + +**Not exploitable in the default install** (correctly noted by the reporter): the shipped installer +stages into `{tmp}` under a `DefaultDirName={autopf}` (Program Files), `PrivilegesRequired=admin` +Inno setup, which restricts the staging dir to Administrators/SYSTEM. The gap bites only when the +payload is staged somewhere user-writable — a custom install directory, or a hand-run +`driver install --dir `. *Fix:* `ensure_admin_only_source` reads the directory's +owner + DACL (`GetNamedSecurityInfoW`) and refuses to proceed unless every principal with a +create/write/delete/DACL-rewrite right is SYSTEM/Administrators/TrustedInstaller (or CREATOR-OWNER +under a privileged owner). Fails **closed** — an unreadable ACL is treated as unsafe — and, like +every other step in the best-effort installer, a refusal only degrades the host to a physical +display; it never aborts the install. + +## Trust architecture assessment + +### Native plane (punktfunk/1) — sound + +- **Transport:** QUIC via quinn 0.11.11 / rustls 0.23.41 (ring). Data-plane sessions are + created with `encrypt: true` (`punktfunk1.rs:959`) and per-session random key+salt. +- **Identity & pinning:** hosts serve a persisted self-signed cert; clients pin its SHA-256 + (TOFU only as the documented bootstrap special case, `endpoint::client_insecure` = + `client_pinned(None)`). Crucially, the pin verifier still verifies TLS 1.2/1.3 handshake + signatures for real (`quic.rs:2064-2096`) — possession of the pinned cert's key is proven, + the classic pin-without-CertificateVerify hole is explicitly avoided. The host-side + `AcceptAnyClientCert` likewise verifies the handshake signature, so a client fingerprint is + proof of key possession, not just a presented blob. +- **Pairing (SPAKE2):** the ceremony binds *both* certificate fingerprints as SPAKE2 + identities, so a MITM presenting different certs to each leg cannot reach a shared key. + Key-confirmation MACs are compared constant-time. The PIN is CSPRNG-minted, **single-use and + consumed before the client's proof is read** (`punktfunk1.rs`, `pair_ceremony`) — an + attacker gets exactly one online guess per operator arming, rate-limited further by the 2s + `PAIRING_COOLDOWN`. Arming windows can be **fingerprint-bound** (prior review #9), and the + delegated-approval pending queue is flood-resistant (per-source-IP cap, parked-knock + eviction protection, TTL — prior review #13, all regression-tested in `native_pairing.rs`). +- **Session AEAD:** AES-128-GCM with a documented nonce-uniqueness contract (per-direction + salt bit, per-session key+salt, zero-key rejected by config validation), sequence bound as + AAD. Wire decoders bound every attacker-controllable header field (`packet.rs:323`, + `ReassemblerLimits`); pairing/control messages are length-checked with exact-trailing-bytes + rejection. +- **Untrusted-input hygiene:** client-supplied device names are sanitized against control + chars, ANSI escapes, and Unicode bidi overrides before storage/log/UI + (`native_pairing.rs::sanitize_device_name`) — approval-UI spoofing is handled. +- **Trust store:** atomic temp+rename writes, owner-only permissions (0600/DACL via + `write_secret_file`), in-memory rollback on persist failure. + +### Management plane — sound + +- HTTPS always; bearer token always required (env > owner-only persisted file > generated; + `mgmt_token.rs`). Token comparison hashes both sides before comparing (`mgmt.rs::token_eq`), + neutralizing timing. +- The **bearer (full-admin) path is honored only from loopback peers**, even though the + listener binds all interfaces by default; LAN callers must present a **paired** mTLS cert + and are confined to a deny-by-default, GET-only allowlist (`cert_may_access`) — a paired + streaming client cannot administer the host (unpair others, read/arm PINs, stop sessions, + or mutate the library/display config). The new display-management routes stayed off the + cert allowlist, with a regression test (`display_settings_surface stays read-only`). +- `/api/v1/local/summary` (tray status) is unauthenticated but loopback-only and + deliberately non-sensitive. + +### Web console (Nitro/Bun BFF) — good design, two hardening gaps (F-1, F-2) + +- Single shared password gate; **fails closed** (503) when `PUNKTFUNK_UI_PASSWORD` is unset; + constant-time compare; sealed (AES-GCM) `httpOnly` `SameSite=lax` session cookie with a + 7-day TTL; open-redirect guard on the post-login path; `/api/**` always gated; the mgmt + bearer token is injected **server-side only** and browser cookies are stripped from the + upstream request. Public-path allowlisting is by path, not extension (with the reasoning + documented — `openapi.json` exposure via a `*.json` allowlist was anticipated and avoided). +- Gaps: no login throttling (F-1), cookie-sealing key derived from the password when + `PUNKTFUNK_UI_SECRET` is unset (F-2), process-wide `NODE_TLS_REJECT_UNAUTHORIZED=0` for the + loopback proxy hop (F-3). + +### GameStream/Moonlight compatibility — correctly quarantined + +Off by default; enabled only by `--gamestream`/`--moonlight`, and documented (SECURITY.md, +`--help`) as legacy-crypto, trusted-LAN-only. Within that constraint the implementation is +defensively built: PIN delivery only via the authenticated management API (never nvhttp — +prior #1), parked-handshake caps (#12), one RSA signature per ceremony (Marvin-amplifier +hardening, S7), phase-repeat rejection, RTSP gated on a paired `/launch` bound to the +launching peer's IP (#4). The AES-128-ECB pairing crypto and mostly-plaintext media streams +are wire-compatibility constraints of the Moonlight protocol, not implementation choices, and +the input-replay gap at the raw-UDP session layer is documented in code +(`session.rs:31-35`) and covered by the trusted-LAN assumption. + +### Discovery & Wake-on-LAN — appropriately advisory + +mDNS TXT records (`fp`, `mac`, `pair`, `mgmt`) are treated as unauthenticated hints +everywhere they're consumed: pinning still gates the actual connection, and a spoofed MAC +only makes a wake fail (magic packets are inert). `wol.rs` detect-and-warn never mutates NIC +state. + +### Supply chain & process — mature + +- **Rust:** `cargo audit` weekly + on every `Cargo.lock` change + on demand + (`.gitea/workflows/audit.yml`); the `.cargo/audit.toml` ignore list is tight, justified, + and *self-correcting* (the RUSTSEC-2023-0071 entry documents that its own earlier rationale + was wrong and re-justifies the accept on corrected grounds — exemplary). Key crypto deps + are current (rustls 0.23.41, quinn 0.11.11, ring 0.17.14). License allowlist enforced via + `cargo-about` (no copyleft in the linked set). Exact toolchain pin (1.96.0). +- **Secrets hygiene:** no committed key material or real `.env` files (templates only); + secret files routed through `write_secret_file` (0600 + Windows SYSTEM/Admins DACL). +- **Process:** SECURITY.md with private reporting, coordinated disclosure, and safe harbor; + two prior deep reviews with per-finding remediation tracking; findings referenced by number + in code comments and regression tests. +- Gaps: no JS-side advisory scanning (F-4); third-party actions tag-pinned, not SHA-pinned + (F-5). + +## Findings (this audit) + +No High findings. Severities assume the documented LAN/VPN threat model. + +| # | Sev | Component | Finding | +|---|-----|-----------|---------| +| F-1 | **Medium** | web console | **No throttling on `POST /_auth/login`** (`web/server/routes/_auth/login.post.ts`). The constant-time compare stops timing leaks but not volume: a LAN peer can brute-force `PUNKTFUNK_UI_PASSWORD` at network speed against a console that is, by design, LAN-exposed. Every other password/PIN gate in the project is rate-limited (pairing cooldown, single-use PINs) — this is the one unthrottled secret. *Fix:* small in-memory failure counter with exponential backoff (per source IP + global), mirroring `PAIRING_COOLDOWN`'s philosophy. | +| F-2 | **Medium** | web console | **Cookie-sealing key is derived from the login password** when `PUNKTFUNK_UI_SECRET` is unset (`web/server/util/auth.ts::sessionConfig` — `sha256("punktfunk-session-v1:" + password)`). A captured session cookie (e.g. sniffed over the plain-HTTP dev/LAN mode, where `Secure` is off by default) becomes an **offline dictionary oracle for the password**: an attacker tries candidate passwords by deriving the key and attempting to unseal — no server round-trips, so F-1's fix doesn't help. *Fix:* generate and persist a random 32-byte secret on first start when `PUNKTFUNK_UI_SECRET` is unset (same pattern as `mgmt_token.rs`), instead of deriving from the password. | +| F-3 | Low | web console | **`NODE_TLS_REJECT_UNAUTHORIZED=0` is process-wide**, not scoped to the loopback mgmt hop it's documented for (`web/.env.example:25`, README). Today the BFF makes no other outbound TLS call, so the practical scope holds — but it's a global switch that silently unverifies any *future* fetch (an update check, an art CDN, a webhook). *Fix:* drop the env var and pass the host's own `cert.pem` as a per-fetch CA (Bun `fetch` `tls.ca` / undici `Agent`), which also removes a scary line from the deployment docs. | +| F-4 | Low | supply chain | **No advisory scanning for the web stack.** `audit.yml` covers the Rust tree only; the Bun/Nitro BFF — the component that holds the login gate, session sealing, and the mgmt token — has a `bun.lock` but no scheduled vulnerability scan. *Fix:* add a job running `bun audit` (or `osv-scanner --lockfile web/bun.lock`) on the same weekly + lockfile-change triggers. | +| F-5 | Low | CI | **Third-party actions are tag-pinned, not SHA-pinned.** `appleboy/ssh-action@v1.2.5` / `appleboy/scp-action@v0.1.7` receive deploy SSH credentials, and `actions/checkout@v4` / `cache@v4` run on every job on self-hosted runners; a moved tag = code execution on the runners plus secret exfiltration. Given how rigorous the Rust-side supply chain is, this is the soft spot. *Fix:* pin at least the secret-touching actions to full commit SHAs. | +| F-6 | Info | tray / local | The tray's status fetch (`punktfunk-tray/src/status.rs`) uses the accept-any-cert verifier against `127.0.0.1:47990`. If the host isn't running, any local unprivileged process can squat the port and feed the tray fake status. Impact is spoofed tray UI only (the tray sends no secrets and the summary is non-sensitive) — consistent with the documented "local processes are out of scope" stance. No action needed; recorded for completeness. | +| F-7 | Info | core session | Input-event replay at the raw-UDP session layer remains unfiltered — **already documented** in `session.rs:31-35` with the correct future home (a sliding window keyed on the authenticated sequence, in the GameStream host). The native plane is unaffected (client input rides QUIC datagrams, which are inherently replay-protected). Keep the doc note honest until the GameStream path grows the window; it stays covered by the opt-in/trusted-LAN caveat. | + +## Prior-review remediations — spot-verified present + +- #1 (PIN only via bearer mgmt API): confirmed — `PinGate` doc + no nvhttp PIN route. +- #2 (mgmt token via `write_secret_file`): confirmed — `mgmt_token.rs:62`. +- #9 (fingerprint-bound arming windows): confirmed — `arm_for`/`pin_for_attempt` + tests. +- #12 (parked-waiter cap): confirmed — `MAX_PARKED_WAITERS` + atomic slot reservation. +- #13 (per-IP pending cap, parked-knock protection): confirmed — constants + tests. +- S7 (rsa Marvin accept): rationale in `.cargo/audit.toml` matches the corrected version; + one-signature-per-ceremony hardening referenced in the ignore justification. + +## Priorities + +1. **F-1 + F-2 together** (web console password gate): a login backoff plus a + generated-not-derived cookie secret close the only realistic LAN-attacker path to admin + found in this audit. Both are small, isolated changes. +2. **F-4 + F-5** (supply chain parity): one CI job and a handful of SHA pins bring the JS + stack and the workflows up to the standard the Rust tree already meets. +3. **F-3** (scoped CA instead of the global TLS switch): low urgency, high + docs/appearance value. + +Nothing found here changes the documented threat model or contradicts SECURITY.md's stated +limits. The next full finding-hunt review should focus on the Windows vdisplay/driver +admission surface (largest post-2026-06-28 delta) and the web console once F-1/F-2 land.