Hyeonsang Cho 6469833886
fix(skills): close SkillScan bypasses in the skill review gate (#5431)
* fix(skills): close SkillScan bypasses in the skill review gate

The public skill review gate re-materialized a package snapshot into a
temp directory for SkillScan, but copied only entries the reader had
decoded as text and skipped every file under any evals/fixtures/
directory. Executable binaries and nested archives never reached the
package rules, and a fixture-shaped path hid any script from the scan.
Readers now keep binary bytes as content_base64, the analyzer writes
every non-symlink file byte for byte, and only eval fixture SKILL.md
samples stay exempt. Files are created exclusively, so a duplicate
archive member or a case-folded name fails the scan closed instead of
overwriting an earlier file.

SkillScan itself skipped any file that was not NUL-free UTF-8. One
Latin-1 byte in a comment hid a reverse shell from the review gate, and
a NUL byte skipped static analysis at install. Code files that fail
strict decoding now raise package-undecodable-script (HIGH) and are
analyzed over a lossy decode, so CRITICAL matches keep blocking.

"Code file" and "executable magic" were defined separately in the
installer and SkillScan and had drifted: SkillScan missed 32-bit
little-endian and fat Mach-O variants the installer blocks. Both rules
now live in skills/package_files.py, shared by the installer, the export
guard, and SkillScan.

* docs(changelog): link the skill review gate fix to #5431

* fix(skills): fail closed on bytes-less snapshot entries and skip text rules for executables

The review analyzer skipped any snapshot entry it could not turn into
bytes. Readers only emit such entries for oversized files, and they also
mark the snapshot truncated, but content_base64 is optional in the
contract, so a reader regression or a hand-built snapshot would silently
drop a file from SkillScan. An entry without bytes now fails the scan
closed (not_assessed: skillscan) unless the snapshot is truncated, and a
text entry without content no longer materializes as an empty file.

A real executable under scripts/ is a code file, so SkillScan decoded it
lossily and ran the text rules over its string tables. An OpenSSH binary
produced a CRITICAL secret-private-key finding from the key-format
banner it embeds. An undecodable file with executable magic still
reports package-undecodable-script, and its CRITICAL
package-executable-binary finding already blocks it, so it now skips the
text rules. Decodable files keep full text analysis.

---------

Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
2026-09-14 21:23:58 +08:00

330 lines
13 KiB
Python

"""Shared skill archive installation logic.
Pure business logic — no FastAPI/HTTP dependencies.
Both Gateway and Client delegate to these functions.
"""
import asyncio
import concurrent.futures
import logging
import os
import posixpath
import shutil
import stat
import zipfile
from pathlib import Path, PurePosixPath, PureWindowsPath
from deerflow.skills.package_files import is_code_path, is_executable_binary_prefix
from deerflow.skills.permissions import make_skill_tree_sandbox_readable
from deerflow.skills.security_scanner import scan_skill_content
from deerflow.skills.security_static_scanner import (
StaticFinding,
StaticScanBlockedError,
StaticScannerError,
enforce_static_scan,
scan_archive_preflight,
skill_scan_enabled,
)
logger = logging.getLogger(__name__)
_PROMPT_INPUT_DIRS = {"references", "templates"}
_PROMPT_INPUT_SUFFIXES = frozenset({".json", ".markdown", ".md", ".rst", ".txt", ".yaml", ".yml"})
class SkillAlreadyExistsError(ValueError):
"""Raised when a skill with the same name is already installed."""
class SkillSecurityScanError(ValueError):
"""Raised when a skill archive fails security scanning."""
findings: list[StaticFinding]
skill_name: str | None
def __init__(self, message: str, *, findings: list[StaticFinding] | None = None, skill_name: str | None = None) -> None:
super().__init__(message)
self.findings = [dict(finding) for finding in (findings or [])]
self.skill_name = skill_name
def is_unsafe_zip_member(info: zipfile.ZipInfo) -> bool:
"""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
normalized = name.replace("\\", "/")
if normalized.startswith("/"):
return True
path = PurePosixPath(normalized)
if path.is_absolute():
return True
if PureWindowsPath(name).is_absolute():
return True
if ".." in path.parts:
return True
if ":" in name:
return True
return False
def is_symlink_member(info: zipfile.ZipInfo) -> bool:
"""Detect symlinks based on the external attributes stored in the ZipInfo."""
mode = info.external_attr >> 16
return stat.S_ISLNK(mode)
def should_ignore_archive_entry(path: Path) -> bool:
"""Return True for macOS metadata dirs and dotfiles."""
return path.name.startswith(".") or path.name == "__MACOSX"
def resolve_skill_dir_from_archive(temp_path: Path) -> Path:
"""Locate the skill root directory from extracted archive contents.
Filters out macOS metadata (__MACOSX) and dotfiles (.DS_Store).
Returns:
Path to the skill directory.
Raises:
ValueError: If the archive is empty after filtering.
"""
items = [p for p in temp_path.iterdir() if not should_ignore_archive_entry(p)]
if not items:
raise ValueError("Skill archive is empty")
if len(items) == 1 and items[0].is_dir():
return items[0]
return temp_path
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.
Protections:
- 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, entry count, or size limit exceeded.
"""
dest_root = dest_path.resolve()
total_written = 0
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}")
if is_symlink_member(info):
logger.warning("Skipping symlink entry in skill archive: %s", info.filename)
continue
normalized_name = posixpath.normpath(info.filename.replace("\\", "/"))
member_path = dest_root.joinpath(*PurePosixPath(normalized_name).parts)
if not member_path.resolve().is_relative_to(dest_root):
raise ValueError(f"Zip entry escapes destination: {info.filename!r}")
member_path.parent.mkdir(parents=True, exist_ok=True)
if info.is_dir():
member_path.mkdir(parents=True, exist_ok=True)
continue
with zip_ref.open(info) as src, member_path.open("wb") as dst:
first_chunk = True
while chunk := src.read(65536):
if first_chunk and is_executable_binary_prefix(chunk):
raise ValueError(f"Archive contains executable binary member: {info.filename!r}")
first_chunk = False
total_written += len(chunk)
if total_written > max_total_size:
raise ValueError("Skill archive is too large or appears highly compressed.")
dst.write(chunk)
if os.name == "posix":
member_path.chmod(0o755 if (info.external_attr >> 16) & 0o111 else 0o644)
def _is_script_support_file(rel_path: Path) -> bool:
return bool(rel_path.parts) and rel_path.parts[0] == "scripts"
def _should_scan_support_file(rel_path: Path) -> bool:
if _is_script_support_file(rel_path):
return True
return bool(rel_path.parts) and rel_path.parts[0] in _PROMPT_INPUT_DIRS and rel_path.suffix.lower() in _PROMPT_INPUT_SUFFIXES
def _has_shebang(path: Path) -> bool:
try:
with path.open("rb") as f:
return f.read(2) == b"#!"
except OSError:
return False
async def _is_code_file(path: Path, rel_path: Path) -> bool:
"""Classify code files anywhere in the tree for the executable scan policy.
Applies :func:`is_code_file` lazily: name checks are pure and stay on the
event loop; only the shebang sniff for extensionless files reads the file
and is offloaded.
"""
if is_code_path(rel_path):
return True
return not rel_path.suffix and await asyncio.to_thread(_has_shebang, path)
def _move_staged_skill_into_reserved_target(staging_target: Path, target: Path) -> None:
installed = False
reserved = False
try:
target.mkdir(mode=0o700)
reserved = True
for child in staging_target.iterdir():
shutil.move(str(child), target / child.name)
make_skill_tree_sandbox_readable(target)
installed = True
except FileExistsError as e:
raise SkillAlreadyExistsError(f"Skill '{target.name}' already exists") from e
finally:
if reserved and not installed and target.exists():
shutil.rmtree(target)
def _findings_for_file(findings: list[StaticFinding], rel_path: str) -> list[StaticFinding]:
return [finding for finding in findings if finding.get("file") in {rel_path, None}]
async def _scan_skill_file_or_raise(skill_dir: Path, path: Path, skill_name: str, *, executable: bool, static_findings: list[StaticFinding] | None = None) -> None:
rel_path = path.relative_to(skill_dir).as_posix()
location = f"{skill_name}/{rel_path}"
try:
content = await asyncio.to_thread(path.read_text, encoding="utf-8")
except UnicodeDecodeError as e:
raise SkillSecurityScanError(f"Security scan failed for skill '{skill_name}': {location} must be valid UTF-8") from e
try:
result = await scan_skill_content(content, executable=executable, location=location, static_findings=static_findings or [])
except Exception as e:
raise SkillSecurityScanError(f"Security scan failed for {location}: {e}") from e
decision = getattr(result, "decision", None)
reason = str(getattr(result, "reason", "") or "No reason provided.")
if decision == "block":
if rel_path == "SKILL.md":
raise SkillSecurityScanError(f"Security scan blocked skill '{skill_name}': {reason}")
raise SkillSecurityScanError(f"Security scan blocked {location}: {reason}")
if executable and decision != "allow":
raise SkillSecurityScanError(f"Security scan rejected executable {location}: {reason}")
if decision not in {"allow", "warn"}:
raise SkillSecurityScanError(f"Security scan failed for {location}: invalid scanner decision {decision!r}")
def scan_archive_preflight_or_raise(archive_path: Path, *, app_config=None) -> None:
if not skill_scan_enabled(app_config):
return
result = scan_archive_preflight(archive_path)
if result["blocked"]:
critical = [finding for finding in result["findings"] if finding["severity"] == "CRITICAL"]
raise SkillSecurityScanError(
f"Static security scan blocked unsafe skill archive: {format_static_archive_findings(critical)}",
findings=critical,
skill_name=None,
)
def format_static_archive_findings(findings: list[StaticFinding]) -> str:
return "; ".join(f"{finding['rule_id']} ({finding['severity']}) at {finding.get('file') or '<archive>'}: {finding['message']}" for finding in findings)
async def _scan_static_skill_archive_or_raise(skill_dir: Path, skill_name: str, *, app_config=None) -> list[StaticFinding]:
try:
return await asyncio.to_thread(enforce_static_scan, skill_dir, skill_name=skill_name, app_config=app_config)
except StaticScanBlockedError as e:
raise SkillSecurityScanError(str(e), findings=e.findings, skill_name=e.skill_name) from e
except StaticScannerError as e:
raise SkillSecurityScanError(f"Static security scan failed for skill '{skill_name}': {e}", skill_name=skill_name) from e
def _collect_scannable_files(skill_dir: Path) -> list[Path]:
"""Enumerate archive files for scanning (blocking; run off the event loop)."""
return [candidate for candidate in sorted(skill_dir.rglob("*")) if candidate.is_file()]
async def _scan_skill_archive_contents_or_raise(skill_dir: Path, skill_name: str, *, app_config=None) -> list[StaticFinding]:
"""Run the skill security scanner against all installable text and script files."""
static_findings = await _scan_static_skill_archive_or_raise(skill_dir, skill_name, app_config=app_config)
skill_md = skill_dir / "SKILL.md"
await _scan_skill_file_or_raise(skill_dir, skill_md, skill_name, executable=False, static_findings=_findings_for_file(static_findings, "SKILL.md"))
for path in await asyncio.to_thread(_collect_scannable_files, skill_dir):
rel_path = path.relative_to(skill_dir)
if rel_path == Path("SKILL.md"):
continue
if path.name == "SKILL.md":
raise SkillSecurityScanError(f"Security scan failed for skill '{skill_name}': nested SKILL.md is not allowed at {skill_name}/{rel_path.as_posix()}")
rel_path_posix = rel_path.as_posix()
if await _is_code_file(path, rel_path):
await _scan_skill_file_or_raise(
skill_dir,
path,
skill_name,
executable=True,
static_findings=_findings_for_file(static_findings, rel_path_posix),
)
elif _should_scan_support_file(rel_path):
await _scan_skill_file_or_raise(
skill_dir,
path,
skill_name,
executable=False,
static_findings=_findings_for_file(static_findings, rel_path_posix),
)
return static_findings
def _run_async_install(coro):
try:
loop = asyncio.get_running_loop()
except RuntimeError:
loop = None
if loop is not None and loop.is_running():
with concurrent.futures.ThreadPoolExecutor(max_workers=1) as executor:
return executor.submit(asyncio.run, coro).result()
return asyncio.run(coro)