T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/git_hooks.py:496
- Finding
- Arbitrary File Deletion Through Unvalidated Hook Path<![CDATA[ ## Vulnerability Details **File Location**: `scripts/git_hooks.py`, lines 496–505 **Vulnerability Type**: Path traversal and unrestricted file deletion **Risk Level**: High ### Complete Code Snippet ```python def remove_hook(repo_dir, hook_type): """Remove an installed hook.""" hook_path = os.path.join(repo_dir, ".git", "hooks", hook_type) if os.path.exists(hook_path): os.remove(hook_path) print(f"✓ Removed {hook_type} hook") return True else: print(f"No {hook_type} hook found") return False ``` ### Technical Analysis The `remove_hook` function uses the command-line-controlled `hook_type` value directly when constructing the deletion target. It does not restrict the value to supported Git hook names, reject absolute paths, reject path separators, or verify the resolved path remains inside `.git/hooks`. Python's `os.path.join()` does not provide containment enforcement. If `hook_type` is an absolute path, the preceding repository and hooks path components are discarded. A relative value containing `../` components can similarly traverse outside the hooks directory after filesystem path resolution. Installation validates hook types against `HOOK_TEMPLATES`, but the removal path has no equivalent validation. ### Attack Path 1. An attacker influences the arguments supplied to the `remove` command, or convinces a user or automated agent to invoke it with a crafted hook type. 2. The attacker supplies an absolute path or traversal path, for example: ```text remove /home/user/important-file ``` or: ```text remove ../../../../target-file ``` 3. `os.path.join()` produces a path outside the intended `.git/hooks` directory. 4. If the path exists, `os.remove()` deletes it without confirmation or containment validation. ### Impact Assessment Exploitation permits deletion of any file writable by the operating-sys ...[truncated 389 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions - Restrict removal to an explicit allowlist of supported Git hook names. - Reject absolute paths, path separators, `.` components, and `..` components. - Resolve both the hooks directory and target with `os.path.realpath()`. - Verify with `os.path.commonpath()` that the target remains inside the hooks directory. - Require the resolved target's parent to be exactly the hooks directory if nested paths are unnecessary. - Consider refusing to follow symbolic links or using directory-relative file operations with appropriate platform safeguards. - Add tests covering absolute paths, traversal sequences, symbolic links, and valid hook names. Example hardening pattern: ```python allowed_hooks = set(HOOK_TEMPLATES) if hook_type not in allowed_hooks: raise ValueError("Unsupported hook type") hooks_dir = os.path.realpath(os.path.join(repo_dir, ".git", "hooks")) hook_path = os.path.realpath(os.path.join(hooks_dir, hook_type)) if os.path.dirname(hook_path) != hooks_dir: raise ValueError("Hook path escapes the hooks directory") ``` ]]>