From a2dc054882b6b094ec6bbbec7cedfb1f5ecea995 Mon Sep 17 00:00:00 2001 From: Miguel Jacq Date: Sun, 28 Jun 2026 21:07:49 +1000 Subject: [PATCH] Close TOCTOU gap by freezing directory harvest bundles into a private 0700 copy before the two consumers that actually read artifacts (manifest and diff) touch them. --- enroll/ansible.py | 15 ---- enroll/diff.py | 20 ++++- enroll/ignore.py | 1 - enroll/manifest.py | 12 ++- enroll/manifest_safety.py | 143 +++++++++++++++++++++++++++++++++- tests/test_diff_bundle.py | 91 +++++++++++++++++++++- tests/test_manifest_safety.py | 89 +++++++++++++++++++++ 7 files changed, 347 insertions(+), 24 deletions(-) diff --git a/enroll/ansible.py b/enroll/ansible.py index f50a19c..75e077a 100644 --- a/enroll/ansible.py +++ b/enroll/ansible.py @@ -1275,21 +1275,6 @@ def _render_role_handlers( return "---\n" + (body.rstrip() + "\n" if body else "") -# --- Ansible variable builders --- -def _normalise_flatpak_item( - item: Any, - *, - method: str, - user: Optional[str] = None, - home: Optional[str] = None, -) -> Dict[str, Any]: - return CMModule.normalise_flatpak_item(item, method=method, user=user, home=home) - - -def _normalise_flatpak_remote(item: Any) -> Dict[str, Any]: - return CMModule.normalise_flatpak_remote(item) - - def _normalise_snap_item(item: Any) -> Dict[str, Any]: out = CMModule.normalise_snap_item(item) diff --git a/enroll/diff.py b/enroll/diff.py index 2d4d6ce..8882d31 100644 --- a/enroll/diff.py +++ b/enroll/diff.py @@ -28,6 +28,7 @@ from .state import ( ) from .pathfilter import PathFilter from .sopsutil import decrypt_file_binary_to, require_sops_cmd +from .manifest_safety import freeze_directory_bundle def _validate_diff_bundle(label: str, bundle_dir: Path) -> None: @@ -145,7 +146,9 @@ class BundleRef: return state_path(self.dir) -def _bundle_from_input(path: str, *, sops_mode: bool) -> BundleRef: +def _bundle_from_input( + path: str, *, sops_mode: bool, freeze: bool = False +) -> BundleRef: """Resolve a user-supplied path to a harvest bundle directory. Accepts: @@ -153,6 +156,14 @@ def _bundle_from_input(path: str, *, sops_mode: bool) -> BundleRef: - a path to state.json inside a bundle directory - (sops mode or .sops) a SOPS-encrypted tar.gz bundle - a plain tar.gz/tgz bundle + + When ``freeze`` is True, a plain *directory* input is copied into a private + 0700 temp directory (no-follow, regular-files-only) before being returned, so + a later consumer cannot be raced by an unprivileged owner mutating the source + directory after validation. Tar/SOPS inputs are always extracted into a + private temp directory and so are inherently frozen. ``freeze`` is left False + for purely diagnostic callers (e.g. ``validate``) that should report on the + exact directory the operator named rather than on a copy of it. """ p = Path(path).expanduser() @@ -162,6 +173,9 @@ def _bundle_from_input(path: str, *, sops_mode: bool) -> BundleRef: p = p.parent if p.is_dir(): + if freeze: + frozen_dir, td_frozen = freeze_directory_bundle(p, label="harvest bundle") + return BundleRef(dir=Path(frozen_dir), tempdir=td_frozen) return BundleRef(dir=p) if not p.exists(): @@ -384,8 +398,8 @@ def compare_harvests( Returns (report, has_changes). """ with ExitStack() as stack: - old_b = _bundle_from_input(old_path, sops_mode=sops_mode) - new_b = _bundle_from_input(new_path, sops_mode=sops_mode) + old_b = _bundle_from_input(old_path, sops_mode=sops_mode, freeze=True) + new_b = _bundle_from_input(new_path, sops_mode=sops_mode, freeze=True) if old_b.tempdir: stack.callback(old_b.tempdir.cleanup) if new_b.tempdir: diff --git a/enroll/ignore.py b/enroll/ignore.py index d93ba74..53529be 100644 --- a/enroll/ignore.py +++ b/enroll/ignore.py @@ -191,7 +191,6 @@ class IgnorePolicy: deny_globs: Optional[list[str]] = None allow_binary_globs: Optional[list[str]] = None max_file_bytes: int = 256_000 - sample_bytes: int = 64_000 # If True, be much less conservative about collecting potentially # sensitive files. This disables deny globs (e.g. /etc/shadow, # /etc/ssl/private/*) and skips heuristic content scanning. diff --git a/enroll/manifest.py b/enroll/manifest.py index 3fea7b8..0d97330 100644 --- a/enroll/manifest.py +++ b/enroll/manifest.py @@ -9,7 +9,7 @@ from typing import List, Optional from .ansible import manifest_from_bundle_dir as manifest_ansible_from_bundle_dir from .harvest_safety import ensure_safe_output_parent -from .manifest_safety import validate_site_fqdn +from .manifest_safety import freeze_directory_bundle, validate_site_fqdn from .remote import _safe_extract_tar from .sopsutil import ( decrypt_file_binary_to, @@ -34,7 +34,15 @@ def _prepare_bundle_dir( p = Path(bundle).expanduser() if p.is_dir(): - return str(p), None + # A directory bundle may still be writable by an unprivileged user (e.g. + # root running `manifest` against /tmp/some-harvest). Validation is a + # point-in-time check, but the renderer/JinjaTurtle re-open artifacts by + # path afterwards, so a mutable source could be raced. Freeze the bundle + # into a private 0700 temp copy (no-follow, regular-files-only) and + # consume that immutable copy instead. Tar/SOPS inputs already get an + # equivalent private extraction below. + frozen_dir, td_frozen = freeze_directory_bundle(p, label="harvest bundle") + return frozen_dir, td_frozen if not sops_mode: raise RuntimeError(f"Harvest path is not a directory: {p}") diff --git a/enroll/manifest_safety.py b/enroll/manifest_safety.py index c7e64e1..a060e81 100644 --- a/enroll/manifest_safety.py +++ b/enroll/manifest_safety.py @@ -1,12 +1,15 @@ from __future__ import annotations +import errno import os import re import shutil import stat +import tempfile from pathlib import Path -from typing import Iterator, Tuple +from typing import Iterator, Optional, Tuple +from .fsutil import open_no_follow_path from .harvest_safety import ( OutputSafetyError, ensure_safe_output_parent, @@ -256,3 +259,141 @@ def copy_safe_artifact_file(src: str | Path, dst: str | Path) -> None: """Copy an already validated artifact file without following symlinks.""" shutil.copy2(src, dst, follow_symlinks=False) + + +_FREEZE_MAX_FILE_BYTES = 64 * 1024 * 1024 +_FREEZE_MAX_FILES = 200_000 + + +def _read_all_no_follow(abs_path: str) -> bytes: + """Read a regular file's bytes via a no-follow, non-hardlinked open. + + Mirrors the harvest-side capture discipline: open every path component + without following symlinks, fstat the resulting descriptor, and refuse + anything that is not a single-linked regular file. This is what makes the + frozen copy independent of later mutation of the source tree -- the bytes + are taken from the descriptor we validated, not re-resolved by path. + """ + + fd: Optional[int] = None + try: + try: + fd = open_no_follow_path(abs_path) + except OSError as e: + if e.errno == errno.ELOOP: + raise ArtifactSafetyError( + f"bundle path contains a symlink component: {abs_path}" + ) from e + if e.errno == errno.ENOTDIR: + raise ArtifactSafetyError( + f"bundle path has a non-directory component: {abs_path}" + ) from e + raise ArtifactSafetyError(f"unable to read bundle file: {abs_path}") from e + + st = os.fstat(fd) + if not stat.S_ISREG(st.st_mode): + raise ArtifactSafetyError(f"bundle file is not a regular file: {abs_path}") + if st.st_nlink > 1: + raise ArtifactSafetyError(f"bundle file is hardlinked: {abs_path}") + if st.st_size > _FREEZE_MAX_FILE_BYTES: + raise ArtifactSafetyError(f"bundle file is too large to freeze: {abs_path}") + + chunks: list[bytes] = [] + remaining = int(st.st_size) + while remaining > 0: + chunk = os.read(fd, min(1024 * 1024, remaining)) + if not chunk: + break + chunks.append(chunk) + remaining -= len(chunk) + return b"".join(chunks) + finally: + if fd is not None: + try: + os.close(fd) + except OSError: + pass + + +def freeze_directory_bundle( + bundle_dir: str | Path, *, label: str = "harvest bundle" +) -> Tuple[str, tempfile.TemporaryDirectory]: + """Copy a directory harvest bundle into a private, immutable-by-attacker tree. + + Validation of a harvest bundle (schema + artifact safety) is point-in-time: + it proves the tree is safe *at the moment it is checked*. When the bundle + directory is still writable by an unprivileged user, an attacker can race the + later consumers (``manifest``/``diff``/``explain`` re-open artifacts by path + to copy, hash, or feed to JinjaTurtle) and swap a validated regular file for a + symlink or different file after the check. + + Tar and SOPS inputs already avoid this: Enroll extracts them into a private + ``0700`` temp directory before validating. Plain *directory* inputs were used + in place. This helper closes that gap by giving directory inputs the same + treatment: it copies the bundle into a fresh ``0700`` temp directory using + no-follow traversal, taking each file's bytes from a validated descriptor + (no symlinks, no hardlinks, regular files only). The returned copy is what the + caller should validate and render from, so a subsequent mutation of the + original directory cannot affect the consumed bundle. + + Returns ``(frozen_bundle_dir, tempdir)``. The caller must keep ``tempdir`` + alive for the lifetime of the bundle use; it cleans up on close. + """ + + src_root = Path(bundle_dir).expanduser() + if not src_root.is_dir(): + raise ArtifactSafetyError(f"{label} is not a directory: {src_root}") + + td = tempfile.TemporaryDirectory(prefix="enroll-frozen-bundle-") + try: + dst_root = Path(td.name) / "bundle" + dst_root.mkdir(mode=0o700, parents=True, exist_ok=False) + try: + os.chmod(dst_root, 0o700) + except OSError: + pass + + file_count = 0 + # followlinks=False: do not descend into symlinked directories. Each + # discovered file is independently re-opened no-follow before copying, so + # a symlinked directory cannot smuggle content into the frozen tree even + # if it is swapped in mid-walk. + for cur, dirs, files in os.walk(src_root, followlinks=False): + cur_p = Path(cur) + + # Refuse symlinked subdirectories rather than silently skipping them, + # so a tampered bundle fails loudly instead of producing a partial + # frozen copy that later diverges from the operator's expectation. + for dname in list(dirs): + dp = cur_p / dname + try: + dst = dp.lstat() + except FileNotFoundError: + dirs.remove(dname) + continue + if stat.S_ISLNK(dst.st_mode): + raise ArtifactSafetyError( + f"{label} contains a symlinked directory: {dp}" + ) + + rel_dir = cur_p.relative_to(src_root) + target_dir = dst_root / rel_dir + target_dir.mkdir(mode=0o700, parents=True, exist_ok=True) + + for fname in files: + file_count += 1 + if file_count > _FREEZE_MAX_FILES: + raise ArtifactSafetyError( + f"{label} has too many files to freeze safely" + ) + src_file = cur_p / fname + data = _read_all_no_follow(str(src_file)) + dst_file = target_dir / fname + fd = open_no_follow_path(str(dst_file), write=True, mode=0o600) + with os.fdopen(fd, "wb") as fh: + fh.write(data) + + return str(dst_root), td + except BaseException: + td.cleanup() + raise diff --git a/tests/test_diff_bundle.py b/tests/test_diff_bundle.py index 34780eb..3b37e19 100644 --- a/tests/test_diff_bundle.py +++ b/tests/test_diff_bundle.py @@ -7,6 +7,7 @@ from pathlib import Path import pytest from enroll.ansible import _role_tag +from enroll.manifest_safety import freeze_directory_bundle from enroll.diff import ( _Spinner, _utc_now_iso, @@ -34,6 +35,8 @@ def test_bundle_from_directory_and_statejson_path(tmp_path: Path): b = _make_bundle_dir(tmp_path) + # Plain resolution (no freeze) returns the directory in place. Freezing is + # opt-in and exercised by the consumer-level tests below. br1 = d._bundle_from_input(str(b), sops_mode=False) assert br1.dir == b assert br1.state_path.exists() @@ -42,6 +45,31 @@ def test_bundle_from_directory_and_statejson_path(tmp_path: Path): assert br2.dir == b +def test_bundle_from_input_directory_freeze_copies_and_protects(tmp_path: Path): + import enroll.diff as d + + b = _make_bundle_dir(tmp_path) + art = b / "artifacts" / "app" + art.mkdir(parents=True) + (art / "f.conf").write_text("orig\n", encoding="utf-8") + + # With freeze=True the bundle is copied into a private temp dir, and mutating + # the source afterwards does not change the frozen copy. + result = d._bundle_from_input(str(b), sops_mode=False, freeze=True) + try: + assert result.dir != b + assert result.tempdir is not None + frozen_f = result.dir / "artifacts" / "app" / "f.conf" + assert frozen_f.read_text(encoding="utf-8") == "orig\n" + + # Attacker rewrites the source after the freeze; frozen copy is unaffected. + (art / "f.conf").write_text("tampered\n", encoding="utf-8") + assert frozen_f.read_text(encoding="utf-8") == "orig\n" + finally: + if result.tempdir: + result.tempdir.cleanup() + + def test_bundle_from_tarball_extracts(tmp_path: Path): import enroll.diff as d @@ -742,9 +770,68 @@ def test_compare_harvests_rejects_unsafe_artifact_symlink(tmp_path: Path): with pytest.raises(RuntimeError) as exc_info: compare_harvests(str(old_bundle), str(new_bundle)) + # The unsafe symlinked artifact is now rejected when the directory bundle is + # frozen into a private copy (no-follow traversal), which happens before + # validation. The diff is still refused; the rejection just fires earlier. msg = str(exc_info.value) - assert "old harvest failed validation" in msg - assert "artifact is a symlink" in msg + assert "symlink" in msg.lower() + + +def test_compare_harvests_freezes_directory_bundles_against_source_mutation( + tmp_path: Path, +): + """End-to-end: a directory diff must read frozen copies, so mutating a source + bundle's artifact after compare_harvests has frozen it cannot change the + result or redirect a read to a secret.""" + + def _bundle(name: str, content: str) -> Path: + b = tmp_path / name + art = b / "artifacts" / "users" + art.mkdir(parents=True) + (art / "passwd").write_text(content, encoding="utf-8") + (b / "state.json").write_text( + json.dumps( + { + "inventory": {"packages": {}}, + "roles": { + "users": { + "managed_files": [ + {"path": "/etc/passwd", "src_rel": "passwd"} + ] + } + }, + } + ), + encoding="utf-8", + ) + return b + + old_bundle = _bundle("old", "same\n") + new_bundle = _bundle("new", "same\n") + + # Identical content -> no file content drift. + report, _ = compare_harvests(str(old_bundle), str(new_bundle)) + changed_paths = [f["path"] for f in report["files"]["changed"]] + assert "/etc/passwd" not in changed_paths + + # The freeze already happened inside compare_harvests above and was torn down, + # so to prove immunity during a single call we instead confirm that the frozen + # copy is what gets hashed: rewrite the source between freeze and hash is not + # observable to the operator because the consumed tree is a private copy. + # Confirm at the unit level that a post-freeze swap to a secret symlink is not + # followed by the hashing path. + secret = tmp_path / "secret" + secret.write_text("TOP-SECRET\n", encoding="utf-8") + frozen_dir, td = freeze_directory_bundle(old_bundle) + try: + src = old_bundle / "artifacts" / "users" / "passwd" + src.unlink() + src.symlink_to(secret) + frozen = Path(frozen_dir) / "artifacts" / "users" / "passwd" + assert not frozen.is_symlink() + assert frozen.read_text(encoding="utf-8") == "same\n" + finally: + td.cleanup() def test_utc_now_iso(): diff --git a/tests/test_manifest_safety.py b/tests/test_manifest_safety.py index deb764d..b379941 100644 --- a/tests/test_manifest_safety.py +++ b/tests/test_manifest_safety.py @@ -9,6 +9,7 @@ from enroll.manifest_safety import ( ArtifactSafetyError, ManifestOutputError, copy_safe_artifact_file, + freeze_directory_bundle, iter_safe_artifact_files, prepare_manifest_output_dir, safe_artifact_file, @@ -124,3 +125,91 @@ def test_iter_safe_artifact_files_rejects_symlink_subdir(tmp_path: Path): with pytest.raises(ArtifactSafetyError, match="directory is a symlink"): list(iter_safe_artifact_files(bundle, "role")) + + +def _write_bundle(tmp_path: Path) -> Path: + bundle = tmp_path / "bundle" + art = bundle / "artifacts" / "app" / "etc" / "app" + art.mkdir(parents=True) + (art / "app.conf").write_text("original-content\n", encoding="utf-8") + (bundle / "state.json").write_text("{}\n", encoding="utf-8") + return bundle + + +def test_freeze_directory_bundle_copies_content(tmp_path: Path): + bundle = _write_bundle(tmp_path) + frozen_dir, td = freeze_directory_bundle(bundle) + try: + frozen = Path(frozen_dir) + assert frozen != bundle + assert (frozen / "state.json").read_text(encoding="utf-8") == "{}\n" + assert (frozen / "artifacts" / "app" / "etc" / "app" / "app.conf").read_text( + encoding="utf-8" + ) == "original-content\n" + # The frozen copy is a private directory. + mode = (Path(frozen_dir)).stat().st_mode & 0o777 + assert mode == 0o700 + finally: + td.cleanup() + + +def test_freeze_directory_bundle_is_immutable_after_source_swap(tmp_path: Path): + """The core TOCTOU regression: mutating the source after freezing must not + change what a consumer reads, and must not let a post-freeze symlink redirect + reads to a secret.""" + bundle = _write_bundle(tmp_path) + art_file = bundle / "artifacts" / "app" / "etc" / "app" / "app.conf" + + frozen_dir, td = freeze_directory_bundle(bundle) + try: + secret = tmp_path / "secret" + secret.write_text("TOP-SECRET\n", encoding="utf-8") + + # Attacker swaps the validated regular file for a symlink to a secret and + # also rewrites another path's content. None of this may affect the frozen + # copy the consumer actually uses. + art_file.unlink() + art_file.symlink_to(secret) + + frozen_file = ( + Path(frozen_dir) / "artifacts" / "app" / "etc" / "app" / "app.conf" + ) + assert not frozen_file.is_symlink() + assert frozen_file.read_text(encoding="utf-8") == "original-content\n" + assert "TOP-SECRET" not in frozen_file.read_text(encoding="utf-8") + finally: + td.cleanup() + + +def test_freeze_directory_bundle_rejects_symlinked_file(tmp_path: Path): + bundle = _write_bundle(tmp_path) + secret = tmp_path / "secret" + secret.write_text("secret\n", encoding="utf-8") + # A bundle that already contains a symlinked artifact at freeze time is + # rejected outright rather than silently following it. + link = bundle / "artifacts" / "app" / "etc" / "app" / "link.conf" + link.symlink_to(secret) + with pytest.raises(ArtifactSafetyError, match="symlink"): + freeze_directory_bundle(bundle) + + +def test_freeze_directory_bundle_rejects_symlinked_subdir(tmp_path: Path): + bundle = _write_bundle(tmp_path) + outside = tmp_path / "outside" + outside.mkdir() + (outside / "x.conf").write_text("x\n", encoding="utf-8") + (bundle / "artifacts" / "app" / "linkdir").symlink_to( + outside, target_is_directory=True + ) + with pytest.raises(ArtifactSafetyError, match="symlink"): + freeze_directory_bundle(bundle) + + +def test_freeze_directory_bundle_rejects_hardlinked_file(tmp_path: Path): + bundle = _write_bundle(tmp_path) + secret = tmp_path / "secret" + secret.write_text("secret\n", encoding="utf-8") + hard = bundle / "artifacts" / "app" / "etc" / "app" / "hard.conf" + os.link(secret, hard) + with pytest.raises(ArtifactSafetyError, match="hardlink"): + freeze_directory_bundle(bundle)