T09 · Insecure Skill Coding Practices
Error
- Location
- docs/AGENT-INTEGRATION.md:135
- Finding
- Shell Command Injection in Documented CLI Integration<![CDATA[ ## Vulnerability Details **File Location**: `docs/AGENT-INTEGRATION.md:135-139, 168-179, 190-202` **Vulnerability Type**: OS command injection through unescaped shell interpolation **Risk Level**: High ### Vulnerable Code ```javascript import { execSync } from 'child_process' const GAME_TYPE = process.env.CLABCRAW_GAME_TYPE || 'poker' const result = JSON.parse( execSync(`node bins/clabcraw-join --game ${GAME_TYPE}`, { encoding: 'utf-8' }) ) ``` The same unsafe pattern is used with game and action data: ```javascript async function playGame(gameId) { while (true) { const state = JSON.parse( execSync(`node bins/clabcraw-state --game ${gameId}`, { encoding: 'utf-8' }) ) if (state.game_status === 'finished') break if (state.is_your_turn) { const action = decideAction(state) let cmd = `node bins/clabcraw-action --game ${gameId} --action ${action.action}` if (action.amount) cmd += ` --amount ${action.amount}` execSync(cmd, { encoding: 'utf-8' }) } await sleep(500) } } ``` Additional examples repeat the vulnerable construction: ```javascript try { execSync(`node bins/clabcraw-join --game ${GAME_TYPE}`, { encoding: 'utf-8' }) } catch (err) { const body = JSON.parse(err.stderr || '{}') if (body.retry_after_seconds) { await sleep(body.retry_after_seconds * 1000) } } try { execSync(`node bins/clabcraw-action --game ${gameId} --action raise --amount 1`) } catch (err) { const body = JSON.parse(err.stderr || '{}') console.log('Valid actions:', body.valid_actions) } ``` ### Technical Analysis `execSync()` executes a command string through a system shell. The examples directly interpolate values originating from the environment, API responses, or strategy output: - `GAME_TYPE` comes from `CLABCRAW_GAME_TYPE`. - `gameId` can originate from the remote game service. - `action.action` and `action.amount` originate from strategy logic or game state process ...[truncated 1883 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions Avoid constructing shell command strings. Use an API that passes executable arguments separately, such as `execFileSync()`: ```javascript import { execFileSync } from 'child_process' const allowedGameTypes = new Set([ 'poker', 'poker-pro', 'poker-novice', 'chess' ]) if (!allowedGameTypes.has(GAME_TYPE)) { throw new Error('Unsupported game type') } const output = execFileSync( process.execPath, ['bins/clabcraw-join', '--game', GAME_TYPE], { encoding: 'utf-8' } ) const result = JSON.parse(output) ``` Apply the following hardening measures: 1. Prefer the non-shell `GameClient` API throughout the documentation. 2. Replace every `execSync(commandString)` example with `execFileSync()` or `spawn()` using an argument array and `shell: false`. 3. Validate game types and action names against explicit allowlists. 4. Validate game IDs against the exact expected UUID format before use. 5. Parse amounts as finite integers and enforce game-specific minimum and maximum values. 6. Never attempt to make command strings safe through ad hoc quoting alone. 7. Add tests using shell metacharacters to verify that supplied values remain literal arguments. ]]>