T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/photo_geolocator.py:320
- Finding
- Report CSV Path Traversal and Symlink Escape Permit Modification of Files Outside the Photo Directory<![CDATA[ ## Vulnerability Details **File Location**: `scripts/photo_geolocator.py:320-341` **Vulnerability Type**: Path traversal and improper filesystem boundary validation **Risk Level**: High ### Vulnerable Code ```python for r in actionable: src = photo_dir / r["filename"] if not src.is_file() or not is_jpg(src): failures.append(f"missing or non-JPG: {r['filename']}") continue try: lat = float(r["inferred_lat"]) lon = float(r["inferred_lon"]) except ValueError: failures.append(f"bad coords for {r['filename']}") continue try: shutil.copy2(src, backup / r["filename"]) except Exception as e: failures.append(f"backup failed for {r['filename']}: {e}") continue description = f"{r['city']}, {r['country']}" user_comment = ( f"confidence={r['confidence']}; landmark={r['landmark']}; source=geo-tag-photos" ) try: write_location(src, lat=lat, lon=lon, description=description, user_comment=user_comment) ``` The extension validation used by this path is also insufficient: ```python def is_jpg(path: Path) -> bool: return path.suffix.lower() in (".jpg", ".jpeg") ``` ### Technical Analysis The `write` command treats the `filename` column of the supplied report CSV as a trusted relative filename. It joins the value directly to both `photo_dir` and `backup` without validating that the resolved paths remain inside those directories. A filename containing `../` can escape the selected directories. In addition, `Path.is_file()` follows symbolic links, while `is_jpg()` only validates the textual suffix. A symlink named with a `.jpg` extension inside the photo directory can therefore refer to a JPG outside that directory and pass both checks. The required backup does not eliminate the vulnerability. `shutil.copy2()` also receives a destination derived from the unvalidated CSV filename, allowing that destination to escap ...[truncated 2149 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Require every report filename to be a simple basename: ```python raw_name = r.get("filename", "") candidate = Path(raw_name) if ( not raw_name or candidate.is_absolute() or candidate.name != raw_name or raw_name in {".", ".."} ): failures.append(f"unsafe filename: {raw_name!r}") continue ``` 2. Resolve the source and verify that it remains beneath the source directory: ```python source_root = photo_dir.resolve(strict=True) src = (source_root / raw_name).resolve(strict=True) try: src.relative_to(source_root) except ValueError: failures.append(f"source escapes photo directory: {raw_name!r}") continue ``` 3. Reject symbolic links unless following them is an explicit supported feature: ```python unresolved_src = source_root / raw_name if unresolved_src.is_symlink(): failures.append(f"symbolic links are not allowed: {raw_name!r}") continue ``` 4. Apply an independent containment check to the backup destination: ```python backup_root = backup.resolve() dst = (backup_root / raw_name).resolve() try: dst.relative_to(backup_root) except ValueError: failures.append(f"backup destination escapes backup directory: {raw_name!r}") continue ``` 5. Avoid overwriting existing backup files by opening destinations with exclusive creation semantics or explicitly rejecting `dst.exists()` before copying. 6. Validate that every actionable row corresponds to an actual filename discovered during a fresh scan of the selected photo directory. Do not rely solely on an earlier report. 7. Add regression tests for: - `../outside.jpg` - Absolute Unix and Windows paths - Nested path separators - Symlinks to files outside the source directory - Backup destination traversal - Existing backup destination files - Report replacement or modification between review and write ] ...[truncated 2 chars]