T09 · Insecure Skill Coding Practices
Warning
- Location
- src/data/storage.py:20
- Finding
- Unrestricted Storage Paths Permit Directory Traversal## Vulnerability Details **File Location**: `src/data/storage.py:20-43` **Vulnerability Type**: Path traversal and arbitrary file access **Risk Level**: Medium ### Vulnerable Code ```python def _get_path(self, filename: str) -> Path: return self.data_dir / filename def load(self, filename: str, default: Any = None) -> Any: """Load a JSON file.""" path = self._get_path(filename) if not path.exists(): return default try: with open(path, 'r', encoding='utf-8') as f: return json.load(f) except (json.JSONDecodeError, IOError): return default def save(self, filename: str, data: Any) -> bool: """Save a JSON file.""" path = self._get_path(filename) try: with open(path, 'w', encoding='utf-8') as f: json.dump(data, f, ensure_ascii=False, indent=2) return True except IOError as e: print(f"Save failed {filename}: {e}") return False ``` ### Technical Analysis `_get_path()` directly joins an unrestricted `filename` value to the configured data directory. It does not reject absolute paths, parent-directory components, path separators, or symbolic-link escapes. It also does not resolve the resulting path and verify that it remains under `data_dir`. A filename such as `../../target.json` can therefore escape the intended storage directory. `load()` may read any accessible file containing valid JSON, while `save()` may overwrite any existing or creatable file writable by the application process with attacker-supplied JSON data. The currently reviewed user-facing command handlers call the storage layer with fixed filenames, so direct exploitation through the documented chat commands was not identified. Exploitation becomes possible if an integration, plugin, test harness, or future command passes attacker-controlled filenames to this public storage API. ### Attack Path 1. An attacker ga ...[truncated 1255 chars]
- Remediation
- ## Remediation Suggestions 1. Allowlist the exact storage filenames required by the application, such as `stock_pool.json`, `positions.json`, `trades.json`, `challenge.json`, `recommendations.json`, and `config.json`. 2. Reject absolute paths, parent-directory components, and values containing directory separators. 3. Resolve both the base directory and destination, then verify that the destination remains inside the base directory. 4. Reject symbolic-link destinations where practical. 5. Keep filename selection internal to the storage layer rather than accepting arbitrary strings from callers. Example hardening: ```python ALLOWED_FILES = { "stock_pool.json", "positions.json", "trades.json", "challenge.json", "recommendations.json", "config.json", } def _get_path(self, filename: str) -> Path: if filename not in ALLOWED_FILES: raise ValueError("Unsupported storage filename") base = self.data_dir.resolve() path = (base / filename).resolve() if path.parent != base: raise ValueError("Storage path escapes the data directory") return path ``` Add tests covering `../`, absolute paths, nested traversal, alternate separators, and symbolic-link escapes.