From 34e57ae5205da1f79ffa3cea8a7bb31be8c76ab4 Mon Sep 17 00:00:00 2001 From: Vicente Date: Wed, 19 Aug 2026 19:07:04 -0700 Subject: [PATCH] fix: the sandbox stops running as root MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `--tmpfs=/work:…,uid=1000` said someone had intended a non-root process. No `--user` flag was ever passed, so every command ran as root inside the container. "Root with all capabilities dropped" sounds equivalent to unprivileged and is not. DAC grants access on an **ownership match**, with no capability involved, so `CapEff=0` does nothing about root-owned files. Measured through the real engine, same image, everything else identical: as root as uid 1000 read /etc/shadow (0640 root:shadow) yes denied list /root (0700 root:root) yes denied `--user=1000:1000` now rides alongside a tmpfs owned `uid=1000,gid=1000`, both from `_SANDBOX_UID` rather than two literals that have to agree — the argv already carried `uid=1000` while the process ran as root, and two-places-must- agree is how most of this file's defects began. The cost I was worried about did not appear. Verified through `SandboxEngine`: `id` reports 1000:1000, capabilities and NoNewPrivs and the seccomp filter are unchanged, `/work` is writable and now *owned* by the user rather than merely mounted for them, and pipelines, `ps`, `df`, `date`, `/etc/os-release` and the session's own `grep`/`journalctl` diagnostics all run. Two tests cover it, and both fail on the previous argv: one asserts the process is not root, one asserts a root-owned file cannot be read — the second is the one that says why the first matters. Co-Authored-By: Claude Opus 5 (1M context) --- ROADMAP.md | 23 +++++++++++++++++------ src/opspilot/sandbox/docker_l2.py | 7 ++++++- tests/test_sandbox_containment.py | 19 +++++++++++++++++++ 3 files changed, 42 insertions(+), 7 deletions(-) diff --git a/ROADMAP.md b/ROADMAP.md index f5f950b..609b257 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -413,12 +413,23 @@ assertion that needs no daemon: that the policy is where the code looks for it. `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. +**And it no longer runs as root.** There was no `--user` flag, though +`--tmpfs=…,uid=1000` said someone had intended one. "Root with no capabilities" +sounds equivalent to unprivileged and is not: DAC grants access on an *ownership +match*, with no capability involved, so the process read root-owned files that +`CapEff=0` did nothing about. Measured, same image, same argv otherwise: + +| | as root | as uid 1000 | +| --- | --- | --- | +| read `/etc/shadow` (0640 root:shadow) | yes | denied | +| list `/root` (0700 root:root) | yes | denied | + +`--user=1000:1000` now rides alongside a tmpfs owned `uid=1000,gid=1000`, from +one constant rather than two literals that have to agree — which is how most of +this file's defects started. The feared cost did not materialise: pipelines, +`ps`, `df`, `date` and the session's own diagnostics all still run, and `/work` +stays writable because it is owned by the user now instead of merely mounted for +them. **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 diff --git a/src/opspilot/sandbox/docker_l2.py b/src/opspilot/sandbox/docker_l2.py index cae85dc..a056276 100644 --- a/src/opspilot/sandbox/docker_l2.py +++ b/src/opspilot/sandbox/docker_l2.py @@ -29,6 +29,10 @@ # made that silent. _SECCOMP_PROFILE: Final[Path] = _SPECS_DIR / "sandbox" / "policies" / "seccomp.template.json" _DEFAULT_IMAGE = "alpine:3.19" +# The process runs as this id and the tmpfs is owned by it. One constant for +# both: the argv already carried `uid=1000` while the container ran as root, +# and two literals that have to agree is how most of this file's defects began. +_SANDBOX_UID: Final[int] = 1000 def _mem_to_docker(mem: str) -> str: @@ -56,7 +60,8 @@ def _build_docker_args(request: ActionRequest, image: str, runtime: str | None = "run", "--rm", "--read-only", - f"--tmpfs={workdir}:size={disk},uid=1000", + f"--tmpfs={workdir}:size={disk},uid={_SANDBOX_UID},gid={_SANDBOX_UID}", + f"--user={_SANDBOX_UID}:{_SANDBOX_UID}", "--cap-drop=ALL", "--security-opt=no-new-privileges", f"--memory={_mem_to_docker(p.resource.memory)}", diff --git a/tests/test_sandbox_containment.py b/tests/test_sandbox_containment.py index 33f5401..3fb7554 100644 --- a/tests/test_sandbox_containment.py +++ b/tests/test_sandbox_containment.py @@ -118,6 +118,25 @@ def test_a_subprocess_can_be_created() -> None: assert "one" in stdout +@needs_docker +def test_the_process_is_not_root() -> None: + stdout, _, _ = _run("id -u") + assert stdout.strip() == "1000", stdout + + +@needs_docker +def test_a_root_owned_file_cannot_be_read() -> None: + """Dropping capabilities is not the same as not being root. + + DAC grants access on an ownership match, with no capability involved, so a + root process reads root-owned files even with `CapEff=0`. The argv carried + `uid=1000` on the tmpfs while the process ran as root, and `/etc/shadow` + (0640 root:shadow) was readable. + """ + stdout, _, _ = _run("cat /etc/shadow >/dev/null 2>&1 && echo READ || echo denied") + assert "denied" in stdout, stdout + + @needs_docker def test_the_process_holds_no_capabilities() -> None: stdout, _, _ = _run("grep -E '^(CapEff|CapBnd):' /proc/self/status")