T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/ecosystems.py:53
- Finding
- Arbitrary Shell Command Injection Through User-Controlled Search Queries## Vulnerability Details **File Location**: `scripts/ecosystems.py:53-68` **Vulnerability Type**: OS command injection through unsafe shell invocation **Risk Level**: High ### Vulnerable Code ```python # Build search query from keywords search_terms = " ".join(query.get('keywords', [])) if not search_terms: search_terms = query.get('task', '') if not search_terms: return [] # Execute Skills CLI search cmd = f'npx skills find "{search_terms}" --json' result = subprocess.run( cmd, shell=True, capture_output=True, text=True, timeout=10 ) ``` ### Technical Analysis The search terms originate from the user's natural-language query and are interpolated directly into a command string. The command is then executed with `shell=True`, causing shell metacharacters, command substitutions, and quote characters in the query to be interpreted by the operating-system shell. Quoting `search_terms` with double quotes is not sufficient. An attacker can include a closing double quote followed by a shell command. The query is split into keywords by `understand_user_need()` in `scripts/skill_finder.py`, but that operation does not sanitize shell syntax. The first five attacker-controlled tokens can therefore reach this command construction. The subprocess timeout only limits execution duration; it does not prevent injected commands from reading files, changing files, spawning background processes, or making network requests. ### Attack Path 1. An attacker supplies a crafted skill-search query containing a closing quote and a shell command, such as tokens structured to produce `" ; id ; echo "`. 2. `SmartSkillFinder.understand_user_need()` converts the query to keyword tokens without removing shell syntax. 3. `search_ecosystems()` passes those tokens to `search_skills_cli()`. 4. `search_skills_cli()` joins the tokens and inserts them into the command string. 5. `subprocess.run(..., ...[truncated 734 chars]
- Remediation
- ## Remediation Suggestions Eliminate shell interpretation and pass each argument separately: ```python result = subprocess.run( ["npx", "--no-install", "skills", "find", search_terms, "--json"], shell=False, capture_output=True, text=True, timeout=10, check=False, ) ``` Apply the following additional controls: 1. Reject control characters and enforce a reasonable maximum query length. 2. Use an allowlisted local executable path rather than relying on shell command resolution. 3. Prefer a documented HTTPS API over invoking a package runner. 4. Run the search integration with minimal filesystem and network privileges. 5. Add regression tests containing quotes, semicolons, command substitutions, newlines, pipes, and redirection operators. 6. Ensure all duplicate implementations use the corrected invocation pattern.