T09 · Insecure Skill Coding Practices
Warning
- Location
- reminder.js:14
- Finding
- Latent OS Command Injection in Push Notification Construction## Vulnerability Details **File Location**: `reminder.js`, lines 14-30 **Vulnerability Type**: OS command injection through unsafe shell interpolation **Risk Level**: Medium ### Vulnerable Code ```javascript function sendPushNotification(messageText) { // Use the openclaw command to send a message through the Feishu channel // --channel selects the channel; --recipient current selects the current user const cmd = `openclaw message send --channel feishu --recipient current --text "${messageText.replace(/"/g, '\\"')}"`; exec(cmd, (error, stdout, stderr) => { if (error) { console.error(`Push failed: ${error.message}`); return; } if (stderr) { console.error(`Push stderr: ${stderr}`); return; } console.log(`Push succeeded: ${stdout}`); }); } ``` ### Technical Analysis `sendPushNotification()` incorporates `messageText` into a command string passed to `child_process.exec()`. The function escapes only double quotation marks. This does not neutralize shell substitutions that remain active inside double-quoted strings, including: - Command substitution using `$(command)` - Command substitution using backticks - Environment-variable expansion - Certain backslash and shell-specific expansion sequences Event titles originate from user messages in `SOUL.md` and are incorporated into reminder messages without validation: ```javascript const title = parts.slice(1).join(' '); ``` The resulting title is persisted in `MEMORY.md` and later included in `messageText`. For example, a title containing `$(touch /tmp/pwned)` would cause the shell to execute `touch /tmp/pwned` when the command string reaches `exec()`. The issue is latent in the artifact as provided because `reminder.js` currently defines `loadEvents()` as an empty placeholder: ```javascript function loadEvents() { /* ... */ } ``` It therefore returns `undefined`, and `checkRemi ...[truncated 2201 chars]
- Remediation
- ## Remediation Suggestions 1. Replace `exec()` with `execFile()` or `spawn()` and pass every command argument separately, with shell processing explicitly disabled: ```javascript const { execFile } = require('child_process'); function sendPushNotification(messageText) { execFile( 'openclaw', [ 'message', 'send', '--channel', 'feishu', '--recipient', 'current', '--text', messageText ], { shell: false }, (error, stdout, stderr) => { if (error) { console.error(`Push failed: ${error.message}`); return; } if (stderr) { console.error(`Push stderr: ${stderr}`); } console.log(`Push succeeded: ${stdout}`); } ); } ``` 2. Do not attempt to make shell command construction safe through manual escaping. Correct escaping is platform- and shell-dependent and is unnecessary when argument-array APIs are used. 3. Validate event titles at ingestion: - Enforce a reasonable maximum length. - Reject control characters and null bytes. - Normalize unexpected Unicode control characters. - Treat validation as defense in depth rather than a replacement for eliminating the shell. 4. Implement `loadEvents()` securely and verify that the parsed value is an array before iterating over it. Reject malformed event records rather than silently forwarding their fields into privileged operations. 5. Run the skill under a dedicated, least-privileged operating-system account with restricted filesystem and credential access. 6. Add automated tests using titles containing `$(...)`, backticks, quotation marks, semicolons, newlines, and backslashes, and verify that none can trigger a secondary process.