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.

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;
-- 
2.18.0.232.gb7bd9486b0

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

On 20/07/2018 - 17:00:39, Daniel P. Berrange wrote:
> 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.

I guess that's the whole point of having the process killed instead of the 
thread. Seccomp is not a big security feature alone by itself, but rather
combined with others techniques.

Marc, from what we've already discussed I think these patches are good enough
for now. Thanks a lot for the contribution.

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

Acked-by: Eduardo Otubo <otubo@redhat.com>