T09 · Insecure Skill Coding Practices
Error
- Location
- unpack.js:40
- Finding
- Shell Command Injection Through User-Controlled Unpack Paths<![CDATA[ ## Vulnerability Details **File Location**: `unpack.js:14-16, 34-40, 117-121` **Vulnerability Type**: OS command injection **Risk Level**: Critical ### Vulnerable Code ```js const packagePath = process.argv[2]; const targetDir = process.argv[3] || process.env.HOME; ``` ```js try { execSync(`tar -xzf "${packagePath}" -C "${targetDir}"`, { stdio: 'inherit' }); console.log('✅ Package extracted\n'); } catch (error) { console.error('❌ Extraction failed:', error.message); process.exit(1); } ``` A second shell invocation uses another path derived from the target directory: ```js const migrationPath = path.join(targetDir, 'MIGRATION.md'); if (fs.existsSync(migrationPath)) { console.log('📖 Migration instructions found. Opening...'); console.log(''); try { execSync(`cat "${migrationPath}"`, { stdio: 'inherit' }); } catch (error) { console.error('Could not display migration guide'); } } ``` ### Technical Analysis Both `packagePath` and `targetDir` are command-line arguments and are interpolated into command strings passed to `execSync`. Because `execSync` executes through a shell, shell metacharacters and command substitutions contained in these values may be interpreted rather than treated as literal path characters. Surrounding a value with double quotes does not make this safe. Command substitution such as `$(command)` remains active inside double quotes, and an embedded quote can terminate the quoted argument. The later `cat` command provides a second injection point because `migrationPath` incorporates the attacker-controlled target directory. ### Attack Path 1. An attacker persuades the user or Agent to unpack a package using a crafted package path or target directory. 2. The crafted path contains shell syntax, such as command substitution or an embedded quote followed by another command. 3. `unpack.js` interpolates the value into the `tar` command string. 4. The system shell evaluates the injected syntax when `execSy ...[truncated 692 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions - Do not construct shell command strings from user input. - Invoke `tar` with an argument array and with shell processing disabled: ```js const { spawnSync } = require('child_process'); const result = spawnSync( 'tar', ['-xzf', packagePath, '-C', targetDir], { stdio: 'inherit', shell: false } ); if (result.error || result.status !== 0) { throw result.error || new Error(`tar exited with status ${result.status}`); } ``` - Replace the `cat` subprocess with `fs.readFileSync(migrationPath, 'utf8')`. - Resolve inputs with `fs.realpathSync` where applicable and enforce an approved target-directory policy. - Reject null bytes and paths outside the expected migration area. - Add regression tests using paths containing quotes, command substitutions, semicolons, spaces, and newlines. ]]>