diff --git a/tests/tools/test_skills_hub.py b/tests/tools/test_skills_hub.py index dc68aca1d33..432ac521316 100644 --- a/tests/tools/test_skills_hub.py +++ b/tests/tools/test_skills_hub.py @@ -1693,3 +1693,223 @@ class TestDownloadDirectoryRecursive: assert "SKILL.md" in files assert "scripts/run.py" not in files # lost due to rate limit + + +# --------------------------------------------------------------------------- +# Install-path safety (lock-file → uninstall rmtree boundary) +# --------------------------------------------------------------------------- + + +class TestInstallPathSafety: + """Guard the lock-file → ``uninstall_skill`` rmtree path. + + The destructive boundary is ``shutil.rmtree(SKILLS_DIR / install_path)``. + Lock-file ``install_path`` values that are absolute, contain ``..``, + point at the skills root itself, or are redirected via a symlink/junction + inside ``skills/`` must be rejected before they reach rmtree. + """ + + @pytest.fixture + def isolated_skills_dir(self, tmp_path, monkeypatch): + skills_dir = tmp_path / "skills" + skills_dir.mkdir() + monkeypatch.setattr("tools.skills_hub.SKILLS_DIR", skills_dir) + return skills_dir + + @pytest.fixture + def patch_lock_file(self, monkeypatch): + """Redirect HubLockFile's default path to a test-controlled file. + + HubLockFile.__init__ captures LOCK_FILE as a default arg at class + definition time, so monkeypatching the module-level LOCK_FILE doesn't + affect later HubLockFile() calls. Patch __defaults__ instead. + """ + def _apply(lock_path): + monkeypatch.setattr(HubLockFile.__init__, "__defaults__", (lock_path,)) + return _apply + + @pytest.mark.parametrize( + "bad_install_path", + [ + "", + ".", + "..", + "../../etc/passwd", + "/etc/passwd", + "skills/../../tmp", + "C:/Windows/System32", + ], + ) + def test_record_install_rejects_unsafe_paths(self, tmp_path, bad_install_path): + """record_install must reject malformed install_path values at write time.""" + lock = HubLockFile(path=tmp_path / "lock.json") + with pytest.raises(ValueError, match="Unsafe"): + lock.record_install( + name="evil", + source="github", + identifier="x", + trust_level="trusted", + scan_verdict="pass", + skill_hash="h1", + install_path=bad_install_path, + files=["SKILL.md"], + ) + + def test_record_install_rejects_mismatched_last_component(self, tmp_path): + """The final component of install_path MUST equal the skill name.""" + lock = HubLockFile(path=tmp_path / "lock.json") + with pytest.raises(ValueError, match="Unsafe install path"): + lock.record_install( + name="legit-skill", + source="github", + identifier="x", + trust_level="trusted", + scan_verdict="pass", + skill_hash="h1", + install_path="legit-skill/evil-suffix", + files=["SKILL.md"], + ) + + def test_record_install_accepts_bare_name(self, tmp_path): + lock = HubLockFile(path=tmp_path / "lock.json") + lock.record_install( + name="good", source="github", identifier="x", + trust_level="trusted", scan_verdict="pass", + skill_hash="h", install_path="good", files=["SKILL.md"], + ) + assert lock.get_installed("good")["install_path"] == "good" + + def test_record_install_accepts_category_and_name(self, tmp_path): + lock = HubLockFile(path=tmp_path / "lock.json") + lock.record_install( + name="good", source="github", identifier="x", + trust_level="trusted", scan_verdict="pass", + skill_hash="h", install_path="devops/good", files=["SKILL.md"], + ) + assert lock.get_installed("good")["install_path"] == "devops/good" + + def test_uninstall_rejects_poisoned_absolute_path(self, tmp_path, isolated_skills_dir, patch_lock_file): + """Hand-edited lock.json with absolute install_path must not delete anything.""" + from tools.skills_hub import uninstall_skill + + lock_path = tmp_path / "lock.json" + target = tmp_path / "victim" + target.mkdir() + (target / "file.txt").write_text("important") + + # Bypass record_install's validator to simulate a poisoned lock file. + lock_path.write_text(json.dumps({ + "installed": { + "evil": { + "source": "github", + "identifier": "x", + "trust_level": "trusted", + "scan_verdict": "pass", + "content_hash": "h", + "install_path": str(target), + "files": [], + "metadata": {}, + "installed_at": "now", + "updated_at": "now", + } + } + })) + + patch_lock_file(lock_path) + ok, msg = uninstall_skill("evil") + assert ok is False + assert "Unsafe" in msg or "Refusing" in msg + assert target.exists() + assert (target / "file.txt").read_text() == "important" + + def test_uninstall_rejects_traversal(self, tmp_path, isolated_skills_dir, patch_lock_file): + from tools.skills_hub import uninstall_skill + + lock_path = tmp_path / "lock.json" + sibling = tmp_path / "sibling" + sibling.mkdir() + (sibling / "data").write_text("nope") + + lock_path.write_text(json.dumps({ + "installed": { + "evil": { + "source": "github", "identifier": "x", + "trust_level": "trusted", "scan_verdict": "pass", + "content_hash": "h", + "install_path": "../sibling", + "files": [], "metadata": {}, + "installed_at": "now", "updated_at": "now", + } + } + })) + + patch_lock_file(lock_path) + ok, msg = uninstall_skill("evil") + assert ok is False + assert sibling.exists() + assert (sibling / "data").read_text() == "nope" + + def test_uninstall_rejects_empty_install_path(self, tmp_path, isolated_skills_dir, patch_lock_file): + """Empty install_path resolves to SKILLS_DIR itself — must be refused.""" + from tools.skills_hub import uninstall_skill + + # Put a sibling skill alongside to prove rmtree doesn't fire. + (isolated_skills_dir / "bystander").mkdir() + (isolated_skills_dir / "bystander" / "SKILL.md").write_text("safe") + + lock_path = tmp_path / "lock.json" + lock_path.write_text(json.dumps({ + "installed": { + "evil": { + "source": "github", "identifier": "x", + "trust_level": "trusted", "scan_verdict": "pass", + "content_hash": "h", + "install_path": "", + "files": [], "metadata": {}, + "installed_at": "now", "updated_at": "now", + } + } + })) + + patch_lock_file(lock_path) + ok, msg = uninstall_skill("evil") + assert ok is False + assert (isolated_skills_dir / "bystander" / "SKILL.md").read_text() == "safe" + + def test_uninstall_rejects_symlink_redirect_inside_skills( + self, tmp_path, isolated_skills_dir, patch_lock_file + ): + """A symlinked skill dir that points outside skills/ must not be followed.""" + from tools.skills_hub import uninstall_skill + + # Outside-tree victim + victim = tmp_path / "victim" + victim.mkdir() + (victim / "important").write_text("don't delete me") + + # Symlink in skills/ pointing to the victim + link = isolated_skills_dir / "evil" + try: + link.symlink_to(victim, target_is_directory=True) + except (OSError, NotImplementedError): + pytest.skip("symlink creation unsupported on this platform") + + lock_path = tmp_path / "lock.json" + lock_path.write_text(json.dumps({ + "installed": { + "evil": { + "source": "github", "identifier": "x", + "trust_level": "trusted", "scan_verdict": "pass", + "content_hash": "h", + "install_path": "evil", + "files": [], "metadata": {}, + "installed_at": "now", "updated_at": "now", + } + } + })) + + patch_lock_file(lock_path) + ok, msg = uninstall_skill("evil") + assert ok is False + assert victim.exists() + assert (victim / "important").read_text() == "don't delete me" diff --git a/tools/skills_hub.py b/tools/skills_hub.py index 35a6749cd5d..9f58e96284b 100644 --- a/tools/skills_hub.py +++ b/tools/skills_hub.py @@ -124,6 +124,69 @@ def _validate_category_name(category: str) -> str: return _normalize_bundle_path(category, field_name="category", allow_nested=False) +def _normalize_lock_install_path(install_path: str, skill_name: str) -> str: + """Validate a skill install path before it touches the lock file or disk. + + Lock-file ``install_path`` entries are the source-of-truth for where + ``uninstall_skill`` will call ``shutil.rmtree``. A poisoned or buggy + entry — empty string, ``"."``, an absolute path, ``../..`` traversal, + or anything whose final component doesn't match the skill name — would + let ``rmtree`` wipe either the entire ``skills/`` tree or content + outside it. + + Enforce that ``install_path`` is exactly ```` or + ``/``. Reject anything else. + """ + safe_skill_name = _validate_skill_name(skill_name) + normalized = _normalize_bundle_path( + install_path, + field_name="install path", + allow_nested=True, + ) + parts = normalized.split("/") + if len(parts) not in {1, 2} or parts[-1] != safe_skill_name: + raise ValueError(f"Unsafe install path: {install_path}") + return normalized + + +def _is_path_redirect(path: Path) -> bool: + """True when ``path`` is a symlink or (on Windows) a directory junction. + + Either form lets an attacker who can write into the ``skills/`` tree + redirect a subsequent ``rmtree`` to content outside it. ``is_junction`` + only exists on Python 3.12+ Windows; gate with ``hasattr``. + """ + return path.is_symlink() or (hasattr(path, "is_junction") and path.is_junction()) + + +def _resolve_lock_install_path(install_path: str, skill_name: str) -> Path: + """Resolve a lock-file install path without allowing escapes from ``SKILLS_DIR``. + + Two layers of defence on top of the existing ``is_relative_to`` check + that's been on main: + + 1. Walk the path component-by-component and refuse if any intermediate + component is a symlink/junction (a path resolution that follows a + symlink to outside skills/ would otherwise be hidden by Path.resolve). + 2. After resolve(), reject not just escape-out but also ``resolved == SKILLS_DIR`` + — an empty/``"."``/``""`` install_path resolves to the skills root itself, + and ``rmtree(SKILLS_DIR)`` would wipe every installed skill. + """ + normalized = _normalize_lock_install_path(install_path, skill_name) + skills_root = SKILLS_DIR.resolve() + + target = SKILLS_DIR + for part in normalized.split("/"): + target = target / part + if _is_path_redirect(target): + raise ValueError(f"Unsafe install path: {install_path}") + + target = target.resolve() + if target == skills_root or not target.is_relative_to(skills_root): + raise ValueError(f"Unsafe install path: {install_path}") + return target + + def _guarded_http_get(url: str, *, timeout: int = 20) -> Optional[httpx.Response]: """Fetch a URL with SSRF and redirect-target validation.""" current_url = url @@ -2788,14 +2851,20 @@ class HubLockFile: files: List[str], metadata: Optional[Dict[str, Any]] = None, ) -> None: + # Validate both the skill name and the install path SHAPE before + # writing into lock.json. A poisoned lock entry is the precondition + # for the uninstall_skill rmtree-escape; reject malformed input at + # write time so the file never carries the bad state. + safe_name = _validate_skill_name(name) + safe_install_path = _normalize_lock_install_path(install_path, safe_name) data = self.load() - data["installed"][name] = { + data["installed"][safe_name] = { "source": source, "identifier": identifier, "trust_level": trust_level, "scan_verdict": scan_verdict, "content_hash": skill_hash, - "install_path": install_path, + "install_path": safe_install_path, "files": files, "metadata": metadata or {}, "installed_at": datetime.now(timezone.utc).isoformat(), @@ -2943,9 +3012,14 @@ def install_from_quarantine( raise ValueError(f"Unsafe quarantine path: {quarantine_path}") if safe_category: - install_dir = SKILLS_DIR / safe_category / safe_skill_name + install_rel_path = f"{safe_category}/{safe_skill_name}" else: - install_dir = SKILLS_DIR / safe_skill_name + install_rel_path = safe_skill_name + + # Resolve via the same lock-path validator the uninstaller uses. Catches + # symlink-in-skills-tree redirects at install time so the lock entry's + # path can never refer to a redirected target. + install_dir = _resolve_lock_install_path(install_rel_path, safe_skill_name) if install_dir.exists(): shutil.rmtree(install_dir) @@ -2999,14 +3073,20 @@ def uninstall_skill(skill_name: str) -> Tuple[bool, str]: if not entry: return False, f"'{skill_name}' is not a hub-installed skill (may be a builtin)" - install_path = SKILLS_DIR / entry["install_path"] - # Prevent path traversal from poisoned lock.json entries + # Validate the lock entry's install_path against the skill name. This is + # the destructive boundary — anything that falls through to the rmtree + # below MUST be inside SKILLS_DIR and MUST NOT be SKILLS_DIR itself + # (an empty/"."/"/" install_path would otherwise wipe the entire tree). + # _resolve_lock_install_path enforces shape ( or + # /), rejects absolute/traversal paths, and walks + # the path component-by-component refusing symlink/junction redirects. try: - resolved = install_path.resolve() - if not resolved.is_relative_to(SKILLS_DIR.resolve()): - return False, f"Refusing to remove '{entry['install_path']}': resolves outside skills directory" - except (ValueError, OSError): - return False, f"Refusing to remove '{entry['install_path']}': path resolution failed" + install_path = _resolve_lock_install_path( + entry.get("install_path", ""), skill_name + ) + except ValueError as exc: + return False, f"Refusing to uninstall '{skill_name}': {exc}" + if install_path.exists(): shutil.rmtree(install_path)