T09 · Insecure Skill Coding Practices
Warning
- Location
- scripts/save.py:185
- Finding
- Save Slot Path Traversal Allows Writes Outside the Save Directory<![CDATA[ ## Vulnerability Details **File Location**: `scripts/save.py`, lines 185-211 **Vulnerability Type**: Path traversal and insufficient filesystem boundary validation **Risk Level**: Medium ### Vulnerable Code ```python def slot_dir(slot): if not slot or any(ch in slot for ch in '/\\:*?"<>|'): # Error reporting omitted; it does not impose additional path constraints. fail(..., 1) return os.path.join(SAVES_DIR, slot) def slot_paths(slot): d = slot_dir(slot) return { "dir": d, "state": os.path.join(d, "state.json"), "memory": os.path.join(d, "memory.md"), "rolls": os.path.join(d, "rolls.jsonl"), "world": os.path.join(d, "world.json"), "archives": os.path.join(d, "archives"), } def atomic_write(path, text): """Temporary file followed by an atomic replacement in the same directory.""" d = os.path.dirname(path) os.makedirs(d, exist_ok=True) fd, tmp = tempfile.mkstemp(dir=d, prefix=".tmp_", suffix=".json") try: with os.fdopen(fd, "w", encoding="utf-8", newline="\n") as f: f.write(text) os.replace(tmp, path) except BaseException: if os.path.exists(tmp): os.remove(tmp) raise ``` The resulting paths are used by commands including `init`, `update`, `world-init`, and `archive`. For example, `cmd_init()` writes to the paths without performing a containment check: ```python atomic_write(p["state"], json.dumps( ordered, ensure_ascii=False, separators=(",", ":") )) atomic_write(p["memory"], memory) os.makedirs(p["archives"], exist_ok=True) ``` ### Technical Analysis `slot_dir()` rejects conventional path separators and several platform-specific reserved characters, but it does not reject the special directory names `.` and `..`. It also does not canonicalize the resulting path or verify that the resolved path remains under `SAVES_DIR`. On POSIX systems, a slot value of `..` produces: ```text ...[truncated 2813 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. **Use a strict slot-name allowlist** Accept only a small, explicit character set and length: ```python import re SLOT_PATTERN = re.compile(r"^[A-Za-z0-9_-]{1,64}$") def validate_slot_name(slot): if not isinstance(slot, str) or not SLOT_PATTERN.fullmatch(slot): fail("Invalid slot name; use 1-64 letters, digits, underscores, or hyphens.", 1) if slot in {".", ".."}: fail("Reserved slot name.", 1) ``` 2. **Canonicalize and enforce containment** Resolve both the base directory and candidate directory, then compare them using `os.path.commonpath()`: ```python def slot_dir(slot): validate_slot_name(slot) base = os.path.realpath(SAVES_DIR) candidate = os.path.realpath(os.path.join(base, slot)) if os.path.commonpath([base, candidate]) != base: fail("Slot path escapes the save directory.", 1) return candidate ``` Do not use string-prefix checks because paths such as `/app/saves-evil` may share a textual prefix without being descendants. 3. **Reject symbolic-link slot directories** Before using an existing slot, inspect it with `os.lstat()` and reject symbolic links: ```python if os.path.lexists(candidate) and os.path.islink(candidate): fail("Symbolic-link save slots are not allowed.", 1) ``` 4. **Protect against time-of-check/time-of-use races** Where supported, use directory file descriptors and no-follow behavior such as `O_NOFOLLOW`. Perform file creation relative to an already-open trusted directory rather than repeatedly resolving path strings. 5. **Revalidate immediately before every write** Apply the same containment and symlink checks in `atomic_write()`, `backup()`, roll-log appends, archive creation, and other write paths. A single validation at argument parsing is insufficient if directories can change before the write. 6. **Add regression tests** Confirm that the ...[truncated 299 chars]