T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/setup.sh:125
- Finding
- JavaScript Code Injection Through Unsafely Interpolated Setup Arguments<![CDATA[ ## Vulnerability Details **File Location**: `scripts/setup.sh`, lines 125-139 **Vulnerability Type**: User-controlled data embedded directly into JavaScript source **Risk Level**: High ### Vulnerable Code ```bash node -e " const fs = require('fs'); let config = {}; try { config = JSON.parse(fs.readFileSync('$CONFIG_PATH', 'utf8')); } catch {} if (!config.mcpServers) config.mcpServers = {}; config.mcpServers.lighthouse = { command: 'npx', args: ['-y', 'lighthouse-mcp-server'], env: { TENCENTCLOUD_SECRET_ID: '$SECRET_ID', TENCENTCLOUD_SECRET_KEY: '$SECRET_KEY' } }; fs.writeFileSync('$TEMP_CONFIG', JSON.stringify(config, null, 2)); " ``` ### Technical Analysis The script inserts `CONFIG_PATH`, `SECRET_ID`, `SECRET_KEY`, and `TEMP_CONFIG` directly into a JavaScript program passed to `node -e`. These values are not encoded as JavaScript string literals. A value containing a single quote can terminate the surrounding JavaScript string and append arbitrary JavaScript. Because Node.js exposes modules such as `child_process`, successful injection can lead directly to operating-system command execution. This path is reached whenever the selected configuration file already exists. Shell quoting at argument parsing does not mitigate the issue because the injection occurs later when the script constructs JavaScript source. ### Attack Path 1. An attacker supplies a malicious SecretId, SecretKey, or configuration path to the setup workflow. 2. The targeted configuration file already exists, causing the update branch to run. 3. The malicious value closes the JavaScript string and introduces additional JavaScript statements. 4. `node -e` evaluates the resulting source. 5. Attacker-controlled JavaScript invokes system commands with the privileges of the user running `setup.sh`. For example, the structural form of a malicious value could be: ```text '; require('child_process').execSync('ATTACKER_COMMAND'); // ``` The e ...[truncated 512 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions Do not construct executable JavaScript by interpolating shell variables. Pass values through environment variables or positional arguments and retrieve them as data: ```bash CONFIG_PATH="$CONFIG_PATH" \ TEMP_CONFIG="$TEMP_CONFIG" \ SECRET_ID="$SECRET_ID" \ SECRET_KEY="$SECRET_KEY" \ node <<'NODE' const fs = require('fs'); const { CONFIG_PATH, TEMP_CONFIG, SECRET_ID, SECRET_KEY } = process.env; let config = {}; try { config = JSON.parse(fs.readFileSync(CONFIG_PATH, 'utf8')); } catch (error) { throw new Error(`Unable to parse existing configuration: ${error.message}`); } config.mcpServers ??= {}; config.mcpServers.lighthouse = { command: 'npx', args: ['-y', 'lighthouse-mcp-server'], env: { TENCENTCLOUD_SECRET_ID: SECRET_ID, TENCENTCLOUD_SECRET_KEY: SECRET_KEY } }; fs.writeFileSync(TEMP_CONFIG, JSON.stringify(config, null, 2), { mode: 0o600 }); NODE ``` Additionally: - Validate `CONFIG_PATH` against an expected directory. - Reject unexpected control characters in credential identifiers. - Do not silently replace malformed existing JSON. - Add regression tests using quotes, newlines, backslashes, and JavaScript-like input. ]]>