T09 · Insecure Skill Coding Practices
Error
- Location
- search.py:12
- Finding
- Shell Command Injection Through User-Controlled Arguments## Vulnerability Details **File Location**: `search.py:12-18`, `search.py:25`, and `search.py:60` **Vulnerability Type**: OS command injection through unsafe shell command construction **Risk Level**: High ### Vulnerable Code ```python def run_mcporter(command): """执行 mcporter 命令""" result = subprocess.run( command, shell=True, capture_output=True, text=True, timeout=60 ) return result.stdout, result.stderr, result.returncode ``` ```python cmd = f'mcporter call minimax.web_search query:"{query}"' stdout, stderr, code = run_mcporter(cmd) ``` ```python cmd = f'mcporter call minimax.understand_image prompt:"{prompt}" image_source:"{image_source}"' stdout, stderr, code = run_mcporter(cmd) ``` ### Technical Analysis The `query`, `prompt`, and `image_path` values originate from command-line arguments and are interpolated directly into shell command strings. The resulting strings are executed with `shell=True`. Double quotes around the interpolated values do not provide effective shell escaping. An attacker can inject quotation marks, command substitutions, or other shell syntax to escape the intended argument context. The shell will interpret the injected syntax before invoking `mcporter`. The issue affects both exposed operations: - `web_search` interpolates the attacker-controlled `query`. - `understand_image` interpolates the attacker-controlled `prompt` and `image_path`. Converting a local path to an absolute path does not neutralize shell metacharacters. ### Attack Path 1. An attacker supplies a crafted value through `--query`, `--prompt`, or `--image`. 2. The application inserts the value into a command string without shell-safe escaping. 3. `run_mcporter()` passes that string to `subprocess.run()` with `shell=True`. 4. The operating-system shell parses the attacker-controlled syntax. 5. The injected command executes with the ...[truncated 958 chars]
- Remediation
- ## Remediation Suggestions Eliminate shell interpretation and pass each command argument as a separate list element: ```python def run_mcporter(command): return subprocess.run( command, shell=False, capture_output=True, text=True, timeout=60, check=False ) result = run_mcporter([ "mcporter", "call", "minimax.web_search", f"query:{query}", ]) ``` Apply the same approach to image analysis: ```python result = run_mcporter([ "mcporter", "call", "minimax.understand_image", f"prompt:{prompt}", f"image_source:{image_source}", ]) ``` Additional hardening should include: - Do not attempt to retain `shell=True` through manual escaping; argument-array execution is safer. - Validate image URLs against an explicit allowlist of accepted schemes such as `https`. - Define appropriate length limits for queries, prompts, URLs, and paths. - Validate local image paths against an approved directory if callers must not access arbitrary local files. - Resolve the executable using a trusted, fixed path where deployment permits, reducing path-based executable substitution risks. - Return controlled error messages rather than exposing arbitrary subprocess diagnostic output to untrusted callers.