T09 · Insecure Skill Coding Practices
Warning
- Location
- SKILL.md:650
- Finding
- Shell Command Injection and API Key Exposure in Transaction Submission Example<![CDATA[ ## Vulnerability Details **File Location**: `SKILL.md`, lines 650-656 **Vulnerability Type**: Shell command injection and insecure credential handling **Risk Level**: Medium ### Vulnerable Code ```javascript function submitTx(tx) { const result = JSON.parse(execSync( `curl -s -X POST https://api.bankr.bot/agent/submit ` + `-H "X-API-Key: ${process.env.BANKR_API_KEY}" ` + `-H "Content-Type: application/json" ` + `-d '${JSON.stringify({ transaction: tx })}'` ).toString()); console.log(`TX: ${result.transactionHash}`); return result; } ``` The example imports the shell execution function at `SKILL.md:625`: ```javascript import { execSync } from "child_process"; ``` ### Technical Analysis The documented transaction workflow constructs a shell command by directly interpolating `process.env.BANKR_API_KEY` and serialized transaction data into a string passed to `execSync`. By default, string-based `execSync` executes the command through a system shell. Shell quoting does not provide a reliable security boundary here: - The API key is placed inside double quotes, where shell substitutions and some metacharacters may still be interpreted. - The serialized transaction is placed inside single quotes. An apostrophe in attacker-influenced data could terminate that quoted argument and introduce additional shell commands. - The expanded API key becomes part of the spawned process command line and may be exposed through local process inspection, diagnostic output, crash reporting, or command logging. - The code does not validate the transaction destination, chain ID, transferred value, or calldata immediately before submission. The helper library normally emits constrained hexadecimal transaction fields, which reduces exposure when only its standard builders are used. However, `submitTx` accepts an arbitrary object, and the example establishes an unsafe pattern that users may reuse with externally supplied transaction data. ### Atta ...[truncated 1186 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions Replace shell-based `curl` execution with Node.js `fetch` so neither the credential nor request body is interpreted by a shell: ```javascript async function submitTx(tx) { validateTransaction(tx); const response = await fetch("https://api.bankr.bot/agent/submit", { method: "POST", headers: { "X-API-Key": process.env.BANKR_API_KEY, "Content-Type": "application/json", }, body: JSON.stringify({ transaction: tx }), }); if (!response.ok) { throw new Error(`Bankr request failed with status ${response.status}`); } const result = await response.json(); console.log(`TX: ${result.transactionHash}`); return result; } ``` Apply the following additional controls: 1. Confirm that `BANKR_API_KEY` is present without printing or logging it. 2. Validate `tx.to` against an explicit allowlist of expected contracts. 3. Require `tx.chainId === 8453`. 4. Parse and enforce an upper limit on `tx.value`. 5. Verify that `tx.data` is hexadecimal and that its function selector matches the intended operation. 6. Present the destination, operation, and value for explicit user approval before submission. 7. Use a restricted Bankr credential with the minimum available permissions. 8. If an external process is unavoidable, use `execFileSync` or `spawn` with a fixed executable and argument array, with `shell: false`; do not place secrets directly in a shell command string. ]]>