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


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