fix: the sandbox stops running as root - #211
Merged
Merged
Conversation
`--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) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#210 left this open on purpose: the argv carried
--tmpfs=…,uid=1000but no--user, so every command ran as root inside the container. I called it"heavily defanged" by the other controls and flagged that closing it might break
a diagnostic. Both halves turned out to be worth checking rather than assuming.
Defanged is not closed
Dropping every capability does not stop root reading root-owned files. DAC
grants access on an ownership match — no capability is consulted — so
CapEff=0000000000000000has nothing to say about it. Measured through the realSandboxEngine, same image, argv otherwise identical:/etc/shadow(0640 root:shadow)/root(0700 root:root)/proc/1/environThe third row is there because it is not a gap, and a table that only showed
the wins would be selling something.
That matters more as soon as
FsPolicy.mountsis used: with identity uidmapping and no user namespace, a root process writes host files as root.
The cost I feared did not appear
Verified through the engine, not by reading the argv:
id→uid=1000 gid=1000 groups=1000CapEff/CapBndstill0000000000000000,NoNewPrivs: 1,Seccomp: 2/workstill writable and now owned1000 1000rather than merely mountedfor a user that was not being used
ps(shows USER 1000),df,date,/etc/os-release, and thesession's own
grep/journalctldiagnostics all run unchangedOne constant, not two literals
--user=1000:1000and--tmpfs=…,uid=1000,gid=1000both come from_SANDBOX_UID. The argv already carrieduid=1000while running as root —two places that have to agree, disagreeing silently, is how most of this file's
defects began (#209's
64Mi, #210's policy path).Tests
Two, both failing on the previous argv:
test_the_process_is_not_root— the assertiontest_a_root_owned_file_cannot_be_read— the reason the first one mattersVerification
pytest -m "not slow and not requires_ollama"— CI's selection — 1374 passed.-m requires_docker14 passed. ruff, ruff format, mypy(154) clean.Still not verified, unchanged from #210: the seccomp profile on the 32-bit
sub-architectures
archMapclaims. No 32-bit host here.🤖 Generated with Claude Code