Re: [PATCH] seccomp: kill process instead of thread for unknown actions

[Kyle Huey]

On Mon, Aug 31, 2020 at 12:37 PM Kees Cook <keescook@xxxxxxxxxxxx> wrote:
>
> On Fri, Aug 28, 2020 at 09:56:13PM -0400, Rich Felker wrote:
> > Asynchronous termination of a thread outside of the userspace thread
> > library's knowledge is an unsafe operation that leaves the process in
> > an inconsistent, corrupt, and possibly unrecoverable state. In order
> > to make new actions that may be added in the future safe on kernels
> > not aware of them, change the default action from
> > SECCOMP_RET_KILL_THREAD to SECCOMP_RET_KILL_PROCESS.
> >
> > Signed-off-by: Rich Felker <dalias@xxxxxxxx>
> > ---
> >
> > This fundamental problem with SECCOMP_RET_KILL_THREAD, and that it
> > should be considered unsafe and deprecated, was recently noted/fixed
> > seccomp in the man page and its example. Here I've only changed the
> > default action for new/unknown action codes. Ideally the behavior for
> > strict seccomp mode would be changed too but I think that breaks
> > stability policy; in any case it's less likely to be an issue since
> > strict mode is hard or impossible to use reasonably in a multithreaded
> > process.
> >
> > Unfortunately changing this now won't help older kernels where unknown
> > new actions would still be handled unsafely, but at least it makes it
> > so the problem will fade away over time.
>
> I think this is probably fine to change now. I'd always wanted to
> "upgrade" the default to KILL_PROCESS, but wanted to wait for
> KILL_PROCESS to exist at all for a while first. :)
>
> I'm not aware of any filter generators (e.g. libseccomp, Chrome) that
> depend on unknown filter return values to cause a KILL_THREAD, and
> everything I've seen indicates that they aren't _accidentally_ depending
> on it either (i.e. they both produce "valid" filters). It's possible
> that something out there doesn't, and in that case, we likely need to
> make a special case for whatever bad filter value it chose, but we can
> cross that bridge when we come to it.
>
> I've added Kyle and Robert to CC as well, as they have noticed subtle
> changes to seccomp behavior in the past. I *think* this change should be
> fine, but perhaps they will see something I don't. :)

I can't think of anything here that would break stuff, though I do
believe rr needs some changes to how it handles this (I don't think
our current behavior is an accurate emulation of the kernel).

- Kyle

> >
> >  kernel/seccomp.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > index d653d8426de9..ce1875fa6b39 100644
> > --- a/kernel/seccomp.c
> > +++ b/kernel/seccomp.c
> > @@ -910,10 +910,10 @@ static int __seccomp_filter(int this_syscall, const struct seccomp_data *sd,
> >                       seccomp_init_siginfo(&info, this_syscall, data);
> >                       do_coredump(&info);
> >               }
> > -             if (action == SECCOMP_RET_KILL_PROCESS)
> > -                     do_group_exit(SIGSYS);
> > -             else
> > +             if (action == SECCOMP_RET_KILL_THREAD)
> >                       do_exit(SIGSYS);
> > +             else
> > +                     do_group_exit(SIGSYS);
>
> I need to think a little more, but I suspect we should change the coredump
> logic (above the quoted code) too... (i.e. "action == SECCOMP_RET_KILL_PROCESS"
> -> "action != SECCOMP_RET_KILL_THREAD")
>
> >       }
> >
> >       unreachable();
> > --
> > 2.21.0
> >
>
> Thanks!
>
> -Kees
>
> --
> Kees Cook