T09 · Insecure Skill Coding Practices
Warning
- Location
- scripts/tesla-smart-charge.py:184
- Finding
- Shell Command Injection in the Automatic Charging Path<![CDATA[ ## Vulnerability Details **File Location**: `scripts/tesla-smart-charge.py`, lines 184-195 **Vulnerability Type**: Shell command injection **Risk Level**: Medium ### Vulnerable Code ```python def start_charging(self): """Trigger charging on the vehicle""" try: subprocess.run( f'TESLA_EMAIL="{self.tesla_email}" python3 {self.tesla_skill_dir}/scripts/tesla.py charge start', shell=True, check=True, capture_output=True ) return True except Exception as e: print(f"❌ Error starting charge: {e}") return False ``` The documentation also incorrectly claims that all subprocess calls avoid shell execution: **File Location**: `SKILL.md`, lines 17-20 ```markdown **Security improvements (v1.1.0+):** - ✅ No shell injection risk: Uses argument lists instead of shell=True - ✅ Email validation: TESLA_EMAIL is validated before use - ✅ Input validation: Charge limits are validated (0-100% range) - ✅ Secure env passing: Credentials passed via environment variables, not string interpolation ``` ### Technical Analysis The `start_charging()` method constructs a command string and executes it through `shell=True`. Consequently, the system shell interprets metacharacters contained in interpolated values. The current normal call path first invokes `get_current_battery()`, which validates `TESLA_EMAIL` using a restrictive regular expression. This substantially reduces direct exploitation through the email value in the present implementation. However: 1. `start_charging()` does not enforce that validation itself. 2. The interpolated Tesla Skill path is not shell-quoted. 3. A crafted installation directory containing shell metacharacters can alter command interpretation. 4. A future caller could invoke `start_charging()` without first passing through email validation. 5. The implementation contradicts the explicit security guarantee in `SKILL.md`. The other Tesla subproce ...[truncated 1453 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions Replace the shell command with an argument list and an explicit environment: ```python def start_charging(self): """Trigger charging on the vehicle.""" try: if not self._is_valid_email(self.tesla_email): print("⚠️ Invalid TESLA_EMAIL format") return False env = os.environ.copy() env["TESLA_EMAIL"] = self.tesla_email subprocess.run( [ "python3", str(self.tesla_skill_dir / "scripts" / "tesla.py"), "charge", "start", ], env=env, shell=False, check=True, capture_output=True, text=True, ) return True except Exception as e: print(f"❌ Error starting charge: {e}") return False ``` Additional hardening should include: - Resolve and verify the Tesla Skill script path before execution. - Confirm that the resolved path remains under the expected Tesla Skill directory. - Apply validation inside every security-sensitive method rather than relying on preceding call paths. - Add tests using paths and inputs containing spaces and shell metacharacters. - Correct the security statement in `SKILL.md` until all shell-based execution has been removed. ]]>