T09 · Insecure Skill Coding Practices
Error
- Location
- scripts/skill_manager.py:189
- Finding
- Protected Skill Deletion Through Symlink Resolution<![CDATA[ ## Vulnerability Details **File Location**: `scripts/skill_manager.py`, lines 189-239 **Vulnerability Type**: Symlink validation after path resolution **Risk Level**: High ### Vulnerable Code ```python # Step 2: Resolve paths and verify containment skill_path = (SKILLS_DIR / skill_name).resolve() skills_dir_resolved = SKILLS_DIR.resolve() # Security check: ensure resolved path is within skills directory if not str(skill_path).startswith(str(skills_dir_resolved) + os.sep): raise ValueError( f"Security violation: skill path '{skill_path}' is outside skills directory. " "Path traversal attempt blocked." ) # Step 3: Verify the path exists and is a directory if not skill_path.exists(): return False if not skill_path.is_dir(): raise ValueError(f"Skill path '{skill_path}' is not a directory") # Step 4: Additional symlink check (prevent symlink attacks) if skill_path.is_symlink(): raise ValueError( f"Security violation: skill path '{skill_path}' is a symlink. " "Symlinks are not allowed for safety." ) registry = load_registry() # Step 5: Prevent uninstalling system skills if skill_name in SYSTEM_SKILLS and not force: raise ValueError( f"Cannot uninstall system skill '{skill_name}'. " "System skills are protected. Use --force to override (not recommended)." ) # Step 6: Archive metadata before removal if archive and skill_name in registry["skills"]: archive_file = ARCHIVE_DIR / f"{skill_name}.json" archive_data = registry["skills"][skill_name].copy() archive_data["uninstalled_at"] = datetime.utcnow().isoformat() + "Z" archive_file.write_text(json.dumps(archive_data, indent=2)) # Step 7: Remove skill directory (now safe) shutil.rmtree(skill_path) ``` ### Technical Analysis The code calls `Path.resolve()` before testing `skill_path.is_symlink()`. Resolution dereferences the original directory entry, so `is_symlink()` examines the resolved target rather tha ...[truncated 2072 chars]
- Remediation
- <![CDATA[ ## Remediation Suggestions 1. Construct the unresolved candidate path and reject symlinks before calling `resolve()`: ```python candidate = SKILLS_DIR / skill_name if candidate.is_symlink(): raise ValueError("Symlinked Skill directories are not allowed") skills_root = SKILLS_DIR.resolve(strict=True) skill_path = candidate.resolve(strict=True) ``` 2. Use path-aware containment rather than string-prefix comparison: ```python if not skill_path.is_relative_to(skills_root): raise ValueError("Resolved Skill path is outside the Skills directory") ``` 3. Verify that the resolved target corresponds to the requested Skill: ```python if skill_path.parent != skills_root or skill_path.name != skill_name: raise ValueError("Skill path does not resolve to the requested direct child") ``` 4. Apply system-Skill protection to the verified resolved target name as well as the supplied name. 5. Minimize time-of-check/time-of-use exposure by revalidating immediately before deletion. Where practical, use descriptor-relative filesystem operations that do not follow symlinks. 6. Add regression tests covering symlinks to ordinary Skills, symlinks to protected Skills, external symlink targets, nested paths, and replacement of a validated directory with a symlink before deletion. ]]>