RE: [PATCH v5 3/7] fs: Add fd_install_received() wrapper for __fd_install_received()

[David Laight]

From: Kees Cook
> Sent: 18 June 2020 21:13
> On Thu, Jun 18, 2020 at 05:49:19AM +0000, Sargun Dhillon wrote:
> > On Wed, Jun 17, 2020 at 03:03:23PM -0700, Kees Cook wrote:
> > > [...]
> > >  static inline int fd_install_received_user(struct file *file, int __user *ufd,
> > >  					   unsigned int o_flags)
> > >  {
> > > +	if (ufd == NULL)
> > > +		return -EFAULT;
> > Isn't this *technically* a behvaiour change? Nonetheless, I think this is a much better
> > approach than forcing everyone to do null checking, and avoids at least one error case
> > where the kernel installs FDs for SCM_RIGHTS, and they're not actualy usable.
> 
> So, the only behavior change I see is that the order of sanity checks is
> changed.
> 
> The loop in scm_detach_fds() is:
> 
> 
>         for (i = 0; i < fdmax; i++) {
>                 err = __scm_install_fd(scm->fp->fp[i], cmsg_data + i, o_flags);
>                 if (err < 0)
>                         break;
>         }
> 
> Before, __scm_install_fd() does:
> 
>         error = security_file_receive(file);
>         if (error)
>                 return error;
> 
>         new_fd = get_unused_fd_flags(o_flags);
>         if (new_fd < 0)
>                 return new_fd;
> 
>         error = put_user(new_fd, ufd);
>         if (error) {
>                 put_unused_fd(new_fd);
>                 return error;
>         }
> 	...
> 
> After, fd_install_received_user() and __fd_install_received() does:
> 
>         if (ufd == NULL)
>                 return -EFAULT;
> 	...
>         error = security_file_receive(file);
>         if (error)
>                 return error;
> 	...
>                 new_fd = get_unused_fd_flags(o_flags);
>                 if (new_fd < 0)
>                         return new_fd;
> 	...
>                 error = put_user(new_fd, ufd);
>                 if (error) {
>                         put_unused_fd(new_fd);
>                         return error;
>                 }
> 
> i.e. if a caller attempts a receive that is rejected by LSM *and*
> includes a NULL userpointer destination, they will get an EFAULT now
> instead of an EPERM.

The 'user' pointer the fd is written to is in the middle of
the 'cmsg' buffer.
So to hit 'ufd == NULL' the program would have to pass a small
negative integer!

The error paths are strange if there are multiple fd in the message.
A quick look at the old code seems to imply that if the user doesn't
supply a big enough buffer then the extra 'file *' just get closed.
OTOH if there is an error processing one of the files the request
fails with the earlier file allocated fd numbers.

In addition most of the userspace buffer is written after the
loop - any errors there return -EFAULT (SIGSEGV) without
even trying to tidy up the allocated fd.

ISTM that the put_user(new_fd, ufd) could be done in __scm_install_fd()
after __fd_install_received() returns.

scm_detach_fds() could do the put_user(SOL_SOCKET,...) before actually
processing the first file - so that the state can be left unchanged
when a naff buffer is passed.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)