T09 · Insecure Skill Coding Practices
Warning
- Location
- video_organizer.py:124
- Finding
- Unvalidated Rename and Backup Paths Allow File Moves Outside Approved Directories<![CDATA[ ## Vulnerability Details **File Location**: `video_organizer.py:124-127` and `video_organizer.py:172-179` **Vulnerability Type**: Path traversal and untrusted filesystem path handling **Risk Level**: Medium ### Vulnerable Code ```python if pattern: new_name = self.generate_name(video_path, pattern, i) target_path = self.input_dir / new_name elif regex: new_name = self.apply_regex(video_path.name, regex) target_path = self.input_dir / new_name ``` The undo operation also trusts paths loaded directly from the backup file: ```python reverse_mappings = {v: k for k, v in backup.items()} count = 0 for new_str, old_str in reverse_mappings.items(): new_path = Path(new_str) old_path = Path(old_str) if new_path.exists() and not old_path.exists(): new_path.rename(old_path) ``` ### Technical Analysis Names generated from the user-controlled `--pattern` or `--regex` arguments are joined directly to `self.input_dir` without verifying that the resulting path remains inside that directory. A generated name can contain `../` traversal components or be an absolute path. With `pathlib`, joining a base path to an absolute path causes the absolute path to replace the base path. Consequently, the rename destination is not reliably confined to the selected video directory. The undo implementation creates an additional attack surface. `load_backup()` parses `.video-organizer-backup.json`, after which `undo()` treats its keys and values as trusted filesystem paths. No schema, ownership, file type, extension, canonical path, or directory-containment checks are performed. Anyone able to alter that JSON file can specify an existing accessible source file and an absent destination path. The destination must not already exist, and all operations remain subject to the operating-system privileges of the process. The code does not create elevated privileges. ### Attack Path #### Rename traversal 1. An operator invokes `rename` with a ...[truncated 1821 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Restrict generated names to a single filename component: - Reject absolute paths. - Reject `.` and `..`. - Reject directory separators. - Require `Path(new_name).name == new_name`. 2. Canonicalize and validate every destination before creating the mapping: ```python base = self.input_dir.resolve() candidate = (base / new_name).resolve() if not candidate.is_relative_to(base): raise ValueError("Generated filename escapes the input directory") ``` 3. Account for symbolic links by resolving the approved root and candidate path immediately before the filesystem mutation. 4. Validate the complete operation plan before moving any file. Abort the entire operation if any mapping is invalid, duplicated, outside an approved root, or conflicts with an existing path. 5. Treat backup JSON as untrusted input: - Require a dictionary with string keys and values. - Reject unknown fields and malformed mappings. - Define explicit source and destination roots for each operation type. - Resolve every loaded path and enforce containment within those roots. - Confirm that source paths are regular files of an expected type. - Reject mappings involving symlinks or unexpected extensions where appropriate. 6. Create backup files with restrictive permissions and avoid following symlinks when opening or replacing them. Write updates atomically through a securely created temporary file in the same directory. 7. Require an explicit confirmation for undo operations that lists the validated source and destination paths before any mutation. 8. Add automated tests covering absolute paths, `../` traversal, nested names, symlink escapes, malformed backup data, duplicate destinations, and mappings outside approved directories. ]]>