T09 · Insecure Skill Coding Practices
- Location
mcp-server.mjs:24- Finding
Shell Command Injection Through MCP Tool Arguments
- Content
View full analysis
Vulnerability Details
File Location:
mcp-server.mjs, lines 24–29 and 76–89
Vulnerability Type: OS command injection
Risk Level: HighVulnerable Code
js function opExec(args) { const token = getSAToken(); return execSync(`op ${args}`, { env: { ...process.env, OP_SERVICE_ACCOUNT_TOKEN: token }, encoding: "utf8", timeout: 15000, }).trim(); }The MCP handlers construct the
argsvalue using untrusted tool parameters:js case "op_read_secret": { const vault = args.vault || DEFAULT_VAULT; const field = args.field || "api key"; const ref = `op://${vault}/${args.item}/${field}`; const value = opExec(`read "${ref}"`); return { content: [{ type: "text", text: value }], }; } case "op_list_items": { const vault = args.vault || DEFAULT_VAULT; const result = opExec(`item list --vault "${vault}" --format json`); const items = JSON.parse(result); const summary = items.map(i => `- ${i.title} (${i.category})`).join("\n"); return { content: [{ type: "text", text: summary || "No items found." }], }; }Technical Analysis
The MCP-controlled
item,vault, andfieldparameters are interpolated into a command string passed toexecSync(). By default,execSync()executes the string through a system shell.Placing input inside double quotes does not make it safe. An attacker can supply a quotation mark to terminate the intended argument and append shell operators and another command. No allowlist validation or shell-safe argument separation is applied.
The child process also receives:
js env: { ...process.env, OP_SERVICE_ACCOUNT_TOKEN: token }Consequently, an injected command inherits both the 1Password service-account token and every environment variable available to the MCP server.
Attack Path
- An attacker or compromised agent invokes
op_read_secretorop_list_items. - The attacker places shell syntax in a parameter, such as an
itemvalue conceptually shap ...[truncated 1355 chars]
- An attacker or compromised agent invokes
- Remediation
View remediation
Remediation Suggestions
-
Replace shell-based
execSync()with an argument-vector API such asexecFileSync():js import { execFileSync } from "node:child_process"; function opExec(args) { const token = getSAToken(); return execFileSync("op", args, { env: { ...process.env, OP_SERVICE_ACCOUNT_TOKEN: token }, encoding: "utf8", timeout: 15000, shell: false, }).trim(); } -
Pass every value as a distinct argument rather than constructing command strings:
js const value = opExec(["read", ref]); const result = opExec([ "item", "list", "--vault", vault, "--format", "json", ]); -
Validate MCP parameters before invocation:
- Require strings of reasonable maximum length.
- Reject control characters, null bytes, and line breaks.
- Validate 1Password references according to the supported vault, item, and field syntax.
- Reject unexpected object or array values.
-
Avoid relying on manual shell escaping. Argument-vector execution with
shell: falseshould be the primary security boundary. -
Minimize the child environment. Pass only variables required by
op, rather than copying all ofprocess.env, where operationally possible. -
Configure the 1Password service account with access only to required custom vaults. Grant
read_itemsby default and addwrite_itemsonly when write functionality is explicitly required. -
Add regression tests using quotation marks, command separators, command substitutions, newlines, and platform-specific shell metacharacters. Tests should verify that these values are either rejected or delivered to
opas literal single arguments.
-