Re: [Qemu-devel] [PATCH 1/2] seccomp: use SIGSYS signal instead of killing the thread

["Daniel P. Berrangé" <berrange@redhat.com>]

On Fri, Jul 20, 2018 at 05:44:24PM +0200, Marc-André Lureau wrote:
> The seccomp action SCMP_ACT_KILL results in immediate termination of
> the thread that made the bad system call. However, qemu being
> multi-threaded, it keeps running. There is no easy way for parent
> process / management layer (libvirt) to know about that situation.
> 
> Instead, the default SIGSYS handler when invoked with SCMP_ACT_TRAP
> will terminate the program and core dump.
> 
> This may not be the most secure solution, but probably better than
> just killing the offending thread. SCMP_ACT_KILL_PROCESS has been
> added in Linux 4.14 to improve the situation, which I propose to use
> by default if available in the next patch.

Note that seccomp doesn't promise to protect against all  types
of vulnerability in a program. It merely aims to stop the program
executing designated system calls.

Using SCMP_ACT_TRAP still prevents syscal execution to exactly the
same extent that SCMP_ACT_KILL does, so its security level is the
same.

What differs is that the userspace app has option to ignore the
syscall and carry on instead of being killed. A malicous attacker
would thus have option to try to influence other parts of QEMU
todo bad stuff, but if they already have control over the userspace
process to this extent, they can likely do such bad stuff even
before executing the syscalls

So I don't think there's any significant difference in security
protection here.  Mostly the difference is just about what the
crash will look like. A full process crash (from the default
signal handler) looks better than a thread crash for the reasons
you've explained.

> 
> Related to:
> https://bugzilla.redhat.com/show_bug.cgi?id=1594456
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qemu-seccomp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qemu-seccomp.c b/qemu-seccomp.c
> index 9cd8eb9499..b117a92559 100644
> --- a/qemu-seccomp.c
> +++ b/qemu-seccomp.c
> @@ -125,7 +125,7 @@ static int seccomp_start(uint32_t seccomp_opts)
>              continue;
>          }
>  
> -        rc = seccomp_rule_add_array(ctx, SCMP_ACT_KILL, blacklist[i].num,
> +        rc = seccomp_rule_add_array(ctx, SCMP_ACT_TRAP, blacklist[i].num,
>                                      blacklist[i].narg, blacklist[i].arg_cmp);
>          if (rc < 0) {
>              goto seccomp_return;

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|