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 <sar...@sargun.me>
> > Suggested-by: Matt Denton <mpden...@google.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()
Fwiw, I don't think we can use BIT() in uapi headers, see:
commit 23b2c96fad21886c53f5e1a4ffedd45ddd2e85ba
Author: Christian Brauner <christian.brau...@ubuntu.com>
Date: Thu Oct 24 23:25:39 2019 +0200
seccomp: rework define for SECCOMP_USER_NOTIF_FLAG_CONTINUE
Switch from BIT(0) to (1UL << 0).
>
> > @@ -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));
> }
> ...
> }
>
> > @@ -763,14 +835,31 @@ static int seccomp_do_user_notification(int
> > this_syscall,
> > /*
> > * This is where we wait for a reply from userspace.
> > */
> > +wait:
> > err = wait_for_completion_interruptible(&n.ready);
> > mutex_lock(&match->notify_lock);
> > if (err == 0) {
> > + /* Check if we were woken up by a addfd message */
> > + addfd = list_first_entry_or_null(&n.addfd,
> > + struct seccomp_kaddfd, list);
> > + if (addfd && n.state != SECCOMP_NOTIFY_REPLIED) {
> > + seccomp_handle_addfd(addfd);
> > + mutex_unlock(&match->notify_lock);
> > + goto wait;
> > + }
> > ret = n.val;
> > err = n.error;
> > flags = n.flags;
> > }
>
> This feels like it needs to be done differently, but when I tried to
> make it "proper" loop, I think it got more ugly:
>
> for (;;) {
> err = wait_for_completion_interruptible(&n.ready);
> mutex_lock(&match->notify_lock);
> if (err == 0) {
> /* Check if we were woken up by a addfd message */
> addfd = list_first_entry_or_null(&n.addfd,
> struct seccomp_kaddfd,
> list);
> if (addfd && n.state != SECCOMP_NOTIFY_REPLIED) {
> seccomp_handle_addfd(addfd);
> mutex_unlock(&match->notify_lock);
> continue;
> }
> ret = n.val;
> err = n.error;
> flags = n.flags;
> }
> break;
> }
>
> So, I guess it's fine how you have it. :)
>
> >
> > + /* If there were any pending addfd calls, clear them out */
> > + list_for_each_entry_safe(addfd, tmp, &n.addfd, list) {
> > + /* The process went away before we got a chance to handle it */
> > + addfd->ret = -ESRCH;
> > + list_del_init(&addfd->list);
> > + complete(&addfd->completion);
> > + }
> > +
> > /*
> > * Note that it's possible the listener died in between the time when
> > * we were notified of a respons (or a signal) and when we were able to
> > @@ -1174,6 +1263,95 @@ static long seccomp_notify_id_valid(struct
> > seccomp_filter *filter,
> > return ret;
> > }
> >
> > +static long seccomp_notify_addfd(struct seccomp_filter *filter,
> > + struct seccomp_notif_addfd __user *uaddfd)
> > +{
> > + struct seccomp_notif_addfd addfd;
> > + struct seccomp_knotif *knotif;
> > + struct seccomp_kaddfd kaddfd;
> > + u64 size;
> > + int ret;
> > +
> > + ret = get_user(size, &uaddfd->size);
> > + if (ret)
> > + return ret;
> > +
> > + ret = copy_struct_from_user(&addfd, sizeof(addfd), uaddfd, size);
> > + if (ret)
> > + return ret;
> > +
> > + if (addfd.newfd_flags & ~O_CLOEXEC)
> > + return -EINVAL;
> > +
> > + if (addfd.flags & ~SECCOMP_ADDFD_FLAG_SETFD)
> > + return -EINVAL;
> > +
> > + if (addfd.newfd && !(addfd.flags & SECCOMP_ADDFD_FLAG_SETFD))
> > + return -EINVAL;
> > +
> > + kaddfd.file = fget(addfd.srcfd);
> > + if (!kaddfd.file)
> > + return -EBADF;
> > +
> > + kaddfd.flags = addfd.newfd_flags;
> > + kaddfd.fd = (addfd.flags & SECCOMP_ADDFD_FLAG_SETFD) ?
> > + addfd.newfd : -1;
>
> Given that -1 is already illegal, do we need SECCOMP_ADDFD_FLAG_SETFD?
> Could a -1 for newfd just be used instead?
>
> > + init_completion(&kaddfd.completion);
> > +
> > + ret = mutex_lock_interruptible(&filter->notify_lock);
> > + if (ret < 0)
> > + goto out;
> > +
> > + knotif = find_notification(filter, addfd.id);
> > + /*
> > + * We do not want to allow for FD injection to occur before the
> > + * notification has been picked up by a userspace handler, or after
> > + * the notification has been replied to.
> > + */
> > + if (!knotif) {
> > + ret = -ENOENT;
> > + goto out_unlock;
> > + }
> > +
> > + if (knotif->state != SECCOMP_NOTIFY_SENT) {
> > + ret = -EINPROGRESS;
> > + goto out_unlock;
> > + }
> > +
> > + list_add(&kaddfd.list, &knotif->addfd);
> > + complete(&knotif->ready);
> > + mutex_unlock(&filter->notify_lock);
> > +
> > + /* Now we wait for it to be processed */
> > + ret = wait_for_completion_interruptible(&kaddfd.completion);
> > + if (ret == 0) {
> > + /*
> > + * We had a successful completion. The other side has already
> > + * removed us from the addfd queue, and
> > + * wait_for_completion_interruptible has a memory barrier.
> > + */
> > + ret = kaddfd.ret;
> > + goto out;
> > + }
> > +
> > + mutex_lock(&filter->notify_lock);
> > + /*
> > + * Even though we were woken up by a signal, and not a successful
> > + * completion, a completion may have happened in the mean time.
> > + */
> > + if (list_empty(&kaddfd.list))
> > + ret = kaddfd.ret;
> > + else
> > + list_del(&kaddfd.list);
> > +
> > +out_unlock:
> > + mutex_unlock(&filter->notify_lock);
> > +out:
> > + fput(kaddfd.file);
> > +
> > + return ret;
> > +}
> > +
> > static long seccomp_notify_ioctl(struct file *file, unsigned int cmd,
> > unsigned long arg)
> > {
> > @@ -1187,6 +1365,8 @@ static long seccomp_notify_ioctl(struct file *file,
> > unsigned int cmd,
> > return seccomp_notify_send(filter, buf);
> > case SECCOMP_IOCTL_NOTIF_ID_VALID:
> > return seccomp_notify_id_valid(filter, buf);
> > + case SECCOMP_IOCTL_NOTIF_ADDFD:
> > + return seccomp_notify_addfd(filter, buf);
> > default:
> > return -EINVAL;
> > }
> > --
> > 2.25.1
> >
>
> Whee! :)
>
> --
> Kees Cook
