T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/ultra-play.js:43
- Finding
- Shell Command Injection in the Ultra Player<![CDATA[ ## Vulnerability Details **File Location**: `scripts/ultra-play.js:43-47`, `scripts/ultra-play.js:74-80`, and `scripts/ultra-play.js:146-152` **Vulnerability Type**: OS command injection through shell command construction **Risk Level**: High ### Vulnerable Code ```js function saveCache(cache) { try { fastExec(`echo '${JSON.stringify(cache)}' > ${CACHE_FILE}`); } catch (e) {} } ``` ```js if (cached && cached.videoId) { console.log(`⚡ Cache hit: ${cached.videoId}`); const url = `${YOUTUBE_WATCH}${cached.videoId}`; fastExec(`openclaw browser open --targetUrl="${url}"`); console.log(`✅ Played in ${Date.now() - start}ms (CACHED)`); return { videoId: cached.videoId, cached: true, time: Date.now() - start }; } ``` ```js case 'direct': const videoId = args[1]; console.log(`🎵 Direct play: ${videoId}`); fastExec(`openclaw browser open --targetUrl="${YOUTUBE_WATCH}${videoId}"`); console.log(`✅ Playing!`); break; ``` The commands are executed by this wrapper: ```js function fastExec(cmd, ignoreError = true) { try { return execSync(cmd, { encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'], timeout: 5000 }).trim(); } catch (e) { return ignoreError ? '' : e.message; } } ``` ### Technical Analysis `execSync()` receives a single command string, causing Node.js to invoke a system shell. User-controlled data is inserted directly into that command string. The `direct` command accepts `args[1]` as a video ID without validating its format. Because the value is placed inside a double-quoted shell argument, embedded quote characters, command substitutions, or other shell syntax can terminate or alter the intended argument. The cache-writing operation is also unsafe. Search queries become properties in the cache object and are serialized by `JSON.stringify()`. JSON serialization does not make the result safe for insertion into a single-quoted shell expression. A query containing a single quote can ...[truncated 1423 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Remove shell-based command construction and invoke the executable with an argument array: ```js const { execFileSync } = require('child_process'); function openBrowser(url) { execFileSync( 'openclaw', ['browser', 'open', `--targetUrl=${url}`], { encoding: 'utf8', stdio: ['ignore', 'pipe', 'ignore'], timeout: 5000 } ); } ``` 2. Validate direct video IDs before use: ```js function validateVideoId(videoId) { if (!/^[A-Za-z0-9_-]{11}$/.test(videoId)) { throw new Error('Invalid YouTube video ID'); } return videoId; } ``` 3. Replace shell commands used for cache operations with Node.js filesystem APIs: ```js const fs = require('fs'); function loadCache() { try { return JSON.parse(fs.readFileSync(CACHE_FILE, 'utf8')); } catch { return {}; } } function saveCache(cache) { fs.writeFileSync( CACHE_FILE, JSON.stringify(cache), { encoding: 'utf8', mode: 0o600 } ); } ``` 4. Use `fs.unlinkSync()` for cache deletion rather than invoking `rm`. 5. Treat cache contents as untrusted input. Validate cached video IDs and URLs again when reading them. 6. Add regression tests covering quotes, shell metacharacters, command substitutions, line breaks, malformed URLs, and poisoned cache content. ]]>