T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/elpa_orchestrator.py:56
- Finding
- Shell Command Injection Through Configuration-Derived Commands and Placeholder Values<![CDATA[ ## Vulnerability Details **File Location**: `scripts/elpa_orchestrator.py`, lines 56-61, 92-95, and 139-147 **Vulnerability Type**: OS command injection caused by unsafe shell execution **Risk Level**: High ### Vulnerable Code ```python def _render_command(template: str, context: Dict[str, Any]) -> str: try: return template.format(**context) except KeyError as exc: missing = str(exc).strip("'") raise ValueError(f"missing placeholder '{missing}' in context for command: {template}") from exc ``` ```python context = _build_context(cfg, run_dir=run_dir, model_dir=model_dir, model_name=name) command = _render_command(cmd_template, context) ``` ```python completed = subprocess.run( item["train_cmd"], shell=True, cwd=item["model_dir"], env=env, stdout=out_f, stderr=err_f, check=False, ) ``` ### Technical Analysis The orchestrator reads `train_cmd` templates and placeholder values from a JSON configuration, combines them using Python string formatting, and submits the resulting string to `subprocess.run()` with `shell=True`. Because the generated command is interpreted by a command shell, shell metacharacters in either the template or substituted values retain their special meaning. A value intended to represent only a dataset path, project path, interpreter, or other argument can therefore introduce additional commands through characters such as `;`, `|`, command substitution, or redirection. For example, a configuration-derived dataset value conceptually equivalent to: ```text /path/to/data.csv; attacker-command ``` would be inserted directly into the command string. When execution is enabled, the shell interprets the text after the semicolon as another command. The documented design intentionally permits users to specify training commands. Nevertheless, the implementation does not distinguish executable command structure from data-only placeholder values. Consequently, configurations ob ...[truncated 1659 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Replace free-form shell command strings with structured argument arrays in the configuration. For example: ```json { "train_args": [ "{python_bin}", "{project_root}/model/train.py", "--data", "{dataset}", "--save-dir", "{model_dir}" ] } ``` 2. Render each argument independently and execute it without a shell: ```python completed = subprocess.run( rendered_args, shell=False, cwd=item["model_dir"], env=env, stdout=out_f, stderr=err_f, check=False, ) ``` 3. Validate data-only placeholders according to their intended types. Paths should be parsed as paths, numeric fields should remain numeric, and executable paths should be selected from an approved configuration or allowlist. 4. Do not attempt to make shell execution safe solely by applying generic quoting after command construction. Separating arguments and using `shell=False` provides a stronger security boundary. 5. If shell syntax is an unavoidable product requirement, explicitly document the configuration as trusted executable code, reject untrusted configurations, display a prominent confirmation before execution, and validate the configuration's provenance or signature. 6. Consider restricting environment-variable overrides, especially security-sensitive variables such as `PATH`, `PYTHONPATH`, and loader-related variables, when configurations are not fully trusted. ]]>