diff --git a/plugins/runtime-basic/agentix/files/__init__.py b/plugins/runtime-basic/agentix/files/__init__.py index 256f9e4..8503262 100644 --- a/plugins/runtime-basic/agentix/files/__init__.py +++ b/plugins/runtime-basic/agentix/files/__init__.py @@ -20,18 +20,26 @@ import for type hints. Writes/reads are confined to `$AGENTIX_UPLOAD_ROOT` (default -`/workspace`). Paths outside that root raise `PermissionError`. +`/workspace`). The invariant is on the RESOLVED target: symlinks are +followed as long as every hop stays under the root (merged-usr images +make `/bin` a symlink; real repos contain symlinks), and any hop whose +target lands outside the root raises `PermissionError`. """ from __future__ import annotations import errno import os +from collections import deque from dataclasses import dataclass from pathlib import Path UPLOAD_ROOT = Path(os.environ.get("AGENTIX_UPLOAD_ROOT", "/workspace")).resolve() +# Linux's own resolution cap is 40; a cycle inside the root would +# otherwise loop the walk forever. +_MAX_SYMLINK_HOPS = 40 + @dataclass class UploadResult: @@ -41,16 +49,6 @@ class UploadResult: size: int -def _resolve_within(path: str) -> Path: - """Return `path` lexically under `UPLOAD_ROOT`. - - Actual open/read/write calls below walk from an already-open root - directory fd with O_NOFOLLOW, so symlink swaps cannot redirect the - final file operation outside the upload root. - """ - return UPLOAD_ROOT / Path(*_relative_parts(path)) - - def _relative_parts(path: str) -> tuple[str, ...]: raw = os.path.normpath(path) if os.path.isabs(raw): @@ -67,55 +65,120 @@ def _relative_parts(path: str) -> tuple[str, ...]: return parts -def _open_parent(parts: tuple[str, ...], *, create: bool) -> tuple[int, str]: - """Open the parent directory under UPLOAD_ROOT without following symlinks. +def _is_symlink(exc: OSError, part: str, dir_fd: int) -> bool: + """Did an O_NOFOLLOW open fail because `part` is a symlink? - Returns `(parent_fd, filename)`. The caller owns `parent_fd`. + Linux reports ELOOP; macOS reports ENOTDIR for the O_DIRECTORY case — + and ENOTDIR is ambiguous with a genuine regular-file-in-the-middle, so + readlink is the arbiter. """ - root_fd = os.open(UPLOAD_ROOT, os.O_RDONLY | os.O_DIRECTORY) - fd = root_fd + if exc.errno not in (errno.ELOOP, errno.ENOTDIR): + return False try: - for part in parts[:-1]: - if create: + os.readlink(part, dir_fd=dir_fd) + except OSError: + return False + return True + + +def _link_components(dir_fd: int, name: str) -> tuple[bool, list[str]]: + """`(is_absolute, components)` of the symlink `name` under `dir_fd`. + + Components keep `..` verbatim — resolution happens against real directory + fds in the walk, never by lexical normalization (which would apply `..` + before the preceding symlink is resolved and pick the wrong file).""" + target = os.readlink(name, dir_fd=dir_fd) + comps = [c for c in target.split("/") if c not in ("", ".")] + return os.path.isabs(target), comps + + +def _open_file(path: str, flags: int, *, create_parents: bool, mode: int = 0o666) -> tuple[int, Path]: + """Open `path` confined under `UPLOAD_ROOT`, following in-root symlinks. + + A kernel-faithful (``RESOLVE_IN_ROOT``-style) walk that makes escape + structurally impossible rather than lexically checked: + + - the root dir is opened ONCE with ``O_NOFOLLOW`` and retained for the + whole walk (never reopened by path), so a concurrent rename/replace of + the root cannot redirect a later step outside the jail; + - every component opens with ``O_NOFOLLOW``; a symlink is read and its + target's components are pushed onto the queue — absolute targets + re-anchor at the root (chroot-like), relative targets resolve against + the current directory; + - ``..`` pops the open-dir stack, clamped at the root — you can never + ascend above it — so ``..`` is applied against actually-resolved + directories, not collapsed lexically. + """ + parts = _relative_parts(path) + root_fd = os.open(UPLOAD_ROOT, os.O_RDONLY | os.O_DIRECTORY | os.O_NOFOLLOW) + dir_stack = [root_fd] # owned fds; dir_stack[-1] is the current directory + name_stack: list[str] = [] # resolved component names from the root + queue = deque(parts) + hops = 0 + try: + while queue: + comp = queue.popleft() + if comp == "..": + if len(dir_stack) > 1: + os.close(dir_stack.pop()) + name_stack.pop() + continue # at root, `..` clamps to root + cur = dir_stack[-1] + is_last = not queue + + if is_last: + try: + final_fd = os.open(comp, flags | os.O_NOFOLLOW, mode, dir_fd=cur) + except OSError as exc: + if not _is_symlink(exc, comp, cur): + raise + hops = _bump_hops(hops, path) + absolute, comps = _link_components(cur, comp) + if absolute: + _reset_to_root(dir_stack, name_stack) + queue.extendleft(reversed(comps)) + continue + return final_fd, UPLOAD_ROOT.joinpath(*name_stack, comp) + + if create_parents: try: - os.mkdir(part, mode=0o777, dir_fd=fd) + os.mkdir(comp, mode=0o777, dir_fd=cur) except FileExistsError: pass try: - next_fd = os.open( - part, - os.O_RDONLY | os.O_DIRECTORY | os.O_NOFOLLOW, - dir_fd=fd, - ) + next_fd = os.open(comp, os.O_RDONLY | os.O_DIRECTORY | os.O_NOFOLLOW, dir_fd=cur) except OSError as exc: - if exc.errno == errno.ELOOP: - raise PermissionError( - f"Refusing to follow symlink inside {UPLOAD_ROOT}" - ) from exc - raise + if not _is_symlink(exc, comp, cur): + raise + hops = _bump_hops(hops, path) + absolute, comps = _link_components(cur, comp) + if absolute: + _reset_to_root(dir_stack, name_stack) + queue.extendleft(reversed(comps)) + continue + dir_stack.append(next_fd) + name_stack.append(comp) + # The path resolved to a directory (the root itself, or a resolved + # dir) with no final file component. + raise IsADirectoryError(f"{path!r} resolves to a directory under {UPLOAD_ROOT}") + finally: + for fd in dir_stack: os.close(fd) - fd = next_fd - return fd, parts[-1] - except Exception: - os.close(fd) - raise -def _open_file(path: str, flags: int, *, create_parents: bool, mode: int = 0o666) -> tuple[int, Path]: - parts = _relative_parts(path) - parent_fd, name = _open_parent(parts, create=create_parents) - try: - try: - fd = os.open(name, flags | os.O_NOFOLLOW, mode, dir_fd=parent_fd) - except OSError as exc: - if exc.errno == errno.ELOOP: - raise PermissionError( - f"Refusing to follow symlink inside {UPLOAD_ROOT}" - ) from exc - raise - return fd, _resolve_within(path) - finally: - os.close(parent_fd) +def _bump_hops(hops: int, path: str) -> int: + hops += 1 + if hops > _MAX_SYMLINK_HOPS: + raise PermissionError(f"Too many symlink hops resolving {path!r} under {UPLOAD_ROOT}") + return hops + + +def _reset_to_root(dir_stack: list[int], name_stack: list[str]) -> None: + """Drop every resolved directory back to the retained root fd — an + absolute symlink target is interpreted relative to the jail root.""" + while len(dir_stack) > 1: + os.close(dir_stack.pop()) + name_stack.clear() async def upload(path: str, content: bytes) -> UploadResult: diff --git a/plugins/runtime-basic/tests/test_primitives.py b/plugins/runtime-basic/tests/test_primitives.py index e32aa06..3ebb70c 100644 --- a/plugins/runtime-basic/tests/test_primitives.py +++ b/plugins/runtime-basic/tests/test_primitives.py @@ -12,22 +12,21 @@ @pytest.mark.asyncio -async def test_files_upload_refuses_symlink_escape(tmp_path, monkeypatch): +async def test_files_upload_symlink_to_outside_cannot_touch_the_real_target(tmp_path, monkeypatch): + """An absolute symlink pointing outside the root is re-anchored at the root + (chroot / RESOLVE_IN_ROOT semantics): the write lands inside the jail and + the real out-of-jail file is never touched — escape is structurally + impossible, not merely refused.""" outside = tmp_path / "outside.txt" root = tmp_path / "workspace" root.mkdir() - monkeypatch.setenv("AGENTIX_UPLOAD_ROOT", str(root)) - # files reads UPLOAD_ROOT from env at import time; force a reload so - # the patched env takes effect for this test. - import importlib - importlib.reload(files) + monkeypatch.setattr(files, "UPLOAD_ROOT", root.resolve()) link = root / "link" - link.symlink_to(outside) - - with pytest.raises(PermissionError): - await files.upload(str(link), b"escape") - assert not outside.exists() + link.symlink_to(outside) # absolute target, outside the root + result = await files.upload("link", b"escape") + assert not outside.exists() # the real out-of-jail path is untouched + assert Path(result.path).resolve().is_relative_to(root.resolve()) @pytest.mark.asyncio @@ -153,3 +152,154 @@ def test_shell_executable_prefers_image_bash_in_clean_mode(tmp_path): # without clean mode the bundled-bash preference is unchanged (falls back # to PATH lookup here because /nix/runtime/bin/bash does not exist) assert bash._shell_executable(None, {"PATH": str(fakebin)}) == str(image_bash) +def _set_files_root(monkeypatch, root): + # UPLOAD_ROOT is computed from env at import time; patch the module + # attribute directly — reloading the module would re-create its classes + # and break identity for everything that imported them. + monkeypatch.setattr(files, "UPLOAD_ROOT", root.resolve()) + + +@pytest.mark.asyncio +async def test_files_follows_directory_symlinks_inside_root(tmp_path, monkeypatch): + """merged-usr layout: /bin -> usr/bin. Symlinks whose targets stay under + the root are legitimate filesystem structure, not an escape.""" + root = tmp_path / "rootfs" + (root / "usr" / "bin").mkdir(parents=True) + (root / "bin").symlink_to("usr/bin") # relative, as real merged-usr images store it + _set_files_root(monkeypatch, root) + + result = await files.upload("bin/tool.sh", b"#!/bin/sh\n") + assert (root / "usr" / "bin" / "tool.sh").read_bytes() == b"#!/bin/sh\n" + assert result.size == 10 + assert await files.download("bin/tool.sh") == b"#!/bin/sh\n" + + +@pytest.mark.asyncio +async def test_files_follows_relative_symlinks_inside_root(tmp_path, monkeypatch): + root = tmp_path / "rootfs" + (root / "usr" / "lib").mkdir(parents=True) + (root / "lib").symlink_to("usr/lib") # relative target, like real images + _set_files_root(monkeypatch, root) + + await files.upload("lib/marker", b"x") + assert (root / "usr" / "lib" / "marker").read_bytes() == b"x" + + +@pytest.mark.asyncio +async def test_files_follows_file_symlink_inside_root(tmp_path, monkeypatch): + root = tmp_path / "rootfs" + root.mkdir() + (root / "real.txt").write_bytes(b"data") + (root / "alias.txt").symlink_to("real.txt") + _set_files_root(monkeypatch, root) + + assert await files.download("alias.txt") == b"data" + await files.upload("alias.txt", b"new") + assert (root / "real.txt").read_bytes() == b"new" + + +@pytest.mark.asyncio +async def test_files_symlinks_pointing_outside_cannot_read_or_write_the_real_target(tmp_path, monkeypatch): + """Absolute out-of-jail symlinks re-anchor at the root: reads miss the real + file and writes land inside the jail — the real out-of-jail data is + never reached.""" + outside = tmp_path / "outside" + outside.mkdir() + (outside / "secret.txt").write_bytes(b"SECRET") + root = tmp_path / "rootfs" + root.mkdir() + (root / "evil").symlink_to(outside) # absolute -> re-anchored under root + (root / "evil_file").symlink_to(outside / "secret.txt") + _set_files_root(monkeypatch, root) + + await files.upload("evil/x.txt", b"boom") + assert not (outside / "x.txt").exists() # write stayed inside the jail + + with pytest.raises(FileNotFoundError): + await files.download("evil_file") + assert (outside / "secret.txt").read_bytes() == b"SECRET" # untouched + + +@pytest.mark.asyncio +async def test_files_symlink_cycle_is_refused(tmp_path, monkeypatch): + root = tmp_path / "rootfs" + root.mkdir() + (root / "a").symlink_to("b") + (root / "b").symlink_to("a") + _set_files_root(monkeypatch, root) + + with pytest.raises(PermissionError, match="symlink"): + await files.download("a") + + +@pytest.mark.asyncio +async def test_files_nested_link_with_dotdot_resolves_like_posix(tmp_path, monkeypatch): + """`alias -> inner/../victim.txt` where `inner -> dest/sub`: POSIX resolves + to dest/victim.txt (`..` applies AFTER the inner symlink), not root-level + victim.txt. Lexical normpath of the target gets this wrong.""" + root = tmp_path / "rootfs" + (root / "dest" / "sub").mkdir(parents=True) + (root / "dest" / "victim.txt").write_bytes(b"correct") + (root / "victim.txt").write_bytes(b"WRONG") + (root / "inner").symlink_to("dest/sub") + (root / "alias").symlink_to("inner/../victim.txt") + _set_files_root(monkeypatch, root) + + assert await files.download("alias") == b"correct" + + +@pytest.mark.asyncio +async def test_files_link_to_link_chain_resolves(tmp_path, monkeypatch): + root = tmp_path / "rootfs" + (root / "a" / "b").mkdir(parents=True) + (root / "a" / "b" / "real.txt").write_bytes(b"deep") + (root / "hop1").symlink_to("hop2") + (root / "hop2").symlink_to("a/b/real.txt") + _set_files_root(monkeypatch, root) + + assert await files.download("hop1") == b"deep" + + +@pytest.mark.asyncio +async def test_files_symlink_target_dotdot_cannot_escape_root(tmp_path, monkeypatch): + """A `..`-bearing target that would climb above root is clamped at root + (kernel RESOLVE_IN_ROOT semantics), never followed outside.""" + outside = tmp_path / "outside" + outside.mkdir() + (outside / "secret.txt").write_bytes(b"SECRET") + root = tmp_path / "rootfs" + root.mkdir() + (root / "escape").symlink_to("../outside/secret.txt") + _set_files_root(monkeypatch, root) + + with pytest.raises(FileNotFoundError): + # ../outside clamps to root/outside (does not exist) — never the real + # sibling outside the jail. + await files.download("escape") + assert (outside / "secret.txt").read_bytes() == b"SECRET" # untouched + + +@pytest.mark.asyncio +async def test_files_absolute_symlink_target_is_reanchored_at_root(tmp_path, monkeypatch): + """An absolute symlink target is interpreted relative to the jail root + (chroot-like), so /etc/x -> root/etc/x, never the host's /etc.""" + root = tmp_path / "rootfs" + (root / "etc").mkdir(parents=True) + (root / "etc" / "conf").write_bytes(b"jailed") + (root / "link").symlink_to("/etc/conf") + _set_files_root(monkeypatch, root) + + assert await files.download("link") == b"jailed" + + +@pytest.mark.asyncio +async def test_files_link_resolving_exactly_to_root_then_suffix(tmp_path, monkeypatch): + """A dir symlink whose target is the root itself must still allow a suffix + under it (empty resolved prefix is valid, not an escape).""" + root = tmp_path / "rootfs" + (root / "sub").mkdir(parents=True) + (root / "sub" / "up").symlink_to("..") # -> root + (root / "target.txt").write_bytes(b"hit") + _set_files_root(monkeypatch, root) + + assert await files.download("sub/up/target.txt") == b"hit"