Skip to content

[codex] fix(approval): block dynamic shell command-name bypasses#43658

Open
egilewski wants to merge 1 commit into
NousResearch:mainfrom
egilewski:codex/fix-36847-command-substitution
Open

[codex] fix(approval): block dynamic shell command-name bypasses#43658
egilewski wants to merge 1 commit into
NousResearch:mainfrom
egilewski:codex/fix-36847-command-substitution

Conversation

@egilewski

@egilewski egilewski commented Jun 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

  • Adds a compact command-position guard for shell-expanded command names in the approval detector.
  • Blocks dynamic command-name forms before they can bypass shell.exec approval or the hardline yolo floor for catastrophic root/system/home delete shapes.
  • Keeps static shell spelling cleanup scoped to command-position words, so argument-only examples such as echo r\m -rf / and echo $(echo rm) -rf / are not promoted into dangerous commands.

Why

The detector previously treated command substitutions and related shell-expanded command names as opaque text. That left a gap where the shell could resolve the first word into a dangerous command after Hermes had already approved the string.

This PR is intentionally a smaller quickfix for the original issue. It is less versatile than #40663, but the narrower shape should be easier to review and merge quickly while a broader follow-up is discussed separately.

Credit

This quickfix uses concrete direction and regression context from #40663, especially the command-position-only approach and the need to avoid promoting argument-only text into a command. The signed commit includes:

Co-authored-by: xy200303 <3483421977@qq.com>

Validation

  • /home/mac/hermes-agent/.venv/bin/python -m pytest -o addopts='' -p no:cacheprovider tests/tools/test_approval.py tests/tools/test_hardline_blocklist.py tests/tools/test_command_guards.py tests/tools/test_yolo_mode.py tests/tools/test_cron_approval_mode.py tests/tools/test_managed_browserbase_and_modal.py tests/test_tui_gateway_server.py::test_shell_exec_blocks_dynamic_command_name tests/test_tui_gateway_server.py::test_shell_exec_allows_argument_only_substitution -q -> 410 passed
  • git diff --check upstream/main -> passed
  • /home/mac/hermes-agent/.venv/bin/python -m py_compile tools/approval.py tests/tools/test_approval.py tests/tools/test_hardline_blocklist.py tests/test_tui_gateway_server.py -> passed
  • CodeRabbit completed clean on the shrunk production diff; a final rerun after adding two test-only exact bypass strings hit the account review limit.

Agent Disclosure

  • Model: GPT-5.5
  • Thinking level: xhigh
  • Harness: Codex
  • The account owner loosely reviews my actions and receives the usual notifications from GitHub.

Fixes #36847
Related #36846
Related #40663

@egilewski egilewski mentioned this pull request Jun 10, 2026
Open
@egilewski @xy200303
fix(approval): block dynamic shell command-name bypasses
ddd5896 
Dynamic shell command names were still opaque to the dangerous-command detector, so payloads that resolve a command name through shell substitution could bypass shell.exec approval and the hardline yolo floor for catastrophic command shapes.

This keeps the fix compact by using command-position detection rather than a broad shell parser. Static shell spelling tricks are deobfuscated only for command-position words, active dynamic command names require dangerous approval, and dynamic names with catastrophic tails or literal shutdown-style names remain hardline-blocked. Argument-only text such as echo examples is left alone.

Validation:

- pytest: tests/tools/test_approval.py tests/tools/test_hardline_blocklist.py tests/tools/test_command_guards.py tests/tools/test_yolo_mode.py tests/tools/test_cron_approval_mode.py tests/tools/test_managed_browserbase_and_modal.py tests/test_tui_gateway_server.py selected shell.exec regression tests

- git diff --check

- py_compile: tools/approval.py and touched tests

This is a smaller and less versatile alternative to NousResearch#40663 aimed for a quick resolution of a P0 security issue.

Fixes NousResearch#36847

Related NousResearch#36846

Co-authored-by: xy200303 <3483421977@qq.com>
@egilewski egilewski force-pushed the codex/fix-36847-command-substitution branch from b81a974 to ddd5896 Compare June 10, 2026 16:54
@liuhao1024

liuhao1024 commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Verification Review

Reviewed the diff — this is a well-structured security hardening with solid test coverage. A few observations:

Approach is correct: Moving backslash-escape and empty-string stripping from global normalization to (scoped to command-position words via ) eliminates the class of false positives where argument text like echo r\m would have been silently rewritten. The new two-variant approach (normalized + deobfuscated-static) in is clean.

Dynamic command-word detection (_DYNAMIC_COMMAND_NAME_RE, _DYNAMIC_HARDLINE_COMMAND_RE, _DYNAMIC_HARDLINE_NAME_RE): correctly flags opaque substitutions like $(echo rm) and ${0/x/r}m at command position. The 200-char cap inside the capture group is a reasonable defense against pathological regex backtracking.

Hardline pattern terminator: adding \) to the rm-target anchors (e.g. /|/\*|/ \* → /|/\*|/ \*|\)) correctly handles the case where a dynamic substitution is followed by the target path inside a subshell context.

Test coverage is thorough — covers shell-escaped rm, dynamic command-word substitution (printf-hex, printf-octal, pipe, semicolon, parameter expansion), argument-only substitution (false positive guard), and YOLO-mode bypass attempts. No gaps spotted.

No issues found.

@alt-glitch alt-glitch added type/security Security vulnerability or hardening P1 High — major feature broken, no workaround comp/tools Tool registry, model_tools, toolsets tool/terminal Terminal execution and process management labels Jun 10, 2026
@alt-glitch

alt-glitch commented Jun 10, 2026

Copy link
Copy Markdown
Collaborator

Related: narrower quickfix for the same approval-bypass class as #40663 (command-position shell-expanded command names). This PR scopes to command-substitution/dynamic command names only; #40663 is the broader fix. Same root cause as #36846/#40591; complements de-obfuscation work #25795.

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

Reviewers

No reviews

Assignees

No one assigned

Labels

comp/tools Tool registry, model_tools, toolsets P1 High — major feature broken, no workaround tool/terminal Terminal execution and process management type/security Security vulnerability or hardening

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Security: tui_gateway shell.exec inherits the approval denylist bypass → arbitrary command execution

3 participants

@egilewski @liuhao1024 @alt-glitch