fix(security): remaining audit findings — mgmt admin gate, RTSP DoS bounds, FEC drop, ALPN, ct-compare
apple / swift (push) Successful in 56s
windows-host / package (push) Successful in 2m25s
windows-msix / package (arm64, C:\Users\Public\ffmpeg-arm64, aarch64-pc-windows-msvc, C:\t-a64) (push) Successful in 1m8s
windows-msix / package (x64, C:\Users\Public\ffmpeg, x86_64-pc-windows-msvc, C:\t) (push) Successful in 1m10s
android / android (push) Successful in 4m42s
ci / rust (push) Successful in 4m44s
ci / web (push) Successful in 30s
ci / docs-site (push) Successful in 35s
windows / build (aarch64-pc-windows-msvc) (push) Successful in 57s
windows / build (x86_64-pc-windows-msvc) (push) Successful in 1m0s
deb / build-publish (push) Successful in 2m10s
decky / build-publish (push) Successful in 11s
docker / build-push (--build-arg FEDORA_VERSION=44, ci, ci/fedora-rpm.Dockerfile, punktfunk-fedora44-rpm) (push) Successful in 4s
docker / build-push (., web/Dockerfile, punktfunk-web) (push) Successful in 4s
docker / build-push (ci, ci/fedora-rpm.Dockerfile, punktfunk-fedora-rpm) (push) Successful in 3s
docker / build-push (ci, ci/rust-ci.Dockerfile, punktfunk-rust-ci) (push) Successful in 4s
docker / build-push (docs-site, docs-site/Dockerfile, punktfunk-docs) (push) Successful in 4s
ci / bench (push) Successful in 4m43s
flatpak / build-publish (push) Successful in 3m59s
rpm / build-publish (bazzite, punktfunk-fedora-rpm) (push) Successful in 8m28s
docker / deploy-docs (push) Successful in 18s
rpm / build-publish (fedora-44, punktfunk-fedora44-rpm) (push) Successful in 8m13s
apple / swift (push) Successful in 56s
windows-host / package (push) Successful in 2m25s
windows-msix / package (arm64, C:\Users\Public\ffmpeg-arm64, aarch64-pc-windows-msvc, C:\t-a64) (push) Successful in 1m8s
windows-msix / package (x64, C:\Users\Public\ffmpeg, x86_64-pc-windows-msvc, C:\t) (push) Successful in 1m10s
android / android (push) Successful in 4m42s
ci / rust (push) Successful in 4m44s
ci / web (push) Successful in 30s
ci / docs-site (push) Successful in 35s
windows / build (aarch64-pc-windows-msvc) (push) Successful in 57s
windows / build (x86_64-pc-windows-msvc) (push) Successful in 1m0s
deb / build-publish (push) Successful in 2m10s
decky / build-publish (push) Successful in 11s
docker / build-push (--build-arg FEDORA_VERSION=44, ci, ci/fedora-rpm.Dockerfile, punktfunk-fedora44-rpm) (push) Successful in 4s
docker / build-push (., web/Dockerfile, punktfunk-web) (push) Successful in 4s
docker / build-push (ci, ci/fedora-rpm.Dockerfile, punktfunk-fedora-rpm) (push) Successful in 3s
docker / build-push (ci, ci/rust-ci.Dockerfile, punktfunk-rust-ci) (push) Successful in 4s
docker / build-push (docs-site, docs-site/Dockerfile, punktfunk-docs) (push) Successful in 4s
ci / bench (push) Successful in 4m43s
flatpak / build-publish (push) Successful in 3m59s
rpm / build-publish (bazzite, punktfunk-fedora-rpm) (push) Successful in 8m28s
docker / deploy-docs (push) Successful in 18s
rpm / build-publish (fedora-44, punktfunk-fedora44-rpm) (push) Successful in 8m13s
Addresses the lower-severity findings from docs/security-review.md (#4-#12). Each fix was adversarially re-reviewed (5-agent pass); two review catches folded in (the Apple client's GET /library cert path; an RTSP header-cap bypass + a spawn-panic counter leak). - #4 [low] mgmt mTLS-paired-cert no longer grants full admin. A paired STREAMING cert authorizes only a read-only allowlist (GET /host,/compositors,/status,/clients,/native/clients,/library); every state-changing route and every PIN-exposing route (/pair, /native/pair) requires the operator's bearer token. New cert_auth_is_a_read_only_allowlist test. (/library kept on the allowlist — the native clients browse it cert-only; its mutations stay token-only.) - #6 [low] RTSP pre-auth DoS bounds: a concurrent-connection cap (RAII slot guard), a per-read timeout (slow-loris), and Content-Length/header/message size caps — closing an unauthenticated slow-loris / memory-growth / thread-exhaustion vector on TCP 48010. - #11 [info] A FEC reconstruction failure is now a counted drop (discard the block, keep the session) instead of being stream-fatal — a lossy link can't be torn down by one bad block. - #10 [info] Fixed ALPN ("pkf1") on both native QUIC endpoints (defense-in-depth; a deliberate coordinated client+host upgrade — a new host rejects an ALPN-less old client). - #8 [info] Constant-time GameStream pairing phase-4 hash compare (crypto::ct_eq). - #7 [low] New VirtualDisplay::set_launch_command carries the launch command per-session on the GameStream path (no process-global env stomp under concurrent sessions); native path keeps the env under today's single-session model (documented; plumb per-session with concurrent sessions). - #5 [low] Legacy GameStream GCM nonce reuse: documented as inherent to Nvidia's old-style control encryption (Apollo/Moonlight identical; key is client-known) — unfixable on the legacy wire; the real fix is V2 control-encryption negotiation. Code comment at control.rs. - #9 [info] GameStream plain-HTTP pairing: documented (inherent to GFE compat; use punktfunk/1). - #12 [low] Web global NODE_TLS_REJECT_UNAUTHORIZED: fix designed (undici dispatcher scoped to the loopback mgmt fetch) but DEFERRED — needs `bun add undici` in the web build env; reverted to keep the web working. Latent-only (the loopback mgmt fetch is the console's only outbound TLS). fmt + clippy -D warnings clean; 94 host + core tests green; no C-ABI/OpenAPI drift. (The HDR Steps 1-2 client work in the tree is the user's parallel WIP — deliberately NOT included here.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -103,6 +103,18 @@ pub fn spawn(state: Arc<AppState>) -> Result<()> {
|
||||
}
|
||||
// Service the pads' force-feedback protocol every tick (games block inside
|
||||
// EVIOCSFF until answered) and relay mixed rumble levels to the client.
|
||||
//
|
||||
// SECURITY NOTE (audit #5, legacy GCM nonce reuse): on the LEGACY control scheme
|
||||
// (`NonceKind::Legacy*`, which we hit because we advertise no encryption) the nonce is
|
||||
// just the per-direction `seq` (`iv[0]=seq&0xff`, rest zero) with NO direction byte —
|
||||
// so host rumble (this `rumble_seq`) and client input (its own seq) share the same
|
||||
// (key, nonce) space when their seqs collide. This is INHERENT to Nvidia's old-style
|
||||
// GameStream control encryption (Apollo/moonlight-common-c are identical: only the V2
|
||||
// scheme adds `iv[10..12] = 'H','C'` to separate the host direction). It can't be fixed
|
||||
// on the legacy wire without breaking Moonlight; the GCM key is the client-supplied
|
||||
// `rikey` (so only a passive eavesdropper who missed the HTTPS /launch is the
|
||||
// adversary). The real fix is V2 control-encryption negotiation; for untrusted networks
|
||||
// use the native punktfunk/1 plane (correct per-direction nonces + seq-as-AAD).
|
||||
if let (Some(pid), Some(scheme)) = (peer, detected) {
|
||||
let key = state.launch.lock().unwrap().map(|s| s.gcm_key);
|
||||
if let Some(key) = key {
|
||||
|
||||
@@ -25,6 +25,12 @@ pub fn sha256(parts: &[&[u8]]) -> [u8; 32] {
|
||||
h.finalize().into()
|
||||
}
|
||||
|
||||
/// Constant-time byte-slice equality — no early exit, so a timing side-channel can't probe the
|
||||
/// expected value byte-by-byte. Returns false on a length mismatch (the length isn't secret here).
|
||||
pub fn ct_eq(a: &[u8], b: &[u8]) -> bool {
|
||||
a.len() == b.len() && a.iter().zip(b).fold(0u8, |acc, (x, y)| acc | (x ^ y)) == 0
|
||||
}
|
||||
|
||||
/// The PIN-derived AES-128 key: `SHA-256(salt || pin)[..16]` (salt first, PIN as ASCII).
|
||||
pub fn pin_key(salt: &[u8; 16], pin: &str) -> [u8; 16] {
|
||||
let d = sha256(&[salt, pin.as_bytes()]);
|
||||
|
||||
@@ -224,7 +224,8 @@ impl Pairing {
|
||||
let client_secret = &data[..16];
|
||||
let client_sig = &data[16..];
|
||||
let expected = crypto::sha256(&[&s.server_challenge, &s.client_cert_sig, client_secret]);
|
||||
let hash_ok = expected[..] == s.client_hash[..];
|
||||
// Constant-time compare so a timing side-channel can't probe the expected hash.
|
||||
let hash_ok = crypto::ct_eq(&expected, &s.client_hash);
|
||||
let sig_ok = verify256(&s.client_pubkey, client_secret, client_sig).is_ok();
|
||||
if hash_ok && sig_ok {
|
||||
{
|
||||
|
||||
@@ -15,12 +15,34 @@ use anyhow::{Context, Result};
|
||||
use std::collections::HashMap;
|
||||
use std::io::{Read, Write};
|
||||
use std::net::{TcpListener, TcpStream};
|
||||
use std::sync::atomic::Ordering;
|
||||
use std::sync::atomic::{AtomicUsize, Ordering};
|
||||
use std::sync::Arc;
|
||||
use std::time::Duration;
|
||||
|
||||
/// Opaque per-session payload the client echoes as its first UDP datagram (port-learning).
|
||||
const PING_PAYLOAD: &str = "0011223344556677";
|
||||
|
||||
// The RTSP listener is UNAUTHENTICATED (no TLS/pairing) and one-thread-per-connection, so bound
|
||||
// every attacker-controllable dimension to deny a pre-auth slow-loris / memory-growth DoS: a hard
|
||||
// cap on concurrent connections, a per-read timeout so a stalled peer can't pin a thread, and
|
||||
// size caps on the request headers + body (real GameStream RTSP messages are a few hundred bytes).
|
||||
const MAX_RTSP_CONNS: usize = 8;
|
||||
const RTSP_READ_TIMEOUT: Duration = Duration::from_secs(15);
|
||||
const MAX_RTSP_HEADER: usize = 16 * 1024;
|
||||
const MAX_RTSP_BODY: usize = 64 * 1024;
|
||||
const MAX_RTSP_MSG: usize = 128 * 1024;
|
||||
|
||||
/// Live RTSP connection count, so a flood can't spawn unbounded threads. Decremented by [`ConnGuard`].
|
||||
static RTSP_ACTIVE: AtomicUsize = AtomicUsize::new(0);
|
||||
|
||||
/// Decrements [`RTSP_ACTIVE`] when a connection thread exits (normally OR on panic).
|
||||
struct ConnGuard;
|
||||
impl Drop for ConnGuard {
|
||||
fn drop(&mut self) {
|
||||
RTSP_ACTIVE.fetch_sub(1, Ordering::Relaxed);
|
||||
}
|
||||
}
|
||||
|
||||
/// Bind 48010 and accept RTSP connections on a dedicated thread.
|
||||
pub fn spawn(state: Arc<AppState>) -> Result<()> {
|
||||
let listener = TcpListener::bind(("0.0.0.0", RTSP_PORT))
|
||||
@@ -32,8 +54,19 @@ pub fn spawn(state: Arc<AppState>) -> Result<()> {
|
||||
for conn in listener.incoming() {
|
||||
match conn {
|
||||
Ok(stream) => {
|
||||
// Reserve a slot; over the cap, drop the connection (close) without a thread.
|
||||
if RTSP_ACTIVE.fetch_add(1, Ordering::Relaxed) >= MAX_RTSP_CONNS {
|
||||
RTSP_ACTIVE.fetch_sub(1, Ordering::Relaxed);
|
||||
tracing::warn!("RTSP: too many concurrent connections — dropping");
|
||||
continue; // `stream` drops → connection closed
|
||||
}
|
||||
// Construct the slot guard BEFORE spawning and move it into the worker, so the
|
||||
// slot is released even if `thread::spawn` itself panics (OS thread-limit) —
|
||||
// the closure (and its captured guard) is dropped during the unwind.
|
||||
let guard = ConnGuard;
|
||||
let st = state.clone();
|
||||
std::thread::spawn(move || {
|
||||
let _guard = guard; // releases the slot on exit/panic
|
||||
if let Err(e) = handle_conn(stream, st) {
|
||||
tracing::warn!(error = %format!("{e:#}"), "RTSP connection ended");
|
||||
}
|
||||
@@ -57,6 +90,8 @@ struct Request {
|
||||
|
||||
fn handle_conn(mut stream: TcpStream, state: Arc<AppState>) -> Result<()> {
|
||||
let peer = stream.peer_addr().ok();
|
||||
// A per-read timeout so a stalled/slow-loris peer can't pin this thread indefinitely.
|
||||
let _ = stream.set_read_timeout(Some(RTSP_READ_TIMEOUT));
|
||||
let mut buf: Vec<u8> = Vec::new();
|
||||
// GameStream RTSP is one request per TCP connection: moonlight-common-c reads the
|
||||
// response until EOF, so we answer one message and close the connection (which signals
|
||||
@@ -82,10 +117,19 @@ fn handle_conn(mut stream: TcpStream, state: Arc<AppState>) -> Result<()> {
|
||||
fn read_message(stream: &mut TcpStream, buf: &mut Vec<u8>) -> Result<Option<Request>> {
|
||||
loop {
|
||||
if let Some(end) = find_subslice(buf, b"\r\n\r\n") {
|
||||
// Cap the header section even when the terminator IS present (a single oversized header
|
||||
// block that fits a `\r\n\r\n` would otherwise skip the no-terminator cap below).
|
||||
if end > MAX_RTSP_HEADER {
|
||||
anyhow::bail!("RTSP headers exceed limit");
|
||||
}
|
||||
let head = std::str::from_utf8(&buf[..end]).context("RTSP header utf8")?;
|
||||
let content_len = header_value(head, "content-length")
|
||||
.and_then(|v| v.trim().parse::<usize>().ok())
|
||||
.unwrap_or(0);
|
||||
// Reject an absurd Content-Length before waiting to buffer it (allocation amplification).
|
||||
if content_len > MAX_RTSP_BODY {
|
||||
anyhow::bail!("RTSP Content-Length {content_len} exceeds limit");
|
||||
}
|
||||
let total = end + 4 + content_len;
|
||||
if buf.len() < total {
|
||||
// headers complete but body still arriving — read more
|
||||
@@ -95,6 +139,9 @@ fn read_message(stream: &mut TcpStream, buf: &mut Vec<u8>) -> Result<Option<Requ
|
||||
buf.drain(..total);
|
||||
return Ok(Some(parse_request(&head, body)));
|
||||
}
|
||||
} else if buf.len() > MAX_RTSP_HEADER {
|
||||
// No header terminator within the cap — a slow-loris dribbling headers forever.
|
||||
anyhow::bail!("RTSP headers exceed limit");
|
||||
}
|
||||
let mut tmp = [0u8; 8192];
|
||||
let n = stream.read(&mut tmp).context("RTSP read")?;
|
||||
@@ -102,6 +149,9 @@ fn read_message(stream: &mut TcpStream, buf: &mut Vec<u8>) -> Result<Option<Requ
|
||||
return Ok(None); // peer closed
|
||||
}
|
||||
buf.extend_from_slice(&tmp[..n]);
|
||||
if buf.len() > MAX_RTSP_MSG {
|
||||
anyhow::bail!("RTSP message exceeds limit");
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -104,16 +104,11 @@ fn run(
|
||||
// output is released when this capturer drops at stream end (RAII via its keepalive).
|
||||
if std::env::var("PUNKTFUNK_VIDEO_SOURCE").as_deref() == Ok("virtual") {
|
||||
// The launched app picks the compositor (e.g. gamescope for game entries) and the
|
||||
// nested command; env vars remain manual overrides / fallbacks.
|
||||
// nested command.
|
||||
let compositor = app
|
||||
.and_then(|a| a.compositor)
|
||||
.map(Ok)
|
||||
.unwrap_or_else(|| crate::vdisplay::detect().context("detect compositor"))?;
|
||||
if let Some(cmd) = app.and_then(|a| a.cmd.as_deref()) {
|
||||
// The gamescope backend reads the nested command from this env var; setting it
|
||||
// per-launch is safe (one stream session at a time).
|
||||
std::env::set_var("PUNKTFUNK_GAMESCOPE_APP", cmd);
|
||||
}
|
||||
tracing::info!(
|
||||
?compositor,
|
||||
app = ?app.map(|a| &a.title),
|
||||
@@ -122,6 +117,9 @@ fn run(
|
||||
"video source: virtual display (native client resolution)"
|
||||
);
|
||||
let mut vd = crate::vdisplay::open(compositor).context("open virtual display")?;
|
||||
// Carry the resolved launch command on the backend instance (per-session) rather than a
|
||||
// process-global env var, so concurrent sessions can't stomp each other's launch target.
|
||||
vd.set_launch_command(app.and_then(|a| a.cmd.clone()));
|
||||
let vout = vd
|
||||
.create(punktfunk_core::Mode {
|
||||
width: cfg.width,
|
||||
|
||||
Reference in New Issue
Block a user