[PATCH 1/1] selinux,smack: don't bypass permissions check in inode_setsecctx hook

[Stephen Smalley]

On Thu, Aug 29, 2024 at 7:15 AM Jeff Layton <jlayton at kernel.org> wrote:
>
> On Wed, 2024-08-28 at 15:51 -0400, Scott Mayhew wrote:
> > Marek Gresko reports that the root user on an NFS client is able to
> > change the security labels on files on an NFS filesystem that is
> > exported with root squashing enabled.
> >
> > The end of the kerneldoc comment for __vfs_setxattr_noperm() states:
> >
> >  *  This function requires the caller to lock the inode's i_mutex before it
> >  *  is executed. It also assumes that the caller will make the appropriate
> >  *  permission checks.
> >
> > nfsd_setattr() does do permissions checking via fh_verify() and
> > nfsd_permission(), but those don't do all the same permissions checks
> > that are done by security_inode_setxattr() and its related LSM hooks do.
> >
> > Since nfsd_setattr() is the only consumer of security_inode_setsecctx(),
> > simplest solution appears to be to replace the call to
> > __vfs_setxattr_noperm() with a call to __vfs_setxattr_locked().  This
> > fixes the above issue and has the added benefit of causing nfsd to
> > recall conflicting delegations on a file when a client tries to change
> > its security label.
> >
> > Reported-by: Marek Gresko <marek.gresko at protonmail.com>
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=218809
> > Signed-off-by: Scott Mayhew <smayhew at redhat.com>
> > ---
> >  security/selinux/hooks.c   | 4 ++--
> >  security/smack/smack_lsm.c | 4 ++--
> >  2 files changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > index bfa61e005aac..400eca4ad0fb 100644
> > --- a/security/selinux/hooks.c
> > +++ b/security/selinux/hooks.c
> > @@ -6660,8 +6660,8 @@ static int selinux_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen
> >   */
> >  static int selinux_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen)
> >  {
> > -     return __vfs_setxattr_noperm(&nop_mnt_idmap, dentry, XATTR_NAME_SELINUX,
> > -                                  ctx, ctxlen, 0);
> > +     return __vfs_setxattr_locked(&nop_mnt_idmap, dentry, XATTR_NAME_SELINUX,
> > +                                  ctx, ctxlen, 0, NULL);
> >  }
> >
> >  static int selinux_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
> > diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> > index 4164699cd4f6..002a1b9ed83a 100644
> > --- a/security/smack/smack_lsm.c
> > +++ b/security/smack/smack_lsm.c
> > @@ -4880,8 +4880,8 @@ static int smack_inode_notifysecctx(struct inode *inode, void *ctx, u32 ctxlen)
> >
> >  static int smack_inode_setsecctx(struct dentry *dentry, void *ctx, u32 ctxlen)
> >  {
> > -     return __vfs_setxattr_noperm(&nop_mnt_idmap, dentry, XATTR_NAME_SMACK,
> > -                                  ctx, ctxlen, 0);
> > +     return __vfs_setxattr_locked(&nop_mnt_idmap, dentry, XATTR_NAME_SMACK,
> > +                                  ctx, ctxlen, 0, NULL);
> >  }
> >
> >  static int smack_inode_getsecctx(struct inode *inode, void **ctx, u32 *ctxlen)
>
> Reviewed-by: Jeff Layton <jlayton at kernel.org>

Reviewed-by: Stephen Smalley <stephen.smalley.work at gmail.com>
Tested-by: Stephen Smalley <stephen.smalley.work at gmail.com>

Passes all the NFS tests in the selinux-testsuite, and also correctly
denies root the ability to relabel a file on a root_squash mount but
allows a normal user to do so (as expected).