Calls to vfs_setlease() from NFSD code cause unnecessary CAP_LEASE security checks

Calls to vfs_setlease() from NFSD code cause unnecessary CAP_LEASE security checks

[Ondrej Mosnacek]

Hello,

In [1] a user reports seeing SELinux denials from NFSD when it writes
into /proc/fs/nfsd/threads with the following kernel backtrace:
 => trace_event_raw_event_selinux_audited
 => avc_audit_post_callback
 => common_lsm_audit
 => slow_avc_audit
 => cred_has_capability.isra.0
 => security_capable
 => capable
 => generic_setlease
 => destroy_unhashed_deleg
 => __destroy_client
 => nfs4_state_shutdown_net
 => nfsd_shutdown_net
 => nfsd_last_thread
 => nfsd_svc
 => write_threads
 => nfsctl_transaction_write
 => vfs_write
 => ksys_write
 => do_syscall_64
 => entry_SYSCALL_64_after_hwframe

It seems to me that the security checks in generic_setlease() should
be skipped (at least) when called through this codepath, since the
userspace process merely writes into /proc/fs/nfsd/threads and it's
just the kernel's internal code that releases the lease as a side
effect. For example, for vfs_write() there is kernel_write(), which
provides a no-security-check equivalent. Should there be something
similar for vfs_setlease() that could be utilized for this purpose?

[1] https://bugzilla.redhat.com/show_bug.cgi?id=2248830

-- 
Ondrej Mosnacek
Senior Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

Re: Calls to vfs_setlease() from NFSD code cause unnecessary CAP_LEASE security checks

[Jeff Layton]

On Fri, 2024-02-02 at 16:31 +0100, Ondrej Mosnacek wrote:
> Hello,
> 
> In [1] a user reports seeing SELinux denials from NFSD when it writes
> into /proc/fs/nfsd/threads with the following kernel backtrace:
>  => trace_event_raw_event_selinux_audited
>  => avc_audit_post_callback
>  => common_lsm_audit
>  => slow_avc_audit
>  => cred_has_capability.isra.0
>  => security_capable
>  => capable
>  => generic_setlease
>  => destroy_unhashed_deleg
>  => __destroy_client
>  => nfs4_state_shutdown_net
>  => nfsd_shutdown_net
>  => nfsd_last_thread
>  => nfsd_svc
>  => write_threads
>  => nfsctl_transaction_write
>  => vfs_write
>  => ksys_write
>  => do_syscall_64
>  => entry_SYSCALL_64_after_hwframe
> 
> It seems to me that the security checks in generic_setlease() should
> be skipped (at least) when called through this codepath, since the
> userspace process merely writes into /proc/fs/nfsd/threads and it's
> just the kernel's internal code that releases the lease as a side
> effect. For example, for vfs_write() there is kernel_write(), which
> provides a no-security-check equivalent. Should there be something
> similar for vfs_setlease() that could be utilized for this purpose?
> 
> [1] https://bugzilla.redhat.com/show_bug.cgi?id=2248830
> 

Thanks for the bug report!

Am I correct that we only want to do this check when someone from
userland tries to set a lease via fcntl? The rest of the callers are all
in-kernel callers and I don't think we need to check for any of them. It
may be simpler to just push this check into the appropriate callers of
generic_setlease instead.

Hmm now that I look too...it looks like we aren't checking CAP_LEASE on
filesystems that have their own ->setlease operation. I'll have a look
at that soon too.

Thanks,
-- 
Jeff Layton <jlayton@kernel.org>

Re: Calls to vfs_setlease() from NFSD code cause unnecessary CAP_LEASE security checks

[Ondrej Mosnacek]

