When preparing to mount container root, we need to make its parent mount
private (i.e. disable propagation), otherwise the new in-container
mounts are leaked to the host.
To find a parent mount, we use to read mountinfo and find the longest
entry which can be a parent of the container root directory.
Unfortunately, due to kernel bug in all Linux kernels older than v5.8
(see [1], [2]), sometimes mountinfo can't be read in its entirety. In
this case, getParentMount may occasionally return a wrong parent mount.
As a result, we do not change the mount propagation to private, and
container mounts are leaked.
Alas, we can not fix the kernel, and reading mountinfo a few times to
ensure its consistency (like it's done in, say, Kubernetes) does not
look like a good solution for performance reasons.
Fortunately, we don't need mountinfo. Let's just traverse the directory
tree, trying to remount it private until we find a mount point (any
error other than EINVAL means we just found it).
Fixes issue 2404.
[1]: https://github.com/kolyshkin/procfs-test
[2]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9f6c61f96f2d97cbb5f
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Initially, this was a commit to switch from strings.Fields to
strings.SplitN in getCpuUsageBreakdown, since strings.Fields
was probably slower than strings.SplitN in some old Go versions.
Afterwards, strings.Cut was also considered for potential
speed improvements.
After writing a benchmark test, we learned that:
- strings.Fields performance is now adequate;
- strings.SplitN is slower than strings.Fields;
- strings.Cut had <5% performance gain from strings.Fields;
So, remove the TODO and keep the benchmark test.
Signed-off-by: Stavros Panakakis <stavrospanakakis@gmail.com>
The example is too long since it lists too many capabilities.
Simplify it, leaving only two capabilities.
Also, remove ambient capabilities from the set. Inheritable capabilities
were removed earlier by commit 98fe566c, but ambient capabilities can't
be raised without inheritable ones.
Fixes: 98fe566c
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Commit 98fe566c removed inheritable capabilities from the example spec
(used by runc spec) and from the libcontainer/integration test config,
but neglected to also remove ambient capabilities.
An ambient capability could only be set if the same inheritable
capability is set, so as a result of the above change ambient
capabilities were not set (but due to a bug in gocapability package,
those errors are never reported).
Once we start using a library with the fix [1], that bug will become
apparent (both bats-based and libct/int tests will fail).
[1]: https://github.com/kolyshkin/capability/pull/3
Fixes: 98fe566c ("runc: do not set inheritable capabilities")
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
For some rootless container, runc has no access to cgroup,
But the container is still running. So we should return the
`ErrNotRunning` and `ErrCgroupNotExist` error seperatlly.
Signed-off-by: lifubang <lifubang@acmcoder.com>
Shared pid namespace means `runc kill` (or `runc delete -f`) have to
kill all container processes, not just init. To do so, it needs a cgroup
to read the PIDs from.
If there is no cgroup, processes will be leaked, and so such
configuration is bad and should not be allowed. To keep backward
compatibility, though, let's merely warn about this for now.
Alas, the only way to know if cgroup access is available is by returning
an error from Manager.Apply. Amend fs cgroup managers to do so (systemd
doesn't need it, since v1 can't work with rootless, and cgroup v2 does
not have a special rootless case).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In these cases, this is exactly what we want to find out.
Slightly improves performance and readability.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
It turns out that the suid and sgid mode bits are silently ignored by
Linux (though the sticky bit is honoured), and some users are requesting
mode bits that are ignored. While returning an error (as securejoin
does) makes some sense, this is a regression.
Ref: https://github.com/cyphar/filepath-securejoin/issues/23
Fixes: dd827f7b71 ("utils: switch to securejoin.MkdirAllHandle")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
It is available since Go 1.21 and is defined during compile time
(i.e. based on GOARCH during build).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
`signalAllProcesses()` depends on the cgroup and is expected to fail
when runc is running in rootless without an access to the cgroup.
When `RootlessCgroups` is set to `true`, runc just ignores the error
from `signalAllProcesses` and may leak some processes running.
(See the comments in PR 4395)
In the future, runc should walk the process tree to avoid such a leak.
Note that `RootlessCgroups` is a misnomer; it is set to `false` despite
the name when cgroup v2 delegation is configured.
This is expected to be renamed in a separate commit.
Fix issue 4394
Signed-off-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
filepath-securejoin has a bunch of extra hardening features and is very
well-tested, so we should use it instead of our own homebrew solution.
A lot of rootfs_linux.go callers pass a SecureJoin'd path, which means
we need to keep the wrapper helpers in utils, but at least the core
logic is no longer in runc. In future we will want to remove this dodgy
logic and just use file handles for everything (using libpathrs,
ideally).
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
While we use SecureJoin to try to make all of our target paths inside
the container safe, SecureJoin is not safe against an attacker than can
change the path after we "resolve" it.
os.MkdirAll can inadvertently follow symlinks and thus an attacker could
end up tricking runc into creating empty directories on the host (note
that the container doesn't get access to these directories, and the host
just sees empty directories). However, this could potentially cause DoS
issues by (for instance) creating a directory in a conf.d directory for
a daemon that doesn't handle subdirectories properly.
In addition, the handling for creating file bind-mounts did a plain
open(O_CREAT) on the SecureJoin'd path, which is even more obviously
unsafe (luckily we didn't use O_TRUNC, or this bug could've allowed an
attacker to cause data loss...). Regardless of the symlink issue,
opening an untrusted file could result in a DoS if the file is a hung
tty or some other "nasty" file. We can use mknodat to safely create a
regular file without opening anything anyway (O_CREAT|O_EXCL would also
work but it makes the logic a bit more complicated, and we don't want to
open the file for any particular reason anyway).
libpathrs[1] is the long-term solution for these kinds of problems, but
for now we can patch this particular issue by creating a more restricted
MkdirAll that refuses to resolve symlinks and does the creation using
file descriptors. This is loosely based on a more secure version that
filepath-securejoin now has[2] and will be added to libpathrs soon[3].
[1]: https://github.com/openSUSE/libpathrs
[2]: https://github.com/cyphar/filepath-securejoin/releases/tag/v0.3.0
[3]: https://github.com/openSUSE/libpathrs/issues/10
Fixes: CVE-2024-45310
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
The warnings fixed were:
libcontainer/configs/config_test.go:205:12: printf: non-constant format string in call to (*testing.common).Errorf (govet)
t.Errorf(fmt.Sprintf("Expected error to not occur but it was %+v", err))
^
libcontainer/cgroups/fs/blkio_test.go:481:13: printf: non-constant format string in call to (*testing.common).Errorf (govet)
t.Errorf(fmt.Sprintf("test case '%s' failed unexpectedly: %s", testCase.desc, err))
^
libcontainer/cgroups/fs/blkio_test.go:595:13: printf: non-constant format string in call to (*testing.common).Errorf (govet)
t.Errorf(fmt.Sprintf("test case '%s' failed unexpectedly: %s", testCase.desc, err))
^
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In all the three cases, we check that the program returned non-zero exit
code. This can be done in a much simpler manner.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
The logic for how we create mountpoints is spread over each mountpoint
preparation function, when in reality the behaviour is pretty uniform
with only a handful of exceptions. So just move it all to one function
that is easier to understand.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Now that we dropped support for go < 1.21, we can use this; moving
the sync.once out of the runningInUserNS() implementation would also
allow for it to be more easily tested if we'd decide to.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The fuzzer for this only runs on Linux; rename the file to be Linux-only
so that we don't have to stub out the uidMapInUserNS function.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This was a poor decision on my side; 4316df8b53
moved this utility to a separate package, and split the exported function
from the implementation (and stubs). Out of convenience, I used an alias
for the latter part, but there's two downsides to that;
- `RunningInUserNS` being an exported var means that (technically) it can
be replaced by other code; perhaps that's a "feature", but not one we
intended it to be used for.
- `RunningInUserNS` being implemented through a var / alias means it's
also documented as such on [pkg.go.dev], which is confusing.
This patch changes it to a regular function, acting as a wrapper for
the underlying implementations. While at it, also slightly touching
up the GoDoc to describe its functionality / behavior.
[pkg.go.dev]: https://pkg.go.dev/github.com/opencontainers/runc@v1.1.13/libcontainer/userns#RunningInUserNS
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Commit 4316df8b53 isolated RunningInUserNS
to a separate package to make it easier to consume without bringing in
additional dependencies, and with the potential to move it separate in
a similar fashion as libcontainer/user was moved to a separate module
in commit ca32014adb. While RunningInUserNS
is fairly trivial to implement, it (or variants of this utility) is used
in many codebases, and moving to a separate module could consolidate
those implementations, as well as making it easier to consume without
large dependency trees (when being a package as part of a larger code
base).
Commit 1912d5988b and follow-ups introduced
cgo code into the userns package, and code introduced in those commits
are not intended for external use, therefore complicating the potential
of moving the userns package separate.
This commit moves the new code to a separate package; some of this code
was included in v1.1.11 and up, but I could not find external consumers
of `GetUserNamespaceMappings` and `IsSameMapping`. The `Mapping` and
`Handles` types (added in ba0b5e2698) only
exist in main and in non-stable releases (v1.2.0-rc.x), so don't need
an alias / deprecation.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
By definition, every container has only 1 init (i.e. PID 1) process.
Apparently, libcontainer API supported running more than 1 init, and
at least one tests mistakenly used it.
Let's not allow that, erroring out if we already have init. Doing
otherwise _probably_ results in some confusion inside the library.
Fix two cases in libct/int which ran two inits inside a container.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. The code to call c.exec from c.Run was initially added by commit
3aacff695. At the time, there was a lock in c.Run. That lock was
removed by commit bd3c4f84, which resulted in part of c.Run executing
without the lock.
2. All the Start/Run/Exec calls were a mere wrappers for start/run/exec
adding a lock, but some more code crept into Start at some point,
e.g. by commits 805b8c73 and 108ee85b8. Since the reason mentioned in
commit 805b8c73 is no longer true after refactoring, we can fix this.
Fix both issues by moving code out of wrappers, and adding locking into
c.Run.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In case file already exists, mknod(2) will return EEXIST.
This os.Stat call was (inadvertently?) added by commit 805b8c73.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
There's too much logic here figuring out which CPUs to use. Runc is a
low level tool and is not supposed to be that "smart". What's worse,
this logic is executed on every exec, making it slower. Some of the
logic in (*setnsProcess).start is executed even if no annotation is set,
thus making ALL execs slow.
Also, this should be a property of a process, rather than annotation.
The plan is to rework this.
This reverts commit afc23e3397.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
It has been pointed out that some controllers can not accept multiple
lines of output at once. In particular, io.max can only set one device
at a time.
Practically, the only multi-line resource values we can get come from
unified.* -- let's write those line by line.
Add a test case.
Reported-by: Tao Shen <shentaoskyking@gmail.com>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
It is not needed since Go 1.20 (which was released in February 2023 and
is no longer supported since February 2024).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Go 1.23 includes a fix (https://go.dev/cl/587919) so go1.23.x can be
used. This fix is also backported to 1.22.4, so go1.22.x can also be
used (when x >= 4). Finally, for glibc >= 2.32 it doesn't really matter.
Add a note about Go 1.22.x > 1.22.4 to README as well.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
When swap is being disabled (as set to 0), or set to max, ignore
non-existent memory.swap.max cgroup file.
If swap is being set explicitly to some value, do return an error like
before.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Go 1.23 tightens access to internal symbols, and even puts runc into
"hall of shame" for using an internal symbol (recently added by commit
da68c8e3). So, while not impossible, it becomes harder to access those
internal symbols, and it is a bad idea in general.
Since Go 1.23 includes https://go.dev/cl/588076, we can clean the
internal rlimit cache by setting the RLIMIT_NOFILE for ourselves,
essentially disabling the rlimit cache.
Once Go 1.22 is no longer supported, we will remove the go:linkname hack.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This test panics if userns is detected (such as when run in a rootless
docker container) because SetV1 does nothing in this case.
We could fix the panic, but it doesn't make sense to run the test at
all.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Kir Kolyshkin