[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] seccomp: use SIGSYS signal instead of killi
From: |
Eduardo Otubo |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] seccomp: use SIGSYS signal instead of killing the thread |
Date: |
Wed, 25 Jul 2018 12:42:05 +0200 |
User-agent: |
Mutt/1.8.3+47 (5f034395e53d) (2017-05-23) |
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 <address@hidden>
> > ---
> > 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é <address@hidden>
>
Acked-by: Eduardo Otubo <address@hidden>
signature.asc
Description: PGP signature