T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/sync-project-skills.js:127
- Finding
- Command Injection Through Project-Controlled Skill Names and Versions<![CDATA[ ## Vulnerability Details **File Location**: `scripts/sync-project-skills.js:127-140`; insufficient validation in `scripts/validate-config.js:137-151` **Vulnerability Type**: OS command injection **Risk Level**: High ### Vulnerable Code ```js function installFromClawhub(skillName, version, force = false) { try { log(`📦 Installing ${skillName} from clawhub...`); const versionSpec = version && version !== 'latest' ? `@${version}` : ''; const forceFlag = force ? ' --force' : ''; const cmd = `clawhub install ${skillName}${versionSpec}${forceFlag}`; execSync(cmd, { stdio: 'inherit', cwd: process.cwd(), env: { ...process.env, FORCE_COLOR: '1' } }); log(`✅ Installed ${skillName}`); return true; } catch (err) { log(`❌ Failed to install ${skillName}: ${err.message}`, 'error'); return false; } } ``` The validator accepts arbitrary non-empty strings without restricting shell metacharacters: ```js if (typeof skill === 'string') { if (!skill.trim()) { errors.push(`${skillPath}: skill name cannot be empty`); } } else if (typeof skill === 'object') { if (!skill.name) { errors.push(`${skillPath}: missing required field "name"`); } if (skill.version && typeof skill.version !== 'string') { errors.push(`${skillPath}.version: must be a string`); } } ``` ### Technical Analysis The `skillName` and `version` values originate from `.openclaw-skills.json`, which may be supplied by an untrusted project or downloaded configuration. These values are directly interpolated into a command string passed to `child_process.execSync()`. By default, `execSync()` executes its string through a platform shell. Shell control characters embedded in either field can therefore terminate or alter the intended `clawhub install` command and introduce an additional command. Type and emptiness checks do not prevent this because malicious values remain valid JSON strings. The synchroniza ...[truncated 1871 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Replace shell-based execution with an argument-array API: ```js const { execFileSync } = require('child_process'); const packageSpec = version && version !== 'latest' ? `${skillName}@${version}` : skillName; const args = ['install', packageSpec]; if (force) args.push('--force'); execFileSync('clawhub', args, { stdio: 'inherit', cwd: process.cwd(), env: { ...process.env, FORCE_COLOR: '1' }, shell: false }); ``` 2. Enforce strict allow-list validation for skill names and versions. Reject whitespace, path separators, shell metacharacters, control characters, and unexpected package syntax. 3. Perform validation inside `loadConfig()` so synchronization cannot bypass it. 4. Require every skill object to contain a non-empty string `name`; reject arrays, nested objects, and unexpected fields where practical. 5. Resolve and display the selected configuration path, then request confirmation before processing a configuration from a parent directory or newly cloned repository. 6. Add security regression tests containing shell metacharacters in both `name` and `version` and verify that no secondary command is executed. 7. Consider running third-party installation in a restricted subprocess or sandbox with minimal filesystem, credential, and network access. ]]>