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

[Chuck Lever III <chuck.lever@oracle.com>]

> 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