[Qemu-devel] [PATCH 0/2] RFC: seccomp action, prefer KILL_PROCESS or TRAP

[Qemu-devel] [PATCH 0/2] RFC: seccomp action, prefer KILL_PROCESS or TRAP

[Marc-André Lureau]

Hi,

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.

Related to:
https://bugzilla.redhat.com/show_bug.cgi?id=1594456

Marc-André Lureau (2):
  seccomp: use SIGSYS signal instead of killing the thread
  RFC: seccomp: prefer SCMP_ACT_KILL_PROCESS if available

 qemu-seccomp.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

-- 
2.18.0.232.gb7bd9486b0

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

[Marc-André Lureau]

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

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

[Daniel P. Berrangé]

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

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

[Eduardo Otubo]

[-- Attachment #1: Type: text/plain, Size: 3061 bytes --]

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>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]

[Qemu-devel] [PATCH 2/2] RFC: seccomp: prefer SCMP_ACT_KILL_PROCESS if available

[Marc-André Lureau]

The upcoming libseccomp release should have SCMP_ACT_KILL_PROCESS
action (https://github.com/seccomp/libseccomp/issues/96).

SCMP_ACT_KILL_PROCESS is preferable to immediately terminate the
offending process, rather than having the SIGSYS handler running.

Use SECCOMP_GET_ACTION_AVAIL to check availability of kernel support,
as libseccomp will fallback on SCMP_ACT_KILL otherwise, and we still
prefer SCMP_ACT_TRAP.

Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
---
 qemu-seccomp.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/qemu-seccomp.c b/qemu-seccomp.c
index b117a92559..505887d5af 100644
--- a/qemu-seccomp.c
+++ b/qemu-seccomp.c
@@ -20,6 +20,7 @@
 #include <sys/prctl.h>
 #include <seccomp.h>
 #include "sysemu/seccomp.h"
+#include <linux/seccomp.h>
 
 /* For some architectures (notably ARM) cacheflush is not supported until
  * libseccomp 2.2.3, but configure enforces that we are using a more recent
@@ -107,12 +108,39 @@ static const struct QemuSeccompSyscall blacklist[] = {
     { SCMP_SYS(sched_get_priority_min), QEMU_SECCOMP_SET_RESOURCECTL },
 };
 
+static inline int
+qemu_seccomp(unsigned int operation, unsigned int flags, void *args)
+{
+#ifdef __NR_seccomp
+    return syscall(__NR_seccomp, operation, flags, args);
+#else
+    return -1;
+#endif
+}
+
+static uint32_t qemu_seccomp_get_kill_action(void)
+{
+#if defined(SECCOMP_GET_ACTION_AVAIL) && defined(SCMP_ACT_KILL_PROCESS) && \
+    defined(SECCOMP_RET_KILL_PROCESS)
+    {
+        uint32_t action = SECCOMP_RET_KILL_PROCESS;
+
+        if (qemu_seccomp(SECCOMP_GET_ACTION_AVAIL, 0, &action) == 0) {
+            return SCMP_ACT_KILL_PROCESS;
+        }
+    }
+#endif
+
+    return SCMP_ACT_TRAP;
+}
+
 
 static int seccomp_start(uint32_t seccomp_opts)
 {
     int rc = 0;
     unsigned int i = 0;
     scmp_filter_ctx ctx;
+    uint32_t action = qemu_seccomp_get_kill_action();
 
     ctx = seccomp_init(SCMP_ACT_ALLOW);
     if (ctx == NULL) {
@@ -125,7 +153,7 @@ static int seccomp_start(uint32_t seccomp_opts)
             continue;
         }
 
-        rc = seccomp_rule_add_array(ctx, SCMP_ACT_TRAP, blacklist[i].num,
+        rc = seccomp_rule_add_array(ctx, action, blacklist[i].num,
                                     blacklist[i].narg, blacklist[i].arg_cmp);
         if (rc < 0) {
             goto seccomp_return;
-- 
2.18.0.232.gb7bd9486b0

Re: [Qemu-devel] [PATCH 2/2] RFC: seccomp: prefer SCMP_ACT_KILL_PROCESS if available

[Daniel P. Berrangé]

On Fri, Jul 20, 2018 at 05:44:25PM +0200, Marc-André Lureau wrote:
> The upcoming libseccomp release should have SCMP_ACT_KILL_PROCESS
> action (https://github.com/seccomp/libseccomp/issues/96).
> 
> SCMP_ACT_KILL_PROCESS is preferable to immediately terminate the
> offending process, rather than having the SIGSYS handler running.
> 
> Use SECCOMP_GET_ACTION_AVAIL to check availability of kernel support,
> as libseccomp will fallback on SCMP_ACT_KILL otherwise, and we still
> prefer SCMP_ACT_TRAP.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qemu-seccomp.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)

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

Re: [Qemu-devel] [PATCH 2/2] RFC: seccomp: prefer SCMP_ACT_KILL_PROCESS if available

[Eduardo Otubo]

[-- Attachment #1: Type: text/plain, Size: 2773 bytes --]

On 20/07/2018 - 17:44:25, Marc-André Lureau wrote:
> The upcoming libseccomp release should have SCMP_ACT_KILL_PROCESS
> action (https://github.com/seccomp/libseccomp/issues/96).
> 
> SCMP_ACT_KILL_PROCESS is preferable to immediately terminate the
> offending process, rather than having the SIGSYS handler running.
> 
> Use SECCOMP_GET_ACTION_AVAIL to check availability of kernel support,
> as libseccomp will fallback on SCMP_ACT_KILL otherwise, and we still
> prefer SCMP_ACT_TRAP.
> 
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  qemu-seccomp.c | 30 +++++++++++++++++++++++++++++-
>  1 file changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/qemu-seccomp.c b/qemu-seccomp.c
> index b117a92559..505887d5af 100644
> --- a/qemu-seccomp.c
> +++ b/qemu-seccomp.c
> @@ -20,6 +20,7 @@
>  #include <sys/prctl.h>
>  #include <seccomp.h>
>  #include "sysemu/seccomp.h"
> +#include <linux/seccomp.h>
>  
>  /* For some architectures (notably ARM) cacheflush is not supported until
>   * libseccomp 2.2.3, but configure enforces that we are using a more recent
> @@ -107,12 +108,39 @@ static const struct QemuSeccompSyscall blacklist[] = {
>      { SCMP_SYS(sched_get_priority_min), QEMU_SECCOMP_SET_RESOURCECTL },
>  };
>  
> +static inline int
> +qemu_seccomp(unsigned int operation, unsigned int flags, void *args)
> +{
> +#ifdef __NR_seccomp
> +    return syscall(__NR_seccomp, operation, flags, args);
> +#else
> +    return -1;
> +#endif
> +}
> +
> +static uint32_t qemu_seccomp_get_kill_action(void)
> +{
> +#if defined(SECCOMP_GET_ACTION_AVAIL) && defined(SCMP_ACT_KILL_PROCESS) && \
> +    defined(SECCOMP_RET_KILL_PROCESS)
> +    {
> +        uint32_t action = SECCOMP_RET_KILL_PROCESS;
> +
> +        if (qemu_seccomp(SECCOMP_GET_ACTION_AVAIL, 0, &action) == 0) {
> +            return SCMP_ACT_KILL_PROCESS;
> +        }
> +    }
> +#endif
> +
> +    return SCMP_ACT_TRAP;
> +}
> +
>  
>  static int seccomp_start(uint32_t seccomp_opts)
>  {
>      int rc = 0;
>      unsigned int i = 0;
>      scmp_filter_ctx ctx;
> +    uint32_t action = qemu_seccomp_get_kill_action();
>  
>      ctx = seccomp_init(SCMP_ACT_ALLOW);
>      if (ctx == NULL) {
> @@ -125,7 +153,7 @@ static int seccomp_start(uint32_t seccomp_opts)
>              continue;
>          }
>  
> -        rc = seccomp_rule_add_array(ctx, SCMP_ACT_TRAP, blacklist[i].num,
> +        rc = seccomp_rule_add_array(ctx, action, blacklist[i].num,
>                                      blacklist[i].narg, blacklist[i].arg_cmp);
>          if (rc < 0) {
>              goto seccomp_return;
> -- 
> 2.18.0.232.gb7bd9486b0
> 

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

-- 
Eduardo Otubo

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]