T09 · Insecure Skill Coding Practices
Warning
- Location
scripts/skill_review.py:458- Finding
Weak VirusTotal URL Validation Permits Arbitrary Browser Navigation
- Content
View full analysis
str | None: """Get VirusTotal analysis for a file. Prefers the VT v3 API (clean, structured) when api_key is available. Falls back to Playwright scraping of the VT GUI page otherwise. Results are cached by file hash — repeated queries for the same version return instantly without API calls. """ if not vt_url or "virustotal.com" not in vt_url: return None file_hash = _hash_from_vt_url(vt_url) if not file_hash: return None # ... try: page.goto(vt_url, wait_until="domcontentloaded", timeout=30000) ``` ### Technical Analysis The function treats a URL as trusted whenever the literal substring `virustotal.com` occurs anywhere in the URL. This does not verify the parsed hostname. For example, all of the following can satisfy the substring check while resolving to a host not controlled by VirusTotal: ```text https://virustotal.com.attacker.example/gui/file/<64-character-hash> https://attacker.example/virustotal.com/gui/file/<64-character-hash> http://virustotal.com.attacker.example/gui/file/<64-character-hash> ``` The URL is obtained from remotely rendered ClawHub page content and is subsequently passed to Playwright's `page.goto`. An attacker able to manipulate the VirusTotal report link can therefore cause the browser to navigate to an attacker-selected endpoint. The hash extraction requirement does not prevent exploitation because an attacker can include a valid-looking 64-character hexadecimal value in the path. Redirect destinations are not validated either. Even an initially accepted URL could redirect the browser to another host. ### Attack Path 1. An attacker c ...[truncated 1393 chars]- Remediation
View remediation
str | None: try: parsed = urlsplit(value) except ValueError: return None if parsed.scheme != "https": return None if parsed.hostname not in {"virustotal.com", "www.virustotal.com"}: return None if parsed.username is not None or parsed.password is not None: return None if parsed.port not in (None, 443): return None if not re.fullmatch(r"/gui/file/[0-9a-fA-F]{64}/?", parsed.path): return None return value ``` Additional hardening should include: 1. Reject all non-HTTPS URLs. 2. Compare the parsed hostname exactly rather than using substring matching. 3. Reject embedded credentials, unexpected ports, fragments, and unexpected paths. 4. Validate every redirect destination and abort navigation if it leaves the approved hostname set. 5. Use Playwright request routing to block requests to loopback, private, link-local, and other non-public address ranges. 6. Prefer constructing the canonical VirusTotal URL from the extracted hash rather than navigating to the remote link verbatim: ```python vt_url = f"https://www.virustotal.com/gui/file/{file_hash}" ``` 7. Consider using a separate disposable browser context with JavaScript disabled where feasible for the scraping fallback. ]]>