Re: [PATCH] xfs: use has_capability_noaudit() instead of capable() where appropriate

From: Dave Chinner <david@fromorbit.com>
To: Ondrej Mosnacek <omosnace@redhat.com>
Cc: "Darrick J . Wong" <darrick.wong@oracle.com>,
	linux-xfs@vger.kernel.org,
	Linux Security Module list 
	<linux-security-module@vger.kernel.org>,
	SElinux list <selinux@vger.kernel.org>
Subject: Re: [PATCH] xfs: use has_capability_noaudit() instead of capable() where appropriate
Date: Fri, 19 Mar 2021 17:00:42 +1100	[thread overview]
Message-ID: <20210319060042.GS63242@dread.disaster.area> (raw)
In-Reply-To: <CAFqZXNv5GPCU040gO3s-o2UTkXF3HExSkx2AjzE+4VC1REsQBg@mail.gmail.com>

On Thu, Mar 18, 2021 at 10:51:29AM +0100, Ondrej Mosnacek wrote:
> On Tue, Mar 16, 2021 at 10:09 PM Dave Chinner <david@fromorbit.com> wrote:
> > On Tue, Mar 16, 2021 at 06:32:26PM +0100, Ondrej Mosnacek wrote:
> > > diff --git a/fs/xfs/xfs_xattr.c b/fs/xfs/xfs_xattr.c
> > > index bca48b308c02..a99d19c2c11f 100644
> > > --- a/fs/xfs/xfs_xattr.c
> > > +++ b/fs/xfs/xfs_xattr.c
> > > @@ -164,7 +164,7 @@ xfs_xattr_put_listent(
> > >                * Only show root namespace entries if we are actually allowed to
> > >                * see them.
> > >                */
> > > -             if (!capable(CAP_SYS_ADMIN))
> > > +             if (!has_capability_noaudit(current, CAP_SYS_ADMIN))
> > >                       return;
> > >
> > >               prefix = XATTR_TRUSTED_PREFIX;
> >
> > This one should absolutely report a denial - someone has tried to
> > read the trusted xattr namespace without permission to do so. That's
> > exactly the sort of thing I'd want to see in an audit log - just
> > because we just elide the xattrs rather than return an error doesn't
> > mean we should not leave an audit trail from the attempted access
> > of kernel trusted attributes...
> 
> I'm not sure about that... without CAP_SYS_ADMIN the caller would
> still get the ACL xattrs, no?

Looks like it, but I have no idea if that's even correct behaviour
or not - access to posix ACL is supposed to be controlled by
the VFS.

> IIUC, it's a filter to not show
> restricted xattrs to unprivileged users via listxattr(2)**, where the
> user is not saying "give me the trusted xattrs", just "give me
> whatever I'm allowed to see", so logging the denial wouldn't make much
> sense - the user may not even care about trusted xattrs when doing the
> syscall (and in 99% of cases a user without CAP_SYS_ADMIN really
> won't).

Ok, I keep forgetting that only XFS has attr_list(3) interfaces
(which predate Linux VFS/syscall xattr support, IIRC) and that
interface requires userspace to pass ATTR_ROOT to retrieve trusted
namespace xattrs...

For while it's a silent content filter for one user interface, it's
an explicit request from another user interface. Make of that what
you will....

> (**) But I don't understand how exactly that function is used and what
> the XFS_ATTR_ROOT flag means, so I may be getting it wrong...

It's the on-disk format flag that says the xattr is in the "root"
namespace (i.e. requires "root" permissions to access). XFS came
from Irix which had different xattr namespaces for system, security,
etc, hence the stuff is stored on XFS is named somewhat differently
compared to native linux filesystems....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com