T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/hydrate_food_images.py:78
- Finding
- Shell Command Injection in the Optional External AI Image Generator<![CDATA[ ## Vulnerability Details **File Location**: `scripts/hydrate_food_images.py:78-88` **Vulnerability Type**: Shell command injection **Risk Level**: High ### Vulnerable Code ```python def external_ai_generate(name: str, out_path: Path, external_ai_cmd: str, timeout: float = 60.0) -> bool: if not external_ai_cmd.strip(): return False cmd = ( external_ai_cmd .replace("{name}", name) .replace("{out_path}", str(out_path)) ) try: result = subprocess.run(cmd, shell=True, check=False, timeout=timeout) ``` ### Technical Analysis The external AI fallback builds a shell command by directly replacing `{name}` and `{out_path}` placeholders with string values and then passes the resulting command to `subprocess.run` with `shell=True`. Because the shell interprets metacharacters such as semicolons, command substitutions, pipes, and redirection operators, placeholder values are treated as executable shell syntax rather than literal command arguments. Dish names normally originate from `assets/menu_db.json`. However, `scripts/expand_menu_db.py` can regenerate database entries from filenames under `assets/foods_image`. Consequently, a crafted filename can become a dish name and later reach this shell command when the external AI fallback is enabled. The `--external-ai-cmd` option is intentionally user-configurable, but this does not make direct shell interpolation safe. Command templates and substituted data must be represented as separate arguments. ### Attack Path 1. An attacker or untrusted archive introduces a file under `assets/foods_image` whose filename stem contains shell metacharacters and an injected command. 2. The operator runs `scripts/expand_menu_db.py`, which imports filename stems into `assets/menu_db.json`. 3. The corresponding image is removed or otherwise becomes missing, causing the dish to be processed by the hydration workflow. 4. The operator runs `scripts/hydrate_food_images.py` ...[truncated 1065 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Remove `shell=True` and invoke the external program through an argument array with `shell=False`. 2. Parse the command template into a controlled argument list before placeholder substitution. Prefer an interface where the executable and each argument are configured separately. 3. Replace placeholders independently in each argument so dish names and paths remain literal values. 4. Validate dish names against a strict allowlist of permitted characters and reject control characters, path separators, and shell metacharacters. 5. Resolve the output path and verify that it remains inside `assets/foods_image`. 6. Consider allowing only explicitly approved external executables rather than accepting an unrestricted command template. 7. Log the executable and sanitized argument list without recording sensitive values. A safer pattern is: ```python import shlex import subprocess template_args = shlex.split(external_ai_cmd) argv = [ arg.replace("{name}", name).replace("{out_path}", str(out_path)) for arg in template_args ] result = subprocess.run( argv, shell=False, check=False, timeout=timeout, ) ``` For stronger protection, avoid parsing a free-form command string altogether and accept a structured executable path and repeated argument options. ]]>