T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/shot.js:13
- Finding
- Shell Command Injection in the Screenshot Utility<![CDATA[ ## Vulnerability Details **File Location**: `scripts/shot.js:13, 26-30, 87` **Vulnerability Type**: OS command injection through shell command construction **Risk Level**: High ### Vulnerable Code ```js const fs=require('fs'),path=require('path'),{execSync}=require('child_process'); const[, ,file,targetMs,out,fx,fy,sc]=process.argv; function findBrowser(){ const cands=[]; if(process.env.BROWSER_PATH)cands.push(process.env.BROWSER_PATH); // ... } execSync( '"'+browser+ '" --headless=new --disable-gpu --hide-scrollbars --user-data-dir="'+profile+ '" --no-first-run --no-default-browser-check --screenshot="'+path.resolve(out)+ '" --window-size=1920,1080 --virtual-time-budget='+budget+ ' "file:///'+tmp.replace(/\\/g,'/')+'"', {stdio:'pipe',timeout:120000} ); ``` ### Technical Analysis The utility passes a dynamically constructed string to `child_process.execSync`. Unlike an argument-array API, `execSync` interprets the supplied string through the operating-system shell. The following values are inserted into the command without shell-safe escaping: - The user-controlled screenshot output argument, `out` - The environment-controlled browser path, `BROWSER_PATH` - Other dynamically constructed filesystem paths Wrapping a value in double quotes is not sufficient shell escaping. An attacker-controlled value containing a quote can terminate the intended quoted argument, after which shell metacharacters can introduce an additional command. Resolving the output with `path.resolve()` normalizes the path but does not remove quotes, command separators, command substitutions, or other shell syntax. The CDP fallback in `scripts/cdp-shot.js` does not have this defect because it invokes `spawn(browser, args, ...)` with an argument array. The vulnerable CLI path is attempted by default unless `SHOT_ENGINE=cdp` is set. ### Attack Path 1. An attacker gains control over the output filename supplied to `scripts/shot.js`, or controls the `BROWS ...[truncated 1323 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Replace `execSync` with an argument-array API that does not invoke a shell: ```js const { spawnSync } = require('child_process'); const args = [ '--headless=new', '--disable-gpu', '--hide-scrollbars', `--user-data-dir=${profile}`, '--no-first-run', '--no-default-browser-check', `--screenshot=${path.resolve(out)}`, '--window-size=1920,1080', `--virtual-time-budget=${budget}`, `file:///${tmp.replace(/\\/g, '/')}` ]; const result = spawnSync(browser, args, { stdio: 'pipe', timeout: 120000, shell: false }); if (result.error) throw result.error; if (result.status !== 0) { throw new Error(`Browser exited with status ${result.status}`); } ``` 2. Alternatively, use `execFileSync(browser, args, options)`, which also separates the executable from its arguments. 3. Validate `BROWSER_PATH` before execution: - Require an absolute path. - Verify that it exists and is a regular executable file. - Reject NUL bytes and control characters. - Where practical, restrict it to an administrator-approved browser allowlist. 4. Validate output paths: - Require a `.png` extension. - Resolve the path under a designated screenshot directory. - Verify with `path.relative()` that the resolved output remains inside that directory. 5. Consider making the existing CDP implementation the only execution path, since it already uses `spawn` with `shell: false`. 6. Add regression tests using arguments containing quotes, spaces, command separators, and command-substitution characters. The tests should verify that such values are rejected or passed as literal arguments and never interpreted by a shell. ]]>