diff --git a/reme/utils/file_sandbox/file_sandbox_backend.py b/reme/utils/file_sandbox/file_sandbox_backend.py index f75563fa..c1aa31e4 100644 --- a/reme/utils/file_sandbox/file_sandbox_backend.py +++ b/reme/utils/file_sandbox/file_sandbox_backend.py @@ -6,8 +6,11 @@ import signal import sys from abc import abstractmethod +import psutil from agentscope.tool import BackendBase, ExecResult +_CLEANUP_TIMEOUT = 2.0 + def contains(root: str, path: str) -> bool: """Whether a path is inside a configured root.""" @@ -25,10 +28,14 @@ class FileSandboxBackend(BackendBase): self._writable_paths = self._validate_paths(writable_paths, "writable_paths") if not any(contains(path, self._cwd) for path in self._read_only_paths + self._writable_paths): self._read_only_paths.append(self._cwd) - runtime_prefix = os.path.realpath(sys.prefix) - if runtime_prefix not in ("/", "/usr", "/usr/local") and not runtime_prefix.startswith("/usr/"): - if not any(contains(path, runtime_prefix) for path in self._read_only_paths + self._writable_paths): - self._read_only_paths.append(self._validate_path(runtime_prefix)) + # A virtual environment can point outside sys.prefix to its base + # interpreter and standard library (notably with Homebrew Python). + for runtime in (sys.prefix, sys.base_prefix, sys.executable): + path = os.path.realpath(runtime) + if path in ("/", "/usr", "/usr/local") or path.startswith("/usr/"): + continue + if not any(contains(root, path) for root in self._read_only_paths + self._writable_paths): + self._read_only_paths.append(self._validate_path(path)) paths = self._read_only_paths + self._writable_paths if len(set(paths)) != len(paths): raise ValueError("sandbox paths must not contain duplicates") @@ -81,15 +88,14 @@ class FileSandboxBackend(BackendBase): ) -> ExecResult: """Execute an argv list with a timeout inside the platform sandbox.""" process = await self._start_process(command, cwd or self._cwd) + communication = asyncio.create_task(process.communicate()) try: - stdout, stderr = await asyncio.wait_for(process.communicate(), timeout=timeout) + stdout, stderr = await asyncio.wait_for(asyncio.shield(communication), timeout=timeout) except TimeoutError: - self._kill_process(process) - await process.communicate() + await self._stop_process(process, communication) return ExecResult(exit_code=-1, stdout=b"", stderr=b"timed out") except BaseException: - self._kill_process(process) - await asyncio.shield(process.communicate()) + await self._stop_process(process, communication) raise finally: self._release_process(process) @@ -97,12 +103,38 @@ class FileSandboxBackend(BackendBase): @staticmethod def _kill_process(process: asyncio.subprocess.Process) -> None: - """Terminate the sandbox launcher and its process group.""" + """Stop the launcher and known descendants, including detached children.""" + if sys.platform == "darwin": + try: + os.killpg(process.pid, signal.SIGSTOP) + except ProcessLookupError: + pass + try: + descendants = psutil.Process(process.pid).children(recursive=True) + except psutil.Error: + descendants = [] + for child in reversed(descendants): + try: + child.kill() + except psutil.Error: + pass try: os.killpg(process.pid, signal.SIGKILL) except ProcessLookupError: pass + async def _stop_process(self, process: asyncio.subprocess.Process, communication: asyncio.Task) -> None: + """Bound pipe draining even if an untracked child kept a pipe open.""" + self._kill_process(process) + try: + await asyncio.wait_for(asyncio.shield(communication), timeout=_CLEANUP_TIMEOUT) + except TimeoutError: + communication.cancel() + try: + await asyncio.wait_for(process.wait(), timeout=_CLEANUP_TIMEOUT) + except TimeoutError: + pass + async def read_file(self, path: str) -> bytes: """Read through a sandboxed command so file rules still apply.""" result = await self.exec_shell(["cat", path]) @@ -120,11 +152,11 @@ class FileSandboxBackend(BackendBase): self._cwd, stdin=asyncio.subprocess.PIPE, ) + communication = asyncio.create_task(process.communicate(data)) try: - _stdout, stderr = await process.communicate(data) + _stdout, stderr = await asyncio.shield(communication) except BaseException: - self._kill_process(process) - await asyncio.shield(process.communicate()) + await self._stop_process(process, communication) raise finally: self._release_process(process) diff --git a/reme/utils/file_sandbox/path_seatbelt_backend.py b/reme/utils/file_sandbox/path_seatbelt_backend.py index 782f5bff..9417562d 100644 --- a/reme/utils/file_sandbox/path_seatbelt_backend.py +++ b/reme/utils/file_sandbox/path_seatbelt_backend.py @@ -60,6 +60,9 @@ class PathSeatbeltBackend(FileSandboxBackend): parent = os.path.dirname(parent) lines.extend(f"(allow file-read-metadata (literal {json.dumps(path)}))" for path in sorted(ancestors)) lines.extend(f"(allow file-read* {self._path_rule(path)})" for path in read_paths) + # Shell redirection to the null device is safe and is accepted by + # AutoBash's read-only policy; do not grant writes to the rest of /dev. + lines.append('(allow file-write* (literal "/dev/null"))') for path in self._writable_paths + [tempdir]: # Seatbelt combines allow rules with OR, unlike ordered bwrap mounts. # Exclude read-only descendants from each writable parent's grant. @@ -72,6 +75,20 @@ class PathSeatbeltBackend(FileSandboxBackend): if exclusions: rule = f"(require-all {rule} {' '.join(exclusions)})" lines.append(f"(allow file-write* {rule})") + # A writable ancestor can otherwise be renamed, moving a protected + # subtree out of the path-based exclusion above. Pin the protected + # path and every ancestor that falls within a writable root. + pinned = set() + for read_only in self._read_only_paths: + current = read_only + while current != "/": + if any(contains(writable, current) for writable in self._writable_paths): + pinned.add(current) + current = os.path.dirname(current) + for path in sorted(pinned): + rule = f"(literal {json.dumps(path)})" + lines.append(f"(deny file-write-unlink {rule})") + lines.append(f"(deny file-write-create {rule})") return "\n".join(lines) async def _start_process(self, command: list[str], cwd: str, *, stdin=asyncio.subprocess.DEVNULL): diff --git a/tests/integration/test_seatbelt_backend.py b/tests/integration/test_seatbelt_backend.py index b7e3fde1..820886e4 100644 --- a/tests/integration/test_seatbelt_backend.py +++ b/tests/integration/test_seatbelt_backend.py @@ -1,6 +1,10 @@ """Exercise macOS sandbox startup and file isolation using real subprocesses.""" +import asyncio import sys +import venv + +import psutil import pytest @@ -29,6 +33,34 @@ async def test_seatbelt_starts_shell_and_python(tmp_path, command): assert result.stderr == b"" +@pytest.mark.asyncio +async def test_seatbelt_starts_virtual_environment_python(tmp_path): + """A symlinked virtual environment interpreter can load its base runtime.""" + workspace = tmp_path.resolve() + environment = workspace / "venv" + venv.EnvBuilder(with_pip=False, symlinks=True).create(environment) + backend = PathSeatbeltBackend(cwd=workspace, read_only_paths=[], writable_paths=[]) + + result = await backend.exec_shell( + [str(environment / "bin" / "python"), "-c", "import os; print(os.getcwd())"], + timeout=10, + ) + + assert result.exit_code == 0, result.stderr.decode(errors="replace") + assert result.stdout.decode().strip() == str(workspace) + + +@pytest.mark.asyncio +async def test_seatbelt_allows_null_device_redirection(tmp_path): + """The shell can discard output through /dev/null inside a file sandbox.""" + backend = PathSeatbeltBackend(cwd=tmp_path.resolve(), read_only_paths=[], writable_paths=[]) + + result = await backend.exec_shell(["/bin/sh", "-c", "echo hi > /dev/null"], timeout=5) + + assert result.exit_code == 0, result.stderr.decode(errors="replace") + assert result.stdout == b"" + + @pytest.mark.asyncio async def test_seatbelt_runtime_permissions_preserve_file_isolation(tmp_path): """Startup grants do not expose sibling files or writable access to protected children.""" @@ -69,3 +101,70 @@ for path, operation in [ assert (output / "allowed.txt").read_text() == "ok" assert not (protected / "denied.txt").exists() assert (root / "outside.txt").read_text() == "private" + + +@pytest.mark.asyncio +async def test_seatbelt_cannot_move_read_only_ancestor(tmp_path): + """Moving a writable ancestor must not change a protected file's effective path.""" + workspace = tmp_path.resolve() + protected = workspace / "parent" / "protected" + protected.mkdir(parents=True) + note = protected / "note.txt" + note.write_text("original", encoding="utf-8") + backend = PathSeatbeltBackend(cwd=workspace, read_only_paths=[str(protected)], writable_paths=[str(workspace)]) + + result = await backend.exec_shell( + ["/bin/sh", "-c", "mv parent renamed && echo changed > renamed/protected/note.txt"], + timeout=5, + ) + assert result.exit_code != 0 + assert note.read_text(encoding="utf-8") == "original" + assert not (workspace / "renamed").exists() + + sibling = await backend.exec_shell(["/bin/sh", "-c", "echo allowed > sibling.txt"], timeout=5) + assert sibling.exit_code == 0, sibling.stderr.decode(errors="replace") + assert (workspace / "sibling.txt").read_text(encoding="utf-8").strip() == "allowed" + + +@pytest.mark.asyncio +@pytest.mark.parametrize("cancel", [False, True], ids=["timeout", "cancel"]) +async def test_seatbelt_stops_detached_child(tmp_path, cancel): + """A child that left the launcher's process group should be stopped when observed.""" + workspace = tmp_path.resolve() + backend = PathSeatbeltBackend(cwd=workspace, read_only_paths=[], writable_paths=[str(workspace)]) + pidfile = workspace / "child.pid" + marker = workspace / "late.txt" + output = workspace / "child.out" + child_code = ( + f"import os,time;os.setsid();open({str(pidfile)!r},'w').write(str(os.getpid()));" + f"time.sleep(1.2);open({str(marker)!r},'w').write('late')" + ) + parent_code = ( + f"import subprocess,sys,time;f=open({str(output)!r},'w');" + f"subprocess.Popen([sys.executable,'-c',{child_code!r}],stdin=f,stdout=f,stderr=f);" + "f.close();time.sleep(10)" + ) + task = asyncio.create_task(backend.exec_shell([sys.executable, "-c", parent_code], timeout=10 if cancel else 0.5)) + try: + for _ in range(100): + if pidfile.exists(): + break + await asyncio.sleep(0.02) + assert pidfile.exists(), output.read_text() if output.exists() else "child did not start" + if cancel: + task.cancel() + with pytest.raises(asyncio.CancelledError): + await asyncio.wait_for(task, timeout=4) + else: + result = await asyncio.wait_for(task, timeout=4) + assert result.exit_code == -1 and result.stderr == b"timed out" + await asyncio.sleep(1.2) + assert not marker.exists() + finally: + if not task.done(): + task.cancel() + if pidfile.exists(): + try: + psutil.Process(int(pidfile.read_text(encoding="utf-8"))).kill() + except psutil.Error: + pass diff --git a/tests/unit/test_file_sandbox_backend.py b/tests/unit/test_file_sandbox_backend.py index 7a717d22..d9d560e8 100644 --- a/tests/unit/test_file_sandbox_backend.py +++ b/tests/unit/test_file_sandbox_backend.py @@ -1,5 +1,6 @@ """Shared file sandbox selection and macOS policy construction.""" +import asyncio import sys import pytest @@ -57,9 +58,36 @@ def test_seatbelt_profile_limits_file_access(tmp_path): assert f'(allow file-read* (subpath "{tmp_path}"))' in profile assert f'(allow file-write* (subpath "{output}"))' in profile assert f'(allow file-write* (subpath "{tmp_path}"))' not in profile + assert '(allow file-write* (literal "/dev/null"))' in profile + assert '(allow file-write* (subpath "/dev"))' not in profile assert "network*" not in profile +def test_sandbox_reads_virtual_environment_base_runtime(tmp_path, monkeypatch): + """A virtual environment grants its real base interpreter without widening writes.""" + workspace = tmp_path / "workspace" + venv = tmp_path / "venv" + runtime = tmp_path / "runtime" + interpreter = tmp_path / "interpreter" / "python" + workspace.mkdir() + (venv / "bin").mkdir(parents=True) + runtime.mkdir() + interpreter.parent.mkdir(parents=True) + interpreter.touch() + (venv / "bin" / "python").symlink_to(interpreter) + monkeypatch.setattr(sys, "prefix", str(venv)) + monkeypatch.setattr(sys, "base_prefix", str(runtime)) + monkeypatch.setattr(sys, "executable", str(venv / "bin" / "python")) + + backend = PathSeatbeltBackend(cwd=workspace, read_only_paths=[], writable_paths=[str(workspace)]) + profile = backend._profile(str(workspace / "temporary")) # pylint: disable=protected-access + + assert f'(allow file-read* (subpath "{runtime}"))' in profile + assert f'(allow file-read* (literal "{interpreter}"))' in profile + assert f'(allow file-write* (subpath "{runtime}"))' not in profile + assert f'(allow file-write* (literal "{interpreter}"))' not in profile + + def test_seatbelt_excludes_read_only_child_from_writable_parent(tmp_path): """A parent write grant must not override a nested read-only restriction.""" protected = tmp_path / "protected" @@ -78,6 +106,35 @@ def test_seatbelt_excludes_read_only_child_from_writable_parent(tmp_path): assert f'(allow file-write* (subpath "{output}"))' in profile +@pytest.mark.asyncio +async def test_sandbox_timeout_does_not_wait_forever_for_inherited_pipe(tmp_path, monkeypatch): + """A surviving child with an output pipe must not hold the timeout response open.""" + backend = PathSeatbeltBackend(cwd=tmp_path, read_only_paths=[], writable_paths=[]) + + class StuckProcess: + """Model a child that keeps its output pipe open after termination.""" + + pid = 12345 + + async def communicate(self): + """Wait forever until the caller cancels pipe draining.""" + await asyncio.Event().wait() + + async def wait(self): + """Model an already reaped launcher.""" + return 0 + + async def start(*_args, **_kwargs): + return StuckProcess() + + monkeypatch.setattr(backend, "_start_process", start) + monkeypatch.setattr(backend, "_kill_process", lambda _process: None) + monkeypatch.setattr("reme.utils.file_sandbox.file_sandbox_backend._CLEANUP_TIMEOUT", 0.01) + + result = await asyncio.wait_for(backend.exec_shell(["true"], timeout=0.01), timeout=1) + assert result.exit_code == -1 and result.stderr == b"timed out" + + @pytest.mark.asyncio async def test_seatbelt_launch_failure_cleans_up_without_host_fallback(tmp_path, monkeypatch): """A failed sandbox launcher never starts the requested command directly."""