T09 · Insecure Skill Coding Practices
Error
- Location
gstack-skills/scripts/state_manager.py:72- Finding
Path Traversal in Workflow State Operations Allows Arbitrary JSON File Access
- Content
View full analysis
Optional[Dict]: workflow_file = self.state_dir / f"{workflow_id}.json" if not workflow_file.exists(): return None with open(workflow_file, "r") as f: return json.load(f) ``` ```python def save_workflow(self, workflow_id: str, state: Dict) -> bool: workflow_file = self.state_dir / f"{workflow_id}.json" state["updated_at"] = datetime.now().isoformat() try: with open(workflow_file, "w") as f: json.dump(state, f, indent=2) return True except Exception as e: print(f"Error saving workflow: {e}") return False ``` ```python def delete_workflow(self, workflow_id: str) -> bool: workflow_file = self.state_dir / f"{workflow_id}.json" if workflow_file.exists(): workflow_file.unlink() return True return False ``` The command-line interface passes user-controlled values directly into these methods: ```python workflow_id = sys.argv[2] ``` ### Technical Analysis The workflow identifier is concatenated into a filesystem path without validation or a containment check: ```python self.state_dir / f"{workflow_id}.json" ``` Although legitimate workflow identifiers are generated as eight hexadecimal characters, externally supplied identifiers are not restricted to that format. Path components such as `..` are therefore interpreted by the operating system rather than treated as an opaque workflow ID. The default state directory is `.workbuddy/gstack-state`. An identifier such as `../../target` produces an effective path similar to: ```text .workbuddy/gstack-state/../../target.json ``` This ...[truncated 2145 chars]- Remediation
View remediation
None: if not WORKFLOW_ID_PATTERN.fullmatch(workflow_id): raise ValueError("Invalid workflow ID") ``` 2. Validate the identifier before every load, save, or delete operation. 3. Resolve the target path and verify that it remains inside the canonical state directory: ```python def workflow_path(self, workflow_id: str) -> Path: validate_workflow_id(workflow_id) base = self.state_dir.resolve() target = (base / f"{workflow_id}.json").resolve() if target.parent != base: raise ValueError("Workflow path escapes the state directory") return target ``` 4. Use the centralized `workflow_path` helper in all filesystem operations rather than reconstructing paths independently. 5. Reject absolute paths, path separators, `..`, unexpected Unicode separators, and identifiers outside the generated format. 6. Protect against symlink races where the threat model includes untrusted local users. Consider opening files relative to a trusted directory descriptor with no-follow behavior. 7. Write state atomically using a temporary file in the same trusted directory followed by an atomic replacement. 8. Apply restrictive permissions to the state directory and files, such as user-only access where supported. 9. Add regression tests for traversal attempts including `../`, nested traversal, absolute-path-like values, symlinks, malformed Unicode, and valid eight-character hexadecimal IDs. ]]>