T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/render-elk.mjs:273
- Finding
- Shell Command Injection During PNG Conversion<![CDATA[ ## Vulnerability Details **File Location**: `scripts/render-elk.mjs:273-281` **Vulnerability Type**: OS command injection through shell-interpreted file paths **Risk Level**: High ### Vulnerable Code ```js if (doPng) { try { const pngName = file.replace('.json', '.png'); execSync(`sips -s format png "${join(outDir, svgName)}" --out "${join(outDir, pngName)}" 2>/dev/null`); console.log(` ✅ ${file} → svg/${svgName} + svg/${pngName}`); } catch { console.log(` ✅ ${file} → svg/${svgName} (PNG conversion failed — sips not available?)`); } } ``` ### Technical Analysis The script constructs a shell command by interpolating the user-selected directory and discovered JSON filename into a string passed to `execSync`. Node.js executes this string through a shell. Although the paths are surrounded by double quotes, double quotes do not suppress shell command substitution such as `$(...)`. Embedded quotes can also terminate the intended quoted argument. Consequently, a malicious directory name or `.json` filename containing shell metacharacters can cause arbitrary commands to be evaluated. The `.json` filename filter does not prevent exploitation because a malicious filename can contain shell syntax while still ending in `.json`. The broad `catch` block may also obscure exploitation by reporting a normal PNG conversion failure after the injected command has run. ### Attack Path 1. An attacker creates or supplies a diagram directory containing a valid JSON file whose filename includes shell command-substitution syntax and ends in `.json`. 2. A user runs the documented batch conversion command with PNG generation enabled: ```bash node scripts/render-elk.mjs --dir <attacker-controlled-folder> --png ``` 3. The malicious filename is used to construct `svgName` and `pngName`. 4. The interpolated command is passed to `execSync`. 5. The shell evaluates the attacker-controlled syntax before invoking `sips`. 6. The injected command exe ...[truncated 529 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions Avoid invoking a shell. Replace `execSync` with an argument-based process API such as `execFileSync`: ```js import { execFileSync } from 'child_process'; execFileSync( 'sips', ['-s', 'format', 'png', join(outDir, svgName), '--out', join(outDir, pngName)], { stdio: 'ignore' } ); ``` Additional hardening should include: 1. Reject filenames containing control characters. 2. Resolve and normalize input and output paths. 3. Verify that generated output paths remain inside the intended output directory. 4. Avoid broad error handling that conceals the actual failure reason; report conversion errors safely without exposing sensitive environment data. 5. Add regression tests using filenames containing spaces, quotes, dollar signs, parentheses, semicolons, and newline characters. ]]>