T09 · Insecure Skill Coding Practices
Warning
- Location
scripts/check_hh_submission_bundle.py:20- Finding
Artifact path validation permits repository-root escape
- Content
View full analysis
bool: return path_text.startswith("/") or path_text.startswith("~") or ":\\" in path_text or path_text.startswith("file://") ``` ```python for field in ARTIFACT_FIELDS: artifact_path = str(artifacts.get(field, "")).strip() if not artifact_path: continue if is_absolute_or_private(artifact_path): add_finding(findings, "error", "absolute-artifact-path", f"{field} uses an absolute or private path.", step_id) continue candidate = repo_root / artifact_path if not candidate.exists(): add_finding(findings, "warning", "missing-artifact", f"{field} path does not exist under repo root: {artifact_path}", step_id) ``` ### Technical Analysis The validator rejects several obvious absolute-path forms but does not canonicalize the resulting artifact path or verify that it remains within `repo_root`. A path such as `../../private/file.txt` is not considered absolute or private by `is_absolute_or_private()`. When joined with `repo_root`, it can resolve to a file outside the intended bundle directory. The same issue applies to symlinks inside the repository that resolve to external locations. Windows drive-qualified paths using forward slashes, such as `C:/private/file.txt`, are also not detected by the check for `":\\"`. The existence test therefore does not establish the property claimed by its warning message: that the artifact exists “under repo root.” It only establishes that the constructed path exists after operating-system path resolution. ### Attack Path 1. An attacker or untrusted bundle producer supplies a manifest containing an artifact path such as: ```json { "artifacts": { "screenshot": "../../ ...[truncated 1316 chars]- Remediation
View remediation
Path: raw = Path(path_text) if raw.is_absolute() or raw.drive: raise ValueError("Artifact path must be relative") root = repo_root.resolve() candidate = (root / raw).resolve(strict=False) try: candidate.relative_to(root) except ValueError as exc: raise ValueError("Artifact path escapes repository root") from exc return candidate ``` Additional hardening should include: 1. Reject paths containing parent traversal components such as `..`, even when canonical containment would otherwise catch them. 2. Use `Path.is_absolute()` and inspect `Path.drive` instead of relying on string-prefix checks. 3. Resolve symlinks and reject candidates whose resolved targets are outside `repo_root`. 4. Require artifacts to be regular files rather than directories or special files: ```python if not candidate.is_file(): # Emit an error. ``` 5. Treat containment violations as errors, not warnings. 6. Add regression tests for POSIX traversal, Windows path forms, mixed separators, URL-like paths, and escaping symlinks. ]]>