T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/download_weights.py:108
- Finding
- Encoded Path Traversal Allows Writes Outside the ComfyUI Models Directory<![CDATA[ ## Vulnerability Details **File Location**: `scripts/download_weights.py:108-149` **Vulnerability Type**: Path traversal and arbitrary file write **Risk Level**: High ### Vulnerable Code ```python def resolve_url_dest( raw: str, base: str, overwrite: bool, default_subfolder: str | None ) -> tuple[str | None, str | None, bool]: """Return (url, dest_path, is_skip). url None = skip/ignore.""" url = raw.strip() if not url or url.startswith("#"): return (None, None, True) subfolder = default_subfolder if default_subfolder in SUBFOLDERS else "checkpoints" if " " in url: url, sub = url.strip().split(None, 1) sub = sub.strip().lower() if sub in SUBFOLDERS: subfolder = sub elif not default_subfolder or default_subfolder not in SUBFOLDERS: subfolder = infer_subfolder(url) base = os.path.expanduser(base) model_dir = os.path.join(base, "models", subfolder) os.makedirs(model_dir, exist_ok=True) path = urlparse(url).path name = path.rstrip("/").split("/")[-1] name = unquote(name) if name else "downloaded.safetensors" out_path = os.path.join(model_dir, name) if os.path.isfile(out_path) and not overwrite: return (None, out_path, True) return (url, out_path, False) ``` The resulting path is subsequently used as a file-write destination: ```python def download_one_fallback( url: str, dest_path: str, overwrite: bool ) -> tuple[str, str]: """Download one file with urllib. Returns (status, path_or_message).""" if os.path.isfile(dest_path) and not overwrite: return ("skipped", dest_path) req = urllib.request.Request(url, headers={"User-Agent": "ComfyUI-Skill/1.0"}) try: with urllib.request.urlopen(req, timeout=600) as resp: with open(dest_path, "wb") as f: while True: chunk = resp.read(1 << 20) if not chunk: break ...[truncated 2324 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Parse and decode the candidate filename before validating it. 2. Reject filenames containing `/`, `\`, NUL characters, `.` or `..` path components, or absolute paths. 3. Prefer reducing the decoded value to a strict basename and enforce an allowlist of filename characters and expected model extensions. 4. Resolve both the destination and permitted model directory before writing: ```python from pathlib import Path model_root = Path(base, "models", subfolder).resolve() decoded_name = unquote(name) if ( decoded_name in {".", ".."} or "/" in decoded_name or "\\" in decoded_name or "\x00" in decoded_name ): raise ValueError("Unsafe destination filename") destination = (model_root / decoded_name).resolve() if destination.parent != model_root: raise ValueError("Destination escapes model directory") ``` 5. Perform this validation before existence checks and before adding a destination to a `pget` manifest. 6. Download to a temporary file within the validated directory and atomically rename it after successful completion. 7. Add regression tests covering encoded traversal, encoded separators, absolute paths, Windows separators, and double-encoded input. ]]>