T09 · Insecure Skill Coding Practices
Error
- Location
scripts/ngrok_preview.py:341- Finding
Path Traversal Through Unsanitized Session Identifiers Enables Out-of-Scope Recursive Deletion
- Content
View full analysis
tuple[Path, dict[str, Any]]: path = STATE_DIR / f"{session_id}.json" if not path.exists(): raise FileNotFoundError(f"No session state found for: {session_id}") return path, read_json(path) ``` ```python if args.delete_session_dir: session_dir = Path(state.get("workspace_dir", "")) if session_dir.exists() and str(session_dir).startswith(str(SESSIONS_DIR)): shutil.rmtree(session_dir) ``` The cleanup command uses the same unsafe deletion check: ```python session_dir = Path(state.get("workspace_dir", "")) if session_dir.exists() and str(session_dir).startswith(str(SESSIONS_DIR)): shutil.rmtree(session_dir) ``` ### Technical Analysis The user-controlled `--session-id` value is directly incorporated into session-directory and state-file paths without validation. Components such as `..`, path separators, and absolute-path syntax are not rejected. The deletion guard compares unresolved path strings: ```python str(session_dir).startswith(str(SESSIONS_DIR)) ``` String-prefix comparison does not establish filesystem containment. A path such as: ```text /home/user/.cache/openclaw-ngrok-preview/sessions/../sessions-victim ``` starts with the textual sessions-directory prefix but resolves to: ```text /home/user/.cache/openclaw-ngrok-preview/sessions-victim ``` Consequently, the check can approve a directory outside `SESSIONS_DIR` ...[truncated 2138 chars]- Remediation
View remediation
str: if not SESSION_ID_RE.fullmatch(session_id): raise ValueError("Invalid session ID") return session_id ``` 2. Resolve paths and verify semantic containment before every creation, read, write, or deletion: ```python def contained_path(root: Path, child: str) -> Path: resolved_root = root.resolve() candidate = (resolved_root / child).resolve() if not candidate.is_relative_to(resolved_root): raise ValueError("Path escapes the permitted root") return candidate ``` 3. Do not use display identifiers as directory names. Generate an internal random identifier, such as a UUID, and store the user-facing session label only as metadata. 4. Before recursive deletion: - Resolve the candidate and root. - Require `candidate.is_relative_to(root)`. - Reject deletion of the root itself. - Consider opening and tracking session directories through trusted internal state rather than accepting reconstructed paths. 5. Treat state files as untrusted input. Validate the schema, normalize paths, and independently re-check containment instead of trusting `workspace_dir`. 6. Add regression tests covering `../`, nested traversal, absolute paths, symbolic links, prefix-collision directories such as `sessions-victim`, and platform-specific path separators. ]]>