From 005dfcbfcc5d3384986bb276b6d5c788324d2559 Mon Sep 17 00:00:00 2001 From: Teknium <127238744+teknium1@users.noreply.github.com> Date: Thu, 13 Aug 2026 10:56:22 -0700 Subject: [PATCH] fix(tools): symlink-safe exclusive creation for all spill/cache writers Spill files (terminal overflow, hook context, web_extract full text, subagent summaries) were written with plain open()/write_text into predictable directories. A pre-planted symlink at any of those paths redirected the write onto an arbitrary user-owned file, and raw pre-redaction terminal/hook spills landed world-readable under the default umask. New tools/spill_safety.py helpers create files with O_CREAT|O_EXCL|O_NOFOLLOW (a link-shaped path fails the write instead of following it) and overwrite via lstat-checked unlink + exclusive re-create, so even the redaction rewrite cannot be diverted. Private tier (0o700 dir / 0o600 file) covers raw terminal and hook spills; cache/web and cache/delegation keep umask perms because those dirs are bind-mounted into remote backends that must read them. Pattern borrowed from DeepSeek Harness dsh-spill-local (MIT): private root + exclusive owner-only opens for spill artifacts. --- tests/tools/test_spill_safety.py | 114 +++++++++++++++++++++++++++++ tools/delegate_tool.py | 7 +- tools/environments/base.py | 11 ++- tools/hook_output_spill.py | 13 +++- tools/spill_safety.py | 118 +++++++++++++++++++++++++++++++ tools/terminal_tool.py | 12 +++- tools/web_tools.py | 9 ++- 7 files changed, 276 insertions(+), 8 deletions(-) create mode 100644 tests/tools/test_spill_safety.py create mode 100644 tools/spill_safety.py diff --git a/tests/tools/test_spill_safety.py b/tests/tools/test_spill_safety.py new file mode 100644 index 0000000000..82440df399 --- /dev/null +++ b/tests/tools/test_spill_safety.py @@ -0,0 +1,114 @@ +"""Symlink-refusal and permission tests for tools.spill_safety. + +The bug class: spill/cache writers used ``open(path, "w")`` / +``Path.write_text`` in predictable directories, which follows a pre-planted +symlink and redirects the write onto an arbitrary user-owned file. Every +helper must refuse the link (never write through it) while keeping normal +writes byte-identical. +""" + +import os +import stat +import sys + +import pytest + +from tools.spill_safety import ( + ensure_spill_dir, + open_exclusive, + write_text_exclusive, +) + +posix_only = pytest.mark.skipif(sys.platform == "win32", reason="POSIX perms/symlinks") + + +def test_write_creates_file_with_content(tmp_path): + target = tmp_path / "spill.txt" + write_text_exclusive(target, "hello\n") + assert target.read_text(encoding="utf-8") == "hello\n" + + +@posix_only +def test_private_file_is_0600(tmp_path): + target = tmp_path / "spill.txt" + write_text_exclusive(target, "secret", private=True) + assert stat.S_IMODE(os.lstat(target).st_mode) == 0o600 + + +@posix_only +def test_private_dir_is_0700_and_tightened(tmp_path): + d = tmp_path / "spills" + d.mkdir(mode=0o755) + ensure_spill_dir(d, private=True) + assert stat.S_IMODE(os.lstat(d).st_mode) == 0o700 + + +def test_ensure_spill_dir_refuses_symlinked_leaf(tmp_path): + victim = tmp_path / "victim-dir" + victim.mkdir() + link = tmp_path / "spills" + link.symlink_to(victim) + with pytest.raises(OSError): + ensure_spill_dir(link) + + +def test_refuses_planted_symlink(tmp_path): + """The core attack: symlink at the spill path must fail, not redirect.""" + victim = tmp_path / "victim.txt" + victim.write_text("original") + target = tmp_path / "spill.txt" + target.symlink_to(victim) + with pytest.raises(OSError): + write_text_exclusive(target, "attacker-controlled") + assert victim.read_text() == "original" + + +def test_refuses_dangling_symlink(tmp_path): + target = tmp_path / "spill.txt" + target.symlink_to(tmp_path / "does-not-exist.txt") + with pytest.raises(OSError): + write_text_exclusive(target, "x") + assert not (tmp_path / "does-not-exist.txt").exists() + + +def test_overwrite_removes_symlink_not_its_target(tmp_path): + victim = tmp_path / "victim.txt" + victim.write_text("original") + target = tmp_path / "spill.txt" + target.symlink_to(victim) + write_text_exclusive(target, "redacted copy", overwrite=True) + # Link replaced by a real file; the link's target untouched. + assert not target.is_symlink() + assert target.read_text(encoding="utf-8") == "redacted copy" + assert victim.read_text() == "original" + + +def test_overwrite_replaces_regular_file(tmp_path): + target = tmp_path / "spill.txt" + target.write_text("raw") + write_text_exclusive(target, "redacted", overwrite=True) + assert target.read_text(encoding="utf-8") == "redacted" + + +def test_overwrite_refuses_directory(tmp_path): + target = tmp_path / "spill.txt" + target.mkdir() + with pytest.raises(OSError): + write_text_exclusive(target, "x", overwrite=True) + assert target.is_dir() + + +def test_exclusive_create_fails_on_existing_without_overwrite(tmp_path): + target = tmp_path / "spill.txt" + target.write_text("first") + with pytest.raises(OSError): + write_text_exclusive(target, "second") + assert target.read_text() == "first" + + +def test_open_exclusive_streaming_write(tmp_path): + target = tmp_path / "spill.log" + with open_exclusive(target, errors="replace") as fh: + fh.write("chunk1") + fh.write("chunk2") + assert target.read_text(encoding="utf-8") == "chunk1chunk2" diff --git a/tools/delegate_tool.py b/tools/delegate_tool.py index 8021051db9..2671b42094 100644 --- a/tools/delegate_tool.py +++ b/tools/delegate_tool.py @@ -2087,7 +2087,12 @@ def _spill_summary_to_file(task_index: int, summary: str) -> Optional[str]: cache_dir.mkdir(parents=True, exist_ok=True) ts = _dt.datetime.now().strftime("%Y%m%d_%H%M%S_%f") path = cache_dir / f"subagent-summary-{task_index}-{ts}.txt" - path.write_text(summary, encoding="utf-8") + from tools.spill_safety import write_text_exclusive + + # Exclusive symlink-refusing create; not private because + # cache/delegation is bind-mounted read-only into remote backends + # whose container UID must be able to read it. + write_text_exclusive(path, summary, private=False) return str(path) except Exception as exc: logger.debug("Failed to spill subagent summary to file: %s", exc) diff --git a/tools/environments/base.py b/tools/environments/base.py index 76fcf81353..b3b04d0a5d 100644 --- a/tools/environments/base.py +++ b/tools/environments/base.py @@ -112,8 +112,15 @@ class _BoundedOutputCollector: return try: if self._spill_fh is None: - self._spill_path.parent.mkdir(parents=True, exist_ok=True) - self._spill_fh = open(self._spill_path, "w", encoding="utf-8", errors="replace") + from tools.spill_safety import ensure_spill_dir, open_exclusive + + # Raw pre-redaction output: private perms + symlink-refusing + # exclusive create (a planted link must fail the spill, never + # redirect the write). + ensure_spill_dir(self._spill_path.parent, private=True) + self._spill_fh = open_exclusive( + self._spill_path, private=True, errors="replace" + ) # Backfill everything retained so far so the file holds the # stream from byte 0, not just from the overflow point. backlog = "".join(self._head) + "".join(self._tail) diff --git a/tools/hook_output_spill.py b/tools/hook_output_spill.py index 58275d6db9..f7d011bc3b 100644 --- a/tools/hook_output_spill.py +++ b/tools/hook_output_spill.py @@ -208,12 +208,21 @@ def spill_if_oversized( saved_path: Optional[str] = None try: spill_dir = _resolve_spill_dir(directory_override, session_id) - spill_dir.mkdir(parents=True, exist_ok=True) + from tools.spill_safety import ensure_spill_dir, write_text_exclusive + + # Hook context may embed raw secrets: private dir/file perms, and an + # exclusive symlink-refusing create so a planted link can't redirect + # the write (predictable per-session directory). + ensure_spill_dir(spill_dir, private=True) filename = f"{uuid.uuid4().hex}.txt" spill_path = spill_dir / filename # Write the raw text plus a trailing newline so tail readers # (``tail -f``, editors) don't report "missing newline". - spill_path.write_text(text if text.endswith("\n") else text + "\n", encoding="utf-8") + write_text_exclusive( + spill_path, + text if text.endswith("\n") else text + "\n", + private=True, + ) saved_path = str(spill_path) except Exception as exc: logger.warning("hook output spill failed: %s", exc) diff --git a/tools/spill_safety.py b/tools/spill_safety.py new file mode 100644 index 0000000000..00d8c2de29 --- /dev/null +++ b/tools/spill_safety.py @@ -0,0 +1,118 @@ +"""Symlink-safe creation helpers for spill/cache files. + +Spill files (oversized terminal output, hook context, subagent summaries, +web_extract full text) are written into predictable, world-discoverable +directories under ``~/.hermes``. A plain ``open(path, "w")`` / +``Path.write_text`` there follows a pre-planted symlink, letting any local +process that can write to the spill directory redirect our write onto an +arbitrary file owned by the user (``~/.bashrc``, ``authorized_keys``, ...). + +Every helper here refuses symlinks by construction: + +* New files are created with ``O_CREAT | O_EXCL``, which fails on ANY + existing path — including a dangling symlink — instead of following it. +* Overwrites first remove the existing path via ``lstat`` + ``unlink`` + (deleting a link deletes the link, never its target), then re-create + exclusively. The check-then-create pair cannot be raced into following a + link because creation itself is exclusive. + +Two privacy tiers: + +* ``private=True`` (default) also forces ``0o700`` directories and + ``0o600`` files — for spills that may hold raw, pre-redaction secrets + (terminal output, hook context). +* ``private=False`` keeps umask-default permissions — for cache dirs that + are bind-mounted into remote terminal backends (Docker/Modal/SSH via + ``credential_files._CACHE_DIRS``), where a non-root container UID must + still be able to read them (public web content, delegation summaries). + +Disk failures are the caller's concern: helpers raise ``OSError`` and the +call sites keep their existing best-effort handling. +""" + +from __future__ import annotations + +import os +import stat +from pathlib import Path +from typing import IO + +__all__ = [ + "ensure_spill_dir", + "open_exclusive", + "write_text_exclusive", +] + +# O_NOFOLLOW is POSIX-only; harmless to omit on Windows since O_EXCL alone +# already refuses every pre-existing path there too. +_O_NOFOLLOW = getattr(os, "O_NOFOLLOW", 0) + + +def ensure_spill_dir(path: Path, *, private: bool = True) -> Path: + """Create ``path`` (and parents) as a directory, refusing symlinks. + + With ``private=True`` the leaf directory is created ``0o700`` and an + already-existing leaf is tightened to ``0o700``. Raises ``OSError`` if + the leaf exists and is not a real directory (e.g. a planted symlink). + """ + path = Path(path) + if private: + path.mkdir(mode=0o700, parents=True, exist_ok=True) + else: + path.mkdir(parents=True, exist_ok=True) + st = os.lstat(path) + if not stat.S_ISDIR(st.st_mode): + raise OSError(f"spill dir is not a directory (symlink?): {path}") + if private and stat.S_IMODE(st.st_mode) != 0o700: + os.chmod(path, 0o700) + return path + + +def open_exclusive( + path: Path, + *, + private: bool = True, + overwrite: bool = False, + encoding: str = "utf-8", + errors: str = "strict", +) -> IO[str]: + """Open ``path`` for writing via exclusive create; never follows a link. + + ``overwrite=True`` first unlinks an existing path (``lstat``-checked so + only the link itself is ever removed, and real directories are refused), + then creates exclusively — so even the overwrite path cannot be + redirected through a symlink. + """ + path = Path(path) + if overwrite: + try: + st = os.lstat(path) + except FileNotFoundError: + pass + else: + if stat.S_ISDIR(st.st_mode): + raise OSError(f"refusing to overwrite a directory: {path}") + os.unlink(path) + mode = 0o600 if private else 0o666 # non-private honors umask + fd = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_EXCL | _O_NOFOLLOW, mode) + try: + return os.fdopen(fd, "w", encoding=encoding, errors=errors) + except Exception: + os.close(fd) + raise + + +def write_text_exclusive( + path: Path, + text: str, + *, + private: bool = True, + overwrite: bool = False, + encoding: str = "utf-8", + errors: str = "strict", +) -> None: + """``Path.write_text`` equivalent that refuses to follow symlinks.""" + with open_exclusive( + path, private=private, overwrite=overwrite, encoding=encoding, errors=errors + ) as fh: + fh.write(text) diff --git a/tools/terminal_tool.py b/tools/terminal_tool.py index 16f1a53ecc..f94731ddd2 100644 --- a/tools/terminal_tool.py +++ b/tools/terminal_tool.py @@ -3434,9 +3434,17 @@ def terminal_tool( try: _sp = Path(spill_file_path) raw_spill = _sp.read_text(encoding="utf-8", errors="replace") - _sp.write_text( + from tools.spill_safety import write_text_exclusive + + # Rewrite in place via lstat-checked unlink + exclusive + # create so the redacted copy can't be diverted through a + # symlink planted between the collector's write and now. + write_text_exclusive( + _sp, redact_terminal_output(strip_ansi(raw_spill), command), - encoding="utf-8", errors="replace", + private=True, + overwrite=True, + errors="replace", ) result_dict["output_total_chars"] = spill_total_chars result_dict["full_output_path"] = spill_file_path diff --git a/tools/web_tools.py b/tools/web_tools.py index 31c425f9a1..e8c62142af 100644 --- a/tools/web_tools.py +++ b/tools/web_tools.py @@ -506,7 +506,14 @@ def _store_full_text(url: str, content: str) -> Optional[str]: + f"\n\n[... stored copy truncated at {MAX_STORED_TEXT_CHARS:,} chars " f"of {len(content):,}; re-extract a more specific URL for the rest ...]" ) - path.write_text(content, encoding="utf-8") + from tools.spill_safety import write_text_exclusive + + # Deterministic filename in a well-known dir: refuse symlinks via + # lstat-unlink + exclusive create. Re-extraction of the same URL + # legitimately overwrites (same slug-digest name). Not private: + # cache/web is bind-mounted into remote backends whose container UID + # must be able to read it, and content is fetched public text. + write_text_exclusive(path, content, private=False, overwrite=True) return str(path) except Exception as exc: # noqa: BLE001 logger.debug("Failed to store full web_extract text for %s: %s", url, exc)