From 327f4aa998334345e7109a18cb5069cc8d7539c6 Mon Sep 17 00:00:00 2001 From: Aeon Date: Sun, 10 May 2026 02:05:58 +0000 Subject: [PATCH] fix(security): tighten IPC socket perms + use constant-time token compare MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two small defense-in-depth fixes for the daemon IPC plumbing in _ipc.py and daemon.py: 1. _ipc.serve() (POSIX): asyncio.start_unix_server() begins accepting connections immediately, but the previous code only chmod'd the AF_UNIX socket file *after* bind() returned. Under the default umask 0o022 the file was briefly mode 0o755, so a co-located unprivileged user could connect() during that window and start issuing CDP commands (control the user's browser session, read cookies, navigate to internal URLs, etc.). Tighten umask to 0o077 around start_unix_server so bind() creates the file with owner-only perms atomically; the explicit chmod 0o600 stays as a belt-and-braces follow-up for backends that historically ignored umask on AF_UNIX bind(). 2. daemon.Daemon.handle() (Windows): the TCP-loopback token comparison used `req.get("token") != expected`, which short-circuits at the first differing byte. The token is 64 hex chars (256 bits of entropy) so the practical timing leak is small, but use hmac.compare_digest as a constant-time compare since it costs one import. Also type-check the received value first because compare_digest raises TypeError on non-str inputs (req is parsed JSON so the field can be any shape). Detected by Aeon + semgrep + manual review. Severity: low (both findings local-only / loopback-only, with high attacker-side cost). Bundled because they share the same threat surface (the daemon IPC entry point) and tests live next to each other in tests/unit/. Verification: `python3 -m pytest tests/unit/` passes 79/79 (5 new tests: socket-perm spy on umask + chmod, four token-compare cases — constant-time-compare invocation, non-string rejection, correct-token acceptance, POSIX no-op short-circuit). --- src/browser_harness/_ipc.py | 2 + src/browser_harness/daemon.py | 11 ++++-- tests/unit/test_daemon.py | 71 +++++++++++++++++++++++++++++++++ tests/unit/test_ipc.py | 74 +++++++++++++++++++++++++++++++++++ 4 files changed, 155 insertions(+), 3 deletions(-) diff --git a/src/browser_harness/_ipc.py b/src/browser_harness/_ipc.py index 2d265766..867a4b78 100644 --- a/src/browser_harness/_ipc.py +++ b/src/browser_harness/_ipc.py @@ -168,6 +168,8 @@ async def serve(name, handler): old_umask = os.umask(0o077) try: server = await asyncio.start_unix_server(handler, path=path) finally: os.umask(old_umask) + # Belt-and-braces in case a future asyncio backend doesn't honour umask. + os.chmod(path, 0o600) _server_token = None async with server: await asyncio.Event().wait() return diff --git a/src/browser_harness/daemon.py b/src/browser_harness/daemon.py index 0f0f2555..59d404c5 100644 --- a/src/browser_harness/daemon.py +++ b/src/browser_harness/daemon.py @@ -1,5 +1,5 @@ """CDP WS holder + IPC relay (Unix socket on POSIX, TCP loopback on Windows). One daemon per BU_NAME.""" -import asyncio, json, os, socket, sys, time, urllib.error, urllib.request +import asyncio, hmac, json, os, socket, sys, time, urllib.error, urllib.request from urllib.parse import urlparse from collections import deque from pathlib import Path @@ -262,9 +262,14 @@ async def handle(self, req): # Token guard for Windows TCP loopback: any local process can otherwise # connect and issue CDP commands. expected_token() is None on POSIX so # this check is a no-op there (AF_UNIX + chmod 600 is the boundary). + # hmac.compare_digest is constant-time so the per-character timing of + # `!=` can't be used to recover the token byte-by-byte; isinstance + # gate is required because compare_digest raises TypeError on non-str. expected = ipc.expected_token() - if expected is not None and req.get("token") != expected: - return {"error": "unauthorized"} + if expected is not None: + received = req.get("token") + if not isinstance(received, str) or not hmac.compare_digest(received, expected): + return {"error": "unauthorized"} meta = req.get("meta") # Liveness probe — lets clients confirm the listener is actually this # daemon and not an unrelated process that reused our port post-crash. diff --git a/tests/unit/test_daemon.py b/tests/unit/test_daemon.py index 90c5bc85..d9e1e5d9 100644 --- a/tests/unit/test_daemon.py +++ b/tests/unit/test_daemon.py @@ -1,5 +1,7 @@ import asyncio +import hmac +from browser_harness import _ipc as ipc from browser_harness import daemon @@ -198,6 +200,75 @@ async def run(): assert {"Page.enable", "DOM.enable", "Runtime.enable", "Network.enable"}.issubset(methods) +def test_handle_token_compare_uses_constant_time_compare(monkeypatch): + """Windows TCP loopback path requires every request to carry an opaque + token (secrets.token_hex(32) — 256 bits). The previous comparison was + `req.get("token") != expected`, which short-circuits at the first + differing byte and leaks the prefix length via timing. Use + hmac.compare_digest, which the stdlib documents as constant-time.""" + monkeypatch.setattr(ipc, "expected_token", lambda: "a" * 64) + calls = [] + real_compare = hmac.compare_digest + + def spy(a, b): + calls.append((a, b)) + return real_compare(a, b) + + monkeypatch.setattr(daemon.hmac, "compare_digest", spy) + + d = _fresh_daemon() + resp = asyncio.run(d.handle({"meta": "ping", "token": "b" * 64})) + + assert resp == {"error": "unauthorized"} + assert calls and calls[0] == ("b" * 64, "a" * 64), ( + f"handle() must call hmac.compare_digest(received, expected) before " + f"accepting a request. Got call list: {calls}" + ) + + +def test_handle_rejects_non_string_token_without_crashing(monkeypatch): + """req is parsed JSON: req.get('token') can be None, list, dict, int — + hmac.compare_digest raises TypeError on non-str/bytes. The handler must + type-check and reject cleanly instead of letting that TypeError bubble + out of the IPC dispatch loop.""" + monkeypatch.setattr(ipc, "expected_token", lambda: "a" * 64) + d = _fresh_daemon() + + for bad in (None, 0, 1, True, False, ["a"], {"a": 1}, b"a" * 64): + resp = asyncio.run(d.handle({"meta": "ping", "token": bad})) + assert resp == {"error": "unauthorized"}, ( + f"handle() must reject non-str token {bad!r} as unauthorized." + ) + + +def test_handle_accepts_correct_token(monkeypatch): + """Sanity check the constant-time path lets the right token through — + otherwise the compare_digest swap would silently lock everyone out.""" + expected = "a" * 64 + monkeypatch.setattr(ipc, "expected_token", lambda: expected) + d = _fresh_daemon() + + resp = asyncio.run(d.handle({"meta": "ping", "token": expected})) + assert resp.get("pong") is True + + +def test_handle_skips_token_check_on_posix_when_expected_is_none(monkeypatch): + """expected_token() is None on POSIX (AF_UNIX + chmod 600 is the + boundary). The handler must short-circuit *before* touching token + fields, so a missing or hostile token shape never trips the type guard + and a legitimate POSIX request reaches its meta branch.""" + monkeypatch.setattr(ipc, "expected_token", lambda: None) + d = _fresh_daemon() + + # No token at all. + resp = asyncio.run(d.handle({"meta": "ping"})) + assert resp.get("pong") is True + + # Token of a wrong type — must still pass (no Windows guard active). + resp = asyncio.run(d.handle({"meta": "ping", "token": [1, 2, 3]})) + assert resp.get("pong") is True + + def test_set_session_first_attach_runs_four_enables_in_parallel(): """When there's no previous session, the disable path is skipped — only the four enables run, still in parallel.""" diff --git a/tests/unit/test_ipc.py b/tests/unit/test_ipc.py index 96e2dbc6..5488dd4d 100644 --- a/tests/unit/test_ipc.py +++ b/tests/unit/test_ipc.py @@ -1,6 +1,80 @@ +import asyncio +import os +import stat +import sys + +import pytest + from browser_harness import _ipc as ipc +# --- serve(): AF_UNIX socket created with owner-only permissions --- + +@pytest.mark.skipif(sys.platform == "win32", reason="POSIX-only: AF_UNIX socket perms") +def test_serve_posix_socket_is_created_with_owner_only_perms(tmp_path, monkeypatch): + """The AF_UNIX socket file must be mode 0o600 from the moment bind() + creates it, not just after the explicit chmod. asyncio.start_unix_server + begins accepting connections immediately, so a co-located unprivileged + user racing connect() against the chmod could otherwise issue CDP + commands during that window. Tightening the umask around bind() makes + the permissions correct atomically; this test asserts (a) umask is set + to 0o077 by the time start_unix_server's bind() runs, and (b) chmod + 0o600 fires on the resulting path as a defence-in-depth follow-up.""" + monkeypatch.setattr(ipc, "BH_TMP_DIR", str(tmp_path)) + monkeypatch.setattr(ipc, "_TMP", tmp_path) + + captured = {"umask_during_bind": None} + chmod_calls = [] + real_start = asyncio.start_unix_server + real_chmod = os.chmod + + async def spy_start(handler, path): + # Snapshot the umask the kernel will see when bind() creates the + # socket file. os.umask(0) flips to 0 and returns the prior value; + # restore immediately so we don't disturb the run. + current = os.umask(0) + os.umask(current) + captured["umask_during_bind"] = current + return await real_start(handler, path) + + def spy_chmod(path, mode): + chmod_calls.append((str(path), mode)) + return real_chmod(path, mode) + + monkeypatch.setattr(asyncio, "start_unix_server", spy_start) + monkeypatch.setattr(os, "chmod", spy_chmod) + + async def run(): + async def handler(reader, writer): + writer.close() + + task = asyncio.create_task(ipc.serve("default", handler)) + # Yield until serve() has reached its blocking point — chmod is the + # last sync step before `await asyncio.Event().wait()`, so seeing + # the chmod call is the cleanest "serve is past setup" signal. + for _ in range(200): + await asyncio.sleep(0) + if chmod_calls and captured["umask_during_bind"] is not None: + break + task.cancel() + try: + await task + except (asyncio.CancelledError, BaseException): + pass + + asyncio.run(run()) + assert captured["umask_during_bind"] == 0o077, ( + f"serve() must tighten umask to 0o077 before start_unix_server's " + f"bind() creates the socket file. Saw umask=" + f"{oct(captured['umask_during_bind']) if captured['umask_during_bind'] is not None else 'unset'}." + ) + sock_path = str(tmp_path / "bu.sock") + assert (sock_path, 0o600) in chmod_calls, ( + f"serve() must chmod the socket file to 0o600 as a belt-and-braces " + f"defence after umask. Got chmod calls: {chmod_calls}" + ) + + # --- identify(): ping payload sanitation --- class _FakeConn: