T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/undo_manager.py:113
- Finding
- Arbitrary File Move and Overwrite Through Forged Undo Records<![CDATA[ ## Vulnerability Details **File Location**: `scripts/undo_manager.py`, lines 113–117, 127–132, 146–152, 167–175, and 224–229 **Vulnerability Type**: Unvalidated path use in privileged filesystem operations **Risk Level**: High ### Vulnerable Code Lines 113–117 allow a path from an operation record to be moved without verifying that it belongs to the card repository: ```python file_path = Path(details.get("path", "")) if file_path.exists(): trash_path = TRASH_DIR / file_path.name shutil.move(str(file_path), str(trash_path)) ``` Lines 127–132 restore a file to an arbitrary destination supplied by the operation record: ```python original_path = Path(details.get("original_path", "")) trash_path = Path(details.get("trash_path", "")) if trash_path.exists(): original_path.parent.mkdir(parents=True, exist_ok=True) shutil.move(str(trash_path), str(original_path)) ``` Lines 146–152 repeat the same issue for batch restoration: ```python original_path = Path(item.get("original_path", "")) trash_path = Path(item.get("trash_path", "")) if trash_path.exists(): try: original_path.parent.mkdir(parents=True, exist_ok=True) shutil.move(str(trash_path), str(original_path)) ``` Lines 167–175 overwrite an existing file with content taken directly from the history record: ```python file_path = Path(details.get("path", "")) backup_content = details.get("backup_content", "") if file_path.exists() and backup_content: current_content = file_path.read_text(encoding='utf-8') file_path.write_text(backup_content, encoding='utf-8') ``` Lines 224–229 expose a command that accepts arbitrary JSON and stores it as an operation record: ```python elif args.command == "log": details = json.loads(args.details) if args.details else {} op_id = log_operation(args.type, details) print(json.dumps({"success": True, "op_id": op_id}, ensure_ascii=False)) ``` ### Technical Analysis The undo subsystem treats data stored in `.syst ...[truncated 2958 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. **Remove or restrict the `log` command** - Do not expose the internal history-writing interface as an unrestricted CLI command. - If it must remain available, enforce a fixed schema for each operation type and reject unknown fields or operation types. - Do not treat possession of a valid JSON object as authorization to perform filesystem operations. 2. **Enforce path containment** - Resolve every source and destination with `Path.resolve()`. - Require card paths and restore destinations to remain under `CARDS_DIR.resolve()`. - Require restore sources to remain under `TRASH_DIR.resolve()`. - Reject absolute or relative paths that resolve outside these approved roots. ```python def require_within(path: Path, root: Path) -> Path: resolved_path = path.expanduser().resolve(strict=False) resolved_root = root.expanduser().resolve(strict=True) if not resolved_path.is_relative_to(resolved_root): raise ValueError(f"Path is outside the approved root: {resolved_path}") return resolved_path ``` 3. **Validate paths according to operation type** - For `create` undo records, require the source to be an approved Markdown card under a recognized card directory. - For `delete` and `batch_delete`, require the source to be inside `TRASH_DIR` and the destination to be inside an approved note directory. - For `update`, require the target to be an approved Markdown card under `CARDS_DIR`. 4. **Reject symbolic-link attacks** - Reject symlink sources and destinations. - Validate resolved paths immediately before each move or write to reduce time-of-check/time-of-use exposure. - Use filesystem operations that avoid following symlinks where supported. 5. **Avoid trusting serialized paths** - Store an immutable card identifier and operation identifier rather than arbitrary absolute paths. - Reconstruct approved paths from trusted application state when performing an undo. - Co ...[truncated 672 chars]