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<![CDATA[ ## Vulnerability Details **File Location**: `scripts/ngrok_preview.py:341-342`, `scripts/ngrok_preview.py:422`, `scripts/ngrok_preview.py:438-441`, `scripts/ngrok_preview.py:465-467`, and `scripts/ngrok_preview.py:528-530` **Vulnerability Type**: Path traversal and unsafe recursive deletion **Risk Level**: High ### Vulnerable Code ```python session_dir = SESSIONS_DIR / session_id if session_dir.exists(): raise RuntimeError(f"Session already exists: {session_id}") ``` ```python write_json(STATE_DIR / f"{session_id}.json", state) ``` ```python def load_state_by_id(session_id: str) -> 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
- <![CDATA[ ## Remediation Suggestions 1. Validate every supplied session ID before using it: - Permit only a strict allowlist such as ASCII letters, digits, underscores, and hyphens. - Enforce a reasonable length, such as 1–64 characters. - Reject path separators, `.` and `..` path components, absolute paths, and platform-specific drive syntax. Example: ```python import re SESSION_ID_RE = re.compile(r"^[A-Za-z0-9_-]{1,64}$") def validate_session_id(session_id: str) -> 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. ]]>