T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/monitor.py:24
- Finding
- Arbitrary Command Execution Through Shell Injection<![CDATA[ ## Vulnerability Details **File Location**: `scripts/monitor.py:24-44`, with exploitable command construction at `scripts/monitor.py:137-143` and `scripts/monitor.py:196-201` **Vulnerability Type**: OS command injection through user-controlled path interpolation **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 ``` The continuous-monitoring path constructs commands using the user-controlled data directory: ```python 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...") 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-time execution path uses the same unsafe pattern: ```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 accepted as an unrestricted string and converted into a `Path`. Derived paths such as `markets_file` and `arbs_file` are then inserted directly into shell command strings. Because `run_command()` invokes `subproce ...[truncated 1881 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions - Remove `shell=True` entirely. - Pass commands as argument arrays so paths and numeric values cannot be interpreted as shell syntax. - Use `sys.executable` instead of relying on the `python3` command resolved from `PATH`. - Resolve and validate the data directory before using it. - Restrict output to an approved directory if arbitrary output locations are not required. - Check each subprocess result before proceeding to load generated files. A safer implementation is: ```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("Child process timed out", file=sys.stderr) return None ``` Commands should then be constructed as lists: ```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), ] ``` ]]>