T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/workflow.js:142
- Finding
- Shell Command Injection Through User-Controlled PDF Paths<![CDATA[ ## Vulnerability Details **File Location**: `scripts/workflow.js:142-146` **Vulnerability Type**: OS command injection **Risk Level**: High ### Vulnerable Code ```javascript const scriptPath = path.join(__dirname, 'generate-pdf.js'); const cmd = `node "${scriptPath}" "${path.resolve(html)}" ${output ? `"${path.resolve(output)}"` : ''}`; const { execSync } = require('child_process'); try { execSync(cmd, { cwd: WORKSPACE, stdio: 'inherit', timeout: 60000 }); ``` ### Technical Analysis The HTML input path and optional output path are incorporated into a command string that is executed through `execSync()`. String-based `execSync()` invokes a command shell. Although the paths are passed through `path.resolve()`, path resolution does not escape embedded quotation marks, command separators, command substitutions, or other shell metacharacters. On filesystems that permit such characters, an attacker-controlled filename can terminate the intended quoted argument and append another shell command. The vulnerable values originate from command-line arguments: ```javascript const html = args[pdfIdx + 1]; const output = args[pdfIdx + 2]; ``` Consequently, any caller able to influence the supplied input or output filename can potentially inject commands. ### Attack Path 1. An attacker creates or supplies an HTML file with a shell-sensitive filename, such as one containing a quotation mark and command separator. 2. The Agent or user invokes: ```bash node scripts/workflow.js --pdf '<attacker-controlled-path>' ``` 3. `path.resolve()` converts the path to an absolute path but preserves the shell-sensitive filename characters. 4. The application places the path inside the `cmd` string. 5. `execSync()` passes the resulting string to a shell. 6. The shell interprets the injected syntax and executes the attacker's command in addition to, or instead of, the PDF generator. An equivalent attack is possible through the optional output path. ### Impact ...[truncated 613 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions Avoid constructing a shell command. Invoke Node.js directly with an argument array: ```javascript const { execFileSync } = require('child_process'); const resolvedHtml = path.resolve(html); const childArgs = [scriptPath, resolvedHtml]; if (output) { childArgs.push(path.resolve(output)); } execFileSync(process.execPath, childArgs, { cwd: WORKSPACE, stdio: 'inherit', timeout: 60000 }); ``` Additional hardening should include: 1. Verify that the input is a regular file and has an expected `.html` extension. 2. Restrict input and output paths to an approved workspace directory using `path.relative()`. 3. Reject null bytes and paths that escape the approved directory. 4. Refuse to overwrite existing files unless explicitly requested. 5. Do not reintroduce `shell: true` when invoking the child process. 6. Add regression tests using filenames containing quotes, semicolons, dollar signs, backticks, spaces, and command-substitution syntax. ]]>