diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 261b9b83..e50f63de 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -42,6 +42,30 @@ jobs: -run TestImagePull_SharedNamespaceHitIsUsableFromJobNamespace \ -v ./pkg/dind/ + # Same rationale as the step above, for the #125 bind-staging tests. The + # security property they cover — that a job cannot swap a bind source + # between validation and runc's mount — is only observable when mount(2) + # is available, which it is not on this unprivileged runner. Printing + # the SKIP keeps that on the record instead of letting a green package + # imply coverage it does not have; the previous attempt at #125 passed + # review on exactly that misreading. + # + # TestBindTranslation_RealContainerd is in the list because it USED to + # run here and no longer does: bind translation now stages every + # job-supplied source as a real bind mount, so the test needs mount(2) + # where it previously did not. That is a genuine loss of CI coverage and + # it should be legible as a SKIP line, not inferred from a green package. + # It is not fixable by making staging degrade when unprivileged — that + # reinstates the escape and turns every one of these tests green while + # the vulnerability is live. + # + # Keep the -run pattern in sync with the test names; see requireBindStaging. + - name: Privileged-test coverage report (dind bind staging, #125) + run: | + go test -tags containers_image_openpgp -count=1 \ + -run 'TestBindStaging|TestBindTranslation|TestSweepStagedBinds|TestEnsureTrustedAncestry|TestUnmountTreeAndRemove|TestResolveBeneathWalk' \ + -v ./pkg/dind/ + - name: E2E — GitHub provider (fake server) run: mage e2egithub diff --git a/docs/arch/dind-bind-translation.md b/docs/arch/dind-bind-translation.md index 7ef48e01..17f553cc 100644 --- a/docs/arch/dind-bind-translation.md +++ b/docs/arch/dind-bind-translation.md @@ -142,7 +142,127 @@ overlay mount so every layer's content is visible. malicious `/home/runner/../../etc/shadow` resolves to `/etc/shadow` and either falls into A's rootfs (which means the sibling sees A's own `/etc/shadow` — exactly what A could already see) or fails to resolve at -all. There is no source path that escapes the runner's rootfs envelope. +all. + +Lexical cleaning is not sufficient on its own — see the next section. + +## Resolution and staging (issue #125) + +The original design resolved a source to a *path string* and put that string +in B's OCI spec. runc then walked the string again, in its own process, at +task start. The job owns every byte of A's rootfs, so it could swap a +validated directory for a symlink in between and runc would mount the +symlink's target. Reproduced against real runc 1.3.4: the container received +the swapped target, exit status 0, nothing logged anywhere. + +Two things fix it, and both are necessary. + +**1. Contained resolution, once, to a descriptor** +(`pkg/dind/bindpin_linux.go`). Every source with a job-supplied component is +resolved with `openat2(2)` under `RESOLVE_IN_ROOT | RESOLVE_NO_MAGICLINKS`, +anchored at the branch's root (A's rootfs, or the host source from A's bind +table). The kernel enforces containment during the walk instead of a string +comparison afterwards, and the result is held open as an `O_PATH` descriptor, +so renaming or replacing a component afterwards cannot change what it points +at. Pre-5.6 kernels fall back to an `O_PATH|O_NOFOLLOW` +component-by-component walk that holds every directory on the way down open, +so `..` steps back to a descriptor already held rather than re-opening a +parent by name. `TestResolveBeneathWalk_MatchesOpenat2` asserts the two +resolvers land on the same inode — including for `..` arriving from a symlink +target, which is the case that made an earlier version of the walk stricter +than `openat2` while claiming equivalence. The fleet is all 6.x, so the walk +is reached only if the latch below trips. + +The "no openat2" latch is process-global and permanent, so only `ENOSYS` and +`E2BIG` set it — errors that can only mean the kernel lacks the syscall. A +refusal (`EPERM`, e.g. a seccomp filter) falls back for that one call without +latching, and `EACCES`/`EINVAL` are ordinary failures that fail the bind +closed. Whichever path it takes, the first bind afterwards logs a WARN naming +the reason; a node silently resolving binds by the fallback used to be +indistinguishable from one that was not. + +`RESOLVE_IN_ROOT` rather than Go's `os.Root`: an absolute symlink is +*reinterpreted* relative to the root, which is what a container rootfs means +and what merged-usr images (`/bin -> /usr/bin`) require. `os.Root` rejects +absolute symlinks outright and would break every Ubuntu runner image. Note the +consequence: a symlink inside A's rootfs whose target is the *host* path of +that rootfs no longer resolves. That cannot occur in a container image — +nothing inside a container can name the host path of its own rootfs — and +following it would mean resolving against the host's root, which is the escape. + +**2. Staging, because a descriptor cannot go in the spec** +(`pkg/dind/bindstage_linux.go`). The obvious handoff — putting +`/proc//fd/` in `ocispec.Mount.Source` — does not work. runc +does not re-resolve the string (its own error echoes it verbatim), but +`mount(2)` rejects it: + +``` +error mounting "/proc/3543706/fd/3" to rootfs at "/marker": +mount src=/proc/3543706/fd/3, dst=/marker, dstFd=/proc/thread-self/fd/11, +flags=MS_BIND|MS_REC: invalid argument +``` + +A bind source must live in the *caller's* mount namespace, and runc always has +its own. The rejection is unconditional — legitimate binds fail identically — +so an fd handoff is a functional regression, not a fix. (This is recorded as +an executable test, `TestBindStaging_RealRunc_ProcFdSourceIsRejected`, so the +next person does not rediscover it from an outage.) + +Instead ephemerd performs the bind itself, in its own mount namespace where +`/proc/self/fd/` resolves fine, onto a path it owns: + +``` +openat2(RESOLVE_IN_ROOT|RESOLVE_NO_MAGICLINKS) -> pinned fd +mount("/proc/self/fd/N" -> /dind-binds//) (same ns: works) +spec.Source = /dind-binds// +``` + +runc re-walks the staging path, which is fine: nothing in the name is derived +from the job's request (the leaf is a bare counter), and no component of it is +swappable by anyone but root. `ensureTrustedAncestry` checks that precondition +on first use and fails the bind rather than assuming it — every component must +be a real directory (not a symlink) owned by root or by ephemerd's own euid, +and not group- or other-writable unless it carries the sticky bit, which is +what makes a data dir under `/tmp` acceptable while a plain group-writable one +is not. The staging directories ephemerd creates itself are 0700. + +Sources with no job-supplied +component — `/var/run/docker.sock`, `/etc/hosts`, `/etc/resolv.conf`, the +runner-mount root itself — are still passed through unpinned and unstaged; +their paths are entirely ephemerd's. + +**Staging directory location.** `/dind-binds//`, deliberately +*not* under `/jobs//`, which the runtime's orphan sweep +`os.RemoveAll`s. Recursively deleting a directory containing a live bind mount +deletes the files visible *through* the mount — here, A's own rootfs. + +**Lifecycle.** Pins are held on the `containerEntry` for the container's whole +life (not just until the task starts: `docker restart` makes runc re-read the +spec) and released in `cleanupContainer`. `Server.Stop` tears down the job's +whole staging directory as a backstop. A hard kill skips both, so +`dind.SweepStagedBinds` runs at daemon startup, from `Runtime.CleanOrphans`, +*before* the container and snapshot sweep — a leaked staging mount holds a +reference to the rootfs it was bound from, which makes the snapshot +undeletable. + +**Failure mode inverted, on purpose.** Before, a swapped source mounted +silently. Now a source that cannot be staged fails the `docker create` with a +400. That is the correct direction, but it is a new way for a job to break, so +the error names the staging directory and the requirement (root with +`CAP_SYS_ADMIN`, writable data dir). + +**Operator note.** `ensureTrustedAncestry` is the one new way an otherwise +working deployment can start rejecting every bind: an ephemerd whose +`--data-dir` sits under a group- or world-writable directory without the +sticky bit now fails `docker create` instead of staging into a path someone +else could swap. The default `/var/lib/ephemerd` is fine, and so is anything +under `/tmp` (sticky). A custom data dir on a shared or relaxed-permission +mount is not, and the error says which component and why. + +**Windows and macOS are unaffected.** Bind translation is not wired into +either native container path; `bindpin_other.go` / `bindstage_other.go` exist +only so the translation policy tests build and run on a dev host, and they +carry no security property. ## Security envelope @@ -250,21 +370,50 @@ the Windows-native runner code path. There are two scenarios: lands in lowerdir ro), unknown source surfaces a 400-shaped error, no-rootfs-registered rejects rather than silently allowing. -All tests pass with `CGO_ENABLED=0 go test ./pkg/dind/` and don't +All of the above pass with `CGO_ENABLED=0 go test ./pkg/dind/` and don't require a real containerd. +`pkg/dind/bindstage_linux_test.go` (Linux, **root only** — `mount(2)` needs +`CAP_SYS_ADMIN` and the project's Linux CI runner is unprivileged) covers the +staging layer itself: the swap-after-validation escape, a control proving the +pre-fix shape *does* follow the swap, the legitimate-bind regression set +(directory, regular file, auto-mkdir, merged-usr symlink traversal, and the +three passthrough binds), teardown, the startup sweep, and the trusted-ancestry +precondition. + +`pkg/dind/bindstage_runc_linux_test.go` (Linux, root, and +`EPHEMERD_TEST_RUNC=`) is the end-to-end proof, because nothing +on the daemon side can observe what runc's `mount(2)` resolves: + +``` +sudo EPHEMERD_TEST_RUNC=$PWD/pkg/containerd/embed/runc \ + go test ./pkg/dind/ -run TestBindStaging_RealRunc -v +``` + +Against runc 1.3.4 it reports the control leaking (`CONTAINER-SAW: SWAPPED`), +the staged source holding (`CONTAINER-SAW: PINNED`), and the `/proc//fd` +source being rejected with `invalid argument`. + +**A green `go test ./pkg/dind/` on an unprivileged host proves none of this.** +The tests that carry the security property skip there, loudly. The previous +attempt at #125 shipped a spec runc rejects outright and the package was still +`ok`. + ## Deferred follow-ups - **Windows-native `container:`.** Different snapshotter and mount semantics; needs its own translation layer or a clean "not supported" rejection at request time. -- **Symlink hardening.** `filepath.Clean` handles `..` but not symlinks - that resolve outside the runner rootfs. The current upperdir/lowerdir - walk only honors paths that exist as plain files/dirs within the - layers, so we don't currently *open* the door — but if a future - layer walk were to add `filepath.EvalSymlinks`, the call needs an - after-the-fact prefix check to confirm the resolved path stays inside - the snapshot directory. +- ~~**Symlink hardening.**~~ Done, and then done again properly: the + after-the-fact prefix check this bullet proposed was implemented, and was + the bug — see "Resolution and staging (issue #125)" above. Containment is + now enforced by the kernel during a single resolution, and the result is a + descriptor rather than a name. +- **`open_tree(OPEN_TREE_CLONE)` + `move_mount`.** A cleaner way to hand a + detached mount to the runtime than a staging path, and it would remove the + staging directory and its lifecycle entirely. It needs the runtime to accept + a mount fd, which the OCI spec does not express and containerd does not + plumb, so it is not reachable from here today. - **Resolved-path caching.** Each `buildBindMounts` call queries the snapshotter and `os.Stat`s every source. A given runner doesn't change its layers within a job, so the resolution can be cached diff --git a/pkg/dind/bindpin.go b/pkg/dind/bindpin.go new file mode 100644 index 00000000..621bdeb3 --- /dev/null +++ b/pkg/dind/bindpin.go @@ -0,0 +1,161 @@ +package dind + +import ( + "os" + "path" + "strings" + "sync" +) + +// bindPin is a bind source that has been resolved ONCE, under containment, and +// is held so that what eventually gets mounted is the object that was checked. +// +// THE BUG THIS EXISTS TO KILL (issue #125, TOCTOU / check-then-use): +// +// Bind translation maps a sibling container's `-v` source (a path in the +// runner's mount namespace) onto a real path on the dind daemon's filesystem, +// and has to prove the result stays inside the runner's rootfs — otherwise a +// job can ask for `-v /esc/x:/y` where `/esc` is a symlink it planted pointing +// at `/`, and the sibling container receives the node's filesystem. +// +// The pre-fix implementation proved containment by resolving symlinks +// (filepath.EvalSymlinks) and then returned the ORIGINAL, unresolved joined +// string. Everything between that check and runc's mount(2) — the rest of +// translation, the containerd container create, the whole Docker +// create-then-start round trip — was a window in which the job (which owns +// every byte of its own rootfs) could replace a validated directory component +// with a symlink out of the rootfs. runc then walked the string again and +// mounted whatever it pointed at by then. Reproduced against real runc 1.3.4: +// the container saw the swapped target, exit status 0, no error anywhere. +// +// A bindPin removes the second walk. The path is resolved once under +// kernel-enforced containment (openat2 with RESOLVE_IN_ROOT) and the resulting +// inode is held open as an O_PATH descriptor. Renaming or replacing a path +// component afterwards cannot change where the descriptor points. +// +// The descriptor is NOT what goes into the OCI spec. See bindStager: a +// /proc//fd/ reference is rejected by mount(2) when the mounter is in +// a different mount namespace, which runc always is. The descriptor is instead +// materialized as a bind mount at a path ephemerd owns, and that path is what +// the spec carries. +type bindPin struct { + // logical is the human-readable path the pin was resolved from + // (root + relative components, before symlink resolution). Diagnostics, + // logging and tests — and, off Linux, the source itself, because there + // is no dind daemon there to defend. Never used as the mount source on + // Linux: mounting a path is exactly the re-resolution this type exists + // to prevent. + logical string + // mode is the file type/permission bits of the pinned object, captured + // through the descriptor (not from a second stat of the path). The + // staging mountpoint is created to match: a directory for a directory, + // an empty file for a file. + mode os.FileMode + // fd is the O_PATH descriptor holding the resolved inode. -1 on + // platforms with no descriptor pinning (see bindpin_other.go), where + // there is also no bind translation in production. + fd int + // staged is the ephemerd-owned path the pin was published at, once a + // bindStager has done so. This is what the OCI spec carries. + staged string + // unstage tears down whatever the stager created. Set by the stager. + unstage func() error + // once makes Close exactly-once. A pin is released from two places that + // can legitimately both run (cleanupContainer on teardown, and the + // error paths out of container create), and a double close would free a + // descriptor number the process may have already handed to something + // else — a far worse bug than the leak it would be preventing. + once sync.Once +} + +// Logical is the path the pin was resolved from. Diagnostics only on Linux. +func (p *bindPin) Logical() string { + if p == nil { + return "" + } + return p.logical +} + +// Mode is the file mode of the pinned object. +func (p *bindPin) Mode() os.FileMode { + if p == nil { + return 0 + } + return p.mode +} + +// Staged is the ephemerd-owned path this pin was published at, or "" if it has +// not been staged yet. +func (p *bindPin) Staged() string { + if p == nil { + return "" + } + return p.staged +} + +// Close releases the staging mount (if any) and the pinned descriptor. Safe on +// nil, safe to call twice, and safe to call concurrently. +func (p *bindPin) Close() error { + if p == nil { + return nil + } + var err error + p.once.Do(func() { + if p.unstage != nil { + err = p.unstage() + } + if p.fd >= 0 { + closePinFd(p.fd) + p.fd = -1 + } + }) + return err +} + +// closeBindPins releases a batch of pins. Used on every path out of container +// create/start once the mount has happened or been abandoned, and again at +// container cleanup. +func closeBindPins(pins []*bindPin) { + for _, p := range pins { + _ = p.Close() + } +} + +// pathComponents splits a POSIX path into its meaningful components, dropping +// "" and "." and rejecting "..". +// +// ".." is refused rather than resolved: the callers pass paths that have +// already been lexically cleaned, so a remaining ".." can only have come from +// a symlink target, and "resolve it and re-check" is exactly the pattern that +// produced the TOCTOU in the first place. Refusing matches what the kernel +// does for us under RESOLVE_IN_ROOT. +func pathComponents(rel string) ([]string, error) { + rel = strings.TrimPrefix(path.Clean("/"+rel), "/") + if rel == "" || rel == "." { + return nil, nil + } + parts := strings.Split(rel, "/") + out := make([]string, 0, len(parts)) + for _, p := range parts { + switch p { + case "", ".": + continue + case "..": + return nil, errBindPathTraversal + } + out = append(out, p) + } + return out, nil +} + +// logicalPath is the human-readable join of a bind root and its resolved +// components. Deliberately path.Join (POSIX) rather than filepath.Join: the +// components come from a Linux mount namespace, and keeping the join stable +// across build hosts is what lets the cross-platform translation tests assert +// exact strings. +func logicalPath(root string, comps []string) string { + if len(comps) == 0 { + return path.Clean(root) + } + return path.Join(root, strings.Join(comps, "/")) +} diff --git a/pkg/dind/bindpin_linux.go b/pkg/dind/bindpin_linux.go new file mode 100644 index 00000000..1feee405 --- /dev/null +++ b/pkg/dind/bindpin_linux.go @@ -0,0 +1,427 @@ +//go:build linux + +package dind + +import ( + "errors" + "fmt" + "io/fs" + "os" + "strings" + "sync/atomic" + + "golang.org/x/sys/unix" +) + +// errBindPathTraversal is returned for a bind source that contains a ".." +// component after cleaning (which can only come from a symlink target). +var errBindPathTraversal = errors.New(`bind source contains a ".." component, which is not permitted`) + +// maxPinSymlinks bounds symlink expansion in the openat2-less fallback walk. +// Matches the kernel's own MAXSYMLINKS. +const maxPinSymlinks = 40 + +// openat2Unsupported latches once the kernel is known not to have openat2, so +// the fallback does not pay for a failing syscall on every bind. Atomic +// because sibling container creates are served concurrently. +// +// The latch is process-global and permanent, which is why only errors that +// genuinely mean "this kernel does not implement the syscall" set it — see +// openat2Missing. Anything conditional (a seccomp filter, a transient EPERM) +// falls back for that one call instead, so a single odd error cannot silently +// convert the node's resolver for the rest of the daemon's uptime. +var openat2Unsupported atomic.Bool + +// openat2FallbackNote carries the reason the resolver fell back, so it reaches +// the operator's log instead of nothing at all. Set at most once per process; +// delivered at most once, by the first bind that looks (see +// openat2FallbackNotice), because pinBindSource has no logger of its own and +// threading one through every call site would be a lot of churn for a +// once-per-process event. +var ( + openat2Noted atomic.Bool + openat2FallbackR atomic.Pointer[string] +) + +func noteOpenat2Fallback(reason string) { + if openat2Noted.CompareAndSwap(false, true) { + openat2FallbackR.Store(&reason) + } +} + +// openat2FallbackNotice returns the fallback reason exactly once, then "". +// Callers with a logger (buildBindMounts) surface it. +func openat2FallbackNotice() string { + if r := openat2FallbackR.Swap(nil); r != nil { + return *r + } + return "" +} + +// closePinFd drops a pinned descriptor. Close errors on a descriptor being +// discarded are not actionable. +func closePinFd(fd int) { _ = unix.Close(fd) } + +// pinBindSource resolves rel underneath root and returns a handle pinned to +// the resolved inode. rel is a POSIX path relative to root (a leading "/" is +// ignored); an empty rel pins root itself. +// +// Containment is enforced by the KERNEL during resolution, not by a string +// comparison afterwards: +// +// - openat2(2) with RESOLVE_IN_ROOT treats root as if it were the process +// root for this one resolution. Symlinks are still followed — the runner's +// rootfs legitimately contains plenty — but a symlink (or a chain of them, +// or a "..") that would leave root cannot: an absolute symlink is +// re-anchored at root, and ".." at root is a no-op. This is exactly the +// view the runner itself has of its own filesystem, which is the view the +// `-v` source was written against. It is also why this is openat2 and not +// Go's os.Root, which rejects absolute symlinks outright and would break +// every bind traversing merged-usr's /bin -> /usr/bin. +// - RESOLVE_NO_MAGICLINKS forbids traversing /proc//fd style magic +// links, which would otherwise be a way back out through a descriptor the +// resolution never inspected. +// +// The result is a descriptor, so there is nothing left for the job to +// re-point. Turning it into something runc can mount is the stager's job. +// +// autoCreate mirrors Docker's behaviour for a `-v` source that does not exist +// yet (the GHA runner emits binds for directories a later step creates). The +// creation is done with mkdirat(2) relative to a pinned parent descriptor and +// each new component is re-opened with O_NOFOLLOW, so a job racing to plant a +// symlink where we are about to mkdir loses: mkdirat fails EEXIST and the +// O_NOFOLLOW|O_DIRECTORY open then fails ENOTDIR/ELOOP rather than following +// it out. +func pinBindSource(root, rel string, autoCreate bool) (*bindPin, error) { + comps, err := pathComponents(rel) + if err != nil { + return nil, err + } + logical := logicalPath(root, comps) + + if len(comps) == 0 { + // Pin root itself. root is a path ephemerd chose (the runner rootfs, + // or a host source out of its own bind table), never one the job + // supplied, so following symlinks in it is not attacker-controlled. + fd, err := unix.Open(root, unix.O_PATH|unix.O_CLOEXEC, 0) + if err != nil { + return nil, fmt.Errorf("opening bind root %s: %w", root, err) + } + return newBindPin(fd, logical) + } + + rootFd, err := openPathDir(root) + if err != nil { + return nil, fmt.Errorf("opening bind root %s: %w", root, err) + } + defer closePinFd(rootFd) + + fd, err := resolveBeneath(rootFd, comps) + if err != nil { + if !autoCreate || !errors.Is(err, fs.ErrNotExist) { + return nil, err + } + fd, err = createBeneath(rootFd, comps) + if err != nil { + return nil, err + } + } + return newBindPin(fd, logical) +} + +// openPathDir opens a directory as an O_PATH anchor. O_PATH means the +// descriptor can only be used to name things relative to it — it grants no +// read or write access to the directory's contents, which is all a resolution +// anchor needs. +func openPathDir(p string) (int, error) { + return unix.Open(p, unix.O_PATH|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) +} + +// newBindPin wraps an already-open O_PATH descriptor. The mode comes from +// fstat on that same descriptor, so the type check the caller performs is +// about the pinned inode and not about whatever the path names by then. +func newBindPin(fd int, logical string) (*bindPin, error) { + var st unix.Stat_t + if err := unix.Fstat(fd, &st); err != nil { + closePinFd(fd) + return nil, fmt.Errorf("stat pinned bind source %s: %w", logical, err) + } + return &bindPin{ + logical: logical, + mode: fileModeFromStat(st.Mode), + fd: fd, + }, nil +} + +// fileModeFromStat converts a raw st_mode into the os.FileMode bits the +// callers care about (IsDir / IsRegular). +func fileModeFromStat(m uint32) os.FileMode { + mode := os.FileMode(m & 0o777) + switch m & unix.S_IFMT { + case unix.S_IFDIR: + mode |= os.ModeDir + case unix.S_IFLNK: + mode |= os.ModeSymlink + case unix.S_IFSOCK: + mode |= os.ModeSocket + case unix.S_IFIFO: + mode |= os.ModeNamedPipe + case unix.S_IFBLK: + mode |= os.ModeDevice + case unix.S_IFCHR: + mode |= os.ModeDevice | os.ModeCharDevice + } + return mode +} + +// resolveBeneath opens comps relative to rootFd without ever leaving rootFd, +// returning an O_PATH descriptor for the result. +func resolveBeneath(rootFd int, comps []string) (int, error) { + if len(comps) == 0 { + return unix.Dup(rootFd) + } + if !openat2Known() { + return resolveBeneathWalk(rootFd, comps) + } + how := &unix.OpenHow{ + Flags: unix.O_PATH | unix.O_CLOEXEC, + Resolve: unix.RESOLVE_IN_ROOT | unix.RESOLVE_NO_MAGICLINKS, + } + fd, err := unix.Openat2(rootFd, strings.Join(comps, "/"), how) + if err == nil { + return fd, nil + } + if openat2Missing(err) { + openat2Unsupported.Store(true) + noteOpenat2Fallback(fmt.Sprintf("openat2(2) is not implemented on this kernel (%v); dind bind sources will be resolved by the equivalent O_PATH|O_NOFOLLOW walk for the rest of this process", err)) + return resolveBeneathWalk(rootFd, comps) + } + if openat2Blocked(err) { + // Conditional, so it does not latch: a seccomp filter that rejects + // the syscall will reject it again next time and we will fall back + // again, at the cost of one failing syscall per bind. That is cheap, + // and it means a one-off EPERM cannot permanently downgrade the node. + noteOpenat2Fallback(fmt.Sprintf("openat2(2) was refused (%v), most likely by a seccomp filter; dind bind sources are being resolved by the equivalent O_PATH|O_NOFOLLOW walk instead", err)) + return resolveBeneathWalk(rootFd, comps) + } + return -1, err +} + +// openat2Known reports whether openat2 is worth trying. The probe is the first +// real call; after that the answer is latched. +func openat2Known() bool { return !openat2Unsupported.Load() } + +// openat2Missing reports errors that can only mean the kernel does not +// implement openat2 at all: ENOSYS on pre-5.6, and E2BIG for an OpenHow the +// kernel cannot parse. These latch. +func openat2Missing(err error) bool { + return errors.Is(err, unix.ENOSYS) || errors.Is(err, unix.E2BIG) +} + +// openat2Blocked reports errors that mean something is refusing the syscall +// rather than lacking it. These fall back without latching. +func openat2Blocked(err error) bool { + return errors.Is(err, unix.EPERM) +} + +// EACCES and EINVAL are deliberately in NEITHER list. +// +// EACCES is an ordinary resolution outcome — one directory along the path we +// may not search — and treating it as "no openat2" would downgrade the whole +// node's resolver because of a single unreadable directory. +// +// EINVAL means a malformed OpenHow (bad flags, or a reserved field set). It is +// unreachable for a correct call: RESOLVE_IN_ROOT shipped in the same release +// as openat2 itself, so there is no kernel that has one without the other. If +// it ever does happen, the bind fails closed with the real error rather than +// quietly switching resolvers. (Note for anyone reading the git history: an +// earlier version of this comment justified excluding EINVAL by claiming +// RESOLVE_IN_ROOT returns it when a path escapes. It does not — it clamps the +// escape instead. EXDEV is RESOLVE_BENEATH's escape error, and we do not use +// RESOLVE_BENEATH. Right call, wrong reason.) + +// resolveBeneathWalk is the openat2-less fallback: a manual component-by- +// component walk that reproduces RESOLVE_IN_ROOT semantics. +// +// It is race-free for the same reason openat2 is: every step is an openat(2) +// relative to a descriptor we already hold, with O_NOFOLLOW, so a component +// swapped after we have opened it cannot redirect the walk. Symlinks are read +// from the descriptor (readlinkat with an empty path), never re-opened by +// name, and an absolute target restarts the walk at rootFd instead of at the +// node's real root. +// +// ".." is handled by holding every directory on the way down open and stepping +// back to the one above — never by re-opening a parent by name, which would be +// a second walk of the sort this whole mechanism exists to remove. At the root +// it is a no-op, exactly as RESOLVE_IN_ROOT clamps it. +// +// An earlier version refused ".." outright, reasoning that a lexically cleaned +// bind source cannot contain one. That is true of the source, but not of a +// SYMLINK TARGET, which is spliced into the walk here — and relative "../" +// targets are everywhere in real images (/etc/alternatives/*, Debian +// multiarch, tool caches). It failed closed rather than unsafely, but it made +// this path meaningfully stricter than openat2 while the comment claimed +// equivalence, which would have surfaced as legitimate binds 400ing on any +// node that ever fell back. +// +// This exists so a node on a pre-5.6 kernel degrades to "slower but equally +// contained" rather than to "unprotected" or "dind is broken". +func resolveBeneathWalk(rootFd int, comps []string) (int, error) { + root, err := unix.Dup(rootFd) + if err != nil { + return -1, fmt.Errorf("dup bind root: %w", err) + } + // stack[0] is always the bind root; the last element is the current + // directory. Everything in between is held open so ".." can step back + // without naming anything. + stack := []int{root} + closeStack := func() { + for _, fd := range stack { + closePinFd(fd) + } + } + + remaining := append([]string(nil), comps...) + links := 0 + + for len(remaining) > 0 { + name := remaining[0] + remaining = remaining[1:] + switch name { + case "", ".": + continue + case "..": + if len(stack) > 1 { + closePinFd(stack[len(stack)-1]) + stack = stack[:len(stack)-1] + } + // At the root, ".." is a no-op: there is no "above" to reach. + continue + } + + cur := stack[len(stack)-1] + next, err := unix.Openat(cur, name, unix.O_PATH|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0) + if err != nil { + closeStack() + return -1, err + } + var st unix.Stat_t + if err := unix.Fstat(next, &st); err != nil { + closePinFd(next) + closeStack() + return -1, fmt.Errorf("stat %s during bind resolution: %w", name, err) + } + if st.Mode&unix.S_IFMT != unix.S_IFLNK { + stack = append(stack, next) + continue + } + + // Symlink: expand it in place rather than following it by name. The + // link itself is not descended into, so the stack does not grow. + links++ + if links > maxPinSymlinks { + closePinFd(next) + closeStack() + return -1, unix.ELOOP + } + target, err := readlinkFd(next) + closePinFd(next) + if err != nil { + closeStack() + return -1, err + } + if strings.HasPrefix(target, "/") { + // Absolute target: re-anchor at the bind root, which is what + // RESOLVE_IN_ROOT does and what the path means from inside the + // runner's own namespace. + for _, fd := range stack[1:] { + closePinFd(fd) + } + stack = stack[:1] + } + remaining = append(strings.Split(target, "/"), remaining...) + } + + result := stack[len(stack)-1] + for _, fd := range stack[:len(stack)-1] { + closePinFd(fd) + } + return result, nil +} + +// readlinkFd reads the target of the symlink an O_PATH|O_NOFOLLOW descriptor +// refers to. readlinkat with an empty pathname operates on dirfd itself. +func readlinkFd(fd int) (string, error) { + buf := make([]byte, unix.PathMax) + n, err := unix.Readlinkat(fd, "", buf) + if err != nil { + return "", fmt.Errorf("reading symlink during bind resolution: %w", err) + } + if n <= 0 || n >= len(buf) { + return "", fmt.Errorf("symlink target during bind resolution is empty or too long (%d bytes)", n) + } + return string(buf[:n]), nil +} + +// createBeneath materializes a bind source that does not exist yet, then pins +// it. Only reached when the caller asked for Docker's auto-mkdir behaviour. +// +// The deepest existing prefix is resolved with the same contained resolver; +// only the missing tail is created, each component with mkdirat(2) against a +// held descriptor and re-opened with O_NOFOLLOW|O_DIRECTORY. New directories +// inherit uid/gid from that closest existing ancestor, which is what lets the +// GHA runner (uid 1001) write into a directory we created on its behalf. +func createBeneath(rootFd int, comps []string) (int, error) { + for i := len(comps) - 1; i >= 0; i-- { + parentFd, err := resolveBeneath(rootFd, comps[:i]) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + continue + } + return -1, err + } + return createUnder(parentFd, comps[i:]) + } + return -1, fmt.Errorf("no existing ancestor found for bind source %q beneath the bind root", strings.Join(comps, "/")) +} + +// createUnder creates missing under parentFd and returns a pinned descriptor +// for the last component. Takes ownership of parentFd. +func createUnder(parentFd int, missing []string) (int, error) { + var pst unix.Stat_t + if err := unix.Fstat(parentFd, &pst); err != nil { + closePinFd(parentFd) + return -1, fmt.Errorf("stat bind source ancestor: %w", err) + } + + cur := parentFd + for _, name := range missing { + if err := unix.Mkdirat(cur, name, 0o755); err != nil && !errors.Is(err, fs.ErrExist) { + closePinFd(cur) + return -1, fmt.Errorf("creating bind source component %q: %w", name, err) + } + // O_NOFOLLOW|O_DIRECTORY: if the mkdirat lost a race to a symlink + // planted by the job, this fails rather than following it. + next, err := unix.Openat(cur, name, unix.O_PATH|unix.O_NOFOLLOW|unix.O_DIRECTORY|unix.O_CLOEXEC, 0) + if err != nil { + closePinFd(cur) + // The common way to get here is that mkdirat returned EEXIST + // because a symlink already occupies the name — either planted + // ahead of us or swapped in during the race. O_NOFOLLOW turns + // that into ELOOP/ENOTDIR instead of an escape, which is the + // entire point, so say what it means. + return -1, fmt.Errorf("bind source component %q could not be opened as a directory after creation; something (most likely a symlink whose target escapes the bind root) already occupies that name: %w", name, err) + } + // Inherit ownership from the closest pre-existing ancestor so the + // runner user can populate what we made for it. Done through the + // descriptor, so it cannot be redirected either. + if err := unix.Fchownat(next, "", int(pst.Uid), int(pst.Gid), unix.AT_EMPTY_PATH); err != nil { + closePinFd(next) + closePinFd(cur) + return -1, fmt.Errorf("chown auto-created bind source component %q to %d:%d: %w", name, pst.Uid, pst.Gid, err) + } + closePinFd(cur) + cur = next + } + return cur, nil +} diff --git a/pkg/dind/bindpin_other.go b/pkg/dind/bindpin_other.go new file mode 100644 index 00000000..2cc2d5f4 --- /dev/null +++ b/pkg/dind/bindpin_other.go @@ -0,0 +1,128 @@ +//go:build !linux + +package dind + +import ( + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + "strings" +) + +// errBindPathTraversal is returned for a bind source that contains a ".." +// component after cleaning (which can only come from a symlink target). +var errBindPathTraversal = errors.New(`bind source contains a ".." component, which is not permitted`) + +// closePinFd is a no-op off Linux: nothing here holds a descriptor. +func closePinFd(int) {} + +// openat2FallbackNotice has nothing to report off Linux — there is no openat2 +// and no fallback walk here. See bindpin_linux.go. +func openat2FallbackNotice() string { return "" } + +// pinBindSource is the non-Linux implementation. It exists so the translation +// logic and its tests build and run on a Windows or macOS dev host. +// +// IT IS NOT RACE-FREE, AND IT IS NOT A PRODUCTION PATH. There is no dind +// daemon off Linux: sibling containers are created by the linux build of +// ephemerd, either on a Linux node or inside the managed Linux VM on a +// Windows/macOS node, and bind translation is not wired into the Windows or +// macOS container paths at all (see runtime.go and containers.go). This +// implementation resolves symlinks and then compares strings — the same +// check-then-use shape bindPin exists to eliminate — and it returns a plain +// path rather than a descriptor, because there is no /proc//fd here and +// nothing to hand it to. +// +// Keeping it deliberately simple (rather than reimplementing a contained walk +// on Windows semantics) means the cross-platform tests exercise the policy — +// which sources are accepted, which are rejected, what gets auto-created — +// while the Linux file carries the security property. +func pinBindSource(root, rel string, autoCreate bool) (*bindPin, error) { + comps, err := pathComponents(rel) + if err != nil { + return nil, err + } + // logicalPath (POSIX join) is what callers compare against; the native + // filesystem calls below are happy with forward slashes on Windows too. + target := logicalPath(root, comps) + + info, err := os.Stat(target) + switch { + case err == nil: + if cerr := containedAfterResolve(root, target); cerr != nil { + return nil, cerr + } + return &bindPin{logical: target, mode: info.Mode(), fd: -1}, nil + case errors.Is(err, fs.ErrNotExist) && autoCreate: + if cerr := closestExistingAncestorContained(root, target); cerr != nil { + return nil, cerr + } + if mkErr := os.MkdirAll(target, 0o755); mkErr != nil { + return nil, fmt.Errorf("creating bind source %s: %w", target, mkErr) + } + if cerr := containedAfterResolve(root, target); cerr != nil { + return nil, cerr + } + info, serr := os.Stat(target) + if serr != nil { + return nil, serr + } + return &bindPin{logical: target, mode: info.Mode(), fd: -1}, nil + default: + return nil, err + } +} + +// containedAfterResolve reports whether target, fully symlink-resolved, is +// root or lives underneath it. +func containedAfterResolve(root, target string) error { + realRoot, err := filepath.EvalSymlinks(root) + if err != nil { + return fmt.Errorf("resolving bind root %s: %w", root, err) + } + realTarget, err := filepath.EvalSymlinks(target) + if err != nil { + return fmt.Errorf("resolving bind candidate %s: %w", target, err) + } + if !isWithin(realRoot, realTarget) { + return fmt.Errorf("resolved path %s escapes bind root %s", realTarget, realRoot) + } + return nil +} + +// closestExistingAncestorContained walks up from a path that does not exist +// yet to the first component that does, and checks containment there — so +// auto-mkdir cannot create a directory outside the root through a symlinked +// intermediate. +func closestExistingAncestorContained(root, target string) error { + ancestor := target + for { + if _, err := os.Lstat(ancestor); err == nil { + return containedAfterResolve(root, ancestor) + } else if !errors.Is(err, fs.ErrNotExist) { + return fmt.Errorf("resolving ancestor %s: %w", ancestor, err) + } + parent := filepath.Dir(ancestor) + if parent == ancestor { + return fmt.Errorf("walked past root resolving ancestor of %s", target) + } + ancestor = parent + } +} + +// isWithin reports whether target is root itself or lives underneath root. +// Both arguments are expected to be cleaned, symlink-resolved absolute paths. +// The separator-terminated prefix check prevents "/a/rootfs-evil" from +// matching root "/a/rootfs". +func isWithin(root, target string) bool { + if target == root { + return true + } + rootWithSep := root + if !strings.HasSuffix(rootWithSep, string(filepath.Separator)) { + rootWithSep += string(filepath.Separator) + } + return strings.HasPrefix(target, rootWithSep) +} diff --git a/pkg/dind/bindstage.go b/pkg/dind/bindstage.go new file mode 100644 index 00000000..df089ad9 --- /dev/null +++ b/pkg/dind/bindstage.go @@ -0,0 +1,77 @@ +package dind + +import ( + "log/slog" + "path/filepath" +) + +// stagingDirName is the immediate child of the ephemerd data directory under +// which every job's bind staging mounts live: /dind-binds//. +// +// It is deliberately NOT under /jobs//, which the runtime's +// orphan sweep os.RemoveAll's: recursively deleting a directory that has a +// bind mount inside it deletes the files visible THROUGH the mount, which here +// would be the runner's own rootfs. +const stagingDirName = "dind-binds" + +// stagingRootDir is the parent of every job's staging directory. +func stagingRootDir(dataDir string) string { + return filepath.Join(dataDir, stagingDirName) +} + +// jobStagingDir is where one job's staged bind sources are published. +func jobStagingDir(dataDir, jobID string) string { + return filepath.Join(stagingRootDir(dataDir), jobID) +} + +// bindStager publishes a resolved bind source at a path ephemerd controls, so +// that the path the OCI spec carries has nothing in it a job can influence. +// +// WHY THIS LAYER EXISTS. Pinning the source to a descriptor (see bindPin) is +// only half a fix. The other half is getting that pinned inode into the spec, +// and the obvious route — putting "/proc//fd/" in +// ocispec.Mount.Source — does not work. runc does not re-resolve the string +// (its own error echoes it verbatim), but mount(2) rejects it: +// +// error mounting "/proc/3543706/fd/3" to rootfs at "/marker": +// mount src=/proc/3543706/fd/3, dst=/marker, dstFd=/proc/thread-self/fd/11, +// flags=MS_BIND|MS_REC: invalid argument +// +// A bind source must live in the CALLER's mount namespace, and runc always +// has its own. That is unconditional — legitimate binds fail exactly the same +// way — so an fd handoff is a functional regression, not a fix. Verified +// against real runc 1.3.4 on kernel 6.8 and 6.18. +// +// What does work is to perform the bind in EPHEMERD's mount namespace, where +// the /proc/self/fd source resolves fine, onto a staging path ephemerd owns, +// and give runc that path. runc re-walks it — that is fine, because every +// component belongs to root and none of them is reachable from a job. +// +// Off Linux there is no dind daemon and no mount(2); see bindstage_other.go. +type bindStager interface { + // stage publishes p and returns the path to put in the OCI spec. It + // attaches the teardown to p, so releasing the pin releases the staging + // mount. Failure must fail the bind — falling back to the resolved path + // would reopen issue #125. + stage(p *bindPin) (string, error) + + // teardown releases everything this stager published, including mounts + // whose pins were lost. Called from Server.Stop. + teardown() +} + +// SweepStagedBinds removes bind staging mounts and directories left behind by +// a previous ephemerd process. STARTUP ONLY: it does not know which jobs are +// live, and unmounting a running job's staged bind would not break that job's +// already-running containers but would break any container it starts next. +// +// A hard kill (SIGKILL, panic, node reset) skips every teardown path, and the +// leaked mounts are not merely untidy: each one pins the runner container's +// rootfs mount, so containerd cannot delete the snapshot and the node +// accumulates undeletable snapshots until it runs out of disk. +func SweepStagedBinds(dataDir string, log *slog.Logger) { + if dataDir == "" { + return + } + sweepStagedBinds(stagingRootDir(dataDir), log) +} diff --git a/pkg/dind/bindstage_linux.go b/pkg/dind/bindstage_linux.go new file mode 100644 index 00000000..81475f4b --- /dev/null +++ b/pkg/dind/bindstage_linux.go @@ -0,0 +1,407 @@ +//go:build linux + +package dind + +import ( + "bufio" + "errors" + "fmt" + "log/slog" + "os" + "path/filepath" + "sort" + "strconv" + "strings" + "sync" + + "golang.org/x/sys/unix" +) + +// newBindStager builds the Linux bind stager for one job. dataDir is the +// ephemerd data directory; the job's staging mounts land under +// /dind-binds//. +func newBindStager(dataDir, jobID string, log *slog.Logger) bindStager { + return &mountStager{dir: jobStagingDir(dataDir, jobID), log: log} +} + +// mountStager materializes pinned bind sources as bind mounts under a +// per-job directory that only root can reach. See bindStager for why the +// mount is necessary and why handing runc a /proc//fd path is not an +// option. +type mountStager struct { + dir string + log *slog.Logger + + // mu serialises staging against teardown. It is held across the whole of + // stage() — including the mkdir and the mount — rather than just the + // bookkeeping, because teardown removes the directory tree and a mount + // appearing between its mount-check and its os.RemoveAll would have + // RemoveAll delete the files visible through that mount, i.e. the + // runner's rootfs. Staging a handful of binds per container create is not + // a contended path; correctness here is worth more than the concurrency. + mu sync.Mutex + // ready is set once the staging directory exists and has been proven + // safe. closed is set by teardown and never cleared: a stager that has + // been torn down must refuse to stage again rather than silently + // recreating the directory the caller just swept, which would leak a + // mount pinning the runner's rootfs until the next daemon startup. + ready bool + closed bool + seq uint64 +} + +// stage binds the pinned inode to / and returns that path. +// +// The name is a bare counter. Nothing derived from the job's requested source +// goes into it: the staging path is the one part of this whole mechanism the +// job must have no influence over, down to the characters in it. +func (m *mountStager) stage(p *bindPin) (string, error) { + if p == nil || p.fd < 0 { + return "", fmt.Errorf("bind source was not pinned; refusing to stage an unpinned source") + } + + m.mu.Lock() + defer m.mu.Unlock() + + if m.closed { + return "", fmt.Errorf("this job's bind staging directory has already been torn down; refusing to stage %s (the job is shutting down)", p.logical) + } + if err := m.ensureDirLocked(); err != nil { + return "", err + } + m.seq++ + target := filepath.Join(m.dir, strconv.FormatUint(m.seq, 10)) + + // The mountpoint has to match the source's type: a directory for a + // directory, an empty regular file for anything else (bind mounts of + // files are how /etc/hosts-shaped sources work). + if p.mode.IsDir() { + if err := os.Mkdir(target, 0o700); err != nil { + return "", fmt.Errorf("creating bind staging mountpoint %s: %w", target, err) + } + } else { + f, err := os.OpenFile(target, os.O_CREATE|os.O_EXCL|os.O_WRONLY, 0o600) + if err != nil { + return "", fmt.Errorf("creating bind staging mountpoint %s: %w", target, err) + } + _ = f.Close() + } + + // /proc/self/fd/ is resolvable here because this IS the process that + // opened it and this IS its mount namespace — the two conditions runc + // cannot satisfy. MS_REC mirrors the "rbind" the spec asks for, so a + // source that has submounts under it carries them across. + src := "/proc/self/fd/" + strconv.Itoa(p.fd) + if err := unix.Mount(src, target, "", unix.MS_BIND|unix.MS_REC, ""); err != nil { + _ = os.Remove(target) + return "", fmt.Errorf("staging bind source %s at %s: %w. "+ + "ephemerd binds every job-supplied -v source into %s before handing it to the container runtime, "+ + "so a job cannot swap the source between validation and mount. %s", + p.logical, target, err, m.dir, stagingMountAdvice(err)) + } + + p.staged = target + // Releasing one pin takes the same lock, so a pin close can never land + // its unmount inside teardown's check-then-remove window either. + p.unstage = func() error { + m.mu.Lock() + defer m.mu.Unlock() + return unmountAndRemove(target) + } + return target, nil +} + +// stagingMountAdvice turns a mount(2) errno into the one sentence an operator +// needs, because the two likely causes want completely different responses and +// the raw errno does not distinguish them: "ephemerd is not privileged enough" +// is a deployment problem, "your data directory is unusable" is a disk or +// configuration problem, and telling someone to check both wastes their time. +func stagingMountAdvice(err error) string { + switch { + case errors.Is(err, unix.EPERM): + return "This one is about PRIVILEGE, not the data directory: ephemerd cannot call mount(2) here, " + + "which needs CAP_SYS_ADMIN. ephemerd requires root on Linux regardless of this feature — the " + + "shipped systemd unit runs as root, the embedded containerd needs mount(2) for the overlayfs " + + "snapshotter, and networking manages iptables and a CNI bridge. Run `ephemerd doctor`, which " + + "checks this explicitly." + case errors.Is(err, unix.EROFS): + return "This one is about the DATA DIRECTORY: it is on a read-only filesystem, so the staging " + + "directory cannot be mounted into. Point --data-dir at writable storage." + case errors.Is(err, unix.ENOSPC): + return "This one is about the DATA DIRECTORY: the filesystem holding it is out of space or inodes." + case errors.Is(err, unix.EACCES): + return "This one is about the DATA DIRECTORY: a component of the staging path is not searchable by " + + "ephemerd. Check the ownership and mode of the --data-dir tree." + default: + return "Check that --data-dir points at a writable directory on a filesystem that supports bind " + + "mounts, and that ephemerd is running as root (`ephemerd doctor`)." + } +} + +// ensureDirLocked creates the per-job staging directory on first use and +// proves it is somewhere a job cannot reach. +// +// The whole point of staging is that runc's second walk of the path is safe, +// and that is only true if every component of it belongs to root and is not +// writable by anyone else. Checking it is cheap (once per job) and it is the +// assumption the entire fix rests on, so it is checked rather than assumed. +func (m *mountStager) ensureDirLocked() error { + if m.ready { + return nil + } + if err := os.MkdirAll(m.dir, 0o700); err != nil { + return fmt.Errorf("creating bind staging dir %s: %w", m.dir, err) + } + // MkdirAll honours the umask and skips existing dirs, so set the mode + // explicitly on both the job dir and its parent. + // + // The symlink check comes BEFORE the chmod, not after: os.Chmod follows + // symlinks, so chmodding first would apply 0700 to whatever a planted + // /dind-binds symlink pointed at — modifying something outside the + // staging tree on the way to refusing to use it. ensureTrustedAncestry + // below repeats the check as part of the full ancestry walk; this is the + // narrower one that has to happen first. + for _, d := range []string{stagingRootParent(m.dir), m.dir} { + info, err := os.Lstat(d) + if err != nil { + return fmt.Errorf("stat bind staging dir %s: %w", d, err) + } + if info.Mode()&os.ModeSymlink != 0 { + return fmt.Errorf("bind staging dir %s is a symlink; every component of the staging path must be a real directory", d) + } + if err := os.Chmod(d, 0o700); err != nil { + return fmt.Errorf("securing bind staging dir %s: %w", d, err) + } + } + if err := ensureTrustedAncestry(m.dir); err != nil { + return fmt.Errorf("bind staging dir %s is not safe to mount into: %w", m.dir, err) + } + + // Best-effort: make the job's staging directory its own private mount so + // the binds published inside it do not propagate into every other mount + // namespace on the node. Not a security boundary — what gets staged is + // the runner's own rootfs, which the runner can already see — so a + // failure here is logged and tolerated rather than failing the job. + if err := unix.Mount(m.dir, m.dir, "", unix.MS_BIND, ""); err != nil { + m.logf("could not self-bind the bind staging dir; staged mounts will propagate normally", "path", m.dir, "error", err) + } else if err := unix.Mount("", m.dir, "", unix.MS_PRIVATE, ""); err != nil { + m.logf("could not make the bind staging dir private; staged mounts will propagate normally", "path", m.dir, "error", err) + } + + m.ready = true + return nil +} + +// teardown removes every mount and directory this job staged. Idempotent, and +// final: nothing can be staged afterwards. +// +// The lock is held across the unmount-and-remove, not just around the flags. +// unmountTreeAndRemove checks that no mounts remain and then calls +// os.RemoveAll; a stage() landing a mount between those two steps would have +// RemoveAll delete through it, destroying the runner's rootfs contents before +// failing with EBUSY on the mountpoint. +func (m *mountStager) teardown() { + m.mu.Lock() + defer m.mu.Unlock() + + ready := m.ready + m.ready = false + m.closed = true + if !ready { + // Nothing was ever staged; the directory may not even exist. Still + // try the removal, since a create that failed halfway can leave it. + if _, err := os.Stat(m.dir); err != nil { + return + } + } + if err := unmountTreeAndRemove(m.dir); err != nil { + m.logf("failed to clean up bind staging dir", "path", m.dir, "error", err) + } +} + +func (m *mountStager) logf(msg string, args ...any) { + if m.log != nil { + m.log.Warn(msg, args...) + } +} + +// stagingRootParent returns the parent of a per-job staging dir, i.e. the +// /dind-binds root. +func stagingRootParent(jobDir string) string { return filepath.Dir(jobDir) } + +// sweepStagedBinds is the startup half of the lifecycle: it unmounts and +// removes staging directories left by a previous ephemerd process. See +// SweepStagedBinds for why leaked staging mounts are worse than untidy. +func sweepStagedBinds(root string, log *slog.Logger) { + entries, err := os.ReadDir(root) + if err != nil { + return // never created, or already gone + } + swept := 0 + for _, e := range entries { + dir := filepath.Join(root, e.Name()) + if err := unmountTreeAndRemove(dir); err != nil { + if log != nil { + log.Warn("failed to sweep leaked dind bind staging dir", "path", dir, "error", err) + } + continue + } + swept++ + } + if swept > 0 && log != nil { + log.Info("swept leaked dind bind staging dirs", "count", swept, "root", root) + } +} + +// unmountTreeAndRemove detaches every mount at or below dir, deepest first, +// and then removes the (now mount-free) directory. +// +// The removal is guarded on the unmount actually having worked. os.RemoveAll +// over a live bind mount deletes the files visible THROUGH the mount — here +// that would be the runner's own rootfs — so "could not unmount" must mean +// "leave it alone and complain", never "delete anyway". +func unmountTreeAndRemove(dir string) error { + return unmountTreeAndRemoveWith(dir, mountPointsUnder, detachMount) +} + +// unmountTreeAndRemoveWith is unmountTreeAndRemove with its two syscall-backed +// steps injectable. The seam exists because the "still mounted → do not +// remove" branch is the most consequential line in this file and there is no +// way to provoke it for real: as root, MNT_DETACH does not fail, so a test +// that tries to leave a mount behind on purpose cannot. +func unmountTreeAndRemoveWith(dir string, list func(string) ([]string, error), detach func(string)) error { + mounts, err := list(dir) + if err != nil { + return err + } + // Deepest first: a parent cannot be unmounted while a child mount sits + // inside it (and MNT_DETACH on the parent would hide, not release, the + // children). + sort.Slice(mounts, func(i, j int) bool { return len(mounts[i]) > len(mounts[j]) }) + for _, mp := range mounts { + detach(mp) + } + remaining, err := list(dir) + if err != nil { + return err + } + if len(remaining) > 0 { + return fmt.Errorf("%d bind staging mount(s) still present under %s (first: %s); not removing the directory, because deleting through a live bind mount would delete the source", len(remaining), dir, remaining[0]) + } + if err := os.RemoveAll(dir); err != nil { + return fmt.Errorf("removing bind staging dir %s: %w", dir, err) + } + return nil +} + +// unmountAndRemove releases one staged bind source. +func unmountAndRemove(target string) error { + detachMount(target) + mounts, err := mountPointsUnder(target) + if err != nil { + return err + } + if len(mounts) > 0 { + return fmt.Errorf("bind staging mount %s could not be detached; leaving the mountpoint in place rather than deleting through it", target) + } + if err := os.Remove(target); err != nil && !os.IsNotExist(err) { + return fmt.Errorf("removing bind staging mountpoint %s: %w", target, err) + } + return nil +} + +// detachMount lazily unmounts mp, repeating for stacked mounts at the same +// point. MNT_DETACH so a descriptor someone still holds cannot turn teardown +// into EBUSY; the mount goes away once the last reference does. +func detachMount(mp string) { + const maxStacked = 16 + for i := 0; i < maxStacked; i++ { + if err := unix.Unmount(mp, unix.MNT_DETACH); err != nil { + return // EINVAL once mp is no longer a mount point + } + } +} + +// mountPointsUnder returns every mount point in this process's mount namespace +// that is dir or lives beneath it. +func mountPointsUnder(dir string) ([]string, error) { + f, err := os.Open("/proc/self/mountinfo") + if err != nil { + return nil, fmt.Errorf("reading mount table: %w", err) + } + // Read-only, so a Close error carries no information a caller could act + // on — but errcheck is right that it must not be silently dropped. + defer func() { _ = f.Close() }() + + prefix := strings.TrimSuffix(dir, "/") + "/" + var out []string + sc := bufio.NewScanner(f) + sc.Buffer(make([]byte, 0, 64*1024), 1024*1024) + for sc.Scan() { + // mountinfo: id parent major:minor root mountpoint options... + fields := strings.Fields(sc.Text()) + if len(fields) < 5 { + continue + } + mp := unescapeMountPath(fields[4]) + if mp == dir || strings.HasPrefix(mp, prefix) { + out = append(out, mp) + } + } + if err := sc.Err(); err != nil { + return nil, fmt.Errorf("reading mount table: %w", err) + } + return out, nil +} + +// unescapeMountPath undoes the octal escaping the kernel applies to mountinfo +// paths (space, tab, newline and backslash). +func unescapeMountPath(s string) string { + if !strings.Contains(s, `\`) { + return s + } + var b strings.Builder + for i := 0; i < len(s); i++ { + if s[i] == '\\' && i+3 < len(s) { + if v, err := strconv.ParseUint(s[i+1:i+4], 8, 8); err == nil { + b.WriteByte(byte(v)) + i += 3 + continue + } + } + b.WriteByte(s[i]) + } + return b.String() +} + +// ensureTrustedAncestry verifies that dir and every directory above it is +// owned by root (or by us, if ephemerd is somehow not root) and is not +// writable by anyone else. A world-writable directory is accepted only when it +// carries the sticky bit, which is what makes /tmp safe: entries in a sticky +// directory can only be renamed or removed by their owner. +// +// This is the precondition that makes runc's re-walk of the staging path safe. +// If it does not hold, staging provides no protection and the bind must fail +// rather than pretend. +func ensureTrustedAncestry(dir string) error { + euid := uint32(os.Geteuid()) + for p := filepath.Clean(dir); ; p = filepath.Dir(p) { + var st unix.Stat_t + if err := unix.Lstat(p, &st); err != nil { + return fmt.Errorf("stat %s: %w", p, err) + } + if st.Mode&unix.S_IFMT == unix.S_IFLNK { + return fmt.Errorf("%s is a symlink; every component of the staging path must be a real directory", p) + } + if st.Uid != 0 && st.Uid != euid { + return fmt.Errorf("%s is owned by uid %d, which is neither root nor ephemerd's uid %d", p, st.Uid, euid) + } + const otherWrite = 0o022 + if st.Mode&otherWrite != 0 && st.Mode&unix.S_ISVTX == 0 { + return fmt.Errorf("%s is writable by group or other (mode %04o) without the sticky bit, so a job with any foothold there could swap it", p, st.Mode&0o7777) + } + if p == filepath.Dir(p) { + return nil + } + } +} diff --git a/pkg/dind/bindstage_linux_test.go b/pkg/dind/bindstage_linux_test.go new file mode 100644 index 00000000..7657b374 --- /dev/null +++ b/pkg/dind/bindstage_linux_test.go @@ -0,0 +1,728 @@ +//go:build linux + +package dind + +import ( + "context" + "io" + "log/slog" + "net" + "os" + "path/filepath" + "strings" + "testing" + + "golang.org/x/sys/unix" +) + +func discardLog() *slog.Logger { + return slog.New(slog.NewTextHandler(io.Discard, nil)) +} + +// newTestStager builds a real mountStager rooted under dir and guarantees its +// mounts are gone before the test's TempDir cleanup runs. +// +// The ordering is not incidental: t.Cleanup is LIFO, so this registration — +// made after t.TempDir() created its own cleanup — runs first. If it did not, +// os.RemoveAll would walk INTO a live bind mount and delete the files it +// exposes, which in production would be the runner's own rootfs. +func newTestStager(t *testing.T, dataDir, jobID string) *mountStager { + t.Helper() + st := newBindStager(dataDir, jobID, discardLog()).(*mountStager) + t.Cleanup(st.teardown) + return st +} + +// plantVictim builds the shape from the issue's reproduction: +// +// /a/real/marker = PINNED (what validation sees) +// /evil/real/marker = SWAPPED (what the attacker substitutes) +// +// It returns dir. The "job" later renames a/ away and drops a symlink to evil/ +// in its place — the deterministic version of the race. +func plantVictim(t *testing.T, dir string) { + t.Helper() + for _, p := range []string{"a/real", "evil/real"} { + if err := os.MkdirAll(filepath.Join(dir, p), 0o755); err != nil { + t.Fatal(err) + } + } + if err := os.WriteFile(filepath.Join(dir, "a", "real", "marker"), []byte("PINNED"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "evil", "real", "marker"), []byte("SWAPPED"), 0o644); err != nil { + t.Fatal(err) + } +} + +// swapVictim performs the attacker's move. +func swapVictim(t *testing.T, dir string) { + t.Helper() + if err := os.Rename(filepath.Join(dir, "a"), filepath.Join(dir, "a_real")); err != nil { + t.Fatal(err) + } + if err := os.Symlink(filepath.Join(dir, "evil"), filepath.Join(dir, "a")); err != nil { + t.Fatal(err) + } +} + +// TestBindStaging_SwapAfterValidationDoesNotLeak is the core #125 regression +// test at the kernel level: the staged path must keep pointing at the inode +// that was validated, even after the job swaps every name that led to it. +// +// This is the exact property runc relies on when it walks the spec's bind +// source at task start. TestBindStaging_RealRunc_SwapDoesNotLeak proves runc +// really does honour it; this one runs without a runc binary. +func TestBindStaging_SwapAfterValidationDoesNotLeak(t *testing.T) { + requireBindStaging(t) + + root := t.TempDir() + rootfs := filepath.Join(root, "rootfs") + if err := os.MkdirAll(rootfs, 0o755); err != nil { + t.Fatal(err) + } + plantVictim(t, rootfs) + + stager := newTestStager(t, root, "job-swap") + + // CHECK: resolve and pin, the way translateBindSource does. + pin, err := pinBindSource(rootfs, "/a/real", true) + if err != nil { + t.Fatalf("pinning a contained bind source must succeed: %v", err) + } + defer func() { _ = pin.Close() }() + staged, err := stager.stage(pin) + if err != nil { + t.Fatalf("staging: %v", err) + } + + // USE: the job swaps the validated directory for a symlink to its own tree. + swapVictim(t, rootfs) + + // The original path now reads the attacker's content — proving the swap + // worked and that this test would catch a regression. + orig, err := os.ReadFile(filepath.Join(rootfs, "a", "real", "marker")) + if err != nil { + t.Fatalf("reading the swapped path: %v", err) + } + if string(orig) != "SWAPPED" { + t.Fatalf("swap did not take effect: original path reads %q, want SWAPPED — the test is not exercising the attack", orig) + } + + // The staged path — what the OCI spec carries — must still be the + // validated inode. + got, err := os.ReadFile(filepath.Join(staged, "marker")) + if err != nil { + t.Fatalf("reading the staged path %s: %v", staged, err) + } + if string(got) != "PINNED" { + t.Fatalf("TOCTOU: staged bind source %s reads %q after the swap, want PINNED — the container would receive the attacker's target", staged, got) + } +} + +// TestBindStaging_UnstagedPathLeaks is the control. It asserts that the +// pre-fix arrangement — a path string in the spec, re-walked later — really +// does follow the swap. Without this, a passing SwapAfterValidation test +// proves nothing: it could be green because the swap never worked. +func TestBindStaging_UnstagedPathLeaks(t *testing.T) { + rootfs := t.TempDir() + plantVictim(t, rootfs) + + // What main returned before this fix: a path string. + source := filepath.Join(rootfs, "a", "real") + + swapVictim(t, rootfs) + + got, err := os.ReadFile(filepath.Join(source, "marker")) + if err != nil { + t.Fatalf("reading the re-walked path: %v", err) + } + if string(got) != "SWAPPED" { + t.Fatalf("re-walking the validated path returned %q, want SWAPPED — this control must reproduce the bug for the staging test above to mean anything", got) + } +} + +// TestBindStaging_LegitimateBindsStillWork is the regression the previous +// attempt shipped: it closed the escape by making every bind fail, legitimate +// ones included. This drives buildBindMounts with the real stager over the +// shapes production actually uses. +func TestBindStaging_LegitimateBindsStillWork(t *testing.T) { + requireBindStaging(t) + + root := t.TempDir() + rootfs := filepath.Join(root, "rootfs") + // A directory source, a regular-file source, and a directory that does + // not exist yet (the GHA lazy-bind case Docker auto-creates). + if err := os.MkdirAll(filepath.Join(rootfs, "home", "runner", "_work", "_temp"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(rootfs, "home", "runner", "_work", "_temp", "step.sh"), []byte("#!/bin/sh\n"), 0o755); err != nil { + t.Fatal(err) + } + // A merged-usr style absolute symlink inside the rootfs: /bin -> /usr/bin. + // This is why resolution is openat2/RESOLVE_IN_ROOT and not os.Root, which + // rejects absolute symlinks outright and would break every Ubuntu runner + // image. + if err := os.MkdirAll(filepath.Join(rootfs, "usr", "bin"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(rootfs, "usr", "bin", "tool"), []byte("tool"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink("/usr/bin", filepath.Join(rootfs, "bin")); err != nil { + t.Fatal(err) + } + + // The ephemerd-owned, non-rootfs binds: a socket and two files. These are + // the passthrough category — no pin, no staging — and they must keep + // working exactly as before. + sock := filepath.Join(root, "docker", "d.sock") + if err := os.MkdirAll(filepath.Dir(sock), 0o755); err != nil { + t.Fatal(err) + } + l, err := (&net.ListenConfig{}).Listen(context.Background(), "unix", sock) + if err != nil { + t.Fatalf("creating a real unix socket for the docker.sock bind: %v", err) + } + defer func() { _ = l.Close() }() + hosts := filepath.Join(root, "hosts") + resolv := filepath.Join(root, "resolv.conf") + for _, p := range []string{hosts, resolv} { + if err := os.WriteFile(p, []byte("# test\n"), 0o644); err != nil { + t.Fatal(err) + } + } + + s := &Server{ + log: discardLog(), + stager: newTestStager(t, root, "job-legit"), + runnerRootfsPath: rootfs, + runnerBindMappings: map[string]string{ + "/var/run/docker.sock": sock, + "/etc/hosts": hosts, + "/etc/resolv.conf": resolv, + }, + } + + binds := []string{ + "/var/run/docker.sock:/var/run/docker.sock", + "/etc/hosts:/etc/hosts", + "/etc/resolv.conf:/etc/resolv.conf", + "/home/runner/_work:/__w", + "/home/runner/_work/_temp:/__w/_temp", + "/home/runner/_work/_temp/step.sh:/step.sh", + "/home/runner/_work/_actions:/__w/_actions", // does not exist yet + "/bin/tool:/tool:ro", // through an absolute in-rootfs symlink + } + + opts, pins, err := s.buildBindMounts(context.Background(), binds) + t.Cleanup(func() { closeBindPins(pins) }) + if err != nil { + t.Fatalf("legitimate binds must still work, got: %v", err) + } + spec := applyOpts(t, opts) + if len(spec.Mounts) != len(binds) { + t.Fatalf("got %d mounts, want %d", len(spec.Mounts), len(binds)) + } + + byDest := map[string]string{} + for _, m := range spec.Mounts { + byDest[m.Destination] = m.Source + } + + // Passthrough sources keep their real paths: nothing job-controlled is in + // them, and docker.sock is a socket, which is not a valid bind-staging + // source in the first place. + for dest, want := range map[string]string{ + "/var/run/docker.sock": sock, + "/etc/hosts": hosts, + "/etc/resolv.conf": resolv, + } { + if byDest[dest] != want { + t.Errorf("%s source = %q, want the ephemerd-owned path %q", dest, byDest[dest], want) + } + } + + // Everything job-supplied is published under the staging dir, and the + // content behind it is the content that was validated. + stagingPrefix := jobStagingDir(root, "job-legit") + "/" + checks := map[string]string{ + "/__w/_temp": "step.sh", + "/step.sh": "", + "/tool": "", + } + for _, dest := range []string{"/__w", "/__w/_temp", "/__w/_actions", "/step.sh", "/tool"} { + src := byDest[dest] + if !strings.HasPrefix(src, stagingPrefix) { + t.Errorf("%s source = %q, want a path under the staging dir %q — a job-supplied source must never reach the spec as its own path", dest, src, stagingPrefix) + continue + } + if _, err := os.Stat(src); err != nil { + t.Errorf("staged source for %s is not readable: %v", dest, err) + } + if child, ok := checks[dest]; ok && child != "" { + if _, err := os.Stat(filepath.Join(src, child)); err != nil { + t.Errorf("staged %s does not expose %s: %v", dest, child, err) + } + } + } + + // The auto-created directory must exist in the runner's rootfs (that is + // the Docker-compat behaviour the GHA runner depends on), not only in the + // staging dir. + if _, err := os.Stat(filepath.Join(rootfs, "home", "runner", "_work", "_actions")); err != nil { + t.Errorf("auto-mkdir did not materialize the lazy bind source in the rootfs: %v", err) + } + + // The symlink-traversing bind must have resolved to the real file. + if body, err := os.ReadFile(byDest["/tool"]); err != nil || string(body) != "tool" { + t.Errorf("bind through the merged-usr symlink read %q (err %v), want %q", body, err, "tool") + } +} + +// TestBindStaging_RunnerBindSuffixIsStaged covers the branch that used to have +// NO containment check of any kind: a source that matches an entry in the +// runner's bind table with a leftover suffix. +// +// That branch matters as much as the rootfs one. The per-job runner directory +// in the bind table is bind-mounted INTO the runner and is therefore fully +// job-writable, so `-v /evil/x:/y` with `evil` a planted symlink +// was a straight escape. The other staging tests all go through the rootfs +// branch, which left this one asserted only by the translation-policy tests +// that do not stage at all. +func TestBindStaging_RunnerBindSuffixIsStaged(t *testing.T) { + requireBindStaging(t) + + root := t.TempDir() + // The per-job runner directory, as it appears in runnerBindMappings. + runnerDir := filepath.Join(root, "runners", "job-x") + if err := os.MkdirAll(runnerDir, 0o755); err != nil { + t.Fatal(err) + } + plantVictim(t, runnerDir) + outside := t.TempDir() + if err := os.WriteFile(filepath.Join(outside, "secret"), []byte("host-fs"), 0o600); err != nil { + t.Fatal(err) + } + + s := &Server{ + log: discardLog(), + stager: newTestStager(t, root, "job-suffix"), + runnerBindMappings: map[string]string{ + "/home/runner/runner": runnerDir, + }, + } + + // A symlink out of the runner directory must be refused, not followed. + if err := os.Symlink(outside, filepath.Join(runnerDir, "esc")); err != nil { + t.Fatal(err) + } + if _, _, err := s.buildBindMounts(context.Background(), []string{"/home/runner/runner/esc/secret:/x"}); err == nil { + t.Error("a suffix escaping the runner directory through a symlink must be rejected") + } + + // The legitimate case is staged, not string-joined. + opts, pins, err := s.buildBindMounts(context.Background(), []string{"/home/runner/runner/a/real:/x"}) + t.Cleanup(func() { closeBindPins(pins) }) + if err != nil { + t.Fatalf("a contained suffix under the runner directory must work: %v", err) + } + spec := applyOpts(t, opts) + staged := spec.Mounts[0].Source + if !strings.HasPrefix(staged, jobStagingDir(root, "job-suffix")+string(filepath.Separator)) { + t.Fatalf("runner-bind suffix source = %q, want a staging path — this branch is as job-controlled as the rootfs branch", staged) + } + + // And it holds the validated inode across the swap, same as the rootfs + // branch does. + swapVictim(t, runnerDir) + if orig, _ := os.ReadFile(filepath.Join(runnerDir, "a", "real", "marker")); string(orig) != "SWAPPED" { + t.Fatalf("swap did not take effect (original reads %q); the test is not exercising the attack", orig) + } + got, err := os.ReadFile(filepath.Join(staged, "marker")) + if err != nil { + t.Fatalf("reading staged runner-bind source: %v", err) + } + if string(got) != "PINNED" { + t.Fatalf("TOCTOU on the runner-bind suffix branch: staged source reads %q, want PINNED", got) + } +} + +// TestBindStaging_StageAfterTeardownIsRefused is the regression guard for the +// Stop() teardown race. +// +// Server.Stop now shuts the HTTP listener down before tearing the stager down, +// so an in-flight create cannot normally reach stage() at that point. This +// asserts the stager's own half of the invariant, which is what makes the +// ordering safe rather than merely lucky: once torn down, staging must FAIL. +// If it instead recreated the directory (which it did before, because +// ensureDirLocked keys off `ready` and teardown cleared it), the mount would +// be published into a directory the caller had already swept — leaking a mount +// that pins the runner's rootfs until the next daemon startup, and racing +// teardown's own os.RemoveAll, which deletes THROUGH a live bind mount. +func TestBindStaging_StageAfterTeardownIsRefused(t *testing.T) { + requireBindStaging(t) + + root := t.TempDir() + rootfs := filepath.Join(root, "rootfs") + if err := os.MkdirAll(filepath.Join(rootfs, "d"), 0o755); err != nil { + t.Fatal(err) + } + stager := newTestStager(t, root, "job-closed") + + pin, err := pinBindSource(rootfs, "/d", false) + if err != nil { + t.Fatal(err) + } + defer func() { _ = pin.Close() }() + if _, err := stager.stage(pin); err != nil { + t.Fatalf("staging before teardown: %v", err) + } + + stager.teardown() + + late, err := pinBindSource(rootfs, "/d", false) + if err != nil { + t.Fatal(err) + } + defer func() { _ = late.Close() }() + if _, err := stager.stage(late); err == nil { + t.Fatal("staging after teardown must fail; silently recreating the staging dir leaks a mount that pins the runner rootfs") + } + if _, err := os.Stat(jobStagingDir(root, "job-closed")); !os.IsNotExist(err) { + t.Errorf("the refused stage recreated the staging dir (stat err = %v)", err) + } +} + +// TestUnmountTreeAndRemove_RefusesWhileMounted covers the single most +// consequential line in this change: the guard that refuses to os.RemoveAll a +// directory that still has a mount under it. +// +// The first subtest measures WHY the guard has to exist rather than asserting +// it, because the failure mode is counter-intuitive: os.RemoveAll does not +// stop at the mountpoint and report EBUSY, it walks in, deletes the files +// visible THROUGH the mount, and reports EBUSY afterwards. In production those +// files are the runner's rootfs. +// +// The second subtest drives the guard itself. It has to inject the mount +// enumeration: as root, MNT_DETACH does not fail, so there is no honest way to +// leave a real mount behind. (Stacking more mounts at one point than +// detachMount unwinds does not work either — mountinfo lists one entry per +// stacked mount, so the caller invokes detachMount once per entry and the +// whole stack comes down.) +func TestUnmountTreeAndRemove_RefusesWhileMounted(t *testing.T) { + requireBindStaging(t) + + t.Run("removeall_deletes_through_a_live_mount", func(t *testing.T) { + base := t.TempDir() + source := filepath.Join(base, "source") + if err := os.MkdirAll(source, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(source, "precious"), []byte("runner rootfs content"), 0o644); err != nil { + t.Fatal(err) + } + staging := filepath.Join(base, "staging") + target := filepath.Join(staging, "0") + if err := os.MkdirAll(target, 0o700); err != nil { + t.Fatal(err) + } + if err := unix.Mount(source, target, "", unix.MS_BIND, ""); err != nil { + t.Fatalf("bind: %v", err) + } + t.Cleanup(func() { _ = unix.Unmount(target, unix.MNT_DETACH) }) + + err := os.RemoveAll(staging) + if err == nil { + t.Skip("os.RemoveAll unexpectedly succeeded over a live bind mount; the hazard this guard exists for does not reproduce here") + } + if _, statErr := os.Stat(filepath.Join(source, "precious")); statErr == nil { + t.Fatalf("os.RemoveAll left the bind source intact (err was %v); if this ever becomes true the guard could be relaxed, but do not relax it on a hunch", err) + } + t.Logf("confirmed: os.RemoveAll reported %v AFTER emptying the bind source — this is what the guard prevents", err) + }) + + t.Run("guard_refuses_and_preserves_the_source", func(t *testing.T) { + base := t.TempDir() + source := filepath.Join(base, "source") + if err := os.MkdirAll(source, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(source, "precious"), []byte("runner rootfs content"), 0o644); err != nil { + t.Fatal(err) + } + staging := filepath.Join(base, "staging") + target := filepath.Join(staging, "0") + if err := os.MkdirAll(target, 0o700); err != nil { + t.Fatal(err) + } + if err := unix.Mount(source, target, "", unix.MS_BIND, ""); err != nil { + t.Fatalf("bind: %v", err) + } + t.Cleanup(func() { _ = unix.Unmount(target, unix.MNT_DETACH) }) + + // A detach that does nothing: the mount survives the unmount pass, + // which is exactly the state the guard is there for. + stubborn := func(string) {} + err := unmountTreeAndRemoveWith(staging, mountPointsUnder, stubborn) + if err == nil { + t.Fatal("unmountTreeAndRemove must refuse while a mount remains, not delete through it") + } + if !strings.Contains(err.Error(), "still present") { + t.Errorf("error %q should explain that mounts remain", err) + } + if _, statErr := os.Stat(filepath.Join(source, "precious")); statErr != nil { + t.Fatalf("the refusal did not protect the bind source — it was deleted: %v", statErr) + } + if _, statErr := os.Stat(staging); statErr != nil { + t.Errorf("staging dir was removed despite the refusal: %v", statErr) + } + }) +} + +// TestResolveBeneathWalk_MatchesOpenat2 pins the claim the fallback's doc +// comment makes: that it is equivalent to openat2 with RESOLVE_IN_ROOT, not +// merely "stricter, therefore safe". +// +// The case that broke it was ".." arriving from a SYMLINK TARGET — the walk +// refused every "..", while RESOLVE_IN_ROOT resolves them clamped at the root. +// Relative "../" targets are ordinary in real images (/etc/alternatives/*, +// Debian multiarch), so the two resolvers disagreeing meant any node that fell +// back would start 400ing legitimate binds. +func TestResolveBeneathWalk_MatchesOpenat2(t *testing.T) { + rootfs := t.TempDir() + for _, d := range []string{"usr/bin", "opt", "deep/a/b/c"} { + if err := os.MkdirAll(filepath.Join(rootfs, d), 0o755); err != nil { + t.Fatal(err) + } + } + if err := os.WriteFile(filepath.Join(rootfs, "usr", "bin", "tool"), []byte("TOOL"), 0o755); err != nil { + t.Fatal(err) + } + // The shapes real images contain. + mkSymlinkOrSkip(t, "../usr/bin/tool", filepath.Join(rootfs, "opt", "rel")) // relative, with .. + mkSymlinkOrSkip(t, "/usr/bin/tool", filepath.Join(rootfs, "opt", "abs")) // container-absolute + mkSymlinkOrSkip(t, "../../../../usr/bin/tool", filepath.Join(rootfs, "deep", "a", "b", "climb")) + // An escape attempt: ".." past the root must clamp, not escape. + mkSymlinkOrSkip(t, "../../../../../../etc/passwd", filepath.Join(rootfs, "opt", "esc")) + + for _, rel := range []string{ + "usr/bin/tool", + "opt/rel", + "opt/abs", + "deep/a/b/climb", + "opt/esc", + "deep/a/../a/b/c", + } { + t.Run(rel, func(t *testing.T) { + rootFd, err := openPathDir(rootfs) + if err != nil { + t.Fatal(err) + } + defer closePinFd(rootFd) + comps, err := pathComponents(rel) + if err != nil { + t.Fatal(err) + } + + viaOpenat2, err2 := resolveBeneath(rootFd, comps) + viaWalk, errW := resolveBeneathWalk(rootFd, comps) + if (err2 == nil) != (errW == nil) { + t.Fatalf("resolvers disagree on %q: openat2 err=%v, fallback walk err=%v", rel, err2, errW) + } + if err2 != nil { + return // both refused; that is agreement + } + defer closePinFd(viaOpenat2) + defer closePinFd(viaWalk) + + var a, b unix.Stat_t + if err := unix.Fstat(viaOpenat2, &a); err != nil { + t.Fatal(err) + } + if err := unix.Fstat(viaWalk, &b); err != nil { + t.Fatal(err) + } + if a.Dev != b.Dev || a.Ino != b.Ino { + t.Fatalf("resolvers landed on different inodes for %q: openat2 %d:%d, walk %d:%d", rel, a.Dev, a.Ino, b.Dev, b.Ino) + } + }) + } +} + +// TestBindStaging_TeardownUnmountsAndRemoves covers the lifecycle: after the +// job's stager tears down, nothing is left mounted and the directory is gone. +// A leaked staging mount holds a reference to the runner rootfs it was bound +// from, which is what stops containerd from deleting the snapshot. +func TestBindStaging_TeardownUnmountsAndRemoves(t *testing.T) { + requireBindStaging(t) + + root := t.TempDir() + rootfs := filepath.Join(root, "rootfs") + if err := os.MkdirAll(filepath.Join(rootfs, "d"), 0o755); err != nil { + t.Fatal(err) + } + stager := newTestStager(t, root, "job-teardown") + + pin, err := pinBindSource(rootfs, "/d", false) + if err != nil { + t.Fatal(err) + } + staged, err := stager.stage(pin) + if err != nil { + t.Fatalf("staging: %v", err) + } + mounts, err := mountPointsUnder(staged) + if err != nil { + t.Fatal(err) + } + if len(mounts) == 0 { + t.Fatalf("no mount at %s after staging — staging did not actually bind anything", staged) + } + + stager.teardown() + + if mounts, err := mountPointsUnder(jobStagingDir(root, "job-teardown")); err != nil { + t.Fatal(err) + } else if len(mounts) != 0 { + t.Errorf("mounts still present after teardown: %v", mounts) + } + if _, err := os.Stat(jobStagingDir(root, "job-teardown")); !os.IsNotExist(err) { + t.Errorf("staging dir still present after teardown (stat err = %v)", err) + } + // Closing the pin afterwards must not blow up or double-unmount. + if err := pin.Close(); err != nil { + t.Errorf("closing a pin whose stager already tore down: %v", err) + } +} + +// TestSweepStagedBinds_RemovesLeakedMounts is the startup half of the +// lifecycle. A SIGKILL leaves the mounts behind with no process to release +// them; the next ephemerd must clear them before it tries to reclaim +// snapshots, or the reclaim silently fails on EBUSY. +func TestSweepStagedBinds_RemovesLeakedMounts(t *testing.T) { + requireBindStaging(t) + + root := t.TempDir() + rootfs := filepath.Join(root, "rootfs") + if err := os.MkdirAll(filepath.Join(rootfs, "d"), 0o755); err != nil { + t.Fatal(err) + } + // Simulate the previous process: stage, then walk away without teardown. + leaked := newBindStager(root, "job-killed", discardLog()).(*mountStager) + t.Cleanup(leaked.teardown) // belt and braces if the sweep under test fails + pin, err := pinBindSource(rootfs, "/d", false) + if err != nil { + t.Fatal(err) + } + staged, err := leaked.stage(pin) + if err != nil { + t.Fatalf("staging: %v", err) + } + // pin is deliberately NOT closed: the previous process died holding it. + + SweepStagedBinds(root, discardLog()) + + if mounts, err := mountPointsUnder(stagingRootDir(root)); err != nil { + t.Fatal(err) + } else if len(mounts) != 0 { + t.Errorf("sweep left mounts behind: %v", mounts) + } + if _, err := os.Stat(staged); !os.IsNotExist(err) { + t.Errorf("sweep left %s behind (stat err = %v)", staged, err) + } +} + +// TestEnsureTrustedAncestry_RejectsJobWritableParent guards the assumption the +// whole design rests on: runc re-walking the staging path is only safe because +// no component of it can be swapped by anyone but root. +func TestEnsureTrustedAncestry_RejectsJobWritableParent(t *testing.T) { + requireBindStaging(t) + + base := t.TempDir() + loose := filepath.Join(base, "loose") + if err := os.MkdirAll(filepath.Join(loose, "child"), 0o755); err != nil { + t.Fatal(err) + } + // World-writable with no sticky bit: anyone can rename "child" away. + if err := os.Chmod(loose, 0o777); err != nil { + t.Fatal(err) + } + if err := ensureTrustedAncestry(filepath.Join(loose, "child")); err == nil { + t.Fatal("a world-writable, non-sticky ancestor must be rejected") + } + + // Sticky (the /tmp arrangement) is fine: only the owner can rename or + // remove entries, so a component cannot be swapped out from under us. + if err := os.Chmod(loose, 0o777|os.ModeSticky); err != nil { + t.Fatal(err) + } + if err := ensureTrustedAncestry(filepath.Join(loose, "child")); err != nil { + t.Errorf("a sticky world-writable ancestor is safe and must be accepted, got: %v", err) + } + + // And the normal case. + if err := os.Chmod(loose, 0o700); err != nil { + t.Fatal(err) + } + if err := ensureTrustedAncestry(filepath.Join(loose, "child")); err != nil { + t.Errorf("a root-owned 0700 ancestry must be accepted, got: %v", err) + } +} + +// TestUnescapeMountPath covers the octal escaping the kernel applies to +// mountinfo paths. A staging dir under a data directory with a space in it +// would otherwise be invisible to the sweep, and an invisible mount is one +// that never gets unmounted. +func TestUnescapeMountPath(t *testing.T) { + cases := map[string]string{ + `/var/lib/ephemerd`: "/var/lib/ephemerd", + `/mnt/my\040data/dind-binds`: "/mnt/my data/dind-binds", + `/a\011b`: "/a\tb", + `/back\134slash`: "/back\\slash", + `/not\09escaped`: `/not\09escaped`, + } + for in, want := range cases { + if got := unescapeMountPath(in); got != want { + t.Errorf("unescapeMountPath(%q) = %q, want %q", in, got, want) + } + } +} + +// TestBindStaging_MountpointTypeMatchesSource makes sure a file source gets a +// file mountpoint. Binding a file onto a directory is ENOTDIR, which would +// break /etc/hosts-shaped binds — a quiet way to regress the feature while all +// the directory tests stay green. +func TestBindStaging_MountpointTypeMatchesSource(t *testing.T) { + requireBindStaging(t) + + root := t.TempDir() + rootfs := filepath.Join(root, "rootfs") + if err := os.MkdirAll(rootfs, 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(rootfs, "file"), []byte("body"), 0o644); err != nil { + t.Fatal(err) + } + stager := newTestStager(t, root, "job-filetype") + + pin, err := pinBindSource(rootfs, "/file", false) + if err != nil { + t.Fatal(err) + } + defer func() { _ = pin.Close() }() + staged, err := stager.stage(pin) + if err != nil { + t.Fatalf("staging a regular file: %v", err) + } + var st unix.Stat_t + if err := unix.Stat(staged, &st); err != nil { + t.Fatal(err) + } + if st.Mode&unix.S_IFMT != unix.S_IFREG { + t.Errorf("staged mountpoint for a file is mode %o, want a regular file", st.Mode) + } + if body, err := os.ReadFile(staged); err != nil || string(body) != "body" { + t.Errorf("staged file reads %q (err %v), want %q", body, err, "body") + } +} diff --git a/pkg/dind/bindstage_other.go b/pkg/dind/bindstage_other.go new file mode 100644 index 00000000..7895c670 --- /dev/null +++ b/pkg/dind/bindstage_other.go @@ -0,0 +1,37 @@ +//go:build !linux + +package dind + +import ( + "log/slog" + "os" +) + +// newBindStager returns the off-Linux stager. There is no mount(2) here and, +// more to the point, no dind daemon: sibling containers are created by the +// linux build of ephemerd — directly on a Linux node, or inside the managed +// Linux VM on a Windows/macOS node — and bind translation is not wired into +// the Windows or macOS container paths at all (see pkg/runtime/runtime.go and +// pkg/dind/containers.go, where the Windows sibling path builds its mounts +// separately). +// +// So this stager hands back the resolved path unchanged. That is the +// pre-#125 behaviour, and it is safe here only because nothing on these +// platforms feeds a job-controlled source into it. +func newBindStager(dataDir, jobID string, log *slog.Logger) bindStager { + return passthroughStager{} +} + +type passthroughStager struct{} + +func (passthroughStager) stage(p *bindPin) (string, error) { + return p.Logical(), nil +} + +func (passthroughStager) teardown() {} + +// sweepStagedBinds has nothing to unmount off Linux; remove the directory if a +// Linux-side data dir ever gets inspected from a dev host. +func sweepStagedBinds(root string, log *slog.Logger) { + _ = os.RemoveAll(root) +} diff --git a/pkg/dind/bindstage_probe_test.go b/pkg/dind/bindstage_probe_test.go new file mode 100644 index 00000000..2878265b --- /dev/null +++ b/pkg/dind/bindstage_probe_test.go @@ -0,0 +1,55 @@ +//go:build !darwin + +package dind + +import ( + "os" + "testing" +) + +// requireBindStaging skips the calling test unless this process can actually +// perform the bind mount that bind staging depends on. +// +// WHY A PROBE AND NOT os.Geteuid() == 0. Capabilities, user namespaces, LSM +// policy and read-only mount propagation all decide this independently of the +// uid, and ephemerd's own CI runners are containers where the uid and the +// capability disagree — the runner is an unprivileged uid with the default OCI +// capability set, so it has no CAP_SYS_ADMIN even where it looks privileged. +// This reuses probeMountPrivilege for exactly that reason; see the long note +// on it in pull_e2e_test.go. +// +// WHY THE PRODUCTION PATH STILL HARD-FAILS. Skipping here is a statement about +// the test environment, not a licence for the daemon to degrade. Root with +// CAP_SYS_ADMIN is a hard, universal prerequisite for ephemerd on Linux: the +// shipped systemd unit sets no User= and no capability bounding set (see +// cmd/ephemerd/install_linux.go), `ephemerd doctor` FAILS rather than warns +// when euid != 0 (cmd/ephemerd/doctor_linux.go: "ephemerd requires root for +// container management"), the in-process containerd already needs mount(2) +// for the overlayfs snapshotter, and networking shells out to iptables and +// manages a CNI bridge. There is no rootless mode anywhere in the tree or the +// docs. So a daemon that cannot mount(2) is already broken for reasons that +// have nothing to do with bind staging, and staging must never quietly fall +// back to putting a job-controlled path in the OCI spec — that is issue #125 +// reopened, green tests and all. +// +// Setting EPHEMERD_TEST_REQUIRE_MOUNT=1 turns the skip into a failure, for +// anywhere this coverage is meant to be guaranteed (a Linux node, or a root +// WSL session). +func requireBindStaging(t *testing.T) { + t.Helper() + + err := probeMountPrivilege(t) + if err == nil { + return + } + if os.Getenv(requireMountEnv) != "" { + t.Fatalf("%s is set, so bind staging coverage must run, but this "+ + "environment cannot mount(2): %v", requireMountEnv, err) + } + t.Skipf("bind staging needs mount(2) (CAP_SYS_ADMIN); this environment "+ + "cannot: %v\n"+ + " to run it: sudo go test -tags containers_image_openpgp "+ + "-run 'TestBindStaging|TestBindTranslation' -v ./pkg/dind/\n"+ + " to forbid the skip (fail instead): set %s=1", + err, requireMountEnv) +} diff --git a/pkg/dind/bindstage_runc_linux_test.go b/pkg/dind/bindstage_runc_linux_test.go new file mode 100644 index 00000000..8d34461b --- /dev/null +++ b/pkg/dind/bindstage_runc_linux_test.go @@ -0,0 +1,254 @@ +//go:build linux + +package dind + +import ( + "encoding/json" + "os" + "os/exec" + "path/filepath" + "strconv" + "strings" + "testing" +) + +// End-to-end proof for issue #125 against a REAL runc. +// +// Everything below the OCI spec is out of ephemerd's hands: what actually +// decides whether the escape is closed is what runc's mount(2) resolves, in +// runc's process, in runc's mount namespace, some time after the spec was +// written. Unit tests on the daemon side cannot see that, and the previous +// attempt at this fix shipped precisely because they could not: it produced a +// spec runc rejects outright (EINVAL on every bind, legitimate ones included) +// and the Go tests were green. +// +// So this test writes a bundle, drives the real pinBindSource + mountStager, +// performs the adversarial swap, runs runc, and asserts on what the container +// printed. +// +// PRIVILEGE / OPT-IN. Needs root (mount(2), and runc itself) and a runc +// binary, so it is gated on EPHEMERD_TEST_RUNC. ephemerd ships one; on a node +// it is at /bin/runc, and in a checkout it is +// pkg/containerd/embed/runc. Run it with: +// +// sudo EPHEMERD_TEST_RUNC=$PWD/pkg/containerd/embed/runc \ +// go test ./pkg/dind/ -run TestBindStaging_RealRunc -v +// +// The attacker's target holds a benign "SWAPPED" marker, never a symlink to +// "/". The mechanism is identical and it cannot mount a host root into a +// container by accident. + +func requireRunc(t *testing.T) string { + t.Helper() + bin := os.Getenv("EPHEMERD_TEST_RUNC") + if bin == "" { + t.Skip("set EPHEMERD_TEST_RUNC= to run the end-to-end bind escape test") + } + if _, err := os.Stat(bin); err != nil { + t.Fatalf("EPHEMERD_TEST_RUNC=%s: %v", bin, err) + } + return bin +} + +// buildContainerInit compiles a tiny static binary that prints what it can +// read at /src/marker. It is the container's whole userspace: building it +// beats depending on a base image being present on whatever machine this runs +// on. +func buildContainerInit(t *testing.T, dir string) string { + t.Helper() + src := filepath.Join(dir, "init.go") + const prog = `package main + +import ( + "fmt" + "os" +) + +func main() { + b, err := os.ReadFile("/src/marker") + if err != nil { + fmt.Printf("CONTAINER-SAW-ERR: %v\n", err) + os.Exit(1) + } + fmt.Printf("CONTAINER-SAW: %s\n", string(b)) +} +` + if err := os.WriteFile(src, []byte(prog), 0o644); err != nil { + t.Fatal(err) + } + out := filepath.Join(dir, "init") + cmd := exec.Command("go", "build", "-o", out, src) + cmd.Env = append(os.Environ(), "CGO_ENABLED=0", "GO111MODULE=off") + if b, err := cmd.CombinedOutput(); err != nil { + t.Skipf("cannot build the container init binary here: %v\n%s", err, b) + } + return out +} + +// writeBundle assembles an OCI bundle whose only interesting mount is +// source -> /src, with the same options ephemerd's withBindMount emits. +func writeBundle(t *testing.T, runcBin, bundle, initBin, source string) { + t.Helper() + rootfs := filepath.Join(bundle, "rootfs") + for _, d := range []string{"proc", "src"} { + if err := os.MkdirAll(filepath.Join(rootfs, d), 0o755); err != nil { + t.Fatal(err) + } + } + body, err := os.ReadFile(initBin) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(rootfs, "init"), body, 0o755); err != nil { + t.Fatal(err) + } + + spec := exec.Command(runcBin, "spec") + spec.Dir = bundle + if b, err := spec.CombinedOutput(); err != nil { + t.Fatalf("runc spec: %v\n%s", err, b) + } + raw, err := os.ReadFile(filepath.Join(bundle, "config.json")) + if err != nil { + t.Fatal(err) + } + var cfg map[string]any + if err := json.Unmarshal(raw, &cfg); err != nil { + t.Fatal(err) + } + proc := cfg["process"].(map[string]any) + proc["args"] = []any{"/init"} + proc["terminal"] = false + cfg["root"] = map[string]any{"path": "rootfs", "readonly": false} + cfg["mounts"] = append(cfg["mounts"].([]any), map[string]any{ + "destination": "/src", + "type": "bind", + "source": source, + "options": []any{"rbind", "rw"}, + }) + patched, err := json.MarshalIndent(cfg, "", " ") + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(bundle, "config.json"), patched, 0o644); err != nil { + t.Fatal(err) + } +} + +func runBundle(t *testing.T, runcBin, bundle, id string) string { + t.Helper() + stateRoot := filepath.Join(t.TempDir(), "runc-state") + cmd := exec.Command(runcBin, "--root", stateRoot, "run", id) + cmd.Dir = bundle + out, _ := cmd.CombinedOutput() + t.Cleanup(func() { + _ = exec.Command(runcBin, "--root", stateRoot, "delete", "-f", id).Run() + }) + return strings.TrimSpace(string(out)) +} + +// TestBindStaging_RealRunc_SwapDoesNotLeak runs both halves in one test so a +// green result is meaningful: the control proves the swap really does redirect +// a path-string source (i.e. the bug is reproduced here), and the staged case +// proves it no longer does. +func TestBindStaging_RealRunc_SwapDoesNotLeak(t *testing.T) { + runcBin := requireRunc(t) + requireBindStaging(t) + + work := t.TempDir() + initBin := buildContainerInit(t, work) + + t.Run("control_path_source_leaks", func(t *testing.T) { + rootfs := filepath.Join(work, "victim-control") + if err := os.MkdirAll(rootfs, 0o755); err != nil { + t.Fatal(err) + } + plantVictim(t, rootfs) + + bundle := filepath.Join(work, "bundle-control") + // The pre-fix spec source: the validated path, as a string. + writeBundle(t, runcBin, bundle, initBin, filepath.Join(rootfs, "a", "real")) + swapVictim(t, rootfs) + + out := runBundle(t, runcBin, bundle, "eph-toctou-control") + if !strings.Contains(out, "CONTAINER-SAW: SWAPPED") { + t.Fatalf("control did not reproduce the escape, so the staged case below proves nothing.\nrunc output: %s", out) + } + t.Logf("control (pre-fix shape): %s", out) + }) + + t.Run("staged_source_does_not_leak", func(t *testing.T) { + root := filepath.Join(work, "data") + rootfs := filepath.Join(root, "rootfs") + if err := os.MkdirAll(rootfs, 0o755); err != nil { + t.Fatal(err) + } + plantVictim(t, rootfs) + + stager := newTestStager(t, root, "job-runc") + pin, err := pinBindSource(rootfs, "/a/real", true) + if err != nil { + t.Fatalf("pinning a contained source: %v", err) + } + defer func() { _ = pin.Close() }() + staged, err := stager.stage(pin) + if err != nil { + t.Fatalf("staging: %v", err) + } + + bundle := filepath.Join(work, "bundle-staged") + writeBundle(t, runcBin, bundle, initBin, staged) + swapVictim(t, rootfs) + + out := runBundle(t, runcBin, bundle, "eph-toctou-staged") + if strings.Contains(out, "CONTAINER-SAW: SWAPPED") { + t.Fatalf("ESCAPE: the container received the attacker's target through the staged source.\nrunc output: %s", out) + } + if !strings.Contains(out, "CONTAINER-SAW: PINNED") { + t.Fatalf("the container did not see the validated content — the bind failed instead of holding, which is the regression the previous fix shipped.\nrunc output: %s", out) + } + t.Logf("staged (fixed shape): %s", out) + }) +} + +// TestBindStaging_RealRunc_ProcFdSourceIsRejected records why the spec cannot +// simply carry /proc//fd/, which is the shape the earlier +// fix/linux-isolation-hardening branch shipped. +// +// runc does not re-resolve the string — its own error echoes it verbatim — but +// mount(2) refuses 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, which makes the fd handoff a functional regression +// rather than a fix. Keeping the finding as an executable test stops the next +// person from re-deriving it from a fleet outage. +func TestBindStaging_RealRunc_ProcFdSourceIsRejected(t *testing.T) { + runcBin := requireRunc(t) + requireBindStaging(t) + + work := t.TempDir() + initBin := buildContainerInit(t, work) + + rootfs := filepath.Join(work, "victim") + if err := os.MkdirAll(rootfs, 0o755); err != nil { + t.Fatal(err) + } + plantVictim(t, rootfs) + + pin, err := pinBindSource(rootfs, "/a/real", false) + if err != nil { + t.Fatal(err) + } + defer func() { _ = pin.Close() }() + bundle := filepath.Join(work, "bundle-procfd") + procSource := "/proc/" + strconv.Itoa(os.Getpid()) + "/fd/" + strconv.Itoa(pin.fd) + writeBundle(t, runcBin, bundle, initBin, procSource) + + out := runBundle(t, runcBin, bundle, "eph-toctou-procfd") + if strings.Contains(out, "CONTAINER-SAW:") { + t.Fatalf("a /proc//fd source unexpectedly mounted — if the kernel now allows this, the staging layer could be simplified.\nrunc output: %s", out) + } + if !strings.Contains(out, "invalid argument") { + t.Logf("note: the fd source failed for a reason other than EINVAL; output: %s", out) + } + t.Logf("proc-fd source (rejected as expected): %s", out) +} diff --git a/pkg/dind/bindstage_testutil_test.go b/pkg/dind/bindstage_testutil_test.go new file mode 100644 index 00000000..cfeec35a --- /dev/null +++ b/pkg/dind/bindstage_testutil_test.go @@ -0,0 +1,31 @@ +package dind + +// unstagedTestStager returns the resolved path unchanged instead of publishing +// it as a bind mount. +// +// IT DELIBERATELY DOES NOT EXIST IN A NON-TEST BUILD. Staging needs mount(2), +// which needs CAP_SYS_ADMIN, and the project's Linux CI runner is unprivileged +// (see the "Privileged-test coverage report" step in .github/workflows/ci.yml, +// which exists for the same reason). Tests that only care about translation +// POLICY — which sources are accepted, which are rejected, what gets +// auto-created, which are forced read-only — install this so they run +// everywhere, on every platform, without privilege. +// +// The security property staging provides is therefore NOT covered by these +// tests, by construction. It is covered by: +// +// - TestBindStaging_* and TestUnmountTreeAndRemove_* in +// bindstage_linux_test.go (Linux, root-gated), which perform the real bind +// and prove the staged content survives an adversarial swap of the +// original path; +// - TestBindStaging_RealRunc_* in bindstage_runc_linux_test.go (Linux, root, +// EPHEMERD_TEST_RUNC=), which drives the whole path through +// a real runc and asserts what the container actually saw. +// +// Keeping the unsafe shortcut in a _test.go file is the point: no production +// wiring can select it, and dind.New never sees it. +type unstagedTestStager struct{} + +func (unstagedTestStager) stage(p *bindPin) (string, error) { return p.Logical(), nil } + +func (unstagedTestStager) teardown() {} diff --git a/pkg/dind/bindtranslate.go b/pkg/dind/bindtranslate.go index 18c38226..deff415b 100644 --- a/pkg/dind/bindtranslate.go +++ b/pkg/dind/bindtranslate.go @@ -3,9 +3,8 @@ package dind import ( "errors" "fmt" - "os" + "io/fs" "path" - "path/filepath" "sort" "strings" ) @@ -14,14 +13,21 @@ import ( // source from the runner container's mount namespace to a real path on the // dind daemon's filesystem. type bindResolution struct { - // HostPath is the path the dind daemon will hand to containerd as the - // OCI bind source. It is always on the dind daemon's filesystem. - HostPath string + // ResolvedPath is the path the source resolved to, before symlink + // resolution. It is the OCI bind source ONLY for sources that carry no + // job-controlled component (Pin == nil). For everything else it is + // diagnostics: mounting it is the second path walk issue #125 is about. + ResolvedPath string // ForceReadOnly is set when the source resolved to a shared image // layer (lowerdir). Writes through that mount would corrupt the // cached image for every other job using the same base, so the bind // is downgraded to ro regardless of what the client requested. ForceReadOnly bool + // Pin holds the resolved inode open. Non-nil whenever any part of the + // source came from the job. The caller must stage it (bindStager) to + // obtain the path for the OCI spec, and must Close it when the + // container goes away. On error nothing is left open. + Pin *bindPin } // translateBindSource maps a bind source path the sibling container received @@ -35,16 +41,15 @@ type bindResolution struct { // runnerRootfsPath is the host-namespace path where the runner container's // merged overlay is mounted by runc (typically // "/run/containerd/io.containerd.runtime.v2.task///rootfs"). When -// non-empty, rootfs sources resolve via "/" — a -// regular path in the host's mount namespace that points at the same -// merged view the runner sees from inside. +// non-empty, rootfs sources resolve beneath it — a regular path in the host's +// mount namespace that points at the same merged view the runner sees from +// inside. // -// The previous draft of this fix tried "/proc//root/" as the -// bind source. That path readlinks correctly, but the kernel refuses it -// at mount(2) because resolving it crosses into the runner's mount -// namespace — bind sources have to be paths in the *calling* process's -// mount namespace. The bundle's rootfs mount is in the host namespace -// so the kernel walks it normally. +// An earlier draft used "/proc//root/" as the bind source. That path +// readlinks correctly, but the kernel refuses it at mount(2) because resolving +// it crosses into the runner's mount namespace — bind sources have to be paths +// in the *calling* process's mount namespace. The bundle's rootfs mount is in +// the host namespace so the kernel walks it normally. // // upperdir / lowerdirs are the explicit layer paths for the test path — // real production calls always pass runnerRootfsPath != "". @@ -59,71 +64,82 @@ type bindResolution struct { // 3. Upperdir match (fallback for tests with no rootfs path). // 4. Lowerdir match (fallback for tests; forced ro). // 5. No match → error. Loud failure replaces the pre-fix silent drop. +// +// SECURITY (issue #125): every branch whose path contains anything the job +// chose resolves through pinBindSource, which contains the resolution inside +// that branch's root and returns a HELD DESCRIPTOR rather than a string. +// +// - Containment matters because a job owns its own rootfs and can plant a +// symlink to "/" anywhere in it; the per-job runner directory (which +// appears in runnerBinds) is likewise bind-mounted into the runner and +// therefore job-writable, so the bind-table branch is just as +// attacker-controlled as the rootfs branch — and it previously had no +// containment check at all. +// - The descriptor matters because a containment check on a path that is +// then handed onward as a string is a check on an object that no longer +// has to be the object that gets mounted. +// +// On success the caller owns bindResolution.Pin: it must stage it and close it. func translateBindSource(src string, runnerBinds map[string]string, runnerRootfsPath string, upperdir string, lowerdirs []string) (bindResolution, error) { // Sources are POSIX paths from the runner's Linux mount namespace; // use path (not filepath) so this evaluates consistently on Windows - // build hosts during testing. Host-side joins below use filepath - // because the dind daemon's filesystem is native. + // build hosts during testing. if !path.IsAbs(src) { return bindResolution{}, fmt.Errorf("bind source %q must be absolute", src) } cleaned := path.Clean(src) if host, suffix, ok := matchBindPrefix(cleaned, runnerBinds); ok { - return bindResolution{HostPath: path.Join(host, suffix)}, nil + if suffix == "" { + // The bind point itself (e.g. /var/run/docker.sock → the per-job + // dind socket, /etc/hosts → the per-job hosts file, the runner + // mount → the per-job runner directory). Every component of the + // host path was chosen by ephemerd and lives in a directory the + // job has no handle on, so there is nothing here for a symlink + // swap to act on: no pin, no staging, passed through exactly as + // before. This is also why /var/run/docker.sock keeps working — + // it is a socket, which is not a valid pin target. + return bindResolution{ResolvedPath: host}, nil + } + // A non-empty suffix is attacker-supplied. The important case is the + // runner directory: it is bind-mounted INTO the runner and is + // therefore fully job-writable, so `-v /evil/x:/y` with + // `evil` a planted symlink was a straight escape. Resolve it strictly + // beneath the host source and pin the result. + pin, err := pinBindSource(host, suffix, true) + if err != nil { + return bindResolution{}, fmt.Errorf("bind source %q rejected: %w", src, err) + } + if err := rejectUnbindableType(src, pin); err != nil { + return bindResolution{}, err + } + return bindResolution{ResolvedPath: pin.Logical(), Pin: pin}, nil } if runnerRootfsPath != "" { - candidate := path.Join(runnerRootfsPath, cleaned) - switch info, err := os.Stat(candidate); { - case err == nil: - // SECURITY: os.Stat followed symlinks, so `candidate` may resolve - // outside runnerRootfsPath if the runner planted a symlink (e.g. - // `ln -s / esc`). Reject any source that escapes the rootfs before - // handing it to containerd as a bind mount — otherwise the sibling - // container gets the VM host's filesystem. - if err := ensureWithinRootfs(runnerRootfsPath, candidate); err != nil { - return bindResolution{}, fmt.Errorf("bind source %q rejected: %w", src, err) - } - if info.IsDir() || info.Mode().IsRegular() { - return bindResolution{HostPath: candidate}, nil - } - return bindResolution{}, fmt.Errorf("bind source %q resolves to %s, which is not a regular file or directory (mode %s)", src, candidate, info.Mode()) - case errors.Is(err, os.ErrNotExist): - // Mirror Docker's auto-mkdir-on-missing-source semantic. The - // GHA runner emits -v entries for paths the runner creates - // lazily inside a step (e.g. /home/runner/_work/_actions - // only exists once actions/checkout downloads its handler). - // Real Docker creates the missing dir at create time and - // the workflow proceeds. Our dind has to do the same or - // every container: job 400s on the first lazy bind source. - // - // SECURITY: the source doesn't exist yet, but an ANCESTOR of it - // might be a symlink that escapes the rootfs — auto-mkdir would - // then create (and bind) a directory on the VM host FS. Verify the - // closest existing ancestor stays within the rootfs before - // creating anything. - if err := ensureAncestorWithinRootfs(runnerRootfsPath, candidate); err != nil { - return bindResolution{}, fmt.Errorf("bind source %q rejected: %w", src, err) - } - if mkErr := ensureBindSourceDir(candidate); mkErr != nil { - return bindResolution{}, fmt.Errorf("bind source %q could not be auto-created at %s: %w", src, candidate, mkErr) + // Mirror Docker's auto-mkdir-on-missing-source semantic. The GHA + // runner emits -v entries for paths it creates lazily inside a step + // (e.g. /home/runner/_work/_actions only exists once actions/checkout + // downloads its handler). Real Docker creates the missing dir at + // create time and the workflow proceeds; our dind has to do the same + // or every `container:` job 400s on the first lazy bind source. The + // creation itself is contained and symlink-safe — see pinBindSource. + pin, err := pinBindSource(runnerRootfsPath, cleaned, true) + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + return bindResolution{}, fmt.Errorf("bind source %q could not be resolved under the runner rootfs; a path component may be a symlink whose target escapes it, and contained resolution never follows one out: %w", src, err) } - // Re-check after creation: mkdir followed intermediate symlinks - // the same way stat would, so confirm the final path is contained. - if err := ensureWithinRootfs(runnerRootfsPath, candidate); err != nil { - return bindResolution{}, fmt.Errorf("bind source %q rejected after auto-create: %w", src, err) - } - return bindResolution{HostPath: candidate}, nil - default: - return bindResolution{}, fmt.Errorf("bind source %q could not be stat'd at %s: %w", src, candidate, err) + return bindResolution{}, fmt.Errorf("bind source %q rejected: %w", src, err) + } + if err := rejectUnbindableType(src, pin); err != nil { + return bindResolution{}, err } + return bindResolution{ResolvedPath: pin.Logical(), Pin: pin}, nil } if upperdir != "" { - candidate := path.Join(upperdir, cleaned) - if _, err := os.Stat(candidate); err == nil { - return bindResolution{HostPath: candidate}, nil + if pin, err := pinBindSource(upperdir, cleaned, false); err == nil { + return bindResolution{ResolvedPath: pin.Logical(), Pin: pin}, nil } } @@ -131,121 +147,35 @@ func translateBindSource(src string, runnerBinds map[string]string, runnerRootfs if lower == "" { continue } - candidate := path.Join(lower, cleaned) - if _, err := os.Stat(candidate); err == nil { - return bindResolution{HostPath: candidate, ForceReadOnly: true}, nil + if pin, err := pinBindSource(lower, cleaned, false); err == nil { + return bindResolution{ResolvedPath: pin.Logical(), ForceReadOnly: true, Pin: pin}, nil } } return bindResolution{}, fmt.Errorf("bind source %q is not visible to ephemerd dind (not in runner rootfs or known bind table)", src) } -// ensureBindSourceDir creates target (and any missing intermediate dirs -// between it and the closest existing ancestor) so a bind for a path the -// runner hasn't materialized yet can still proceed. Mirrors Docker's -// behavior for missing -v sources. +// rejectUnbindableType refuses a pinned source that is neither a directory nor +// a regular file, closing the pin on the way out. // -// Newly-created directories inherit ownership from the closest existing -// ancestor (Linux only, no-op elsewhere). This matters for the GHA -// `container:` flow: the closest ancestor is typically /home/runner/_work -// owned by uid 1001 (the runner user), so children we create are also -// uid 1001 — the runner can write into them once a step downloads an -// action or stages a file. Without the ownership flow, the new dir is -// root-owned and the runner gets EACCES the first time it tries to -// populate it. -func ensureBindSourceDir(target string) error { - ancestor := target - var newDirs []string - for { - info, err := os.Stat(ancestor) - if err == nil { - if !info.IsDir() { - return fmt.Errorf("ancestor %s is not a directory", ancestor) - } - break - } - if !errors.Is(err, os.ErrNotExist) { - return fmt.Errorf("stat ancestor %s: %w", ancestor, err) - } - newDirs = append(newDirs, ancestor) - parent := filepath.Dir(ancestor) - if parent == ancestor { - return fmt.Errorf("walked past root without finding existing ancestor of %s", target) - } - ancestor = parent - } - if len(newDirs) == 0 { +// Applied to every pinned branch, not just the rootfs one. The runner +// directory reachable through the bind table is job-writable, so a job can put +// a FIFO or a unix socket under it and ask for it by name; the mountpoint the +// stager would create for it is a plain file, and binding a FIFO onto one is +// at best confusing and at worst a container that blocks forever on open. +// There is no escalation either way — this is about the two pinned branches +// answering the same question the same way. +// +// The exact-match bind-table entries are deliberately not subject to this: +// /var/run/docker.sock IS a socket, it is ephemerd's own, and it is passed +// through unpinned. +func rejectUnbindableType(src string, pin *bindPin) error { + mode := pin.Mode() + if mode.IsDir() || mode.IsRegular() { return nil } - if err := os.MkdirAll(target, 0o755); err != nil { - return fmt.Errorf("mkdir %s: %w", target, err) - } - return chownNewDirsLikeAncestor(newDirs, ancestor) -} - -// ensureWithinRootfs verifies that candidate, after fully resolving symlinks, -// stays inside runnerRootfsPath (also symlink-resolved). It defends against a -// runner planting a symlink whose target escapes the rootfs onto the VM host -// filesystem. candidate is expected to exist; a resolve failure is treated as -// a rejection rather than a pass. -func ensureWithinRootfs(runnerRootfsPath, candidate string) error { - realRoot, err := filepath.EvalSymlinks(runnerRootfsPath) - if err != nil { - return fmt.Errorf("resolving runner rootfs %s: %w", runnerRootfsPath, err) - } - realCandidate, err := filepath.EvalSymlinks(candidate) - if err != nil { - return fmt.Errorf("resolving bind candidate %s: %w", candidate, err) - } - if !isWithin(realRoot, realCandidate) { - return fmt.Errorf("resolved path %s escapes runner rootfs %s", realCandidate, realRoot) - } - return nil -} - -// ensureAncestorWithinRootfs verifies containment for a candidate that does not -// exist yet: it walks up to the closest existing ancestor, resolves that -// ancestor's symlinks, and confirms it is inside the rootfs. This catches a -// symlinked intermediate directory before auto-mkdir would create (and bind) a -// path on the VM host filesystem. -func ensureAncestorWithinRootfs(runnerRootfsPath, candidate string) error { - realRoot, err := filepath.EvalSymlinks(runnerRootfsPath) - if err != nil { - return fmt.Errorf("resolving runner rootfs %s: %w", runnerRootfsPath, err) - } - ancestor := candidate - for { - resolved, err := filepath.EvalSymlinks(ancestor) - if err == nil { - if !isWithin(realRoot, resolved) { - return fmt.Errorf("closest existing ancestor %s (resolved %s) escapes runner rootfs %s", ancestor, resolved, realRoot) - } - return nil - } - if !errors.Is(err, os.ErrNotExist) { - return fmt.Errorf("resolving ancestor %s: %w", ancestor, err) - } - parent := filepath.Dir(ancestor) - if parent == ancestor { - return fmt.Errorf("walked past root resolving ancestor of %s", candidate) - } - ancestor = parent - } -} - -// isWithin reports whether target is root itself or lives underneath root. -// Both arguments are expected to be cleaned, symlink-resolved absolute paths. -// The separator-terminated prefix check prevents "/a/rootfs-evil" from -// matching root "/a/rootfs". -func isWithin(root, target string) bool { - if target == root { - return true - } - rootWithSep := root - if !strings.HasSuffix(rootWithSep, string(filepath.Separator)) { - rootWithSep += string(filepath.Separator) - } - return strings.HasPrefix(target, rootWithSep) + _ = pin.Close() + return fmt.Errorf("bind source %q resolves to something that is not a regular file or directory (mode %s)", src, mode) } // matchBindPrefix returns the host source for the longest runnerBinds key diff --git a/pkg/dind/bindtranslate_e2e_test.go b/pkg/dind/bindtranslate_e2e_test.go index d999003b..8cbcb2cd 100644 --- a/pkg/dind/bindtranslate_e2e_test.go +++ b/pkg/dind/bindtranslate_e2e_test.go @@ -51,6 +51,13 @@ func TestBindTranslation_RealContainerd(t *testing.T) { // doc, deferred follow-ups. t.Skipf("bind translation requires overlayfs snapshotter; goos=%s", goruntime.GOOS) } + // Bind translation now publishes every job-supplied source as a bind mount + // under /dind-binds before it reaches the OCI spec (issue #125), so + // this test needs mount(2) where it previously did not. It fails closed by + // design: staging must never degrade to putting a job-controlled path in + // the spec, because that is the vulnerability, and it would leave every + // test green while the escape was live. See requireBindStaging. + requireBindStaging(t) ctrdClient := sharedTestContainerd(t) log := slog.New(slog.NewTextHandler(io.Discard, &slog.HandlerOptions{Level: slog.LevelInfo})) @@ -197,7 +204,17 @@ func TestBindTranslation_RealContainerd(t *testing.T) { } } - opts, err := s.buildBindMounts(ctx, binds) + opts, pins, err := s.buildBindMounts(ctx, binds) + // The pins own real staging bind mounts here (this test constructs the + // server through dind.New, so it gets the production stager). Releasing + // them unmounts the staged sources; the stager teardown additionally + // releases the staging directory itself, which is a mount of its own — + // without it the dataDir removal below fails EBUSY. In production + // Server.Stop does both. + t.Cleanup(func() { + closeBindPins(pins) + s.stager.teardown() + }) if err != nil { t.Fatalf("buildBindMounts: %v", err) } @@ -218,13 +235,21 @@ func TestBindTranslation_RealContainerd(t *testing.T) { t.Errorf("docker.sock translated to %q, want %q", got, socketPath) } - // _temp must resolve into the snapshot upperdir, and the marker file - // must be reachable from that path — proves the snapshot's actual - // on-disk layout is what translation hands to containerd. + // _temp must expose the snapshot upperdir's contents, and the marker + // file must be reachable from the path the spec carries — which proves + // the snapshot's actual on-disk layout is what translation hands to + // containerd. + // + // The spec source is the STAGING path, not the upperdir path: the source + // is job-supplied, so it is pinned and republished under + // /dind-binds/ where the job cannot swap a component of it. See + // bindStager and issue #125. Asserting on the bytes reachable through it + // is the assertion that matters — asserting on the string would only + // re-encode the design. tempSrc := byDest["/__w/_temp"] - wantTempPrefix := filepath.Join(upperdir, "home", "runner", "_work", "_temp") - if !strings.HasPrefix(filepath.Clean(tempSrc), filepath.Clean(wantTempPrefix)) { - t.Errorf("_temp source %q does not point into upperdir %q", tempSrc, wantTempPrefix) + wantStagingPrefix := jobStagingDir(dataDir, "bind-translate-e2e") + string(filepath.Separator) + if !strings.HasPrefix(tempSrc, wantStagingPrefix) { + t.Errorf("_temp source %q is not published under the staging dir %q — a job-supplied source must never reach the spec as a path the job can influence", tempSrc, wantStagingPrefix) } gotMarker, err := os.ReadFile(filepath.Join(tempSrc, "marker.sh")) if err != nil { @@ -240,6 +265,13 @@ func TestBindTranslation_RealContainerd(t *testing.T) { // drops the bind and continues, leaving the test no way to notice. Against // the fix, `/etc/shadow` is not in the runner rootfs or bind table, so // buildBindMounts returns an error that the handler will surface as 400. +// +// Deliberately NOT gated on requireBindStaging, unlike the test above: the +// rejection happens in translateBindSource, before anything is pinned, so this +// never reaches the stager and needs no mount privilege. That is a property +// worth stating rather than leaving to luck — it is the reason this test still +// runs on the unprivileged CI runner, and if a future change moves the +// rejection after staging it should be re-gated rather than quietly skipped. func TestBindTranslation_RejectsForeignSource(t *testing.T) { if testing.Short() { t.Skip("skipping bind-translation e2e in short mode") @@ -303,7 +335,7 @@ func TestBindTranslation_RejectsForeignSource(t *testing.T) { } s.SetRunnerRootfs(snapshotKey, "", nil) - _, err = s.buildBindMounts(ctx, []string{"/etc/shadow:/x"}) + _, _, err = s.buildBindMounts(ctx, []string{"/etc/shadow:/x"}) if err == nil { t.Fatal("expected error rejecting /etc/shadow, got nil — silent-drop regression") } diff --git a/pkg/dind/bindtranslate_linux.go b/pkg/dind/bindtranslate_linux.go deleted file mode 100644 index 098fd888..00000000 --- a/pkg/dind/bindtranslate_linux.go +++ /dev/null @@ -1,34 +0,0 @@ -//go:build linux - -package dind - -import ( - "fmt" - "os" - "syscall" -) - -// chownNewDirsLikeAncestor copies the uid/gid of ancestor onto every dir -// in newDirs. Used by ensureBindSourceDir so an auto-created bind source -// inherits the runner user's ownership instead of being root-owned. -// Linux-only because Stat_t and the chown semantics differ on Windows -// (where ownership is ACL-based, not uid/gid). -func chownNewDirsLikeAncestor(newDirs []string, ancestor string) error { - info, err := os.Stat(ancestor) - if err != nil { - return fmt.Errorf("re-stat ancestor %s: %w", ancestor, err) - } - stat, ok := info.Sys().(*syscall.Stat_t) - if !ok { - // Filesystem doesn't expose POSIX stat info — skip chown rather - // than fail the whole bind (root ownership beats no mount). - return nil - } - uid, gid := int(stat.Uid), int(stat.Gid) - for _, d := range newDirs { - if err := os.Chown(d, uid, gid); err != nil { - return fmt.Errorf("chown %s to %d:%d: %w", d, uid, gid, err) - } - } - return nil -} diff --git a/pkg/dind/bindtranslate_other.go b/pkg/dind/bindtranslate_other.go deleted file mode 100644 index 37e31588..00000000 --- a/pkg/dind/bindtranslate_other.go +++ /dev/null @@ -1,10 +0,0 @@ -//go:build !linux - -package dind - -// chownNewDirsLikeAncestor is a no-op outside Linux. Ownership inheritance -// only matters in production (in-VM Linux); cross-platform tests don't -// care about uid/gid on the auto-created tempdirs they generate. -func chownNewDirsLikeAncestor(newDirs []string, ancestor string) error { - return nil -} diff --git a/pkg/dind/bindtranslate_symlink_test.go b/pkg/dind/bindtranslate_symlink_test.go index bb32f9f2..33bcc182 100644 --- a/pkg/dind/bindtranslate_symlink_test.go +++ b/pkg/dind/bindtranslate_symlink_test.go @@ -3,6 +3,7 @@ package dind import ( "os" "path/filepath" + goruntime "runtime" "strings" "testing" ) @@ -73,22 +74,58 @@ func TestTranslateBindSource_SymlinkEscapeViaAncestor_Rejected(t *testing.T) { // TestTranslateBindSource_InternalSymlink_Allowed confirms the fix does not // over-reject: a symlink that stays inside the rootfs (a legitimate overlay // arrangement) resolves fine. +// +// Both realistic forms are covered, because they are what real runner images +// contain: a relative link, and an ABSOLUTE link written from the container's +// point of view — Ubuntu's merged-usr layout makes /bin, /lib and /sbin +// absolute symlinks into /usr, so every bind that traverses them takes the +// second path. Resolution therefore has to reinterpret an absolute symlink +// relative to the rootfs (which openat2's RESOLVE_IN_ROOT does, and Go's +// os.Root notably does not — it rejects absolute symlinks outright, which is +// why os.Root is not used here). +// +// A link whose target is the rootfs's own HOST path is deliberately not +// covered as an "allowed" case: it cannot occur in a container image (nothing +// inside the container can name the host path of its own rootfs) and treating +// it as in-bounds would mean resolving symlink targets against the host's root +// rather than the container's, which is the escape this all exists to prevent. +// See TestTranslateBindSource_SymlinkEscape_Rejected. func TestTranslateBindSource_InternalSymlink_Allowed(t *testing.T) { - rootfs := t.TempDir() - if err := os.MkdirAll(filepath.Join(rootfs, "real", "dir"), 0o755); err != nil { - t.Fatal(err) - } - if err := os.WriteFile(filepath.Join(rootfs, "real", "dir", "file"), []byte("ok"), 0o644); err != nil { - t.Fatal(err) - } - // `/link -> /real` — an in-rootfs symlink. - mkSymlinkOrSkip(t, filepath.Join(rootfs, "real"), filepath.Join(rootfs, "link")) + for _, tc := range []struct { + name string + target string // symlink target, as written inside the rootfs + linuxOnly bool + }{ + {name: "relative", target: "real"}, + // Reinterpreting an absolute target relative to the rootfs is + // RESOLVE_IN_ROOT, i.e. a kernel feature. The dev-host stub in + // bindpin_other.go has no equivalent (it resolves against the real + // filesystem root) and is not a production path — see the comment + // there. Running this case off Linux would only assert that the stub + // is a stub. + {name: "container_absolute_merged_usr_style", target: "/real", linuxOnly: true}, + } { + t.Run(tc.name, func(t *testing.T) { + if tc.linuxOnly && goruntime.GOOS != "linux" { + t.Skipf("absolute in-rootfs symlinks are reinterpreted by openat2's RESOLVE_IN_ROOT; goos=%s has no bind translation in production", goruntime.GOOS) + } + rootfs := t.TempDir() + if err := os.MkdirAll(filepath.Join(rootfs, "real", "dir"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(rootfs, "real", "dir", "file"), []byte("ok"), 0o644); err != nil { + t.Fatal(err) + } + mkSymlinkOrSkip(t, tc.target, filepath.Join(rootfs, "link")) - got, err := translateBindSource("/link/dir/file", nil, rootfs, "", nil) - if err != nil { - t.Fatalf("in-rootfs symlink should be allowed, got: %v", err) - } - if got.HostPath == "" { - t.Fatal("expected a resolved host path") + got, err := translateBindSource("/link/dir/file", nil, rootfs, "", nil) + if err != nil { + t.Fatalf("in-rootfs symlink should be allowed, got: %v", err) + } + if got.ResolvedPath == "" { + t.Fatal("expected a resolved host path") + } + closeBindPins([]*bindPin{got.Pin}) + }) } } diff --git a/pkg/dind/bindtranslate_test.go b/pkg/dind/bindtranslate_test.go index 856488b4..f41e052f 100644 --- a/pkg/dind/bindtranslate_test.go +++ b/pkg/dind/bindtranslate_test.go @@ -15,7 +15,6 @@ import ( ocispec "github.com/opencontainers/runtime-spec/specs-go" ) - // applyOpts invokes a list of oci.SpecOpts against an empty spec so tests // can assert what they produced. withBindMount and friends don't touch the // oci.Client / containers.Container args, so nil values are fine. @@ -34,6 +33,9 @@ func testServer() *Server { return &Server{ log: slog.New(slog.NewTextHandler(io.Discard, nil)), mu: sync.Mutex{}, + // Translation policy only — see unstagedTestStager for why these + // tests cannot exercise the real staging mount and what does. + stager: unstagedTestStager{}, } } @@ -53,8 +55,8 @@ func TestTranslateBindSource_UpperdirMatch_ReturnsReadWrite(t *testing.T) { t.Fatalf("translate: %v", err) } want := path.Join(upper, "home/runner/_work/_temp") - if got.HostPath != want { - t.Errorf("HostPath = %q, want %q", got.HostPath, want) + if got.ResolvedPath != want { + t.Errorf("ResolvedPath = %q, want %q", got.ResolvedPath, want) } if got.ForceReadOnly { t.Error("ForceReadOnly = true, want false (upperdir is writable)") @@ -77,8 +79,8 @@ func TestTranslateBindSource_LowerdirMatch_ForcesReadOnly(t *testing.T) { t.Fatalf("translate: %v", err) } want := path.Join(lower, "home/runner/externals") - if got.HostPath != want { - t.Errorf("HostPath = %q, want %q", got.HostPath, want) + if got.ResolvedPath != want { + t.Errorf("ResolvedPath = %q, want %q", got.ResolvedPath, want) } if !got.ForceReadOnly { t.Error("ForceReadOnly = false, want true (image layer must stay immutable)") @@ -98,8 +100,8 @@ func TestTranslateBindSource_RunnerBind_Translates(t *testing.T) { if err != nil { t.Fatalf("translate: %v", err) } - if got.HostPath != "/run/ephemerd/jobs/abc/docker/d.sock" { - t.Errorf("HostPath = %q, want translated socket path", got.HostPath) + if got.ResolvedPath != "/run/ephemerd/jobs/abc/docker/d.sock" { + t.Errorf("ResolvedPath = %q, want translated socket path", got.ResolvedPath) } if got.ForceReadOnly { t.Error("ForceReadOnly = true on runner-bind translation; that path category should preserve writability") @@ -110,16 +112,33 @@ func TestTranslateBindSource_RunnerBind_Translates(t *testing.T) { // requests like -v /workspace/foo:/x when the runner has /workspace bound // to a host scratch dir. The leftover suffix must be appended to the host // source. +// +// The scratch dir is real, not a made-up path, because the suffix is +// job-supplied and therefore resolved beneath the host source rather than +// string-joined onto it (issue #125: the per-job runner directory reached +// through this branch is bind-mounted INTO the runner and is fully +// job-writable, so `-v /evil/x:/y` with `evil` a planted +// symlink used to be a straight escape — this branch had no containment +// check of any kind). Resolution needs the root to exist; in production +// every entry in the bind table is a file or directory ephemerd created. func TestTranslateBindSource_RunnerBindSubpath_Translates(t *testing.T) { - binds := map[string]string{"/workspace": "/srv/ephemerd/scratch"} + scratch := t.TempDir() + if err := os.MkdirAll(filepath.Join(scratch, "foo", "bar"), 0o755); err != nil { + t.Fatal(err) + } + binds := map[string]string{"/workspace": scratch} got, err := translateBindSource("/workspace/foo/bar", binds, "", "", nil) if err != nil { t.Fatalf("translate: %v", err) } - want := "/srv/ephemerd/scratch/foo/bar" - if got.HostPath != want { - t.Errorf("HostPath = %q, want %q", got.HostPath, want) + want := path.Join(scratch, "foo/bar") + if got.ResolvedPath != want { + t.Errorf("ResolvedPath = %q, want %q", got.ResolvedPath, want) + } + if got.Pin == nil { + t.Error("Pin = nil for a job-supplied suffix; that source must be pinned and staged, not string-joined") } + closeBindPins([]*bindPin{got.Pin}) } // TestTranslateBindSource_LongestPrefixWins guards against a parent bind @@ -134,8 +153,8 @@ func TestTranslateBindSource_LongestPrefixWins(t *testing.T) { if err != nil { t.Fatalf("translate: %v", err) } - if got.HostPath != "/host/etc/hosts.runtime" { - t.Errorf("HostPath = %q — longest prefix /etc/hosts should win over /etc", got.HostPath) + if got.ResolvedPath != "/host/etc/hosts.runtime" { + t.Errorf("ResolvedPath = %q — longest prefix /etc/hosts should win over /etc", got.ResolvedPath) } } @@ -183,8 +202,8 @@ func TestTranslateBindSource_DotDotTraversal_StaysInsideUpperdir(t *testing.T) { t.Fatalf("translate: %v", err) } want := path.Join(upper, "home/etc/shadow") - if got.HostPath != want { - t.Errorf("HostPath = %q, want %q (must stay inside upperdir tree)", got.HostPath, want) + if got.ResolvedPath != want { + t.Errorf("ResolvedPath = %q, want %q (must stay inside upperdir tree)", got.ResolvedPath, want) } // A path that climbs above /: path.Clean(/../../etc/shadow) = /etc/shadow. @@ -222,8 +241,8 @@ func TestTranslateBindSource_PreferUpperOverLower(t *testing.T) { t.Fatalf("translate: %v", err) } want := path.Join(upper, filepath.ToSlash(rel)) - if got.HostPath != want { - t.Errorf("HostPath = %q, want upperdir copy %q", got.HostPath, want) + if got.ResolvedPath != want { + t.Errorf("ResolvedPath = %q, want upperdir copy %q", got.ResolvedPath, want) } if got.ForceReadOnly { t.Error("ForceReadOnly = true, want false — upperdir copy is writable") @@ -259,8 +278,8 @@ func TestTranslateBindSource_RootfsPathResolvesToMergedView(t *testing.T) { t.Fatalf("translate: %v", err) } want := path.Join(rootfs, "home/runner/externals/node20/bin/node") - if got.HostPath != want { - t.Errorf("HostPath = %q, want %q (must resolve against the rootfs mount)", got.HostPath, want) + if got.ResolvedPath != want { + t.Errorf("ResolvedPath = %q, want %q (must resolve against the rootfs mount)", got.ResolvedPath, want) } if got.ForceReadOnly { t.Error("ForceReadOnly = true on rootfs-path resolution; writes copy-up into the runner's own upperdir and shouldn't be downgraded") @@ -268,7 +287,7 @@ func TestTranslateBindSource_RootfsPathResolvesToMergedView(t *testing.T) { // Round-trip: reading through the translated path must return the // planted bytes, proving the resolved host path actually points at // the runner's view of this file. - gotBody, err := os.ReadFile(got.HostPath) + gotBody, err := os.ReadFile(got.ResolvedPath) if err != nil { t.Fatalf("read via translated path: %v", err) } @@ -297,10 +316,10 @@ func TestTranslateBindSource_RootfsPathAutoCreatesMissingSource(t *testing.T) { t.Fatalf("translate: %v (auto-mkdir should have created the missing dir)", err) } wantPath := path.Join(rootfs, "home/runner/_work/_actions") - if got.HostPath != wantPath { - t.Errorf("HostPath = %q, want %q", got.HostPath, wantPath) + if got.ResolvedPath != wantPath { + t.Errorf("ResolvedPath = %q, want %q", got.ResolvedPath, wantPath) } - info, err := os.Stat(got.HostPath) + info, err := os.Stat(got.ResolvedPath) if err != nil { t.Fatalf("stat auto-created dir: %v", err) } @@ -380,7 +399,8 @@ func TestBuildBindMounts_GHARunnerContainer(t *testing.T) { "/home/runner/_work/_temp/_github_home:/github/home", "/home/runner/_work/_temp/_github_workflow:/github/workflow", } - opts, err := s.buildBindMounts(context.Background(), binds) + opts, pins, err := s.buildBindMounts(context.Background(), binds) + t.Cleanup(func() { closeBindPins(pins) }) if err != nil { t.Fatalf("buildBindMounts: %v", err) } @@ -446,7 +466,7 @@ func TestBuildBindMounts_RejectsUnknownSource(t *testing.T) { return []string{t.TempDir()}, nil } - _, err := s.buildBindMounts(context.Background(), []string{"/etc/shadow:/x"}) + _, _, err := s.buildBindMounts(context.Background(), []string{"/etc/shadow:/x"}) if err == nil { t.Fatal("expected error rejecting unknown bind source, got nil") } @@ -461,7 +481,7 @@ func TestBuildBindMounts_RejectsUnknownSource(t *testing.T) { // already in the runner-bind table gets rejected loudly. func TestBuildBindMounts_NoRunnerRegistered(t *testing.T) { s := testServer() - _, err := s.buildBindMounts(context.Background(), []string{"/home/runner/_work/_temp:/x"}) + _, _, err := s.buildBindMounts(context.Background(), []string{"/home/runner/_work/_temp:/x"}) if err == nil { t.Fatal("expected error when runner rootfs is unregistered, got nil") } diff --git a/pkg/dind/containers.go b/pkg/dind/containers.go index 7b1b73d7..954e9bf9 100644 --- a/pkg/dind/containers.go +++ b/pkg/dind/containers.go @@ -88,6 +88,15 @@ type containerEntry struct { ExtraHosts []string // user-provided "host:ip" entries (--add-host) PortForwards []func() // stop functions for port-forward proxy goroutines + // BindPins own the staging bind mounts that back this container's + // job-supplied -v sources (see bindPin / bindStager and issue #125). + // + // They are held for the container's whole life, not just until the task + // starts: `docker restart` creates a new task, and runc re-reads the + // spec's bind source when it does. Released in cleanupContainer, and + // again — Close is idempotent — by the job-wide teardown in Server.Stop. + BindPins []*bindPin + // started is closed by handleContainerStart once the task is created and // running. handleContainerAttach blocks on it so the Docker CLI's "attach // then start" sequence works correctly: attach hijacks the conn early, @@ -362,6 +371,19 @@ func (s *Server) handleContainerCreate(w http.ResponseWriter, r *http.Request) { return } + // Bind sources are resolved to pinned inodes and published at staging + // paths ephemerd owns (see bindStager). Both the descriptors and the + // staging mounts have to be released if this create does not end with a + // registered containerEntry to own them — hence the adoption guard rather + // than a plain defer. + var bindPins []*bindPin + pinsAdopted := false + defer func() { + if !pinsAdopted { + closeBindPins(bindPins) + } + }() + // Privileged-elevation gate runs before the client check: it's a // request-shape validation, not a runtime-state check. This also // makes the unit tests trivial — they don't need a containerd client. @@ -527,7 +549,8 @@ func (s *Server) handleContainerCreate(w http.ResponseWriter, r *http.Request) { // /var/run/docker.sock). The pre-fix shim silently dropped any // bind whose source didn't os.Stat — leaving GHA `container:` jobs // failing downstream with "sh: cannot open /__w/_temp/.sh". - bindOpts, berr := s.buildBindMounts(r.Context(), req.HostConfig.Binds) + bindOpts, pins, berr := s.buildBindMounts(r.Context(), req.HostConfig.Binds) + bindPins = pins if berr != nil { // Log at WARN so operators can see WHY a sibling create was // rejected — the message also goes to the 400 response, but @@ -687,6 +710,7 @@ func (s *Server) handleContainerCreate(w http.ResponseWriter, r *http.Request) { Tty: req.Tty, HostsPath: hostsPath, ExtraHosts: extraHosts, + BindPins: bindPins, started: make(chan struct{}), } @@ -694,6 +718,9 @@ func (s *Server) handleContainerCreate(w http.ResponseWriter, r *http.Request) { s.containers[id] = entry s.assignContainerNetwork(entry, req) s.mu.Unlock() + // The entry now owns the pins and their staging mounts; cleanupContainer + // is responsible for releasing them. + pinsAdopted = true s.log.Info("container created", "id", id, "name", name, "image", req.Image, "labels", entry.Labels) @@ -1428,6 +1455,13 @@ func (s *Server) cleanupContainer(ctx context.Context, id string, entry *contain if err := os.RemoveAll(buildkitDir); err != nil { s.log.Debug("buildkit dir cleanup", "id", id, "error", err) } + + // Release the staged bind sources last: the container's own copies of + // those mounts are independent, but tearing ours down only after runc has + // finished with the container removes any ordering question. Close is + // idempotent, so Server.Stop's job-wide teardown is a no-op after this. + closeBindPins(entry.BindPins) + entry.BindPins = nil } // destroyAllContainers cleans up every container in the map. @@ -1481,7 +1515,13 @@ func generateContainerID() string { // surface HTTP 400 — the pre-fix shim silently dropped these, which left // GHA `container:` jobs to fail downstream with confusing "cannot open" // errors. See translateBindSource for the resolution policy. -func (s *Server) buildBindMounts(ctx context.Context, binds []string) ([]oci.SpecOpts, error) { +// +// The returned pins own the staging mounts that back the spec sources. They +// MUST be closed when the container is destroyed (cleanupContainer) or the +// node leaks a bind mount per bind — and each of those pins the runner's +// rootfs, which blocks snapshot deletion. On error this function releases +// everything it opened before returning. +func (s *Server) buildBindMounts(ctx context.Context, binds []string) ([]oci.SpecOpts, []*bindPin, error) { upperdir, lowerdirs, layerErr := s.runnerRootfsLayers(ctx) if layerErr != nil { s.log.Warn("could not load runner rootfs layers for bind translation", "error", layerErr) @@ -1491,7 +1531,22 @@ func (s *Server) buildBindMounts(ctx context.Context, binds []string) ([]oci.Spe runnerRootfs := s.runnerRootfsPath s.mu.Unlock() + // The bind resolver has no logger of its own. If it has had to fall back + // from openat2 to the manual contained walk, this is where that becomes + // visible — once per process, on the first bind after it happened. + // Without it the switch is silent, and a node resolving binds by the + // fallback path looks identical to one that is not until something + // unexpected starts failing. + if note := openat2FallbackNotice(); note != "" { + s.log.Warn("dind bind source resolver fell back from openat2", "detail", note) + } + out := make([]oci.SpecOpts, 0, len(binds)) + var pins []*bindPin + fail := func(format string, args ...any) ([]oci.SpecOpts, []*bindPin, error) { + closeBindPins(pins) + return nil, nil, fmt.Errorf(format, args...) + } for _, bind := range binds { parts := strings.SplitN(bind, ":", 3) if len(parts) < 2 { @@ -1501,20 +1556,39 @@ func (s *Server) buildBindMounts(ctx context.Context, binds []string) ([]oci.Spe requestedRO := len(parts) == 3 && parts[2] == "ro" if err := s.rejectUnbackedGuestBind(src, runnerBinds); err != nil { - return nil, fmt.Errorf("bind mount %s -> %s rejected: %w", src, dst, err) + return fail("bind mount %s -> %s rejected: %w", src, dst, err) } resolved, terr := translateBindSource(src, runnerBinds, runnerRootfs, upperdir, lowerdirs) if terr != nil { - return nil, fmt.Errorf("bind mount %s -> %s rejected: %w", src, dst, terr) + return fail("bind mount %s -> %s rejected: %w", src, dst, terr) + } + + // What goes in the spec. For a source with no job-controlled + // component there is no pin and the resolved path is already safe + // for runc to walk. For everything else the pinned inode is + // published at an ephemerd-owned staging path first — see + // bindStager, and issue #125 for what happens without it. + specSource := resolved.ResolvedPath + if resolved.Pin != nil { + pins = append(pins, resolved.Pin) + if s.stager == nil { + return fail("bind mount %s -> %s rejected: this dind server has no bind stager, so the source cannot be published at a path the job is unable to swap; refusing rather than mounting a job-controlled path (this is a wiring bug — dind.New always installs one)", src, dst) + } + staged, serr := s.stager.stage(resolved.Pin) + if serr != nil { + return fail("bind mount %s -> %s rejected: %w", src, dst, serr) + } + specSource = staged } + mountOpts := []string{"rbind", "rw"} if requestedRO || resolved.ForceReadOnly { mountOpts = []string{"rbind", "ro"} } - out = append(out, withBindMount(resolved.HostPath, dst, mountOpts)) + out = append(out, withBindMount(specSource, dst, mountOpts)) } - return out, nil + return out, pins, nil } // rejectUnbackedGuestBind refuses a sibling -v source whose contents this diff --git a/pkg/dind/dind.go b/pkg/dind/dind.go index 70322c89..50b70170 100644 --- a/pkg/dind/dind.go +++ b/pkg/dind/dind.go @@ -46,8 +46,8 @@ const sharedNamespace = "ephemerd" // Server is a per-job fake Docker daemon. type Server struct { jobID string - jobNamespace string // per-job containerd namespace for isolation - cacheNamespace string // per-(provider,repo) shared image cache namespace; empty disables caching + jobNamespace string // per-job containerd namespace for isolation + cacheNamespace string // per-(provider,repo) shared image cache namespace; empty disables caching transport Transport // how the API is exposed to the job container; see listen.go dockerDir string // /jobs//docker — per-job scratch, exists on every transport sockPath string // host-side unix socket path; empty on the TCP transport @@ -71,6 +71,13 @@ type Server struct { runnerNetNS string // path to runner container's net namespace; used to install DNAT rules for port bindings allowPrivileged bool // gate for docker run --privileged / --cap-add; see config.DindConfig.AllowPrivileged + // stager publishes every job-supplied bind source at a path under + // /dind-binds// that only root can reach, so the path + // the OCI spec carries has nothing in it the job can swap between + // validation and runc's mount. See bindStager and issue #125. Always + // set by New; a nil stager makes bind translation fail closed. + stager bindStager + // mirror routes this job's image pulls through a LAN pull-through // cache. Nil means no mirror and every pull path below is unchanged. // This is the hot one: dind pulls into a per-job namespace, so the @@ -231,6 +238,7 @@ func New(cfg Config) (*Server, error) { buildkit: cfg.BuildKit, runnerNetNS: cfg.RunnerNetNS, allowPrivileged: cfg.AllowPrivileged, + stager: newBindStager(cfg.DataDir, cfg.JobID, cfg.Log), mirror: cfg.RegistryMirror, log: cfg.Log.With("component", "dind", "job_id", cfg.JobID), images: make(map[string]*imageEntry), @@ -415,10 +423,46 @@ func (s *Server) Start() error { func (s *Server) Stop() { s.log.Info("stopping fake docker daemon") + // STOP SERVING FIRST. Everything below tears down state that in-flight + // requests can still be creating, and the most dangerous of those is bind + // staging: handleContainerCreate publishes bind mounts under the job's + // staging directory, and the stager teardown further down removes that + // directory. os.RemoveAll walking into a bind mount that appeared after + // the teardown's mount check deletes the files visible THROUGH the mount + // — the runner's own rootfs — and only then reports EBUSY on the + // mountpoint. (Verified: RemoveAll over a live bind mount empties the + // source and returns "device or resource busy" afterwards.) + // + // The stager refuses to stage after teardown and holds its lock across + // the mount, so the race is closed on that side too; this ordering means + // there is nothing left to race with in the first place. It is also just + // correct on its own terms — a sibling that still holds DOCKER_HOST (the + // TCP transport) can keep issuing docker create calls right through + // teardown, because siblings are not in the runner's cgroup and outlive + // the runner task the runtime killed before calling Stop. + if s.server != nil { + if err := s.server.Shutdown(context.Background()); err != nil { + s.log.Debug("shutting down fake docker server", "error", err) + } + } + if s.listener != nil { + if err := s.listener.Close(); err != nil { + s.log.Debug("closing listener", "error", err) + } + } + // Destroy all exec processes and containers created through this socket. s.destroyAllExecs() s.destroyAllContainers() + // Every container's cleanup released its own staged bind mounts; this is + // the backstop for anything that never made it onto a containerEntry + // (a create that failed between staging and registration). Leaving one + // behind pins the runner's rootfs mount and blocks snapshot deletion. + if s.stager != nil { + s.stager.teardown() + } + // Clean up the per-job containerd namespace. destroyAllContainers handles // containers tracked in the in-memory map; this catches stragglers // (kindest/node-side containerd creations that landed in the same @@ -443,16 +487,6 @@ func (s *Server) Stop() { CleanupJobBuildRecords(context.Background(), s.client, s.buildkit.ContainerdNamespace(), s.jobID, s.log) } - if s.server != nil { - if err := s.server.Shutdown(context.Background()); err != nil { - s.log.Debug("shutting down fake docker server", "error", err) - } - } - if s.listener != nil { - if err := s.listener.Close(); err != nil { - s.log.Debug("closing listener", "error", err) - } - } // Remove the firewall allow opened for this job's TCP listener. The // container address is half the rule, so it has to go back in for the // delete to find the right one — and, crucially, only that one: a diff --git a/pkg/runtime/runtime.go b/pkg/runtime/runtime.go index 1dc50bcf..41bdf882 100644 --- a/pkg/runtime/runtime.go +++ b/pkg/runtime/runtime.go @@ -310,6 +310,15 @@ func (r *Runtime) SetTaskHooks(onStarted, onDestroy func(*RunnerEnv)) { func (r *Runtime) CleanOrphans(ctx context.Context) error { ctx = namespaces.WithNamespace(ctx, namespace) + // Unmount dind bind-staging mounts left by a previous process. This runs + // FIRST, before any container or snapshot deletion: each leaked staging + // mount holds a reference to the runner rootfs it was bound from, so + // while one is present containerd cannot delete that container's + // snapshot and the sweep below silently fails to reclaim the space. + // A hard kill (SIGKILL, panic, node reset) is the case that produces + // them — every graceful path unmounts as it goes. + dind.SweepStagedBinds(r.cfg.DataDir, r.cfg.Log) + // Clean orphan containers (and their associated snapshots) containers, err := r.client.Containers(ctx) if err != nil {