T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/delete_personality.py:43
- Finding
- Directory Traversal Enables Recursive Deletion Outside the Personality Store<![CDATA[ ## Vulnerability Details **File Location**: `scripts/delete_personality.py`, lines 43-48 and 72-73 **Vulnerability Type**: Unrestricted path traversal leading to recursive directory deletion **Risk Level**: High ### Vulnerable Code ```python # Verify personality exists if not personality_exists(personalities_dir, personality_name): return { "status": "error", "message": f"Personality '{personality_name}' not found.", "code": "personality_not_found" } # Delete personality folder try: personality_folder = personalities_dir / personality_name shutil.rmtree(personality_folder) except Exception as e: return { "status": "error", "message": "Failed to delete personality folder.", "error_detail": str(e), "code": "deletion_failed" } ``` The validation helper also constructs the path without enforcing containment: ```python def personality_exists(personalities_dir, name): """Check if personality folder exists and is valid.""" personality_folder = personalities_dir / name is_valid, _ = verify_personality_folder(personality_folder) return is_valid ``` ### Technical Analysis `delete_personality()` does not call `validate_personality_name()` and does not verify that the resolved target remains beneath the personalities directory. Python's `pathlib` accepts `..` components and absolute paths when joining paths. The only prerequisite imposed by `personality_exists()` is that the resulting directory contain readable `SOUL.md` and `IDENTITY.md` files. It does not establish that the directory is an actual personality directory. Consequently, a value such as `..` resolves from: ```text ~/.openclaw/workspace/personalities/.. ``` to: ```text ~/.openclaw/workspace ``` The workspace is expected to contain `SOUL.md` and `IDENTITY.md`, so it can satisfy the existence check. `shutil.rmtree()` may then recursively delete the entire workspace. Absolute paths or traversal p ...[truncated 1559 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Apply `validate_personality_name()` before performing any lookup or deletion: ```python is_valid, error = validate_personality_name(personality_name) if not is_valid: return { "status": "error", "message": f"Invalid personality name: {error}", "code": "invalid_name" } ``` 2. Enforce resolved-path containment: ```python root = personalities_dir.resolve() candidate = (personalities_dir / personality_name).resolve(strict=True) if not candidate.is_relative_to(root): raise ValueError("Personality path escapes the personality directory") ``` 3. Require the target to be a direct child: ```python if candidate.parent != root: raise ValueError("Nested or external personality paths are not allowed") ``` 4. Reject absolute paths, path separators, `.` and `..` explicitly. 5. Do not recursively delete symlinked personality directories. Use `lstat()` or equivalent checks and reject symbolic links before destructive operations. 6. Perform the same centralized validation in every command that accepts or reads a personality name. 7. Add regression tests for `..`, `../target`, absolute paths, repeated separators, Unicode separator variants, and symlink-based paths. ]]>