Message-ID: <20200530011054.GA14852@ircssh-2.c.rugged-nimbus-611.internal>
Date:   Sat, 30 May 2020 01:10:55 +0000
From:   Sargun Dhillon <sargun@...gun.me>
To:     Kees Cook <keescook@...omium.org>
Cc:     christian.brauner@...ntu.com,
        containers@...ts.linux-foundation.org, cyphar@...har.com,
        jannh@...gle.com, jeffv@...gle.com, linux-api@...r.kernel.org,
        linux-kernel@...r.kernel.org, palmer@...gle.com, rsesek@...gle.com,
        tycho@...ho.ws, Matt Denton <mpdenton@...gle.com>,
        Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH v2 2/3] seccomp: Introduce addfd ioctl to seccomp user
 notifier

On Fri, May 29, 2020 at 12:31:37AM -0700, Kees Cook wrote:
> On Thu, May 28, 2020 at 04:08:57AM -0700, Sargun Dhillon wrote:
> > This adds a seccomp notifier ioctl which allows for the listener to "add"
> > file descriptors to a process which originated a seccomp user
> > notification. This allows calls like mount, and mknod to be "implemented",
> > as the return value, and the arguments are data in memory. On the other
> > hand, calls like connect can be "implemented" using pidfd_getfd.
> > 
> > Unfortunately, there are calls which return file descriptors, like
> > open, which are vulnerable to TOC-TOU attacks, and require that the
> > more privileged supervisor can inspect the argument, and perform the
> > syscall on behalf of the process generating the notifiation. This
> > allows the file descriptor generated from that open call to be
> > returned to the calling process.
> > 
> > In addition, there is funcitonality to allow for replacement of
> > specific file descriptors, following dup2-like semantics.
> > 
> > Signed-off-by: Sargun Dhillon <sargun@...gun.me>
> > Suggested-by: Matt Denton <mpdenton@...gle.com>
> 
> This looks mostly really clean. When I've got more brain tomorrow I want to
> double-check the locking, but I think the use of notify_lock and being
> in the ioctl fully protects everything from any use-after-free-like
> issues.
> 
> Notes below...
> 
> > +/* valid flags for seccomp_notif_addfd */
> > +#define SECCOMP_ADDFD_FLAG_SETFD	(1UL << 0) /* Specify remote fd */
> 
> Nit: please use BIT()
> 
> > @@ -735,6 +770,41 @@ static u64 seccomp_next_notify_id(struct seccomp_filter *filter)
> >  	return filter->notif->next_id++;
> >  }
> >  
> > +static void seccomp_handle_addfd(struct seccomp_kaddfd *addfd)
> > +{
> > +	struct socket *sock;
> > +	int ret, err;
> > +
> > +	/*
> > +	 * Remove the notification, and reset the list pointers, indicating
> > +	 * that it has been handled.
> > +	 */
> > +	list_del_init(&addfd->list);
> > +
> > +	ret = security_file_receive(addfd->file);
> > +	if (ret)
> > +		goto out;
> > +
> > +	if (addfd->fd == -1) {
> > +		ret = get_unused_fd_flags(addfd->flags);
> > +		if (ret >= 0)
> > +			fd_install(ret, get_file(addfd->file));
> > +	} else {
> > +		ret = replace_fd(addfd->fd, addfd->file, addfd->flags);
> > +	}
> > +
> > +	/* These are the semantics from copying FDs via SCM_RIGHTS */
> > +	sock = sock_from_file(addfd->file, &err);
> > +	if (sock) {
> > +		sock_update_netprioidx(&sock->sk->sk_cgrp_data);
> > +		sock_update_classid(&sock->sk->sk_cgrp_data);
> > +	}
> 
> This made my eye twitch. ;) I see this is borrowed from
> scm_detach_fds()... this really feels like the kind of thing that will
> quickly go out of sync. I think this "receive an fd" logic needs to be
> lifted out of scm_detach_fds() so it and seccomp can share it. I'm not
> sure how to parameterize it quite right, though. Perhaps:
> 
> int file_receive(int fd, unsigned long flags, struct file *file)
> {
> 	struct socket *sock;
> 	int ret;
> 
> 	ret = security_file_receive(file);
> 	if (ret)
> 		return ret;
> 
> 	/* Install the file. */
> 	if (fd == -1) {
> 		ret = get_unused_fd_flags(flags);
> 		if (ret >= 0)
> 			fd_install(ret, get_file(file));
> 	} else {
> 		ret = replace_fd(fd, file, flags);
> 	}
> 
> 	/* Bump the usage count. */
> 	sock = sock_from_file(addfd->file, &err);
> 	if (sock) {
> 		sock_update_netprioidx(&sock->sk->sk_cgrp_data);
> 		sock_update_classid(&sock->sk->sk_cgrp_data);
> 	}
> 
> 	return ret;
> }
> 
> 
> static void seccomp_handle_addfd(struct seccomp_kaddfd *addfd)
> {
> 	/*
> 	 * Remove the notification, and reset the list pointers, indicating
> 	 * that it has been handled.
> 	 */
> 	list_del_init(&addfd->list);
> 	addfd->ret = file_receive(addfd->fd, addfd->flags, addfd->file);
> 	complete(&addfd->completion);
> }
> 
> scm_detach_fds()
> 	...
> 	for (i=0, cmfptr=(__force int __user *)CMSG_DATA(cm); i<fdmax;
>              i++, cmfptr++)
> 	{
> 
> 		err = file_receive(-1, MSG_CMSG_CLOEXEC & msg->msg_flags
>                                           ? O_CLOEXEC : 0, fp[i]);
> 		if (err < 0)
> 			break;
> 		err = put_user(err, cmfptr);
> 		if (err)
> 			/* wat */
> 	}
> 	...
> 
> I'm not sure on the put_user() failure, though. We could check early
> for faults with a put_user(0, cmfptr) before the file_receive() call, or
> we could just ignore it? I'm not sure what SCM does here. I guess
> worst-case:
> 
> int file_receive(int fd, unsigned long flags, struct file *file,
> 		 int __user *fdptr)
> {
> 		...
> 		ret = get_unused_fd_flags(flags);
> 		if (ret >= 0) {
> 			if (cmfptr) {
> 				int err;
> 
> 	                	err = put_user(ret, cmfptr);
> 				if (err) {
> 					put_unused_fd(ret);
> 					return err;
> 				}
> 			}
> 			fd_install(ret, get_file(file));
> 		}
> 		...
> }
> 
What about:

