Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

containers: trim mountpoint from stored paths #3231

Conversation

NDStrahilevitz
Copy link
Collaborator

This ensures that only the cgroup subpath is evaluated for a container id.
This resolves the case where container mounting the cgroup fs through kubernetes may have their container id as part of the mountpoint. Then, events on the host would be evaluated with respect to that mountpoint, including its container id.

This ensures that only the cgroup subpath is evaluated for a container
id.
This resolves the case where container mounting the cgroup fs through
kubernetes may have their container id as part of the mountpoint.
Then, events on the host would be evaluated with respect to that
mountpoint, including its container id.
Copy link
Contributor

@rafaeldtinoco rafaeldtinoco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I only missed an output example with the error/issue described (and possibly a reproducer). Its always good to provide the error output and, perhaps, if possible, a reproducer.

In this case:

cgroupId: 17001, NEW path: /system.slice/docker.service
cgroupId: 16929, OLD path: /sys/fs/cgroup/system.slice/docker.socket
cgroupId: 16929, NEW path: /system.slice/docker.socket
cgroupId: 2619, OLD path: /sys/fs/cgroup/system.slice/haveged.service
cgroupId: 2619, NEW path: /system.slice/haveged.service
cgroupId: 2089, OLD path: /sys/fs/cgroup/system.slice/home.mount
cgroupId: 2089, NEW path: /system.slice/home.mount
cgroupId: 3149, OLD path: /sys/fs/cgroup/system.slice/networkd-dispatcher.service
cgroupId: 3149, NEW path: /system.slice/networkd-dispatcher.service
cgroupId: 6985, OLD path: /sys/fs/cgroup/system.slice/nmbd.service
cgroupId: 6985, NEW path: /system.slice/nmbd.service
cgroupId: 3202, OLD path: /sys/fs/cgroup/system.slice/rsyslog.service
cgroupId: 3202, NEW path: /system.slice/rsyslog.service
cgroupId: 7520, OLD path: /sys/fs/cgroup/system.slice/smbd.service
cgroupId: 7520, NEW path: /system.slice/smbd.service
cgroupId: 5420, OLD path: /sys/fs/cgroup/system.slice/ssh.service

the change is definitely something good. I can imagine that nested cgroupfs root mountpoint directories could face trouble when parsing for container engines (that is probably your case ?).

@NDStrahilevitz
Copy link
Collaborator Author

NDStrahilevitz commented Jun 14, 2023

@rafaeldtinoco
Sure, i'll add some examples while waiting to merge (since internal e2es aren't passing and all).
So when deploying tracee in kubernetes with a mount like /sys => /something/sys, the mount will exist in /proc/mounts both in /something/sys/fs/cgroup/cpuset AND in /run/containerd/io.containerd.runtime.v2.task/k8s.io/<mounting_container_id>/rootfs/host/sys/fs/cgroup/cpuset.

Now, tracee searches for existing mountpoints before mounting the cgroup controller directory itself. If it finds one, it leaves it unmanaged. It's key to note that there can be multiple mounts and tracee will use the last one found in /proc/mounts.

So now let's examine the full case.
Let's say we did this mount and tracee detected it in <...long_path_with_container_id>/sys/fs/cgroup/cpuset. Tracee will initially populate all cgroup paths, including that one, in containers.Populate(). Since cpuset has cgroup id(inode) 1, it may share the cgroup id of all host events. As such, ALL host events will now be associated to that cgroup path, and as such, falsely attributed to the mounting container.

Sidenote: It's interesting that before we changed cgroup path parsing to go in reverse this was even worse as ALL events would be attributed to the mounting container (because back then the first container id counted, not the last.

@NDStrahilevitz NDStrahilevitz merged commit 6dca27a into aquasecurity:main Jun 14, 2023
26 checks passed
@NDStrahilevitz NDStrahilevitz deleted the trim_cgroup_mountpoint_from_path branch June 14, 2023 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants