Skip to content

fix: shell injection in tui_gateway/server.py (ISSUE-001) - #33503

Open
ErnestHysa wants to merge 2 commits into
NousResearch:mainfrom
ErnestHysa:fix/issue-001-shell-injection
Open

fix: shell injection in tui_gateway/server.py (ISSUE-001)#33503
ErnestHysa wants to merge 2 commits into
NousResearch:mainfrom
ErnestHysa:fix/issue-001-shell-injection

Conversation

@ErnestHysa

Copy link
Copy Markdown
Contributor

Summary

Fix for ISSUE-001: Shell injection vulnerability in TUI Gateway.

Changes

  1. Added detect_dangerous_command import at the top of tui_gateway/server.py (line 18)
  2. Added dangerous command check for quick_commands exec path (around line 4959)
  3. Added dangerous command check for /exec slash command path (around line 6981)

Both paths now return error 4005 when a dangerous command is detected.

Verification

grep -n detect_dangerous_command tui_gateway/server.py

Shows: line 18 (import), line 4959 (quick_commands), line 6981 (exec)

grep -n shell=True tui_gateway/server.py

Shows: lines 4968 and 6988 (subprocess.run calls - now protected)

- Add detect_dangerous_command check for quick_commands exec path (line ~4959)
- Add detect_dangerous_command check for /exec slash command path (line ~6981)
- Move import to top-level file (line 18) so ImportError is not silently skipped
- Return error 4005 when dangerous command detected in both paths
@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

Superior version of your own #33495 (same fix: shell injection in TUI quick_commands and /exec). Also related to long chain: #20333, #10698, #5381, #15542, #17045. Note: this PR adds tirith_security.py which appears to be a custom security module not part of the upstream repo.

@ErnestHysa

Copy link
Copy Markdown
Contributor Author

Confirmed — tirith_security.py is not a custom module; it already exists in upstream (tools/tirith_security.py). This PR applies a legitimate security improvement: least-privilege chmod (owner-only execute 0o100 instead of world-executable 0o777) and inlines the _extract_tirith_binary() helper. No conflict with upstream.

@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 identifying the missing quick-command safety gate. Current main still executes configured quick commands with shell=True at tui_gateway/server.py:11856-11873, so the core direction remains useful.

Problems

  • The /exec hunk is superseded: current shell.exec already applies hardline and dangerous-command checks and fails closed on a missing approval module at tui_gateway/server.py:14401-14414 (commit 621bf3a873b6b466b7fca6fbd6f4c7cf83a70fdd).
  • The quick-command hunk should mirror that current guard by checking detect_hardline_command before detect_dangerous_command; the PR adds only the latter.
  • tools/tirith_security.py:426 preserves pre-existing group/world execute bits because it ORs the old mode, so it does not guarantee owner-only execution.
  • Please add TUI command.dispatch regression tests; tests/test_tui_gateway_server.py:4588-4607 currently covers only subprocess failure output.

Suggested changes

  • Salvage the quick-command guard onto tui_gateway/server.py:11856-11873, including both current shell.exec checks and 4005 responses.
  • Clear group/world execute bits explicitly if the permission hardening remains.

Automated hermes-sweeper review.

Comment thread tui_gateway/server.py
@@ -4954,8 +4955,16 @@ def _(rid, params: dict) -> dict:
if name in qcmds:
qc = qcmds[name]
if qc.get("type") == "exec":

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.

Please also check detect_hardline_command before this dangerous-command check. Current shell.exec applies the hardline block first (tui_gateway/server.py:14403-14412); the quick-command path should preserve the same unconditional safety boundary.

Comment thread tools/tirith_security.py
pass
return None, "cross_device_copy_failed"
os.chmod(dest, os.stat(dest).st_mode | stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH)
os.chmod(dest, os.stat(dest).st_mode | stat.S_IXUSR)

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.

OR-ing S_IXUSR retains any existing S_IXGRP and S_IXOTH bits from the extracted or copied binary. Clear those bits explicitly if this change is intended to guarantee owner-only execution.

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