fedora-selinux / selinux-policy

selinux-policy for Fedora is a large patch off the mainline
GNU General Public License v2.0
164 stars 165 forks source link

policy: support pidfs #2050

Closed brauner closed 6 months ago

brauner commented 7 months ago

pidfds are ported to a tiny in-kernel filesystem that is not mountable in userspace. This is comparable to sockfs, pipefs, nsfs, or anon_inodefs to name a few examples.

Before pidfs it wasn't possible for selinux to manage them because they didn't go through LSM hooks. We can now start doing that. But if we do it right away we'd get permission errors.

pidfds are used in systemd, dbus, LXC, polkit etc. and they currently start failing because selinux denies pidfs:

Feb 23 12:09:58 fed1 audit[353]: AVC avc: denied { read write open } for pid=353 comm="systemd-userdbd" path="pidfd:[709]" dev="pidfs" ino=709 scontext=system_u:system_r:systemd_userdbd_t:>

brauner commented 7 months ago

I lack the Selinux knowledge to fill in any potentially missing bits. So help would be very much appreciated!

packit-as-a-service[bot] commented 7 months ago

Cockpit tests failed for commit 1dfa131ade53d776088dc8b1c1519dbb442f36d7. @martinpitt, @jelly, @mvollmer please check.

brauner commented 7 months ago

Fwiw, I also filed https://bugzilla.redhat.com/show_bug.cgi?id=2265630

brauner commented 7 months ago

Closing this since I now filed a bugzilla entry.

martinpitt commented 7 months ago

Rawhide failure is the same issue as in https://github.com/fedora-selinux/selinux-policy/pull/2049#issuecomment-1960826431

@brauner You said "closing this", but you actually didn't. Also, this change looks legit?

brauner commented 7 months ago

Rawhide failure is the same issue as in #2049 (comment)

@brauner You said "closing this", but you actually didn't. Also, this change looks legit?

Yeah, sorry. I didn't want to keep a review open in case that would be obviously wrong. If this looks ok the I'm happy if this is merged ofc.

WOnder93 commented 7 months ago

I believe we should do the same as refpolicy, i.e. use fs_use_task for this filesystem.

WOnder93 commented 7 months ago

I have a concern, though... if we do use fs_use_task, then from the policy POV the pidfs/pidfd operations will be indistinguishable from procfs file access (i.e. the access vectors will be just <source domain> <target domain> file <perms> with nothing identifying them as pidfs/pidfd operations.

So I think a better approach would be to add some pidfd-specific SELinux security class (with an appropriate set of permissions) and LSM hooks in the kernel and use a single label for the pidfs pseudofiles (allowing to grant/deny access to pidfs also at the filesystem level, although with less granularity; or it could even be universally allowed, leaving access control up to the new pidfd-specific SELinux permissions).

In fact, reading up on pidfd_open(2), it seems that labeling pidfds by the creating process doesn't actually gain much anyway, since the label would not refer to the process identified by the original PID, but merely to the process that has created the fd. So I retract my comment above - the best we can do with the current kernel situation is indeed to label everything pidfs_t, as the PR already does.

brauner commented 7 months ago

Sounds good to me.

zpytela commented 6 months ago

Merging, thank you.