Files
punktfunk/design/security-review.md
T
enricobuehler 7b99b41ede docs(design): trim shipped plans, consolidate cluster, add index
Much of design/ described work that has since shipped. Trim each doc to
its durable rationale + still-open items (the code is the source of truth
for shipped detail; git history holds the full originals).

- Shipped plans -> status stubs: stats-capture, gamestream-host-plan,
  apple-stage2-presenter, windows-service.
- Trimmed completed-out / open-kept: implementation-plan, hdr-pipeline,
  host-latency, gpu-contention (fixed stale status table), game-library,
  linux-setup (fixed m0->spike + stale zero-copy claim),
  session-aware-host-followups, windows-client-bootstrap,
  windows-dualsense-{scoping,game-detection}, windows-virtual-display,
  security-review (per-finding status table; #12 still open),
  apollo-comparison (shipped backlog collapsed to one-liners).
- Windows-host cluster consolidated: windows-host.md -> redirect into
  windows-host-rewrite.md (whose stale scorecard is corrected -- goal1 is
  merged, M4 done); windows-secure-desktop.md archived (now a fallback
  behind IDD-push primary).
- Kept evergreen: ci.md, gamescope-multiuser.md, windows-build-and-packaging.md.
- New design/README.md: per-doc status table + consolidated open-items
  roll-up so nothing is tracked in only one buried doc.
- Repoint 5 code comments to the archived secure-desktop doc path.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
2026-06-26 16:39:06 +00:00

30 KiB
Raw Permalink Blame History

punktfunk — security audit (2026-06-21)

Status: AUDIT COMPLETE (2026-06-21). 11 of 12 confirmed findings are FIXED (3526517 · 3c55ec3) or accepted-risk DOCUMENTED-INHERENT (#5/#9); one remains OPEN: #12 (global NODE_TLS_REJECT_UNAUTHORIZED, DEFERRED). This doc is trimmed to the audit trail (executive summary · threat model · per-finding status · cross-cutting themes · positives · refuted findings) plus the accepted-risk rationale and the open item. Per-finding remediation prose for the FIXED findings is collapsed to one line + commit — the shipped code is the source of truth and git history holds the full original.

Whole-project audit by a 10-surface multi-agent review; every finding adversarially verified (reachability, attacker-control, existing mitigation). 10 surfaces · 20 raw findings → 18 confirmed/partial, 2 refuted. Threat model: a malicious network client (pre- and post-pairing) is the primary adversary; also an on-path MITM and a local unprivileged user (the host is privileged).

Remediation status (2026-06-21)

All 12 confirmed findings have been addressed — fixed, or documented where a fix isn't safely possible:

# Sev Status
#1 high FIXED (3526517) — secret files 0600 + dir 0700 / Windows icacls DACL
#2 high FIXED (3526517) — single-use SPAKE2 PIN (consumed at the host key-confirmation)
#3 med FIXED (3526517) — RTSP packetSize bounded + saturating packetizer math
#4 low FIXED (3c55ec3) — mgmt mTLS-cert auth restricted to a read-only allowlist; admin/state-changing routes require the bearer token
#5 low DOCUMENTED (won't-fix on legacy) — legacy GameStream GCM nonce reuse is inherent to Nvidia's old-style control encryption (Apollo/Moonlight identical); the GCM key is client-known. Real fix = V2 control-encryption negotiation; use punktfunk/1 for untrusted nets. Code comment at control.rs rumble loop.
#6 low FIXED (3c55ec3) — RTSP Content-Length/header caps + per-read timeout + concurrent-connection cap
#7 low FIXED (3c55ec3, GameStream) / DOCUMENTED (native) — new VirtualDisplay::set_launch_command carries the launch command per-session (GameStream); native path keeps the env (safe under today's single-session model; plumb per-session with concurrent sessions)
#8 info FIXED (3c55ec3) — constant-time GameStream phase-4 hash compare (crypto::ct_eq)
#9 info DOCUMENTED — GameStream pairing over plain HTTP is inherent to GFE compat; steer untrusted networks to the SPAKE2 native plane
#10 info FIXED (3c55ec3) — fixed ALPN (pkf1) on both QUIC endpoints (coordinated client+host upgrade)
#11 info FIXED (3c55ec3) — FEC reconstruction failure is now a counted drop, not stream-fatal
#12 low DEFERRED (fix ready, reverted) — the scoped-dispatcher fix (undici Agent on proxyRequest's fetch option) is designed and the mechanism verified sound (h3 honors the fetch option), but it needs undici added as a web dependency (bun add undici + lockfile regen), which requires the web build env — not available here. Reverted to keep the web build/proxy working. Latent-only: the loopback mgmt fetch is the web console's ONLY outbound TLS, so the global env weakens nothing today. Apply with: cd web && bun add undici, then scope rejectUnauthorized:false to the mgmt fetch and drop the global env.

Open items

Exactly one finding is still open — everything else is FIXED or accepted-risk DOCUMENTED (see the table above and #5/#9 below).

  • #12 [LOW] Web console sets NODE_TLS_REJECT_UNAUTHORIZED=0 process-globally — DEFERRED. Latent-only today: the only server-side outbound TLS hop is the loopback proxy to https://127.0.0.1:47990, which cannot be MITM'd, so impact is nil now. The fix is designed and verified sound (scope rejectUnauthorized:false to a per-request https.Agent/undici Agent pinned to the host cert on proxyRequest's fetch, and drop the global env) but was reverted because it needs bun add undici (+ lockfile regen) in the web build env, not available here. Guard: this MUST be fixed before the web app gains ANY off-loopback server-side outbound TLS — an update check, webhook, metadata fetch, or pointing PUNKTFUNK_MGMT_URL off-loopback would make it silently exploitable. Full detail in the findings section below.

Executive summary

Overall the punktfunk host is a security-conscious codebase with a strong cryptographic and wire-parsing core: the FEC/reassembler path bounds every attacker-controlled length field before allocation, AES-GCM is used correctly with per-direction nonce separation and seq-as-AAD on the native plane, and the native trust model (SPAKE2 PIN binding both cert fingerprints, fingerprint pinning that still verifies the real TLS handshake signature) is genuinely sound. The most serious real defects are (1) local secret-disclosure of the host's master private key (key.pem) — written with no restrictive mode/ACL while the far-less-sensitive mgmt token is carefully 0600 — which on Windows (%ProgramData% default Users-read ACL, LocalSystem service) is a near-certain cross-privilege host-impersonation primitive, and (2) the native SPAKE2 PIN ceremony permitting unlimited online guesses against a static, non-rotating 4-digit PIN (no disarm-on-failure, no lockout), which contradicts the documented "one online guess" guarantee and lets a pre-auth LAN attacker brute-force pairing of a fully-trusted rogue client in a few hours against the default standalone/CLI flow. Dominant themes: file-permission hygiene on secrets is inconsistent (the secure pattern exists but is applied selectively), pairing throttling relies on a single global rate-limit rather than attempt-bounding, and authorization is overbroad (any streaming-paired cert is also a full mgmt admin). The remaining findings are a contained pre-auth RTSP video-thread DoS (unbounded packetSize and Content-Length), a legacy GameStream control-stream GCM nonce-reuse that is muted by modern V2 negotiation and being key-gated, and several defense-in-depth nits (non-constant-time GameStream hash compare, no QUIC ALPN, cross-session env-var launch confusion, global NODE_TLS_REJECT_UNAUTHORIZED). No memory-unsafety or RCE was found on attacker wire bytes; panics are safe Rust and isolated by panic=unwind. Net: a solid foundation whose highest-leverage fixes are tightening secret file permissions and making the PIN single-use/lockout-bounded.

The executive summary describes the pre-fix state captured by the audit; it is retained verbatim as the historical record. See the status table for what shipped.

Findings (ranked by severity × exploitability)

FIXED findings are collapsed to one line + commit. The two accepted-risk findings (#5, #9) and the one open finding (#12) keep their full rationale.

🟠 #1 [HIGH] Host master private key (key.pem) written with no restrictive file mode / ACL — local secret disclosure enabling full host impersonation

Surface: secrets-availability · FIXED (3526517). key.pem is the single trust root for ALL surfaces (GameStream TLS cert + pairing signing key, the punktfunk/1 QUIC identity every client pins, the mgmt HTTPS cert); ServerIdentity::load_or_create wrote it with a bare fs::write/create_dir_all while the less-sensitive mgmt token used OpenOptions::mode(0o600). On Windows (%ProgramData% Users-read ACL + LocalSystem service) this was a near-certain cross-privilege host-impersonation primitive; on Linux it landed at umask (verified world-readable). Fix: 0600 file + 0700 dir on Unix mirroring mgmt_token.rs, SYSTEM+Administrators-only DACL on the Windows subtree, extended to client-key.pem/trust stores, + a 0600 regression test. Refs gamestream/cert.rs, gamestream/mod.rs, mgmt_token.rs, service.rs, native_pairing.rs.

🟠 #2 [HIGH] Native SPAKE2 PIN ceremony allows unlimited online guesses against a static 4-digit PIN — pre-auth brute-force to a fully-trusted rogue client

Surface: pairing-pin · FIXED (3526517). pair_ceremony logged+errored on a wrong PIN but never called np.disarm()/rotated; current_pin() returned the same value forever, throttled only by one process-wide 2s PAIRING_COOLDOWN over a 10,000-PIN space (~2.8h avg → permanently-pinned rogue cert with input/capture/launch). The standalone punktfunk1-host default (--require-pairing arms expires_at:None) made the indefinite static-PIN window the DEFAULT, not opt-in — contradicting the documented "one online guess". Fix: a failed confirmation now consumes/disarms the PIN (the actual "one online guess"); disarm after a successful pair too. Refs punktfunk1.rs, native_pairing.rs, mgmt.rs.

🟡 #3 [MEDIUM] Pre-auth RTSP ANNOUNCE packetSize underflows/panics the GameStream video pipeline

Surface: gamestream-parsing · FIXED (3526517). The RTSP listener on TCP 48010 does no TLS/pairing/auth; x-nv-video[0].packetSize flowed unbounded into VideoPacketizer::new where payload_per_shard = packet_size - 16 → packetSize==16 div-by-zero, <16 underflow OOB slice, ==17 per-frame datagram flood; the safe-Rust panic unwound before running.store(false), wedging the session until restart (not RCE — isolated by panic=unwind). Fix: clamp packetSize (floor ~64 / cap ~2048) + checked/saturating packetizer math + store(false) on the unwind path + a {0,15,16,17} regression test. Refs gamestream/rtsp.rs, gamestream/video.rs, gamestream/stream.rs.

🔵 #4 [LOW] Any paired punktfunk/1 streaming client gets full management-API authority via the mTLS-paired-cert auth path

Surface: authz-trust · FIXED (3c55ec3). require_auth granted any verified peer cert in the native paired store full unscoped /api/v1 access — the SAME set that admits a device to stream — so a watch-only device could DELETE /clients/{fp}, arm pairing + read the PIN, approve knocks, DELETE /session, CRUD the library. Fix: mTLS-cert auth restricted to a read-only allowlist; state-changing/admin/pairing-administration routes require the bearer token. Refs mgmt.rs:459-488.

🔵 #5 [LOW] GameStream legacy control-stream AES-GCM nonce reuse across directions (host rumble vs client input share key+nonce)

Surface: crypto · DOCUMENTED (won't-fix on legacy). Refs: crates/punktfunk-host/src/gamestream/control.rs:373-400, crates/punktfunk-host/src/gamestream/control.rs:257-266, crates/punktfunk-host/src/gamestream/control.rs:67,106-114

Why it ranks here / impact: Ranked #5: a real, correctly-identified catastrophic-class crypto defect (AES-GCM (key,nonce) reuse) but adjusted to low because reachability and impact are heavily muted. The legacy NonceKind branches apply no direction separation (other => other), so host rumble (rumble_seq from 0) and client control (seq from 0) under the shared rikey produce identical (key,nonce). BUT: (1) it only triggers on the legacy auto-detected scheme — modern moonlight-common-c negotiates the V2 scheme which flips marker[0] to 'H' and is direction-separated, so the default path is safe; the doc claim 'the legacy path — which we hit' is stale; (2) the rikey is delivered only over the mTLS /launch, so a pure MITM cannot derive the key — only a paired client can; (3) a paired client can already legitimately send any client→host control message (in-scope-by-design), so forgery is largely redundant and the only genuinely new gain is recovering low-value rumble keystream / forging rumble to its own client. Post-auth, conditional path.

Fix: Separate the two directions' nonce spaces for the legacy schemes too — set a reserved high bit/byte of the legacy IV for host-originated packets (mirror the V2 'H' marker), or better, HKDF-derive an independent host→client key from the rikey with a direction label so host and client never share a GCM key. Never let host rumble and client input share (key,nonce). (Inherent to the legacy Nvidia wire; the real fix is the V2 control-encryption / punktfunk/1 path. A code comment marks it at the control.rs rumble loop.)

🔵 #6 [LOW] RTSP request Content-Length / header size unbounded with no read timeout or connection cap — pre-auth slow-loris / memory-growth DoS

Surface: gamestream-parsing · FIXED (3c55ec3). read_message computed total = end+4+content_len with no cap and looped extend_from_slice; the header scan was unbounded and one unbounded native thread spawned per connection with no global limit (slow exhaustion + thread/FD exhaustion on a privileged plaintext LAN listener). Fix: Content-Length/total-header caps + per-read timeout + concurrent-connection cap. Refs gamestream/rtsp.rs.

🔵 #7 [LOW] Per-session launch command carried via process-global PUNKTFUNK_GAMESCOPE_APP env var, stomped under concurrent native sessions

Surface: privilege-process-launch · FIXED (3c55ec3, GameStream) / DOCUMENTED (native). serve_session did std::env::set_var(PUNKTFUNK_GAMESCOPE_APP) per connection under DEFAULT_MAX_CONCURRENT=4 — a TOCTOU where client B's launch overwrote what client A's gamescope bare-spawn read (and a never-cleared value leaked to a later no-launch client). NOT command injection: cmd always resolves through library::launch_command (digit-validated Steam appids / operator-only custom store), so the worst case is a different operator-approved title, and only the gamescope bare-spawn backend reads the var. Fix: VirtualDisplay::set_launch_command carries it per-session for GameStream; the native path keeps the env — safe under today's single-session model, plumb per-session as concurrent sessions land (still-open follow-up). Refs punktfunk1.rs, vdisplay/gamescope.rs.

#8 [INFO] GameStream pairing phase-4 hash compare is not constant-time

Surface: pairing-pin · FIXED (3c55ec3). Variable-time == on attacker-influenced 32-byte phase-4 SHA-256 digests; not weaponizable (expected mixes undisclosed host-random server_challenge, a mismatch map.removes the session forcing fresh randomness — no stable secret, no timing→PIN path). Fixed for consistency with the native ceremony: crypto::ct_eq. Refs gamestream/pairing.rs:226-247.

#9 [INFO] GameStream pairing ceremony runs over plain HTTP — inherited GFE brute-forceable-PIN / MITM weakness

Surface: authz-trust · DOCUMENTED (inherent to GameStream compat). Refs: crates/punktfunk-host/src/gamestream/nvhttp.rs:33, crates/punktfunk-host/src/gamestream/nvhttp.rs:215-264, crates/punktfunk-host/src/gamestream/pairing.rs:102-247

Why it ranks here / impact: Ranked #9: info — real but intentional Moonlight-compat behavior, on record rather than a regression. The whole /pair flow (incl. phase-4 cert pinning) is on plain HTTP 47989 with no transport confidentiality and no rate-limiting; the AES key is pin_key(salt,pin) = SHA-256(salt||pin)[..16] feeding AES-128-ECB, so an on-path attacker observing a legitimate pairing can offline-brute-force the 4-digit PIN and forge a clientpairingsecret to get a cert pinned. This is the well-known GFE/Sunshine construction, fixed by interop, and is precisely why punktfunk/1's SPAKE2 path exists; it requires an active MITM during an operator-initiated pairing within the 300s window. A paired GameStream client is in-scope-by-design.

Fix: Inherent to GameStream compatibility — document it and steer users to punktfunk/1 (SPAKE2) for untrusted networks. Optionally rate-limit pairing sessions per uniqueid/IP and tighten/expire the awaiting-PIN window aggressively.

#10 [INFO] No ALPN configured on the native QUIC server/client (cross-protocol confusion hardening absent)

Surface: cert-tls-identity · FIXED (3c55ec3). No alpn_protocols was set on either endpoint, but no reachable confusion attack: ALPACA needs two TLS services sharing a cert on the SAME transport — GameStream is TLS-over-TCP, punktfunk/1 is TLS-in-QUIC (UDP), and there is exactly one QUIC server, with trust already enforced by fingerprint pinning + Hello/Welcome magic. Fixed as cheap future-proofing: fixed ALPN pkf1 on both endpoints (coordinated client+host upgrade). Refs quic.rs:1335-1448.

#11 [INFO] FEC reconstruct error on the receive path is stream-fatal — code-contract inconsistency (not an exploitable DoS)

Surface: core-wire-deser · FIXED (3c55ec3). Reassembler::push propagated coder.reconstruct(...)? and both receive-side callers treated any non-NoFrame error as fatal — inconsistent with the surrounding "malformed = silent drop, never fatal" discipline. Every Err arm was traced unreachable from hostile input (header firewall + block-geometry pinning guarantee equal-length correctly-counted shards; Config::validate rejects odd/zero shard_payload; MDS Reed-Solomon decodes any data_shards distinct shards; reaching the reassembler needs an AES-GCM-decryptable packet; client-side only). Fixed as defense-in-depth: a reconstruct failure is now a counted drop returning Ok(None), reserving Err for genuinely fatal conditions. Refs packet.rs, session.rs, clients/probe/src/main.rs, spike.rs.

🔵 #12 [LOW] Web console sets NODE_TLS_REJECT_UNAUTHORIZED=0 process-globally — latent footgun disabling all outbound TLS verification

Surface: deps-config-exposure · DEFERRED — THE ONE STILL-OPEN ITEM. Refs: web/.env.example:22-24, web/web.env.example:11-14, web/server/util/auth.ts:17-22, web/vite.config.ts:23

Why it ranks here / impact: Ranked #12: low and not currently exploitable (attackerControlled false), included as a latent defense-in-depth defect. NODE_TLS_REJECT_UNAUTHORIZED=0 disables certificate validation for every outbound TLS connection the Node process makes, but the only current server-side outbound hop is the loopback proxy to https://127.0.0.1:47990 (CDN/art fetches are browser-side), and a loopback connection cannot be MITM'd — so impact is nil today. Real impact materializes silently if anyone later adds a server-side off-host HTTPS call (update check, webhook, metadata fetch) or points PUNKTFUNK_MGMT_URL off-loopback.

Fix: Do not disable TLS verification globally. Pin the host's self-signed cert for the single loopback fetch: pass an https.Agent with the host cert as ca (or rejectUnauthorized:false on that one Agent only) to the proxyRequest fetch in server/routes/api/[...].ts, and drop NODE_TLS_REJECT_UNAUTHORIZED from the deployment env. Reverted because it needs undici added as a web dependency (bun add undici + lockfile regen) in the web build env. Apply with cd web && bun add undici, then scope rejectUnauthorized:false to the mgmt fetch and drop the global env.

Cross-cutting themes

  • Inconsistent secret file-permission hygiene: the secure 0600/ACL pattern exists (mgmt-token) but is applied selectively, leaving the master private key and trust stores at umask/default-ACL — the highest-impact local-privilege gap, acute on Windows %ProgramData% + LocalSystem.
  • Pairing throttling is rate-based, not attempt-bounded: a single global 2s cooldown and a static non-rotating 4-digit PIN with no disarm-on-failure/lockout means the documented 'one online guess' property is not actually implemented for the native ceremony.
  • Overbroad authorization / collapsed trust tiers: 'paired to stream' equals 'paired to administer' (mgmt mTLS), and GameStream pairing inherits the plaintext-HTTP brute-forceable-PIN model — coarse trust boundaries where finer scopes are warranted.
  • Pre-auth attack surface on the GameStream/RTSP listeners with missing input bounds and resource caps (unbounded packetSize, unbounded Content-Length, no timeouts/connection caps) — contained DoS, but on a privileged plaintext service.
  • Stale concurrency assumptions and process-global mutable state (legacy GCM nonce direction, PUNKTFUNK_GAMESCOPE_APP env var) that were safe under a since-removed 'one session at a time' invariant and now cause cross-session confusion / crypto reuse.
  • Strong, well-tested cryptographic and memory-safety core (bounded wire parsing, correct AEAD/SPAKE2/pinning, catch_unwind FFI, panic=unwind isolation) — the foundation is solid; the residual risk is in operational hardening and trust-tier granularity, not in unsafe/RCE.

Security controls done right (positives)

  • Defense-in-depth wire parsing: every attacker-controllable FEC/reassembler header field is bounded against negotiated limits BEFORE any allocation keyed on it (packet.rs:328-343) — shard_bytes exact-match, data/total/block counts in range, indices in bounds, frame_bytes<=max — with no integer overflow in the size math and regression tests (rejects_oversized_shard_counts, rejects_inconsistent_block_geometry_without_panicking).
  • Reassembler memory is bounded to a 16-frame reorder window with prune-on-push and completed-frame dedup (packet.rs:451-468), so a flood of distinct frame indices cannot grow memory unboundedly and late shards cannot resurrect emitted frames.
  • AEAD gates the reassembler: on an encrypted session open_from_wire verifies the GCM tag (with seq as AAD) before any bytes reach push (session.rs:120-131), so an attacker cannot reach the reassembler without the session key; oversized-datagram truncation is always detectable (recv buffers MAX+1, len>MAX dropped).
  • Native AES-GCM is correct and misuse-resistant: 96-bit nonce = 4-byte salt || 8-byte BE seq with seq also as AEAD AAD (tampering fails the tag, not shifts the nonce), per-direction salt-bit separation gives disjoint nonce spaces under the shared key, a fresh CSPRNG 128-bit key per session, and Config::validate rejects encrypt=true with an all-zero key (crypto.rs, session.rs).
  • Host-side data-plane datagram decoders (mic / RichInput / HidOutput / InputEvent / gamepad) are all length-checked, Option-returning and non-fatal — the privileged host drops anything malformed and keeps draining, never reassembles attacker video, and never panics on truncated/hostile input.
  • punktfunk/1 trust establishment is sound: PinVerify rejects a fingerprint mismatch AND still performs real TLS 1.2/1.3 CertificateVerify signature checks (not stubbed), so an active MITM cannot replay the host's public cert to satisfy a pin without the private key (quic.rs:1547-1608) — the single most important thing to get right, done correctly.
  • SPAKE2 PIN pairing for the native plane is built correctly: a balanced PAKE binding BOTH cert fingerprints as identities and into the key-confirmation transcript, a wrong PIN yields a different key (one online guess, no offline dictionary, no error oracle), MITM with different certs per leg reaches no shared key, and confirmation MACs use a constant-time ct_eq — all exercised by tests.
  • Authorization is cleanly split from authentication on both planes: AcceptAnyClientCert verifiers accept any self-signed cert at the handshake but still verify the handshake signature, so the post-handshake fingerprint proves key possession, and authorization is then enforced against the paired allow-list (--require-pairing default fail-closed; certless peers rejected).
  • GameStream post-pair endpoints (applist/launch/resume/cancel) are gated by peer_is_paired() requiring a pinned mutual-TLS client cert, fail-closed for certless/unknown/None peers, with a dedicated regression test (nvhttp.rs:46-55, 303-328).
  • No command injection on the launch surface: client-supplied launch ids resolve against the host's OWN catalog (client can only pick an existing title), Steam appids are digits-only validated (with a 570; rm -rf ~ rejection test), custom commands come only from the operator mgmt store, and gamescope::spawn uses argv (Command::new + args), never /bin/sh -c.
  • Client-controlled display dimensions are validated (encode::validate_dimensions: zero/odd/over-max rejected) on both the initial Hello and mid-stream Reconfigure before reaching encoders or the privileged SudoVDA ADD ioctl, which marshals a fixed #[repr(C)] struct and only selects driver-advertised modes.
  • mgmt API is authenticated on every route except /health even on loopback (fails closed on a blank token), with constant-time SHA-256-digest token comparison and a CSPRNG token persisted 0600 on Unix (O_CREAT mode + follow-up set_permissions, never briefly world-readable).
  • Attacker-controlled device names are sanitized before logging/storage/UI (C0/C1 controls, Unicode bidi/format overrides, BOM stripped, length-capped, fingerprint fallback), blocking log/console-injection and trusted-device spoofing in the approval UI.
  • The pending-knock / delegated-approval queue is bounded (PENDING_CAP=32, LRU eviction) and time-bounded (10-min TTL), in-memory only, so a LAN scanner cannot grow it unboundedly or poison the persistent trust store.
  • Both trust stores are persisted atomically (temp + rename) with in-memory rollback on a failed persist, so a crash or full disk mid-write cannot truncate the allow-list and silently lock out or un-gate paired clients.
  • C-ABI boundary is hardened: config_from_ptr enforces the struct_size skew guard before dereferencing, every narrowing field is range-checked before truncation, every data-processing entry point is wrapped in catch_unwind returning Panic (no unwind across FFI), and null/zero-length handling is consistent and safe.
  • No secret material is logged anywhere — PINs, GCM/rikey keys, nonces/salts, the mgmt token, and private keys never reach tracing/println; pairing-failure logs include only the sanitized device name + fingerprint.
  • Crypto stack is current and free of disclosed-vuln versions (rustls 0.23.40, quinn 0.11.9, ring 0.17.14, aes-gcm 0.10.3, spake2 0.4.0), ring-only with no aws-lc C dep; rsa 0.9.10 (RUSTSEC-2023-0071 Marvin) is used ONLY for sign/verify, never decryption, so the vulnerable path is not exercised.
  • The Windows service launches the host with a correctly-scoped duplicated SYSTEM token (only the session id retargeted), a fixed winsta0\default desktop, a command line built from current_exe + an operator-controlled host.env subcommand (never network input), and a kill-on-job-close job object so a crash never orphans the SYSTEM host.

Refuted (investigated, NOT vulnerabilities)

  • [unsafe-ffi-cabi] Free/close FFI entry points run Drop without catch_unwind — a panic in teardown is UB across the C boundary — Verified all cited code. abi.rs:272-276 (punktfunk_session_free) and abi.rs:1627-1631 (punktfunk_connection_close) are verbatim as described — both call drop(Box::from_raw(..)) outside guard()/catch_unwind, while the guard helper (abi.rs:168-170) wraps catch_unwind. The doc at abi.rs:11 ("every entry point is wrapped in catch_unwind") is literally inaccurate for these two — that doc-vs-code discrepancy is REAL.

But the finding's security claim — "the unwind would cross the extern "C" frame, which is undefined behavior" — is REFUTED by the build toolchain. This crate is edition 2021 built with rustc 1.96.0. Since Rust 1.81 (Sept 2024), an unwind that reaches a non--unwind extern "C" boundary is a defined, safe process abort, not UB. None of these functions use extern "C-unwind". So the worst possible outcome is a clean abort, which is also exactly what catch_unwind→PunktfunkStatus::Panic would avoid only by returning a status — but free/close return void, so there is no status to return anyway.

Moreover the precondition (a panicking Drop) does not exist in the code. NativeClient::drop discards the worker join result with let _ = w.join(), so a worker-thread panic/poison cannot re-enter Drop. Config::drop only zeroizes. Session has no custom Drop. The transport Drop closes a socket. There is no .unwrap(), no Mutex::lock, and no result-propagating thread join on any teardown path — the finding's speculated panic sources are not present.

The finding is correctly self-rated as informational and explicitly non-attacker-controlled / non-pre-auth. The accurate residual is a documentation inconsistency (the module doc overstates the catch_unwind invariant), not a security weakness. The substantive recommendation (keep Drop impls panic-free) is already satisfied. Net: refuted as a vulnerability; severity info, and even as a code-quality nit the UB framing is incorrect for the current compiler. Worth at most a one-line doc fix to say free/close intentionally abort-on-panic.

  • [unsafe-ffi-cabi] C-ABI pointer/length contracts (32-byte fingerprint buffers, caller buffers) are trusted, not validated — standard FFI, embedder-only — All three cited spans are present and behave exactly as described. abi.rs:877 from_raw_parts(pin_sha256, 32) (after null-check at 873), abi.rs:905 from_raw_parts_mut(observed_sha256_out, 32) (after null-check at 903), abi.rs:1003 from_raw_parts_mut(host_sha256_out, 32) (after null-check at 990). The submit_frame/send_mic claim is also accurate: host_submit_frame (283-302, with an extra null+len guard) and send_mic (1233-1253) build slices from caller (data,len). These are C-ABI entry points whose pointer/length arguments come from the trusted embedding app (PunktfunkKit/GTK/WinUI), not from wire bytes. None of the threat-model adversaries can influence them: the malicious network client (pre- or post-auth) controls protocol bytes that flow through the embedder, not the embedder's own FFI call arguments; a MITM is irrelevant; a local unprivileged user cannot call into another process's loaded library. Each function carries a documented Safety contract and null-checks its pointers, and the hard-coded 32 matches the fingerprint type, so even a buggy-but-honest caller passing a correctly-sized buffer is safe. This is idiomatic, sound FFI — not a vulnerability. The finding's own posture (info, attackerControlled=false, preAuth=false, no fix required, listed for the record) is correct and well-calibrated. In vuln terms this is refuted (not a vuln / not reachable by any in-scope adversary), consistent with the info classification.