Re: [Qemu-devel] [PATCHv5 1/5] seccomp: changing from whitelist to blacklist

[Eduardo Otubo]

On Fri, Sep 08, 2017 at 11:43:27AM +0200, Thomas Huth wrote:
> On 08.09.2017 11:10, Eduardo Otubo wrote:
> > This patch changes the default behavior of the seccomp filter from
> > whitelist to blacklist. By default now all system calls are allowed and
> > a small black list of definitely forbidden ones was created.
> > 
> > Signed-off-by: Eduardo Otubo <address@hidden>
> > ---
> >  include/sysemu/seccomp.h |   2 +
> >  qemu-seccomp.c           | 264 
> > ++++++-----------------------------------------
> >  vl.c                     |   1 -
> >  3 files changed, 35 insertions(+), 232 deletions(-)
> > 
> > diff --git a/include/sysemu/seccomp.h b/include/sysemu/seccomp.h
> > index cfc06008cb..23b9c3c789 100644
> > --- a/include/sysemu/seccomp.h
> > +++ b/include/sysemu/seccomp.h
> > @@ -15,6 +15,8 @@
> >  #ifndef QEMU_SECCOMP_H
> >  #define QEMU_SECCOMP_H
> >  
> > +#define QEMU_SECCOMP_SET_DEFAULT     (1 << 0)
> > +
> >  #include <seccomp.h>
> >  
> >  int seccomp_start(void);
> > diff --git a/qemu-seccomp.c b/qemu-seccomp.c
> > index df75d9c471..bc9a1f77ff 100644
> > --- a/qemu-seccomp.c
> > +++ b/qemu-seccomp.c
> > @@ -28,232 +28,34 @@
> >  
> >  struct QemuSeccompSyscall {
> >      int32_t num;
> > -    uint8_t priority;
> > +    int type;
> 
> What's this "type" field good for? I failed to spot the place in the
> sources where you are using it...? Anyway, some comments here right
> after the struct members would be useful.

The type is exactly the type of the system call on the blacklist array
below. Being QEMU_SECCOMP_SET_DEFAULT, QEMU_SECCOMP_SET_OBSOLETE, etc.

Do you think comments here worth a full v6?

> 
>  Thomas
> 
> > +    uint8_t set;
> >  };
> >  
> > -static const struct QemuSeccompSyscall seccomp_whitelist[] = {
> > -    { SCMP_SYS(timer_settime), 255 },
> [...]
> > -    { SCMP_SYS(memfd_create), 240 },
> > -#ifdef HAVE_CACHEFLUSH
> > -    { SCMP_SYS(cacheflush), 240 },
> > -#endif
> > -    { SCMP_SYS(sysinfo), 240 },
> > +static const struct QemuSeccompSyscall blacklist[] = {
> > +    /* default set of syscalls to blacklist */
> > +    { SCMP_SYS(reboot),                 1, QEMU_SECCOMP_SET_DEFAULT },
> > +    { SCMP_SYS(swapon),                 1, QEMU_SECCOMP_SET_DEFAULT },
> > +    { SCMP_SYS(swapoff),                1, QEMU_SECCOMP_SET_DEFAULT },
> > +    { SCMP_SYS(syslog),                 1, QEMU_SECCOMP_SET_DEFAULT },
> > +    { SCMP_SYS(mount),                  1, QEMU_SECCOMP_SET_DEFAULT },
> > +    { SCMP_SYS(umount),                 1, QEMU_SECCOMP_SET_DEFAULT },
> > +    { SCMP_SYS(kexec_load),             1, QEMU_SECCOMP_SET_DEFAULT },
> > +    { SCMP_SYS(afs_syscall),            1, QEMU_SECCOMP_SET_DEFAULT },
> > +    { SCMP_SYS(break),                  1, QEMU_SECCOMP_SET_DEFAULT },
> > +    { SCMP_SYS(ftime),                  1, QEMU_SECCOMP_SET_DEFAULT },
> > +    { SCMP_SYS(getpmsg),                1, QEMU_SECCOMP_SET_DEFAULT },
> > +    { SCMP_SYS(gtty),                   1, QEMU_SECCOMP_SET_DEFAULT },
> > +    { SCMP_SYS(lock),                   1, QEMU_SECCOMP_SET_DEFAULT },
> > +    { SCMP_SYS(mpx),                    1, QEMU_SECCOMP_SET_DEFAULT },
> > +    { SCMP_SYS(prof),                   1, QEMU_SECCOMP_SET_DEFAULT },
> > +    { SCMP_SYS(profil),                 1, QEMU_SECCOMP_SET_DEFAULT },
> > +    { SCMP_SYS(putpmsg),                1, QEMU_SECCOMP_SET_DEFAULT },
> > +    { SCMP_SYS(security),               1, QEMU_SECCOMP_SET_DEFAULT },
> > +    { SCMP_SYS(stty),                   1, QEMU_SECCOMP_SET_DEFAULT },
> > +    { SCMP_SYS(tuxcall),                1, QEMU_SECCOMP_SET_DEFAULT },
> > +    { SCMP_SYS(ulimit),                 1, QEMU_SECCOMP_SET_DEFAULT },
> > +    { SCMP_SYS(vserver),                1, QEMU_SECCOMP_SET_DEFAULT },
> >  };
> >  
> >  int seccomp_start(void)
> > @@ -262,19 +64,19 @@ int seccomp_start(void)
> >      unsigned int i = 0;
> >      scmp_filter_ctx ctx;
> >  
> > -    ctx = seccomp_init(SCMP_ACT_KILL);
> > +    ctx = seccomp_init(SCMP_ACT_ALLOW);
> >      if (ctx == NULL) {
> >          rc = -1;
> >          goto seccomp_return;
> >      }
> >  
> > -    for (i = 0; i < ARRAY_SIZE(seccomp_whitelist); i++) {
> > -        rc = seccomp_rule_add(ctx, SCMP_ACT_ALLOW, 
> > seccomp_whitelist[i].num, 0);
> > -        if (rc < 0) {
> > -            goto seccomp_return;
> > +    for (i = 0; i < ARRAY_SIZE(blacklist); i++) {
> > +        switch (blacklist[i].set) {
> > +        default:
> > +            break;
> >          }
> > -        rc = seccomp_syscall_priority(ctx, seccomp_whitelist[i].num,
> > -                                      seccomp_whitelist[i].priority);
> > +
> > +        rc = seccomp_rule_add(ctx, SCMP_ACT_KILL, blacklist[i].num, 0);
> >          if (rc < 0) {
> >              goto seccomp_return;
> >          }
> > diff --git a/vl.c b/vl.c
> > index fb1f05b937..76e0b3a946 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -1032,7 +1032,6 @@ static int bt_parse(const char *opt)
> >  
> >  static int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp)
> >  {
> > -    /* FIXME: change this to true for 1.3 */
> >      if (qemu_opt_get_bool(opts, "enable", false)) {
> >  #ifdef CONFIG_SECCOMP
> >          if (seccomp_start() < 0) {
> > 
> 
> 

-- 
Eduardo Otubo
Senior Software Engineer @ RedHat