T09 · Insecure Skill Coding Practices
Error
- Location
scripts/register_candidate.py:69- Finding
Symlink-Following Cleanup Can Delete Files Outside the Job Directory
- Content
View full analysis
None: if not directory.is_dir(): return for path in directory.iterdir(): if path.is_dir() and not path.is_symlink(): shutil.rmtree(path) else: path.unlink() ``` The helper is invoked on predictable directories inside a supplied job directory: ```python for directory in ( job_dir / "renders" / "source", job_dir / "renders" / "candidate", job_dir / "comparisons", ): _clear_directory_contents(directory) ``` ### Technical Analysis The cleanup function checks `directory.is_dir()` but does not reject a top-level cleanup directory that is itself a symbolic link. In Python, `Path.is_dir()` follows symbolic links. Likewise, `directory.iterdir()` enumerates the contents of the symlink target. Consequently, if `comparisons`, `renders/source`, or `renders/candidate` is replaced with a symbolic link to another writable directory, the function operates on the contents of that external directory: - External regular files and symbolic links are deleted with `unlink()`. - External real subdirectories are recursively deleted with `shutil.rmtree()`. - No resolved-path containment check verifies that the cleanup target remains beneath `job_dir`. - No ownership or trusted-directory validation is performed before destructive operations. The child-level `not path.is_symlink()` check prevents recursively following a child directory symlink, but it does not protect against the top-level cleanup directory being a symlink. ### Attack Path 1. An attacker supplies a pre-existing job directory or gains the ability to modify a job directory before candidate registration. 2. The attacker repla ...[truncated 1281 chars]- Remediation
View remediation
None: job_root = job_dir.resolve() if directory.is_symlink(): raise SkillError(f"Refusing to clean a symbolic link: {directory}") resolved = directory.resolve(strict=True) try: resolved.relative_to(job_root) except ValueError as exc: raise SkillError( f"Cleanup directory is outside the job directory: {resolved}" ) from exc if resolved == job_root or not resolved.is_dir(): raise SkillError(f"Invalid cleanup directory: {resolved}") for path in resolved.iterdir(): if path.is_symlink(): path.unlink() elif path.is_dir(): shutil.rmtree(path) else: path.unlink() ``` 2. Validate every parent component with `lstat()` or equivalent logic so that no parent directory in the cleanup path is a symlink. 3. Restrict cleanup to directories created and recorded by the application rather than accepting arbitrary existing directory structures. 4. Consider deleting and safely recreating the managed directory itself, after proving that its resolved parent is inside the job root. 5. Reduce time-of-check/time-of-use exposure by using directory-file-descriptor-based operations where supported. On Unix-like systems, relative operations through trusted directory descriptors can provide stronger resistance to symlink races. 6. Add regression tests covering: - A top-level `comparisons` symlink to an external directory. - Symlinks in `renders/source` and `renders/candidate`. - Symlinked parent components. - Replacement of a validated directory with a symlink during cleanup. - Verification that files outside the job root are never modified. ]]>