It seems that golangci-lint didn't warn us about new lint errors that
were added after we enabled it, so just run the full thing and give us
all the errors on every PR run -- as long as we keep master lint-clean
it doesn't matter whether we set this or not.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Most of these were false positives or cases where we want to ignore the
lint, but the change to the BPF generation is actually useful.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
This is to ensure that we aren't leaking eBPF programs after "runc
update". Unfortunately we cannot directly test the behaviour of cgroup
program updates in an integration test because "runc update" doesn't
support that behaviour at the moment.
So instead we rely on the fact that each "runc update" implicitly
triggers the devices rules to be updated. Without the previous patches
applied, this new test will fail with errors (on cgroupv2 systems).
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
In the normal cases (only one existing filter or no existing filters),
just make use of BPF_F_REPLACE if there is one existing filter. However
if there is more than one filter applied, we should probably remove all
other filters since the alternative is that we will never remove our old
filters.
The only two other viable ways of solving this problem would be to use
BPF pins to either pin the eBPF program using a predictable name (so we
can always only replace *our* programs) or to switch away from custom
programs and instead use eBPF maps (which are pinned) and thus we just
update the map conntents to update the ruleset. Unfortunately these both
would add a hard requirement of bpffs and would require at least a minor
rewrite of the eBPF filtering code -- which is better left for another
time.
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
There were several issues with the previous cgroupv2 devices filter
generator implementation, stemming from the previous implementation
using a few too many tricks to implement the correct cgroup behaviour
(rules were handled in reverse order, with wildcards having particularly
special interpretations). As a result, some slightly odd configurations
with rules in specific orders could result in incorrect filters being
generated.
By switching to the emulator which is already used by cgroupv1, we can
guarantee that the behaviour of filters in both cgroup versions will be
identical, as well as making use of the hardenings in the emulator (not
allowing users to add deny rules the kernel will ignore).
(Note that because the ordering of the devices emulator rules is
deterministic and based on the rule value, the existing test rules had
to be reordered slightly.)
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
The devices cgroup emulator is also useful for removing unneeded rules
as well as computing what the final default-allow state of the filter
will be (allow-list or deny-list).
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
When running inside a Docker container, systemd is not available. The
new TestFdLeaksSystemd forgot to include the relevant t.Skip section.
Fixes: a7feb42395 ("libct/int: add TestFdLeaksSystemd")
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
According to cgroup v2 documentation [1]:
> Freezing of the cgroup may take some time; when this action is
> completed, the “frozen” value in the cgroup.events control file will
> be updated to “1” and the corresponding notification will be issued.
Implement polling of cgroup.events, waiting for "frozen 1" to appear.
In case something goes wrong, limit the maximum number of retries and
return "undefined" after some time (currently 10s).
[1] https://www.kernel.org/doc/html/latest/admin-guide/cgroup-v2.html
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Before this patch, setFreezer does
- open/read/close (to check if the freezer is supported)
- open/write/close (to set the value)
- open/read/close (to check the value)
Three opens is a bit excessive. Refactor to only open the file once:
- open (to check if the freezer is supported)
- write (to set the value)
- seek/read (to check the value)
- close
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
In case configs.Undefined or any wrong value is passed, there is no need
to check whether the freezer is supported.
Move arguments check to the beginning to avoid an unnecessary call to
supportFreezer().
While at it, simplify the "whether to return an error if freezer is not
supported" check.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Because the target of a mount is inside a container (which may be a
volume that is shared with another container), there exists a race
condition where the target of the mount may change to a path containing
a symlink after we have sanitised the path -- resulting in us
inadvertently mounting the path outside of the container.
This is not immediately useful because we are in a mount namespace with
MS_SLAVE mount propagation applied to "/", so we cannot mount on top of
host paths in the host namespace. However, if any subsequent mountpoints
in the configuration use a subdirectory of that host path as a source,
those subsequent mounts will use an attacker-controlled source path
(resolved within the host rootfs) -- allowing the bind-mounting of "/"
into the container.
While arguably configuration issues like this are not entirely within
runc's threat model, within the context of Kubernetes (and possibly
other container managers that provide semi-arbitrary container creation
privileges to untrusted users) this is a legitimate issue. Since we
cannot block mounting from the host into the container, we need to block
the first stage of this attack (mounting onto a path outside the
container).
The long-term plan to solve this would be to migrate to libpathrs, but
as a stop-gap we implement libpathrs-like path verification through
readlink(/proc/self/fd/$n) and then do mount operations through the
procfd once it's been verified to be inside the container. The target
could move after we've checked it, but if it is inside the container
then we can assume that it is safe for the same reason that libpathrs
operations would be safe.
A slight wrinkle is the "copyup" functionality we provide for tmpfs,
which is the only case where we want to do a mount on the host
filesystem. To facilitate this, I split out the copy-up functionality
entirely so that the logic isn't interspersed with the regular tmpfs
logic. In addition, all dependencies on m.Destination being overwritten
have been removed since that pattern was just begging to be a source of
more mount-target bugs (we do still have to modify m.Destination for
tmpfs-copyup but we only do it temporarily).
Fixes: CVE-2021-30465
Reported-by: Etienne Champetier <champetier.etienne@gmail.com>
Co-authored-by: Noah Meyerhans <nmeyerha@amazon.com>
Reviewed-by: Samuel Karp <skarp@amazon.com>
Reviewed-by: Kir Kolyshkin <kolyshkin@gmail.com> (@kolyshkin)
Reviewed-by: Akihiro Suda <akihiro.suda.cz@hco.ntt.co.jp>
Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>
Dockerfile: switch from go 1.15 to go 1.16.x.
Vagrantfile.centos7: switch from go 1.15 to go 1.16.4.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Kir Kolyshkin (8):
tests/int/helpers: rm old code
libct/cg/sd/v2: call initPath from Path
tests/int/update.bats: don't set cpuset in setup
tests/int/helpers: generalize require cgroups_freezer
tests/int: enable/use requires cgroups_<ctrl>
tests/int/cgroups: don't check for hugetlb
tests/int: run rootless_cgroup tests for v2+systemd
Vagrantfile.fedora: set Delegate=yes
LGTMs: AkihiroSuda cyphar
Closes#2944
Instead of listing all individual controllers we want to be delegated,
just say "yes" which means "all the controllers that are available".
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Before this commit, "require rootless_cgroup" feature check required "cgroup"
to be present in ROOTLESS_FEATURES. The idea of the requirement, though, is
to ensure that rootless runc can manage cgroups.
In case of systemd + cgroup v2, rootless runc can manage cgroups,
thanks to systemd delegation, so modify the feature check accordingly.
Next, convert (simplify) some of the existing users to the modified check.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Systemd is not able to delegate hugetlb controller, and it is needed for
cgroup v2 + systemd + rootless case (which is currently skipped because
of "requires rootless_cgroup", but will be enabled by a later commit).
The failure being fixed looks like this:
> not ok 4 runc create (limits + cgrouppath + permission on the cgroup dir) succeeds
> # (from function `check_cgroup_value' in file /vagrant/tests/integration/helpers.bash, line 188,
> # in test file /vagrant/tests/integration/cgroups.bats, line 53)
> # `check_cgroup_value "cgroup.controllers" "$(cat /sys/fs/cgroup/user.slice/user-$(id -u).slice/cgroup.controllers)"' failed
> # <....>
> # /sys/fs/cgroup/user.slice/user-2000.slice/user@2000.service/machine.slice/runc-cgroups-integration-test-20341.scope/cgroup.controllers
> # current cpuset cpu io memory pids !? cpuset cpu io memory hugetlb pids
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
1. In case of cgroup v2 + systemd + rootless, get the list of controllers from
the own cgroup, rather than from the root one (as systemd cgroup delegation
is required in this case, and it might not be set or fully working).
2. Use "requires cgroups_<controller>" in tests that need those
controllers.
Tested on Fedora 31 (which does not support delegation of cpuset controller).
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Setting cpuset.cpus requires cpuset controller, which might
not be available in case of cgroup v2 + systemd < 244.
Rather than require cpuset support from every test case, do not set the initial
cpuset value from setup(), and do not check it.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Sometimes Path() is called before m.path is initialized (in particular,
this happens from (*linuxContainer).newInitConfig), so we do need to
make sure to call initPath.
This fixes the following integration tests (for cgroup v2 + systemd case,
currently not enabled -- to be enabled by further commits):
* runc run (blkio weight)
* runc run (cgroupv2 mount inside container)
Fixes: ff692f289b ("Fix cgroup2 mount for rootless case")
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This was added by commit ca4f427af, together with set_resources_limit,
and was needed for the latter, since sed was used to update resources.
Now when we switched to jq in commit 79fe41d3c1, this kludge is no
longer needed.
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Qiang Huang