From: Ondrej Mosnacek <omosnace@redhat.com>
To: Dave Chinner <david@fromorbit.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: Thu, 18 Mar 2021 10:51:29 +0100 [thread overview]
Message-ID: <CAFqZXNv5GPCU040gO3s-o2UTkXF3HExSkx2AjzE+4VC1REsQBg@mail.gmail.com> (raw)
In-Reply-To: <20210316205010.GN63242@dread.disaster.area>
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:
> > In cases when a negative result of a capability check doesn't lead to an
> > immediate, user-visible error, only a subtle difference in behavior, it
> > is better to use has_capability_noaudit(current, ...), so that LSMs
> > (e.g. SELinux) don't generate a denial record in the audit log each time
> > the capability status is queried. This patch should cover all such cases
> > in fs/xfs/.
>
> Is this something new? I only see 4 calls to
> has_capability_noaudit() in 5.12-rc3...
I don't know all the history, but I don't think it's new. It's just
that few people really are aware of the difference and no one from the
LSM/SELinux cared enough to maintain proper use across the kernel...
>
> Also, has_capability_noaudit() is an awful name. capable_noaudit()
> would actually be self explanatory to anyone who is used to doing
> capability checks via capable(), ns_capable(), ns_capable_noaudit(),
> inode_owner_or_capable(), capable_wrt_inode_uidgid(), etc...
>
> Please fix the name of this function to be consistent with the
> existing capability APIs before propagating it further into the
> kernel.
That's a fair point - I should take this opportunity to add the
missing function and add some documentation... I'll make sure to do
better in v2.
>
> > Note that I kept the capable(CAP_FSETID) checks, since these will only
> > be executed if the user explicitly tries to set the SUID/SGID bit, and
> > it likely makes sense to log such attempts even if the syscall doesn't
> > fail and just ignores the bits.
>
> So how on earth are we supposed to maintain this code correctly?
> These are undocumented rules that seemingly are applied to random
> subsystems and to seemingly random capable() calls in those
> subsystems. ANd you don't even document it in this code where there
> are other capable(...) checks that will generate audit records...
>
> How are we supposed to know when an audit record should be emitted
> or not by some unknown LSM when we do a capability check?
> Capabilities are already an awful nightmare maze of similar but
> slightly different capability checks, and this doesn't improve the
> situation at all.
>
> Please make this easier to get right iand maintain correctly (an
> absolute, non-negotiable requirement for anything security related)
> before you spray yet another poorly documented capability function
> into the wider kernel.
Again, you're right that I shouldn't have taken the lazy path :)
>
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> > fs/xfs/xfs_fsmap.c | 4 ++--
> > fs/xfs/xfs_ioctl.c | 5 ++++-
> > fs/xfs/xfs_iops.c | 6 ++++--
> > fs/xfs/xfs_xattr.c | 2 +-
> > 4 files changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/fs/xfs/xfs_fsmap.c b/fs/xfs/xfs_fsmap.c
> > index 9ce5e7d5bf8f..14672e7ee535 100644
> > --- a/fs/xfs/xfs_fsmap.c
> > +++ b/fs/xfs/xfs_fsmap.c
> > @@ -842,8 +842,8 @@ xfs_getfsmap(
> > !xfs_getfsmap_is_valid_device(mp, &head->fmh_keys[1]))
> > return -EINVAL;
> >
> > - use_rmap = capable(CAP_SYS_ADMIN) &&
> > - xfs_sb_version_hasrmapbt(&mp->m_sb);
> > + use_rmap = xfs_sb_version_hasrmapbt(&mp->m_sb) &&
> > + has_capability_noaudit(current, CAP_SYS_ADMIN);
> > head->fmh_entries = 0;
> >
> > /* Set up our device handlers. */
> > diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
> > index 3fbd98f61ea5..3cfc1a25069c 100644
> > --- a/fs/xfs/xfs_ioctl.c
> > +++ b/fs/xfs/xfs_ioctl.c
> > @@ -1470,8 +1470,11 @@ xfs_ioctl_setattr(
> >
> > if (XFS_IS_QUOTA_RUNNING(mp) && XFS_IS_PQUOTA_ON(mp) &&
> > ip->i_d.di_projid != fa->fsx_projid) {
> > + int flags = has_capability_noaudit(current, CAP_FOWNER) ?
> > + XFS_QMOPT_FORCE_RES : 0;
> > +
> > code = xfs_qm_vop_chown_reserve(tp, ip, NULL, NULL, pdqp,
> > - capable(CAP_FOWNER) ? XFS_QMOPT_FORCE_RES : 0);
> > + flags);
> > if (code) /* out of quota */
> > goto error_trans_cancel;
> > }
>
> You missed a capable() call here - see the call to
> xfs_trans_alloc_ichange( ... capabale(CAP_FOWNER), ...); in
> xfs_ioctl_setattr_get_trans().
Ah, I mistakenly based the path against an old tree. Sorry, I'll redo
it against xfs/for-next...
>
> > diff --git a/fs/xfs/xfs_iops.c b/fs/xfs/xfs_iops.c
> > index 67c8dc9de8aa..abbb417c4fbd 100644
> > --- a/fs/xfs/xfs_iops.c
> > +++ b/fs/xfs/xfs_iops.c
> > @@ -729,10 +729,12 @@ xfs_setattr_nonsize(
> > if (XFS_IS_QUOTA_RUNNING(mp) &&
> > ((XFS_IS_UQUOTA_ON(mp) && !uid_eq(iuid, uid)) ||
> > (XFS_IS_GQUOTA_ON(mp) && !gid_eq(igid, gid)))) {
> > + int flags = has_capability_noaudit(current, CAP_FOWNER) ?
> > + XFS_QMOPT_FORCE_RES : 0;
> > +
> > ASSERT(tp);
> > error = xfs_qm_vop_chown_reserve(tp, ip, udqp, gdqp,
> > - NULL, capable(CAP_FOWNER) ?
> > - XFS_QMOPT_FORCE_RES : 0);
> > + NULL, flags);
> > if (error) /* out of quota */
> > goto out_cancel;
> > }
>
> You missed a capable() call here - see the call to
> xfs_trans_alloc_ichange( ... capabale(CAP_FOWNER), ...); in this
> function.
>
> I think this demonstrates just how fragile and hard to maintain the
> approach being taken here is.
>
> > 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? 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).
(**) 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...
--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
next prev parent reply other threads:[~2021-03-18 9:52 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-16 17:32 [PATCH] xfs: use has_capability_noaudit() instead of capable() where appropriate Ondrej Mosnacek
2021-03-16 20:50 ` Dave Chinner
2021-03-18 9:51 ` Ondrej Mosnacek [this message]
2021-03-19 6:00 ` Dave Chinner
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAFqZXNv5GPCU040gO3s-o2UTkXF3HExSkx2AjzE+4VC1REsQBg@mail.gmail.com \
--to=omosnace@redhat.com \
--cc=darrick.wong@oracle.com \
--cc=david@fromorbit.com \
--cc=linux-security-module@vger.kernel.org \
--cc=linux-xfs@vger.kernel.org \
--cc=selinux@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).