T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/daily_popular_report.py:14
- Finding
- Shell Command Injection Through Unescaped Browser Command Arguments<![CDATA[ ## Vulnerability Details **File Location**: `scripts/daily_popular_report.py:14-19, 25-35` **Vulnerability Type**: OS command injection caused by unsafe shell invocation **Risk Level**: High ### Complete Code Snippet ```python def run_cmd(cmd, timeout=30): """Run shell command and return output.""" result = subprocess.run( cmd, shell=True, capture_output=True, text=True, timeout=timeout ) return result.stdout, result.stderr, result.returncode def browser_open(url): """Open URL in openclaw browser.""" cmd = f'openclaw browser --browser-profile openclaw open "{url}"' out, err, rc = run_cmd(cmd) return rc == 0 def browser_snapshot(save_path=None): """Get snapshot of current page. Optionally save to file.""" if save_path: cmd = f'openclaw browser --browser-profile openclaw snapshot > "{save_path}" 2>&1' out, err, rc = run_cmd(cmd, timeout=20) ``` ### Technical Analysis `run_cmd()` passes a dynamically constructed string to `subprocess.run()` with `shell=True`. Both `browser_open()` and `browser_snapshot()` interpolate function arguments directly into quoted shell commands without shell escaping. Quotation marks alone do not provide a security boundary. An argument containing a quote followed by shell syntax can terminate the quoted value and append another command. Redirection syntax is also deliberately interpreted by the shell in `browser_snapshot()`. The current `main()` function primarily supplies fixed URLs and stock-derived URLs containing six-digit tickers, which reduces exposure through the default execution path. However, the helper functions themselves accept unrestricted strings and are callable by imported code or future integrations. Any path that forwards untrusted input to these functions creates a command-execution vulnerability. ### Attack Path 1. An attacker influences a URL or snapshot output path passed to `browser_open()` or `bro ...[truncated 1289 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Remove `shell=True` and pass each command as a separate argument: ```python def run_cmd(args, timeout=30): return subprocess.run( args, shell=False, capture_output=True, text=True, timeout=timeout, check=False ) def browser_open(url): result = run_cmd([ "openclaw", "browser", "--browser-profile", "openclaw", "open", url, ]) return result.returncode == 0 ``` 2. Do not use shell redirection to save snapshots. Capture standard output and write it through Python: ```python from pathlib import Path def browser_snapshot(save_path=None): result = run_cmd([ "openclaw", "browser", "--browser-profile", "openclaw", "snapshot", ], timeout=20) if result.returncode != 0: return None if save_path: destination = Path(save_path).resolve() destination.write_text( result.stdout + result.stderr, encoding="utf-8" ) return result.stdout ``` 3. Restrict output paths to an explicitly approved directory and reject paths that escape it after canonicalization. 4. Validate stock tickers with a full-match expression such as `r"\d{6}"` before constructing URLs. 5. Restrict browser destinations to HTTPS and an allowlist of expected Naver hostnames. 6. Add tests containing quotation marks, shell metacharacters, redirection operators, and path traversal sequences to verify that arguments cannot become executable shell syntax. ]]>