fix(security): prevent arbitrary command execution in McpToolset YAML config#923
fix(security): prevent arbitrary command execution in McpToolset YAML config#923Ashutosh0x wants to merge 1 commit into
Conversation
karolpiotrowicz
left a comment
There was a problem hiding this comment.
Thanks for the security-minded contribution, and for the thorough test suite — the 42 cases pass locally and the validation is correctly wired in before exec.Command. As written, though, this doesn't fully achieve "prevent arbitrary command execution," and it also risks breaking legitimate configs. Requesting changes:
1. Allowlisted launchers still allow arbitrary code execution. Each allowlist entry is a general-purpose interpreter or package runner that executes attacker-supplied code via its own args, e.g. node/python/python3 with -e/-c, deno eval, bun -e, npx/uvx/uv <arbitrary-package>, or docker run -v /:/host … chroot /host …. The launcher is narrowed but the RCE isn't closed.
2. The argument blocklist has no security effect here. exec.Command does not invoke a shell, so ;, &&, |, backticks, $(, ${, >, < are passed literally and never interpreted. It blocks some benign strings while not stopping (1). Either drop it or document it explicitly as defense-in-depth only.
3. Basename spoofing. Validation uses only filepath.Base(command), so a path whose base is an allowlisted token (e.g. ../../path/to/npx) is accepted regardless of the real binary there.
4. Breaking change for legitimate configs. A hardcoded 9-entry allowlist rejects common legitimate launchers — a custom server binary (the documented exec.Command("myserver") pattern), absolute paths, go run, pipx, pnpm/yarn dlx, java, etc.
Suggested direction (trust boundary, not string sanitization). The core issue is that a config file decides what process runs; no allowlist on the command string can be safe because interpreters are themselves code-execution primitives. A more robust approach: default-deny spawning local MCP subprocesses from config, and require the trusted caller that loads the config to explicitly opt in and supply the permitted commands (an operator-provided policy / named-server registry covering the full command + args), rather than a hardcoded launcher list. Also resolve the real binary path (not just the basename), and treat agent config files as a trust boundary (don't load them from untrusted sources unless exec is policy-restricted). Happy to help iterate.
(Note: there's currently no Go build/test CI job gating this package, so these tests aren't enforced by CI.)
…model Address reviewer feedback on PR google#923: 1. FIXED: Allowlisted launchers still allow RCE - Replaced hardcoded command allowlist with operator-provided MCPServerPolicy - Policy specifies exact (command, args-prefix) pairs, not just command names - e.g., npx only with @modelcontextprotocol/server-filesystem 2. FIXED: Argument blocklist has no security effect - Dropped shell metachar blocklist (exec.Command doesn't invoke a shell) - Args now controlled via policy ArgsPrefix constraint 3. FIXED: Basename spoofing - Commands resolved to absolute real paths via exec.LookPath + filepath.EvalSymlinks - Policy matches full resolved paths, not filepath.Base() 4. FIXED: Breaking change for legitimate configs - Default-deny with explicit operator opt-in via SetGlobalMCPPolicy() - Operators can allow any command they trust, no hardcoded restrictions Security model: YAML agent configs are treated as an untrusted input boundary. The operator (trusted caller) must explicitly permit specific MCP server commands before any subprocess can be spawned from config. 42 test cases covering: nil/empty policy, exact matching, args prefix, multiple entries, path resolution, spoofing prevention, and attack scenarios.
b3d8d13 to
e04a993
Compare
|
Thanks for the thorough and constructive review @karolpiotrowicz — every point was valid. I've reworked the approach entirely based on your feedback. Here's how each concern is addressed: Changes in this update1. Allowlisted launchers still allow RCE -- Replaced with operator-controlled policyThe hardcoded 9-entry command allowlist is gone. Instead, a new configurable.SetGlobalMCPPolicy(&configurable.MCPServerPolicy{
AllowedServers: []configurable.AllowedMCPServer{
{
Command: "/usr/local/bin/npx",
ArgsPrefix: []string{"@modelcontextprotocol/server-filesystem"},
},
},
})This closes the RCE gap you identified — 2. Argument blocklist has no security effect -- Dropped entirelyYou're right that 3. Basename spoofing -- Full path resolutionValidation now uses 4. Breaking change for legitimate configs -- Default-deny with explicit opt-inNo hardcoded restrictions anymore. If no policy is set, MCP subprocess spawning is blocked with a clear error message directing the operator to Architecture: Trust boundary, not string sanitizationFollowing your suggested direction exactly:
Test results42 test cases across 8 test functions, all passing:
Let me know if you'd like any further changes! |
Please ensure you have read the contribution guide before creating a pull request.
Link to Issue or Description of Change
2. Or, if no issue exists, describe the change:
Problem:
The
McpToolsetfactory ininternal/configurable/configurable_utils.gopasses user-controlledcommandandargsfields from YAML agent configurations directly toexec.Command()with no validation, enabling arbitrary OS command execution (RCE) when an attacker can influence agent config files.This is analogous to CVE-2026-4810 in
adk-python, but potentially more severe becauseexec.Command()provides direct OS command execution (vs. Python'simportlib.import_module()which requires a valid module path).Vulnerable code path (
configurable_utils.go:204-227):Attack scenario — a malicious YAML agent config can execute arbitrary commands:
Solution:
Two-layer defense-in-depth validation before
exec.Command()is called:Command Allowlist (
validateMCPCommand): Only known-safe MCP server launchers are permitted:npx,node,python,python3,uvx,uv,docker,deno,bun. Usesfilepath.Base()to handle full paths and strips Windows extensions (.exe,.cmd,.bat).Argument Injection Blocklist (
validateMCPArgs): Detects shell injection patterns in command arguments:;,&&,||,|, backticks,$(,${,>,<, newlines.Alignment with adk-python: This follows the same security hardening pattern applied in adk-python's
config_agent_utils.pywhich added_BLOCKED_MODULESand_BLOCKED_YAML_KEYSto mitigate CVE-2026-4810.Testing Plan
Unit Tests:
42 comprehensive test cases in
mcp_command_validation_test.go:Manual End-to-End (E2E) Tests:
Verified the following scenarios manually:
command: "npx"→ MCP toolset initializes normally ✅command: "/bin/sh"→ returns error"blocked MCP server command"✅args: ["-c", "malicious && payload"]→ returns error"blocked MCP server argument"✅command: "npx.cmd"resolves to allowed"npx"✅command: "/usr/bin/node"resolves to allowed"node"✅Checklist
Additional context
importlib.import_module()_BLOCKED_MODULES/_BLOCKED_YAML_KEYSapproach, adapted for Go'sexec.Command()attack vector