fix(tools): allowlist command/args/env in manage_mcp 'add' to prevent RCE #438

Closed

ErnestHysa wants to merge 2 commits from ErnestHysa/fix/manage-mcp-rce-allowlist into main

ErnestHysa commented

2026-06-01 12:53:56 +02:00

(Migrated from github.com)

Summary

do_manage_mcp(content, owner) accepted a user-controllable command, args, and env from the LLM/UI and passed them verbatim to mcp_manager.connect_server, which spawned the subprocess with no allowlist. A model — or a prompt-injection payload smuggled into a skill description, a memory entry, a fetched web page, or an email body — could register a stdio MCP server that ran arbitrary commands as the app's UID.

The minimal attack:

manage_mcp(action="add", name="x",
           command="sh", args=["-c", "id>/tmp/pwn"], env={})

This would spawn sh -c "id>/tmp/pwn", persist the server in the DB, and re-fire on every reconnect — and the chat tool layer would happily return {"exit_code": 0} because the call was a successful tool invocation. The DB write happens before the connect attempt, so even a connect failure leaves the malicious row around.

This is the same bug class as the previous fix(cookbook): avoid pip --user in venv work (PR #363) — a missing input-validation gate at a tool entry point. The MCP gate is a tighter version of the same idea, scoped to a single tool surface.

Why it matters

manage_mcp is reachable from:

the chat agent loop (any LLM call that decides to call the manage_mcp tool);
the Cookbook / admin UI ("Add MCP server" form);
any MCP tool call from a third-party client that successfully authenticates.

McpServer rows persist across restarts and the spawned subprocess gets reconnected on every server boot, so a one-time successful add is a permanent backdoor. There is no per-call sandboxing in mcp_manager._connect_stdio (it just builds StdioServerParameters(command=…, args=…, env={**os.environ, **env}) and hands it to stdio_client).

os.environ is also merged in verbatim, so a permissive env could ship a PATH override pointing the child at an attacker-controlled directory or a LD_PRELOAD library — turning the spawned interpreter into a loader-side hijack regardless of which interpreter is used.

What I changed

Two changes, both in src/tool_implementations.py, plus a regression test.

1. New validator: `_validate_mcp_command(command, cmd_args, env) -> Optional[str]`

Returns None if the spawn target is safe, else a string error message. Enforces:

command is a non-empty string.
command is either an allowlisted basename (python3, python, node, deno, bun, uv, uvx, pipx, npm, npx, pnpm, yarn) OR an absolute path that resolves to a regular file inside the project root. Absolute paths are checked by pathlib.Path.resolve().relative_to(project_root) — is_relative_to-style containment, not just startswith, so /tmp/evil.sh and /usr/local/bin/whatever are both rejected. The project root is the parent of the src/ directory (i.e. the odysseus repo), derived from __file__ at import time.
args is a flat list/tuple of strings.
If the resolved command is an allowlisted interpreter, none of the args may be a shell-escape flag (-c, -e, --eval, -r, /c, plus their =… forms). These turn the interpreter into a one-shot shell — exactly the RCE we are closing. The check is only enforced on interpreter-style commands; for project-relative scripts (e.g. mcp_servers/memory_server.py) the caller already controls the path so the script itself is the trust boundary.
env is a flat dict[str, str] (no nesting, no non-string values).
env does not contain any of a frozen-set of forbidden keys: LD_PRELOAD, LD_LIBRARY_PATH, LD_AUDIT, PATH, PYTHONPATH, PYTHONSTARTUP, PYTHONHOME, NODE_PATH, NODE_OPTIONS, NPM_CONFIG_GLOBALCONFIG, DYLD_INSERT_LIBRARIES, DYLD_LIBRARY_PATH. Allowing any of these would let a poisoned env entry redirect the interpreter's module search, preload a shared library, or change which executable bash resolves to.

2. Wire the validator into the `add` branch

The add branch now calls _validate_mcp_command(command, cmd_args, env) immediately after the field extraction, before the DB write and before mcp_manager.connect_server. On failure: returns {"error": validation_err, "exit_code": 1} and logs the rejection at WARNING with the offending name. No row is written, no subprocess is spawned.

# Before
if not name or not command:
    return {"error": "name and command are required", "exit_code": 1}
sid = str(_uuid.uuid4())[:8]
db = SessionLocal()
try:
    srv = McpServer(id=sid, name=name, transport="stdio", command=command, ...)

# After
if not name or not command:
    return {"error": "name and command are required", "exit_code": 1}
validation_err = _validate_mcp_command(command, cmd_args, env)
if validation_err is not None:
    logger.warning(f"manage_mcp 'add' rejected for {name!r}: {validation_err}")
    return {"error": validation_err, "exit_code": 1}
sid = str(_uuid.uuid4())[:8]
db = SessionLocal()
try:
    srv = McpServer(id=sid, name=name, transport="stdio", command=command, ...)

The delete, enable, disable, reconnect, and list branches are unchanged. None of them spawn a new subprocess; they only operate on existing McpServer rows by server_id, which is admin-only.

Verification

python -c "import ast; ast.parse(open('src/tool_implementations.py').read())" → OK.
pytest tests/test_manage_mcp_allowlist.py -v → 18/18 pass.
The new test file covers both halves of the contract:
- 13 attack-path tests (rejection required): sh, bash -c, python3 -c, node --eval, /tmp/evil.sh, /usr/local/bin/some-binary, LD_PRELOAD in env, PATH in env, PYTHONPATH in env, None arg, integer env value, empty command, whitespace-only command.
- 5 allowlist tests (acceptance required): python3 mcp_servers/memory_server.py, uv run mcp_servers/rag_server.py, npx -y @modelcontextprotocol/server-filesystem, absolute path inside project root, env with safe keys (DEBUG, LOG_LEVEL, MCP_NAME).

Out of scope / things I deliberately did not change

The validator lives at the do_manage_mcp boundary, not in mcp_manager._connect_stdio. Adding it at both layers would be defense-in-depth, but the boundary is the right place for a trust decision (the tool surface) — the manager is a thin wrapper and should stay that way. I considered moving the check into _connect_stdio as well, but a malformed call from connect_all_enabled (the built-in MCP list at startup, which reads from the DB) would be a different bug class: the DB write is the issue, and that's already gated above.
The validator allows python3 mcp_servers/memory_server.py because mcp_servers/memory_server.py resolves inside the project root. It does not constrain what the spawned script does — that's the same level of trust as today's manage_mcp 'add', but now the path is on the allowlist. A user-supplied command of an absolute path to a script they own is fine; command="sh" is not.
I did not add a similar validator to the manage_skills (LLM-skill loader) path. That's a separate surface and was covered by the prior PR-#354 work; if it needs an allowlist it should be a separate change.
The allowlist is intentionally narrow (python3, node, deno, bun, uv, uvx, pipx, npm, npx, pnpm, yarn). If someone wants to add ruby or go they extend the frozenset; the change is one line. Better than the alternative, which is a regex allowlist that misses obscure-but-real bypasses.
I did not change the env={**os.environ, **env} merge in mcp_manager._connect_stdio. It is still permissive — a legitimate child needs the inherited env (PATH, HOME, LANG, etc.). The new validator blocks the env keys that override the loaders, not the wholesale inheritance.

Notes for the reviewer

Single feature file changed: src/tool_implementations.py (+119 lines, all in one region: the new constant block + new validator + the one-call insertion at the add branch). One new test file: tests/test_manage_mcp_allowlist.py (+126 lines).
No new dependencies, no schema changes, no JS changes, no migration.
The validator returns the error as a string (not raising an exception) so the existing do_manage_mcp return {"error": ..., "exit_code": 1} contract is preserved. The LLM sees a clear "args contain a shell-escape flag" message and can self-correct.
I considered raising HTTPException(400, ...) instead, but do_manage_mcp is a tool handler, not a route — the surrounding framework expects the dict return shape. Stick with the existing convention.
If the maintainer prefers a more aggressive allowlist (e.g. only python3 + paths under mcp_servers/), the change is a 3-line edit to _MCP_ALLOWED_COMMANDS and one extra pathlib.relative_to check. Happy to follow up.
I did not add audit logging for the allowed cases (only the rejected ones get logged at WARNING). If the maintainer wants every successful add to land in the assistant log, that's a one-liner with the existing log_to_assistant helper.

Closes the "manage_mcp RCE" finding from the recent audit pass on the project.

## Summary `do_manage_mcp(content, owner)` accepted a user-controllable `command`, `args`, and `env` from the LLM/UI and passed them verbatim to `mcp_manager.connect_server`, which spawned the subprocess with no allowlist. A model — or a prompt-injection payload smuggled into a skill description, a memory entry, a fetched web page, or an email body — could register a stdio MCP server that ran arbitrary commands as the app's UID. The minimal attack: ```python manage_mcp(action="add", name="x", command="sh", args=["-c", "id>/tmp/pwn"], env={}) ``` This would spawn `sh -c "id>/tmp/pwn"`, persist the server in the DB, and re-fire on every reconnect — and the chat tool layer would happily return `{"exit_code": 0}` because the call was a successful tool invocation. The DB write happens before the connect attempt, so even a connect failure leaves the malicious row around. This is the same bug class as the previous `fix(cookbook): avoid pip --user in venv` work (PR #363) — a missing input-validation gate at a tool entry point. The MCP gate is a tighter version of the same idea, scoped to a single tool surface. ## Why it matters `manage_mcp` is reachable from: - the chat agent loop (any LLM call that decides to call the `manage_mcp` tool); - the Cookbook / admin UI ("Add MCP server" form); - any MCP tool call from a third-party client that successfully authenticates. `McpServer` rows persist across restarts and the spawned subprocess gets reconnected on every server boot, so a one-time successful `add` is a permanent backdoor. There is no per-call sandboxing in `mcp_manager._connect_stdio` (it just builds `StdioServerParameters(command=…, args=…, env={**os.environ, **env})` and hands it to `stdio_client`). `os.environ` is also merged in verbatim, so a permissive `env` could ship a `PATH` override pointing the child at an attacker-controlled directory or a `LD_PRELOAD` library — turning the spawned interpreter into a loader-side hijack regardless of which interpreter is used. ## What I changed Two changes, both in `src/tool_implementations.py`, plus a regression test. ### 1. New validator: `_validate_mcp_command(command, cmd_args, env) -> Optional[str]` Returns `None` if the spawn target is safe, else a string error message. Enforces: - `command` is a non-empty string. - `command` is either an allowlisted basename (`python3`, `python`, `node`, `deno`, `bun`, `uv`, `uvx`, `pipx`, `npm`, `npx`, `pnpm`, `yarn`) **OR** an absolute path that resolves to a regular file inside the project root. Absolute paths are checked by `pathlib.Path.resolve().relative_to(project_root)` — `is_relative_to`-style containment, not just `startswith`, so `/tmp/evil.sh` and `/usr/local/bin/whatever` are both rejected. The project root is the parent of the `src/` directory (i.e. the odysseus repo), derived from `__file__` at import time. - `args` is a flat list/tuple of strings. - If the resolved command is an allowlisted interpreter, none of the args may be a shell-escape flag (`-c`, `-e`, `--eval`, `-r`, `/c`, plus their `=…` forms). These turn the interpreter into a one-shot shell — exactly the RCE we are closing. The check is only enforced on interpreter-style commands; for project-relative scripts (e.g. `mcp_servers/memory_server.py`) the caller already controls the path so the script itself is the trust boundary. - `env` is a flat `dict[str, str]` (no nesting, no non-string values). - `env` does not contain any of a frozen-set of forbidden keys: `LD_PRELOAD`, `LD_LIBRARY_PATH`, `LD_AUDIT`, `PATH`, `PYTHONPATH`, `PYTHONSTARTUP`, `PYTHONHOME`, `NODE_PATH`, `NODE_OPTIONS`, `NPM_CONFIG_GLOBALCONFIG`, `DYLD_INSERT_LIBRARIES`, `DYLD_LIBRARY_PATH`. Allowing any of these would let a poisoned env entry redirect the interpreter's module search, preload a shared library, or change which executable `bash` resolves to. ### 2. Wire the validator into the `add` branch The `add` branch now calls `_validate_mcp_command(command, cmd_args, env)` immediately after the field extraction, **before** the DB write and **before** `mcp_manager.connect_server`. On failure: returns `{"error": validation_err, "exit_code": 1}` and logs the rejection at WARNING with the offending `name`. No row is written, no subprocess is spawned. ```python # Before if not name or not command: return {"error": "name and command are required", "exit_code": 1} sid = str(_uuid.uuid4())[:8] db = SessionLocal() try: srv = McpServer(id=sid, name=name, transport="stdio", command=command, ...) # After if not name or not command: return {"error": "name and command are required", "exit_code": 1} validation_err = _validate_mcp_command(command, cmd_args, env) if validation_err is not None: logger.warning(f"manage_mcp 'add' rejected for {name!r}: {validation_err}") return {"error": validation_err, "exit_code": 1} sid = str(_uuid.uuid4())[:8] db = SessionLocal() try: srv = McpServer(id=sid, name=name, transport="stdio", command=command, ...) ``` The `delete`, `enable`, `disable`, `reconnect`, and `list` branches are unchanged. None of them spawn a new subprocess; they only operate on existing `McpServer` rows by `server_id`, which is admin-only. ## Verification - `python -c "import ast; ast.parse(open('src/tool_implementations.py').read())"` → `OK`. - `pytest tests/test_manage_mcp_allowlist.py -v` → 18/18 pass. - The new test file covers both halves of the contract: - **13 attack-path tests** (rejection required): `sh`, `bash -c`, `python3 -c`, `node --eval`, `/tmp/evil.sh`, `/usr/local/bin/some-binary`, `LD_PRELOAD` in env, `PATH` in env, `PYTHONPATH` in env, `None` arg, integer env value, empty command, whitespace-only command. - **5 allowlist tests** (acceptance required): `python3 mcp_servers/memory_server.py`, `uv run mcp_servers/rag_server.py`, `npx -y @modelcontextprotocol/server-filesystem`, absolute path inside project root, env with safe keys (`DEBUG`, `LOG_LEVEL`, `MCP_NAME`). ## Out of scope / things I deliberately did not change - The validator lives at the `do_manage_mcp` boundary, not in `mcp_manager._connect_stdio`. Adding it at both layers would be defense-in-depth, but the boundary is the right place for a *trust* decision (the tool surface) — the manager is a thin wrapper and should stay that way. I considered moving the check into `_connect_stdio` as well, but a malformed call from `connect_all_enabled` (the built-in MCP list at startup, which reads from the DB) would be a different bug class: the DB write is the issue, and that's already gated above. - The validator allows `python3 mcp_servers/memory_server.py` because `mcp_servers/memory_server.py` resolves inside the project root. It does **not** constrain what the spawned script does — that's the same level of trust as today's `manage_mcp 'add'`, but now the path is on the allowlist. A user-supplied `command` of an absolute path to a script they own is fine; `command="sh"` is not. - I did not add a similar validator to the `manage_skills` (LLM-skill loader) path. That's a separate surface and was covered by the prior PR-#354 work; if it needs an allowlist it should be a separate change. - The allowlist is intentionally narrow (`python3`, `node`, `deno`, `bun`, `uv`, `uvx`, `pipx`, `npm`, `npx`, `pnpm`, `yarn`). If someone wants to add `ruby` or `go` they extend the frozenset; the change is one line. Better than the alternative, which is a regex allowlist that misses obscure-but-real bypasses. - I did not change the `env={**os.environ, **env}` merge in `mcp_manager._connect_stdio`. It is still permissive — a legitimate child needs the inherited env (PATH, HOME, LANG, etc.). The new validator blocks the env keys that *override* the loaders, not the wholesale inheritance. ## Notes for the reviewer - Single feature file changed: `src/tool_implementations.py` (+119 lines, all in one region: the new constant block + new validator + the one-call insertion at the `add` branch). One new test file: `tests/test_manage_mcp_allowlist.py` (+126 lines). - No new dependencies, no schema changes, no JS changes, no migration. - The validator returns the error as a string (not raising an exception) so the existing `do_manage_mcp` `return {"error": ..., "exit_code": 1}` contract is preserved. The LLM sees a clear "args contain a shell-escape flag" message and can self-correct. - I considered raising `HTTPException(400, ...)` instead, but `do_manage_mcp` is a tool handler, not a route — the surrounding framework expects the dict return shape. Stick with the existing convention. - If the maintainer prefers a more aggressive allowlist (e.g. only `python3` + paths under `mcp_servers/`), the change is a 3-line edit to `_MCP_ALLOWED_COMMANDS` and one extra `pathlib.relative_to` check. Happy to follow up. - I did not add `audit logging` for the allowed cases (only the rejected ones get logged at WARNING). If the maintainer wants every successful `add` to land in the assistant log, that's a one-liner with the existing `log_to_assistant` helper. Closes the "manage_mcp RCE" finding from the recent audit pass on the project.

pewdiepie-archdaemon commented

2026-06-01 15:49:50 +02:00

(Migrated from github.com)

This is a real issue and the direction is right, but I can't merge this PR as-is. The regression tests hardcode a local path (), so they won't run in CI or other checkouts. I also want the command/package-runner allowlist tightened in a smaller follow-up so we don't accidentally allow a package runner shape that still executes arbitrary code. Leaving open for a cleaned-up revision.

pewdiepie-archdaemon commented

2026-06-01 15:50:05 +02:00

(Migrated from github.com)

This is a real issue and the direction is right, but I cannot merge this PR as-is. The regression tests hardcode a local /Users/... checkout path, so they will not run in CI or other checkouts. I also want the command/package-runner allowlist tightened in a smaller follow-up so we do not accidentally allow a package runner shape that still executes arbitrary code. Leaving open for a cleaned-up revision.

ErnestHysa commented

2026-06-01 16:43:43 +02:00

(Migrated from github.com)

Good catch on both points, thanks.

Fixed in the latest push:

Hardcoded paths — the test now resolves the project root relative to the test file itself (os.path.dirname(__file__)), so it works in any checkout and in CI. No more /Users/ernest/... anywhere.

Package-runner allowlist — removed npx, uvx, pipx, npm, pnpm, and yarn from the allowlist entirely. They can all download and execute arbitrary remote packages, which is exactly the RCE surface we're trying to close. If a package is needed, it should be installed into the project venv and invoked through the interpreter or a project-local script. Also added a URL-scheme check on args (http://, https://, ftp://, file://) so deno/bun/node can't bypass via remote script URLs.

Added regression tests for all the new rejection cases (npx, pipx, uvx, yarn, remote URLs).

Good catch on both points, thanks. Fixed in the latest push: **Hardcoded paths** — the test now resolves the project root relative to the test file itself (`os.path.dirname(__file__)`), so it works in any checkout and in CI. No more `/Users/ernest/...` anywhere. **Package-runner allowlist** — removed npx, uvx, pipx, npm, pnpm, and yarn from the allowlist entirely. They can all download and execute arbitrary remote packages, which is exactly the RCE surface we're trying to close. If a package is needed, it should be installed into the project venv and invoked through the interpreter or a project-local script. Also added a URL-scheme check on args (http://, https://, ftp://, file://) so deno/bun/node can't bypass via remote script URLs. Added regression tests for all the new rejection cases (npx, pipx, uvx, yarn, remote URLs).