fix(security): stage dind bind sources so runc cannot re-walk a job-controlled path - #173
Merged
Conversation
…ontrolled path #125. translateBindSource proved containment with filepath.EvalSymlinks and then returned the *unresolved* joined string, which containers.go handed to containerd as the OCI bind source. runc walks that string again, in its own process, at task start. Between the check and the mount — the rest of translation, the containerd container create, the whole Docker create-then-start round trip — the job (which owns every byte of its own rootfs) could swap a validated component for a symlink out of the rootfs, and runc mounted whatever it pointed at by then. The runner-bind branch was worse: it had no containment check at all, and the per-job runner directory in that table is bind-mounted into the runner and therefore job-writable. Reproduced against real runc 1.3.4 (kernel 6.18, WSL2): with the validated directory swapped after the spec was written, the container printed the attacker's marker and exited 0. Two halves, both necessary. Resolution (pkg/dind/bindpin_linux.go). Every source with a job-supplied component is resolved once with openat2(RESOLVE_IN_ROOT|RESOLVE_NO_MAGICLINKS) anchored at that branch's root, and the result is held as an O_PATH descriptor. Containment is enforced by the kernel during the walk instead of by a string comparison after it, and there is no name left to re-point. RESOLVE_IN_ROOT rather than os.Root because it reinterprets absolute symlinks relative to the root, which merged-usr runner images require. Pre-5.6 kernels fall back to an equivalent O_PATH|O_NOFOLLOW component walk. Auto-mkdir creates through mkdirat against a pinned parent and re-opens O_NOFOLLOW, so losing the race to a planted symlink fails instead of escaping. Staging (pkg/dind/bindstage_linux.go). The descriptor cannot go in the spec: runc does not re-resolve /proc/<pid>/fd/<n> (its error echoes it verbatim) but mount(2) rejects it, because a bind source must live in the caller's mount namespace and runc always has its own. That is unconditional — legitimate binds fail identically — so the fd handoff the earlier fix/linux-isolation-hardening branch shipped is a functional regression, not a fix. Verified here and kept as an executable test. Instead ephemerd performs the bind itself, in its own mount namespace where /proc/self/fd resolves, onto <data>/dind-binds/<job>/<n>, and gives runc that path. Every component is root-owned and 0700 and nothing in the name comes from the request, so runc re-walking it is safe; ensureTrustedAncestry checks that precondition rather than assuming it. Staging lives outside <data>/jobs/ on purpose: the orphan sweep RemoveAll's that tree, and deleting through a live bind mount deletes the source — which here would be the runner's rootfs. Pins are held on the containerEntry for the container's life (docker restart makes runc re-read the spec) and released in cleanupContainer; Server.Stop tears down the job's staging dir; a hard kill skips both, so dind.SweepStagedBinds runs from Runtime.CleanOrphans before the container/snapshot sweep, because a leaked staging mount pins the rootfs and makes the snapshot undeletable. Failure mode inverts on purpose: a source that cannot be staged now fails docker create with a 400 instead of mounting something unvalidated. The error names the staging dir and the requirement. Windows and macOS are untouched — bind translation is not wired into either native container path. bindpin_other.go / bindstage_other.go exist only so the translation-policy tests build on a dev host and carry no security property. VERIFIED ON LINUX, not inferred. On WSL2 (kernel 6.18) as root, against the runc this repo embeds: control (pre-fix shape): CONTAINER-SAW: SWAPPED staged (fixed shape): CONTAINER-SAW: PINNED /proc/<pid>/fd source: mount ... flags=MS_BIND|MS_REC: invalid argument plus the legitimate-bind regression set (directory, regular file, auto-mkdir, merged-usr symlink traversal, docker.sock/hosts/resolv.conf passthrough), teardown, the startup sweep, and the full pkg/dind suite against a real containerd. The tests that carry the security property need CAP_SYS_ADMIN and skip on the unprivileged CI runner; ci.yml prints the skips, following the precedent already there for the dind pull test, because a green package on an unprivileged host is exactly how the previous attempt passed review.
…ootfs Review of #173. The staging fix was sound but its teardown was not. BLOCKER. Server.Stop() tore down containers and the bind stager while the HTTP listener was still accepting. An in-flight handleContainerCreate could therefore call stage() concurrently with teardown(), and stage() released the stager lock before its unix.Mount. unmountTreeAndRemove checks that no mounts remain and then calls os.RemoveAll, so a mount landing between those two steps put RemoveAll on top of a live bind mount. That is not merely untidy. Measured, not assumed: source before: [precious precious2] RemoveAll err: unlinkat .../job/staged: device or resource busy source after: [] os.RemoveAll walks INTO the mount, deletes the files visible through it, and only then reports EBUSY on the mountpoint. In production those files are the runner's own rootfs. Reachable from a job: sibling containers are not in the runner's cgroup, so they outlive the task the runtime kills before Stop, and on the TCP transport they still hold DOCKER_HOST. Second defect in the same area: teardown cleared `ready` before unmounting, so a racing stage() would silently recreate the staging directory the caller had just swept, leaking a mount that pins the rootfs until the next startup. Three changes, any one of which closes it; all three because this one destroys data: - Stop() shuts the HTTP server and listener down FIRST, before touching any state an in-flight request can still be creating. - stage() holds the stager lock across the mkdir and the mount, and teardown holds it across the unmount-and-remove, so the two are mutually exclusive. Pin release takes the same lock for the same reason. - teardown sets a `closed` flag that is never cleared: staging after teardown now fails instead of recreating the directory. Covered by TestBindStaging_StageAfterTeardownIsRefused and TestUnmountTreeAndRemove_RefusesWhileMounted, the latter measuring the RemoveAll hazard in one subtest so the guard's existence is justified by evidence rather than by comment. The fallback walk was not equivalent, and said it was. resolveBeneathWalk refused every "..", including ones spliced in from a SYMLINK TARGET, while RESOLVE_IN_ROOT resolves those clamped at the root. Relative "../" targets are ordinary in real images (/etc/alternatives/*, Debian multiarch), so any node that ever fell back would have started 400ing legitimate binds while the doc claimed parity. It now holds every directory on the way down open and steps ".." back to a descriptor it already has — never re-opening a parent by name, which would be the second walk this whole mechanism removes. TestResolveBeneathWalk_MatchesOpenat2 asserts both resolvers land on the same inode across six shapes, including "../usr/bin/tool" and a "../" chain that tries to climb past the root. The openat2 latch was silent, permanent and node-wide. One spurious error could convert a node's resolver for the rest of the daemon's uptime with nothing in the log. Now only ENOSYS and E2BIG latch (the kernel genuinely lacks the syscall); EPERM — a seccomp filter — falls back for that call alone; and the first bind afterwards logs a WARN naming the reason. The EINVAL justification was also simply wrong: RESOLVE_IN_ROOT clamps an escape rather than returning EINVAL, and EXDEV is RESOLVE_BENEATH's error. Right call, wrong reason, now corrected in place so the next reader does not inherit it. Also from review: - The runnerBinds-suffix branch — the one that previously had NO containment check and reaches the job-writable runner directory — had no staging test. TestBindStaging_RunnerBindSuffixIsStaged now covers it: staged, survives the swap, symlink escape refused. - ensureDirLocked chmodded before checking for a symlink, so a planted <data>/dind-binds symlink got its target chmodded 0700 on the way to being refused. Checked first now. - The runnerBinds-suffix branch skipped the non-dir/non-regular rejection the rootfs branch applies. Both go through rejectUnbindableType. - gofmt: three files (two of the three predate this branch). - Doc: "every component is root-owned and 0700" overstated ensureTrustedAncestry, which also accepts euid-owned and sticky world-writable; corrected, with an operator note that a --data-dir under a group-writable path now hard-fails every bind. Re-verified after the changes, Linux/WSL2 kernel 6.18, euid 0, runc 1.3.4: control CONTAINER-SAW: SWAPPED, staged CONTAINER-SAW: PINNED, proc-fd source still EINVAL, whole pkg/dind and pkg/runtime green, gofmt and vet clean, and zero dind-binds entries left in /proc/self/mountinfo. Neutering stage() to return the resolved path now fails 5 tests including the real-runc ESCAPE assertion.
CI caught eight unchecked Close() calls that could not surface locally - the pinned golangci-lint panics on Go 1.26, so errcheck only ever runs on the Linux CI job. The production one (mountPointsUnder) reads /proc/self/mountinfo, so a Close error carries nothing a caller could act on; it is now explicitly discarded rather than silently dropped, which is the distinction errcheck is actually making. The seven in tests are the same shape.
… luck CI went red on TestBindTranslation_RealContainerd. The test is pre-existing; what changed under it is that bind translation now stages every job-supplied source as a real bind mount, so a test that never needed mount(2) suddenly does, and ephemerd's CI runner is an unprivileged container. Not fixed by degrading staging. A fallback to the resolved path when unprivileged would put a job-controlled path back in the OCI spec — issue #125 reopened — and every test would go green while the escape was live. That is the exact shape of the previous broken fix. Failing closed is correct; the test has to say so instead. Verified the premise rather than assuming it: root with CAP_SYS_ADMIN is a hard, universal prerequisite for ephemerd on Linux, so the production hard-fail costs nothing real. The shipped systemd unit sets no User= and no capability bounding set (cmd/ephemerd/install_linux.go), `ephemerd doctor` FAILS rather than warns when euid != 0 with "ephemerd requires root for container management" (cmd/ephemerd/doctor_linux.go), the in-process containerd already needs mount(2) for the overlayfs snapshotter, networking manages iptables chains and a CNI bridge, and there is no rootless mode anywhere in the tree or the docs. Every documented invocation is sudo. Gating now reuses the house pattern instead of a new one. pull_e2e_test.go already solved this: probeMountPrivilege performs the actual bind mount and reports the error, because "capabilities, user namespaces, LSM policy and read-only mount propagation all decide this independently of the uid, and ephemerd's own runners are containers where they disagree". requireBindStaging wraps it, and EPHEMERD_TEST_REQUIRE_MOUNT=1 turns the skip into a failure where the coverage must be guaranteed. This replaces the os.Geteuid() == 0 check the staging tests shipped with — which was wrong for the same reason: under `capsh --drop=cap_sys_admin` the euid is still 0 and every one of those tests would have tried to mount and failed. The lost coverage is made legible, not implied. TestBindTranslation_ RealContainerd is added to the "Privileged-test coverage report" step in ci.yml so its SKIP line is printed; `go test ./...` buffers package output and prints it only on failure, so on a green run a skip reaches nobody. The step's comment says why it is in the list and why making staging degrade is not the alternative. Audited the other four tests that reach buildBindMounts rather than guessing which else might be affected. Three use unstagedTestStager and never stage by design. The fourth, TestBindTranslation_RejectsForeignSource, uses the real stager but is rejected in translateBindSource before anything is pinned, so it needs no privilege — deliberately left ungated, with a comment saying that is a property to preserve and to re-gate it if the rejection ever moves after staging. Nothing in test/e2e passes Binds at all, so none of it is affected. Also, since the errno is the operator's only clue: stagingMountAdvice splits the mount failure into "this is about PRIVILEGE" (EPERM, pointing at `ephemerd doctor`) and "this is about the DATA DIRECTORY" (EROFS, ENOSPC, EACCES, each naming the specific problem), instead of one sentence listing both and making the reader guess. Repaired encoding damage in bindstage_linux_test.go and bindstage_runc_linux_test.go: both had picked up a UTF-8 BOM and 27 em-dashes double-encoded to "—". The BOM did not break the //go:build linux constraint (verified via go list — both files stay in IgnoredGoFiles on Windows), so this was hygiene rather than breakage, but they are new files and should not enter history corrupted. Verified on Linux/WSL2 kernel 6.18 both ways: CAP_SYS_ADMIN dropped (capsh, euid still 0 — the CI condition): whole pkg/dind: 188 pass, 14 skip, 0 fail TestBindTranslation_RealContainerd SKIP (was FAIL) TestBindTranslation_RejectsForeignSource PASS (never reaches the stager) TestBindStaging_UnstagedPathLeaks + TestResolveBeneathWalk_MatchesOpenat2 still PASS — the unprivileged tier keeps real coverage EPHEMERD_TEST_REQUIRE_MOUNT=1 correctly turns the skip into a failure Privileged, EPHEMERD_TEST_REQUIRE_MOUNT=1 so nothing may skip: pkg/dind and pkg/runtime ok, control CONTAINER-SAW: SWAPPED, staged CONTAINER-SAW: PINNED, proc-fd source still EINVAL Windows pkg/dind and pkg/runtime ok. gofmt, go build ./... and go vet ./... clean; no BOM or mojibake left; zero dind-binds entries in /proc/self/mountinfo afterwards.
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
1 participant
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.
Fixes #125 — a TOCTOU in dind bind translation that lets a job escape the runner rootfs.
The bug
translateBindSourcedidos.Stat(candidate), validated containment, and returnedcandidateas a path string. That string went into the OCI spec and runc walked it again, later, in its own process. Two walks of a job-controlled directory: rename the validated dir, drop a symlink to/in its place, and runc mounts the attacker's target.Three cases, run against real runc 1.3.4 — the same version the fleet embeds
Case B independently confirms the existing
fix/linux-isolation-hardeningbranch is a hard functional regression, not a fix: a bind source must live in the caller's mount namespace, ephemerd is a host daemon, and siblings always get their own namespace — so every pinned bind fails closed, legitimate ones included. Only itsopenat2resolver is kept here; its #125 portion should be dropped, not rebased.Design
openat2(RESOLVE_IN_ROOT|RESOLVE_NO_MAGICLINKS)anchored at the branch root, held asO_PATH. Pre-5.6 fallback walk retained. Auto-mkdir viamkdiratagainst a pinned parent +O_NOFOLLOWreopen.mount("/proc/self/fd/N" → <data>/dind-binds/<job>/<n>)in ephemerd's own namespace, and the spec carries the staging path. The leaf is a bare counter — nothing from the request enters the path.ensureTrustedAncestryverifies every component is root-owned and not group/other-writable rather than assuming it, and fails the bind if not.The detail that matters most
Staging lives at
<data>/dind-binds/, deliberately not under<data>/jobs/: the runtime's orphan sweepRemoveAlls that tree, and deleting through a live bind mount deletes the source — the runner's rootfs.unmountTreeAndRemoverefuses to remove a directory that still has mounts beneath it, andSweepStagedBindsruns before the container/snapshot sweep, because a leaked staging mount pins the rootfs and makes the snapshot undeletable.Pins are held on
containerEntryfor the container's life, not just to task start —docker restartmakes runc re-read the spec.open_tree(OPEN_TREE_CLONE)+move_mountwas evaluated and rejected: it needs the runtime to accept a mount fd, which the OCI spec does not express and containerd does not plumb. Recorded in the arch doc as the cleaner future option.Validation
Linux (WSL2, root, real runc + real containerd) — where the security property is actually proven:
TestBindStaging_RealRunc_SwapDoesNotLeak— controlSWAPPED, stagedPINNED. The control is in the same test deliberately; without it a green run proves nothing.TestBindStaging_RealRunc_ProcFdSourceIsRejected— keeps the dead end as an executable fact.TestBindStaging_LegitimateBindsStillWork— the regression the previous attempt shipped: directory, regular file, auto-mkdir, merged-usr/bin → /usr/bin, anddocker.sock//etc/hosts//etc/resolv.confpassthrough, all throughbuildBindMountswith the real stager.pkg/dind+pkg/runtimeok, includingTestBindTranslation_RealContainerd.go test -raceok. Zero mounts left underdind-bindsafterwards.Windows: packages ok; bind translation is untouched there and the
_other.gofiles are dev-host stubs carrying no security property, labelled as such.Running on Linux caught two things Windows unit tests could not — the same blind spot that let the broken fix through review: a symlink test using the rootfs's host path (which
RESOLVE_IN_ROOTcorrectly refuses), and anEBUSYon dataDir cleanup because the staging dir is itself a mount.CI's Linux runner is unprivileged, so it cannot run the privileged tests. A
Privileged-test coverage reportstep makes the SKIP visible rather than implied by a green package, following the precedent already inci.yml.Behaviour change
A source that cannot be staged now fails
docker createwith 400 instead of silently mounting something unvalidated. The error names the staging dir and the requirement.Not validated
Live fleet end-to-end (not deployed); the pre-5.6 fallback walk (no such kernel available); arm64 (all runc work was amd64); and the in-VM data dir on a virtiofs/9p share.
Related
#172 (
--security-opt seccomp=unconfinedbypasses theallow_privilegedgate) was found during this work, verified onmain, and deliberately not fixed here — separate blast radius, deserves its own review. #130, #102 and #126 are independent of this fix and should be three separate PRs.