filesystem: Add labeling for pidfs. #762
Draft
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
Signed-off-by: Chris PeBenito <[email protected]>
|
Need to decide if we should use task sid or a single common genfs label, like anon_inode. @stephensmalley @jwcart2 @pcmoore if you have thoughts |
|
This will also allow you to do fine-grained labeling finally because it now goes through the regular |
|
This requires https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/fs/pidfs.c?h=next-20240223&id=a1a466d5af6c8ed03bfa30305b89cfe8bce1f3f9 (so linux-next for now) |
|
As a pidfd is a reference to a task, I believe it should use the task's SID/label. |
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]>
Signed-off-by: Chris PeBenito <[email protected]>
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]>
"undesirable" is certainly a mild way to put it ;) It's definitely wrong and something we need to resolve. I'll take a look it as I suspect this is going to require kernel code to handle properly. |
I don't understand what that is supposed to mean? The pidfd entries are anonymous inodes. They don't have any process credentials attached to them? I was talking about |
Documentation/filesystems/credentials.rst |
It might be my lack of knowledge injecting confusion into this. I'll leave it to Paul and the other kernel devs to clarify. |
|
I haven't had a chance to look at any code yet, I'm basing my comments purely on what has been written here and the pidfs patchset cover letter, but you've explicitly mentioned moving away from the anonymous inode infrastructure @brauner:
... if this isn't the case, that would be good to know. |
Anonymous inodes are just inodes marked with S_PRIVATE, so no we're not moving away from that. |
|
We're just using our own superblock just like |
Sure, but you did mention that you were moving away from the anonymous inode infrastructure which could be relevant from a LSM perspective. I still need to check the code. |
|
There is also still the original issue that we likely want to label pidfds based on their associated task, similar to what we do with process entries in procfs. While the concept is independent of the pidfd/pidfs implementation, how we satisfy this requirement is obviously very implementation specific. |
|
Sure https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.pidfd |
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]>
|
This PR has not had any recent activity. It will be closed in 7 days if it makes no further progress. |
|
This PR has not had any recent activity. It will be closed in 7 days if it makes no further progress. |
|
Closing stale PR. |
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.
Add task SID labeling for
pidfs, which is the new backing pseudo filesystem forpidfds.The existing rules will allow domains to open pidfds and use them internally, but other domains will require additional access (
fdandfile) when passing around the pidfd.