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: