Skip to content

Fix shell injection in TUI quick_commands and /exec slash command - #33495

Open
ErnestHysa wants to merge 1 commit into
NousResearch:mainfrom
ErnestHysa:fix/tui-shell-injection
Open

Fix shell injection in TUI quick_commands and /exec slash command#33495
ErnestHysa wants to merge 1 commit into
NousResearch:mainfrom
ErnestHysa:fix/tui-shell-injection

Conversation

@ErnestHysa

Copy link
Copy Markdown
Contributor

Bug (Before)

Two RPC handlers in tui_gateway/server.py passed user-supplied command strings directly to subprocess.run() with shell=True:

  1. quick_commands exec (line ~4957): The command.dispatch method resolved a quick-command name and executed its configured command via subprocess.run(cmd, shell=True) — allowing shell injection through malicious command arguments.

  2. /exec slash command (line ~6984): The shell.exec method executed subprocess.run(cmd, shell=True) after only a detect_dangerous_command check — which only blocks a known blocklist, not arbitrary shell metacharacters.

Fix (After)

Both paths now:

  • Parse the command string with shlex.split() into a list of arguments
  • Pass shell=False to subprocess.run() — preventing shell interpretation of &, |, ;, $, backticks, etc.
  • The quick_commands path also gains the detect_dangerous_command check that was missing

Impact

An attacker who could send RPC requests to the TUI gateway could execute arbitrary shell commands on the host system with the privileges of the hermes-agent process. This is now prevented.

@alt-glitch alt-glitch added type/security Security vulnerability or hardening P2 Medium — degraded but workaround exists comp/tui Terminal UI (ui-tui/ + tui_gateway/) labels May 27, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Related to open #28214 (centralized guarded command helper for TUI shell exec) and addresses #16560 (command injection via shell=True in tui_gateway). Multiple closed PRs have attempted this fix: #20333, #10698, #5381, #15542, #17045. Compare scope with #28214 which is still open.

@ErnestHysa

Copy link
Copy Markdown
Contributor Author

@alt-glitch — noted, thanks. I've reviewed the scope of #28214 (centralized guarded command helper) and this PR. The PRs complement each other: #28214 is the long-term centralized-helper refactor; #33495 is the targeted shell injection fix in quick_commands and /exec. The fix in 556e965 uses shlex.split() and subprocess.run(..., shell=False) for the rendered template, matching the same hardening approach in #33503. No conflict with #28214 — if #28214 lands, the detect_dangerous_command guard from this PR can be migrated to the centralized helper.

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for targeting a real historical shell.exec concern.

Problems

  • The /exec portion is already superseded: current tui_gateway/server.py:14401-14418 checks hardline and dangerous commands, fails closed when approval support is unavailable, and disconnects stdin. This landed in 621bf3a873b6b466b7fca6fbd6f4c7cf83a70fdd.
  • The quick-command injection premise does not hold on current main. command.dispatch receives arg at tui_gateway/server.py:11850, but executes only qc.get("command", "") at line 11866; the request argument is not interpolated into that command.
  • shlex.split(...), shell=False would break documented operator shell snippets. website/docs/user-guide/configuration.md:1687 configures cd ... && git pull && pip install -e ., which requires shell interpretation. Current quick commands also preserve sanitized environment, stdin=subprocess.DEVNULL, and output redaction at tui_gateway/server.py:11860-11881.

Suggested changes

  • Do not salvage the quick-command conversion as an injection fix; retain the operator-configured shell-snippet contract and current protections.
  • Drop the superseded /exec change.

This is an automated hermes-sweeper review.

Comment thread tui_gateway/server.py
return _err(
rid, 4005, f"blocked: {desc}. Use the agent for dangerous commands."
)
except ImportError:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the documented quick-command contract from operator-authored shell snippets to a single argv invocation. For example, the current configuration guide uses cd ... && git pull && pip install -e .; shlex.split() would pass && literally and break it. The RPC arg is not interpolated into qc["command"], so it is not the claimed injection channel.

Comment thread tui_gateway/server.py
try:
r = subprocess.run(
cmd, shell=True, capture_output=True, text=True, timeout=30, cwd=os.getcwd()
shlex.split(cmd), shell=False, capture_output=True, text=True, timeout=30, cwd=os.getcwd()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This portion is stale against current main: shell.exec now applies both detect_hardline_command and detect_dangerous_command, fails closed if approval support is unavailable, and uses stdin=subprocess.DEVNULL before its shell execution path.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/tui Terminal UI (ui-tui/ + tui_gateway/) P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants