From fdf55512164e5ceabf4f3c9b33831ff0c3bc822f Mon Sep 17 00:00:00 2001 From: Luther Monson Date: Tue, 18 Aug 2026 16:30:41 -0700 Subject: [PATCH 1/4] fix(security): stage dind bind sources so runc cannot re-walk a job-controlled path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #125. translateBindSource proved containment with filepath.EvalSymlinks and then returned the *unresolved* joined string, which containers.go handed to containerd as the OCI bind source. runc walks that string again, in its own process, at task start. Between the check and the mount — the rest of translation, the containerd container create, the whole Docker create-then-start round trip — the job (which owns every byte of its own rootfs) could swap a validated component for a symlink out of the rootfs, and runc mounted whatever it pointed at by then. The runner-bind branch was worse: it had no containment check at all, and the per-job runner directory in that table is bind-mounted into the runner and therefore job-writable. Reproduced against real runc 1.3.4 (kernel 6.18, WSL2): with the validated directory swapped after the spec was written, the container printed the attacker's marker and exited 0. Two halves, both necessary. Resolution (pkg/dind/bindpin_linux.go). Every source with a job-supplied component is resolved once with openat2(RESOLVE_IN_ROOT|RESOLVE_NO_MAGICLINKS) anchored at that branch's root, and the result is held as an O_PATH descriptor. Containment is enforced by the kernel during the walk instead of by a string comparison after it, and there is no name left to re-point. RESOLVE_IN_ROOT rather than os.Root because it reinterprets absolute symlinks relative to the root, which merged-usr runner images require. Pre-5.6 kernels fall back to an equivalent O_PATH|O_NOFOLLOW component walk. Auto-mkdir creates through mkdirat against a pinned parent and re-opens O_NOFOLLOW, so losing the race to a planted symlink fails instead of escaping. Staging (pkg/dind/bindstage_linux.go). The descriptor cannot go in the spec: runc does not re-resolve /proc//fd/ (its error echoes it verbatim) but mount(2) rejects it, because a bind source must live in the caller's mount namespace and runc always has its own. That is unconditional — legitimate binds fail identically — so the fd handoff the earlier fix/linux-isolation-hardening branch shipped is a functional regression, not a fix. Verified here and kept as an executable test. Instead ephemerd performs the bind itself, in its own mount namespace where /proc/self/fd resolves, onto /dind-binds//, and gives runc that path. Every component is root-owned and 0700 and nothing in the name comes from the request, so runc re-walking it is safe; ensureTrustedAncestry checks that precondition rather than assuming it. Staging lives outside /jobs/ on purpose: the orphan sweep RemoveAll's that tree, and deleting through a live bind mount deletes the source — which here would be the runner's rootfs. Pins are held on the containerEntry for the container's life (docker restart makes runc re-read the spec) and released in cleanupContainer; Server.Stop tears down the job's staging dir; a hard kill skips both, so dind.SweepStagedBinds runs from Runtime.CleanOrphans before the container/snapshot sweep, because a leaked staging mount pins the rootfs and makes the snapshot undeletable. Failure mode inverts on purpose: a source that cannot be staged now fails docker create with a 400 instead of mounting something unvalidated. The error names the staging dir and the requirement. Windows and macOS are untouched — bind translation is not wired into either native container path. bindpin_other.go / bindstage_other.go exist only so the translation-policy tests build on a dev host and carry no security property. VERIFIED ON LINUX, not inferred. On WSL2 (kernel 6.18) as root, against the runc this repo embeds: control (pre-fix shape): CONTAINER-SAW: SWAPPED staged (fixed shape): CONTAINER-SAW: PINNED /proc//fd source: mount ... flags=MS_BIND|MS_REC: invalid argument plus the legitimate-bind regression set (directory, regular file, auto-mkdir, merged-usr symlink traversal, docker.sock/hosts/resolv.conf passthrough), teardown, the startup sweep, and the full pkg/dind suite against a real containerd. The tests that carry the security property need CAP_SYS_ADMIN and skip on the unprivileged CI runner; ci.yml prints the skips, following the precedent already there for the dind pull test, because a green package on an unprivileged host is exactly how the previous attempt passed review. --- .github/workflows/ci.yml | 13 + docs/arch/dind-bind-translation.md | 139 +++++++- pkg/dind/bindpin.go | 161 +++++++++ pkg/dind/bindpin_linux.go | 344 ++++++++++++++++++ pkg/dind/bindpin_other.go | 124 +++++++ pkg/dind/bindstage.go | 77 ++++ pkg/dind/bindstage_linux.go | 324 +++++++++++++++++ pkg/dind/bindstage_linux_test.go | 470 +++++++++++++++++++++++++ pkg/dind/bindstage_other.go | 37 ++ pkg/dind/bindstage_runc_linux_test.go | 255 ++++++++++++++ pkg/dind/bindstage_testutil_test.go | 30 ++ pkg/dind/bindtranslate.go | 252 +++++-------- pkg/dind/bindtranslate_e2e_test.go | 34 +- pkg/dind/bindtranslate_linux.go | 34 -- pkg/dind/bindtranslate_other.go | 10 - pkg/dind/bindtranslate_symlink_test.go | 67 +++- pkg/dind/bindtranslate_test.go | 71 ++-- pkg/dind/containers.go | 76 +++- pkg/dind/dind.go | 16 + pkg/runtime/runtime.go | 9 + 20 files changed, 2263 insertions(+), 280 deletions(-) create mode 100644 pkg/dind/bindpin.go create mode 100644 pkg/dind/bindpin_linux.go create mode 100644 pkg/dind/bindpin_other.go create mode 100644 pkg/dind/bindstage.go create mode 100644 pkg/dind/bindstage_linux.go create mode 100644 pkg/dind/bindstage_linux_test.go create mode 100644 pkg/dind/bindstage_other.go create mode 100644 pkg/dind/bindstage_runc_linux_test.go create mode 100644 pkg/dind/bindstage_testutil_test.go delete mode 100644 pkg/dind/bindtranslate_linux.go delete mode 100644 pkg/dind/bindtranslate_other.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 261b9b83..0d905a38 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -42,6 +42,19 @@ 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. + - name: Privileged-test coverage report (dind bind staging, #125) + run: | + go test -tags containers_image_openpgp -count=1 \ + -run 'TestBindStaging|TestSweepStagedBinds|TestEnsureTrustedAncestry' \ + -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..a38b1361 100644 --- a/docs/arch/dind-bind-translation.md +++ b/docs/arch/dind-bind-translation.md @@ -142,7 +142,99 @@ 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 equivalent `O_PATH|O_NOFOLLOW` +component-by-component walk. + +`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: every component is root-owned +and 0700, and nothing in the name is derived from the job's request (the leaf +is a bare counter). `ensureTrustedAncestry` checks that precondition on first +use and fails the bind rather than assuming it. 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). + +**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 +342,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..a5ff91fb --- /dev/null +++ b/pkg/dind/bindpin_linux.go @@ -0,0 +1,344 @@ +//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 support openat2, +// so the fallback does not pay for a failing syscall on every bind. Atomic +// because sibling container creates are served concurrently. +var openat2Unsupported atomic.Bool + +// 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 isOpenat2Unavailable(err) { + openat2Unsupported.Store(true) + 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() } + +// isOpenat2Unavailable distinguishes "this kernel/sandbox has no openat2" from +// a genuine resolution failure. ENOSYS is a pre-5.6 kernel; EPERM is a seccomp +// filter that does not know the syscall; E2BIG is an OpenHow the kernel does +// not understand. +// +// EACCES and EINVAL are deliberately NOT in this list even though the syscall +// can return them for an unsupported flag set: both are also ordinary +// resolution outcomes (EACCES on a directory we may not search, EINVAL from +// RESOLVE_IN_ROOT rejecting an escape on some kernels), and treating a real +// rejection as "kernel too old" would silently downgrade every subsequent bind +// on the node to the fallback walk. +func isOpenat2Unavailable(err error) bool { + return errors.Is(err, unix.ENOSYS) || + errors.Is(err, unix.EPERM) || + errors.Is(err, unix.E2BIG) +} + +// 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. +// +// 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) { + cur, err := unix.Dup(rootFd) + if err != nil { + return -1, fmt.Errorf("dup bind root: %w", err) + } + remaining := append([]string(nil), comps...) + links := 0 + + for len(remaining) > 0 { + name := remaining[0] + remaining = remaining[1:] + switch name { + case "", ".": + continue + case "..": + // RESOLVE_IN_ROOT makes ".." at the root a no-op; anywhere else + // it would need parent tracking. Refusing is stricter and, for + // a lexically cleaned bind source, unreachable except through a + // symlink target. + closePinFd(cur) + return -1, errBindPathTraversal + } + + next, err := unix.Openat(cur, name, unix.O_PATH|unix.O_NOFOLLOW|unix.O_CLOEXEC, 0) + if err != nil { + closePinFd(cur) + return -1, err + } + var st unix.Stat_t + if err := unix.Fstat(next, &st); err != nil { + closePinFd(next) + closePinFd(cur) + return -1, fmt.Errorf("stat %s during bind resolution: %w", name, err) + } + if st.Mode&unix.S_IFMT != unix.S_IFLNK { + closePinFd(cur) + cur = next + continue + } + + // Symlink: expand it in place rather than following it by name. + links++ + if links > maxPinSymlinks { + closePinFd(next) + closePinFd(cur) + return -1, unix.ELOOP + } + target, err := readlinkFd(next) + closePinFd(next) + if err != nil { + closePinFd(cur) + 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. + closePinFd(cur) + if cur, err = unix.Dup(rootFd); err != nil { + return -1, fmt.Errorf("dup bind root: %w", err) + } + } + remaining = append(strings.Split(target, "/"), remaining...) + } + return cur, 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..83890293 --- /dev/null +++ b/pkg/dind/bindpin_other.go @@ -0,0 +1,124 @@ +//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) {} + +// 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..06b92cbb --- /dev/null +++ b/pkg/dind/bindstage_linux.go @@ -0,0 +1,324 @@ +//go:build linux + +package dind + +import ( + "bufio" + "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 sync.Mutex + ready 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() + if err := m.ensureDirLocked(); err != nil { + m.mu.Unlock() + return "", err + } + m.seq++ + target := filepath.Join(m.dir, strconv.FormatUint(m.seq, 10)) + m.mu.Unlock() + + // 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 the source cannot be swapped between validation and mount; "+ + "this requires ephemerd to run as root with CAP_SYS_ADMIN on a writable data directory)", + p.logical, target, err, m.dir) + } + + p.staged = target + p.unstage = func() error { return unmountAndRemove(target) } + return target, nil +} + +// 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. + for _, d := range []string{stagingRootParent(m.dir), m.dir} { + 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. +func (m *mountStager) teardown() { + m.mu.Lock() + ready := m.ready + m.ready = false + m.mu.Unlock() + 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 { + mounts, err := mountPointsUnder(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 { + detachMount(mp) + } + remaining, err := mountPointsUnder(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) + } + defer 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..a5719258 --- /dev/null +++ b/pkg/dind/bindstage_linux_test.go @@ -0,0 +1,470 @@ +//go:build linux + +package dind + +import ( + "context" + "io" + "log/slog" + "net" + "os" + "path/filepath" + "strings" + "testing" + + "golang.org/x/sys/unix" +) + +// requireMountPrivilege skips a test that needs mount(2). The project's Linux +// CI runner is unprivileged; these tests are meant to be run on a Linux node +// (or WSL) as root, and skipping loudly is better than a green run that proved +// nothing — which is exactly how the previous attempt at #125 passed review. +func requireMountPrivilege(t *testing.T) { + t.Helper() + if os.Geteuid() != 0 { + t.Skip("bind staging needs CAP_SYS_ADMIN (mount(2)); run as root to exercise it") + } +} + +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) { + requireMountPrivilege(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 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) { + requireMountPrivilege(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 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_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) { + requireMountPrivilege(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) { + requireMountPrivilege(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) { + requireMountPrivilege(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) { + requireMountPrivilege(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 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_runc_linux_test.go b/pkg/dind/bindstage_runc_linux_test.go new file mode 100644 index 00000000..c118c389 --- /dev/null +++ b/pkg/dind/bindstage_runc_linux_test.go @@ -0,0 +1,255 @@ +//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) + requireMountPrivilege(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 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) + requireMountPrivilege(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 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..8eead0eb --- /dev/null +++ b/pkg/dind/bindstage_testutil_test.go @@ -0,0 +1,30 @@ +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: +// +// - TestMountStager_* in bindstage_linux_test.go (Linux, root-gated), which +// performs the real bind and proves 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..61427339 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,81 @@ 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) + } + 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) + // 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) } - 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) - } - // 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) + } + mode := pin.Mode() + if !mode.IsDir() && !mode.IsRegular() { + _ = pin.Close() + return bindResolution{}, fmt.Errorf("bind source %q resolves to something that is not a regular file or directory (mode %s)", src, mode) } + 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,123 +146,14 @@ 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. -// -// 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 { - 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) -} - // matchBindPrefix returns the host source for the longest runnerBinds key // that contains src, along with the leftover suffix within that bind. // Longest-prefix wins so a child mount (e.g. /etc/hosts) is preferred over diff --git a/pkg/dind/bindtranslate_e2e_test.go b/pkg/dind/bindtranslate_e2e_test.go index d999003b..64886759 100644 --- a/pkg/dind/bindtranslate_e2e_test.go +++ b/pkg/dind/bindtranslate_e2e_test.go @@ -197,7 +197,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 +228,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 { @@ -303,7 +321,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..83b81e36 100644 --- a/pkg/dind/bindtranslate_test.go +++ b/pkg/dind/bindtranslate_test.go @@ -34,6 +34,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 +56,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 +80,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 +101,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 +113,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 +154,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 +203,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 +242,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 +279,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 +288,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 +317,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 +400,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 +467,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 +482,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 fc6570d7..6a0e9d24 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, @@ -298,6 +307,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. @@ -475,7 +497,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 @@ -635,6 +658,7 @@ func (s *Server) handleContainerCreate(w http.ResponseWriter, r *http.Request) { Tty: req.Tty, HostsPath: hostsPath, ExtraHosts: extraHosts, + BindPins: bindPins, started: make(chan struct{}), } @@ -642,6 +666,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) @@ -1376,6 +1403,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. @@ -1429,7 +1463,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) @@ -1440,6 +1480,11 @@ func (s *Server) buildBindMounts(ctx context.Context, binds []string) ([]oci.Spe s.mu.Unlock() 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 { @@ -1449,20 +1494,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 a3af56cc..b1d03cee 100644 --- a/pkg/dind/dind.go +++ b/pkg/dind/dind.go @@ -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 @@ -230,6 +237,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), @@ -418,6 +426,14 @@ func (s *Server) Stop() { 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 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 { From 37e7eccc4f2e37562fcd4c480d7598ef338ebd2f Mon Sep 17 00:00:00 2001 From: Luther Monson Date: Tue, 18 Aug 2026 18:53:44 -0700 Subject: [PATCH 2/4] fix(dind): close the Stop() teardown race that could empty a runner rootfs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #173. The staging fix was sound but its teardown was not. BLOCKER. Server.Stop() tore down containers and the bind stager while the HTTP listener was still accepting. An in-flight handleContainerCreate could therefore call stage() concurrently with teardown(), and stage() released the stager lock before its unix.Mount. unmountTreeAndRemove checks that no mounts remain and then calls os.RemoveAll, so a mount landing between those two steps put RemoveAll on top of a live bind mount. That is not merely untidy. Measured, not assumed: source before: [precious precious2] RemoveAll err: unlinkat .../job/staged: device or resource busy source after: [] os.RemoveAll walks INTO the mount, deletes the files visible through it, and only then reports EBUSY on the mountpoint. In production those files are the runner's own rootfs. Reachable from a job: sibling containers are not in the runner's cgroup, so they outlive the task the runtime kills before Stop, and on the TCP transport they still hold DOCKER_HOST. Second defect in the same area: teardown cleared `ready` before unmounting, so a racing stage() would silently recreate the staging directory the caller had just swept, leaking a mount that pins the rootfs until the next startup. Three changes, any one of which closes it; all three because this one destroys data: - Stop() shuts the HTTP server and listener down FIRST, before touching any state an in-flight request can still be creating. - stage() holds the stager lock across the mkdir and the mount, and teardown holds it across the unmount-and-remove, so the two are mutually exclusive. Pin release takes the same lock for the same reason. - teardown sets a `closed` flag that is never cleared: staging after teardown now fails instead of recreating the directory. Covered by TestBindStaging_StageAfterTeardownIsRefused and TestUnmountTreeAndRemove_RefusesWhileMounted, the latter measuring the RemoveAll hazard in one subtest so the guard's existence is justified by evidence rather than by comment. The fallback walk was not equivalent, and said it was. resolveBeneathWalk refused every "..", including ones spliced in from a SYMLINK TARGET, while RESOLVE_IN_ROOT resolves those clamped at the root. Relative "../" targets are ordinary in real images (/etc/alternatives/*, Debian multiarch), so any node that ever fell back would have started 400ing legitimate binds while the doc claimed parity. It now holds every directory on the way down open and steps ".." back to a descriptor it already has — never re-opening a parent by name, which would be the second walk this whole mechanism removes. TestResolveBeneathWalk_MatchesOpenat2 asserts both resolvers land on the same inode across six shapes, including "../usr/bin/tool" and a "../" chain that tries to climb past the root. The openat2 latch was silent, permanent and node-wide. One spurious error could convert a node's resolver for the rest of the daemon's uptime with nothing in the log. Now only ENOSYS and E2BIG latch (the kernel genuinely lacks the syscall); EPERM — a seccomp filter — falls back for that call alone; and the first bind afterwards logs a WARN naming the reason. The EINVAL justification was also simply wrong: RESOLVE_IN_ROOT clamps an escape rather than returning EINVAL, and EXDEV is RESOLVE_BENEATH's error. Right call, wrong reason, now corrected in place so the next reader does not inherit it. Also from review: - The runnerBinds-suffix branch — the one that previously had NO containment check and reaches the job-writable runner directory — had no staging test. TestBindStaging_RunnerBindSuffixIsStaged now covers it: staged, survives the swap, symlink escape refused. - ensureDirLocked chmodded before checking for a symlink, so a planted /dind-binds symlink got its target chmodded 0700 on the way to being refused. Checked first now. - The runnerBinds-suffix branch skipped the non-dir/non-regular rejection the rootfs branch applies. Both go through rejectUnbindableType. - gofmt: three files (two of the three predate this branch). - Doc: "every component is root-owned and 0700" overstated ensureTrustedAncestry, which also accepts euid-owned and sticky world-writable; corrected, with an operator note that a --data-dir under a group-writable path now hard-fails every bind. Re-verified after the changes, Linux/WSL2 kernel 6.18, euid 0, runc 1.3.4: control CONTAINER-SAW: SWAPPED, staged CONTAINER-SAW: PINNED, proc-fd source still EINVAL, whole pkg/dind and pkg/runtime green, gofmt and vet clean, and zero dind-binds entries left in /proc/self/mountinfo. Neutering stage() to return the resolved path now fails 5 tests including the real-runc ESCAPE assertion. --- docs/arch/dind-bind-translation.md | 40 +++- pkg/dind/bindpin_linux.go | 155 +++++++++++---- pkg/dind/bindpin_other.go | 4 + pkg/dind/bindstage_linux.go | 76 ++++++-- pkg/dind/bindstage_linux_test.go | 280 +++++++++++++++++++++++++++- pkg/dind/bindstage_testutil_test.go | 7 +- pkg/dind/bindtranslate.go | 32 +++- pkg/dind/bindtranslate_test.go | 1 - pkg/dind/containers.go | 10 + pkg/dind/dind.go | 42 +++-- 10 files changed, 569 insertions(+), 78 deletions(-) diff --git a/docs/arch/dind-bind-translation.md b/docs/arch/dind-bind-translation.md index a38b1361..17f553cc 100644 --- a/docs/arch/dind-bind-translation.md +++ b/docs/arch/dind-bind-translation.md @@ -164,8 +164,22 @@ 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 equivalent `O_PATH|O_NOFOLLOW` -component-by-component walk. +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 @@ -203,10 +217,16 @@ mount("/proc/self/fd/N" -> /dind-binds//) (same ns: works) spec.Source = /dind-binds// ``` -runc re-walks the staging path, which is fine: every component is root-owned -and 0700, and nothing in the name is derived from the job's request (the leaf -is a bare counter). `ensureTrustedAncestry` checks that precondition on first -use and fails the bind rather than assuming it. Sources with no job-supplied +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. @@ -231,6 +251,14 @@ silently. Now a source that cannot be staged fails the `docker create` with a 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 diff --git a/pkg/dind/bindpin_linux.go b/pkg/dind/bindpin_linux.go index a5ff91fb..1feee405 100644 --- a/pkg/dind/bindpin_linux.go +++ b/pkg/dind/bindpin_linux.go @@ -21,11 +21,43 @@ var errBindPathTraversal = errors.New(`bind source contains a ".." component, wh // Matches the kernel's own MAXSYMLINKS. const maxPinSymlinks = 40 -// openat2Unsupported latches once the kernel is known not to support openat2, -// so the fallback does not pay for a failing syscall on every bind. Atomic +// 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) } @@ -159,8 +191,17 @@ func resolveBeneath(rootFd int, comps []string) (int, error) { if err == nil { return fd, nil } - if isOpenat2Unavailable(err) { + 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 @@ -170,23 +211,35 @@ func resolveBeneath(rootFd int, comps []string) (int, error) { // real call; after that the answer is latched. func openat2Known() bool { return !openat2Unsupported.Load() } -// isOpenat2Unavailable distinguishes "this kernel/sandbox has no openat2" from -// a genuine resolution failure. ENOSYS is a pre-5.6 kernel; EPERM is a seccomp -// filter that does not know the syscall; E2BIG is an OpenHow the kernel does -// not understand. -// -// EACCES and EINVAL are deliberately NOT in this list even though the syscall -// can return them for an unsupported flag set: both are also ordinary -// resolution outcomes (EACCES on a directory we may not search, EINVAL from -// RESOLVE_IN_ROOT rejecting an escape on some kernels), and treating a real -// rejection as "kernel too old" would silently downgrade every subsequent bind -// on the node to the fallback walk. -func isOpenat2Unavailable(err error) bool { - return errors.Is(err, unix.ENOSYS) || - errors.Is(err, unix.EPERM) || - errors.Is(err, unix.E2BIG) +// 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. // @@ -197,13 +250,37 @@ func isOpenat2Unavailable(err error) bool { // 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) { - cur, err := unix.Dup(rootFd) + 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 @@ -214,56 +291,62 @@ func resolveBeneathWalk(rootFd int, comps []string) (int, error) { case "", ".": continue case "..": - // RESOLVE_IN_ROOT makes ".." at the root a no-op; anywhere else - // it would need parent tracking. Refusing is stricter and, for - // a lexically cleaned bind source, unreachable except through a - // symlink target. - closePinFd(cur) - return -1, errBindPathTraversal + 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 { - closePinFd(cur) + closeStack() return -1, err } var st unix.Stat_t if err := unix.Fstat(next, &st); err != nil { closePinFd(next) - closePinFd(cur) + closeStack() return -1, fmt.Errorf("stat %s during bind resolution: %w", name, err) } if st.Mode&unix.S_IFMT != unix.S_IFLNK { - closePinFd(cur) - cur = next + stack = append(stack, next) continue } - // Symlink: expand it in place rather than following it by name. + // 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) - closePinFd(cur) + closeStack() return -1, unix.ELOOP } target, err := readlinkFd(next) closePinFd(next) if err != nil { - closePinFd(cur) + 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. - closePinFd(cur) - if cur, err = unix.Dup(rootFd); err != nil { - return -1, fmt.Errorf("dup bind root: %w", err) + for _, fd := range stack[1:] { + closePinFd(fd) } + stack = stack[:1] } remaining = append(strings.Split(target, "/"), remaining...) } - return cur, nil + + 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 diff --git a/pkg/dind/bindpin_other.go b/pkg/dind/bindpin_other.go index 83890293..2cc2d5f4 100644 --- a/pkg/dind/bindpin_other.go +++ b/pkg/dind/bindpin_other.go @@ -18,6 +18,10 @@ var errBindPathTraversal = errors.New(`bind source contains a ".." component, wh // 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. // diff --git a/pkg/dind/bindstage_linux.go b/pkg/dind/bindstage_linux.go index 06b92cbb..70d6b909 100644 --- a/pkg/dind/bindstage_linux.go +++ b/pkg/dind/bindstage_linux.go @@ -31,9 +31,22 @@ type mountStager struct { dir string log *slog.Logger - mu sync.Mutex - ready bool - seq uint64 + // 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. @@ -47,13 +60,16 @@ func (m *mountStager) stage(p *bindPin) (string, error) { } 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 { - m.mu.Unlock() return "", err } m.seq++ target := filepath.Join(m.dir, strconv.FormatUint(m.seq, 10)) - m.mu.Unlock() // The mountpoint has to match the source's type: a directory for a // directory, an empty regular file for anything else (bind mounts of @@ -85,7 +101,13 @@ func (m *mountStager) stage(p *bindPin) (string, error) { } p.staged = target - p.unstage = func() error { return unmountAndRemove(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 } @@ -105,7 +127,21 @@ func (m *mountStager) ensureDirLocked() error { } // 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) } @@ -129,12 +165,21 @@ func (m *mountStager) ensureDirLocked() error { return nil } -// teardown removes every mount and directory this job staged. Idempotent. +// 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.mu.Unlock() + 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. @@ -189,7 +234,16 @@ func sweepStagedBinds(root string, log *slog.Logger) { // 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 { - mounts, err := mountPointsUnder(dir) + 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 } @@ -198,9 +252,9 @@ func unmountTreeAndRemove(dir string) error { // children). sort.Slice(mounts, func(i, j int) bool { return len(mounts[i]) > len(mounts[j]) }) for _, mp := range mounts { - detachMount(mp) + detach(mp) } - remaining, err := mountPointsUnder(dir) + remaining, err := list(dir) if err != nil { return err } diff --git a/pkg/dind/bindstage_linux_test.go b/pkg/dind/bindstage_linux_test.go index a5719258..0982bdd3 100644 --- a/pkg/dind/bindstage_linux_test.go +++ b/pkg/dind/bindstage_linux_test.go @@ -292,6 +292,276 @@ func TestBindStaging_LegitimateBindsStillWork(t *testing.T) { } } +// 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) { + requireMountPrivilege(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) { + requireMountPrivilege(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 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 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) { + requireMountPrivilege(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 @@ -418,11 +688,11 @@ func TestEnsureTrustedAncestry_RejectsJobWritableParent(t *testing.T) { // 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`, + `/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 { diff --git a/pkg/dind/bindstage_testutil_test.go b/pkg/dind/bindstage_testutil_test.go index 8eead0eb..cfeec35a 100644 --- a/pkg/dind/bindstage_testutil_test.go +++ b/pkg/dind/bindstage_testutil_test.go @@ -14,9 +14,10 @@ package dind // The security property staging provides is therefore NOT covered by these // tests, by construction. It is covered by: // -// - TestMountStager_* in bindstage_linux_test.go (Linux, root-gated), which -// performs the real bind and proves the staged content survives an -// adversarial swap of the original path; +// - 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. diff --git a/pkg/dind/bindtranslate.go b/pkg/dind/bindtranslate.go index 61427339..deff415b 100644 --- a/pkg/dind/bindtranslate.go +++ b/pkg/dind/bindtranslate.go @@ -110,6 +110,9 @@ func translateBindSource(src string, runnerBinds map[string]string, runnerRootfs 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 } @@ -128,10 +131,8 @@ func translateBindSource(src string, runnerBinds map[string]string, runnerRootfs } return bindResolution{}, fmt.Errorf("bind source %q rejected: %w", src, err) } - mode := pin.Mode() - if !mode.IsDir() && !mode.IsRegular() { - _ = pin.Close() - return bindResolution{}, fmt.Errorf("bind source %q resolves to something that is not a regular file or directory (mode %s)", src, mode) + if err := rejectUnbindableType(src, pin); err != nil { + return bindResolution{}, err } return bindResolution{ResolvedPath: pin.Logical(), Pin: pin}, nil } @@ -154,6 +155,29 @@ func translateBindSource(src string, runnerBinds map[string]string, runnerRootfs return bindResolution{}, fmt.Errorf("bind source %q is not visible to ephemerd dind (not in runner rootfs or known bind table)", src) } +// rejectUnbindableType refuses a pinned source that is neither a directory nor +// a regular file, closing the pin on the way out. +// +// 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 + } + _ = 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 // that contains src, along with the leftover suffix within that bind. // Longest-prefix wins so a child mount (e.g. /etc/hosts) is preferred over diff --git a/pkg/dind/bindtranslate_test.go b/pkg/dind/bindtranslate_test.go index 83b81e36..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. diff --git a/pkg/dind/containers.go b/pkg/dind/containers.go index 6a0e9d24..5e1b6e3c 100644 --- a/pkg/dind/containers.go +++ b/pkg/dind/containers.go @@ -1479,6 +1479,16 @@ 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) { diff --git a/pkg/dind/dind.go b/pkg/dind/dind.go index b1d03cee..f0cefcc5 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 @@ -422,6 +422,34 @@ 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() @@ -458,16 +486,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 From cf52b6b98c00c9b06e0361392f30689a1987df5d Mon Sep 17 00:00:00 2001 From: Luther Monson Date: Tue, 18 Aug 2026 19:05:33 -0700 Subject: [PATCH 3/4] fix(lint): check the Close errors errcheck flagged CI caught eight unchecked Close() calls that could not surface locally - the pinned golangci-lint panics on Go 1.26, so errcheck only ever runs on the Linux CI job. The production one (mountPointsUnder) reads /proc/self/mountinfo, so a Close error carries nothing a caller could act on; it is now explicitly discarded rather than silently dropped, which is the distinction errcheck is actually making. The seven in tests are the same shape. --- pkg/dind/bindstage_linux.go | 4 +- pkg/dind/bindstage_linux_test.go | 53 +++++++++++++-------------- pkg/dind/bindstage_runc_linux_test.go | 13 +++---- 3 files changed, 35 insertions(+), 35 deletions(-) diff --git a/pkg/dind/bindstage_linux.go b/pkg/dind/bindstage_linux.go index 70d6b909..f97c8343 100644 --- a/pkg/dind/bindstage_linux.go +++ b/pkg/dind/bindstage_linux.go @@ -302,7 +302,9 @@ func mountPointsUnder(dir string) ([]string, error) { if err != nil { return nil, fmt.Errorf("reading mount table: %w", err) } - defer f.Close() + // 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 diff --git a/pkg/dind/bindstage_linux_test.go b/pkg/dind/bindstage_linux_test.go index 0982bdd3..f07ee3eb 100644 --- a/pkg/dind/bindstage_linux_test.go +++ b/pkg/dind/bindstage_linux_test.go @@ -1,4 +1,4 @@ -//go:build linux +//go:build linux package dind @@ -18,7 +18,7 @@ import ( // requireMountPrivilege skips a test that needs mount(2). The project's Linux // CI runner is unprivileged; these tests are meant to be run on a Linux node // (or WSL) as root, and skipping loudly is better than a green run that proved -// nothing — which is exactly how the previous attempt at #125 passed review. +// nothing — which is exactly how the previous attempt at #125 passed review. func requireMountPrivilege(t *testing.T) { t.Helper() if os.Geteuid() != 0 { @@ -33,8 +33,8 @@ func discardLog() *slog.Logger { // 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, +// 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 { @@ -50,7 +50,7 @@ func newTestStager(t *testing.T, dataDir, jobID string) *mountStager { // /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. +// 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"} { @@ -101,8 +101,7 @@ func TestBindStaging_SwapAfterValidationDoesNotLeak(t *testing.T) { if err != nil { t.Fatalf("pinning a contained bind source must succeed: %v", err) } - defer pin.Close() - + defer func() { _ = pin.Close() }() staged, err := stager.stage(pin) if err != nil { t.Fatalf("staging: %v", err) @@ -111,29 +110,29 @@ func TestBindStaging_SwapAfterValidationDoesNotLeak(t *testing.T) { // 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 + // 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) + 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 + // 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) + 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 +// 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) { @@ -150,7 +149,7 @@ func TestBindStaging_UnstagedPathLeaks(t *testing.T) { 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) + 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) } } @@ -186,7 +185,7 @@ func TestBindStaging_LegitimateBindsStillWork(t *testing.T) { } // The ephemerd-owned, non-rootfs binds: a socket and two files. These are - // the passthrough category — no pin, no staging — and they must keep + // 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 { @@ -196,7 +195,7 @@ func TestBindStaging_LegitimateBindsStillWork(t *testing.T) { if err != nil { t.Fatalf("creating a real unix socket for the docker.sock bind: %v", err) } - defer l.Close() + defer func() { _ = l.Close() }() hosts := filepath.Join(root, "hosts") resolv := filepath.Join(root, "resolv.conf") for _, p := range []string{hosts, resolv} { @@ -266,7 +265,7 @@ func TestBindStaging_LegitimateBindsStillWork(t *testing.T) { 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) + 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 { @@ -342,7 +341,7 @@ func TestBindStaging_RunnerBindSuffixIsStaged(t *testing.T) { 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) + 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 @@ -369,7 +368,7 @@ func TestBindStaging_RunnerBindSuffixIsStaged(t *testing.T) { // 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 +// 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) { @@ -386,7 +385,7 @@ func TestBindStaging_StageAfterTeardownIsRefused(t *testing.T) { if err != nil { t.Fatal(err) } - defer pin.Close() + defer func() { _ = pin.Close() }() if _, err := stager.stage(pin); err != nil { t.Fatalf("staging before teardown: %v", err) } @@ -397,7 +396,7 @@ func TestBindStaging_StageAfterTeardownIsRefused(t *testing.T) { if err != nil { t.Fatal(err) } - defer late.Close() + 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") } @@ -419,7 +418,7 @@ func TestBindStaging_StageAfterTeardownIsRefused(t *testing.T) { // 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 +// 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) { @@ -451,7 +450,7 @@ func TestUnmountTreeAndRemove_RefusesWhileMounted(t *testing.T) { 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.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) { @@ -484,7 +483,7 @@ func TestUnmountTreeAndRemove_RefusesWhileMounted(t *testing.T) { 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) + 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) @@ -496,7 +495,7 @@ func TestUnmountTreeAndRemove_RefusesWhileMounted(t *testing.T) { // 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 +// 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 @@ -589,7 +588,7 @@ func TestBindStaging_TeardownUnmountsAndRemoves(t *testing.T) { t.Fatal(err) } if len(mounts) == 0 { - t.Fatalf("no mount at %s after staging — staging did not actually bind anything", staged) + t.Fatalf("no mount at %s after staging — staging did not actually bind anything", staged) } stager.teardown() @@ -703,7 +702,7 @@ func TestUnescapeMountPath(t *testing.T) { // 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 +// 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) { requireMountPrivilege(t) @@ -722,7 +721,7 @@ func TestBindStaging_MountpointTypeMatchesSource(t *testing.T) { if err != nil { t.Fatal(err) } - defer pin.Close() + defer func() { _ = pin.Close() }() staged, err := stager.stage(pin) if err != nil { t.Fatalf("staging a regular file: %v", err) diff --git a/pkg/dind/bindstage_runc_linux_test.go b/pkg/dind/bindstage_runc_linux_test.go index c118c389..4d93523b 100644 --- a/pkg/dind/bindstage_runc_linux_test.go +++ b/pkg/dind/bindstage_runc_linux_test.go @@ -1,4 +1,4 @@ -//go:build linux +//go:build linux package dind @@ -190,7 +190,7 @@ func TestBindStaging_RealRunc_SwapDoesNotLeak(t *testing.T) { if err != nil { t.Fatalf("pinning a contained source: %v", err) } - defer pin.Close() + defer func() { _ = pin.Close() }() staged, err := stager.stage(pin) if err != nil { t.Fatalf("staging: %v", err) @@ -205,7 +205,7 @@ func TestBindStaging_RealRunc_SwapDoesNotLeak(t *testing.T) { 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.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) }) @@ -215,7 +215,7 @@ func TestBindStaging_RealRunc_SwapDoesNotLeak(t *testing.T) { // 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 +// 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 @@ -238,15 +238,14 @@ func TestBindStaging_RealRunc_ProcFdSourceIsRejected(t *testing.T) { if err != nil { t.Fatal(err) } - defer pin.Close() - + 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) + 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) From 7b45d85faa4eee99d78ae637a1d65bc2f4324b05 Mon Sep 17 00:00:00 2001 From: Luther Monson Date: Tue, 18 Aug 2026 19:20:20 -0700 Subject: [PATCH 4/4] fix(test): gate bind-translation e2e on a real mount(2) probe, not on luck MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI went red on TestBindTranslation_RealContainerd. The test is pre-existing; what changed under it is that bind translation now stages every job-supplied source as a real bind mount, so a test that never needed mount(2) suddenly does, and ephemerd's CI runner is an unprivileged container. Not fixed by degrading staging. A fallback to the resolved path when unprivileged would put a job-controlled path back in the OCI spec — issue #125 reopened — and every test would go green while the escape was live. That is the exact shape of the previous broken fix. Failing closed is correct; the test has to say so instead. Verified the premise rather than assuming it: root with CAP_SYS_ADMIN is a hard, universal prerequisite for ephemerd on Linux, so the production hard-fail costs nothing real. The shipped systemd unit sets no User= and no capability bounding set (cmd/ephemerd/install_linux.go), `ephemerd doctor` FAILS rather than warns when euid != 0 with "ephemerd requires root for container management" (cmd/ephemerd/doctor_linux.go), the in-process containerd already needs mount(2) for the overlayfs snapshotter, networking manages iptables chains and a CNI bridge, and there is no rootless mode anywhere in the tree or the docs. Every documented invocation is sudo. Gating now reuses the house pattern instead of a new one. pull_e2e_test.go already solved this: probeMountPrivilege performs the actual bind mount and reports the error, because "capabilities, user namespaces, LSM policy and read-only mount propagation all decide this independently of the uid, and ephemerd's own runners are containers where they disagree". requireBindStaging wraps it, and EPHEMERD_TEST_REQUIRE_MOUNT=1 turns the skip into a failure where the coverage must be guaranteed. This replaces the os.Geteuid() == 0 check the staging tests shipped with — which was wrong for the same reason: under `capsh --drop=cap_sys_admin` the euid is still 0 and every one of those tests would have tried to mount and failed. The lost coverage is made legible, not implied. TestBindTranslation_ RealContainerd is added to the "Privileged-test coverage report" step in ci.yml so its SKIP line is printed; `go test ./...` buffers package output and prints it only on failure, so on a green run a skip reaches nobody. The step's comment says why it is in the list and why making staging degrade is not the alternative. Audited the other four tests that reach buildBindMounts rather than guessing which else might be affected. Three use unstagedTestStager and never stage by design. The fourth, TestBindTranslation_RejectsForeignSource, uses the real stager but is rejected in translateBindSource before anything is pinned, so it needs no privilege — deliberately left ungated, with a comment saying that is a property to preserve and to re-gate it if the rejection ever moves after staging. Nothing in test/e2e passes Binds at all, so none of it is affected. Also, since the errno is the operator's only clue: stagingMountAdvice splits the mount failure into "this is about PRIVILEGE" (EPERM, pointing at `ephemerd doctor`) and "this is about the DATA DIRECTORY" (EROFS, ENOSPC, EACCES, each naming the specific problem), instead of one sentence listing both and making the reader guess. Repaired encoding damage in bindstage_linux_test.go and bindstage_runc_linux_test.go: both had picked up a UTF-8 BOM and 27 em-dashes double-encoded to "—". The BOM did not break the //go:build linux constraint (verified via go list — both files stay in IgnoredGoFiles on Windows), so this was hygiene rather than breakage, but they are new files and should not enter history corrupted. Verified on Linux/WSL2 kernel 6.18 both ways: CAP_SYS_ADMIN dropped (capsh, euid still 0 — the CI condition): whole pkg/dind: 188 pass, 14 skip, 0 fail TestBindTranslation_RealContainerd SKIP (was FAIL) TestBindTranslation_RejectsForeignSource PASS (never reaches the stager) TestBindStaging_UnstagedPathLeaks + TestResolveBeneathWalk_MatchesOpenat2 still PASS — the unprivileged tier keeps real coverage EPHEMERD_TEST_REQUIRE_MOUNT=1 correctly turns the skip into a failure Privileged, EPHEMERD_TEST_REQUIRE_MOUNT=1 so nothing may skip: pkg/dind and pkg/runtime ok, control CONTAINER-SAW: SWAPPED, staged CONTAINER-SAW: PINNED, proc-fd source still EINVAL Windows pkg/dind and pkg/runtime ok. gofmt, go build ./... and go vet ./... clean; no BOM or mojibake left; zero dind-binds entries in /proc/self/mountinfo afterwards. --- .github/workflows/ci.yml | 13 ++++- pkg/dind/bindstage_linux.go | 37 ++++++++++++-- pkg/dind/bindstage_linux_test.go | 69 +++++++++++---------------- pkg/dind/bindstage_probe_test.go | 55 +++++++++++++++++++++ pkg/dind/bindstage_runc_linux_test.go | 12 ++--- pkg/dind/bindtranslate_e2e_test.go | 14 ++++++ 6 files changed, 148 insertions(+), 52 deletions(-) create mode 100644 pkg/dind/bindstage_probe_test.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 0d905a38..e50f63de 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -49,10 +49,21 @@ jobs: # 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|TestSweepStagedBinds|TestEnsureTrustedAncestry' \ + -run 'TestBindStaging|TestBindTranslation|TestSweepStagedBinds|TestEnsureTrustedAncestry|TestUnmountTreeAndRemove|TestResolveBeneathWalk' \ -v ./pkg/dind/ - name: E2E — GitHub provider (fake server) diff --git a/pkg/dind/bindstage_linux.go b/pkg/dind/bindstage_linux.go index f97c8343..81475f4b 100644 --- a/pkg/dind/bindstage_linux.go +++ b/pkg/dind/bindstage_linux.go @@ -4,6 +4,7 @@ package dind import ( "bufio" + "errors" "fmt" "log/slog" "os" @@ -93,11 +94,10 @@ func (m *mountStager) stage(p *bindPin) (string, error) { 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 the source cannot be swapped between validation and mount; "+ - "this requires ephemerd to run as root with CAP_SYS_ADMIN on a writable data directory)", - p.logical, target, err, m.dir) + 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 @@ -111,6 +111,33 @@ func (m *mountStager) stage(p *bindPin) (string, error) { 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. // diff --git a/pkg/dind/bindstage_linux_test.go b/pkg/dind/bindstage_linux_test.go index f07ee3eb..7657b374 100644 --- a/pkg/dind/bindstage_linux_test.go +++ b/pkg/dind/bindstage_linux_test.go @@ -1,4 +1,4 @@ -//go:build linux +//go:build linux package dind @@ -15,17 +15,6 @@ import ( "golang.org/x/sys/unix" ) -// requireMountPrivilege skips a test that needs mount(2). The project's Linux -// CI runner is unprivileged; these tests are meant to be run on a Linux node -// (or WSL) as root, and skipping loudly is better than a green run that proved -// nothing — which is exactly how the previous attempt at #125 passed review. -func requireMountPrivilege(t *testing.T) { - t.Helper() - if os.Geteuid() != 0 { - t.Skip("bind staging needs CAP_SYS_ADMIN (mount(2)); run as root to exercise it") - } -} - func discardLog() *slog.Logger { return slog.New(slog.NewTextHandler(io.Discard, nil)) } @@ -33,8 +22,8 @@ func discardLog() *slog.Logger { // 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, +// 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 { @@ -50,7 +39,7 @@ func newTestStager(t *testing.T, dataDir, jobID string) *mountStager { // /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. +// 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"} { @@ -85,7 +74,7 @@ func swapVictim(t *testing.T, dir string) { // 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) { - requireMountPrivilege(t) + requireBindStaging(t) root := t.TempDir() rootfs := filepath.Join(root, "rootfs") @@ -110,29 +99,29 @@ func TestBindStaging_SwapAfterValidationDoesNotLeak(t *testing.T) { // 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 + // 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) + 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 + // 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) + 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 +// 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) { @@ -149,7 +138,7 @@ func TestBindStaging_UnstagedPathLeaks(t *testing.T) { 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) + 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) } } @@ -158,7 +147,7 @@ func TestBindStaging_UnstagedPathLeaks(t *testing.T) { // ones included. This drives buildBindMounts with the real stager over the // shapes production actually uses. func TestBindStaging_LegitimateBindsStillWork(t *testing.T) { - requireMountPrivilege(t) + requireBindStaging(t) root := t.TempDir() rootfs := filepath.Join(root, "rootfs") @@ -185,7 +174,7 @@ func TestBindStaging_LegitimateBindsStillWork(t *testing.T) { } // The ephemerd-owned, non-rootfs binds: a socket and two files. These are - // the passthrough category — no pin, no staging — and they must keep + // 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 { @@ -265,7 +254,7 @@ func TestBindStaging_LegitimateBindsStillWork(t *testing.T) { 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) + 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 { @@ -302,7 +291,7 @@ func TestBindStaging_LegitimateBindsStillWork(t *testing.T) { // branch, which left this one asserted only by the translation-policy tests // that do not stage at all. func TestBindStaging_RunnerBindSuffixIsStaged(t *testing.T) { - requireMountPrivilege(t) + requireBindStaging(t) root := t.TempDir() // The per-job runner directory, as it appears in runnerBindMappings. @@ -341,7 +330,7 @@ func TestBindStaging_RunnerBindSuffixIsStaged(t *testing.T) { 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) + 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 @@ -368,11 +357,11 @@ func TestBindStaging_RunnerBindSuffixIsStaged(t *testing.T) { // 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 +// 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) { - requireMountPrivilege(t) + requireBindStaging(t) root := t.TempDir() rootfs := filepath.Join(root, "rootfs") @@ -418,11 +407,11 @@ func TestBindStaging_StageAfterTeardownIsRefused(t *testing.T) { // 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 +// 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) { - requireMountPrivilege(t) + requireBindStaging(t) t.Run("removeall_deletes_through_a_live_mount", func(t *testing.T) { base := t.TempDir() @@ -450,7 +439,7 @@ func TestUnmountTreeAndRemove_RefusesWhileMounted(t *testing.T) { 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.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) { @@ -483,7 +472,7 @@ func TestUnmountTreeAndRemove_RefusesWhileMounted(t *testing.T) { 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) + 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) @@ -495,7 +484,7 @@ func TestUnmountTreeAndRemove_RefusesWhileMounted(t *testing.T) { // 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 +// 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 @@ -566,7 +555,7 @@ func TestResolveBeneathWalk_MatchesOpenat2(t *testing.T) { // 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) { - requireMountPrivilege(t) + requireBindStaging(t) root := t.TempDir() rootfs := filepath.Join(root, "rootfs") @@ -588,7 +577,7 @@ func TestBindStaging_TeardownUnmountsAndRemoves(t *testing.T) { t.Fatal(err) } if len(mounts) == 0 { - t.Fatalf("no mount at %s after staging — staging did not actually bind anything", staged) + t.Fatalf("no mount at %s after staging — staging did not actually bind anything", staged) } stager.teardown() @@ -612,7 +601,7 @@ func TestBindStaging_TeardownUnmountsAndRemoves(t *testing.T) { // 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) { - requireMountPrivilege(t) + requireBindStaging(t) root := t.TempDir() rootfs := filepath.Join(root, "rootfs") @@ -648,7 +637,7 @@ func TestSweepStagedBinds_RemovesLeakedMounts(t *testing.T) { // 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) { - requireMountPrivilege(t) + requireBindStaging(t) base := t.TempDir() loose := filepath.Join(base, "loose") @@ -702,10 +691,10 @@ func TestUnescapeMountPath(t *testing.T) { // 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 +// 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) { - requireMountPrivilege(t) + requireBindStaging(t) root := t.TempDir() rootfs := filepath.Join(root, "rootfs") 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 index 4d93523b..8d34461b 100644 --- a/pkg/dind/bindstage_runc_linux_test.go +++ b/pkg/dind/bindstage_runc_linux_test.go @@ -1,4 +1,4 @@ -//go:build linux +//go:build linux package dind @@ -153,7 +153,7 @@ func runBundle(t *testing.T, runcBin, bundle, id string) string { // proves it no longer does. func TestBindStaging_RealRunc_SwapDoesNotLeak(t *testing.T) { runcBin := requireRunc(t) - requireMountPrivilege(t) + requireBindStaging(t) work := t.TempDir() initBin := buildContainerInit(t, work) @@ -205,7 +205,7 @@ func TestBindStaging_RealRunc_SwapDoesNotLeak(t *testing.T) { 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.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) }) @@ -215,7 +215,7 @@ func TestBindStaging_RealRunc_SwapDoesNotLeak(t *testing.T) { // 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 +// 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 @@ -223,7 +223,7 @@ func TestBindStaging_RealRunc_SwapDoesNotLeak(t *testing.T) { // person from re-deriving it from a fleet outage. func TestBindStaging_RealRunc_ProcFdSourceIsRejected(t *testing.T) { runcBin := requireRunc(t) - requireMountPrivilege(t) + requireBindStaging(t) work := t.TempDir() initBin := buildContainerInit(t, work) @@ -245,7 +245,7 @@ func TestBindStaging_RealRunc_ProcFdSourceIsRejected(t *testing.T) { 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) + 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) diff --git a/pkg/dind/bindtranslate_e2e_test.go b/pkg/dind/bindtranslate_e2e_test.go index 64886759..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})) @@ -258,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")