diff mbox series

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

Message ID 20240828195129.223395-2-smayhew@redhat.com (mailing list archive)
State Handled Elsewhere
Delegated to: Paul Moore
Headers show
Series selinux,smack: don't bypass permissions check in inode_setsecctx hook | expand

Commit Message

Scott Mayhew Aug. 28, 2024, 7:51 p.m. UTC
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@protonmail.com>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=218809
Signed-off-by: Scott Mayhew <smayhew@redhat.com>
---
 security/selinux/hooks.c   | 4 ++--
 security/smack/smack_lsm.c | 4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

Comments

Jeff Layton Aug. 28, 2024, 8:03 p.m. UTC | #1
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@protonmail.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218809
> Signed-off-by: Scott Mayhew <smayhew@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)

Acked-by: Jeff Layton <jlayton@kernel.org>
Paul Moore Aug. 28, 2024, 9:05 p.m. UTC | #2
On Wed, Aug 28, 2024 at 3:51 PM Scott Mayhew <smayhew@redhat.com> 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@protonmail.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218809
> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> ---
>  security/selinux/hooks.c   | 4 ++--
>  security/smack/smack_lsm.c | 4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)

Thanks Scott, this looks good to me, but since it touches Smack too
I'd also like to get Casey's ACK on this patch; if for some reason we
don't hear from Casey after a bit I'll go ahead and merge it.
Speaking of merging, since this touches both SELinux and Smack I'll
likely pull this in via the LSM tree, with a marking for the stable
kernels, if anyone has any objections to that please let me know.
Chuck Lever Aug. 28, 2024, 9:08 p.m. UTC | #3
On Wed, Aug 28, 2024 at 03:51:29PM -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@protonmail.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218809
> Signed-off-by: Scott Mayhew <smayhew@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)
> -- 
> 2.46.0
> 

Nice, thorough work, Scott.

Reviewed-by: Chuck Lever <chuck.lever@oracle.com>
Casey Schaufler Aug. 28, 2024, 9:28 p.m. UTC | #4
On 8/28/2024 2:05 PM, Paul Moore wrote:
> On Wed, Aug 28, 2024 at 3:51 PM Scott Mayhew <smayhew@redhat.com> 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@protonmail.com>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218809
>> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
>> ---
>>  security/selinux/hooks.c   | 4 ++--
>>  security/smack/smack_lsm.c | 4 ++--
>>  2 files changed, 4 insertions(+), 4 deletions(-)
> Thanks Scott, this looks good to me, but since it touches Smack too
> I'd also like to get Casey's ACK on this patch;

Testing labeled NFS has always been a challenge for the somewhat
limited resources available to the Smack project. I'll Ack the patch,
but won't claim to have tested it.

>  if for some reason we
> don't hear from Casey after a bit I'll go ahead and merge it.
> Speaking of merging, since this touches both SELinux and Smack I'll
> likely pull this in via the LSM tree, with a marking for the stable
> kernels, if anyone has any objections to that please let me know.
>
Paul Moore Aug. 28, 2024, 11:08 p.m. UTC | #5
On Wed, Aug 28, 2024 at 5:29 PM Casey Schaufler <casey@schaufler-ca.com> wrote:
> On 8/28/2024 2:05 PM, Paul Moore wrote:
> > On Wed, Aug 28, 2024 at 3:51 PM Scott Mayhew <smayhew@redhat.com> 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@protonmail.com>
> >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218809
> >> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> >> ---
> >>  security/selinux/hooks.c   | 4 ++--
> >>  security/smack/smack_lsm.c | 4 ++--
> >>  2 files changed, 4 insertions(+), 4 deletions(-)
> > Thanks Scott, this looks good to me, but since it touches Smack too
> > I'd also like to get Casey's ACK on this patch;
>
> Testing labeled NFS has always been a challenge for the somewhat
> limited resources available to the Smack project. I'll Ack the patch,
> but won't claim to have tested it.

