-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(input-handler): bound URL, zip, and git ingest paths #164
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,7 +23,14 @@ | |
| - Single markdown files | ||
| - Local directories | ||
|
|
||
| Ported from legacy implementation. | ||
| Each remote/archive ingest path is bounded by ``INGEST_MAX_BYTES`` and | ||
| ``INGEST_MAX_ZIP_MEMBERS`` so that the per-file analysis caps downstream | ||
| of ``InputHandler.resolve()`` are not defeated by an oversized download, | ||
| a zip bomb, or a too-large git clone. This file fails closed on any | ||
| ingest budget breach (closes #21 / #131). | ||
|
|
||
| URL-based ingest is additionally gated by an SSRF host allowlist plus a | ||
| private-IP check, and zip extraction is guarded against zip-slip. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
@@ -62,6 +69,30 @@ | |
| } | ||
| ) | ||
|
|
||
| # Hard ceiling on what any single ingest path can pull into the temp dir. | ||
| # Sized above the per-file analysis cap (``MAX_FILE_BYTES`` = 1 MB) so a | ||
| # legitimate multi-file skill is not blocked at ingest, but tight enough | ||
| # to bound memory / disk DoS from a malicious source. | ||
| INGEST_MAX_BYTES = 100 * 1024 * 1024 # 100 MiB | ||
|
|
||
| # Hard ceiling on the number of members in a zip we are willing to | ||
| # extract. Catches the "many tiny files" zip-bomb variant where each | ||
| # entry is small but the entry count itself exhausts the filesystem. | ||
| INGEST_MAX_ZIP_MEMBERS = 10_000 | ||
|
|
||
| # Chunk size for streaming HTTP downloads. Small enough that the | ||
| # byte-count breach check fires promptly; large enough to keep syscall | ||
| # overhead reasonable on legitimate inputs. | ||
| _DOWNLOAD_CHUNK_BYTES = 64 * 1024 | ||
|
|
||
|
|
||
| class IngestLimitExceededError(ValueError): | ||
| """Raised when an ingest path exceeds an ``INGEST_MAX_*`` budget. | ||
|
|
||
| Subclass of ``ValueError`` so existing callers that catch | ||
| ``ValueError`` from ``InputHandler.resolve()`` continue to work. | ||
| """ | ||
|
|
||
|
|
||
| def _is_private_ip(host: str) -> bool: | ||
| """Return True if host resolves to a private/reserved IP address.""" | ||
|
|
@@ -103,8 +134,10 @@ def resolve(self, input_path: str) -> tuple[Path, str]: | |
| source_type is one of: "git", "url", "zip", "file", "directory" | ||
|
|
||
| Raises: | ||
| ValueError: If input type cannot be determined | ||
| FileNotFoundError: If local path doesn't exist | ||
| ValueError: If input type cannot be determined, or if an | ||
| ingest path exceeds ``INGEST_MAX_BYTES`` / | ||
| ``INGEST_MAX_ZIP_MEMBERS`` (``IngestLimitExceededError``). | ||
| FileNotFoundError: If local path doesn't exist. | ||
| """ | ||
| input_path = input_path.strip() | ||
|
|
||
|
|
@@ -191,7 +224,7 @@ def _validate_url_host(self, url: str, allowed_hosts: frozenset[str]) -> str: | |
| return host | ||
|
|
||
| def _clone_git(self, url: str) -> Path: | ||
| """Clone a Git repository to a temporary directory.""" | ||
| """Clone a Git repository to a temporary directory, bounded by ``INGEST_MAX_BYTES``.""" | ||
| self._validate_url_host(url, ALLOWED_GIT_HOSTS) | ||
| temp_dir = self._get_temp_dir() | ||
| clone_dir = temp_dir / "repo" | ||
|
|
@@ -214,44 +247,139 @@ def _clone_git(self, url: str) -> Path: | |
| raise ValueError( | ||
| "Git is not installed. Please install git to scan repositories." | ||
| ) from None | ||
|
|
||
| # Post-clone size check: a successful --depth 1 clone may still | ||
| # land an arbitrarily large tree on disk before we can measure | ||
| # it, so this is a fail-closed cap rather than a hard prefilter. | ||
| # Residual window: within the 60s clone timeout an attacker can | ||
| # transiently consume up to whatever the network + disk let | ||
| # through before this check runs; bounded by the timeout, but | ||
| # not zero. ``.git/`` objects are counted toward the cap, so a | ||
| # legitimate repo with a working tree just under ``INGEST_MAX_BYTES`` | ||
| # can still be rejected once packfiles are added. | ||
| total = _directory_size_bytes(clone_dir) | ||
| if total > INGEST_MAX_BYTES: | ||
| shutil.rmtree(clone_dir, ignore_errors=True) | ||
| logger.warning( | ||
| "Git clone of %s exceeded ingest cap: %d > %d bytes", | ||
| url, | ||
| total, | ||
| INGEST_MAX_BYTES, | ||
| ) | ||
| raise IngestLimitExceededError( | ||
| f"Git clone exceeded ingest cap: {total} bytes > " | ||
| f"INGEST_MAX_BYTES ({INGEST_MAX_BYTES})" | ||
| ) | ||
| return clone_dir | ||
|
|
||
| def _download_file(self, url: str) -> Path: | ||
| """Download a file from URL to a temporary directory.""" | ||
| """Download a file from URL to a temporary directory. | ||
|
|
||
| Streams the body to disk in chunks while running a byte counter. | ||
| The cap check fires before each chunk is written, so a breach | ||
| aborts immediately without accumulating the body in memory. A | ||
| partial file produced by a mid-stream breach is removed before | ||
| the exception propagates. | ||
| """ | ||
| self._validate_url_host(url, ALLOWED_DOWNLOAD_HOSTS) | ||
| temp_dir = self._get_temp_dir() | ||
| parsed = urlparse(url) | ||
| filename = Path(parsed.path).name or "SKILL.md" | ||
| # Write to a stable target path inside the temp dir so we can | ||
| # rename / move it after the download succeeds without ever | ||
| # holding the body in memory. Use a sentinel name for the | ||
| # download itself; we rename / replace at the end. | ||
| download_path = temp_dir / "_download.partial" | ||
| content_type = "" | ||
| try: | ||
| with httpx.Client(follow_redirects=False, timeout=30) as client: | ||
| response = client.get(url) | ||
| response.raise_for_status() | ||
| content = response.content | ||
| with client.stream("GET", url) as response: | ||
| response.raise_for_status() | ||
| content_type = response.headers.get("content-type", "") | ||
| # Cheap up-front check: trust Content-Length when the | ||
| # server provides it, so we abort before reading any | ||
| # body bytes. Streaming check below covers the case | ||
| # where the header is missing or wrong. | ||
| declared = response.headers.get("content-length") | ||
| if declared is not None: | ||
| try: | ||
| declared_bytes = int(declared) | ||
| except ValueError: | ||
| # Malformed header — fall through to the | ||
| # streamed byte counter, which is authoritative. | ||
| declared_bytes = None | ||
| if declared_bytes is not None and declared_bytes > INGEST_MAX_BYTES: | ||
| raise IngestLimitExceededError( | ||
| f"Download exceeded ingest cap: " | ||
| f"Content-Length {declared} bytes > " | ||
| f"INGEST_MAX_BYTES ({INGEST_MAX_BYTES})" | ||
| ) | ||
|
|
||
| received = 0 | ||
| with download_path.open("wb") as out: | ||
| for chunk in response.iter_bytes(_DOWNLOAD_CHUNK_BYTES): | ||
| received += len(chunk) | ||
| if received > INGEST_MAX_BYTES: | ||
| raise IngestLimitExceededError( | ||
| f"Download exceeded ingest cap: streamed " | ||
| f"{received} bytes > INGEST_MAX_BYTES " | ||
| f"({INGEST_MAX_BYTES})" | ||
| ) | ||
| out.write(chunk) | ||
| except httpx.HTTPError as e: | ||
| # Best-effort cleanup of any partial download. | ||
| download_path.unlink(missing_ok=True) | ||
| logger.warning("Download failed for %s: %s", url, e) | ||
| raise ValueError(f"Failed to download file: {e}") from e | ||
| if filename.endswith(".zip") or ( | ||
| response.headers.get("content-type", "").startswith("application/zip") | ||
| ): | ||
| except IngestLimitExceededError: | ||
| # Don't leave the partial bomb on disk. | ||
| download_path.unlink(missing_ok=True) | ||
| raise | ||
|
|
||
| is_zip = filename.endswith(".zip") or content_type.startswith("application/zip") | ||
| if is_zip: | ||
| zip_path = temp_dir / "download.zip" | ||
| zip_path.write_bytes(content) | ||
| download_path.replace(zip_path) | ||
| return self._extract_zip(zip_path) | ||
| file_path = temp_dir / filename | ||
| file_path.write_bytes(content) | ||
| download_path.replace(file_path) | ||
| return temp_dir | ||
|
|
||
| def _extract_zip(self, zip_path: Path) -> Path: | ||
| """Extract a zip file to a temporary directory with path traversal protection.""" | ||
| """Extract a zip file, bounded by ``INGEST_MAX_BYTES`` and ``INGEST_MAX_ZIP_MEMBERS``. | ||
|
|
||
| Sums ``ZipInfo.file_size`` (uncompressed size) across all members | ||
| before extracting and refuses to extract if either the total or | ||
| the member count exceeds the cap. This rejects classic zip | ||
| bombs (small archive, huge declared uncompressed size) without | ||
| materialising any of the bomb on disk. A zip-slip check on each | ||
| member name is applied before extraction to reject entries whose | ||
| resolved path escapes the extraction directory. | ||
| """ | ||
| if not zip_path.exists(): | ||
| raise FileNotFoundError(f"Zip file not found: {zip_path}") from None | ||
| temp_dir = self._get_temp_dir() | ||
| extract_dir = temp_dir / "extracted" | ||
| extract_dir.mkdir(exist_ok=True) | ||
| try: | ||
| with zipfile.ZipFile(zip_path, "r") as zf: | ||
| infos = zf.infolist() | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Main's
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed in the rebase.
So the size caps fire first (cheap, protects against bombs before we even touch member names), then zip-slip runs, then extract. Nothing was dropped from main's protections. |
||
| if len(infos) > INGEST_MAX_ZIP_MEMBERS: | ||
| raise IngestLimitExceededError( | ||
| f"Zip exceeded ingest cap: {len(infos)} members > " | ||
| f"INGEST_MAX_ZIP_MEMBERS ({INGEST_MAX_ZIP_MEMBERS})" | ||
| ) | ||
| total_uncompressed = sum(info.file_size for info in infos) | ||
| if total_uncompressed > INGEST_MAX_BYTES: | ||
| raise IngestLimitExceededError( | ||
| f"Zip exceeded ingest cap: uncompressed " | ||
| f"{total_uncompressed} bytes > INGEST_MAX_BYTES " | ||
| f"({INGEST_MAX_BYTES})" | ||
| ) | ||
| extract_root = extract_dir.resolve() | ||
| for member in zf.namelist(): | ||
| member_path = (extract_dir / member).resolve() | ||
| if not str(member_path).startswith(str(extract_dir.resolve())): | ||
| if not str(member_path).startswith(str(extract_root)): | ||
| raise ValueError( | ||
| f"Zip entry '{member}' would escape extraction directory (zip-slip). " | ||
| "Archive is potentially malicious." | ||
|
|
@@ -273,3 +401,25 @@ def _wrap_single_file(self, file_path: Path) -> Path: | |
| dest = temp_dir / file_path.name | ||
| shutil.copy2(file_path, dest) | ||
| return temp_dir | ||
|
|
||
|
|
||
| def _directory_size_bytes(path: Path) -> int: | ||
| """Return the total size of all regular files under *path*, in bytes. | ||
|
|
||
| Symlinks are explicitly skipped via ``Path.is_symlink()`` — note that | ||
| ``Path.is_file()`` follows symlinks and would otherwise return | ||
| ``True`` for a symlink pointing at a regular file, so the | ||
| ``not p.is_symlink()`` guard is load-bearing and must not be removed. | ||
| This is what prevents a malicious symlink to ``/dev/zero`` (or any | ||
| large file outside the walked tree) from inflating the count. | ||
| """ | ||
| total = 0 | ||
| for p in path.rglob("*"): | ||
| if p.is_file() and not p.is_symlink(): | ||
| try: | ||
| total += p.stat().st_size | ||
| except OSError: | ||
| # File disappeared mid-walk (race with concurrent fs ops). | ||
| # Skip rather than fail the whole ingest. | ||
| continue | ||
| return total | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note this check is post-hoc: the entire clone already landed on disk before it runs, so disk is transiently consumable up to whatever fits in the 60s timeout. Acceptable as a bound, but worth a comment documenting the residual window. Also,
.git/objects count toward the cap, so a legitimate repo with a working tree just under 100 MiB may be rejected.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documented as requested. Added an in-code comment right above the size check in
_clone_git:Deliberately not fixing either issue in this PR:
git clone --filter=blob:limit=…(partial clone) plus streaming the pack, which is a much larger change and mainly buys tighter bounding on the same disk that the 60s timeout already caps. Documented, not addressed..git/counting toward the cap is arguably correct — an attacker who ships a bloated packfile with a small working tree would otherwise slip through. Excluding.git/would need a separate size accounting pass. Also documented for future callers who hit it on legitimate repos.Happy to spin either out as follow-up issues if you'd rather have them tracked.