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.
This commit is contained in:
parent
1bfbfd90f6
commit
a2dc054882
GPG key ID:
03906B4110AAD3B8
7 changed files with 347 additions and 24 deletions
|
|
@ -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)
|
||||
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
|
|
@ -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}")
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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():
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue