[security] fix(process): guard stdin submissions - #22557
Conversation
|
CI note: the |
5e0b15f to
7848b79
Compare
7166e89 to
a0728fc
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying a real second-stage execution boundary: current main still writes process.write/process.submit payloads directly to PTY or pipe sinks in tools/process_registry.py:1526-1545.
Problems
- The added
check_all_command_guards(command_text, "local")call attools/process_registry.py:96omits the interactive approval callback. The normal terminal route supplies it attools/terminal_tool.py:281-286;tools/approval.py:2907-2909only forwards an explicitly supplied callback, andtools/approval.py:1698-1718denies callback-less prompt_toolkit approvals. This would make dangerous interactive stdin submissions fail closed rather than use the established approval UI. - The new tests cover mocked pipe writes only. Current interactive stdin is PTY-backed (
tests/tools/test_process_registry.py:473-489), so the user-facing sink needs coverage too.
Suggested changes
- Route stdin approval through the existing callback-aware terminal guard, or pass the current terminal approval callback explicitly.
- Add PTY-path and callback-path regression coverage alongside the blocked-write assertion.
Automated hermes-sweeper review.
| command_text = data | ||
| from tools.approval import check_all_command_guards | ||
|
|
||
| approval = check_all_command_guards(command_text, "local") |
There was a problem hiding this comment.
This bypasses the terminal wrapper that passes _get_approval_callback() (tools/terminal_tool.py:281-286). On current main, check_all_command_guards forwards only its explicit callback to prompt_dangerous_approval; without one, an active prompt_toolkit CLI fails closed instead of showing the established approval UI. Please preserve that callback path here.
a0728fc to
2ec3dd9
Compare
|
Thanks, updated in 2ec3dd9. Changes made:
Validation:
|
Summary
This PR hardens the background-process stdin boundary so
process.writeandprocess.submitcannot be used as a second-stage command execution channel that bypasses terminal approval.Hermes already runs dangerous-command and hardline checks before starting a command through
terminal(). Before this PR, those checks covered only the initial process command. A model/tool flow could start an innocuous interactive process such asbash, then submit dangerous command text through theprocesstool's stdin path without reusing the same approval guard.This PR:
write_stdin()andsubmit_stdin().Security issues covered
process.write/process.submitcould feed dangerous commands into an already-running shell without terminal approvalBefore this PR
terminal()checked_check_all_guards(command, env_type)only for the initial command.terminal(command="bash", background=True)was allowed because the launcher itself is not dangerous.process(action="submit", data="rm -rf $HOME")wrote directly to the shell's stdin.ProcessRegistry.write_stdin()sent data tosession._pty.write(...)orsession.process.stdin.write(...)without calling the approval guard.After this PR
ProcessRegistry.write_stdin()calls a shared stdin guard before writing any data to the process.tools.approval.check_all_command_guards(..., "local"), preserving the existing terminal approval semantics.Why this matters
Background process stdin is an execution channel when the target process is a shell or interpreter. Treating only the process launcher as approval-relevant leaves a gap: the safe-looking launcher can be approved while the dangerous command arrives one tool call later.
That breaks the user's expectation that catastrophic local commands go through Hermes' dangerous-command and hardline approval layer before execution.
How this differs from related issue/PR
Several public items already touch terminal approval, but this patch fixes a distinct second-stage channel:
execute_codesandbox access to the terminal tool.force=Trueparameter.shell.execfail-open behavior when the approval module cannot load.Those items focus on the initial command, UI/transport behavior, or individual detection patterns. This PR covers the later
ProcessRegistry.write_stdin()/submit_stdin()path, where command text can reach an already-running process without passing through the same guard.Attack flow
Affected code
tools/process_registry.py,tools/terminal_tool.py,tools/approval.pytests/tools/test_process_registry.pyRoot cause
Process stdin approval bypass:
ProcessRegistry.write_stdin()wrote stdin data directly to PTY or pipe sinks without applying approval checks.CVSS assessment
CVSS:3.1/AV:L/AC:L/PR:L/UI:R/S:U/C:H/I:H/A:HRationale:
Safe reproduction steps
rmso it writes a marker instead of deleting anything:submitreturnsstatus: blocked, and the marker is not created.Expected vulnerable behavior
On vulnerable code:
With this PR:
Changes in this PR
_check_process_stdin_guards()intools/process_registry.py.tools.approval.check_all_command_guards(..., "local")for process stdin payloads.blocked/approval_requiredresponses.stdin.write()/stdin.flush().Files changed
tools/process_registry.pytests/tools/test_process_registry.pywrite_stdin, blockedsubmit_stdin, and safe stdin writesMaintainer impact
terminal()or through a laterprocess.submitcall.Fix rationale
The right boundary is immediately before stdin data reaches the running process. Checking only at process creation cannot protect interactive shells, REPLs, or interpreters because executable command text may arrive later.
Reusing the existing terminal guard keeps the policy consistent and avoids creating a separate process-specific detection layer. Blocking non-UTF-8 bytes is a conservative fail-closed choice because opaque bytes cannot be scanned reliably before reaching a local process.
Type of change
Test plan
python3.11 -m py_compile tools/process_registry.pyruff check tools/process_registry.py tests/tools/test_process_registry.pygit diff --checkpytest tests/tools/test_process_registry.py::TestStdinApprovalGuard -qpytest tests/tools/test_process_registry.py tests/tools/test_approval.py -qstatus: blocked, and/tmp/hermes-process-stdin-approval-bypasswas not created.Executed with:
Focused results:
TestStdinApprovalGuard:3 passed182 passedDisclosure notes
process.write/process.submit.