T09 · Insecure Skill Coding Practices
- Location
- src/request_publish.js:25
- Finding
- Shell Command Injection Through Untrusted CLI Arguments and Stored Post Data<![CDATA[ ## Vulnerability Details **File Location**: `src/request_publish.js:25-30, 68-70`; `src/approve_post.js:31-49`; `src/login.js:80-84, 108-122`; equivalent command construction also occurs in `src/force_login.js` and `src/verify_login.js` **Vulnerability Type**: OS command injection **Risk Level**: Critical ### Vulnerable Code ```js // src/request_publish.js:25-30 if (images.length > 0) { try { for (const img of images) { const uploadCmd = `node skills/feishu-sender/upload_image.js "${img}"`; const rawOutput = execSync(uploadCmd).toString(); ``` ```js // src/request_publish.js:68-70 const sendCmd = `node skills/feishu-sender/send_post.js "${TARGET_CHAT_ID}" '${msg}'`; try { execSync(sendCmd); ``` ```js // src/approve_post.js:31-37 const msg = JSON.stringify({ zh_cn: { title: "发布成功!🚦", content: [[{ tag: "text", text: `已执行审核通过的操作:${post.content}` }]] } }); require('child_process').execSync( `node skills/feishu-sender/send_post.js "${TARGET_CHAT_ID}" '${msg}'` ); ``` ```js // src/login.js:80-84 // Use send_post logic directly or via exec // Since we are inside node, we can just call the script or reuse logic if modular // But for now, exec is safest to avoid context issues const sendCmd = `node skills/feishu-sender/send_post.js "${TARGET_CHAT_ID}" '${msg}'`; execSync(sendCmd); ``` ### Technical Analysis The scripts interpolate caller-controlled values directly into command strings passed to `child_process.execSync()`. A shell interprets these strings. The affected values include: - `TARGET_CHAT_ID`, obtained from `process.argv[2]`. - Image paths obtained from command-line arguments. - Post content embedded in serialized Feishu messages. - Error text embedded in failure notifications. Double quotation marks do not prevent command substitution in common shells. In addition, the serialized message is enclosed in single quotation marks, but JSON strings can legitimately contain apost ...[truncated 1669 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Replace every string-based `execSync()` invocation with a non-shell API: ```js const { execFileSync } = require('child_process'); execFileSync(process.execPath, [ path.resolve('skills/feishu-sender/send_post.js'), TARGET_CHAT_ID, msg ], { shell: false, stdio: 'inherit' }); ``` 2. Invoke Feishu functionality through an imported module rather than launching another script where possible. 3. Pass large structured messages through stdin or a securely created file rather than shell syntax. 4. Apply strict allow-list validation to chat IDs. 5. Resolve image paths, verify that they are regular files beneath an approved asset directory, and reject symlinks and traversal. 6. Treat post content and error messages as untrusted even after human approval. 7. Add regression tests containing apostrophes, quotation marks, command separators, backticks, and command-substitution syntax. 8. Search the entire project for `exec`, `execSync`, or `{ shell: true }` and eliminate all cases where arguments can contain external data. ]]>
