Skip to content

feat(security): screen rewritten commands and scrub child credentials in the bash tool - #229

Open
raymondginger2018-sudo wants to merge 1 commit into
HKUDS:mainfrom
raymondginger2018-sudo:fix/security-bash-tool
Open

feat(security): screen rewritten commands and scrub child credentials in the bash tool#229
raymondginger2018-sudo wants to merge 1 commit into
HKUDS:mainfrom
raymondginger2018-sudo:fix/security-bash-tool

Conversation

@raymondginger2018-sudo

Copy link
Copy Markdown
Contributor

Summary

The native bash tool (core/harness/tools/shell.py) executed whatever command text it was handed, and passed that child the full process environment. Both are reachable by an intermediary between the client and the model provider: a relay, gateway, or any OpenAI-compatible proxy terminates TLS by design, so it can rewrite a tool call on its way back - or simply read the request body.

Two fail-closed, explicitly waivable changes:

  1. screen_egress() / screen_install() / screen_all() join the existing screen_command() in core/harness/command_guard.py. screen_egress flags a fetch piped into an interpreter and any host outside an opt-in allow-list. screen_install flags an install from a non-canonical index and a package name one edit away from a declared dependency - Damerau-Levenshtein, so requests -> reqeusts counts as one edit.
  2. core/harness/env_sanitize.py lifts the credential-shaped environment scrub out of core/harness/agents/external_backend.py (which already used it for spawned agent CLIs) so every child that can echo its environment uses it too.

Both are applied in BashTool; the scrub also covers hook commands and the code-mode runtime.

Changes

file what
core/harness/env_sanitize.py new - one credential-name pattern and one scrub helper, shared
core/harness/command_guard.py + screen_egress, screen_install, screen_all, find_confusables
core/harness/tools/shell.py runs screen_all before spawning; passes the scrubbed env
core/harness/agents/external_backend.py imports the shared helpers instead of owning them
core/harness/hooks/execution.py, core/harness/code_mode/tool.py pass the scrubbed env

Security Considerations

Attack surface changed. Two paths that previously ran unscreened now fail closed: (a) a shell command whose text may not have been written by the model, and (b) the environment handed to any child process the agent spawns.

Why this is fail-safe.

  • No new default blocks a previously-working command. With no allow-list configured, screen_egress only rejects a blocked host and the pipe-to-interpreter shape; screen_install only rejects a non-canonical index or a name one edit from a declared dependency.
  • Every screen is individually waivable by a named env var: DEEPCODE_ALLOW_REMOTE_SCRIPT=1, DEEPCODE_COMMAND_SCREEN=0, DEEPCODE_BASH_FULL_ENV=1.
  • screen_command is unchanged; its existing behaviour and tests are untouched.
  • A screen that cannot parse a command passes it through - it never blocks what it cannot understand.
  • The environment scrub keeps PATH, HOME, locale and proxy variables, so children run normally; a caller that needs a specific variable forwards it explicitly, and the merge happens after the scrub.

What this is not. Neither screen is the execution boundary - the workspace sandbox is. screen_egress in particular is a coarse filter: an attacker who can host the payload on an allow-listed domain, or drop a stager locally and run it through an innocuous command, walks straight through. The docstrings say this explicitly so the code is not mistaken for a guarantee.

Security verification.

  • tests/test_command_guard_egress.py (50 cases) - canonical pipe-to-interpreter payloads, benign fetches that must NOT fire, allow-list opt-in semantics, blocked-host enforcement, the transposition typosquat, non-canonical indexes, and every waiver path.
  • tests/test_env_sanitize.py (35 cases) - the credential names this repo's own .env defines are all matched and dropped; PATH/HOME survive; explicit forwarding wins; both waiver paths.
  • tests/test_shell_search_tools.py - refusal happens before the process is spawned, and the pre-existing destructive-command path still works.

Tests

pytest tests/test_command_guard_egress.py tests/test_env_sanitize.py tests/test_shell_search_tools.py -q
137 passed

Verified under the repository's own pyproject.toml pytest config; ruff check and ruff format clean against the pinned ruff.

Notes

… in the bash tool

The native bash tool executed whatever command text it was handed and passed it
the full process environment. Both are reachable by an intermediary between the
client and the model provider: a relay, gateway, or any OpenAI-compatible proxy
terminates TLS by design and can rewrite a tool call on its way back, or just
read the request body.

Two changes, both fail-closed and both waivable by name:

* core.harness.command_guard gains screen_egress() and screen_install() beside
  the existing screen_command(), plus screen_all() to run them in order.
  screen_egress flags a fetch piped into an interpreter and any host outside an
  opt-in allow-list. screen_install flags an install from a non-canonical index
  and a package name one edit from a declared dependency (Damerau-Levenshtein,
  so the classic transposition counts as one edit).
* core.harness.env_sanitize lifts the credential-shaped environment scrub out of
  core.harness.agents.external_backend, which already used it for spawned agent
  CLIs, so every child that can echo its environment uses it too.

Both are applied in BashTool; the scrub also covers hook commands and the
code-mode runtime. DEEPCODE_ALLOW_REMOTE_SCRIPT=1, DEEPCODE_BASH_FULL_ENV=1 and
DEEPCODE_COMMAND_SCREEN=0 waive the respective check.

Neither screen is the security boundary - the workspace sandbox is. They are
cheap first passes that fail closed on shapes recognisable by inspection, and
the docstrings say so.

Tests: tests/test_command_guard_egress.py (50), tests/test_env_sanitize.py (35),
plus screen cases in tests/test_shell_search_tools.py.

Refs HKUDS#128, and HKUDS#228 which applies the same screen to the other call site.
@raymondginger2018-sudo

Copy link
Copy Markdown
Contributor Author

@Zongwei9888 requesting security review per CONTRIBUTING.md.

Two housekeeping notes from the fork side, so they are not mistaken for a missing checklist:

  • The security label could not be applied: it does not exist in this repository (GET /labels returns bug / dependencies / documentation / duplicate / enhancement / good first issue / help wanted / invalid / python / question / wontfix), and creating it returns 403 for a fork contributor. Please apply it if you want it tracked.
  • POST /pulls/229/requested_reviewers returns 404 from this account, so this comment is the review request instead.

Scope note. #228 applies the same screen_command screen to tools/command_executor.py; this PR covers the native bash tool (core/harness/tools/shell.py), which is a separate call site with no screening at all. They are siblings, not duplicates - either order merges cleanly.

Size note. 1101 insertions across 9 files is above the usual split threshold. It is one file family (command_guard / shell / env_sanitize) serving one intent, and the two features share hunks inside shell.py. Say the word and I will split it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant