fix(stream): gate every vhost and refuse CRLF in broker headers

A full security review of the broker, the s6 init scripts, the sudoers
rule and the Dockerfile found two exploitable faults. Both are fixed
here. The third finding, an unset BROKER_SECRET accepting every request,
stays as it is and is now written up in the README as a known debug-only
hole rather than treated as a bug.

The nginx gate covered one vhost. The awk that injects it anchored on
ssl_certificate_key, which appears only in the 3001 SSL server block,
but the base image ships a second, identical plain-HTTP vhost on 3000:
same /websocket proxy to selkies, same /files alias of /config/Desktop.
Anyone reaching port 3000 got the full interactive desktop with the ROM
library mounted and no stream token asked for, and publishing 3000 is
exactly what the base image's own docs tell you to do. Confirmed by
reading /etc/nginx/sites-available/default out of the running container.

The anchor is now every `server {` block. /50x.html is exempted because
it is the error_page target, and a gated error page turns a broker
outage into an internal-redirect loop. The injected port comes from
BROKER_PORT instead of a hardcoded 8000. The script counts the vhosts it
gated, warns when that count is under the two the base image ships, and
reports an ERROR when it matched nothing rather than claiming success.

stream_sid is a Secure cookie, so reaching 3000 directly over plain HTTP
now admits the query token but cannot persist the cookie: 3000 works
behind a TLS-terminating proxy, and unproxied plain HTTP fails closed
instead of serving the desktop.

Two response headers echoed filenames straight to the wire.
X-State-Filename and the save-file Content-Disposition both carry names
taken off the filesystem, where CR and LF are legal bytes, and
http.server writes header values through without validating them. A
state file named with a CRLF could split the response and inject headers
of the attacker's choosing into a browser that fetched it. Both emit
sites now run through a shared _header_token, and PUT /state-file
refuses a non-printable-ASCII filename outright rather than storing a
name it would have to mangle on the way back out.

Hardening applied alongside. The shared secret is compared as bytes,
because hmac.compare_digest refuses a str carrying non-ASCII characters,
so a UTF-8 BROKER_SECRET would have raised inside every check and
answered 500 to every request. The gated vhosts also send
Referrer-Policy: same-origin, so the stream token in the iframe URL does
not leak through a Referer.

Twelve tests cover the new behavior: the header filter, the PUT refusal,
the GET sanitizing a CRLF name already on disk, and the secret
comparison across the UTF-8, ASCII, missing-header and unset-secret
cases.

Deploying this needs a recreate, not a restart. The idempotency check
greps the site config for the marker it already wrote, and /etc/nginx
lives in the container's writable layer, so a restarted container keeps
its single-vhost gate.

Author: LoneAngelFayt

README.md

@@ -108,7 +108,7 @@ Save states don't care about any of this. The requirement is only for memory-car
 
 | Variable | Default | What it does |
 |---|---|---|
