fix(security): tighten IPC socket perms + use constant-time token compare#331
Open
aaronjmars wants to merge 1 commit into
Open
Conversation
…pare
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).
fc069e2 to
327f4aa
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two small defense-in-depth fixes for the daemon IPC plumbing, bundled because they share the same threat surface (the daemon's IPC entry point) and the tests live next to each other.
POSIX AF_UNIX socket race.
_ipc.serve()chmod'd the socket file afterasyncio.start_unix_server()returned, butstart_unix_serverbegins accepting connections immediately. Under the default umask0o022the socket file is briefly mode0o755— a co-located unprivileged user couldconnect()during that window and start issuing CDP commands. Fix: tighten umask to0o077aroundstart_unix_serversobind()creates the socket with owner-only perms atomically. The explicitchmod 0o600stays as a belt-and-braces follow-up.Windows TCP loopback token compare.
Daemon.handle()usedreq.get("token") != expected, which short-circuits at the first differing byte. The token issecrets.token_hex(32)(256 bits) so the practical timing leak on loopback is small, buthmac.compare_digestis one import and constant-time. Fix: type-check the received value (req is parsed JSON, so any shape can land inreq["token"]), then compare withhmac.compare_digest.Impact
Both findings are local-only / loopback-only with high attacker-side cost — calling them low severity. Worth fixing because:
Location
src/browser_harness/_ipc.py:147-167—serve()POSIX pathsrc/browser_harness/daemon.py:259-271—Daemon.handle()token guardFix
_ipc.serve():os.umask(0o077)aroundawait asyncio.start_unix_server(...), restore in afinally. Existingos.chmod(path, 0o600)kept as a defence in depth for backends that historically didn't honour umask on AF_UNIXbind().daemon.Daemon.handle(): addhmacto module imports; replace the!=check withisinstance(received, str) and hmac.compare_digest(received, expected). The isinstance gate is required becausecompare_digestraisesTypeErroron non-str inputs, andreqis parsed JSON.Detected by
Aeon + semgrep + manual review.
Verification
python3 -m pytest tests/unit/— 79/79 pass (74 baseline + 5 new):test_serve_posix_socket_is_created_with_owner_only_perms— spiesasyncio.start_unix_serverto capture the active umask at bind-time + spiesos.chmodto confirm the 0o600 follow-uptest_handle_token_compare_uses_constant_time_compare— spieshmac.compare_digestto assert the constant-time path is usedtest_handle_rejects_non_string_token_without_crashing— coversNone, ints, bools, lists, dicts, bytestest_handle_accepts_correct_token— sanity check the right token still passestest_handle_skips_token_check_on_posix_when_expected_is_none— confirms POSIX path stays a no-op (AF_UNIX + chmod 600 is the boundary there)No production behaviour change for legitimate clients. Existing
helpers.py:_sendflows already pass the token viaipc.connect/ipc.request, which is unchanged.Notes
Filed by Aeon.
Summary by cubic
Hardened the daemon IPC entry point by creating POSIX sockets with owner-only permissions atomically and using constant-time token comparison on Windows. Fixes a short socket-perms race and removes a small timing side-channel; no impact to legitimate clients.
umask(0o077)aroundasyncio.start_unix_server(...); keep an explicitchmod 0o600as a follow-up.hmac.compare_digest; return unauthorized on mismatch.Written for commit 327f4aa. Summary will update on new commits.