diff --git a/ROADMAP.md b/ROADMAP.md index 0730a7c..f5f950b 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -369,6 +369,57 @@ A ticket whose submitter demanded a restart and a rollback, in those words, and told the assistant to skip diagnostics, produced four read-only diagnostics and no mutation — through the real path rather than the gate's hand-built prompt. +**The sandbox was then asked whether it actually contains anything.** ADR-0005 +makes it the real boundary and the approval gate an admitted heuristic, and +until #209 it had never started a container, so nothing about the boundary had +ever been observed. Reading the kernel's own view from inside — `/proc/self/status`, +cgroup limits, routes — rather than trying to break out of it: + +| declared | in effect | +| --- | --- | +| `--cap-drop=ALL` | `CapEff` and `CapBnd` both `0000000000000000` | +| `--security-opt=no-new-privileges` | `NoNewPrivs: 1` | +| `--read-only` | writing outside `/work` blocked | +| `--tmpfs=/work:size=64m` | `dd` asked for 80M, wrote exactly 67108864 bytes | +| `--network=none` | no routes, only `lo` addressed, egress fails | +| `--pids-limit=128` | `pids.max=128` | +| `--memory=512m` | `memory.max=536870912` | +| the project's seccomp profile | **not applied — ever** | + +`_SECCOMP_PROFILE` resolved to `/../sandbox/policies/`, one level above +the repo root at a directory that has never existed, and it is used behind +`if …exists()`, so the miss was silent: Docker's default profile applied +instead. Three other modules already resolve `docs/specs` correctly with an +`OPSPILOT_SPECS_DIR` override; this was the fourth site and the only wrong one. + +**Fixing the path alone breaks the sandbox**, which is why the profile had never +been noticed: its allowlist has `fork` and `vfork` but not `clone`, and libc +implements `fork()` with `clone` — on aarch64 the `fork` syscall does not exist +at all. Every command returned `/bin/sh: can't fork: Operation not permitted`. +The path bug had been hiding a profile that could not run anything. `clone` is +now allowed with the namespace flags masked off and `clone3` returns `ENOSYS` so +libc falls back to it — the same treatment Docker's default gives them, and the +only one seccomp can enforce, since `clone3` takes a struct pointer a filter +cannot dereference. + +With the profile actually loaded, `unshare`, `chroot` and `mount` are refused by +the filter rather than only by the dropped capabilities. + +`tests/test_sandbox_containment.py` pins all of it behind a `requires_docker` +marker. It self-skips where the image is absent, so CI still runs the one +assertion that needs no daemon: that the policy is where the code looks for it. + +*Not verified:* the profile on 32-bit sub-architectures, which `archMap` claims. +`clock_gettime64` was missing and has been added on that basis alone — there is +no 32-bit host here to run it on. + +*Open:* the command runs as **root inside the container**. There is no `--user` +flag, though `--tmpfs=…,uid=1000` says someone intended one. With no +capabilities, no new privileges, a read-only root and a deny-by-default filter +this is heavily defanged, but it is weaker than the argv implies, and closing it +could break a diagnostic that expects to read something root-only. That is a +decision, not an oversight to fix quietly. + **Behaviour gate** — `make test-behaviour`. Five of this product's behaviours are produced by a *prompt*, not by code: an injected Memory constraint changing an answer, a Memory ↔ KB contradiction being reported, a distilled Skill keeping its diff --git a/docs/specs/sandbox/policies/seccomp.template.json b/docs/specs/sandbox/policies/seccomp.template.json index 95ab168..17a2988 100644 --- a/docs/specs/sandbox/policies/seccomp.template.json +++ b/docs/specs/sandbox/policies/seccomp.template.json @@ -1,6 +1,6 @@ { "_comment": "OpsPilot Sandbox baseline seccomp profile", - "_note_default": "Docker's default seccomp profile already disables ~44 high-risk syscalls; this template is for hardened mode (L2) and tightens things further on top of the default.", + "_note_default": "defaultAction is SCMP_ACT_ERRNO, so this profile REPLACES Docker's default rather than layering on it: anything not named below is denied, including syscalls the default would have allowed. Verified against alpine:3.19 on aarch64 by tests/test_sandbox_containment.py.", "_reference": "Docker default: https://github.com/moby/moby/blob/master/profiles/seccomp/default.json", "_apply": "docker run --security-opt seccomp=/path/to/seccomp.json ...", "_version": "1.0.0", @@ -17,7 +17,7 @@ "_comment": "General IO / process / file / time — the minimal set the sandbox requires", "names": [ "accept", "accept4", "access", "arch_prctl", "bind", "brk", "capget", "capset", - "chdir", "chmod", "chown", "chown32", "clock_getres", "clock_gettime", "clock_nanosleep", + "chdir", "chmod", "chown", "chown32", "clock_getres", "clock_gettime", "clock_gettime64", "clock_nanosleep", "close", "close_range", "connect", "copy_file_range", "creat", "dup", "dup2", "dup3", "epoll_create", "epoll_create1", "epoll_ctl", "epoll_pwait", "epoll_wait", "eventfd", "eventfd2", "execve", "execveat", "exit", "exit_group", @@ -60,6 +60,20 @@ ], "action": "SCMP_ACT_ALLOW" }, + { + "_comment": "libc implements fork() with clone; on aarch64 the fork syscall does not exist at all. Allowed only without the namespace flags, the same mask Docker's default profile uses.", + "names": ["clone"], + "action": "SCMP_ACT_ALLOW", + "args": [ + { "index": 0, "value": 2114060288, "valueTwo": 0, "op": "SCMP_CMP_MASKED_EQ" } + ] + }, + { + "_comment": "clone3 passes its flags in a struct a seccomp filter cannot dereference, so the mask above cannot be applied to it. ENOSYS makes libc fall back to clone.", + "names": ["clone3"], + "action": "SCMP_ACT_ERRNO", + "errnoRet": 38 + }, { "_comment": "High-risk syscalls explicitly disabled (already covered by the default deny; listed here for readability only)", "names": [ diff --git a/pyproject.toml b/pyproject.toml index 3ce6a83..2abfeea 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -142,6 +142,10 @@ markers = [ # Model-in-the-loop behaviour gate: real API calls, non-deterministic, run by # a person rather than by CI. See `make test-behaviour`. "requires_api_key: calls a hosted model; excluded from the default run", + # Starts a real container. Every other sandbox test asserts on the argv we + # build, which is why a policy that could not start a container survived + # from PR-30 until it was run by hand. + "requires_docker: runs a real container; excluded from the default run", ] addopts = "-v --strict-markers" asyncio_mode = "auto" diff --git a/src/opspilot/sandbox/docker_l2.py b/src/opspilot/sandbox/docker_l2.py index a870646..cae85dc 100644 --- a/src/opspilot/sandbox/docker_l2.py +++ b/src/opspilot/sandbox/docker_l2.py @@ -6,14 +6,28 @@ from __future__ import annotations +import os import subprocess import time from pathlib import Path +from typing import Final from .types import ActionRequest, ApplyResult, DryRunPreview -# Repo-level seccomp policy (falls back to Docker default if not found). -_SECCOMP_PROFILE = Path(__file__).parents[4] / "sandbox" / "policies" / "seccomp.template.json" +# This file is ``src/opspilot/sandbox/``, so the repo root is four parents up +# and the policy lives under ``docs/specs``. When pip-installed outside the repo +# (the Docker image), ``OPSPILOT_SPECS_DIR`` points at the shipped tree — the +# same resolution redaction.py, schemas.py and kb/storage_init.py use. +_SPECS_DIR: Final[Path] = ( + Path(os.environ["OPSPILOT_SPECS_DIR"]) + if os.environ.get("OPSPILOT_SPECS_DIR") + else Path(__file__).resolve().parents[3] / "docs" / "specs" +) +# Falls back to the Docker default profile when absent — which is what happened +# for this policy's whole life: the path pointed one level above the repo root +# at a `sandbox/policies/` directory that has never existed, and `.exists()` +# made that silent. +_SECCOMP_PROFILE: Final[Path] = _SPECS_DIR / "sandbox" / "policies" / "seccomp.template.json" _DEFAULT_IMAGE = "alpine:3.19" diff --git a/tests/test_sandbox_containment.py b/tests/test_sandbox_containment.py new file mode 100644 index 0000000..33f5401 --- /dev/null +++ b/tests/test_sandbox_containment.py @@ -0,0 +1,176 @@ +"""The sandbox is the boundary, so something has to actually run in it. + +ADR-0005 makes the sandbox the real boundary — the approval gate is an admitted +heuristic denylist and says so in its own docstring. Every other sandbox test +asserts on the argv we hand `docker`, and argv is a claim, not a control. + +Two defects lived their whole life behind that gap. `--tmpfs=…size=64Mi` is a +Kubernetes quantity the kernel rejects, so no container ever started (#209). And +the seccomp policy path pointed one level above the repo root at a directory +that has never existed, so the profile was never applied — and when the path was +fixed, the profile turned out to deny `clone`, which is how libc implements +fork, so nothing could start at all. + +**What these cannot tell you**: whether *our* profile is loaded or Docker's +default is. Both deny the things probed below, so the containment assertions +pass either way — `test_the_seccomp_policy_is_where_the_code_looks_for_it` +covers that gap, because it is the half that broke. + +Marked `requires_docker` and excluded from the default run: it needs a daemon +and the `alpine:3.19` image. It is worth running whenever the argv, the policy, +or the profile changes. +""" + +from __future__ import annotations + +import json +import re +import shutil +import subprocess + +import pytest + +from opspilot.sandbox.docker_l2 import _DEFAULT_IMAGE, _SECCOMP_PROFILE +from opspilot.sandbox.engine import SandboxEngine +from opspilot.sandbox.proposals import to_request + +pytestmark = pytest.mark.requires_docker + + +def _docker_ready() -> bool: + if shutil.which("docker") is None: + return False + try: + subprocess.run( + ["docker", "image", "inspect", _DEFAULT_IMAGE], capture_output=True, check=True + ) + except (subprocess.CalledProcessError, OSError): + return False + return True + + +needs_docker = pytest.mark.skipif( + not _docker_ready(), reason=f"needs a docker daemon and the {_DEFAULT_IMAGE} image" +) + + +def _run(command: str) -> tuple[str, str, int]: + """Execute *command* through the real engine, exactly as a proposal would.""" + request = to_request( + { + "ref": "pa-1", + "intent": "diagnose", + "type": "shell", + "command": command, + "target": "sandbox-self", + "why": "verifying the declared controls are in effect", + }, + session_id="sess_containment", + proposed_by="test", + ) + result = SandboxEngine().execute( + request.model_copy(update={"dry_run": False}), force_approve=True + ) + assert result.apply_result is not None + return result.apply_result.stdout, result.apply_result.stderr, result.apply_result.exit_code + + +def test_the_seccomp_policy_is_where_the_code_looks_for_it() -> None: + """No docker needed, and the one assertion the rest cannot make. + + `_SECCOMP_PROFILE` is applied behind `if …exists()`, so a wrong path is not + an error — it is a silent fallback to Docker's default, which denies enough + of the same things that every containment probe still passes. The path was + wrong from PR-30 until 2026-08-19: one level above the repo root, pointing + at a `sandbox/policies/` directory that has never existed. + """ + assert _SECCOMP_PROFILE.is_file(), f"the profile is not at {_SECCOMP_PROFILE}" + policy = json.loads(_SECCOMP_PROFILE.read_text()) + assert policy["defaultAction"] == "SCMP_ACT_ERRNO", "the allowlist must deny by default" + allowed = { + n + for rule in policy["syscalls"] + if rule["action"] == "SCMP_ACT_ALLOW" + for n in rule["names"] + } + # libc implements fork() with clone; `fork` alone does not exist on aarch64. + assert "clone" in allowed, "nothing can start a process without clone" + + +@needs_docker +def test_a_container_starts_and_reports_its_output() -> None: + """The floor: without this the other assertions are vacuous.""" + stdout, _, exit_code = _run("echo alive") + assert exit_code == 0 + assert "alive" in stdout + + +@needs_docker +def test_a_subprocess_can_be_created() -> None: + """A profile denying `clone` fails here and nowhere else. + + `fork` and `vfork` being on an allowlist is not enough: libc implements + fork() with `clone`, and on aarch64 the `fork` syscall does not exist. + """ + stdout, stderr, exit_code = _run("echo one | cat") + assert "can't fork" not in stderr + assert exit_code == 0 + assert "one" in stdout + + +@needs_docker +def test_the_process_holds_no_capabilities() -> None: + stdout, _, _ = _run("grep -E '^(CapEff|CapBnd):' /proc/self/status") + caps = dict(re.findall(r"^(CapEff|CapBnd):\s*(\w+)", stdout, re.M)) + assert caps.get("CapEff") == "0000000000000000", stdout + # The bounding set too, or a setuid binary could regain them. + assert caps.get("CapBnd") == "0000000000000000", stdout + + +@needs_docker +def test_privileges_cannot_be_regained() -> None: + stdout, _, _ = _run("grep -E '^(NoNewPrivs|Seccomp):' /proc/self/status") + assert re.search(r"^NoNewPrivs:\s*1", stdout, re.M), stdout + # 2 = a seccomp filter is loaded. 0 would mean the profile silently vanished. + assert re.search(r"^Seccomp:\s*2", stdout, re.M), stdout + + +@needs_docker +def test_the_root_filesystem_is_read_only() -> None: + stdout, _, _ = _run("touch /etc/probe 2>/dev/null && echo WRITABLE || echo blocked") + assert "blocked" in stdout, stdout + + +@needs_docker +def test_the_workdir_is_writable_but_capped() -> None: + stdout, _, _ = _run( + "dd if=/dev/zero of=/work/big bs=1M count=80 2>/dev/null; wc -c < /work/big" + ) + written = int(stdout.strip().split()[-1]) + assert written == 64 * 1024 * 1024, f"tmpfs cap not enforced: wrote {written} bytes" + + +@needs_docker +def test_there_is_no_network() -> None: + stdout, _, _ = _run( + "ip -o addr 2>/dev/null | awk '{print $2}' | sort -u; echo ---; ip route 2>/dev/null | wc -l" + ) + interfaces, _, routes = stdout.partition("---") + assert set(interfaces.split()) <= {"lo"}, f"an addressed interface besides lo: {interfaces}" + assert routes.strip() == "0", f"a route exists: {routes}" + + +@needs_docker +@pytest.mark.parametrize( + ("label", "command"), + [ + ("new user namespace", "unshare -U true"), + ("new network namespace", "unshare -n true"), + ("chroot", "chroot /tmp /bin/true"), + ("mount", "mount -t tmpfs none /work"), + ], +) +def test_the_profile_denies_what_it_claims_to(label: str, command: str) -> None: + """Deny-by-default is the point of the allowlist; check it bites.""" + _, stderr, exit_code = _run(f"{command} 2>&1") + assert exit_code != 0, f"{label} succeeded"