/*
 * File Receive - Retrieve a file from another process
 *
 * It can either replace an existing fd, or use a newly allocated fd. If you
 * intend on using an existing fd, replace should be false, and flags will
 * be ignored. The fd should be allocated using get_unused_fd_flags with the
 * flags that you want. It does not consume the reference to file.
 *
 * Returns 0 upon success
 */
static int __file_receive(int fd, unsigned int flags, struct file *file,
			  bool replace)
{
	struct socket *sock;
	int err;

	err = security_file_receive(file);
	if (err)
		return err;

	/* Is this an existing FD? */
	if (replace) {
		err = replace_fd(fd, file, flags);
		if (err)
			return err;
	} else {
		fd_install(fd, get_file(file));
	}

	sock = sock_from_file(file, &err);
	if (sock) {
		sock_update_netprioidx(&sock->sk->sk_cgrp_data);
		sock_update_classid(&sock->sk->sk_cgrp_data);
	}

	return 0;
}

int file_receive_replace(int fd, unsigned int flags, struct file *file)
{
	return __file_receive(fd, flags, file, true);
}

int file_receive(int fd, struct file *file)
{
	return __file_receive(fd, 0, file, false);
}


// And then SCM reads:
	for (i=0, cmfptr=(__force int __user *)CMSG_DATA(cm); i<fdmax;
	     i++, cmfptr++)
	{
		int new_fd;
		err = get_unused_fd_flags(MSG_CMSG_CLOEXEC & msg->msg_flags
					  ? O_CLOEXEC : 0);
		if (err < 0)
			break;
		new_fd = err;
		err = put_user(new_fd, cmfptr);
		if (err) {
			put_unused_fd(new_fd);
			break;
		}

		err = file_receive(new_fd, fp[i]);
		if (err) {
			put_unused_fd(new_fd);
			break;
		}
	}

And our code reads:


static void seccomp_handle_addfd(struct seccomp_kaddfd *addfd)
{
	int ret, err;

	/*
	 * Remove the notification, and reset the list pointers, indicating
	 * that it has been handled.
	 */
	list_del_init(&addfd->list);

	if (addfd->fd == -1) {
		ret = get_unused_fd_flags(addfd->flags);
		if (ret < 0)
			goto err;

		err = file_receive(ret, addfd->file);
		if (err) {
			put_unused_fd(ret);
			ret = err;
		}
	} else {
		ret = file_receive_replace(addfd->fd, addfd->flags,
					   addfd->file);
	}

err:
	addfd->ret = ret;
	complete(&addfd->completion);
}


And the pidfd getfd code reads:

static int pidfd_getfd(struct pid *pid, int fd)
{
	struct task_struct *task;
	struct file *file;
	int ret, err;

	task = get_pid_task(pid, PIDTYPE_PID);
	if (!task)
		return -ESRCH;

	file = __pidfd_fget(task, fd);
	put_task_struct(task);
	if (IS_ERR(file))
		return PTR_ERR(file);

	ret = get_unused_fd_flags(O_CLOEXEC);
	if (ret >= 0) {
		err = file_receive(ret, file);
		if (err) {
			put_unused_fd(ret);
			ret = err;
		}
	}

	fput(file);
	return ret;
}

Powered by blists - more mailing lists