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

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

[Rich Felker]

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@libc.org>
---

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.

 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);
 	}
 
 	unreachable();
-- 
2.21.0

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

[Kees Cook]

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@libc.org>
> ---
> 
> 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. :)


> 
>  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

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@chromium.org> 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@libc.org>
> > ---
> >
> > 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

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

[Kees Cook]

On Fri, 28 Aug 2020 21:56:13 -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.

Applied, thanks!

[1/1] seccomp: kill process instead of thread for unknown actions
      https://git.kernel.org/kees/c/4d671d922d51

I tweaked the coredump logic and added a selftest (and CCed you) for
the behavior (since it wasn't included in the existing tests).

-- 
Kees Cook