diff --git a/backend/packages/harness/deerflow/skills/installer.py b/backend/packages/harness/deerflow/skills/installer.py index 7c791dc08..b32d41a17 100644 --- a/backend/packages/harness/deerflow/skills/installer.py +++ b/backend/packages/harness/deerflow/skills/installer.py @@ -118,6 +118,7 @@ def safe_extract_skill_archive( zip_ref: zipfile.ZipFile, dest_path: Path, max_total_size: int = 512 * 1024 * 1024, + max_entries: int = 4096, ) -> None: """Safely extract a skill archive with security protections. @@ -125,15 +126,28 @@ def safe_extract_skill_archive( - Reject absolute paths and directory traversal (..). - Skip symlink entries instead of materialising them. - Enforce a hard limit on total uncompressed size (zip bomb defence). + - Enforce a hard limit on member count (zip bomb defence by entry count — + a huge number of tiny/empty members can be cheap to store yet still + slow to extract, independent of total size). - Reject executable binaries (ELF/PE/Mach-O) by magic bytes. Raises: - ValueError: If unsafe members, executable binaries, or size limit exceeded. + ValueError: If unsafe members, executable binaries, entry count, or size limit exceeded. """ dest_root = dest_path.resolve() total_written = 0 - for info in zip_ref.infolist(): + infos = zip_ref.infolist() + if len(infos) > max_entries: + # Early-abort before any per-member work below — mirrors the same + # early-abort in skillscan/orchestrator.py::scan_archive_preflight + # (its comment: "a huge member count is a bounded DoS vector even + # when the total size is small"). That scan is optional + # (skill_scan.enabled); this check must hold unconditionally since + # it lives in the extraction path every install goes through. + raise ValueError(f"Skill archive contains too many entries ({len(infos)} > {max_entries}).") + + for info in infos: if is_unsafe_zip_member(info): raise ValueError(f"Archive contains unsafe member path: {info.filename!r}") diff --git a/backend/tests/test_skills_installer.py b/backend/tests/test_skills_installer.py index b0e2d1e36..5884efb9e 100644 --- a/backend/tests/test_skills_installer.py +++ b/backend/tests/test_skills_installer.py @@ -153,6 +153,25 @@ class TestSafeExtract: assert (dest / "normal.txt").exists() assert not (dest / "link.txt").exists() + def test_rejects_too_many_entries(self, tmp_path): + """Entry-count cap is independent of total size: 4 tiny files still trips a low max_entries.""" + zip_path = self._make_zip(tmp_path, {f"file-{i}.txt": "x" for i in range(4)}) + dest = tmp_path / "out" + dest.mkdir() + with zipfile.ZipFile(zip_path) as zf: + with pytest.raises(ValueError, match="too many entries"): + safe_extract_skill_archive(zf, dest, max_entries=3) + assert not any(dest.iterdir()) + + def test_allows_entries_at_the_cap(self, tmp_path): + """The cap is inclusive: exactly max_entries members is not rejected.""" + zip_path = self._make_zip(tmp_path, {f"file-{i}.txt": "x" for i in range(5)}) + dest = tmp_path / "out" + dest.mkdir() + with zipfile.ZipFile(zip_path) as zf: + safe_extract_skill_archive(zf, dest, max_entries=5) + assert len(list(dest.iterdir())) == 5 + def test_normal_archive(self, tmp_path): zip_path = self._make_zip( tmp_path, @@ -227,6 +246,86 @@ class TestSafeExtract: assert (dest / "my-skill" / "assets" / "blob.bin").exists() +# --------------------------------------------------------------------------- +# Entry-count cap must apply unconditionally, independent of skill_scan.enabled. +# +# scan_archive_preflight() (skillscan/orchestrator.py) already caps member +# count at 4096, but only runs as part of the optional native scanner +# (skill_scan.enabled, default true). When that scanner is disabled, +# safe_extract_skill_archive was the only remaining guard on the extraction +# path, and it only capped total bytes — not entry count. These tests pin +# the fix: the cap now lives in extraction itself, so it holds regardless of +# skill_scan.enabled. +# --------------------------------------------------------------------------- + + +class TestEntryCountCapAppliesRegardlessOfSkillScan: + @pytest.fixture(autouse=True) + def _allow_security_scan(self, monkeypatch): + async def _scan(*args, **kwargs): + return ScanResult(decision="allow", reason="ok") + + monkeypatch.setattr("deerflow.skills.installer.scan_skill_content", _scan) + + def _make_storage(self, skills_root: Path, *, skill_scan_enabled: bool): + from types import SimpleNamespace + + from deerflow.skills.storage.local_skill_storage import LocalSkillStorage + + return LocalSkillStorage( + host_path=str(skills_root), + app_config=SimpleNamespace(skill_scan=SimpleNamespace(enabled=skill_scan_enabled)), + ) + + def _make_many_entry_zip(self, tmp_path: Path, *, entry_count: int, skill_name: str = "test-skill") -> Path: + """A real archive with ``entry_count`` tiny members and a small total size — + matches the reported shape (50,000 entries, ~5MB total).""" + zip_path = tmp_path / f"{skill_name}.skill" + with zipfile.ZipFile(zip_path, "w", zipfile.ZIP_DEFLATED) as zf: + zf.writestr(f"{skill_name}/SKILL.md", f"---\nname: {skill_name}\ndescription: A test skill\n---\n\n# {skill_name}\n") + for i in range(entry_count): + zf.writestr(f"{skill_name}/pad-{i:06d}.txt", "") + return zip_path + + def test_rejects_high_entry_count_archive_even_with_skill_scan_disabled(self, tmp_path): + """The previously-vulnerable configuration: skill_scan disabled, so + scan_archive_preflight's member cap never runs. safe_extract_skill_archive + must still reject the archive unconditionally, on its own.""" + zip_path = self._make_many_entry_zip(tmp_path, entry_count=50_000) + skills_root = tmp_path / "skills" + skills_root.mkdir() + storage = self._make_storage(skills_root, skill_scan_enabled=False) + + with pytest.raises(ValueError, match="too many entries"): + storage.install_skill_from_archive(zip_path) + + assert not (skills_root / "custom" / "test-skill").exists() + + def test_scan_archive_preflight_independently_flags_the_same_archive(self, tmp_path): + """Cross-check tying the two protections together: the pre-existing optional + scanner also flags this exact archive by member count when it does run.""" + from deerflow.skills.skillscan.orchestrator import scan_archive_preflight + + zip_path = self._make_many_entry_zip(tmp_path, entry_count=50_000) + + result = scan_archive_preflight(zip_path) + + assert result["blocked"] is True + assert any(finding["rule_id"] == "package-too-many-members" for finding in result["findings"]) + + def test_normal_skill_archive_still_installs_with_skill_scan_disabled(self, tmp_path): + """Same disabled-scan configuration, but a small, legitimate skill: must still install.""" + zip_path = self._make_many_entry_zip(tmp_path, entry_count=5, skill_name="small-skill") + skills_root = tmp_path / "skills" + skills_root.mkdir() + storage = self._make_storage(skills_root, skill_scan_enabled=False) + + result = storage.install_skill_from_archive(zip_path) + + assert result["success"] is True + assert (skills_root / "custom" / "small-skill" / "SKILL.md").exists() + + # --------------------------------------------------------------------------- # install_skill_from_archive (full integration) # --------------------------------------------------------------------------- diff --git a/config.example.yaml b/config.example.yaml index af8fa944f..14b5ab997 100644 --- a/config.example.yaml +++ b/config.example.yaml @@ -1364,8 +1364,10 @@ skills: # Native deterministic skill safety scanning. This runs before the LLM skill # scanner on skill install/update and agent-managed skill writes. skill_scan: - # Set false to disable the new deterministic analyzers. Safe archive - # extraction and the LLM skill scanner still run. + # Set false to disable the new deterministic analyzers (nested-archive, + # secret-pattern, and other content-level checks). Safe archive extraction + # (path traversal, symlinks, executable-binary, total-size, and entry-count + # limits) and the LLM skill scanner still run unconditionally. enabled: true # Note: To restrict which skills are loaded for a specific custom agent,