fix(browser): verify daemon identity before orphan reaper kills a PID (#14073)

[teknium1]

Summary

The browser orphan reaper can no longer SIGTERM an arbitrary same-user process — it now verifies a live PID really is this session's agent-browser daemon before tree-killing it. Closes #14073.

Root cause: _reap_orphaned_browser_sessions() reads a daemon PID from <session>.pid in a world-writable, predictably-named temp dir (/tmp/agent-browser-h_*) that we don't write — the daemon does — then calls _terminate_host_pid(pid) (a process-tree kill) after only a liveness check. A same-user actor can plant a fake socket dir whose .pid points at any victim PID (no .owner_pid → legacy path → untracked → reaped), and OS PID reuse after the real daemon exits lands the recorded PID on an unrelated process. Either way → arbitrary same-user process DoS.

Changes

• tools/browser_tool.py: new _verify_reapable_browser_daemon(pid, socket_dir, session), gated before the kill. Via psutil (hard dep; fine cross-platform for the same-user processes the reaper can signal) it requires both:
  1. Identity — agent-browser appears in the process name or cmdline.
  2. Binding — the live process references this session's socket dir, via cmdline or AGENT_BROWSER_SOCKET_DIR in its environ.
     Fail-closed on any ambiguity (unreadable cmdline, no match): the process and its socket dir are left untouched for a later sweep.
• tests/tools/test_browser_orphan_reaper.py: new TestReaperIdentityGuard (8 cases) + a real-process E2E in-test; the three pre-existing "should reap" tests now mock the guard True (its own behavior is covered separately).

The binding check (2) is the real spoof defense: a planted/recycled PID won't embed our exact session socket path. An attacker would need a process that genuinely references this dir — i.e. a real daemon they already own and could signal directly.

Why this implementation

Builds on @sgaofen's fix in #14394 (best-effort cmdline identity check). Rewritten to:

• use psutil instead of /proc+ps — cross-platform, so Windows is covered (the original was POSIX-only, matches gateway/status.py's existing proc.cmdline() usage); and
• add the session-socket-dir binding check, which the cmdline-only version lacked — this is what closes the recycled-PID hole, not just the wrong-binary one.

Two adjacent open PRs are separate concerns, not duplicates of this: #43846 (recycled-PID start-time identity in process_registry/whatsapp) and #15008 (SIGTERM→SIGKILL escalation). Neither closes #14073.

Validation

	Result
tests/tools/test_browser_orphan_reaper.py	26 passed
tests/tools/test_browser_hardening.py + test_browser_cleanup.py	30 passed
E2E, real processes (planted .pid → real unrelated sleep)	victim survives, socket dir retained
E2E (real process cmdline-bound to our socket dir)	still reaped — feature preserved

Co-authored-by: sgaofen 135070653+sgaofen@users.noreply.github.com

Infographic

[github-actions]

🔎 Lint report: hermes/hermes-815c35d4 vs origin/main

ruff

Total: 0 on HEAD, 0 on base (➖ 0)

🆕 New issues: none

✅ Fixed issues: none

Unchanged: 0 pre-existing issues carried over.

ty (type checker)

Total: 11344 on HEAD, 11341 on base (🆕 +3)

🆕 New issues (2):

Rule	Count
unresolved-import	2

First entries

tools/browser_tool.py:1354: [unresolved-import] unresolved-import: Cannot resolve imported module `psutil`
tests/tools/test_browser_orphan_reaper.py:419: [unresolved-import] unresolved-import: Cannot resolve imported module `psutil`

✅ Fixed issues: none

Unchanged: 5955 pre-existing issues carried over.

Diagnostics are surfaced as warnings — this check never fails the build.

[alt-glitch]

Related: #14073 (the bug — browser orphan reaper trusts /tmp PID files and can SIGTERM arbitrary same-user processes) and #14394 (earlier community attempt). This is the authoritative fix adding _verify_reapable_browser_daemon to confirm a PID really is this session's agent-browser daemon before tree-killing it, defeating planted pid files and recycled PIDs.

[egilewski]

looks mergeable

I reviewed the browser orphan reaper hardening against current GitHub main and did not find a security blocker. The patch adds a fail-closed _verify_reapable_browser_daemon() gate before the orphan reaper calls ProcessRegistry._terminate_host_pid(): a live PID now has to look like agent-browser and be bound to the scanned session socket directory through its command line or AGENT_BROWSER_SOCKET_DIR. That addresses the planted/reused .pid file path without removing cleanup for real orphaned browser daemons.

Evidence I checked:

• git merge-tree --write-tree 6f0ecf37dad0bcb989ea6139def524e6f0304d55 f1adb009d8a31d25fd961f0e19958bb2894d984c succeeded with tree d86db6ed4b3037af629dd61dcb5494f8e4aa3437.
• git diff --check f79e0a7060d0303f4f248d2e03b101909748e781..f1adb009d8a31d25fd961f0e19958bb2894d984c was clean.
• python -B -m py_compile tools/browser_tool.py tests/tools/test_browser_orphan_reaper.py on the PR worktree passed.
• A mocked current-main probe reproduced the old sink: an alive PID read from a planted legacy socket dir reached the mocked _terminate_host_pid(12345).
• The same mocked planted-PID probe on PR head left terminate_calls empty and kept the socket dir.
• A positive PR-head probe with an agent-browser process bound to that exact socket dir still reached the mocked termination sink and removed the socket dir.

Security evidence:

• trust boundary: world-writable, predictable agent-browser-* temp socket directories and daemon .pid files read during orphan cleanup.
• source/sink/invariant: untrusted .pid file to ProcessRegistry._terminate_host_pid(); invariant is "only reap a daemon that is genuinely this session's agent-browser process".
• current-main reproduction: mocked current-main probe showed a planted live PID reaches the termination sink.
• PR-head or patch-replay validation: mocked PR-head probe blocks a planted non-browser PID before the sink.
• positive/negative cases: non-browser planted PID is refused; agent-browser process bound to the exact socket dir is still reaped.
• residual bypass search: checked the new identity and binding checks, access-denied/no-such-process fail-closed handling, and the full reaper path before the termination call; no remaining same-user arbitrary-process kill path found in the changed scope.
• reviewer-tool status: CodeRabbit completed with no findings in the clean-pass flow.

Signed: GPT-5.5-xhigh in Codex