T09 · Insecure Skill Coding Practices
Error
- Location
- src/executor.ts:21
- Finding
- Shell Command Injection in Wallet Transaction Execution<![CDATA[ ## Vulnerability Details **File Location**: `src/executor.ts:21-42` **Vulnerability Type**: OS command injection through unsafe shell command construction **Risk Level**: High ### Vulnerable Code ```ts const { topic, chain = "eip155:56" } = options; const { params } = step; const args = [ "call", "--topic", topic, "--chain", chain, "--to", params.to, "--data", params.data, ]; if (params.value && params.value > 0n) { args.push("--value", params.value.toString()); } try { const cmd = `node "${WALLET_CONNECT_CLI}" ${args.map((a) => `"${a}"`).join(" ")}`; const result = execSync(cmd, { encoding: "utf-8", timeout: 5 * 60 * 1000, env: { ...process.env, WALLETCONNECT_PROJECT_ID: process.env.WALLETCONNECT_PROJECT_ID, }, }); ``` ### Technical Analysis The executor builds a single command string and passes it to `execSync()`. String-based `execSync()` execution invokes a command shell. Arguments are surrounded with double quotes, but embedded quotation marks, command substitutions, and other shell metacharacters are not escaped. The WalletConnect topic can originate from the `--wallet-topic` command-line option or persisted wallet-session data. It is checked only for presence and is not constrained to an expected topic format. SDK-provided transaction fields are also interpolated into the same command string. Double-quote wrapping does not provide a safe process boundary. A malicious value containing a closing quote and shell syntax can escape the intended argument and cause the shell to interpret additional commands. ### Attack Path 1. An attacker causes a crafted WalletConnect topic to enter the application through `--wallet-topic`, persisted lending context, or wallet-session data. 2. The user or Agent invokes a state-changing lending operation such as deposit, withdraw, supply, borrow, or repay. 3. The operation reaches `executeStep()` with the attacker-controlled topic. 4. `executeStep()` i ...[truncated 1262 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions Eliminate shell interpretation entirely. Execute Node directly with an argument array and explicitly disable shell processing: ```ts import { execFileSync } from "node:child_process"; const result = execFileSync( process.execPath, [WALLET_CONNECT_CLI, ...args], { encoding: "utf-8", timeout: 5 * 60 * 1000, shell: false, env: { ...process.env, WALLETCONNECT_PROJECT_ID: process.env.WALLETCONNECT_PROJECT_ID, }, } ); ``` Alternatively, use `spawnSync()` with the executable and arguments supplied separately. Apply defense-in-depth validation before process creation: - Require WalletConnect topics to match the exact format and length accepted by the wallet integration. - Restrict `chain` to the supported chain allowlist. - Validate `params.to` as an EVM address. - Validate `params.data` as bounded hexadecimal calldata. - Validate transaction values as non-negative integers within expected limits. - Resolve and verify the wallet CLI path before execution. - Add regression tests containing quotes, command substitutions, spaces, semicolons, and newline characters to verify that all values remain literal arguments. ]]>