T09 · Insecure Skill Coding Practices
Error
- Location
- SKILL.md:45
- Finding
- Shell Command Injection Through an Unsafely Generated Cleanup Path<![CDATA[ ## Vulnerability Details **File Location**: `SKILL.md:45`; `scripts/gif_multi.py:228-231`; `scripts/gif_multi.py:344-347` **Vulnerability Type**: Shell command injection through an attacker-influenced filename **Risk Level**: High ### Vulnerable Code ```markdown 5. **Clean up**: `exec(rm <path>)` after sending. ``` ```python # Unique per-channel filename with timestamp — no topic/thread collisions safe_ch = channel.replace("/", "_").replace(" ", "_") ts = int(time.time() * 1000) out_base = os.path.join(out_dir, f"gif_{safe_ch}_{ts}") ``` ```python gif_path = os.path.join(out_dir, f"source_{channel}.gif") urllib.request.urlretrieve(result["gif_url"], gif_path) conv = convert(gif_path, channel, out_dir) ``` ### Technical Analysis The `--channel` value contributes directly to generated filenames. The only sanitization applied to the converted output filename replaces forward slashes and spaces: ```python safe_ch = channel.replace("/", "_").replace(" ", "_") ``` This does not remove or reject shell metacharacters such as semicolons, command substitutions, backticks, quotes, redirection operators, or newline characters. The source GIF filename uses the channel value without even this limited sanitization. The script returns the resulting path to the Agent, while `SKILL.md` instructs the Agent to delete that path using the shell-oriented operation `exec(rm <path>)`. If the Agent interpolates the returned path into this command without robust argument separation or shell escaping, shell metacharacters embedded in the channel value can be interpreted as executable syntax. The use of `subprocess.run()` for FFmpeg does not itself introduce shell injection because it passes an argument list and does not enable `shell=True`. The vulnerable boundary is the documented cleanup operation performed after the script returns an attacker-influenced path. ### Attack Path 1. An attacker causes a crafted channel identifier to be passed to the script, directly th ...[truncated 1386 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Do not perform cleanup by interpolating a returned path into shell command text. 2. Delete the file through a filesystem API such as `os.remove()` inside the Python script after the send operation, or use a tool interface that passes the path as a distinct argument without invoking a shell. 3. If external cleanup is unavoidable, pass an argument array equivalent to `["rm", "--", path]` rather than constructing `rm <path>`. 4. Validate channel identifiers against a strict allowlist derived from `CHANNEL_PROFILES` or discovered channel IDs. 5. Generate cache filenames independently of user-controlled values, such as with `tempfile`, a UUID, or a cryptographically random token. 6. If a channel label must appear in a filename, permit only a narrow character set such as ASCII letters, digits, underscores, and hyphens. Reject rather than partially transform invalid identifiers. 7. Ensure the final resolved file path remains inside the expected cache directory before creating, returning, or deleting it. 8. Update `SKILL.md` to prescribe a non-shell cleanup mechanism. ]]>