T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/init_planner.py:27
- Finding
- Arbitrary Recursive Directory Deletion Through the Force Option<![CDATA[ ## Vulnerability Details **File Location**: `scripts/init_planner.py`, lines 27–33 and 44–60 **Vulnerability Type**: Unrestricted recursive deletion of a user-selected filesystem path **Risk Level**: High ### Vulnerable Code ```python ap.add_argument( "--target", default="./planner", help="Target directory to create (default: ./planner)", ) ap.add_argument( "--force", action="store_true", help="Overwrite the target directory if it already exists (DANGEROUS).", ) ``` ```python target_dir = Path(args.target).expanduser().resolve() if target_dir.exists(): if any(target_dir.iterdir()): if not args.force: die( "Refusing to overwrite existing non-empty directory:\n" f" {target_dir}\n\n" "If you really want to replace it, re-run with --force (this will delete it first)." ) shutil.rmtree(target_dir) else: # copytree() requires the destination to not exist shutil.rmtree(target_dir) shutil.copytree(template_dir, target_dir) ``` ### Technical Analysis The `--target` argument accepts an arbitrary path, expands user-directory syntax, and resolves it to an absolute path. If `--force` is supplied, the script passes that path directly to `shutil.rmtree()` without enforcing any filesystem boundary. The script does not: - Require the target to be inside the current workspace. - Reject dangerous locations such as the user's home directory, repository root, or another unrelated data directory. - Require a planner-specific sentinel file before deletion. - Request interactive confirmation showing the resolved path. - Create a backup before recursively deleting the target. - Verify that the target is the expected planner directory. The warning in the argument description does not prevent accidental or attacker-influenced invocation. This is an unsafe destructive-operation design rather than a privilege-escalation flaw: deletion ...[truncated 1266 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions Implement layered safeguards around destructive replacement: 1. Restrict the resolved target to an explicitly approved workspace root using `Path.relative_to()` or an equivalent containment check. 2. Reject dangerous targets, including: - Filesystem root. - The user's home directory. - The current workspace root. - The Skill installation directory. - Any directory outside the approved workspace. 3. Require a planner-specific sentinel file before replacing an existing non-empty directory. 4. Separate initialization from replacement. Prefer refusing all non-empty targets and provide a dedicated migration or reset command. 5. If forced replacement remains necessary, require interactive confirmation that reproduces the fully resolved path. 6. Move the existing planner to a timestamped backup rather than deleting it immediately. 7. Reject symlink targets and verify relevant path components before deletion. 8. Consider requiring both a narrowly named flag such as `--replace-existing-planner` and a confirmation token rather than a generic `--force`. Example containment control: ```python workspace = Path.cwd().resolve() target_dir = Path(args.target).expanduser().resolve() try: target_dir.relative_to(workspace) except ValueError: die(f"Refusing target outside workspace: {target_dir}") dangerous = {Path("/").resolve(), Path.home().resolve(), workspace} if target_dir in dangerous: die(f"Refusing dangerous target: {target_dir}") sentinel = target_dir / "config.toml" if target_dir.exists() and any(target_dir.iterdir()) and not sentinel.is_file(): die("Refusing to replace a directory that is not an existing planner.") ``` ]]>