Re: [PATCH 050/104] virtiofsd: add seccomp whitelist

[Dr. David Alan Gilbert]

* Daniel P. Berrangé (address@hidden) wrote:
> On Thu, Dec 12, 2019 at 04:38:10PM +0000, Dr. David Alan Gilbert (git) wrote:
> > From: Stefan Hajnoczi <address@hidden>
> > 
> > Only allow system calls that are needed by virtiofsd.  All other system
> > calls cause SIGSYS to be directed at the thread and the process will
> > coredump.
> > 
> > Restricting system calls reduces the kernel attack surface and limits
> > what the process can do when compromised.
> > 
> > Signed-off-by: Stefan Hajnoczi <address@hidden>
> > with additional entries by:
> > Signed-off-by: Ganesh Maharaj Mahalingam <address@hidden>
> > Signed-off-by: Masayoshi Mizuma <address@hidden>
> > Signed-off-by: Misono Tomohiro <address@hidden>
> > Signed-off-by: piaojun <address@hidden>
> > Signed-off-by: Vivek Goyal <address@hidden>
> > Signed-off-by: Eric Ren <address@hidden>
> > ---
> >  Makefile                         |   2 +
> >  tools/virtiofsd/Makefile.objs    |   5 +-
> >  tools/virtiofsd/passthrough_ll.c |   2 +
> >  tools/virtiofsd/seccomp.c        | 141 +++++++++++++++++++++++++++++++
> >  tools/virtiofsd/seccomp.h        |  14 +++
> >  5 files changed, 163 insertions(+), 1 deletion(-)
> >  create mode 100644 tools/virtiofsd/seccomp.c
> >  create mode 100644 tools/virtiofsd/seccomp.h
> > 
> > diff --git a/Makefile b/Makefile
> > index 8a5746d8a0..3f5d04e1f7 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -322,8 +322,10 @@ HELPERS-y =
> >  HELPERS-$(call land,$(CONFIG_SOFTMMU),$(CONFIG_LINUX)) = 
> > qemu-bridge-helper$(EXESUF)
> >  
> >  ifdef CONFIG_LINUX
> > +ifdef CONFIG_SECCOMP
> >  HELPERS-y += virtiofsd$(EXESUF)
> >  vhost-user-json-y += tools/virtiofsd/50-qemu-virtiofsd.json
> > +endif
> >  
> >  ifdef CONFIG_VIRGL
> >  ifdef CONFIG_GBM
> > diff --git a/tools/virtiofsd/Makefile.objs b/tools/virtiofsd/Makefile.objs
> > index 67be16332c..941b19f18e 100644
> > --- a/tools/virtiofsd/Makefile.objs
> > +++ b/tools/virtiofsd/Makefile.objs
> > @@ -6,5 +6,8 @@ virtiofsd-obj-y = buffer.o \
> >                    fuse_signals.o \
> >                    fuse_virtio.o \
> >                    helper.o \
> > -                  passthrough_ll.o
> > +                  passthrough_ll.o \
> > +                  seccomp.o
> >  
> > +seccomp.o-cflags := $(SECCOMP_CFLAGS)
> > +seccomp.o-libs := $(SECCOMP_LIBS)
> > diff --git a/tools/virtiofsd/passthrough_ll.c 
> > b/tools/virtiofsd/passthrough_ll.c
> > index 754ef2618b..701608c6df 100644
> > --- a/tools/virtiofsd/passthrough_ll.c
> > +++ b/tools/virtiofsd/passthrough_ll.c
> > @@ -58,6 +58,7 @@
> >  #include <unistd.h>
> >  
> >  #include "passthrough_helpers.h"
> > +#include "seccomp.h"
> >  
> >  #define HAVE_POSIX_FALLOCATE 1
> >  struct lo_map_elem {
> > @@ -2073,6 +2074,7 @@ static void setup_sandbox(struct lo_data *lo, struct 
> > fuse_session *se)
> >  {
> >      setup_namespaces(lo, se);
> >      setup_mounts(lo->source);
> > +    setup_seccomp();
> >  }
> >  
> >  int main(int argc, char *argv[])
> > diff --git a/tools/virtiofsd/seccomp.c b/tools/virtiofsd/seccomp.c
> > new file mode 100644
> > index 0000000000..6359bb55bb
> > --- /dev/null
> > +++ b/tools/virtiofsd/seccomp.c
> > @@ -0,0 +1,141 @@
> > +/*
> > + * Seccomp sandboxing for virtiofsd
> > + *
> > + * Copyright (C) 2019 Red Hat, Inc.
> > + *
> > + * SPDX-License-Identifier: GPL-2.0-or-later
> > + */
> > +
> > +#include "seccomp.h"
> > +#include "fuse_i.h"
> > +#include "fuse_log.h"
> > +#include <errno.h>
> > +#include <glib.h>
> > +#include <seccomp.h>
> > +#include <stdlib.h>
> > +
> > +/* Bodge for libseccomp 2.4.2 which broke ppoll */
> > +#if !defined(__SNR_ppoll) && defined(__SNR_brk)
> > +#ifdef __NR_ppoll
> > +#define __SNR_ppoll __NR_ppoll
> > +#else
> > +#define __SNR_ppoll __PNR_ppoll
> > +#endif
> > +#endif
> > +
> > +static const int syscall_whitelist[] = {
> > +    /* TODO ireg sem*() syscalls */
> > +    SCMP_SYS(brk),
> > +    SCMP_SYS(capget), /* For CAP_FSETID */
> > +    SCMP_SYS(capset),
> > +    SCMP_SYS(clock_gettime),
> > +    SCMP_SYS(clone),
> 
> clone2 ?  clone3 ? IIC some archs in Linux
> will require the newer variants.

It looks like clone2 was an Itanium only thing; lets ignore that.
Clone3 is very new; so we're going to have to do:

  #ifdef __NR_clone3
  SCMP_SYS(clone3);
  #endif

> > +    SCMP_SYS(close),
> > +    SCMP_SYS(copy_file_range),
> > +    SCMP_SYS(dup),
> > +    SCMP_SYS(eventfd2),
> > +    SCMP_SYS(exit),
> > +    SCMP_SYS(exit_group),
> > +    SCMP_SYS(fallocate),
> > +    SCMP_SYS(fchmodat),
> > +    SCMP_SYS(fchownat),
> > +    SCMP_SYS(fcntl),
> > +    SCMP_SYS(fdatasync),
> > +    SCMP_SYS(fgetxattr),
> > +    SCMP_SYS(flistxattr),
> > +    SCMP_SYS(flock),
> > +    SCMP_SYS(fremovexattr),
> > +    SCMP_SYS(fsetxattr),
> > +    SCMP_SYS(fstat),
> > +    SCMP_SYS(fstatfs),
> > +    SCMP_SYS(fsync),
> > +    SCMP_SYS(ftruncate),
> > +    SCMP_SYS(futex),
> > +    SCMP_SYS(getdents),
> > +    SCMP_SYS(getdents64),
> > +    SCMP_SYS(getegid),
> > +    SCMP_SYS(geteuid),
> > +    SCMP_SYS(getpid),
> > +    SCMP_SYS(gettid),
> > +    SCMP_SYS(gettimeofday),
> > +    SCMP_SYS(linkat),
> > +    SCMP_SYS(lseek),
> > +    SCMP_SYS(madvise),
> > +    SCMP_SYS(mkdirat),
> > +    SCMP_SYS(mknodat),
> > +    SCMP_SYS(mmap),
> > +    SCMP_SYS(mprotect),
> > +    SCMP_SYS(mremap),
> > +    SCMP_SYS(munmap),
> > +    SCMP_SYS(newfstatat),
> > +    SCMP_SYS(open),
> > +    SCMP_SYS(openat),
> > +    SCMP_SYS(ppoll),
> > +    SCMP_SYS(prctl), /* TODO restrict to just PR_SET_NAME? */
> > +    SCMP_SYS(preadv),
> > +    SCMP_SYS(pread64),
> > +    SCMP_SYS(pwritev),
> > +    SCMP_SYS(pwrite64),
> > +    SCMP_SYS(read),
> > +    SCMP_SYS(readlinkat),
> > +    SCMP_SYS(recvmsg),
> > +    SCMP_SYS(renameat),
> > +    SCMP_SYS(renameat2),
> > +    SCMP_SYS(rt_sigaction),
> > +    SCMP_SYS(rt_sigprocmask),
> > +    SCMP_SYS(rt_sigreturn),
> > +    SCMP_SYS(sendmsg),
> > +    SCMP_SYS(setresgid),
> 
> Should be setresgid32 instead I think. We don't
> want the legacy syscall that's limted to 16-bit GIDs
> 
> Needs the code fix I mention in an earlier patch too.
> 
> > +    SCMP_SYS(setresuid),
> 
> Same as above

OK.
Interestingly I see setresuid/setresgid blacklisted as SET_PRIVILEGED
in qemu's qemu-secomp.c but not the 32 versions; perhaps those should
be added - but then I don't understand why qemu would ever allow them to
be enabled.

Dave

> > +    SCMP_SYS(set_robust_list),
> > +    SCMP_SYS(symlinkat),
> > +    SCMP_SYS(time), /* Rarely needed, except on static builds */
> > +    SCMP_SYS(tgkill),
> > +    SCMP_SYS(unlinkat),
> > +    SCMP_SYS(utimensat),
> > +    SCMP_SYS(write),
> > +    SCMP_SYS(writev),
> > +};
> 
> 
> 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 :|
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK