Fix macOS file sandbox isolation and runtime access

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)

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):

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

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."""