-| `BROKER_SECRET` | *(none)* | Shared secret, sent as `X-Broker-Secret`. Unset means every request is accepted, which is not safe on a shared network. |
+| `BROKER_SECRET` | *(none)* | Shared secret, sent as `X-Broker-Secret`. Unset means every request is accepted. Debug-only, see [Security](#security). |
 | `BROKER_PORT` | `8000` | Port the broker listens on. |
 | `ROM_ROOT` | `/romm/library` | Where ROMs are mounted. A `rom_path` outside this is rejected. |
 | `SAVE_SLOT` | `10` | Slot `/save-and-exit` uses when none is given. 10 as autosave leaves 1-9 for the player. |
@@ -208,6 +208,18 @@ State files are named `{SERIAL} ({CRC}).{slot:02d}.p2s`, which is PCSX2's own sc
 
 It is refused while a game is running or a launch is in flight, because PCSX2 holds the card open and swapping it mid-game corrupts it. Hydrate before launch or after exit. `POST /launch` is refused for the duration too, so a launch can't slip in mid-replace. Dashboard crash recovery is *not* blocked, since the gameless dashboard never opens a card and the swap is atomic either way.
 
+## Security
+
+Two credentials, guarding two different things.
+
+**`BROKER_SECRET` guards the API on port 8000.** Sent as `X-Broker-Secret`, compared in constant time. `GET /health` and `GET /verify` are deliberately exempt: `/health` so it works as a container healthcheck, `/verify` because nginx's `auth_request` cannot forward the secret and the stream token is itself the credential there.
+
+**The stream token guards the desktop on 3000/3001.** It is minted per session by `POST /launch`, 256 bits from `secrets.token_urlsafe`, and enforced by an nginx `auth_request` that the mod injects into *every* `server` block in the site config. That matters: the base image ships two identical vhosts, plain HTTP on 3000 and TLS on 3001, both proxying the same selkies stream and both serving `/config/Desktop` at `/files`. Gating only the TLS one left a complete bypass a port number away. The `stream_sid` cookie is `Secure`, so 3000 is usable only behind a TLS-terminating proxy — direct plain-HTTP browsing to it now fails closed.
+
+**Leaving `BROKER_SECRET` unset is a known hole, not a supported mode.** The broker runs as root inside the container, so the open API means root-privileged reads and writes under `/config`, plus arbitrary launches within `ROM_ROOT`. Worse, the two credentials stop being independent: `POST /launch` *returns* a stream token, so an unauthenticated broker hands out the credential that opens the desktop. Use it for local debugging on a trusted host and nothing else. The broker logs a warning at startup when it is unset.
+
+Port 8000 is plain HTTP, so the secret crosses the network in the clear. Keep the broker on an internal network, or put TLS in front of it.
+
 ## Troubleshooting
 
 **Reading the logs.** Every request the broker refuses or fails is logged to stdout, not only returned as JSON, so `docker logs pcsx2` is enough to see what went wrong without turning on `DEBUG`. The shape is `HTTP <code> <method> <path> from <caller>: <reason>`. A `WARNING` is a caller-side rejection (bad slot, no game running, a claim already held); an `ERROR` is a broker-side fault (pactl failed, a state file that could not be written) and always deserves attention. An unhandled crash inside a handler logs a full traceback and still answers `500 internal broker error` rather than dropping the connection. Stream tokens are redacted from logged URLs, so log output is safe to paste into a bug report.
@@ -238,7 +250,9 @@ The usual cause is the hardware encoder: Selkies routes `x264enc` through VAAPI
 
 Confirm it took with `docker logs pcsx2 | grep -E "VAAPI|CPU Software"`. If it still cycles, drop to `SELKIES_ENCODER=jpeg` to skip H.264 entirely, at a real cost in bandwidth and sharpness.
 
-**"An error occurred, restarting stream", over and over.** The stream client says this whenever its WebSocket drops, so the useful signal is on the broker side: `docker logs pcsx2 | grep "/verify"`. A `403` there names the reason (`no stream token in the request`, `stream token expired`, `stream token superseded by a newer launch`), and the token is redacted so the line is safe to share. No `/verify` lines *at all* means the gate is not running: either the mod's init never patched nginx (`docker logs pcsx2 | grep broker-mod`), or your reverse proxy is reaching the ungated plain-HTTP vhost on port 3000 instead of the SSL vhost on 3001. Point it at 3001 and do not publish 3000: the gate is the only thing standing between the ROM library and anyone who finds the address.
+**"An error occurred, restarting stream", over and over.** The stream client says this whenever its WebSocket drops, so the useful signal is on the broker side: `docker logs pcsx2 | grep "/verify"`. A `403` there names the reason (`no stream token in the request`, `stream token expired`, `stream token superseded by a newer launch`), and the token is redacted so the line is safe to share. No `/verify` lines *at all* means the gate is not running: the mod's init never patched nginx. Check `docker logs pcsx2 | grep broker-mod` for the "Applied nginx stream gate to N vhost(s)" line, which should say 2.
+
+**A stream that 403s on every asset over plain HTTP.** The `stream_sid` cookie is `Secure`, so a browser will not store it on an unencrypted origin. The query token admits the first request and everything after it is refused. Reach the container over TLS: either port 3001 directly, or port 3000 behind a reverse proxy that terminates TLS. Plain-HTTP 3000 straight to a browser is not a supported shape and now fails closed rather than serving an ungated desktop.
 
 ## Development
 
@@ -250,7 +264,7 @@ python3 -m unittest discover -s tests -v
 
 CI runs the same tests under pytest, plus `ruff check .` against the shared [ruff.toml](ruff.toml). `tests/` is excluded from the image by `.dockerignore` and never ships.
 
-The suite covers what can be tested without a running emulator: save-file and memory-card archive handling (round trips, the last-write-wins mtime guard, path-traversal and subtree rejection, File-card refusal), `PCSX2.ini` patching, ROM path resolution, and the session state machine (crash-loop limiter, launch and card-op claims, deferred `load_slot` generation checks). Anything needing a real X display, PINE socket, or pcsx2-qt process is out of scope on purpose, because it is only provable on a live container.
+The suite covers what can be tested without a running emulator: save-file and memory-card archive handling (round trips, the last-write-wins mtime guard, path-traversal and subtree rejection, File-card refusal), `PCSX2.ini` patching, ROM path resolution, the session state machine (crash-loop limiter, launch and card-op claims, deferred `load_slot` generation checks), the stream-token gate decision, and the header/secret handling that keeps a filename off the response line and a UTF-8 secret out of a 500. Anything needing a real X display, PINE socket, or pcsx2-qt process is out of scope on purpose, because it is only provable on a live container.
 
 **Init ordering matters more than it looks.** The mod ships two s6 oneshots. `init-pcsx2-config` rewrites files that base-image services read exactly once at startup (selkies' `input_handler.py`, the nginx site config, labwc's autostart), so `init-services` is made to depend on it and the whole service stack waits. Drop that edge and the patches still land on disk while changing nothing about the processes already running, which fails silently: the log says "Applied nginx stream gate" and the live nginx has no gate. Keep that script fast and offline. `init-pcsx2-deps` holds the slow networked work (apt, `patches.zip`) and only `svc-broker` waits for it, so a GitHub timeout cannot stall the stream.
 

root/etc/s6-overlay/s6-rc.d/init-pcsx2-config/init.sh

@@ -110,47 +110,72 @@ else
     echo "[broker-mod] ERROR: selkies input_handler.py not found under /lsiopy, /usr/lib, /usr/local/lib, /opt or /config/.local, so neither the gamepad EOF fix nor the log-level patch was applied"
 fi
 
-# Gate the browser-facing stream with nginx auth_request. The 3001 SSL vhost is
-# the host RomM loads in the iframe; without this, anyone who learns the address
-# gets an interactive desktop with the ROM library mounted, since RomM's auth
-# never sits on this socket. auth_request sends every 3001 request to the
-# broker's /verify, which checks the session-bound stream token RomM appends to
-# the iframe URL and, on the first (query-token) hit, hands back a stream_sid
-# cookie that carries every later asset and the WebSocket upgrade. Anchored on
-# ssl_certificate_key, which appears only in the 3001 server block, so the plain
-# 3000 vhost is untouched. The broker exempts /verify from its shared secret
-# because nginx cannot forward that secret and the stream token is the credential.
+# Gate the browser-facing stream with nginx auth_request. Without this, anyone
+# who learns the address gets an interactive desktop with the ROM library
+# mounted, since RomM's auth never sits on this socket. auth_request sends every
+# request to the broker's /verify, which checks the session-bound stream token
+# RomM appends to the iframe URL and, on the first (query-token) hit, hands back
+# a stream_sid cookie that carries every later asset and the WebSocket upgrade.
+# The broker exempts /verify from its shared secret because nginx cannot forward
+# that secret and the stream token is the credential.
+#
+# EVERY server block is gated, not just the 3001 SSL vhost RomM points at. The
+# base image ships a second, identical plain-HTTP vhost on 3000 (same /websocket
+# proxy to selkies, same /files alias of /config/Desktop), so anchoring this on
+# ssl_certificate_key left a complete bypass of the gate one port number away,
+# and publishing 3000 is exactly what the base image's own docs tell you to do.
+# Note that stream_sid is a Secure cookie: reaching 3000 directly over plain
+# HTTP admits the query token but the cookie will not stick, so 3000 only works
+# behind a proxy that terminates TLS. That is the supported shape; unproxied
+# plain-HTTP 3000 is not, and now fails closed instead of serving the desktop.
+#
+# /50x.html is exempted because it is the error_page target: left gated, a
+# broker outage makes each request bounce between the 500 and its own gated
+# error page until nginx's internal-redirect limit trips.
 NGINX_SITE="/etc/nginx/sites-available/default"
 if [ -f "$NGINX_SITE" ]; then
     if grep -q "RomM stream gate" "$NGINX_SITE"; then
         echo "[broker-mod] nginx stream gate already applied."
     else
-        awk '
-          { print }
-          /ssl_certificate_key/ && !injected {
+        awk -v bport="${BROKER_PORT:-8000}" '
+          /^[[:space:]]*server[[:space:]]*\{/ {
+            print
             print "  # ── RomM stream gate (pcsx2-broker-mod) ──"
             print "  auth_request /_stream_auth;"
             print "  auth_request_set $stream_set_cookie $upstream_http_set_cookie;"
             print "  add_header Set-Cookie $stream_set_cookie;"
+            print "  add_header Referrer-Policy \"same-origin\" always;"
             print "  location = /_stream_auth {"
             print "    internal;"
             print "    auth_request off;"
-            print "    proxy_pass http://127.0.0.1:8000/verify;"
+            print "    proxy_pass http://127.0.0.1:" bport "/verify;"
             print "    proxy_pass_request_body off;"
             print "    proxy_set_header Content-Length \"\";"
             print "    proxy_set_header X-Original-URI $request_uri;"
             print "  }"
-            injected = 1
+            next
+          }
+          /^[[:space:]]*location[[:space:]]*=[[:space:]]*\/50x\.html[[:space:]]*\{/ {
+            print
+            print "    auth_request off;"
+            next
           }
-        ' "$NGINX_SITE" > "$NGINX_SITE.tmp" && mv "$NGINX_SITE.tmp" "$NGINX_SITE"
-        if grep -q "RomM stream gate" "$NGINX_SITE"; then
-            echo "[broker-mod] Applied nginx stream gate to 3001 vhost."
+          { print }
+        ' "$NGINX_SITE" > "$NGINX_SITE.tmp"
+        gated=$(grep -c "RomM stream gate" "$NGINX_SITE.tmp" 2>/dev/null)
+        [ -n "$gated" ] || gated=0
+        if [ "$gated" -gt 0 ]; then
+            mv "$NGINX_SITE.tmp" "$NGINX_SITE"
+            echo "[broker-mod] Applied nginx stream gate to $gated vhost(s)."
+            if [ "$gated" -lt 2 ]; then
+                echo "[broker-mod] WARNING: only $gated vhost gated; the base image ships two (3000 and 3001). Any other listener is serving the desktop ungated."
+            fi
             if ! nginx -t 2>/dev/null; then
                 echo "[broker-mod] WARNING: nginx -t failed now (ssl cert may not exist yet at init); nginx revalidates at start."
             fi
         else
             rm -f "$NGINX_SITE.tmp"
-            echo "[broker-mod] ERROR: nginx stream gate not applied (ssl_certificate_key anchor missing; base image may have changed)."
+            echo "[broker-mod] ERROR: nginx stream gate not applied (no 'server {' block matched; base image may have changed)."
         fi
     fi
 else

root/root/broker.py

@@ -29,6 +29,11 @@
 
 PORT     = int(os.environ.get("BROKER_PORT", "8000"))
 SECRET   = os.environ.get("BROKER_SECRET", "")
+# The secret is compared as bytes, never as str: hmac.compare_digest refuses a
+# str with non-ASCII characters, so a UTF-8 BROKER_SECRET would raise inside
+# every _check_secret and answer 500 to every request. Encoding once here keeps
+# the comparison total for any secret an operator can set.
+_SECRET_BYTES = SECRET.encode("utf-8")
 ROM_ROOT = Path(os.environ.get("ROM_ROOT", "/romm/library")).resolve()
 
 # JSON request bodies are tiny (a rom_path, a slot number); anything larger
@@ -1812,6 +1817,18 @@ def _redact_uri(uri: str) -> str:
     return re.sub(r"(stream_token=)[^&]*", r"\1REDACTED", uri)
 
 
+def _header_token(value: str, fallback: str) -> str:
+    """Reduce `value` to something safe to send as an HTTP header value.
+
+    Header values are latin-1 and http.server writes them through without
+    validating, so a CR or LF reaching send_header splits the response. Both
+    callers pass names taken off the filesystem, where those bytes are legal
+    in a filename, so the filter belongs here rather than at each call site.
+    """
+    safe = "".join(c for c in value if c.isascii() and c.isprintable()).strip()
+    return safe or fallback
+
+
 class BrokerHandler(BaseHTTPRequestHandler):
 
     def log_message(self, fmt, *args):
@@ -1820,9 +1837,12 @@ def log_message(self, fmt, *args):
     def _check_secret(self) -> bool:
         if not SECRET:
             return True
+        # latin-1 is how http.server decoded the header, so encoding it back
+        # recovers exactly the bytes that arrived on the wire, which is what
+        # _SECRET_BYTES is comparable against.
         return hmac.compare_digest(
-            self.headers.get("X-Broker-Secret", ""),
-            SECRET,
+            self.headers.get("X-Broker-Secret", "").encode("latin-1", "replace"),
+            _SECRET_BYTES,
         )
 
     def _verify_stream(self) -> None:
@@ -1967,7 +1987,9 @@ def _get_state_file(self):
         self.send_response(200)
         self.send_header("Content-Type", "application/octet-stream")
         self.send_header("Content-Length", str(len(content)))
-        self.send_header("X-State-Filename", state_path.name)
+        self.send_header(
+            "X-State-Filename", _header_token(state_path.name, "state.p2s")
+        )
         self.end_headers()
         self.wfile.write(content)
         log.info("state-file: served %s (%d bytes)", state_path.name, len(content))
@@ -1996,9 +2018,7 @@ def _get_save_file(self):
             )
             return
         # Header values must be latin-1; ROM stems can be anything.
-        safe_name = "".join(
-            c for c in (rom_name or "pcsx2") if c.isascii() and c.isprintable()
-        ).strip() or "pcsx2"
+        safe_name = _header_token(rom_name or "pcsx2", "pcsx2")
         self.send_response(200)
         self.send_header("Content-Type", "application/zip")
         self.send_header("Content-Length", str(len(archive)))
@@ -2155,6 +2175,13 @@ def _handle_PUT(self):
         if not filename or filename.startswith(".") or not filename.endswith(".p2s"):
             self._send_json(400, {"error": "filename must be a .p2s basename"})
             return
+        # CR and LF are legal in a Linux filename, so without this a stored name
+        # carrying them would split the response when GET /state-file echoes it
+        # back as X-State-Filename. _header_token sanitises that emit too; this
+        # refuses the name outright rather than silently storing a mangled one.
+        if filename != _header_token(filename, ""):
+            self._send_json(400, {"error": "filename must be printable ASCII"})
+            return
 
         try:
             length = int(self.headers.get("Content-Length", 0))