T09 · Insecure Skill Coding Practices
Warning
- Location
- scripts/director.py:45
- Finding
- Arbitrary JSON File Access Through Unsanitized Movie IDs## Vulnerability Details **File Location**: `scripts/director.py:45-54` **Vulnerability Type**: Path traversal and arbitrary file read/write **Risk Level**: Medium ### Vulnerable Code ```python def load_movie(movie_id): f = get_data_dir() / f"{movie_id}.json" if not f.exists(): return None return json.loads(f.read_text(encoding="utf-8")) def save_movie(movie): f = get_data_dir() / f"{movie['id']}.json" f.write_text(json.dumps(movie, ensure_ascii=False, indent=2), encoding="utf-8") ``` ### Technical Analysis The application directly interpolates a CLI-controlled movie ID into a filesystem path without validating its format or verifying that the resolved path remains inside `~/.openclaw/memory/movies/`. A movie ID containing `../` components can traverse outside the intended storage directory. An absolute path is also unsafe because Python's `pathlib` discards the preceding base path when the right-hand operand is absolute. This affects every command that passes an untrusted `--id` value to `load_movie`. After loading a document, mutating commands call `save_movie`, which independently trusts the `id` property stored inside that document. An attacker can therefore control both the source path and, through the loaded JSON content, the subsequent destination path. The mandatory `.json` suffix limits accessible targets to paths ending in `.json`, and successful mutation requires the selected document to contain the structure expected by the invoked command. These constraints reduce, but do not eliminate, the vulnerability. ### Attack Path 1. The attacker identifies or creates a structurally valid JSON document outside `~/.openclaw/memory/movies/`, such as `/tmp/movie.json`. 2. The attacker supplies an absolute or traversal-based identifier to a command that accepts `--id`, for example: ```bash python scripts/director.py approve --id /tmp/movie ``` 3. `load_movie()` constructs the path from the unvalidated identifier an ...[truncated 1120 chars]
- Remediation
- ## Remediation Suggestions 1. Enforce the generated movie-ID format before every read or write: ```python import re MOVIE_ID_PATTERN = re.compile(r"^movie_[0-9]{7}$") def validate_movie_id(movie_id): if not isinstance(movie_id, str) or not MOVIE_ID_PATTERN.fullmatch(movie_id): raise ValueError("Invalid movie ID") return movie_id ``` 2. Resolve candidate paths and enforce containment within the movie data directory: ```python def movie_path(movie_id): movie_id = validate_movie_id(movie_id) base = get_data_dir().resolve() candidate = (base / f"{movie_id}.json").resolve() if candidate.parent != base: raise ValueError("Movie path escapes the data directory") return candidate ``` 3. Use the same validated path helper in both `load_movie()` and `save_movie()`. 4. Do not trust the `id` field read from persisted JSON. Retain the validated ID supplied to `load_movie()` and use that trusted value when saving. Alternatively, verify that the document's `id` exactly matches the validated filename ID before processing it. 5. Reject absolute paths, path separators, dot components, and unexpected Unicode representations in identifiers as defense in depth. 6. Perform writes atomically by creating a temporary file in the same protected directory, setting restrictive permissions, flushing it, and replacing the destination only after serialization succeeds. 7. Add regression tests covering absolute IDs, `../` traversal, nested separators, malformed IDs, and persisted documents whose internal `id` differs from the filename.