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

update systemd bpf mount unit file to be more secure #10793

Closed
travisghansen opened this issue Mar 31, 2020 · 9 comments · Fixed by #10805
Closed

update systemd bpf mount unit file to be more secure #10793

travisghansen opened this issue Mar 31, 2020 · 9 comments · Fixed by #10805
Assignees
Labels
area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. kind/community-report This was reported by a user in the Cilium community, eg via Slack.

Comments

@travisghansen
Copy link
Contributor

How to reproduce the issue

The example systemd unit file found here: https://cilium.readthedocs.io/en/stable/kubernetes/configuration/#bpffs-systemd

resulted in (what I think) are pretty insecure permissions for the /sys/fs/bpf directory for me on a centos7 install (don't have it in front of me directly right now but it was essentially 777 with some tmp mode set).

I updated it to the following to lock it down a bit tighter:

[Unit]
Description=Cilium BPF mounts
Documentation=http://docs.cilium.io/
DefaultDependencies=no
Before=local-fs.target umount.target
After=swap.target

[Mount]
What=bpffs
Where=/sys/fs/bpf
Type=bpf
Options=rw,nosuid,nodev,noexec,relatime,mode=700

[Install]
WantedBy=multi-user.target

I don't know enough about bpf to say if it was truly dangerous but by the little that I do know of the nature of bpf I'd rather be safe than sorry :)

@aanm
Copy link
Member

aanm commented Mar 31, 2020

cc @borkmann

@aanm
Copy link
Member

aanm commented Mar 31, 2020

thanks for opening the issue @travisghansen!

@aanm aanm added kind/community-report This was reported by a user in the Cilium community, eg via Slack. needs/triage This issue requires triaging to establish severity and next steps. labels Mar 31, 2020
@borkmann
Copy link
Member

borkmann commented Mar 31, 2020

@travisghansen, by default it's 777 that is correct, though everything Cilium related gets pinned with 600 (and generally retrieval needs CAP_SYS_ADMIN). From your above options, rw,nosuid,nodev,noexec,relatime,mode=700, it should really be rw,relatime,mode=700. BPF is a special file system and only allows BPF progs or maps to be pinned there, so no executables and such and thus these options do not apply. Could you send a patch updating to rw,relatime,mode=700 or do you prefer to to submit a PR? Thanks

@travisghansen
Copy link
Contributor Author

Ok not sure where arch got those options... might be good to see what it is on centos8 or Ubuntu as well.

I will say the options above at least crudely work with cilium as that's what I used and initial testing shows files in the dir and traffic flowing at a basic level.

@travisghansen
Copy link
Contributor Author

I may have misunderstood the context of the earlier comment as well. I'm less concerned about cilium rules as I am foreign stuff being created in the fs due to so permissive options.

Here's a fresh centos 8 install where support for bpf appears to be 'native' in some shape or form...notice the options are exactly the same as what I proposed as used by arch as well:

mount | grep bpf
bpf on /sys/fs/bpf type bpf (rw,nosuid,nodev,noexec,relatime,mode=700)
[root@centos-8 ~]# ls -l /sys/fs/
total 0
drwx-----T.  2 root root   0 Mar 31 21:14 bpf
drwxr-xr-x. 14 root root 360 Mar 31 21:14 cgroup
drwxr-xr-x.  4 root root   0 Mar 31 21:25 ext4
drwxr-xr-x.  3 root root   0 Mar 31 21:15 fuse
drwxr-x---.  2 root root   0 Mar 31 21:14 pstore
drwxr-xr-x.  7 root root   0 Mar 31 21:14 selinux
drwxr-xr-x.  4 root root   0 Mar 31 21:25 xfs

@borkmann
Copy link
Member

borkmann commented Apr 1, 2020

I may have misunderstood the context of the earlier comment as well. I'm less concerned about cilium rules as I am foreign stuff being created in the fs due to so permissive options.

Here's a fresh centos 8 install where support for bpf appears to be 'native' in some shape or form...notice the options are exactly the same as what I proposed as used by arch as well:

mount | grep bpf
bpf on /sys/fs/bpf type bpf (rw,nosuid,nodev,noexec,relatime,mode=700)

Ok, fair enough, I presume newer systemd is auto-mounting in this case. bpffs only takes the mode into account (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/kernel/bpf/inode.c#n606), but having the rest doesn't hurt. Could you send a PR to update the Cilium Unit file? Thanks

@qmonnet
Copy link
Member

qmonnet commented Apr 1, 2020

For the record, on a quick grep I see at least three locations where we have this systemd snippet:

Documentation/kubernetes/configuration.rst
contrib/systemd/sys-fs-bpf.mount
install/kubernetes/cilium/charts/nodeinit/templates/daemonset.yaml

@qmonnet qmonnet added area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. and removed needs/triage This issue requires triaging to establish severity and next steps. labels Apr 1, 2020
@travisghansen
Copy link
Contributor Author

Yeah I assumed the same thing regarding newer versions of systemd auto-detecting and mounting.

I'm pretty strapped for time (and I'm pretty weak with go) ATM so probably best if someone else runs with this. Thanks!

@borkmann
Copy link
Member

borkmann commented Apr 1, 2020

Ok, no problem, I'll look into it.

borkmann added a commit that referenced this issue Apr 1, 2020
Given bpf fs wasn't mounted before, then mount it with stricter
permissions than the default ones (777). Also add few other options
as discussed in #10793 such as `nosuid,nodev,noexec` though at least
from bpf fs side these are ignored.

Fixes: #10793
Reported-by: Travis Glenn Hansen <travisghansen@yahoo.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
borkmann added a commit that referenced this issue Apr 1, 2020
Given bpf fs wasn't mounted before, then mount it with stricter
permissions than the default ones (777). Also add few other options
as discussed in #10793 such as `nosuid,nodev,noexec` though at least
from bpf fs side these are ignored.

Fixes: #10793
Reported-by: Travis Glenn Hansen <travisghansen@yahoo.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
joestringer pushed a commit that referenced this issue Apr 2, 2020
[ upstream commit 95529fb ]

Given bpf fs wasn't mounted before, then mount it with stricter
permissions than the default ones (777). Also add few other options
as discussed in #10793 such as `nosuid,nodev,noexec` though at least
from bpf fs side these are ignored.

Fixes: #10793
Reported-by: Travis Glenn Hansen <travisghansen@yahoo.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Signed-off-by: Joe Stringer <joe@cilium.io>
borkmann added a commit that referenced this issue Apr 2, 2020
[ upstream commit 95529fb ]

Given bpf fs wasn't mounted before, then mount it with stricter
permissions than the default ones (777). Also add few other options
as discussed in #10793 such as `nosuid,nodev,noexec` though at least
from bpf fs side these are ignored.

Fixes: #10793
Reported-by: Travis Glenn Hansen <travisghansen@yahoo.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Signed-off-by: Joe Stringer <joe@cilium.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. kind/community-report This was reported by a user in the Cilium community, eg via Slack.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants