T09 · Insecure Skill Coding Practices
Error
- Location
scripts/wm_report.py:5393- Finding
Path Traversal Enables Recursive Deletion Outside the Skill Cache
- Content
View full analysis
Path: return cache_root() / sha ``` ```python def materialize_tables(sha12: str, *, out_name: str | None, force: bool) -> dict: d = entry_dir(sha12) ... result_name = out_name or f"result-{ts}" result_dir = d / result_name ... if result_dir.exists() and force: ... shutil.rmtree(result_dir) ``` ```python elif cargs.id: d = entry_dir(cargs.id) if d.is_dir(): shutil.rmtree(d) ``` ### Technical Analysis The `entry_dir()` function directly joins an unvalidated cache identifier to the configured cache root. It does not require the identifier to be a 12-character hexadecimal digest, reject absolute paths, reject `..` components, or verify that the resolved path remains under the cache root. The `cache clean --id` command passes its user-controlled identifier to this function and then recursively deletes the resulting directory with `shutil.rmtree()`. A second instance occurs in `materialize_tables()`. The user-controlled `--out` value becomes `result_name`, which is joined to the cache entry directory without validation. When `--force` is supplied, the resulting path is recursively deleted if it exists. `pathlib` does not provide containment enforcement when using `/`. A value containing traversal components can resolve outside the intended cache. An absolute right-hand operand can also replace the preceding path entirely. ### Attack Path #### Cache deletion path 1. An attacker influences the argument passed to `cache clean --id`. 2. The attacker supplies an escaping identifier, such as `../../target-directory`, or an absolute directory path. 3. `entry_dir()` constructs the path without vali ...[truncated 1315 chars]- Remediation
View remediation
str: if not CACHE_ID_RE.fullmatch(value): raise SystemExit("Invalid cache ID") return value ``` 2. Restrict result names to safe basenames: ```python RESULT_NAME_RE = re.compile(r"^result-[A-Za-z0-9_.-]+$") def validate_result_name(value: str) -> str: if not RESULT_NAME_RE.fullmatch(value): raise SystemExit("Invalid result name") return value ``` 3. Enforce path containment after canonicalization: ```python def contained_path(root: Path, child: str) -> Path: resolved_root = root.expanduser().resolve() candidate = (resolved_root / child).resolve() try: candidate.relative_to(resolved_root) except ValueError: raise SystemExit("Path escapes the permitted root") return candidate ``` 4. Apply the containment check immediately before every `shutil.rmtree()` operation, rather than relying only on earlier validation. 5. Reject absolute paths, path separators, empty identifiers, `.` components, and `..` components. 6. Add regression tests covering absolute paths, nested traversal, symbolic links, and traversal supplied through both `--id` and `--out`. 7. Consider deleting only known cache layouts containing an expected marker file, while still retaining canonical containment checks. ]]>