T09 · Insecure Skill Coding Practices
- Location
scripts/delete_recovery.py:735- Finding
Arbitrary Directory Deletion Through Unvalidated Backup Folder Argument
- Content
View full analysis
Vulnerability Details
File Location:
scripts/delete_recovery.py:735-743
Vulnerability Type: Path traversal and unrestricted recursive directory deletion
Risk Level: HighVulnerable Code
python def delete_backup_folder(backup_folder): """Delete a specific backup folder manually; also removes its manifest entries.""" backup_dir = BACKUP_ROOT / backup_folder if backup_dir.exists(): shutil.rmtree(backup_dir) # v0.6.0: remove all manifest entries for this folder # Use tombstone approach for large manifest removed = _remove_manifest_entry(backup_folder, None)The value of
backup_folderoriginates from the command-line argument supplied to thedelete_backupaction. It is joined toBACKUP_ROOTwithout validation before being passed toshutil.rmtree().Technical Analysis
The implementation assumes that
backup_folderis a simple backup identifier, such as a timestamp, but does not enforce that assumption. It does not reject:- Absolute paths
- Parent-directory components such as
.. - Path separators
- Symbolic links resolving outside the backup root
- Non-backup directory names
With
pathlib, joining a base path with an absolute path discards the base path. For example,BACKUP_ROOT / "/tmp/target"evaluates to/tmp/target. A relative value containing sufficient../components can also resolve outsideBACKUP_ROOT.The restore and verification code uses the same unsafe construction at
scripts/delete_recovery.py:589-593andscripts/delete_recovery.py:674-678. Those paths create additional out-of-root file access opportunities, although recursive deletion is the most severe confirmed sink.Attack Path
- An attacker or untrusted caller gains the ability to invoke the Skill command with controlled arguments.
- The caller supplies an absolute directory or a traversal path as the backup folder:
bash
...[truncated 1218 chars]
- Remediation
View remediation
Remediation Suggestions
-
Accept only strict backup identifiers:
python import re if not re.fullmatch(r"\d{12}|\d{14}", backup_folder): raise ValueError("Invalid backup folder identifier") -
Reject absolute paths, separators,
.and..components before constructing the target. -
Resolve the candidate and verify containment using path semantics rather than string-prefix checks:
python root = BACKUP_ROOT.resolve() candidate = (root / backup_folder).resolve(strict=True) if candidate == root or not candidate.is_relative_to(root): raise SafePathError("Backup folder is outside the backup root") -
Require the target to be a direct child of
BACKUP_ROOTand a real directory:python if candidate.parent != root or not candidate.is_dir() or candidate.is_symlink(): raise SafePathError("Invalid backup directory") -
Apply equivalent containment validation to
backup_folderandsafe_namein restore and verification operations. -
Where supported, use descriptor-relative filesystem operations and avoid following symbolic links to reduce time-of-check/time-of-use race exposure.
-
Add regression tests covering absolute paths, nested traversal, symbolic links, malformed timestamp names, and attempts to delete
BACKUP_ROOTitself.
-
