lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 19 Jun 2020 08:20:55 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Kees Cook' <keescook@...omium.org>,
        Sargun Dhillon <sargun@...gun.me>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Christian Brauner" <christian@...uner.io>,
        Tycho Andersen <tycho@...ho.ws>,
        "Christoph Hellwig" <hch@....de>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Aleksa Sarai <cyphar@...har.com>,
        Matt Denton <mpdenton@...gle.com>,
        Jann Horn <jannh@...gle.com>, Chris Palmer <palmer@...gle.com>,
        Robert Sesek <rsesek@...gle.com>,
        Giuseppe Scrivano <gscrivan@...hat.com>,
        "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
        Andy Lutomirski <luto@...capital.net>,
        Will Drewry <wad@...omium.org>, Shuah Khan <shuah@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "containers@...ts.linux-foundation.org" 
        <containers@...ts.linux-foundation.org>,
        "linux-api@...r.kernel.org" <linux-api@...r.kernel.org>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        "linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>
Subject: RE: [PATCH v5 3/7] fs: Add fd_install_received() wrapper for
 __fd_install_received()

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)

Powered by blists - more mailing lists