mirror of
https://github.com/github/spec-kit.git
synced 2026-08-03 06:26:30 +08:00
Harden extension URL download cache against symlink and junction races (#3869)
* fix(extensions): harden URL download cache Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8f71e02a-bc64-4593-b305-2554debe96f6 * fix(extensions): retain secure archive descriptor Assisted-by: GitHub Copilot (model: GPT-5.6 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8f71e02a-bc64-4593-b305-2554debe96f6 * Harden extension URL cache anchor opens Assisted-by: GitHub Copilot (model: MAI-Code-1-Flash, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8f71e02a-bc64-4593-b305-2554debe96f6 * Use descriptor-safe mkdir for cache components Assisted-by: GitHub Copilot (model: MAI-Code-1-Flash, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8f71e02a-bc64-4593-b305-2554debe96f6 * Harden extension URL download cache Assisted-by: GitHub Copilot (model: MAI-Code-1-Flash, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8f71e02a-bc64-4593-b305-2554debe96f6 * Align extension manifest regression expectation Assisted-by: GitHub Copilot (model: MAI-Code-1-Flash, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8f71e02a-bc64-4593-b305-2554debe96f6 * Make download cache leaf anonymous to remove cleanup TOCTOU Address review: the best-effort cleanup walk re-derived the downloads directory by path, so a cache ancestor swapped after the archive was opened could redirect os.unlink to a replacement leaf, and it silently no-op'd (failing open) on platforms without descriptor-relative unlink. _safe_open_download_zip now unlinks the exclusively-created leaf immediately via the same directory descriptor, returning an fd backed by an anonymous inode. Installation already consumes that descriptor through archive_file, so the on-disk pathname is never reopened and no cleanup walk is needed. The capability gate additionally requires os.unlink in os.supports_dir_fd, so unsupported platforms fail closed. Removed the now-unused _safe_unlink_download_zip helper and its cleanup finally, and updated the tests accordingly. Assisted-by: GitHub Copilot (model: MAI-Code-1-Flash, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8f71e02a-bc64-4593-b305-2554debe96f6 * Fix Windows test matrix for cache hardening tests The hardened cache primitives fail closed on platforms without dir_fd/ O_NOFOLLOW, so on the windows-latest matrix several tests errored instead of exercising POSIX behavior: - test_symlinked_cache_ancestor_is_refused and test_cache_ancestor_resolving_outside_project_is_refused called _validate_safe_cache_dir directly and expected typer.Exit, but on Windows it raises NotImplementedError first. Guard both with _require_secure_dir_fd() so they skip where the primitive is unavailable. - test_safe_open_fails_closed_without_atomic_platform_support built its download dir via _validate_safe_cache_dir, which itself fails closed on Windows; construct the directory directly so the assertion targets _safe_open_download_zip's platform gate in isolation. - The _open_test_download_zip stand-in unlinked a still-open file, which raises PermissionError on Windows. Use O_TEMPORARY there (auto-delete on close) and keep immediate unlink on POSIX. Assisted-by: GitHub Copilot (model: MAI-Code-1-Flash, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8f71e02a-bc64-4593-b305-2554debe96f6 * Support Windows in extension URL download-cache hardening Replace the fail-closed NotImplementedError on platforms lacking dir_fd with a portable, still-hardened download path so `specify extension add --from <url>` works on Windows instead of rejecting the install. - `_validate_safe_cache_dir` now dispatches to a POSIX dir_fd + O_NOFOLLOW walk when available, and otherwise a portable path-wise walk that rejects symlink/junction components before and after each mkdir and requires every component to resolve back under the project root. - `_safe_open_download_zip` keeps the POSIX anonymous-inode create/unlink and adds a portable leaf create using O_EXCL + O_TEMPORARY (auto-delete on close) plus a post-open fstat/lstat inode-identity check to detect a leaf swapped underneath us. Installation still consumes only the open descriptor, so the cache pathname is never reopened. - Detect the symlink-refusal case via errno (ELOOP/ENOTDIR/EMLINK) instead of FileExistsError, and add O_CLOEXEC to the descriptor-walk opens. - Drop the now-unreachable NotImplementedError handling in the --from branch. - Tests: cover the portable path (success, symlinked-leaf refusal, symlinked ancestor refusal, full --from install) and keep the POSIX-only cases guarded. Assisted-by: GitHub Copilot (model: Claude Opus 4.8, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8f71e02a-bc64-4593-b305-2554debe96f6 --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 8f71e02a-bc64-4593-b305-2554debe96f6
This commit is contained in:
Manfred Riem
@@ -20,7 +20,7 @@ import tempfile
|
||||
from dataclasses import dataclass
|
||||
from datetime import datetime, timezone
|
||||
from pathlib import Path
|
||||
from typing import Any, Callable, Dict, List, Optional, Set
|
||||
from typing import Any, BinaryIO, Callable, Dict, List, Optional, Set
|
||||
|
||||
import pathspec
|
||||
import yaml
|
||||
@@ -2428,6 +2428,8 @@ class ExtensionManager:
|
||||
speckit_version: str,
|
||||
priority: int = 10,
|
||||
force: bool = False,
|
||||
*,
|
||||
archive_file: BinaryIO | None = None,
|
||||
) -> ExtensionManifest:
|
||||
"""Install extension from ZIP file.
|
||||
|
||||
@@ -2437,6 +2439,8 @@ class ExtensionManager:
|
||||
priority: Resolution priority (lower = higher precedence, default 10)
|
||||
force: If True and extension is already installed, remove it first
|
||||
before proceeding with installation
|
||||
archive_file: Already-open archive stream to consume instead of
|
||||
reopening ``zip_path``
|
||||
|
||||
Returns:
|
||||
Installed extension manifest
|
||||
@@ -2452,7 +2456,12 @@ class ExtensionManager:
|
||||
with tempfile.TemporaryDirectory() as tmpdir:
|
||||
temp_path = Path(tmpdir)
|
||||
|
||||
safe_extract_zip(zip_path, temp_path, error_type=ValidationError)
|
||||
safe_extract_zip(
|
||||
zip_path,
|
||||
temp_path,
|
||||
archive_file=archive_file,
|
||||
error_type=ValidationError,
|
||||
)
|
||||
|
||||
# Find extension directory (may be nested)
|
||||
extension_dir = temp_path
|
||||
|
||||
@@ -8,9 +8,11 @@ which re-fetch from the parent package at call time so test monkeypatching of
|
||||
"""
|
||||
from __future__ import annotations
|
||||
|
||||
import errno
|
||||
import hashlib
|
||||
import os
|
||||
import shutil
|
||||
import stat
|
||||
import tempfile
|
||||
import zipfile
|
||||
from pathlib import Path
|
||||
@@ -437,6 +439,278 @@ def catalog_remove(
|
||||
console.print("\n[dim]No catalogs remain in config. Built-in defaults will be used.[/dim]")
|
||||
|
||||
|
||||
# Relative path, below the project root, of the extension URL download cache.
|
||||
_CACHE_REL_PARTS = (".specify", "extensions", ".cache", "downloads")
|
||||
|
||||
|
||||
def _has_secure_dir_fd() -> bool:
|
||||
"""Whether this platform supports the strongest (POSIX) hardening path.
|
||||
|
||||
The descriptor-anchored walk needs ``O_NOFOLLOW`` plus ``dir_fd`` support
|
||||
for ``os.open``/``os.mkdir``/``os.unlink``. When any of those is missing
|
||||
(notably on Windows) the caller falls back to the portable path-wise walk,
|
||||
which reproduces the same guarantees using symlink/reparse-point rejection,
|
||||
resolve-under-root containment checks, and post-open inode-identity
|
||||
verification instead of file descriptors.
|
||||
"""
|
||||
return bool(
|
||||
getattr(os, "O_NOFOLLOW", 0)
|
||||
and os.open in os.supports_dir_fd
|
||||
and os.mkdir in os.supports_dir_fd
|
||||
and os.unlink in os.supports_dir_fd
|
||||
)
|
||||
|
||||
|
||||
def _is_symlink_refusal_errno(exc: OSError) -> bool:
|
||||
"""Whether an ``os.open``/``os.mkdir`` error means a component is a symlink.
|
||||
|
||||
Opening an ``O_NOFOLLOW`` path whose final component is a symlink raises
|
||||
``ELOOP`` on Linux and ``EMLINK`` on some BSDs, while a symlinked component
|
||||
that no longer resolves to a directory surfaces as ``ENOTDIR``.
|
||||
"""
|
||||
return exc.errno in (errno.ELOOP, errno.ENOTDIR, getattr(errno, "EMLINK", -1))
|
||||
|
||||
|
||||
def _verify_leaf_identity(fd: int, path: Path) -> None:
|
||||
"""Confirm ``fd`` still refers to the regular file at ``path``.
|
||||
|
||||
Mirrors the workflow installer's staged-file check: comparing the open
|
||||
descriptor's ``fstat`` against a ``lstat`` of the pathname detects a leaf
|
||||
that was swapped for a symlink/reparse point between creation and use, so
|
||||
the portable (dir_fd-less) path is not vulnerable to an ancestor swap race.
|
||||
"""
|
||||
path_stat = path.stat(follow_symlinks=False)
|
||||
open_stat = os.fstat(fd)
|
||||
if (
|
||||
not stat.S_ISREG(path_stat.st_mode)
|
||||
or path_stat.st_dev != open_stat.st_dev
|
||||
or path_stat.st_ino != open_stat.st_ino
|
||||
):
|
||||
raise OSError(
|
||||
errno.ENOTDIR, "Download file changed between creation and open"
|
||||
)
|
||||
|
||||
|
||||
def _validate_safe_cache_dir(project_root: Path) -> Path:
|
||||
"""Create and validate the extension URL download cache one component at a
|
||||
time, refusing symlinked/junctioned components on every supported platform."""
|
||||
download_dir = project_root.joinpath(*_CACHE_REL_PARTS)
|
||||
try:
|
||||
if _has_secure_dir_fd():
|
||||
_validate_cache_dir_via_dir_fd(project_root, download_dir)
|
||||
else:
|
||||
_validate_cache_dir_via_paths(project_root, download_dir)
|
||||
except typer.Exit:
|
||||
raise
|
||||
except FileExistsError:
|
||||
console.print(
|
||||
"[red]Error:[/red] Refusing to use symlinked download cache directory"
|
||||
)
|
||||
raise typer.Exit(1)
|
||||
except OSError as exc:
|
||||
if _is_symlink_refusal_errno(exc):
|
||||
console.print(
|
||||
"[red]Error:[/red] Refusing to use symlinked download cache directory"
|
||||
)
|
||||
raise typer.Exit(1)
|
||||
console.print(
|
||||
"[red]Error:[/red] Could not prepare download cache directory: "
|
||||
f"{_escape_markup(str(exc))}"
|
||||
)
|
||||
raise typer.Exit(1)
|
||||
|
||||
return download_dir
|
||||
|
||||
|
||||
def _validate_cache_dir_via_dir_fd(project_root: Path, download_dir: Path) -> None:
|
||||
"""POSIX cache-dir walk anchored on ``dir_fd`` + ``O_NOFOLLOW`` descriptors."""
|
||||
o_nofollow = getattr(os, "O_NOFOLLOW", 0)
|
||||
o_directory = getattr(os, "O_DIRECTORY", 0)
|
||||
o_cloexec = getattr(os, "O_CLOEXEC", 0)
|
||||
walk_flags = os.O_RDONLY | o_directory | o_nofollow | o_cloexec
|
||||
|
||||
project_root_resolved = project_root.resolve()
|
||||
parent_fd = os.open(project_root, walk_flags)
|
||||
current_path = project_root
|
||||
try:
|
||||
for part in _CACHE_REL_PARTS:
|
||||
current_path = current_path / part
|
||||
|
||||
try:
|
||||
child_fd = os.open(part, walk_flags, dir_fd=parent_fd)
|
||||
except FileNotFoundError:
|
||||
try:
|
||||
os.mkdir(part, dir_fd=parent_fd)
|
||||
except FileExistsError:
|
||||
pass
|
||||
child_fd = os.open(part, walk_flags, dir_fd=parent_fd)
|
||||
|
||||
try:
|
||||
current_path.resolve().relative_to(project_root_resolved)
|
||||
except (OSError, ValueError):
|
||||
try:
|
||||
os.close(child_fd)
|
||||
except OSError:
|
||||
pass
|
||||
console.print(
|
||||
"[red]Error:[/red] Download cache directory escapes project root"
|
||||
)
|
||||
raise typer.Exit(1)
|
||||
|
||||
os.close(parent_fd)
|
||||
parent_fd = child_fd
|
||||
finally:
|
||||
if parent_fd >= 0:
|
||||
try:
|
||||
os.close(parent_fd)
|
||||
except OSError:
|
||||
pass
|
||||
|
||||
|
||||
def _validate_cache_dir_via_paths(project_root: Path, download_dir: Path) -> None:
|
||||
"""Portable cache-dir walk for platforms without ``dir_fd`` (e.g. Windows).
|
||||
|
||||
Each component is created individually while a symlink/junction is rejected
|
||||
both before and after creation, and every component is required to resolve
|
||||
back under the project root so a mount-point alias or reparse point cannot
|
||||
redirect the cache outside the project.
|
||||
"""
|
||||
project_root_resolved = project_root.resolve()
|
||||
current_path = project_root
|
||||
for part in _CACHE_REL_PARTS:
|
||||
current_path = current_path / part
|
||||
|
||||
if current_path.is_symlink():
|
||||
console.print(
|
||||
"[red]Error:[/red] Refusing to use symlinked download cache directory"
|
||||
)
|
||||
raise typer.Exit(1)
|
||||
|
||||
try:
|
||||
current_path.mkdir()
|
||||
except FileExistsError:
|
||||
pass
|
||||
|
||||
# Re-check after creation: a component swapped for a symlink/junction
|
||||
# (or an existing non-directory) between the check and mkdir is caught
|
||||
# here before the walk descends into it.
|
||||
if current_path.is_symlink() or not current_path.is_dir():
|
||||
console.print(
|
||||
"[red]Error:[/red] Refusing to use symlinked download cache directory"
|
||||
)
|
||||
raise typer.Exit(1)
|
||||
|
||||
try:
|
||||
current_path.resolve().relative_to(project_root_resolved)
|
||||
except (OSError, ValueError):
|
||||
console.print(
|
||||
"[red]Error:[/red] Download cache directory escapes project root"
|
||||
)
|
||||
raise typer.Exit(1)
|
||||
|
||||
|
||||
def _safe_open_download_zip(
|
||||
project_root: Path, download_dir: Path, zip_filename: str
|
||||
) -> int:
|
||||
"""Exclusively create a download ZIP and return an owned descriptor.
|
||||
|
||||
The archive never persists as a nameable on-disk file: the POSIX path
|
||||
unlinks the leaf immediately after exclusive creation (anonymous inode),
|
||||
while the portable path opens it with ``O_TEMPORARY`` so the OS deletes it
|
||||
when the last handle closes. Installation proceeds entirely through the
|
||||
returned descriptor, removing the pathname-reopen and cleanup-walk TOCTOU
|
||||
classes on every supported platform.
|
||||
"""
|
||||
if _has_secure_dir_fd():
|
||||
return _open_download_zip_via_dir_fd(
|
||||
project_root, download_dir, zip_filename
|
||||
)
|
||||
return _open_download_zip_via_paths(project_root, download_dir, zip_filename)
|
||||
|
||||
|
||||
def _open_download_zip_via_dir_fd(
|
||||
project_root: Path, download_dir: Path, zip_filename: str
|
||||
) -> int:
|
||||
"""POSIX leaf create: descriptor walk, ``O_EXCL`` create, immediate unlink."""
|
||||
o_nofollow = getattr(os, "O_NOFOLLOW", 0)
|
||||
o_directory = getattr(os, "O_DIRECTORY", 0)
|
||||
o_cloexec = getattr(os, "O_CLOEXEC", 0)
|
||||
walk_flags = os.O_RDONLY | o_directory | o_nofollow | o_cloexec
|
||||
|
||||
rel_parts = download_dir.relative_to(project_root).parts
|
||||
parent_fd = os.open(project_root, walk_flags)
|
||||
try:
|
||||
for part in rel_parts:
|
||||
new_fd = os.open(part, walk_flags, dir_fd=parent_fd)
|
||||
os.close(parent_fd)
|
||||
parent_fd = new_fd
|
||||
|
||||
download_fd = os.open(
|
||||
zip_filename,
|
||||
os.O_RDWR | os.O_CREAT | os.O_EXCL | o_nofollow | o_cloexec,
|
||||
0o600,
|
||||
dir_fd=parent_fd,
|
||||
)
|
||||
try:
|
||||
os.unlink(zip_filename, dir_fd=parent_fd)
|
||||
except OSError:
|
||||
os.close(download_fd)
|
||||
raise
|
||||
return download_fd
|
||||
finally:
|
||||
os.close(parent_fd)
|
||||
|
||||
|
||||
def _open_download_zip_via_paths(
|
||||
project_root: Path, download_dir: Path, zip_filename: str
|
||||
) -> int:
|
||||
"""Portable leaf create for platforms without ``dir_fd`` (e.g. Windows).
|
||||
|
||||
The cache directory is re-validated (real directory, under the project
|
||||
root) immediately before an exclusive create. ``O_EXCL`` guarantees an
|
||||
attacker cannot pre-stage the leaf as a symlink/junction, ``O_TEMPORARY``
|
||||
makes the OS delete it on close, and a post-open inode-identity check
|
||||
detects a leaf swapped underneath us. The returned descriptor is the only
|
||||
handle installation ever uses, so the cache pathname is never reopened.
|
||||
"""
|
||||
zip_path = download_dir / zip_filename
|
||||
project_root_resolved = project_root.resolve()
|
||||
|
||||
if download_dir.is_symlink() or not download_dir.is_dir():
|
||||
raise OSError(
|
||||
errno.ENOTDIR, "Download cache directory is not a real directory"
|
||||
)
|
||||
try:
|
||||
download_dir.resolve().relative_to(project_root_resolved)
|
||||
except (OSError, ValueError):
|
||||
raise OSError(errno.ENOTDIR, "Download cache directory escapes project root")
|
||||
if zip_path.is_symlink():
|
||||
raise OSError(errno.ELOOP, "Refusing to write through a symlinked download file")
|
||||
|
||||
flags = os.O_RDWR | os.O_CREAT | os.O_EXCL
|
||||
flags |= getattr(os, "O_NOFOLLOW", 0)
|
||||
flags |= getattr(os, "O_CLOEXEC", 0)
|
||||
flags |= getattr(os, "O_BINARY", 0)
|
||||
o_temporary = getattr(os, "O_TEMPORARY", 0)
|
||||
flags |= o_temporary
|
||||
|
||||
download_fd = os.open(zip_path, flags, 0o600)
|
||||
try:
|
||||
_verify_leaf_identity(download_fd, zip_path)
|
||||
except OSError:
|
||||
os.close(download_fd)
|
||||
# Without O_TEMPORARY the leaf is not auto-deleted, so remove the file
|
||||
# we just exclusively created (best effort, never through a symlink).
|
||||
if not o_temporary:
|
||||
try:
|
||||
if not zip_path.is_symlink():
|
||||
zip_path.unlink()
|
||||
except OSError:
|
||||
pass
|
||||
raise
|
||||
return download_fd
|
||||
|
||||
|
||||
@extension_app.command("add")
|
||||
def extension_add(
|
||||
extension: str = typer.Argument(help="Extension name or path"),
|
||||
@@ -544,16 +818,13 @@ def extension_add(
|
||||
|
||||
console.print(f"Downloading from {safe_url}...")
|
||||
|
||||
# Download ZIP to temp location
|
||||
download_dir = project_root / ".specify" / "extensions" / ".cache" / "downloads"
|
||||
download_dir.mkdir(parents=True, exist_ok=True)
|
||||
with tempfile.NamedTemporaryFile(
|
||||
prefix="extension-url-download-",
|
||||
suffix=".zip",
|
||||
dir=download_dir,
|
||||
delete=False,
|
||||
) as download_file:
|
||||
zip_path = Path(download_file.name)
|
||||
download_dir = _validate_safe_cache_dir(project_root)
|
||||
zip_filename = f"extension-url-download-{uuid4().hex}.zip"
|
||||
# Only used for diagnostic messages: the real archive is a
|
||||
# transient inode (unlinked on POSIX, O_TEMPORARY on Windows)
|
||||
# consumed via ``archive_file`` below, so this path is never
|
||||
# opened again.
|
||||
zip_path = download_dir / zip_filename
|
||||
|
||||
try:
|
||||
# Use the catalog's authenticated fetch so configured
|
||||
@@ -587,20 +858,66 @@ def extension_add(
|
||||
)
|
||||
raise typer.Exit(1)
|
||||
|
||||
zip_path.write_bytes(zip_data)
|
||||
download_fd = -1
|
||||
download_file = None
|
||||
try:
|
||||
try:
|
||||
download_fd = _safe_open_download_zip(
|
||||
project_root, download_dir, zip_filename
|
||||
)
|
||||
except OSError as exc:
|
||||
console.print(
|
||||
"[red]Error:[/red] Could not safely create download file: "
|
||||
f"{_escape_markup(str(exc))}"
|
||||
)
|
||||
raise typer.Exit(1)
|
||||
|
||||
# Install from downloaded ZIP
|
||||
manifest = manager.install_from_zip(zip_path, speckit_version, priority=priority, force=force)
|
||||
try:
|
||||
download_file = os.fdopen(download_fd, "w+b")
|
||||
download_fd = -1
|
||||
download_file.write(zip_data)
|
||||
download_file.flush()
|
||||
download_file.seek(0)
|
||||
except OSError as exc:
|
||||
console.print(
|
||||
"[red]Error:[/red] Could not safely write download file: "
|
||||
f"{_escape_markup(str(exc))}"
|
||||
)
|
||||
raise typer.Exit(1)
|
||||
|
||||
# Consume the transient inode reserved above rather
|
||||
# than reopening the cache pathname during extraction.
|
||||
try:
|
||||
manifest = manager.install_from_zip(
|
||||
zip_path,
|
||||
speckit_version,
|
||||
priority=priority,
|
||||
force=force,
|
||||
archive_file=download_file,
|
||||
)
|
||||
except OSError as exc:
|
||||
console.print(
|
||||
"[red]Error:[/red] Could not install extension from downloaded archive: "
|
||||
f"{_escape_markup(str(exc))}"
|
||||
)
|
||||
raise typer.Exit(1)
|
||||
finally:
|
||||
if download_file is not None:
|
||||
try:
|
||||
download_file.close()
|
||||
except OSError:
|
||||
pass
|
||||
elif download_fd >= 0:
|
||||
try:
|
||||
os.close(download_fd)
|
||||
except OSError:
|
||||
pass
|
||||
except urllib.error.URLError as e:
|
||||
console.print(
|
||||
f"[red]Error:[/red] Failed to download from {safe_url}: "
|
||||
f"{_escape_markup(str(e))}"
|
||||
)
|
||||
raise typer.Exit(1)
|
||||
finally:
|
||||
# Clean up downloaded ZIP
|
||||
if zip_path.exists():
|
||||
zip_path.unlink()
|
||||
|
||||
else:
|
||||
# Try bundled extensions first (shipped with spec-kit)
|
||||
|
||||
Reference in New Issue
Block a user