diff mbox series

[v2] fs: don't audit the capability check in simple_xattr_list()

Message ID 20221103151205.702826-1-omosnace@redhat.com
State New
Headers
Series [v2] fs: don't audit the capability check in simple_xattr_list() |

Commit Message

Ondrej Mosnacek Nov. 3, 2022, 3:12 p.m. UTC 
  The check being unconditional may lead to unwanted denials reported by
LSMs when a process has the capability granted by DAC, but denied by an
LSM. In the case of SELinux such denials are a problem, since they can't
be effectively filtered out via the policy and when not silenced, they
produce noise that may hide a true problem or an attack.

Checking for the capability only if any trusted xattr is actually
present wouldn't really address the issue, since calling listxattr(2) on
such node on its own doesn't indicate an explicit attempt to see the
trusted xattrs. Additionally, it could potentially leak the presence of
trusted xattrs to an unprivileged user if they can check for the denials
(e.g. through dmesg).

Therefore, it's best (and simplest) to keep the check unconditional and
instead use ns_capable_noaudit() that will silence any associated LSM
denials.

Fixes: 38f38657444d ("xattr: extract simple_xattr code from tmpfs")
Reported-by: Martin Pitt <mpitt@redhat.com>
Suggested-by: Christian Brauner (Microsoft) <brauner@kernel.org>
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---

v1 -> v2: switch to simpler and better solution as suggested by Christian

v1: https://lore.kernel.org/selinux/CAFqZXNuC7c0Ukx_okYZ7rsKycQY5P1zpMPmmq_T5Qyzbg-x7yQ@mail.gmail.com/T/

 fs/xattr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Christian Brauner Nov. 3, 2022, 3:30 p.m. UTC | #1 
On Thu, Nov 03, 2022 at 04:12:05PM +0100, Ondrej Mosnacek wrote:
> The check being unconditional may lead to unwanted denials reported by
> LSMs when a process has the capability granted by DAC, but denied by an
> LSM. In the case of SELinux such denials are a problem, since they can't
> be effectively filtered out via the policy and when not silenced, they
> produce noise that may hide a true problem or an attack.
> 
> Checking for the capability only if any trusted xattr is actually
> present wouldn't really address the issue, since calling listxattr(2) on
> such node on its own doesn't indicate an explicit attempt to see the
> trusted xattrs. Additionally, it could potentially leak the presence of
> trusted xattrs to an unprivileged user if they can check for the denials
> (e.g. through dmesg).
> 
> Therefore, it's best (and simplest) to keep the check unconditional and
> instead use ns_capable_noaudit() that will silence any associated LSM
> denials.
> 
> Fixes: 38f38657444d ("xattr: extract simple_xattr code from tmpfs")
> Reported-by: Martin Pitt <mpitt@redhat.com>
> Suggested-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---

Looks good,
Reviewed-by: Christian Brauner (Microsoft) <brauner@kernel.org>
Paul Moore Nov. 5, 2022, 4:38 a.m. UTC | #2 
On Thu, Nov 3, 2022 at 11:13 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> The check being unconditional may lead to unwanted denials reported by
> LSMs when a process has the capability granted by DAC, but denied by an
> LSM. In the case of SELinux such denials are a problem, since they can't
> be effectively filtered out via the policy and when not silenced, they
> produce noise that may hide a true problem or an attack.
>
> Checking for the capability only if any trusted xattr is actually
> present wouldn't really address the issue, since calling listxattr(2) on
> such node on its own doesn't indicate an explicit attempt to see the
> trusted xattrs. Additionally, it could potentially leak the presence of
> trusted xattrs to an unprivileged user if they can check for the denials
> (e.g. through dmesg).
>
> Therefore, it's best (and simplest) to keep the check unconditional and
> instead use ns_capable_noaudit() that will silence any associated LSM
> denials.
>
> Fixes: 38f38657444d ("xattr: extract simple_xattr code from tmpfs")
> Reported-by: Martin Pitt <mpitt@redhat.com>
> Suggested-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> ---
>
> v1 -> v2: switch to simpler and better solution as suggested by Christian
>
> v1: https://lore.kernel.org/selinux/CAFqZXNuC7c0Ukx_okYZ7rsKycQY5P1zpMPmmq_T5Qyzbg-x7yQ@mail.gmail.com/T/
>
>  fs/xattr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

VFS folks, this should really go through a vfs tree, but if nobody
wants to pick it up *and* there are no objections to the change, I can
take this via the LSM tree.

