policy: support pidfs #2050
Merged
policy: support pidfs #2050
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 security_file_open(). They now do making it possible
for selinux to manage them.
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:>
Signed-off-by: Christian Brauner <[email protected]>
|
I lack the Selinux knowledge to fill in any potentially missing bits. So help would be very much appreciated! |
|
Cockpit tests failed for commit 1dfa131. @martinpitt, @jelly, @mvollmer please check. |
|
Fwiw, I also filed https://bugzilla.redhat.com/show_bug.cgi?id=2265630 |
brauner
added a commit
to brauner/linux
that referenced
this pull request
Feb 23, 2024
Moving pidfds from the anonymous inode infrastructure to a separate tiny
in-kernel filesystem similar to sockfs, pipefs, and anon_inodefs causes
Selinux denials and thus various userspace components that make heavy
use of pidfds to fail.
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:>
So far pidfds weren't able to be mediated by selinux which was requested
multiple times. Now that pidfs exists it is actually possible to medite
pidfds because they go through the regular open path that calls the
security_file_open() hook. This is a huge advantage.
Until the Selinux policy is fixed we need to default to n to avoid
breaking people. That process is under way in [1] and [2].
Link: https://bugzilla.redhat.com/show_bug.cgi?id=2265630 [1]
Link: fedora-selinux/selinux-policy#2050 [2]
Reported-by: Nathan Chancellor <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Christian Brauner <[email protected]>
brauner
added a commit
to brauner/linux
that referenced
this pull request
Feb 24, 2024
Moving pidfds from the anonymous inode infrastructure to a separate tiny
in-kernel filesystem similar to sockfs, pipefs, and anon_inodefs causes
Selinux denials and thus various userspace components that make heavy
use of pidfds to fail if we would be using dentry_open() directly.
pidfds used anon_inode_getfile() which never was subject to any LSM
hooks. But dentry_open() is and that would cause regressions:
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:>
The failures that are seen are selinux denials. But the core failure is
dbus-broker. That cascades into other services failing that depend on
dbus-broker. For example, when dbus-broker fails to start polkit and all
the others won't be able to work because they depend on dbus-broker.
The reason for dbus-broker failing is because it doesn't handle failures
for SO_PEERPIDFD correctly. Last kernel release (either v6.7 or v6.6,
I'm not completely sure right now) we introduced SO_PEERPIDFD (and
SCM_PIDFD). SO_PEERPIDFD allows dbus-broker and polkit and others to
receive a pidfd for the peer of an AF_UNIX socket. This is the first
time in the history of Linux that we can safely authenticate clients in
a race-free manner.
dbus-broker immediately made use of this but messed up the error
checking. It only allowed EINVAL as a valid failure for SO_PEERPIDFD.
That's obviously problematic not just because of LSM denials but because
of seccomp denials that would prevent SO_PEERPIDFD from working; or any
other new error code from there.
So this is catching a flawed implementation in dbus-broker as well. It
has to fallback to the old pid-based authentication when SO_PEERPIDFD
doesn't work no matter the reasons otherwise it'll always risk such
failures. So overall that LSM denial should not have caused dbus-broker
to fail. It can never assume that a feature released one kernel ago like
SO_PEERPIDFD can be assumed to be available.
So, the next fix separate from the selinux policy update is to fix
dbus-broker at [3]. That should make it into Fedora as well. In addition
the selinux reference policy should also be updated. See [4] for that.
If Selinux is in enforcing mode in userspace and it encounters anything
that it doesn't know about it will deny it by default. And the policy is
entirely in userspace including declaring new types for stuff like nsfs
or pidfs to allow it. There's just nothing to do in the kernel so we
can't use dentry_open(). So instead we use alloc_file(). Once selinux is
ready we can switch to dentry_open() or we introduce separate LSM hook
for pidfds.
Link: https://bugzilla.redhat.com/show_bug.cgi?id=2265630 [1]
Link: fedora-selinux/selinux-policy#2050 [2]
Link: bus1/dbus-broker#343 [3]
Link: SELinuxProject/refpolicy#762 [4]
Reported-by: Nathan Chancellor <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Link: https://lore.kernel.org/r/20240218-neufahrzeuge-brauhaus-fb0eb6459771@brauner
Signed-off-by: Christian Brauner <[email protected]>
brauner
added a commit
to brauner/linux
that referenced
this pull request
Feb 25, 2024
Moving pidfds from the anonymous inode infrastructure to a separate tiny
in-kernel filesystem similar to sockfs, pipefs, and anon_inodefs causes
Selinux denials and thus various userspace components that make heavy
use of pidfds to fail if we would be using dentry_open() directly.
pidfds used anon_inode_getfile() which never was subject to any LSM
hooks. But dentry_open() is and that would cause regressions:
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:>
The failures that are seen are selinux denials. But the core failure is
dbus-broker. That cascades into other services failing that depend on
dbus-broker. For example, when dbus-broker fails to start polkit and all
the others won't be able to work because they depend on dbus-broker.
The reason for dbus-broker failing is because it doesn't handle failures
for SO_PEERPIDFD correctly. Last kernel release (either v6.7 or v6.6,
I'm not completely sure right now) we introduced SO_PEERPIDFD (and
SCM_PIDFD). SO_PEERPIDFD allows dbus-broker and polkit and others to
receive a pidfd for the peer of an AF_UNIX socket. This is the first
time in the history of Linux that we can safely authenticate clients in
a race-free manner.
dbus-broker immediately made use of this but messed up the error
checking. It only allowed EINVAL as a valid failure for SO_PEERPIDFD.
That's obviously problematic not just because of LSM denials but because
of seccomp denials that would prevent SO_PEERPIDFD from working; or any
other new error code from there.
So this is catching a flawed implementation in dbus-broker as well. It
has to fallback to the old pid-based authentication when SO_PEERPIDFD
doesn't work no matter the reasons otherwise it'll always risk such
failures. So overall that LSM denial should not have caused dbus-broker
to fail. It can never assume that a feature released one kernel ago like
SO_PEERPIDFD can be assumed to be available.
So, the next fix separate from the selinux policy update is to fix
dbus-broker at [3]. That should make it into Fedora as well. In addition
the selinux reference policy should also be updated. See [4] for that.
If Selinux is in enforcing mode in userspace and it encounters anything
that it doesn't know about it will deny it by default. And the policy is
entirely in userspace including declaring new types for stuff like nsfs
or pidfs to allow it. There's just nothing to do in the kernel so we
can't use dentry_open(). So instead we use alloc_file(). Once selinux is
ready we can switch to dentry_open() or we introduce separate LSM hook
for pidfds.
Link: https://bugzilla.redhat.com/show_bug.cgi?id=2265630 [1]
Link: fedora-selinux/selinux-policy#2050 [2]
Link: bus1/dbus-broker#343 [3]
Link: SELinuxProject/refpolicy#762 [4]
Reported-by: Nathan Chancellor <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Link: https://lore.kernel.org/r/20240218-neufahrzeuge-brauhaus-fb0eb6459771@brauner
Signed-off-by: Christian Brauner <[email protected]>
|
Closing this since I now filed a bugzilla entry. |
|
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. |
|
I believe we should do the same as refpolicy, i.e. use |
|
I have a concern, though... if we do use 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 |
|
Sounds good to me. |
brauner
added a commit
to brauner/linux
that referenced
this pull request
Feb 27, 2024
Moving pidfds from the anonymous inode infrastructure to a separate tiny
in-kernel filesystem similar to sockfs, pipefs, and anon_inodefs causes
Selinux denials and thus various userspace components that make heavy
use of pidfds to fail if we would be using dentry_open() directly.
pidfds used anon_inode_getfile() which never was subject to any LSM
hooks. But dentry_open() is and that would cause regressions:
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:>
The failures that are seen are selinux denials. But the core failure is
dbus-broker. That cascades into other services failing that depend on
dbus-broker. For example, when dbus-broker fails to start polkit and all
the others won't be able to work because they depend on dbus-broker.
The reason for dbus-broker failing is because it doesn't handle failures
for SO_PEERPIDFD correctly. Last kernel release (either v6.7 or v6.6,
I'm not completely sure right now) we introduced SO_PEERPIDFD (and
SCM_PIDFD). SO_PEERPIDFD allows dbus-broker and polkit and others to
receive a pidfd for the peer of an AF_UNIX socket. This is the first
time in the history of Linux that we can safely authenticate clients in
a race-free manner.
dbus-broker immediately made use of this but messed up the error
checking. It only allowed EINVAL as a valid failure for SO_PEERPIDFD.
That's obviously problematic not just because of LSM denials but because
of seccomp denials that would prevent SO_PEERPIDFD from working; or any
other new error code from there.
So this is catching a flawed implementation in dbus-broker as well. It
has to fallback to the old pid-based authentication when SO_PEERPIDFD
doesn't work no matter the reasons otherwise it'll always risk such
failures. So overall that LSM denial should not have caused dbus-broker
to fail. It can never assume that a feature released one kernel ago like
SO_PEERPIDFD can be assumed to be available.
So, the next fix separate from the selinux policy update is to fix
dbus-broker at [3]. That should make it into Fedora as well. In addition
the selinux reference policy should also be updated. See [4] for that.
If Selinux is in enforcing mode in userspace and it encounters anything
that it doesn't know about it will deny it by default. And the policy is
entirely in userspace including declaring new types for stuff like nsfs
or pidfs to allow it. So for now we continue to raise S_PRIVATE on the
inode if it's a pidfs inode.
Link: https://bugzilla.redhat.com/show_bug.cgi?id=2265630 [1]
Link: fedora-selinux/selinux-policy#2050 [2]
Link: bus1/dbus-broker#343 [3]
Link: SELinuxProject/refpolicy#762 [4]
Reported-by: Nathan Chancellor <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Link: https://lore.kernel.org/r/20240218-neufahrzeuge-brauhaus-fb0eb6459771@brauner
Signed-off-by: Christian Brauner <[email protected]>
brauner
added a commit
to brauner/linux
that referenced
this pull request
Feb 28, 2024
Moving pidfds from the anonymous inode infrastructure to a separate tiny in-kernel filesystem similar to sockfs, pipefs, and anon_inodefs causes selinux denials and thus various userspace components that make heavy use of pidfds to fail as pidfds used anon_inode_getfile() which aren't subject to any LSM hooks. But dentry_open() is and that would cause regressions. The failures that are seen are selinux denials. But the core failure is dbus-broker. That cascades into other services failing that depend on dbus-broker. For example, when dbus-broker fails to start polkit and all the others won't be able to work because they depend on dbus-broker. The reason for dbus-broker failing is because it doesn't handle failures for SO_PEERPIDFD correctly. Last kernel release we introduced SO_PEERPIDFD (and SCM_PIDFD). SO_PEERPIDFD allows dbus-broker and polkit and others to receive a pidfd for the peer of an AF_UNIX socket. This is the first time in the history of Linux that we can safely authenticate clients in a race-free manner. dbus-broker immediately made use of this but messed up the error checking. It only allowed EINVAL as a valid failure for SO_PEERPIDFD. That's obviously problematic not just because of LSM denials but because of seccomp denials that would prevent SO_PEERPIDFD from working; or any other new error code from there. So this is catching a flawed implementation in dbus-broker as well. It has to fallback to the old pid-based authentication when SO_PEERPIDFD doesn't work no matter the reasons otherwise it'll always risk such failures. So overall that LSM denial should not have caused dbus-broker to fail. It can never assume that a feature released one kernel ago like SO_PEERPIDFD can be assumed to be available. So, the next fix separate from the selinux policy update is to try and fix dbus-broker at [3]. That should make it into Fedora as well. In addition the selinux reference policy should also be updated. See [4] for that. If Selinux is in enforcing mode in userspace and it encounters anything that it doesn't know about it will deny it by default. And the policy is entirely in userspace including declaring new types for stuff like nsfs or pidfs to allow it. For now we continue to raise S_PRIVATE on the inode if it's a pidfs inode which means things behave exactly like before. Link: https://bugzilla.redhat.com/show_bug.cgi?id=2265630 Link: fedora-selinux/selinux-policy#2050 Link: bus1/dbus-broker#343 [3] Link: SELinuxProject/refpolicy#762 [4] Reported-by: Nathan Chancellor <[email protected]> Link: https://lore.kernel.org/r/[email protected] Link: https://lore.kernel.org/r/20240218-neufahrzeuge-brauhaus-fb0eb6459771@brauner Signed-off-by: Christian Brauner <[email protected]>
brauner
added a commit
to brauner/linux
that referenced
this pull request
Mar 1, 2024
Moving pidfds from the anonymous inode infrastructure to a separate tiny in-kernel filesystem similar to sockfs, pipefs, and anon_inodefs causes selinux denials and thus various userspace components that make heavy use of pidfds to fail as pidfds used anon_inode_getfile() which aren't subject to any LSM hooks. But dentry_open() is and that would cause regressions. The failures that are seen are selinux denials. But the core failure is dbus-broker. That cascades into other services failing that depend on dbus-broker. For example, when dbus-broker fails to start polkit and all the others won't be able to work because they depend on dbus-broker. The reason for dbus-broker failing is because it doesn't handle failures for SO_PEERPIDFD correctly. Last kernel release we introduced SO_PEERPIDFD (and SCM_PIDFD). SO_PEERPIDFD allows dbus-broker and polkit and others to receive a pidfd for the peer of an AF_UNIX socket. This is the first time in the history of Linux that we can safely authenticate clients in a race-free manner. dbus-broker immediately made use of this but messed up the error checking. It only allowed EINVAL as a valid failure for SO_PEERPIDFD. That's obviously problematic not just because of LSM denials but because of seccomp denials that would prevent SO_PEERPIDFD from working; or any other new error code from there. So this is catching a flawed implementation in dbus-broker as well. It has to fallback to the old pid-based authentication when SO_PEERPIDFD doesn't work no matter the reasons otherwise it'll always risk such failures. So overall that LSM denial should not have caused dbus-broker to fail. It can never assume that a feature released one kernel ago like SO_PEERPIDFD can be assumed to be available. So, the next fix separate from the selinux policy update is to try and fix dbus-broker at [3]. That should make it into Fedora as well. In addition the selinux reference policy should also be updated. See [4] for that. If Selinux is in enforcing mode in userspace and it encounters anything that it doesn't know about it will deny it by default. And the policy is entirely in userspace including declaring new types for stuff like nsfs or pidfs to allow it. For now we continue to raise S_PRIVATE on the inode if it's a pidfs inode which means things behave exactly like before. Link: https://bugzilla.redhat.com/show_bug.cgi?id=2265630 Link: fedora-selinux/selinux-policy#2050 Link: bus1/dbus-broker#343 [3] Link: SELinuxProject/refpolicy#762 [4] Reported-by: Nathan Chancellor <[email protected]> Link: https://lore.kernel.org/r/[email protected] Link: https://lore.kernel.org/r/20240218-neufahrzeuge-brauhaus-fb0eb6459771@brauner Signed-off-by: Christian Brauner <[email protected]>
|
Merging, thank you. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:>