T09 · Insecure Skill Coding Practices
Warning
- Location
- plan_tracker.py:51
- Finding
- Arbitrary JSON File Read and Write Through Unsanitized Plan IDs## Vulnerability Details **File Location**: `plan_tracker.py`, lines 51-88 **Vulnerability Type**: Path traversal and unrestricted file access **Risk Level**: Medium ### Vulnerable Code ```python def _get_plan_path(self, plan_id: str) -> str: return os.path.join(self.storage_dir, f"{plan_id}.json") ``` ```python def save_plan(self, plan: Plan) -> None: """Save a plan.""" path = self._get_plan_path(plan.id) with open(path, 'w', encoding='utf-8') as f: json.dump({ 'id': plan.id, 'title': plan.title, 'description': plan.description, 'tasks': [asdict(t) for t in plan.tasks], 'status': plan.status, 'created_at': plan.created_at, 'updated_at': plan.updated_at }, f, indent=2, ensure_ascii=False) ``` ```python def load_plan(self, plan_id: str) -> Optional[Plan]: """Load a plan.""" path = self._get_plan_path(plan_id) if not os.path.exists(path): return None with open(path, 'r', encoding='utf-8') as f: data = json.load(f) ``` ### Technical Analysis `_get_plan_path()` directly incorporates a caller-controlled plan identifier into a filesystem path. It does not validate the identifier, reject absolute paths or traversal components, resolve the resulting path, or verify that it remains under `storage_dir`. Because `os.path.join()` discards the preceding storage directory when its subsequent component is absolute, a plan ID such as `/tmp/target` resolves to `/tmp/target.json`. Relative traversal values such as `../../tmp/target` can similarly escape the intended plan directory. Both access directions are affected: - `load_plan()` can open and parse an attacker-selected JSON file outside the plan directory. - `save_plan()` trusts `plan.id` and can create or overwrite an attacker-selected `.json` file. - Symlinks inside the storage directory coul ...[truncated 1536 chars]
- Remediation
- ## Remediation Suggestions 1. Enforce the generated plan-ID format before any filesystem operation. For example, accept only eight lowercase hexadecimal characters with `^[0-9a-f]{8}$`. 2. Explicitly reject absolute paths, path separators, `.` and `..` components, and identifiers outside the expected character set. 3. Resolve both the storage directory and candidate path with `os.path.realpath()` or `pathlib.Path.resolve()`. 4. Verify containment using `os.path.commonpath()` before opening the file. Reject the operation unless the resolved candidate is strictly inside the resolved storage directory. 5. Apply validation independently in both `load_plan()` and `save_plan()` so that a forged `Plan.id` cannot bypass protections. 6. Mitigate symlink attacks by rejecting symlink targets or using platform-supported no-follow file-opening options where available. 7. Write through a securely created temporary file inside the storage directory and atomically replace the destination. 8. Create plan files with restrictive permissions appropriate for potentially sensitive task data. 9. Add tests covering absolute paths, `../` traversal, nested separators, malformed IDs, symlinks, and valid generated IDs.