Reviewed-by: Paul Moore <paul@paul-moore.com>
Christian Brauner Nov. 5, 2022, 11:34 a.m. UTC | #3 
On Sat, Nov 05, 2022 at 12:38:57AM -0400, Paul Moore wrote:
> On Thu, Nov 3, 2022 at 11:13 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > The check being unconditional may lead to unwanted denials reported by
> > LSMs when a process has the capability granted by DAC, but denied by an
> > LSM. In the case of SELinux such denials are a problem, since they can't
> > be effectively filtered out via the policy and when not silenced, they
> > produce noise that may hide a true problem or an attack.
> >
> > Checking for the capability only if any trusted xattr is actually
> > present wouldn't really address the issue, since calling listxattr(2) on
> > such node on its own doesn't indicate an explicit attempt to see the
> > trusted xattrs. Additionally, it could potentially leak the presence of
> > trusted xattrs to an unprivileged user if they can check for the denials
> > (e.g. through dmesg).
> >
> > Therefore, it's best (and simplest) to keep the check unconditional and
> > instead use ns_capable_noaudit() that will silence any associated LSM
> > denials.
> >
> > Fixes: 38f38657444d ("xattr: extract simple_xattr code from tmpfs")
> > Reported-by: Martin Pitt <mpitt@redhat.com>
> > Suggested-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >
> > v1 -> v2: switch to simpler and better solution as suggested by Christian
> >
> > v1: https://lore.kernel.org/selinux/CAFqZXNuC7c0Ukx_okYZ7rsKycQY5P1zpMPmmq_T5Qyzbg-x7yQ@mail.gmail.com/T/
> >
> >  fs/xattr.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> VFS folks, this should really go through a vfs tree, but if nobody
> wants to pick it up *and* there are no objections to the change, I can
> take this via the LSM tree.

I can pick this up as I'm currently massaging the simple xattr
infrastructure. I think the fix is pretty straightforward otherwise.

Christian
Paul Moore Nov. 6, 2022, 10:50 p.m. UTC | #4 
On Sat, Nov 5, 2022 at 7:34 AM Christian Brauner <brauner@kernel.org> wrote:
>
> On Sat, Nov 05, 2022 at 12:38:57AM -0400, Paul Moore wrote:
> > On Thu, Nov 3, 2022 at 11:13 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > >
> > > The check being unconditional may lead to unwanted denials reported by
> > > LSMs when a process has the capability granted by DAC, but denied by an
> > > LSM. In the case of SELinux such denials are a problem, since they can't
> > > be effectively filtered out via the policy and when not silenced, they
> > > produce noise that may hide a true problem or an attack.
> > >
> > > Checking for the capability only if any trusted xattr is actually
> > > present wouldn't really address the issue, since calling listxattr(2) on
> > > such node on its own doesn't indicate an explicit attempt to see the
> > > trusted xattrs. Additionally, it could potentially leak the presence of
> > > trusted xattrs to an unprivileged user if they can check for the denials
> > > (e.g. through dmesg).
> > >
> > > Therefore, it's best (and simplest) to keep the check unconditional and
> > > instead use ns_capable_noaudit() that will silence any associated LSM
> > > denials.
> > >
> > > Fixes: 38f38657444d ("xattr: extract simple_xattr code from tmpfs")
> > > Reported-by: Martin Pitt <mpitt@redhat.com>
> > > Suggested-by: Christian Brauner (Microsoft) <brauner@kernel.org>
> > > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > > ---
> > >
> > > v1 -> v2: switch to simpler and better solution as suggested by Christian
> > >
> > > v1: https://lore.kernel.org/selinux/CAFqZXNuC7c0Ukx_okYZ7rsKycQY5P1zpMPmmq_T5Qyzbg-x7yQ@mail.gmail.com/T/
> > >
> > >  fs/xattr.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > VFS folks, this should really go through a vfs tree, but if nobody
> > wants to pick it up *and* there are no objections to the change, I can
> > take this via the LSM tree.
>
> I can pick this up as I'm currently massaging the simple xattr
> infrastructure.

Thanks Christian.

> I think the fix is pretty straightforward otherwise.

Agreed.
diff mbox series

Patch

diff --git a/fs/xattr.c b/fs/xattr.c
index 61107b6bbed2..427b8cea1f96 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -1140,7 +1140,7 @@  static int xattr_list_one(char **buffer, ssize_t *remaining_size,
 ssize_t simple_xattr_list(struct inode *inode, struct simple_xattrs *xattrs,
 			  char *buffer, size_t size)
 {
-	bool trusted = capable(CAP_SYS_ADMIN);
+	bool trusted = ns_capable_noaudit(&init_user_ns, CAP_SYS_ADMIN);
 	struct simple_xattr *xattr;
 	ssize_t remaining_size = size;
 	int err = 0;