Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190306201413.14153-1-tycho@tycho.ws> <20190306201413.14153-2-tycho@tycho.ws>
 <CAGXu5jKaK-mTq=Qez7E_fYysz916_xEN7Houj-y8gdmNGkMMVw@mail.gmail.com>
 <CAGXu5jKM1k0roNJEqtns2fe-yY_2G_qhvBHids-ZWimL-ddWsw@mail.gmail.com> <20190423233422.GK3758@cisco>
In-Reply-To: <20190423233422.GK3758@cisco>
From:   Kees Cook <keescook@chromium.org>
Date:   Tue, 23 Apr 2019 18:02:08 -0700
Message-ID: <CAGXu5jJFdyBGw8OQdWJ2NL54O0qHHkg0b3uyqm6Prg_ym7-p3g@mail.gmail.com>
Subject: Re: [PATCH 2/2] seccomp: disallow NEW_LISTENER and TSYNC flags
To:     Tycho Andersen <tycho@tycho.ws>
Cc:     James Morris <jmorris@namei.org>,
        LKML <linux-kernel@vger.kernel.org>,
        "# 3.4.x" <stable@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Apr 23, 2019 at 4:34 PM Tycho Andersen <tycho@tycho.ws> wrote:
>
> On Tue, Apr 23, 2019 at 04:31:45PM -0700, Kees Cook wrote:
> > On Tue, Apr 23, 2019 at 3:09 PM Kees Cook <keescook@chromium.org> wrote:
> > >
> > > On Wed, Mar 6, 2019 at 12:14 PM Tycho Andersen <tycho@tycho.ws> wrote:
> > > >
> > > > As the comment notes, the return codes for TSYNC and NEW_LISTENER conflict,
> > > > because they both return positive values, one in the case of success and
> > > > one in the case of error. So, let's disallow both of these flags together.
> > > >
> > > > While this is technically a userspace break, all the users I know of are
> > > > still waiting on me to land this feature in libseccomp, so I think it'll be
> > > > safe. Also, at present my use case doesn't require TSYNC at all, so this
> > > > isn't a big deal to disallow. If someone wanted to support this, a path
> > > > forward would be to add a new flag like
> > > > TSYNC_AND_LISTENER_YES_I_UNDERSTAND_THAT_TSYNC_WILL_JUST_RETURN_EAGAIN, but
> > > > the use cases are so different I don't see it really happening.
> > > >
> > > > Finally, it's worth noting that this does actually fix a UAF issue: at the end
> > > > of seccomp_set_mode_filter(), we have:
> > > >
> > > >         if (flags & SECCOMP_FILTER_FLAG_NEW_LISTENER) {
> > > >                 if (ret < 0) {
> > > >                         listener_f->private_data = NULL;
> > > >                         fput(listener_f);
> > > >                         put_unused_fd(listener);
> > > >                 } else {
> > > >                         fd_install(listener, listener_f);
> > > >                         ret = listener;
> > > >                 }
> > > >         }
> > > > out_free:
> > > >         seccomp_filter_free(prepared);
> > > >
> > > > But if ret > 0 because TSYNC raced, we'll install the listener fd and then free
> > > > the filter out from underneath it, causing a UAF when the task closes it or
> > > > dies. This patch also switches the condition to be simply if (ret), so that
> > > > if someone does add the flag mentioned above, they won't have to remember
> > > > to fix this too.
> > > >
> > > > Signed-off-by: Tycho Andersen <tycho@tycho.ws>
> > > > Fixes: 6a21cc50f0c7 ("seccomp: add a return code to trap to userspace")
> > > > CC: stable@vger.kernel.org # v5.0+
> > >
> > > Thanks! Sorry I missed this. James, can you take this for Linus's
> > > fixes for v5.1? (Or should I send a pull request to you?)
> > >
> > > Acked-by: Kees Cook <keescook@chromium.org>
> > >
> > > Let's also add:
> > >
> > > Reported-by: syzbot+b562969adb2e04af3442@syzkaller.appspotmail.com
> > >
> > > > ---
> > > >  kernel/seccomp.c | 17 +++++++++++++++--
> > > >  1 file changed, 15 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > > > index d0d355ded2f4..79bada51091b 100644
> > > > --- a/kernel/seccomp.c
> > > > +++ b/kernel/seccomp.c
> > > > @@ -500,7 +500,10 @@ seccomp_prepare_user_filter(const char __user *user_filter)
> > > >   *
> > > >   * Caller must be holding current->sighand->siglock lock.
> > > >   *
> > > > - * Returns 0 on success, -ve on error.
> > > > + * Returns 0 on success, -ve on error, or
> > > > + *   - in TSYNC mode: the pid of a thread which was either not in the correct
> > > > + *     seccomp mode or did not have an ancestral seccomp filter
> > > > + *   - in NEW_LISTENER mode: the fd of the new listener
> > > >   */
> > > >  static long seccomp_attach_filter(unsigned int flags,
> > > >                                   struct seccomp_filter *filter)
> > > > @@ -1256,6 +1259,16 @@ static long seccomp_set_mode_filter(unsigned int flags,
> > > >         if (flags & ~SECCOMP_FILTER_FLAG_MASK)
> > > >                 return -EINVAL;
> > > >
> > > > +       /*
> > > > +        * In the successful case, NEW_LISTENER returns the new listener fd.
> > > > +        * But in the failure case, TSYNC returns the thread that died. If you
> > > > +        * combine these two flags, there's no way to tell whether something
> > > > +        * succeded or failed. So, let's disallow this combination.
> > >
> > > also a tiny typo: succeeded
> > >
> > > > +        */
> > > > +       if ((flags & SECCOMP_FILTER_FLAG_TSYNC) &&
> > > > +           (flags && SECCOMP_FILTER_FLAG_NEW_LISTENER))
> >
> > also a typo: && should be &
>
> Oh, yes. Do you want me to send another version?

Nah, I fixed it up. :)


-- 
Kees Cook