T09 · Insecure Skill Coding Practices
Error
- Location
- organizer.py:210
- Finding
- Unvalidated Execution Plan Allows Arbitrary Filesystem Moves<![CDATA[ ## Vulnerability Details **File Location**: `organizer.py:210-219`, `organizer.py:339-342`; execution workflow documented at `SKILL.md:39-43` **Vulnerability Type**: Unvalidated caller-controlled source and destination paths **Risk Level**: High ### Vulnerable Code ```python def execute_plan(plan: List[Dict]) -> Dict: results = {"moved": [], "errors": []} log_lines = [f"# Organize log {datetime.now().strftime('%Y-%m-%d %H:%M:%S')}", ""] for item in plan: source = Path(item["source"]) target = Path(item["final_target"]) try: target.parent.mkdir(parents=True, exist_ok=True) shutil.move(str(source), str(target)) results["moved"].append({"from": str(source), "to": str(target)}) ``` The command-line handler passes decoded, caller-controlled JSON directly to this function: ```python if args.execute: plan = json.loads(args.execute) results = execute_plan(plan) update_index(config) print(json.dumps(results, ensure_ascii=False, indent=2)) return ``` The documented workflow explicitly permits modification of destination-related plan fields before execution: ```markdown - If the user requests **changes**: adjust the plan (modify `target_subdir` and `final_target` fields accordingly), show the updated table, and ask again Pass the (possibly modified) plan JSON back to the script: ```bash python3 ~/.claude/skills/organize/organizer.py --execute '<JSON>' ``` ``` ### Technical Analysis `execute_plan()` treats the `source` and `final_target` fields in supplied JSON as trusted filesystem paths. It does not verify that: - The source is a regular file located directly inside the configured `downloads_dir`. - The destination is contained within the configured `target_root`. - The source and destination are free of symbolic-link redirection. - The destination does not already exist at execution time. - The plan was produced by the current invocation of `--scan`. - The d ...[truncated 2226 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Resolve and validate every path immediately before the move: ```python downloads_root = Path(config["downloads_dir"]).resolve() target_root = Path(config["target_root"]).resolve() source = Path(item["source"]).resolve(strict=True) target = Path(item["final_target"]).resolve(strict=False) if source.parent != downloads_root: raise ValueError("Source must be a direct child of downloads_dir") if not target.is_relative_to(target_root): raise ValueError("Destination must remain under target_root") if not source.is_file() or source.is_symlink(): raise ValueError("Source must be a non-symlink regular file") ``` 2. Pass the loaded configuration into `execute_plan()` so it can enforce source and destination boundaries rather than trusting the caller. 3. Do not accept complete source and destination paths from the caller. Return opaque identifiers from `--scan`, store the associated plan internally, and recompute paths during execution. 4. Permit user adjustments only through a validated relative target subdirectory. Reject absolute paths, `..` components, empty components, and paths escaping `target_root`. 5. Re-run conflict resolution immediately before each move to mitigate scan-to-execution races. Use move semantics that explicitly refuse to overwrite an existing destination. 6. Reject unknown or malformed plan fields and enforce a strict JSON schema. 7. Consider binding each plan to the current scan using a short-lived nonce or authenticated digest so arbitrary plans cannot be submitted directly. 8. Perform symlink-aware checks and ensure every relevant parent directory remains under the validated root at the moment of use. ]]>