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

[Eduardo Otubo <otubo@redhat.com>]

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