diff --git a/src/agents/sandbox/sandboxes/unix_local.py b/src/agents/sandbox/sandboxes/unix_local.py index ea7f83e9d8..40befdb15e 100644 --- a/src/agents/sandbox/sandboxes/unix_local.py +++ b/src/agents/sandbox/sandboxes/unix_local.py @@ -15,13 +15,14 @@ import shlex import shutil import signal +import stat import tarfile import tempfile import termios import time import uuid from collections import deque -from collections.abc import Collection, Mapping, Sequence +from collections.abc import Callable, Collection, Mapping, Sequence from contextlib import suppress from dataclasses import dataclass, field from functools import partial @@ -152,6 +153,115 @@ class _UnixPtyProcessEntry: wait_task: asyncio.Task[None] | None = None +_SPECIAL_FILE_KINDS: tuple[tuple[Callable[[int], bool], str], ...] = ( + (stat.S_ISFIFO, "fifo"), + (stat.S_ISSOCK, "socket"), + (stat.S_ISCHR, "character device"), + (stat.S_ISBLK, "block device"), +) +_SPECIAL_FILE_EXIT_CODE = 3 + +# User-scoped writer, run as the requested user via the confined exec path. It does the +# special-file classification itself (host-side checks cannot see inside a directory only +# that user may search): an existing entry is opened read-write, which unlike a write-only +# open never blocks on a FIFO, and the type of the *descriptor* is tested through /dev/fd. +_USER_WRITE_SCRIPT = ( + 'target="$1"\n' + 'mkdir -p "$(dirname "$target")" || exit 1\n' + 'if [ -e "$target" ] || [ -L "$target" ]; then\n' + ' exec 3<>"$target" || exit 1\n' + " if [ -p /dev/fd/3 ] || [ -S /dev/fd/3 ] || [ -c /dev/fd/3 ] || [ -b /dev/fd/3 ]; then\n" + f" exit {_SPECIAL_FILE_EXIT_CODE}\n" + " fi\n" + " exec 3>&-\n" + "fi\n" + 'cat > "$target"\n' +) + + +def _special_file_kind(mode: int) -> str | None: + return next((name for predicate, name in _SPECIAL_FILE_KINDS if predicate(mode)), None) + + +def _raise_for_special_file(kind: str | None, *, path: Path, for_write: bool) -> None: + if kind is None: + return + context = {"reason": f"not a regular file: {kind}"} + if for_write: + raise WorkspaceArchiveWriteError(path=path, context=context) + raise WorkspaceArchiveReadError(path=path, context=context) + + +def _classify_mode(mode: int, *, workspace_path: Path, path: Path, for_write: bool) -> None: + if stat.S_ISDIR(mode): + raise IsADirectoryError(errno.EISDIR, os.strerror(errno.EISDIR), str(workspace_path)) + _raise_for_special_file(_special_file_kind(mode), path=path, for_write=for_write) + + +def _open_regular_file(workspace_path: Path, *, path: Path, for_write: bool) -> int: + """Open a workspace file for in-process I/O, refusing FIFOs, sockets, and device nodes. + + `open()` on a FIFO with no peer blocks the calling thread, and this session performs + file I/O synchronously on the event loop, so such an open would stall the whole + process; a device node may block or act on open regardless of `O_NONBLOCK`. + + Where the platform offers `O_PATH` (Linux), the entry is pinned with a descriptor that + does not open it, classified with `fstat()`, and then that same inode is opened for I/O + through `/proc/self/fd`, so a replacement of the path between the two steps cannot + reach a blocking open. A missing target is created with `O_EXCL`, which guarantees the + created entry is a regular file. Elsewhere the entry is classified with `stat()` + before a non-blocking open and again with `fstat()` on the opened descriptor. + Missing paths keep their existing error handling; a directory is reported like the + blocking `open()` did. + """ + cloexec = getattr(os, "O_CLOEXEC", 0) + if for_write: + io_flags = os.O_WRONLY | os.O_TRUNC + else: + io_flags = os.O_RDONLY + o_path = getattr(os, "O_PATH", None) + if o_path is not None: + try: + pin = os.open(workspace_path, o_path | cloexec) + except FileNotFoundError: + if not for_write: + raise + # Create the file ourselves; O_EXCL means whatever we get back is the + # regular file this call created, never an entry swapped in meanwhile. + return os.open(workspace_path, os.O_WRONLY | os.O_CREAT | os.O_EXCL | cloexec, 0o666) + try: + _classify_mode( + os.fstat(pin).st_mode, workspace_path=workspace_path, path=path, for_write=for_write + ) + return os.open(f"/proc/self/fd/{pin}", io_flags | cloexec) + finally: + os.close(pin) + + try: + _classify_mode( + workspace_path.stat().st_mode, + workspace_path=workspace_path, + path=path, + for_write=for_write, + ) + except OSError: + pass # missing or unreadable: let the open below report it + flags = io_flags | os.O_NONBLOCK | cloexec + if for_write: + flags |= os.O_CREAT + fd = os.open(workspace_path, flags, 0o666) + try: + _classify_mode( + os.fstat(fd).st_mode, workspace_path=workspace_path, path=path, for_write=for_write + ) + # Regular files ignore O_NONBLOCK; clear it anyway so the handle behaves like open(). + fcntl.fcntl(fd, fcntl.F_SETFL, fcntl.fcntl(fd, fcntl.F_GETFL) & ~os.O_NONBLOCK) + except BaseException: + os.close(fd) + raise + return fd + + class UnixLocalSandboxSession(BaseSandboxSession): """ Unix-only session implementation that runs commands on the host and uses the host filesystem @@ -985,11 +1095,12 @@ async def read(self, path: Path, *, user: str | User | None = None) -> io.IOBase workspace_path = self.normalize_path(path) try: - return workspace_path.open("rb") + fd = _open_regular_file(workspace_path, path=path, for_write=False) except FileNotFoundError as e: raise WorkspaceReadNotFoundError(path=path, cause=e) from e except OSError as e: raise WorkspaceArchiveReadError(path=path, cause=e) from e + return os.fdopen(fd, "rb") async def write( self, @@ -1007,7 +1118,8 @@ async def write( try: workspace_path.parent.mkdir(parents=True, exist_ok=True) - with workspace_path.open("wb") as f: + fd = _open_regular_file(workspace_path, path=workspace_path, for_write=True) + with os.fdopen(fd, "wb") as f: shutil.copyfileobj(payload.stream, f) except OSError as e: raise WorkspaceArchiveWriteError(path=workspace_path, cause=e) from e @@ -1024,7 +1136,7 @@ async def _write_stream_with_exec( command_parts = self._prepare_exec_command( "sh", "-c", - 'mkdir -p "$(dirname "$1")" && cat > "$1"', + _USER_WRITE_SCRIPT, "sh", str(path), shell=False, @@ -1062,6 +1174,10 @@ async def _write_stream_with_exec( except OSError as e: raise WorkspaceArchiveWriteError(path=path, cause=e) from e + if proc.returncode == _SPECIAL_FILE_EXIT_CODE: + raise WorkspaceArchiveWriteError( + path=path, context={"reason": "not a regular file", "user": str(user)} + ) if proc.returncode: raise WorkspaceArchiveWriteError( path=path, diff --git a/tests/sandbox/test_unix_local.py b/tests/sandbox/test_unix_local.py index fb13be3c51..33c1e2073b 100644 --- a/tests/sandbox/test_unix_local.py +++ b/tests/sandbox/test_unix_local.py @@ -2,18 +2,26 @@ import asyncio import io +import os +import shutil import signal +import subprocess import tarfile +import tempfile import threading import time from pathlib import Path from types import SimpleNamespace -from typing import cast +from typing import Any, cast import pytest from agents.sandbox import SandboxPathGrant -from agents.sandbox.errors import PtySessionNotFoundError +from agents.sandbox.errors import ( + PtySessionNotFoundError, + WorkspaceArchiveReadError, + WorkspaceArchiveWriteError, +) from agents.sandbox.manifest import Environment, Manifest from agents.sandbox.sandboxes import unix_local as unix_local_module from agents.sandbox.sandboxes.unix_local import ( @@ -513,3 +521,173 @@ def _slow_extract(tar: object, **kwargs: object) -> None: # the workspace root are only released once nothing is still writing to them. assert events == ["extract-start", "extract-end"] assert not buf.closed + + +@pytest.mark.asyncio +@pytest.mark.skipif(not hasattr(os, "mkfifo"), reason="requires FIFO support") +async def test_unix_local_read_and_write_refuse_a_fifo_instead_of_blocking( + tmp_path: Path, +) -> None: + workspace = tmp_path / "ws" + workspace.mkdir() + fifo = workspace / "pipe" + os.mkfifo(fifo) + # Keep both ends of the pipe open so the previous behaviour (opening the FIFO) returns + # instead of blocking the event loop, letting the assertions below fail cleanly. + peer_fd = os.open(fifo, os.O_RDWR | os.O_NONBLOCK) + try: + async with await UnixLocalSandboxClient().create( + manifest=Manifest(root=str(workspace)), snapshot=None, options=None + ) as session: + with pytest.raises(WorkspaceArchiveReadError) as read_error: + await session.read(Path("pipe")) + assert read_error.value.context["reason"] == "not a regular file: fifo" + + with pytest.raises(WorkspaceArchiveWriteError) as write_error: + await session.write(Path("pipe"), io.BytesIO(b"payload")) + assert write_error.value.context["reason"] == "not a regular file: fifo" + + # A link to the FIFO resolves to the same entry and is refused the same way. + (workspace / "pipe_link").symlink_to(fifo) + with pytest.raises(WorkspaceArchiveReadError): + await session.read(Path("pipe_link")) + + # Regular-file behaviour is unchanged. + await session.write(Path("plain.txt"), io.BytesIO(b"hello")) + handle = await session.read(Path("plain.txt")) + try: + assert handle.read() == b"hello" + finally: + handle.close() + finally: + os.close(peer_fd) + + assert fifo.is_fifo() + + +@pytest.mark.asyncio +@pytest.mark.skipif(not hasattr(os, "mkfifo"), reason="requires FIFO support") +async def test_unix_local_refuses_a_fifo_without_a_peer_and_a_directory_read( + tmp_path: Path, +) -> None: + workspace = tmp_path / "ws" + workspace.mkdir() + fifo = workspace / "pipe" + os.mkfifo(fifo) + (workspace / "dir").mkdir() + + async with await UnixLocalSandboxClient().create( + manifest=Manifest(root=str(workspace)), snapshot=None, options=None + ) as session: + # No peer holds the pipe open: a blocking open would never return. + with pytest.raises(WorkspaceArchiveWriteError): + await asyncio.wait_for(session.write(Path("pipe"), io.BytesIO(b"payload")), 5) + with pytest.raises(WorkspaceArchiveReadError) as read_error: + await asyncio.wait_for(session.read(Path("pipe")), 5) + assert read_error.value.context["reason"] == "not a regular file: fifo" + + with pytest.raises(WorkspaceArchiveReadError) as dir_error: + await session.read(Path("dir")) + assert isinstance(dir_error.value.__cause__, IsADirectoryError) + + assert fifo.is_fifo() + + +@pytest.mark.skipif(not hasattr(os, "mkfifo"), reason="requires FIFO support") +def test_open_regular_file_never_opens_a_fifo_for_io( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + fifo = tmp_path / "pipe" + os.mkfifo(fifo) + real_open = os.open + o_path = getattr(os, "O_PATH", 0) + + def guarded_open(path: object, flags: int, *args: object) -> int: + if Path(str(path)) == fifo and not (o_path and flags & o_path): + raise AssertionError("the FIFO must be classified without an I/O open") + return real_open(cast(Any, path), flags, *cast(Any, args)) + + monkeypatch.setattr(unix_local_module.os, "open", guarded_open) + + with pytest.raises(WorkspaceArchiveReadError) as read_error: + unix_local_module._open_regular_file(fifo, path=Path("pipe"), for_write=False) + assert read_error.value.context["reason"] == "not a regular file: fifo" + with pytest.raises(WorkspaceArchiveWriteError): + unix_local_module._open_regular_file(fifo, path=Path("pipe"), for_write=True) + assert fifo.is_fifo() + + +def test_open_regular_file_creates_missing_targets_and_reads_them_back(tmp_path: Path) -> None: + target = tmp_path / "new.txt" + fd = unix_local_module._open_regular_file(target, path=Path("new.txt"), for_write=True) + with os.fdopen(fd, "wb") as handle: + handle.write(b"hello") + fd = unix_local_module._open_regular_file(target, path=Path("new.txt"), for_write=False) + with os.fdopen(fd, "rb") as handle: + assert handle.read() == b"hello" + with pytest.raises(FileNotFoundError): + unix_local_module._open_regular_file( + tmp_path / "absent", path=Path("absent"), for_write=False + ) + with pytest.raises(IsADirectoryError): + unix_local_module._open_regular_file(tmp_path, path=Path("."), for_write=False) + + +def _run_user_write_script( + target: Path, payload: bytes, *, user: int | None +) -> subprocess.CompletedProcess[bytes]: + return subprocess.run( + ["sh", "-c", unix_local_module._USER_WRITE_SCRIPT, "sh", str(target)], + input=payload, + capture_output=True, + check=False, + timeout=5, # a regression would block on the FIFO; the watchdog fails the test instead + user=user, + group=user, + ) + + +@pytest.mark.skipif(not hasattr(os, "mkfifo"), reason="requires FIFO support") +def test_user_write_script_refuses_a_fifo_and_writes_regular_files(tmp_path: Path) -> None: + fifo = tmp_path / "pipe" + os.mkfifo(fifo) + + refused = _run_user_write_script(fifo, b"payload", user=None) + assert refused.returncode == unix_local_module._SPECIAL_FILE_EXIT_CODE, refused.stderr + assert fifo.is_fifo() + + created = _run_user_write_script(tmp_path / "sub" / "new.txt", b"hello", user=None) + assert created.returncode == 0, created.stderr + assert (tmp_path / "sub" / "new.txt").read_bytes() == b"hello" + + existing = tmp_path / "existing.txt" + existing.write_bytes(b"old content that is longer") + rewritten = _run_user_write_script(existing, b"new", user=None) + assert rewritten.returncode == 0, rewritten.stderr + assert existing.read_bytes() == b"new" + + +@pytest.mark.skipif(not hasattr(os, "mkfifo"), reason="requires FIFO support") +@pytest.mark.skipif(os.geteuid() != 0, reason="needs root to run the writer as another user") +def test_user_write_script_refuses_a_fifo_under_a_user_only_parent(tmp_path: Path) -> None: + """The requested user, not the SDK identity, is the one that can see and open the FIFO.""" + nobody = 65534 + # pytest's tmp_path sits under a root-only directory; the requested user must be able + # to reach the parent, so build it under the world-traversable temp root instead. + parent = Path(tempfile.mkdtemp(prefix="unix-local-private-")) + try: + fifo = parent / "pipe" + os.mkfifo(fifo) + os.chown(fifo, nobody, nobody) + os.chown(parent, nobody, nobody) + parent.chmod(0o700) + + refused = _run_user_write_script(fifo, b"payload", user=nobody) + assert refused.returncode == unix_local_module._SPECIAL_FILE_EXIT_CODE, refused.stderr + assert fifo.is_fifo() + + written = _run_user_write_script(parent / "note.txt", b"hello", user=nobody) + assert written.returncode == 0, written.stderr + assert (parent / "note.txt").read_bytes() == b"hello" + finally: + shutil.rmtree(parent, ignore_errors=True)