On Fri, Feb 2, 2024 at 5:08 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> On Fri, 2024-02-02 at 16:31 +0100, Ondrej Mosnacek wrote:
> > Hello,
> >
> > In [1] a user reports seeing SELinux denials from NFSD when it writes
> > into /proc/fs/nfsd/threads with the following kernel backtrace:
> >  => trace_event_raw_event_selinux_audited
> >  => avc_audit_post_callback
> >  => common_lsm_audit
> >  => slow_avc_audit
> >  => cred_has_capability.isra.0
> >  => security_capable
> >  => capable
> >  => generic_setlease
> >  => destroy_unhashed_deleg
> >  => __destroy_client
> >  => nfs4_state_shutdown_net
> >  => nfsd_shutdown_net
> >  => nfsd_last_thread
> >  => nfsd_svc
> >  => write_threads
> >  => nfsctl_transaction_write
> >  => vfs_write
> >  => ksys_write
> >  => do_syscall_64
> >  => entry_SYSCALL_64_after_hwframe
> >
> > It seems to me that the security checks in generic_setlease() should
> > be skipped (at least) when called through this codepath, since the
> > userspace process merely writes into /proc/fs/nfsd/threads and it's
> > just the kernel's internal code that releases the lease as a side
> > effect. For example, for vfs_write() there is kernel_write(), which
> > provides a no-security-check equivalent. Should there be something
> > similar for vfs_setlease() that could be utilized for this purpose?
> >
> > [1] https://bugzilla.redhat.com/show_bug.cgi?id=2248830
> >
>
> Thanks for the bug report!
>
> Am I correct that we only want to do this check when someone from
> userland tries to set a lease via fcntl? The rest of the callers are all
> in-kernel callers and I don't think we need to check for any of them. It
> may be simpler to just push this check into the appropriate callers of
> generic_setlease instead.
>
> Hmm now that I look too...it looks like we aren't checking CAP_LEASE on
> filesystems that have their own ->setlease operation. I'll have a look
> at that soon too.

I did briefly check this while analyzing the issue and all of the
setlease fops implementations seemed to be either simple_nosetlease()
or some wrappers around generic_setlease(), which should both be OK.
But it can't hurt to double-check :)

-- 
Ondrej Mosnacek
Senior Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.

Re: Calls to vfs_setlease() from NFSD code cause unnecessary CAP_LEASE security checks

[Ondrej Mosnacek]

On Fri, Feb 2, 2024 at 5:31 PM Ondrej Mosnacek <omosnace@redhat.com> wrote:
>
> On Fri, Feb 2, 2024 at 5:08 PM Jeff Layton <jlayton@kernel.org> wrote:
> >
> > On Fri, 2024-02-02 at 16:31 +0100, Ondrej Mosnacek wrote:
> > > Hello,
> > >
> > > In [1] a user reports seeing SELinux denials from NFSD when it writes
> > > into /proc/fs/nfsd/threads with the following kernel backtrace:
> > >  => trace_event_raw_event_selinux_audited
> > >  => avc_audit_post_callback
> > >  => common_lsm_audit
> > >  => slow_avc_audit
> > >  => cred_has_capability.isra.0
> > >  => security_capable
> > >  => capable
> > >  => generic_setlease
> > >  => destroy_unhashed_deleg
> > >  => __destroy_client
> > >  => nfs4_state_shutdown_net
> > >  => nfsd_shutdown_net
> > >  => nfsd_last_thread
> > >  => nfsd_svc
> > >  => write_threads
> > >  => nfsctl_transaction_write
> > >  => vfs_write
> > >  => ksys_write
> > >  => do_syscall_64
> > >  => entry_SYSCALL_64_after_hwframe
> > >
> > > It seems to me that the security checks in generic_setlease() should
> > > be skipped (at least) when called through this codepath, since the
> > > userspace process merely writes into /proc/fs/nfsd/threads and it's
> > > just the kernel's internal code that releases the lease as a side
> > > effect. For example, for vfs_write() there is kernel_write(), which
> > > provides a no-security-check equivalent. Should there be something
> > > similar for vfs_setlease() that could be utilized for this purpose?
> > >
> > > [1] https://bugzilla.redhat.com/show_bug.cgi?id=2248830
> > >
> >
> > Thanks for the bug report!
> >
> > Am I correct that we only want to do this check when someone from
> > userland tries to set a lease via fcntl? The rest of the callers are all
> > in-kernel callers and I don't think we need to check for any of them. It
> > may be simpler to just push this check into the appropriate callers of
> > generic_setlease instead.
> >
> > Hmm now that I look too...it looks like we aren't checking CAP_LEASE on
> > filesystems that have their own ->setlease operation. I'll have a look
> > at that soon too.
>
> I did briefly check this while analyzing the issue and all of the
> setlease fops implementations seemed to be either simple_nosetlease()
> or some wrappers around generic_setlease(), which should both be OK.
> But it can't hurt to double-check :)

To close the loop here - there is now a fix from Jeff in linux-next:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=7b8001013d720c232ad9ae7aae0ef0e7c281c6d4

Thank you, Jeff, for taking care of it!

-- 
Ondrej Mosnacek
Senior Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.