T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/monitor.py:30
- Finding
- OS Command Injection Through User-Controlled Data Directory<![CDATA[ ## Vulnerability Details **File Location**: `scripts/monitor.py:25-47`, `scripts/monitor.py:137-142`, and `scripts/monitor.py:198-199` **Vulnerability Type**: OS command injection through unsafe shell command construction **Risk Level**: High ### Vulnerable Code ```python def run_command(cmd, description=""): """Run a shell command and return the output.""" if description: print(f"[{datetime.now().strftime('%H:%M:%S')}] {description}", file=sys.stderr) try: result = subprocess.run( cmd, shell=True, capture_output=True, text=True, timeout=60 ) if result.returncode != 0: print(f"Error: {result.stderr}", file=sys.stderr) return None return result.stdout except subprocess.TimeoutExpired: print(f"Timeout running: {cmd}", file=sys.stderr) return None except Exception as e: print(f"Exception: {e}", file=sys.stderr) return None ``` The continuous-monitoring path constructs commands using the user-controlled data directory: ```python # Step 1: Fetch markets script_dir = Path(__file__).parent fetch_cmd = f"python3 {script_dir}/fetch_markets.py --output {markets_file} --min-volume 50000" run_command(fetch_cmd, "Fetching markets...") # Step 2: Detect arbitrage detect_cmd = f"python3 {script_dir}/detect_arbitrage.py {markets_file} --min-edge {min_edge} --output {arbs_file}" run_command(detect_cmd, "Detecting arbitrage...") ``` The one-shot path is affected as well: ```python run_command(f"python3 {script_dir}/fetch_markets.py --output {markets_file}") run_command(f"python3 {script_dir}/detect_arbitrage.py {markets_file} --min-edge {args.min_edge} --output {arbs_file}") ``` ### Technical Analysis The `--data-dir` command-line argument is converted to a `Path`, incorporated into `markets_file` and `arbs_file`, and then interpolated directly into shell command str ...[truncated 1991 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions Eliminate shell interpretation and pass each subprocess argument as a separate list element: ```python def run_command(cmd, description=""): if description: print(f"[{datetime.now().strftime('%H:%M:%S')}] {description}", file=sys.stderr) try: result = subprocess.run( cmd, shell=False, capture_output=True, text=True, timeout=60, check=False ) if result.returncode != 0: print(f"Error: {result.stderr}", file=sys.stderr) return None return result.stdout except subprocess.TimeoutExpired: print("Subprocess timed out", file=sys.stderr) return None ``` Construct commands without string interpolation: ```python fetch_cmd = [ sys.executable, str(script_dir / "fetch_markets.py"), "--output", str(markets_file), "--min-volume", "50000", ] detect_cmd = [ sys.executable, str(script_dir / "detect_arbitrage.py"), str(markets_file), "--min-edge", str(min_edge), "--output", str(arbs_file), ] ``` Additional hardening should include: 1. Validate that `--data-dir` resolves to an approved directory or permitted base path. 2. Reject control characters and unexpected path forms. 3. Use `sys.executable` rather than relying on a `python3` executable resolved through `PATH`. 4. Apply the same argument-array construction in both continuous and one-shot modes. 5. Add tests using directory names containing spaces and shell metacharacters to verify that they are handled only as literal path data. ]]>