diff --git a/backend/packages/harness/deerflow/skills/installer.py b/backend/packages/harness/deerflow/skills/installer.py index b32d41a17..a5020ee33 100644 --- a/backend/packages/harness/deerflow/skills/installer.py +++ b/backend/packages/harness/deerflow/skills/installer.py @@ -62,7 +62,21 @@ class SkillSecurityScanError(ValueError): def is_unsafe_zip_member(info: zipfile.ZipInfo) -> bool: - """Return True if the zip member path is absolute or attempts directory traversal.""" + """Return True if the zip member path is absolute, attempts directory + traversal, or contains a colon. + + A colon has no legitimate use in a relative archive member path — zip + entries always use ``/`` separators, and a real Windows drive prefix + (``C:\\...``) is already rejected above as absolute. But on Windows/NTFS, + a colon anywhere else in a path (e.g. ``scripts/run.sh:hidden.txt``) + addresses an Alternate Data Stream on the preceding path component + instead of creating a new file: it silently attaches extra content to + ``scripts/run.sh`` rather than creating a sibling file. That stream is + invisible to ``Path.rglob()`` / ``os.walk()``-based listing, so it would + let an archive smuggle content past directory-based security scanning + while the content still lands on disk. Reject outright rather than + trying to allow-list "safe" colon positions. + """ name = info.filename if not name: return False @@ -76,6 +90,8 @@ def is_unsafe_zip_member(info: zipfile.ZipInfo) -> bool: return True if ".." in path.parts: return True + if ":" in name: + return True return False diff --git a/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py b/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py index 6e9316a23..b7be862db 100644 --- a/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py +++ b/backend/packages/harness/deerflow/skills/skillscan/orchestrator.py @@ -43,6 +43,12 @@ _MAX_ARCHIVE_MEMBERS = 4096 _SPECS = [ RuleSpec("package-path-traversal", "CRITICAL", "Archive member path traverses outside the skill root.", "Remove parent-directory traversal from the package path."), RuleSpec("package-absolute-path", "CRITICAL", "Archive member path is absolute.", "Use relative paths inside the skill archive."), + RuleSpec( + "package-ads-stream-name", + "CRITICAL", + "Archive member path contains a colon, which on Windows/NTFS addresses an alternate data stream hidden from directory listing.", + "Remove colons from archive member paths.", + ), RuleSpec("package-symlink", "HIGH", "Package contains a symlink entry.", "Remove symlinks from the skill package."), RuleSpec("package-nested-skill-md", "CRITICAL", "Package contains a nested SKILL.md file.", "Keep exactly one SKILL.md at the skill root."), RuleSpec("package-oversized-total", "CRITICAL", "Package total uncompressed size exceeds the limit.", "Remove large files or split assets out of the skill package."), @@ -246,6 +252,8 @@ def _scan_archive_member_metadata(info: zipfile.ZipInfo, normalized: str) -> lis findings.append(_finding("package-absolute-path", file=normalized, evidence=info.filename)) elif _archive_member_traverses(info.filename): findings.append(_finding("package-path-traversal", file=normalized, evidence=info.filename)) + elif _archive_member_has_colon(info.filename): + findings.append(_finding("package-ads-stream-name", file=normalized, evidence=info.filename)) if _is_symlink_member(info): findings.append(_finding("package-symlink", file=normalized, evidence=info.filename)) parts = PurePosixPath(normalized).parts @@ -545,6 +553,18 @@ def _archive_member_traverses(name: str) -> bool: return ".." in PurePosixPath(name.replace("\\", "/")).parts +def _archive_member_has_colon(name: str) -> bool: + # A colon has no legitimate use in a relative archive member path (zip + # entries use ``/`` separators; a real Windows drive prefix is already + # caught by ``_archive_member_is_absolute``). On Windows/NTFS a colon + # elsewhere in the path addresses an Alternate Data Stream on the + # preceding path component (e.g. ``scripts/run.sh:hidden.txt`` attaches + # hidden content to ``run.sh`` instead of creating a sibling file), and + # that stream is invisible to directory-listing-based scanning. Reject + # outright rather than trying to allow-list "safe" colon positions. + return ":" in name + + def _is_symlink_member(info: zipfile.ZipInfo) -> bool: return stat.S_ISLNK(info.external_attr >> 16) diff --git a/backend/tests/test_skills_installer.py b/backend/tests/test_skills_installer.py index 5884efb9e..0a3a94a65 100644 --- a/backend/tests/test_skills_installer.py +++ b/backend/tests/test_skills_installer.py @@ -35,6 +35,14 @@ class TestIsUnsafeZipMember: info = zipfile.ZipInfo("C:\\Windows\\system32\\drivers\\etc\\hosts") assert is_unsafe_zip_member(info) is True + def test_colon_in_member_name_ntfs_ads(self): + """A colon after the first path component addresses a Windows NTFS + Alternate Data Stream (e.g. ``run.sh:hidden.txt`` hides content inside + ``run.sh`` instead of creating a new file), invisible to rglob/os.walk + based scanning. Must be rejected outright.""" + info = zipfile.ZipInfo("my-skill/scripts/run.sh:hidden.txt") + assert is_unsafe_zip_member(info) is True + def test_dotdot_traversal(self): info = zipfile.ZipInfo("foo/../../../etc/passwd") assert is_unsafe_zip_member(info) is True @@ -139,6 +147,21 @@ class TestSafeExtract: with pytest.raises(ValueError, match="unsafe"): safe_extract_skill_archive(zf, dest) + def test_rejects_ntfs_ads_colon_member(self, tmp_path): + """A zip member named like ``scripts/run.sh:hidden.txt`` is an NTFS + Alternate-Data-Stream address, not a nested path. Extraction must + reject the whole archive instead of silently attaching hidden + content to ``run.sh``.""" + zip_path = tmp_path / "ads.zip" + with zipfile.ZipFile(zip_path, "w") as zf: + zf.writestr("my-skill/scripts/run.sh", "#!/bin/sh\necho ok\n") + zf.writestr("my-skill/scripts/run.sh:hidden.txt", "HIDDEN_PAYLOAD_MARKER") + dest = tmp_path / "out" + dest.mkdir() + with zipfile.ZipFile(zip_path) as zf: + with pytest.raises(ValueError, match="unsafe"): + safe_extract_skill_archive(zf, dest) + def test_skips_symlinks(self, tmp_path): zip_path = tmp_path / "sym.zip" with zipfile.ZipFile(zip_path, "w") as zf: @@ -553,6 +576,42 @@ class TestInstallSkillFromArchive: assert not (skills_root / "custom" / "test-skill").exists() + def test_ntfs_ads_smuggling_prevented(self, tmp_path, monkeypatch): + """End-to-end regression: an archive member name like + ``scripts/run.sh:hidden.txt`` addresses a Windows NTFS Alternate Data + Stream rather than a nested file. It must be rejected by the archive + preflight scan before extraction — not installed with its payload + left invisible to directory-based scanning.""" + marker = "HIDDEN_ADS_PAYLOAD_MARKER_TEST" + zip_path = tmp_path / "ads-skill.skill" + with zipfile.ZipFile(zip_path, "w") as zf: + zf.writestr("ads-skill/SKILL.md", "---\nname: ads-skill\ndescription: A test skill\n---\n\n# ads-skill\n") + zf.writestr("ads-skill/scripts/run.sh", "#!/bin/sh\necho ok\n") + zf.writestr("ads-skill/scripts/run.sh:hidden.txt", marker) + skills_root = tmp_path / "skills" + skills_root.mkdir() + llm_calls = [] + + async def _scan(*args, **kwargs): + llm_calls.append({"args": args, "kwargs": kwargs}) + return ScanResult(decision="allow", reason="ok") + + monkeypatch.setattr("deerflow.skills.installer.scan_skill_content", _scan) + + with pytest.raises(SkillSecurityScanError) as excinfo: + get_or_new_skill_storage(skills_path=skills_root).install_skill_from_archive(zip_path) + + assert "Static security scan blocked" in str(excinfo.value) + assert excinfo.value.findings + assert excinfo.value.findings[0]["rule_id"] == "package-ads-stream-name" + assert llm_calls == [] + assert not (skills_root / "custom" / "ads-skill").exists() + # The marker must not have leaked into skills_root anywhere (e.g. a + # partially-extracted temp dir surviving past cleanup). + for path in skills_root.rglob("*"): + if path.is_file(): + assert marker not in path.read_text(encoding="utf-8", errors="ignore") + def test_nested_skill_markdown_prevents_install(self, tmp_path): zip_path = tmp_path / "test-skill.skill" with zipfile.ZipFile(zip_path, "w") as zf: diff --git a/backend/tests/test_skillscan_native.py b/backend/tests/test_skillscan_native.py index 65cb96a69..19ba7eaa6 100644 --- a/backend/tests/test_skillscan_native.py +++ b/backend/tests/test_skillscan_native.py @@ -174,6 +174,25 @@ def test_archive_preflight_reports_package_findings(tmp_path: Path) -> None: assert result["blocked"] is True +def test_archive_preflight_rejects_ntfs_ads_colon_member(tmp_path: Path) -> None: + """A member name like ``scripts/run.sh:hidden.txt`` addresses a Windows + NTFS Alternate Data Stream on ``run.sh`` rather than a nested file. Such + a stream is invisible to rglob/os.walk-based scanning once extracted, so + the archive-level preflight must block it before extraction ever runs.""" + archive = tmp_path / "demo-skill.skill" + with zipfile.ZipFile(archive, "w") as zf: + zf.writestr("demo-skill/SKILL.md", "---\nname: demo-skill\ndescription: Demo skill\n---\n") + zf.writestr("demo-skill/scripts/run.sh", "#!/bin/sh\necho ok\n") + zf.writestr("demo-skill/scripts/run.sh:hidden.txt", "HIDDEN_PAYLOAD_MARKER") + + result = scan_archive_preflight(archive) + + finding = _finding_by_rule(result["findings"], "package-ads-stream-name") + assert finding["severity"] == "CRITICAL" + assert finding["file"] == "demo-skill/scripts/run.sh:hidden.txt" + assert result["blocked"] is True + + def test_nested_zip_with_executable_member_escalates_to_critical(tmp_path: Path) -> None: archive = tmp_path / "demo-skill.skill" with zipfile.ZipFile(archive, "w") as zf: