T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/signet.py:480
- Finding
- Arbitrary Directory Deletion Through Unvalidated Skill Name## Vulnerability Details **File Location**: `scripts/signet.py:480-502` **Vulnerability Type**: Path traversal and arbitrary recursive directory deletion **Risk Level**: Critical The `restore` command constructs both the snapshot source path and restoration destination directly from the user-controlled `skill_name` argument. ```python def cmd_restore(ws, skill_name): banner("RESTORE SKILL", ws) snap_dir = snapshots_base(ws) / skill_name meta_path = snapshots_base(ws) / f"{skill_name}.json" if not snap_dir.is_dir(): print(f"No snapshot found for: {skill_name}") return 1 snap_meta = None if meta_path.exists(): try: with open(meta_path, "r", encoding="utf-8") as f: snap_meta = json.load(f) except (json.JSONDecodeError, OSError): pass snap_composite, snap_files = skill_hash(snap_dir) if snap_meta: expected = snap_meta.get("composite_hash") if expected and snap_composite != expected: print(f"SNAPSHOT CORRUPTED! Expected: {short(expected)} Got: {short(snap_composite)}") return 2 print(f" Snapshot verified: {short(snap_composite)}") else: print(" WARNING: No snapshot metadata. Restoring unverified.") skill_dir = ws / "skills" / skill_name if skill_dir.exists(): shutil.rmtree(skill_dir) shutil.copytree(str(snap_dir), str(skill_dir)) ``` ### Technical Analysis Python's `pathlib` discards the preceding path components when the right-hand operand of `/` is an absolute path. Consequently, if `skill_name` is an absolute path such as `/tmp/victim`, both of the following expressions resolve to `/tmp/victim`: - `snapshots_base(ws) / skill_name` - `ws / "skills" / skill_name` The initial `snap_dir.is_dir()` check succeeds when the attacker-selected target exists. Because snapshot metadata is optional, the comman ...[truncated 1722 chars]
- Remediation
- ## Remediation Suggestions - Treat `skill_name` as an identifier rather than a filesystem path. Reject absolute paths, empty names, `.` and `..`, and names containing `/`, `\`, or platform-specific path separators. - Use a strict allowlist such as letters, digits, hyphens, and underscores. - Resolve both source and destination paths before performing filesystem operations and verify containment with `Path.relative_to()` or an equivalent safe check. - Require the resolved snapshot source to be a direct child of `.signet/snapshots` and the destination to be a direct child of `skills`. - Explicitly reject operations when the resolved source and destination are equal. - Require valid snapshot metadata and a matching trusted hash before any destructive restoration operation. - Copy into a newly created temporary directory under the destination base, verify the copy, and only then atomically replace the existing skill. - Avoid deleting the existing destination until all source validation and copy preparation have succeeded.