Understood, thanks for the quick reply.
Paul Moore Aug. 28, 2024, 11:19 p.m. UTC | #6
On Wed, Aug 28, 2024 at 5:05 PM Paul Moore <paul@paul-moore.com> wrote:
> On Wed, Aug 28, 2024 at 3:51 PM Scott Mayhew <smayhew@redhat.com> 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@protonmail.com>
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=218809
> > Signed-off-by: Scott Mayhew <smayhew@redhat.com>
> > ---
> >  security/selinux/hooks.c   | 4 ++--
> >  security/smack/smack_lsm.c | 4 ++--
> >  2 files changed, 4 insertions(+), 4 deletions(-)
>
> Thanks Scott, this looks good to me, but since it touches Smack too
> I'd also like to get Casey's ACK on this patch; if for some reason we
> don't hear from Casey after a bit I'll go ahead and merge it.
> Speaking of merging, since this touches both SELinux and Smack I'll
> likely pull this in via the LSM tree, with a marking for the stable
> kernels, if anyone has any objections to that please let me know.

Merged into lsm/stable-6.11 so we can get this into linux-next and the
automated SELinux testing, assuming all goes we'll I'll send this up
to Linus later this week.  Thanks all!
Jeff Layton Aug. 29, 2024, 11:15 a.m. UTC | #7
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@protonmail.com>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218809
> Signed-off-by: Scott Mayhew <smayhew@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@kernel.org>
Stephen Smalley Aug. 29, 2024, 1:03 p.m. UTC | #8
On Thu, Aug 29, 2024 at 7:15 AM Jeff Layton <jlayton@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@protonmail.com>
> > Link: https://bugzilla.kernel.org/show_bug.cgi?id=218809
> > Signed-off-by: Scott Mayhew <smayhew@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@kernel.org>

Reviewed-by: Stephen Smalley <stephen.smalley.work@gmail.com>
Tested-by: Stephen Smalley <stephen.smalley.work@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).
Chuck Lever Aug. 29, 2024, 2:16 p.m. UTC | #9
> On Aug 28, 2024, at 7:19 PM, Paul Moore <paul@paul-moore.com> wrote:
> 
> On Wed, Aug 28, 2024 at 5:05 PM Paul Moore <paul@paul-moore.com> wrote:
>> On Wed, Aug 28, 2024 at 3:51 PM Scott Mayhew <smayhew@redhat.com> 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@protonmail.com>
>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=218809
>>> Signed-off-by: Scott Mayhew <smayhew@redhat.com>
>>> ---
>>> security/selinux/hooks.c   | 4 ++--
>>> security/smack/smack_lsm.c | 4 ++--
>>> 2 files changed, 4 insertions(+), 4 deletions(-)
>> 
>> Thanks Scott, this looks good to me, but since it touches Smack too
>> I'd also like to get Casey's ACK on this patch; if for some reason we
>> don't hear from Casey after a bit I'll go ahead and merge it.
>> Speaking of merging, since this touches both SELinux and Smack I'll
>> likely pull this in via the LSM tree, with a marking for the stable
>> kernels, if anyone has any objections to that please let me know.
> 
> Merged into lsm/stable-6.11 so we can get this into linux-next and the
> automated SELinux testing, assuming all goes we'll I'll send this up
> to Linus later this week.  Thanks all!

Paul, may I recommend adding Cc: stable once your testing passes?


--
Chuck Lever
Paul Moore Aug. 29, 2024, 2:51 p.m. UTC | #10
On Thu, Aug 29, 2024 at 10:16 AM Chuck Lever III <chuck.lever@oracle.com> wrote:
> > On Aug 28, 2024, at 7:19 PM, Paul Moore <paul@paul-moore.com> wrote:
> >
> > Merged into lsm/stable-6.11 so we can get this into linux-next and the
> > automated SELinux testing, assuming all goes we'll I'll send this up
> > to Linus later this week.  Thanks all!
>
> Paul, may I recommend adding Cc: stable once your testing passes?

Yep, I did that yesterday before merging.  Typically if I merge
anything into one of the stable-X.Y branches it gets a stable too.
I'll update the commit in a few minutes with the tags from Jeff and
Stephen.

https://git.kernel.org/pub/scm/linux/kernel/git/pcmoore/lsm.git/log/?h=stable-6.11
diff mbox series

Patch

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)