diff --git a/go.mod b/go.mod index 6a042f9a83c..143b85ae8c8 100644 --- a/go.mod +++ b/go.mod @@ -6,7 +6,7 @@ require ( github.com/checkpoint-restore/go-criu/v7 v7.2.0 github.com/containerd/console v1.0.5 github.com/coreos/go-systemd/v22 v22.6.0 - github.com/cyphar/filepath-securejoin v0.6.0 + github.com/cyphar/filepath-securejoin v0.6.1 github.com/docker/go-units v0.5.0 github.com/godbus/dbus/v5 v5.1.0 github.com/moby/sys/capability v0.4.0 diff --git a/go.sum b/go.sum index e14f751576b..2ac91642631 100644 --- a/go.sum +++ b/go.sum @@ -11,8 +11,8 @@ github.com/coreos/go-systemd/v22 v22.6.0 h1:aGVa/v8B7hpb0TKl0MWoAavPDmHvobFe5R5z github.com/coreos/go-systemd/v22 v22.6.0/go.mod h1:iG+pp635Fo7ZmV/j14KUcmEyWF+0X7Lua8rrTWzYgWU= github.com/cpuguy83/go-md2man/v2 v2.0.7 h1:zbFlGlXEAKlwXpmvle3d8Oe3YnkKIK4xSRTd3sHPnBo= github.com/cpuguy83/go-md2man/v2 v2.0.7/go.mod h1:oOW0eioCTA6cOiMLiUPZOpcVxMig6NIQQ7OS05n1F4g= -github.com/cyphar/filepath-securejoin v0.6.0 h1:BtGB77njd6SVO6VztOHfPxKitJvd/VPT+OFBFMOi1Is= -github.com/cyphar/filepath-securejoin v0.6.0/go.mod h1:A8hd4EnAeyujCJRrICiOWqjS1AX0a9kM5XL+NwKoYSc= +github.com/cyphar/filepath-securejoin v0.6.1 h1:5CeZ1jPXEiYt3+Z6zqprSAgSWiggmpVyciv8syjIpVE= +github.com/cyphar/filepath-securejoin v0.6.1/go.mod h1:A8hd4EnAeyujCJRrICiOWqjS1AX0a9kM5XL+NwKoYSc= github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c= github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38= diff --git a/libcontainer/rootfs_linux.go b/libcontainer/rootfs_linux.go index 94b664eeb5a..9f2af0a212d 100644 --- a/libcontainer/rootfs_linux.go +++ b/libcontainer/rootfs_linux.go @@ -589,6 +589,12 @@ func (m *mountEntry) createOpenMountpoint(rootfs string) (Err error) { func mountToRootfs(c *mountConfig, m mountEntry) error { rootfs := c.root + defer func() { + if m.dstFile != nil { + _ = m.dstFile.Close() + m.dstFile = nil + } + }() // procfs and sysfs are special because we need to ensure they are actually // mounted on a specific path in a container without any funny business. @@ -629,12 +635,6 @@ func mountToRootfs(c *mountConfig, m mountEntry) error { if err := m.createOpenMountpoint(rootfs); err != nil { return fmt.Errorf("create mountpoint for %s mount: %w", m.Destination, err) } - defer func() { - if m.dstFile != nil { - _ = m.dstFile.Close() - m.dstFile = nil - } - }() switch m.Device { case "mqueue": @@ -995,6 +995,8 @@ func createDeviceNode(rootfs string, node *devices.Device, bind bool) error { if err != nil { return fmt.Errorf("mkdir parent of device inode %q: %w", node.Path, err) } + defer destDir.Close() + if bind { return bindMountDeviceNode(destDir, destName, node) } diff --git a/tests/integration/create.bats b/tests/integration/create.bats index f12291ca9d9..7c9110e7674 100644 --- a/tests/integration/create.bats +++ b/tests/integration/create.bats @@ -10,6 +10,70 @@ function teardown() { teardown_bundle } +# is_allowed_fdtarget checks whether the target of a file descriptor symlink +# conforms to the allowed whitelist. +# +# This whitelist reflects the set of file descriptors that runc legitimately +# opens during container lifecycle operations (e.g., exec, create, and run). +# If runc's internal behavior changes (e.g., new FD types are introduced), +# this function MUST be updated accordingly to avoid false positives. +# +is_allowed_fdtarget() { + local target="$1" + { + # pty devices for stdio + grep -Ex "/dev/pts/[0-9]+" <<<"$target" || + # eventfd, eventpoll, signalfd, etc. + grep -Ex "anon_inode:\[.+\]" <<<"$target" || + # procfs handle cache (pathrs-lite / libpathrs) + grep -Ex "/(proc)?" <<<"$target" || + # anonymous sockets used for IPC + grep -Ex "socket:\[[0-9]+\]" <<<"$target" || + # anonymous pipes used for I/O forwarding + grep -Ex "pipe:\[[0-9]+\]" <<<"$target" || + # "runc start" synchronisation barrier FIFO + grep -Ex ".*/exec\.fifo" <<<"$target" || + # temporary internal fd used in exec.fifo FIFO reopen (pathrs-lite / libpathrs) + grep -Ex "(/proc)?/1/task/1/fd" <<<"$target" || + # overlayfs binary reference (CVE-2019-5736) + grep -Ex "/runc" <<<"$target" || + # memfd cloned binary (CVE-2019-5736) + grep -Fx "/memfd:runc_cloned:/proc/self/exe (deleted)" <<<"$target" + } >/dev/null + return "$?" +} + +@test "runc create[detect fd leak as comprehensively as possible]" { + runc create --console-socket "$CONSOLE_SOCKET" test_busybox + [ "$status" -eq 0 ] + + testcontainer test_busybox created + + pid=$(__runc state test_busybox | jq '.pid') + violation_found=0 + + while IFS= read -rd '' link; do + fd_name=$(basename "$link") + # Skip . and .. + if [[ "$fd_name" == "." || "$fd_name" == ".." ]]; then + continue + fi + + # Resolve symlink target (use readlink) + target=$(readlink "$link" 2>/dev/null) + if [[ -z "$target" ]]; then + echo "Warning: Cannot read target of $link" + continue + fi + + if ! is_allowed_fdtarget "$target"; then + echo "Violation: FD $fd_name -> '$target'" + violation_found=1 + fi + done < <(find "/proc/$pid/fd" -type l -print0) + [ "$violation_found" -eq 0 ] +} + @test "runc create" { runc create --console-socket "$CONSOLE_SOCKET" test_busybox [ "$status" -eq 0 ] diff --git a/tests/integration/seccomp.bats b/tests/integration/seccomp.bats index 748dbd2bfca..db9571e0d67 100644 --- a/tests/integration/seccomp.bats +++ b/tests/integration/seccomp.bats @@ -185,3 +185,16 @@ function flags_value() { [[ "$output" == *"error running startContainer hook"* ]] [[ "$output" == *"bad system call"* ]] } + +@test "runc run [seccomp] (verify syscall compatibility after seccomp enforcement)" { + update_config ' .process.args = ["true"] + | .process.noNewPrivileges = false + | .linux.seccomp = { + "defaultAction":"SCMP_ACT_ALLOW", + "architectures":["SCMP_ARCH_X86","SCMP_ARCH_X32","SCMP_ARCH_X86_64","SCMP_ARCH_AARCH64","SCMP_ARCH_ARM"], + "syscalls":[{"names":["close_range", "fsopen", "fsconfig", "fspick", "openat2", "open_tree", "move_mount", "mount_setattr"], "action":"SCMP_ACT_ERRNO", "errnoRet": 38}] + }' + + runc run test_busybox + [ "$status" -eq 0 ] +} diff --git a/vendor/github.com/cyphar/filepath-securejoin/CHANGELOG.md b/vendor/github.com/cyphar/filepath-securejoin/CHANGELOG.md index 734cf61e322..6d016d05c00 100644 --- a/vendor/github.com/cyphar/filepath-securejoin/CHANGELOG.md +++ b/vendor/github.com/cyphar/filepath-securejoin/CHANGELOG.md @@ -6,49 +6,39 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ## [Unreleased] ## -## [0.6.0] - 2025-11-03 ## +## [0.6.1] - 2025-11-19 ## -> By the Power of Greyskull! +> At last up jumped the cunning spider, and fiercely held her fast. + +### Fixed ### +- Our logic for deciding whether to use `openat2(2)` or fallback to an `O_PATH` + resolver would cache the result to avoid doing needless test runs of + `openat2(2)`. However, this causes issues when `pathrs-lite` is being used by + a program that applies new seccomp-bpf filters onto itself -- if the filter + denies `openat2(2)` then we would return that error rather than falling back + to the `O_PATH` resolver. To resolve this issue, we no longer cache the + result if `openat2(2)` was successful, only if there was an error. +- A file descriptor leak in our `openat2` wrapper (when doing the necessary + `dup` for `RESOLVE_IN_ROOT`) has been removed. -While quite small code-wise, this release marks a very key point in the -development of filepath-securejoin. - -filepath-securejoin was originally intended (back in 2017) to simply be a -single-purpose library that would take some common code used in container -runtimes (specifically, Docker's `FollowSymlinksInScope`) and make it more -general-purpose (with the eventual goals of it ending up in the Go stdlib). - -Of course, I quickly discovered that this problem was actually far more -complicated to solve when dealing with racing attackers, which lead to me -developing `openat2(2)` and [libpathrs][]. I had originally planned for -libpathrs to completely replace filepath-securejoin "once it was ready" but in -the interim we needed to fix several race attacks in runc as part of security -advisories. Obviously we couldn't require the usage of a pre-0.1 Rust library -in runc so it was necessary to port bits of libpathrs into filepath-securejoin. -(Ironically the first prototypes of libpathrs were originally written in Go and -then rewritten to Rust, so the code in filepath-securejoin is actually Go code -that was rewritten to Rust then re-rewritten to Go.) - -It then became clear that pure-Go libraries will likely not be willing to -require CGo for all of their builds, so it was necessary to accept that -filepath-securejoin will need to stay. As such, in v0.5.0 we provided more -pure-Go implementations of features from libpathrs but moved them into -`pathrs-lite` subpackage to clarify what purpose these helpers serve. - -This release finally closes the loop and makes it so that pathrs-lite can -transparently use libpathrs (via a `libpathrs` build-tag). This means that -upstream libraries can use the pure Go version if they prefer, but downstreams -(either downstream library users or even downstream distributions) are able to -migrate to libpathrs for all usages of pathrs-lite in an entire Go binary. - -I should make it clear that I do not plan to port the rest of libpathrs to Go, -as I do not wish to maintain two copies of the same codebase. pathrs-lite -already provides the core essentials necessary to operate on paths safely for -most modern systems. Users who want additional hardening or more ergonomic APIs -are free to use [`cyphar.com/go-pathrs`][go-pathrs] (libpathrs's Go bindings). +## [0.5.2] - 2025-11-19 ## -[libpathrs]: https://github.com/cyphar/libpathrs -[go-pathrs]: https://cyphar.com/go-pathrs +> "Will you walk into my parlour?" said a spider to a fly. + +### Fixed ### +- Our logic for deciding whether to use `openat2(2)` or fallback to an `O_PATH` + resolver would cache the result to avoid doing needless test runs of + `openat2(2)`. However, this causes issues when `pathrs-lite` is being used by + a program that applies new seccomp-bpf filters onto itself -- if the filter + denies `openat2(2)` then we would return that error rather than falling back + to the `O_PATH` resolver. To resolve this issue, we no longer cache the + result if `openat2(2)` was successful, only if there was an error. +- A file descriptor leak in our `openat2` wrapper (when doing the necessary + `dup` for `RESOLVE_IN_ROOT`) has been removed. + +## [0.6.0] - 2025-11-03 ## + +> By the Power of Greyskull! ### Breaking ### - The deprecated `MkdirAll`, `MkdirAllHandle`, `OpenInRoot`, `OpenatInRoot` and @@ -56,12 +46,12 @@ are free to use [`cyphar.com/go-pathrs`][go-pathrs] (libpathrs's Go bindings). directly. ### Added ### -- `pathrs-lite` now has support for using [libpathrs][libpathrs] as a backend. - This is opt-in and can be enabled at build time with the `libpathrs` build - tag. The intention is to allow for downstream libraries and other projects to - make use of the pure-Go `github.com/cyphar/filepath-securejoin/pathrs-lite` - package and distributors can then opt-in to using `libpathrs` for the entire - binary if they wish. +- `pathrs-lite` now has support for using libpathrs as a backend. This is + opt-in and can be enabled at build time with the `libpathrs` build tag. The + intention is to allow for downstream libraries and other projects to make use + of the pure-Go `github.com/cyphar/filepath-securejoin/pathrs-lite` package + and distributors can then opt-in to using `libpathrs` for the entire binary + if they wish. ## [0.5.1] - 2025-10-31 ## @@ -440,8 +430,10 @@ This is our first release of `github.com/cyphar/filepath-securejoin`, containing a full implementation with a coverage of 93.5% (the only missing cases are the error cases, which are hard to mocktest at the moment). -[Unreleased]: https://github.com/cyphar/filepath-securejoin/compare/v0.6.0...HEAD -[0.6.0]: https://github.com/cyphar/filepath-securejoin/compare/v0.5.1...v0.6.0 +[Unreleased]: https://github.com/cyphar/filepath-securejoin/compare/v0.6.1...HEAD +[0.6.1]: https://github.com/cyphar/filepath-securejoin/compare/v0.6.0...v0.6.1 +[0.6.0]: https://github.com/cyphar/filepath-securejoin/compare/v0.5.0...v0.6.0 +[0.5.2]: https://github.com/cyphar/filepath-securejoin/compare/v0.5.1...v0.5.2 [0.5.1]: https://github.com/cyphar/filepath-securejoin/compare/v0.5.0...v0.5.1 [0.5.0]: https://github.com/cyphar/filepath-securejoin/compare/v0.4.1...v0.5.0 [0.4.1]: https://github.com/cyphar/filepath-securejoin/compare/v0.4.0...v0.4.1 diff --git a/vendor/github.com/cyphar/filepath-securejoin/VERSION b/vendor/github.com/cyphar/filepath-securejoin/VERSION index a918a2aa18d..ee6cdce3c29 100644 --- a/vendor/github.com/cyphar/filepath-securejoin/VERSION +++ b/vendor/github.com/cyphar/filepath-securejoin/VERSION @@ -1 +1 @@ -0.6.0 +0.6.1 diff --git a/vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/fd/openat2_linux.go b/vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/fd/openat2_linux.go index 3e937fe3c16..63863647d5b 100644 --- a/vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/fd/openat2_linux.go +++ b/vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/fd/openat2_linux.go @@ -39,7 +39,9 @@ const scopedLookupMaxRetries = 128 // Openat2 is an [Fd]-based wrapper around unix.Openat2, but with some retry // logic in case of EAGAIN errors. -func Openat2(dir Fd, path string, how *unix.OpenHow) (*os.File, error) { +// +// NOTE: This is a variable so that the lookup tests can force openat2 to fail. +var Openat2 = func(dir Fd, path string, how *unix.OpenHow) (*os.File, error) { dirFd, fullPath := prepareAt(dir, path) // Make sure we always set O_CLOEXEC. how.Flags |= unix.O_CLOEXEC diff --git a/vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/gocompat/gocompat_atomic_go119.go b/vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/gocompat/gocompat_atomic_go119.go new file mode 100644 index 00000000000..ac93cb045e1 --- /dev/null +++ b/vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/gocompat/gocompat_atomic_go119.go @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: BSD-3-Clause + +//go:build linux && go1.19 + +// Copyright 2022 The Go Authors. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package gocompat + +import ( + "sync/atomic" +) + +// A Bool is an atomic boolean value. +// The zero value is false. +// +// Bool must not be copied after first use. +type Bool = atomic.Bool diff --git a/vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/gocompat/gocompat_atomic_unsupported.go b/vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/gocompat/gocompat_atomic_unsupported.go new file mode 100644 index 00000000000..21b5b29ada9 --- /dev/null +++ b/vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/gocompat/gocompat_atomic_unsupported.go @@ -0,0 +1,48 @@ +// SPDX-License-Identifier: BSD-3-Clause + +//go:build linux && !go1.19 + +// Copyright (C) 2024-2025 SUSE LLC. All rights reserved. +// Use of this source code is governed by a BSD-style +// license that can be found in the LICENSE file. + +package gocompat + +import ( + "sync/atomic" +) + +// noCopy may be added to structs which must not be copied +// after the first use. +// +// See https://golang.org/issues/8005#issuecomment-190753527 +// for details. +// +// Note that it must not be embedded, due to the Lock and Unlock methods. +type noCopy struct{} + +// Lock is a no-op used by -copylocks checker from `go vet`. +func (*noCopy) Lock() {} + +// b32 returns a uint32 0 or 1 representing b. +func b32(b bool) uint32 { + if b { + return 1 + } + return 0 +} + +// A Bool is an atomic boolean value. +// The zero value is false. +// +// Bool must not be copied after first use. +type Bool struct { + _ noCopy + v uint32 +} + +// Load atomically loads and returns the value stored in x. +func (x *Bool) Load() bool { return atomic.LoadUint32(&x.v) != 0 } + +// Store atomically stores val into x. +func (x *Bool) Store(val bool) { atomic.StoreUint32(&x.v, b32(val)) } diff --git a/vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/gopathrs/lookup_linux.go b/vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/gopathrs/lookup_linux.go index 56480f0cee1..ad233f14053 100644 --- a/vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/gopathrs/lookup_linux.go +++ b/vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/gopathrs/lookup_linux.go @@ -193,8 +193,13 @@ func lookupInRoot(root fd.Fd, unsafePath string, partial bool) (Handle *os.File, // managed open, along with the remaining path components not opened. // Try to use openat2 if possible. - if linux.HasOpenat2() { - return lookupOpenat2(root, unsafePath, partial) + // + // NOTE: If openat2(2) works normally but fails for this lookup, it is + // probably not a good idea to fall-back to the O_PATH resolver. An + // attacker could find a bug in the O_PATH resolver and uncontionally + // falling back to the O_PATH resolver would form a downgrade attack. + if handle, remainingPath, err := lookupOpenat2(root, unsafePath, partial); err == nil || linux.HasOpenat2() { + return handle, remainingPath, err } // Get the "actual" root path from /proc/self/fd. This is necessary if the diff --git a/vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/gopathrs/openat2_linux.go b/vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/gopathrs/openat2_linux.go index b80ecd08953..9c5c268f665 100644 --- a/vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/gopathrs/openat2_linux.go +++ b/vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/gopathrs/openat2_linux.go @@ -41,6 +41,7 @@ func openat2(dir fd.Fd, path string, how *unix.OpenHow) (*os.File, error) { if err != nil { return nil, err } + _ = file.Close() file = newFile } } diff --git a/vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/linux/openat2_linux.go b/vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/linux/openat2_linux.go index 399609dc361..dc5f65cef7f 100644 --- a/vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/linux/openat2_linux.go +++ b/vendor/github.com/cyphar/filepath-securejoin/pathrs-lite/internal/linux/openat2_linux.go @@ -17,15 +17,27 @@ import ( "github.com/cyphar/filepath-securejoin/pathrs-lite/internal/gocompat" ) +// sawOpenat2Error stores whether we have seen an error from HasOpenat2. This +// is a one-way toggle, so as soon as we see an error we "lock" into that mode. +// We cannot use sync.OnceValue to store the success/fail state once because it +// is possible for the program we are running in to apply a seccomp-bpf filter +// and thus disable openat2 during execution. +var sawOpenat2Error gocompat.Bool + // HasOpenat2 returns whether openat2(2) is supported on the running kernel. -var HasOpenat2 = gocompat.SyncOnceValue(func() bool { +var HasOpenat2 = func() bool { + if sawOpenat2Error.Load() { + return false + } + fd, err := unix.Openat2(unix.AT_FDCWD, ".", &unix.OpenHow{ Flags: unix.O_PATH | unix.O_CLOEXEC, Resolve: unix.RESOLVE_NO_SYMLINKS | unix.RESOLVE_IN_ROOT, }) if err != nil { + sawOpenat2Error.Store(true) // doesn't matter if we race here return false } _ = unix.Close(fd) return true -}) +} diff --git a/vendor/modules.txt b/vendor/modules.txt index 6375a65d558..311d10619e2 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -33,7 +33,7 @@ github.com/coreos/go-systemd/v22/dbus # github.com/cpuguy83/go-md2man/v2 v2.0.7 ## explicit; go 1.12 github.com/cpuguy83/go-md2man/v2/md2man -# github.com/cyphar/filepath-securejoin v0.6.0 +# github.com/cyphar/filepath-securejoin v0.6.1 ## explicit; go 1.18 github.com/cyphar/filepath-securejoin github.com/cyphar/filepath-securejoin/internal/consts