* [Qemu-devel] [PATCH 0/5] seccomp: feature refactoring @ 2017-03-14 11:32 Eduardo Otubo 2017-03-14 11:32 ` [Qemu-devel] [PATCH 1/5] seccomp: changing from whitelist to blacklist Eduardo Otubo ` (5 more replies) 0 siblings, 6 replies; 20+ messages in thread From: Eduardo Otubo @ 2017-03-14 11:32 UTC (permalink / raw) To: qemu-devel; +Cc: berrange, thuth Hi all, This is first attempt of refactoring the seccomp feature, following Daniel's ideas: https://lists.gnu.org/archive/html/qemu-devel/2017-02/msg03348.html Best regards, Eduardo Otubo (5): seccomp: changing from whitelist to blacklist seccomp: add obsolete argument to command line seccomp: add elevateprivileges argument to command line seccomp: add spawn argument to command line seccomp: add resourcecontrol argument to command line include/sysemu/seccomp.h | 7 +- qemu-options.hx | 19 ++- qemu-seccomp.c | 375 ++++++++++++++++++----------------------------- vl.c | 49 ++++++- 4 files changed, 217 insertions(+), 233 deletions(-) -- 2.11.0 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH 1/5] seccomp: changing from whitelist to blacklist 2017-03-14 11:32 [Qemu-devel] [PATCH 0/5] seccomp: feature refactoring Eduardo Otubo @ 2017-03-14 11:32 ` Eduardo Otubo 2017-03-14 11:32 ` [Qemu-devel] [PATCH 2/5] seccomp: add obsolete argument to command line Eduardo Otubo ` (4 subsequent siblings) 5 siblings, 0 replies; 20+ messages in thread From: Eduardo Otubo @ 2017-03-14 11:32 UTC (permalink / raw) To: qemu-devel; +Cc: berrange, thuth 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 <eduardo.otubo@profitbricks.com> --- qemu-seccomp.c | 256 +++++++-------------------------------------------------- 1 file changed, 28 insertions(+), 228 deletions(-) diff --git a/qemu-seccomp.c b/qemu-seccomp.c index df75d9c471..f8877b07b5 100644 --- a/qemu-seccomp.c +++ b/qemu-seccomp.c @@ -31,229 +31,29 @@ struct QemuSeccompSyscall { uint8_t priority; }; -static const struct QemuSeccompSyscall seccomp_whitelist[] = { - { SCMP_SYS(timer_settime), 255 }, - { SCMP_SYS(timer_gettime), 254 }, - { SCMP_SYS(futex), 253 }, - { SCMP_SYS(select), 252 }, - { SCMP_SYS(recvfrom), 251 }, - { SCMP_SYS(sendto), 250 }, - { SCMP_SYS(socketcall), 250 }, - { SCMP_SYS(read), 249 }, - { SCMP_SYS(io_submit), 249 }, - { SCMP_SYS(brk), 248 }, - { SCMP_SYS(clone), 247 }, - { SCMP_SYS(mmap), 247 }, - { SCMP_SYS(mprotect), 246 }, - { SCMP_SYS(execve), 245 }, - { SCMP_SYS(open), 245 }, - { SCMP_SYS(ioctl), 245 }, - { SCMP_SYS(socket), 245 }, - { SCMP_SYS(setsockopt), 245 }, - { SCMP_SYS(recvmsg), 245 }, - { SCMP_SYS(sendmsg), 245 }, - { SCMP_SYS(accept), 245 }, - { SCMP_SYS(connect), 245 }, - { SCMP_SYS(socketpair), 245 }, - { SCMP_SYS(bind), 245 }, - { SCMP_SYS(listen), 245 }, - { SCMP_SYS(semget), 245 }, - { SCMP_SYS(ipc), 245 }, - { SCMP_SYS(gettimeofday), 245 }, - { SCMP_SYS(readlink), 245 }, - { SCMP_SYS(access), 245 }, - { SCMP_SYS(prctl), 245 }, - { SCMP_SYS(signalfd), 245 }, - { SCMP_SYS(getrlimit), 245 }, - { SCMP_SYS(getrusage), 245 }, - { SCMP_SYS(set_tid_address), 245 }, - { SCMP_SYS(statfs), 245 }, - { SCMP_SYS(unlink), 245 }, - { SCMP_SYS(wait4), 245 }, - { SCMP_SYS(fcntl64), 245 }, - { SCMP_SYS(fstat64), 245 }, - { SCMP_SYS(stat64), 245 }, - { SCMP_SYS(getgid32), 245 }, - { SCMP_SYS(getegid32), 245 }, - { SCMP_SYS(getuid32), 245 }, - { SCMP_SYS(geteuid32), 245 }, - { SCMP_SYS(sigreturn), 245 }, - { SCMP_SYS(_newselect), 245 }, - { SCMP_SYS(_llseek), 245 }, - { SCMP_SYS(mmap2), 245 }, - { SCMP_SYS(sigprocmask), 245 }, - { SCMP_SYS(sched_getparam), 245 }, - { SCMP_SYS(sched_getscheduler), 245 }, - { SCMP_SYS(fstat), 245 }, - { SCMP_SYS(clock_getres), 245 }, - { SCMP_SYS(sched_get_priority_min), 245 }, - { SCMP_SYS(sched_get_priority_max), 245 }, - { SCMP_SYS(stat), 245 }, - { SCMP_SYS(uname), 245 }, - { SCMP_SYS(eventfd2), 245 }, - { SCMP_SYS(io_getevents), 245 }, - { SCMP_SYS(dup), 245 }, - { SCMP_SYS(dup2), 245 }, - { SCMP_SYS(dup3), 245 }, - { SCMP_SYS(gettid), 245 }, - { SCMP_SYS(getgid), 245 }, - { SCMP_SYS(getegid), 245 }, - { SCMP_SYS(getuid), 245 }, - { SCMP_SYS(geteuid), 245 }, - { SCMP_SYS(timer_create), 245 }, - { SCMP_SYS(times), 245 }, - { SCMP_SYS(exit), 245 }, - { SCMP_SYS(clock_gettime), 245 }, - { SCMP_SYS(time), 245 }, - { SCMP_SYS(restart_syscall), 245 }, - { SCMP_SYS(pwrite64), 245 }, - { SCMP_SYS(nanosleep), 245 }, - { SCMP_SYS(chown), 245 }, - { SCMP_SYS(openat), 245 }, - { SCMP_SYS(getdents), 245 }, - { SCMP_SYS(timer_delete), 245 }, - { SCMP_SYS(exit_group), 245 }, - { SCMP_SYS(rt_sigreturn), 245 }, - { SCMP_SYS(sync), 245 }, - { SCMP_SYS(pread64), 245 }, - { SCMP_SYS(madvise), 245 }, - { SCMP_SYS(set_robust_list), 245 }, - { SCMP_SYS(lseek), 245 }, - { SCMP_SYS(pselect6), 245 }, - { SCMP_SYS(fork), 245 }, - { SCMP_SYS(rt_sigprocmask), 245 }, - { SCMP_SYS(write), 244 }, - { SCMP_SYS(fcntl), 243 }, - { SCMP_SYS(tgkill), 242 }, - { SCMP_SYS(kill), 242 }, - { SCMP_SYS(rt_sigaction), 242 }, - { SCMP_SYS(pipe2), 242 }, - { SCMP_SYS(munmap), 242 }, - { SCMP_SYS(mremap), 242 }, - { SCMP_SYS(fdatasync), 242 }, - { SCMP_SYS(close), 242 }, - { SCMP_SYS(rt_sigpending), 242 }, - { SCMP_SYS(rt_sigtimedwait), 242 }, - { SCMP_SYS(readv), 242 }, - { SCMP_SYS(writev), 242 }, - { SCMP_SYS(preadv), 242 }, - { SCMP_SYS(pwritev), 242 }, - { SCMP_SYS(setrlimit), 242 }, - { SCMP_SYS(ftruncate), 242 }, - { SCMP_SYS(lstat), 242 }, - { SCMP_SYS(pipe), 242 }, - { SCMP_SYS(umask), 242 }, - { SCMP_SYS(chdir), 242 }, - { SCMP_SYS(setitimer), 242 }, - { SCMP_SYS(setsid), 242 }, - { SCMP_SYS(poll), 242 }, - { SCMP_SYS(epoll_create), 242 }, - { SCMP_SYS(epoll_ctl), 242 }, - { SCMP_SYS(epoll_wait), 242 }, - { SCMP_SYS(waitpid), 242 }, - { SCMP_SYS(getsockname), 242 }, - { SCMP_SYS(getpeername), 242 }, - { SCMP_SYS(accept4), 242 }, - { SCMP_SYS(timerfd_settime), 242 }, - { SCMP_SYS(newfstatat), 241 }, - { SCMP_SYS(shutdown), 241 }, - { SCMP_SYS(getsockopt), 241 }, - { SCMP_SYS(semop), 241 }, - { SCMP_SYS(semtimedop), 241 }, - { SCMP_SYS(epoll_ctl_old), 241 }, - { SCMP_SYS(epoll_wait_old), 241 }, - { SCMP_SYS(epoll_pwait), 241 }, - { SCMP_SYS(epoll_create1), 241 }, - { SCMP_SYS(ppoll), 241 }, - { SCMP_SYS(creat), 241 }, - { SCMP_SYS(link), 241 }, - { SCMP_SYS(getpid), 241 }, - { SCMP_SYS(getppid), 241 }, - { SCMP_SYS(getpgrp), 241 }, - { SCMP_SYS(getpgid), 241 }, - { SCMP_SYS(getsid), 241 }, - { SCMP_SYS(getdents64), 241 }, - { SCMP_SYS(getresuid), 241 }, - { SCMP_SYS(getresgid), 241 }, - { SCMP_SYS(getgroups), 241 }, - { SCMP_SYS(getresuid32), 241 }, - { SCMP_SYS(getresgid32), 241 }, - { SCMP_SYS(getgroups32), 241 }, - { SCMP_SYS(signal), 241 }, - { SCMP_SYS(sigaction), 241 }, - { SCMP_SYS(sigsuspend), 241 }, - { SCMP_SYS(sigpending), 241 }, - { SCMP_SYS(truncate64), 241 }, - { SCMP_SYS(ftruncate64), 241 }, - { SCMP_SYS(fchown32), 241 }, - { SCMP_SYS(chown32), 241 }, - { SCMP_SYS(lchown32), 241 }, - { SCMP_SYS(statfs64), 241 }, - { SCMP_SYS(fstatfs64), 241 }, - { SCMP_SYS(fstatat64), 241 }, - { SCMP_SYS(lstat64), 241 }, - { SCMP_SYS(sendfile64), 241 }, - { SCMP_SYS(ugetrlimit), 241 }, - { SCMP_SYS(alarm), 241 }, - { SCMP_SYS(rt_sigsuspend), 241 }, - { SCMP_SYS(rt_sigqueueinfo), 241 }, - { SCMP_SYS(rt_tgsigqueueinfo), 241 }, - { SCMP_SYS(sigaltstack), 241 }, - { SCMP_SYS(signalfd4), 241 }, - { SCMP_SYS(truncate), 241 }, - { SCMP_SYS(fchown), 241 }, - { SCMP_SYS(lchown), 241 }, - { SCMP_SYS(fchownat), 241 }, - { SCMP_SYS(fstatfs), 241 }, - { SCMP_SYS(getitimer), 241 }, - { SCMP_SYS(syncfs), 241 }, - { SCMP_SYS(fsync), 241 }, - { SCMP_SYS(fchdir), 241 }, - { SCMP_SYS(msync), 241 }, - { SCMP_SYS(sched_setparam), 241 }, - { SCMP_SYS(sched_setscheduler), 241 }, - { SCMP_SYS(sched_yield), 241 }, - { SCMP_SYS(sched_rr_get_interval), 241 }, - { SCMP_SYS(sched_setaffinity), 241 }, - { SCMP_SYS(sched_getaffinity), 241 }, - { SCMP_SYS(readahead), 241 }, - { SCMP_SYS(timer_getoverrun), 241 }, - { SCMP_SYS(unlinkat), 241 }, - { SCMP_SYS(readlinkat), 241 }, - { SCMP_SYS(faccessat), 241 }, - { SCMP_SYS(get_robust_list), 241 }, - { SCMP_SYS(splice), 241 }, - { SCMP_SYS(vmsplice), 241 }, - { SCMP_SYS(getcpu), 241 }, - { SCMP_SYS(sendmmsg), 241 }, - { SCMP_SYS(recvmmsg), 241 }, - { SCMP_SYS(prlimit64), 241 }, - { SCMP_SYS(waitid), 241 }, - { SCMP_SYS(io_cancel), 241 }, - { SCMP_SYS(io_setup), 241 }, - { SCMP_SYS(io_destroy), 241 }, - { SCMP_SYS(arch_prctl), 240 }, - { SCMP_SYS(mkdir), 240 }, - { SCMP_SYS(fchmod), 240 }, - { SCMP_SYS(shmget), 240 }, - { SCMP_SYS(shmat), 240 }, - { SCMP_SYS(shmdt), 240 }, - { SCMP_SYS(timerfd_create), 240 }, - { SCMP_SYS(shmctl), 240 }, - { SCMP_SYS(mlockall), 240 }, - { SCMP_SYS(mlock), 240 }, - { SCMP_SYS(munlock), 240 }, - { SCMP_SYS(semctl), 240 }, - { SCMP_SYS(fallocate), 240 }, - { SCMP_SYS(fadvise64), 240 }, - { SCMP_SYS(inotify_init1), 240 }, - { SCMP_SYS(inotify_add_watch), 240 }, - { SCMP_SYS(mbind), 240 }, - { SCMP_SYS(memfd_create), 240 }, -#ifdef HAVE_CACHEFLUSH - { SCMP_SYS(cacheflush), 240 }, -#endif - { SCMP_SYS(sysinfo), 240 }, +static const struct QemuSeccompSyscall blacklist[] = { + { SCMP_SYS(reboot), 255 }, + { SCMP_SYS(swapon), 255 }, + { SCMP_SYS(swapoff), 255 }, + { SCMP_SYS(syslog), 255 }, + { SCMP_SYS(mount), 255 }, + { SCMP_SYS(umount), 255 }, + { SCMP_SYS(kexec_load), 255 }, + { SCMP_SYS(afs_syscall), 255 }, + { SCMP_SYS(break), 255 }, + { SCMP_SYS(ftime), 255 }, + { SCMP_SYS(getpmsg), 255 }, + { SCMP_SYS(gtty), 255 }, + { SCMP_SYS(lock), 255 }, + { SCMP_SYS(mpx), 255 }, + { SCMP_SYS(prof), 255 }, + { SCMP_SYS(profil), 255 }, + { SCMP_SYS(putpmsg), 255 }, + { SCMP_SYS(security), 255 }, + { SCMP_SYS(stty), 255 }, + { SCMP_SYS(tuxcall), 255 }, + { SCMP_SYS(ulimit), 255 }, + { SCMP_SYS(vserver), 255 }, }; int seccomp_start(void) @@ -262,19 +62,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); + for (i = 0; i < ARRAY_SIZE(blacklist); i++) { + rc = seccomp_rule_add(ctx, SCMP_ACT_KILL, blacklist[i].num, 0); if (rc < 0) { goto seccomp_return; } - rc = seccomp_syscall_priority(ctx, seccomp_whitelist[i].num, - seccomp_whitelist[i].priority); + rc = seccomp_syscall_priority(ctx, blacklist[i].num, + blacklist[i].priority); if (rc < 0) { goto seccomp_return; } -- 2.11.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH 2/5] seccomp: add obsolete argument to command line 2017-03-14 11:32 [Qemu-devel] [PATCH 0/5] seccomp: feature refactoring Eduardo Otubo 2017-03-14 11:32 ` [Qemu-devel] [PATCH 1/5] seccomp: changing from whitelist to blacklist Eduardo Otubo @ 2017-03-14 11:32 ` Eduardo Otubo 2017-03-14 11:47 ` Paolo Bonzini 2017-03-14 12:01 ` Thomas Huth 2017-03-14 11:32 ` [Qemu-devel] [PATCH 3/5] seccomp: add elevateprivileges " Eduardo Otubo ` (3 subsequent siblings) 5 siblings, 2 replies; 20+ messages in thread From: Eduardo Otubo @ 2017-03-14 11:32 UTC (permalink / raw) To: qemu-devel; +Cc: berrange, thuth This patch introduces the argument [,obsolete=allow] to the `-sandbox on' option. It allows Qemu to run safely on old system that still relies on old system calls. Signed-off-by: Eduardo Otubo <eduardo.otubo@profitbricks.com> --- include/sysemu/seccomp.h | 4 +++- qemu-options.hx | 9 +++++++-- qemu-seccomp.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++- vl.c | 16 +++++++++++++++- 4 files changed, 71 insertions(+), 5 deletions(-) diff --git a/include/sysemu/seccomp.h b/include/sysemu/seccomp.h index cfc06008cb..7a7bde246b 100644 --- a/include/sysemu/seccomp.h +++ b/include/sysemu/seccomp.h @@ -15,7 +15,9 @@ #ifndef QEMU_SECCOMP_H #define QEMU_SECCOMP_H +#define OBSOLETE 0x0001 + #include <seccomp.h> -int seccomp_start(void); +int seccomp_start(uint8_t seccomp_opts); #endif diff --git a/qemu-options.hx b/qemu-options.hx index 8dd8ee34a6..1403d0c85f 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -3732,13 +3732,18 @@ Old param mode (ARM only). ETEXI DEF("sandbox", HAS_ARG, QEMU_OPTION_sandbox, \ - "-sandbox <arg> Enable seccomp mode 2 system call filter (default 'off').\n", + "-sandbox on[,obsolete=allow] Enable seccomp mode 2 system call filter (default 'off').\n" \ + " obsolete: Allow obsolete system calls", QEMU_ARCH_ALL) STEXI -@item -sandbox @var{arg} +@item -sandbox @var{arg}[,obsolete=@var{string}] @findex -sandbox Enable Seccomp mode 2 system call filter. 'on' will enable syscall filtering and 'off' will disable it. The default is 'off'. +@table @option +@item obsolete=@var{string} +Enable Obsolete system calls +@end table ETEXI DEF("readconfig", HAS_ARG, QEMU_OPTION_readconfig, diff --git a/qemu-seccomp.c b/qemu-seccomp.c index f8877b07b5..5ef36890da 100644 --- a/qemu-seccomp.c +++ b/qemu-seccomp.c @@ -31,6 +31,35 @@ struct QemuSeccompSyscall { uint8_t priority; }; +static const struct QemuSeccompSyscall obsolete[] = { + { SCMP_SYS(readdir), 255 }, + { SCMP_SYS(_sysctl), 255 }, + { SCMP_SYS(afs_syscall), 255 }, + { SCMP_SYS(bdflush), 255 }, + { SCMP_SYS(break), 255 }, + { SCMP_SYS(create_module), 255 }, + { SCMP_SYS(ftime), 255 }, + { SCMP_SYS(get_kernel_syms), 255 }, + { SCMP_SYS(getpmsg), 255 }, + { SCMP_SYS(gtty), 255 }, + { SCMP_SYS(lock), 255 }, + { SCMP_SYS(mpx), 255 }, + { SCMP_SYS(prof), 255 }, + { SCMP_SYS(profil), 255 }, + { SCMP_SYS(putpmsg), 255 }, + { SCMP_SYS(query_module), 255 }, + { SCMP_SYS(security), 255 }, + { SCMP_SYS(sgetmask), 255 }, + { SCMP_SYS(ssetmask), 255 }, + { SCMP_SYS(stty), 255 }, + { SCMP_SYS(sysfs), 255 }, + { SCMP_SYS(tuxcall), 255 }, + { SCMP_SYS(ulimit), 255 }, + { SCMP_SYS(uselib), 255 }, + { SCMP_SYS(ustat), 255 }, + { SCMP_SYS(vserver), 255 }, +}; + static const struct QemuSeccompSyscall blacklist[] = { { SCMP_SYS(reboot), 255 }, { SCMP_SYS(swapon), 255 }, @@ -56,7 +85,20 @@ static const struct QemuSeccompSyscall blacklist[] = { { SCMP_SYS(vserver), 255 }, }; -int seccomp_start(void) +static int is_obsolete(int syscall) +{ + unsigned int i = 0; + + for (i = 0; i < ARRAY_SIZE(obsolete); i++) { + if (syscall == obsolete[i].num) { + return 1; + } + } + + return 0; +} + +int seccomp_start(uint8_t seccomp_opts) { int rc = 0; unsigned int i = 0; @@ -69,6 +111,9 @@ int seccomp_start(void) } for (i = 0; i < ARRAY_SIZE(blacklist); i++) { + if ((seccomp_opts & OBSOLETE) && is_obsolete(blacklist[i].num)) { + continue; + } 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 1a95500ac7..7b08b3383b 100644 --- a/vl.c +++ b/vl.c @@ -269,6 +269,10 @@ static QemuOptsList qemu_sandbox_opts = { .name = "enable", .type = QEMU_OPT_BOOL, }, + { + .name = "obsolete", + .type = QEMU_OPT_STRING, + }, { /* end of list */ } }, }; @@ -1031,7 +1035,17 @@ 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) { + uint8_t seccomp_opts = 0x0000; + const char * value = NULL; + + value = qemu_opt_get(opts,"obsolete"); + if (value) { + if (strcmp(value, "allow") == 0) { + seccomp_opts |= OBSOLETE; + } + } + + if (seccomp_start(seccomp_opts) < 0) { error_report("failed to install seccomp syscall filter " "in the kernel"); return -1; -- 2.11.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 2/5] seccomp: add obsolete argument to command line 2017-03-14 11:32 ` [Qemu-devel] [PATCH 2/5] seccomp: add obsolete argument to command line Eduardo Otubo @ 2017-03-14 11:47 ` Paolo Bonzini 2017-03-14 12:01 ` Thomas Huth 1 sibling, 0 replies; 20+ messages in thread From: Paolo Bonzini @ 2017-03-14 11:47 UTC (permalink / raw) To: Eduardo Otubo, qemu-devel; +Cc: thuth On 14/03/2017 12:32, Eduardo Otubo wrote: > +static const struct QemuSeccompSyscall obsolete[] = { > + { SCMP_SYS(readdir), 255 }, > + { SCMP_SYS(_sysctl), 255 }, > + { SCMP_SYS(afs_syscall), 255 }, > + { SCMP_SYS(bdflush), 255 }, > + { SCMP_SYS(break), 255 }, > + { SCMP_SYS(create_module), 255 }, > + { SCMP_SYS(ftime), 255 }, > + { SCMP_SYS(get_kernel_syms), 255 }, > + { SCMP_SYS(getpmsg), 255 }, > + { SCMP_SYS(gtty), 255 }, > + { SCMP_SYS(lock), 255 }, > + { SCMP_SYS(mpx), 255 }, > + { SCMP_SYS(prof), 255 }, > + { SCMP_SYS(profil), 255 }, > + { SCMP_SYS(putpmsg), 255 }, > + { SCMP_SYS(query_module), 255 }, > + { SCMP_SYS(security), 255 }, > + { SCMP_SYS(sgetmask), 255 }, > + { SCMP_SYS(ssetmask), 255 }, > + { SCMP_SYS(stty), 255 }, > + { SCMP_SYS(sysfs), 255 }, > + { SCMP_SYS(tuxcall), 255 }, > + { SCMP_SYS(ulimit), 255 }, > + { SCMP_SYS(uselib), 255 }, > + { SCMP_SYS(ustat), 255 }, > + { SCMP_SYS(vserver), 255 }, > +}; Some of these are already blacklisted in patch 1. Paolo ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 2/5] seccomp: add obsolete argument to command line 2017-03-14 11:32 ` [Qemu-devel] [PATCH 2/5] seccomp: add obsolete argument to command line Eduardo Otubo 2017-03-14 11:47 ` Paolo Bonzini @ 2017-03-14 12:01 ` Thomas Huth 2017-03-14 12:22 ` Eduardo Otubo 1 sibling, 1 reply; 20+ messages in thread From: Thomas Huth @ 2017-03-14 12:01 UTC (permalink / raw) To: Eduardo Otubo, qemu-devel On 14.03.2017 12:32, Eduardo Otubo wrote: > This patch introduces the argument [,obsolete=allow] to the `-sandbox on' > option. It allows Qemu to run safely on old system that still relies on > old system calls. > > Signed-off-by: Eduardo Otubo <eduardo.otubo@profitbricks.com> > --- > include/sysemu/seccomp.h | 4 +++- > qemu-options.hx | 9 +++++++-- > qemu-seccomp.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++- > vl.c | 16 +++++++++++++++- > 4 files changed, 71 insertions(+), 5 deletions(-) > > diff --git a/include/sysemu/seccomp.h b/include/sysemu/seccomp.h > index cfc06008cb..7a7bde246b 100644 > --- a/include/sysemu/seccomp.h > +++ b/include/sysemu/seccomp.h > @@ -15,7 +15,9 @@ > #ifndef QEMU_SECCOMP_H > #define QEMU_SECCOMP_H > > +#define OBSOLETE 0x0001 > + > #include <seccomp.h> > > -int seccomp_start(void); > +int seccomp_start(uint8_t seccomp_opts); > #endif > diff --git a/qemu-options.hx b/qemu-options.hx > index 8dd8ee34a6..1403d0c85f 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -3732,13 +3732,18 @@ Old param mode (ARM only). > ETEXI > > DEF("sandbox", HAS_ARG, QEMU_OPTION_sandbox, \ > - "-sandbox <arg> Enable seccomp mode 2 system call filter (default 'off').\n", > + "-sandbox on[,obsolete=allow] Enable seccomp mode 2 system call filter (default 'off').\n" \ > + " obsolete: Allow obsolete system calls", > QEMU_ARCH_ALL) > STEXI > -@item -sandbox @var{arg} > +@item -sandbox @var{arg}[,obsolete=@var{string}] > @findex -sandbox > Enable Seccomp mode 2 system call filter. 'on' will enable syscall filtering and 'off' will > disable it. The default is 'off'. > +@table @option > +@item obsolete=@var{string} > +Enable Obsolete system calls > +@end table > ETEXI > > DEF("readconfig", HAS_ARG, QEMU_OPTION_readconfig, > diff --git a/qemu-seccomp.c b/qemu-seccomp.c > index f8877b07b5..5ef36890da 100644 > --- a/qemu-seccomp.c > +++ b/qemu-seccomp.c > @@ -31,6 +31,35 @@ struct QemuSeccompSyscall { > uint8_t priority; > }; > > +static const struct QemuSeccompSyscall obsolete[] = { > + { SCMP_SYS(readdir), 255 }, > + { SCMP_SYS(_sysctl), 255 }, > + { SCMP_SYS(afs_syscall), 255 }, > + { SCMP_SYS(bdflush), 255 }, > + { SCMP_SYS(break), 255 }, > + { SCMP_SYS(create_module), 255 }, > + { SCMP_SYS(ftime), 255 }, > + { SCMP_SYS(get_kernel_syms), 255 }, > + { SCMP_SYS(getpmsg), 255 }, > + { SCMP_SYS(gtty), 255 }, > + { SCMP_SYS(lock), 255 }, > + { SCMP_SYS(mpx), 255 }, > + { SCMP_SYS(prof), 255 }, > + { SCMP_SYS(profil), 255 }, > + { SCMP_SYS(putpmsg), 255 }, > + { SCMP_SYS(query_module), 255 }, > + { SCMP_SYS(security), 255 }, > + { SCMP_SYS(sgetmask), 255 }, > + { SCMP_SYS(ssetmask), 255 }, > + { SCMP_SYS(stty), 255 }, > + { SCMP_SYS(sysfs), 255 }, > + { SCMP_SYS(tuxcall), 255 }, > + { SCMP_SYS(ulimit), 255 }, > + { SCMP_SYS(uselib), 255 }, > + { SCMP_SYS(ustat), 255 }, > + { SCMP_SYS(vserver), 255 }, > +}; > + > static const struct QemuSeccompSyscall blacklist[] = { > { SCMP_SYS(reboot), 255 }, > { SCMP_SYS(swapon), 255 }, > @@ -56,7 +85,20 @@ static const struct QemuSeccompSyscall blacklist[] = { > { SCMP_SYS(vserver), 255 }, > }; > > -int seccomp_start(void) > +static int is_obsolete(int syscall) > +{ > + unsigned int i = 0; > + > + for (i = 0; i < ARRAY_SIZE(obsolete); i++) { > + if (syscall == obsolete[i].num) { > + return 1; > + } > + } > + > + return 0; > +} > + > +int seccomp_start(uint8_t seccomp_opts) > { > int rc = 0; > unsigned int i = 0; > @@ -69,6 +111,9 @@ int seccomp_start(void) > } > > for (i = 0; i < ARRAY_SIZE(blacklist); i++) { > + if ((seccomp_opts & OBSOLETE) && is_obsolete(blacklist[i].num)) { > + continue; > + } > 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 1a95500ac7..7b08b3383b 100644 > --- a/vl.c > +++ b/vl.c > @@ -269,6 +269,10 @@ static QemuOptsList qemu_sandbox_opts = { > .name = "enable", > .type = QEMU_OPT_BOOL, > }, > + { > + .name = "obsolete", > + .type = QEMU_OPT_STRING, > + }, Since this is basically a boolean switch, could you maybe rather use "QEMU_OPT_BOOL" here, too (and also in the following patches)? I think it is also easier for the users to memorize "obsolete=on" instead of yet-another-parameter-syntax like "obsolete=allow". Thomas ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 2/5] seccomp: add obsolete argument to command line 2017-03-14 12:01 ` Thomas Huth @ 2017-03-14 12:22 ` Eduardo Otubo 0 siblings, 0 replies; 20+ messages in thread From: Eduardo Otubo @ 2017-03-14 12:22 UTC (permalink / raw) To: Thomas Huth; +Cc: qemu-devel, berrange On Tue, Mar 14, 2017 at 01=01=11PM +0100, Thomas Huth wrote: > On 14.03.2017 12:32, Eduardo Otubo wrote: > > This patch introduces the argument [,obsolete=allow] to the `-sandbox on' > > option. It allows Qemu to run safely on old system that still relies on > > old system calls. > > > > Signed-off-by: Eduardo Otubo <eduardo.otubo@profitbricks.com> > > --- > > include/sysemu/seccomp.h | 4 +++- > > qemu-options.hx | 9 +++++++-- > > qemu-seccomp.c | 47 ++++++++++++++++++++++++++++++++++++++++++++++- > > vl.c | 16 +++++++++++++++- > > 4 files changed, 71 insertions(+), 5 deletions(-) > > > > diff --git a/include/sysemu/seccomp.h b/include/sysemu/seccomp.h > > index cfc06008cb..7a7bde246b 100644 > > --- a/include/sysemu/seccomp.h > > +++ b/include/sysemu/seccomp.h > > @@ -15,7 +15,9 @@ > > #ifndef QEMU_SECCOMP_H > > #define QEMU_SECCOMP_H > > > > +#define OBSOLETE 0x0001 > > + > > #include <seccomp.h> > > > > -int seccomp_start(void); > > +int seccomp_start(uint8_t seccomp_opts); > > #endif > > diff --git a/qemu-options.hx b/qemu-options.hx > > index 8dd8ee34a6..1403d0c85f 100644 > > --- a/qemu-options.hx > > +++ b/qemu-options.hx > > @@ -3732,13 +3732,18 @@ Old param mode (ARM only). > > ETEXI > > > > DEF("sandbox", HAS_ARG, QEMU_OPTION_sandbox, \ > > - "-sandbox <arg> Enable seccomp mode 2 system call filter (default 'off').\n", > > + "-sandbox on[,obsolete=allow] Enable seccomp mode 2 system call filter (default 'off').\n" \ > > + " obsolete: Allow obsolete system calls", > > QEMU_ARCH_ALL) > > STEXI > > -@item -sandbox @var{arg} > > +@item -sandbox @var{arg}[,obsolete=@var{string}] > > @findex -sandbox > > Enable Seccomp mode 2 system call filter. 'on' will enable syscall filtering and 'off' will > > disable it. The default is 'off'. > > +@table @option > > +@item obsolete=@var{string} > > +Enable Obsolete system calls > > +@end table > > ETEXI > > > > DEF("readconfig", HAS_ARG, QEMU_OPTION_readconfig, > > diff --git a/qemu-seccomp.c b/qemu-seccomp.c > > index f8877b07b5..5ef36890da 100644 > > --- a/qemu-seccomp.c > > +++ b/qemu-seccomp.c > > @@ -31,6 +31,35 @@ struct QemuSeccompSyscall { > > uint8_t priority; > > }; > > > > +static const struct QemuSeccompSyscall obsolete[] = { > > + { SCMP_SYS(readdir), 255 }, > > + { SCMP_SYS(_sysctl), 255 }, > > + { SCMP_SYS(afs_syscall), 255 }, > > + { SCMP_SYS(bdflush), 255 }, > > + { SCMP_SYS(break), 255 }, > > + { SCMP_SYS(create_module), 255 }, > > + { SCMP_SYS(ftime), 255 }, > > + { SCMP_SYS(get_kernel_syms), 255 }, > > + { SCMP_SYS(getpmsg), 255 }, > > + { SCMP_SYS(gtty), 255 }, > > + { SCMP_SYS(lock), 255 }, > > + { SCMP_SYS(mpx), 255 }, > > + { SCMP_SYS(prof), 255 }, > > + { SCMP_SYS(profil), 255 }, > > + { SCMP_SYS(putpmsg), 255 }, > > + { SCMP_SYS(query_module), 255 }, > > + { SCMP_SYS(security), 255 }, > > + { SCMP_SYS(sgetmask), 255 }, > > + { SCMP_SYS(ssetmask), 255 }, > > + { SCMP_SYS(stty), 255 }, > > + { SCMP_SYS(sysfs), 255 }, > > + { SCMP_SYS(tuxcall), 255 }, > > + { SCMP_SYS(ulimit), 255 }, > > + { SCMP_SYS(uselib), 255 }, > > + { SCMP_SYS(ustat), 255 }, > > + { SCMP_SYS(vserver), 255 }, > > +}; > > + > > static const struct QemuSeccompSyscall blacklist[] = { > > { SCMP_SYS(reboot), 255 }, > > { SCMP_SYS(swapon), 255 }, > > @@ -56,7 +85,20 @@ static const struct QemuSeccompSyscall blacklist[] = { > > { SCMP_SYS(vserver), 255 }, > > }; > > > > -int seccomp_start(void) > > +static int is_obsolete(int syscall) > > +{ > > + unsigned int i = 0; > > + > > + for (i = 0; i < ARRAY_SIZE(obsolete); i++) { > > + if (syscall == obsolete[i].num) { > > + return 1; > > + } > > + } > > + > > + return 0; > > +} > > + > > +int seccomp_start(uint8_t seccomp_opts) > > { > > int rc = 0; > > unsigned int i = 0; > > @@ -69,6 +111,9 @@ int seccomp_start(void) > > } > > > > for (i = 0; i < ARRAY_SIZE(blacklist); i++) { > > + if ((seccomp_opts & OBSOLETE) && is_obsolete(blacklist[i].num)) { > > + continue; > > + } > > 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 1a95500ac7..7b08b3383b 100644 > > --- a/vl.c > > +++ b/vl.c > > @@ -269,6 +269,10 @@ static QemuOptsList qemu_sandbox_opts = { > > .name = "enable", > > .type = QEMU_OPT_BOOL, > > }, > > + { > > + .name = "obsolete", > > + .type = QEMU_OPT_STRING, > > + }, > > Since this is basically a boolean switch, could you maybe rather use > "QEMU_OPT_BOOL" here, too (and also in the following patches)? I think > it is also easier for the users to memorize "obsolete=on" instead of > yet-another-parameter-syntax like "obsolete=allow". > Yes, that's definitely in my mind. I just wanted to make the first draft the same way Daniel designed and evolve from there. -- Eduardo Otubo ProfitBricks GmbH ^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH 3/5] seccomp: add elevateprivileges argument to command line 2017-03-14 11:32 [Qemu-devel] [PATCH 0/5] seccomp: feature refactoring Eduardo Otubo 2017-03-14 11:32 ` [Qemu-devel] [PATCH 1/5] seccomp: changing from whitelist to blacklist Eduardo Otubo 2017-03-14 11:32 ` [Qemu-devel] [PATCH 2/5] seccomp: add obsolete argument to command line Eduardo Otubo @ 2017-03-14 11:32 ` Eduardo Otubo 2017-03-14 11:47 ` Paolo Bonzini 2017-03-14 11:32 ` [Qemu-devel] [PATCH 4/5] seccomp: add spawn " Eduardo Otubo ` (2 subsequent siblings) 5 siblings, 1 reply; 20+ messages in thread From: Eduardo Otubo @ 2017-03-14 11:32 UTC (permalink / raw) To: qemu-devel; +Cc: berrange, thuth This patch introduces the new argument [,elevateprivileges=deny] to the `-sandbox on'. It avoids Qemu process to elevate its privileges by blacklisting all set*uid|gid system calls Signed-off-by: Eduardo Otubo <eduardo.otubo@profitbricks.com> --- include/sysemu/seccomp.h | 1 + qemu-options.hx | 8 ++++++-- qemu-seccomp.c | 28 ++++++++++++++++++++++++++++ vl.c | 11 +++++++++++ 4 files changed, 46 insertions(+), 2 deletions(-) diff --git a/include/sysemu/seccomp.h b/include/sysemu/seccomp.h index 7a7bde246b..e6e78d85ce 100644 --- a/include/sysemu/seccomp.h +++ b/include/sysemu/seccomp.h @@ -16,6 +16,7 @@ #define QEMU_SECCOMP_H #define OBSOLETE 0x0001 +#define PRIVILEGED 0x0010 #include <seccomp.h> diff --git a/qemu-options.hx b/qemu-options.hx index 1403d0c85f..47018db5aa 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -3732,8 +3732,10 @@ Old param mode (ARM only). ETEXI DEF("sandbox", HAS_ARG, QEMU_OPTION_sandbox, \ - "-sandbox on[,obsolete=allow] Enable seccomp mode 2 system call filter (default 'off').\n" \ - " obsolete: Allow obsolete system calls", + "-sandbox on[,obsolete=allow][,elevateprivileges=deny]\n" \ + " Enable seccomp mode 2 system call filter (default 'off').\n" \ + " obsolete: Allow obsolete system calls\n" \ + " elevateprivileges: avoids Qemu process to elevate its privileges by blacklisting all set*uid|gid system calls", QEMU_ARCH_ALL) STEXI @item -sandbox @var{arg}[,obsolete=@var{string}] @@ -3743,6 +3745,8 @@ disable it. The default is 'off'. @table @option @item obsolete=@var{string} Enable Obsolete system calls +@item elevateprivileges=@var{string} +Disable set*uid|gid systema calls @end table ETEXI diff --git a/qemu-seccomp.c b/qemu-seccomp.c index 5ef36890da..5aa6590386 100644 --- a/qemu-seccomp.c +++ b/qemu-seccomp.c @@ -31,6 +31,19 @@ struct QemuSeccompSyscall { uint8_t priority; }; +static const struct QemuSeccompSyscall privileged_syscalls[] = { + { SCMP_SYS(setuid), 255 }, + { SCMP_SYS(setgid), 255 }, + { SCMP_SYS(setpgid), 255 }, + { SCMP_SYS(setsid), 255 }, + { SCMP_SYS(setreuid), 255 }, + { SCMP_SYS(setregid), 255 }, + { SCMP_SYS(setresuid), 255 }, + { SCMP_SYS(setresgid), 255 }, + { SCMP_SYS(setfsuid), 255 }, + { SCMP_SYS(setfsgid), 255 }, +}; + static const struct QemuSeccompSyscall obsolete[] = { { SCMP_SYS(readdir), 255 }, { SCMP_SYS(_sysctl), 255 }, @@ -125,6 +138,21 @@ int seccomp_start(uint8_t seccomp_opts) } } + if (seccomp_opts & PRIVILEGED) { + for (i = 0; i < ARRAY_SIZE(privileged_syscalls); i++) { + rc = seccomp_rule_add(ctx, SCMP_ACT_KILL, privileged_syscalls[i].num, 0); + if (rc < 0) { + goto seccomp_return; + } + rc = seccomp_syscall_priority(ctx, privileged_syscalls[i].num, + privileged_syscalls[i].priority); + if (rc < 0) { + goto seccomp_return; + } + } + } + + rc = seccomp_load(ctx); seccomp_return: diff --git a/vl.c b/vl.c index 7b08b3383b..d071e240b0 100644 --- a/vl.c +++ b/vl.c @@ -273,6 +273,10 @@ static QemuOptsList qemu_sandbox_opts = { .name = "obsolete", .type = QEMU_OPT_STRING, }, + { + .name = "elevateprivileges", + .type = QEMU_OPT_STRING, + }, { /* end of list */ } }, }; @@ -1045,6 +1049,13 @@ static int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp) } } + value = qemu_opt_get(opts,"elevateprivileges"); + if (value) { + if (strcmp(value, "deny") == 0) { + seccomp_opts |= PRIVILEGED; + } + } + if (seccomp_start(seccomp_opts) < 0) { error_report("failed to install seccomp syscall filter " "in the kernel"); -- 2.11.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] seccomp: add elevateprivileges argument to command line 2017-03-14 11:32 ` [Qemu-devel] [PATCH 3/5] seccomp: add elevateprivileges " Eduardo Otubo @ 2017-03-14 11:47 ` Paolo Bonzini 2017-03-14 11:52 ` Daniel P. Berrange 0 siblings, 1 reply; 20+ messages in thread From: Paolo Bonzini @ 2017-03-14 11:47 UTC (permalink / raw) To: Eduardo Otubo, qemu-devel; +Cc: thuth On 14/03/2017 12:32, Eduardo Otubo wrote: > This patch introduces the new argument [,elevateprivileges=deny] to the > `-sandbox on'. It avoids Qemu process to elevate its privileges by > blacklisting all set*uid|gid system calls > > Signed-off-by: Eduardo Otubo <eduardo.otubo@profitbricks.com> > --- > include/sysemu/seccomp.h | 1 + > qemu-options.hx | 8 ++++++-- > qemu-seccomp.c | 28 ++++++++++++++++++++++++++++ > vl.c | 11 +++++++++++ > 4 files changed, 46 insertions(+), 2 deletions(-) > > diff --git a/include/sysemu/seccomp.h b/include/sysemu/seccomp.h > index 7a7bde246b..e6e78d85ce 100644 > --- a/include/sysemu/seccomp.h > +++ b/include/sysemu/seccomp.h > @@ -16,6 +16,7 @@ > #define QEMU_SECCOMP_H > > #define OBSOLETE 0x0001 > +#define PRIVILEGED 0x0010 > > #include <seccomp.h> > > diff --git a/qemu-options.hx b/qemu-options.hx > index 1403d0c85f..47018db5aa 100644 > --- a/qemu-options.hx > +++ b/qemu-options.hx > @@ -3732,8 +3732,10 @@ Old param mode (ARM only). > ETEXI > > DEF("sandbox", HAS_ARG, QEMU_OPTION_sandbox, \ > - "-sandbox on[,obsolete=allow] Enable seccomp mode 2 system call filter (default 'off').\n" \ > - " obsolete: Allow obsolete system calls", > + "-sandbox on[,obsolete=allow][,elevateprivileges=deny]\n" \ > + " Enable seccomp mode 2 system call filter (default 'off').\n" \ > + " obsolete: Allow obsolete system calls\n" \ > + " elevateprivileges: avoids Qemu process to elevate its privileges by blacklisting all set*uid|gid system calls", Why allow these by default? Paolo > QEMU_ARCH_ALL) > STEXI > @item -sandbox @var{arg}[,obsolete=@var{string}] > @@ -3743,6 +3745,8 @@ disable it. The default is 'off'. > @table @option > @item obsolete=@var{string} > Enable Obsolete system calls > +@item elevateprivileges=@var{string} > +Disable set*uid|gid systema calls > @end table > ETEXI > > diff --git a/qemu-seccomp.c b/qemu-seccomp.c > index 5ef36890da..5aa6590386 100644 > --- a/qemu-seccomp.c > +++ b/qemu-seccomp.c > @@ -31,6 +31,19 @@ struct QemuSeccompSyscall { > uint8_t priority; > }; > > +static const struct QemuSeccompSyscall privileged_syscalls[] = { > + { SCMP_SYS(setuid), 255 }, > + { SCMP_SYS(setgid), 255 }, > + { SCMP_SYS(setpgid), 255 }, > + { SCMP_SYS(setsid), 255 }, > + { SCMP_SYS(setreuid), 255 }, > + { SCMP_SYS(setregid), 255 }, > + { SCMP_SYS(setresuid), 255 }, > + { SCMP_SYS(setresgid), 255 }, > + { SCMP_SYS(setfsuid), 255 }, > + { SCMP_SYS(setfsgid), 255 }, > +}; > + > static const struct QemuSeccompSyscall obsolete[] = { > { SCMP_SYS(readdir), 255 }, > { SCMP_SYS(_sysctl), 255 }, > @@ -125,6 +138,21 @@ int seccomp_start(uint8_t seccomp_opts) > } > } > > + if (seccomp_opts & PRIVILEGED) { > + for (i = 0; i < ARRAY_SIZE(privileged_syscalls); i++) { > + rc = seccomp_rule_add(ctx, SCMP_ACT_KILL, privileged_syscalls[i].num, 0); > + if (rc < 0) { > + goto seccomp_return; > + } > + rc = seccomp_syscall_priority(ctx, privileged_syscalls[i].num, > + privileged_syscalls[i].priority); > + if (rc < 0) { > + goto seccomp_return; > + } > + } > + } > + > + > rc = seccomp_load(ctx); > > seccomp_return: > diff --git a/vl.c b/vl.c > index 7b08b3383b..d071e240b0 100644 > --- a/vl.c > +++ b/vl.c > @@ -273,6 +273,10 @@ static QemuOptsList qemu_sandbox_opts = { > .name = "obsolete", > .type = QEMU_OPT_STRING, > }, > + { > + .name = "elevateprivileges", > + .type = QEMU_OPT_STRING, > + }, > { /* end of list */ } > }, > }; > @@ -1045,6 +1049,13 @@ static int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp) > } > } > > + value = qemu_opt_get(opts,"elevateprivileges"); > + if (value) { > + if (strcmp(value, "deny") == 0) { > + seccomp_opts |= PRIVILEGED; > + } > + } > + > if (seccomp_start(seccomp_opts) < 0) { > error_report("failed to install seccomp syscall filter " > "in the kernel"); > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] seccomp: add elevateprivileges argument to command line 2017-03-14 11:47 ` Paolo Bonzini @ 2017-03-14 11:52 ` Daniel P. Berrange 2017-03-14 12:13 ` Paolo Bonzini 0 siblings, 1 reply; 20+ messages in thread From: Daniel P. Berrange @ 2017-03-14 11:52 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Eduardo Otubo, qemu-devel, thuth On Tue, Mar 14, 2017 at 12:47:10PM +0100, Paolo Bonzini wrote: > > > On 14/03/2017 12:32, Eduardo Otubo wrote: > > This patch introduces the new argument [,elevateprivileges=deny] to the > > `-sandbox on'. It avoids Qemu process to elevate its privileges by > > blacklisting all set*uid|gid system calls > > > > Signed-off-by: Eduardo Otubo <eduardo.otubo@profitbricks.com> > > --- > > include/sysemu/seccomp.h | 1 + > > qemu-options.hx | 8 ++++++-- > > qemu-seccomp.c | 28 ++++++++++++++++++++++++++++ > > vl.c | 11 +++++++++++ > > 4 files changed, 46 insertions(+), 2 deletions(-) > > > > diff --git a/include/sysemu/seccomp.h b/include/sysemu/seccomp.h > > index 7a7bde246b..e6e78d85ce 100644 > > --- a/include/sysemu/seccomp.h > > +++ b/include/sysemu/seccomp.h > > @@ -16,6 +16,7 @@ > > #define QEMU_SECCOMP_H > > > > #define OBSOLETE 0x0001 > > +#define PRIVILEGED 0x0010 > > > > #include <seccomp.h> > > > > diff --git a/qemu-options.hx b/qemu-options.hx > > index 1403d0c85f..47018db5aa 100644 > > --- a/qemu-options.hx > > +++ b/qemu-options.hx > > @@ -3732,8 +3732,10 @@ Old param mode (ARM only). > > ETEXI > > > > DEF("sandbox", HAS_ARG, QEMU_OPTION_sandbox, \ > > - "-sandbox on[,obsolete=allow] Enable seccomp mode 2 system call filter (default 'off').\n" \ > > - " obsolete: Allow obsolete system calls", > > + "-sandbox on[,obsolete=allow][,elevateprivileges=deny]\n" \ > > + " Enable seccomp mode 2 system call filter (default 'off').\n" \ > > + " obsolete: Allow obsolete system calls\n" \ > > + " elevateprivileges: avoids Qemu process to elevate its privileges by blacklisting all set*uid|gid system calls", > > Why allow these by default? The goal is that '-sandbox on' should not break *any* QEMU feature. It needs to be a safe thing that people can unconditionally turn on without thinking about it. The QEMU bridge helper requires setuid privs, hence elevated privileges needs to be permitted by default. Similarly I could see the qemu ifup scripts, or migrate 'exec' command wanting to elevate privs via setuid binaries if QEMU was running unprivileged. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :| ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] seccomp: add elevateprivileges argument to command line 2017-03-14 11:52 ` Daniel P. Berrange @ 2017-03-14 12:13 ` Paolo Bonzini 2017-03-14 12:24 ` Daniel P. Berrange 2017-03-14 12:32 ` Eduardo Otubo 0 siblings, 2 replies; 20+ messages in thread From: Paolo Bonzini @ 2017-03-14 12:13 UTC (permalink / raw) To: Daniel P. Berrange; +Cc: Eduardo Otubo, qemu-devel, thuth, Andy Lutomirski On 14/03/2017 12:52, Daniel P. Berrange wrote: >>> DEF("sandbox", HAS_ARG, QEMU_OPTION_sandbox, \ >>> - "-sandbox on[,obsolete=allow] Enable seccomp mode 2 system call filter (default 'off').\n" \ >>> - " obsolete: Allow obsolete system calls", >>> + "-sandbox on[,obsolete=allow][,elevateprivileges=deny]\n" \ >>> + " Enable seccomp mode 2 system call filter (default 'off').\n" \ >>> + " obsolete: Allow obsolete system calls\n" \ >>> + " elevateprivileges: avoids Qemu process to elevate its privileges by blacklisting all set*uid|gid system calls", >> Why allow these by default? > The goal is that '-sandbox on' should not break *any* QEMU feature. It > needs to be a safe thing that people can unconditionally turn on without > thinking about it. Sure, but what advantages would it provide if the default blacklist does not block anything meaningful? At the very least, spawn=deny should default elevateprivileges to deny too. I think there should be a list (as small as possible) of features that are sacrificed by "-sandbox on". > The QEMU bridge helper requires setuid privs, hence > elevated privileges needs to be permitted by default. QEMU itself should not be getting additional privileges, only the helper and in turn the helper or ifup scripts can be limited through MAC. The issue is that seccomp persists across execve. Currently, unprivileged users are only allowed to install seccomp filters if no_new_privs is set. Would it make sense if seccomp filters without no_new_privs succeeded, except that the filter would not persist across execve of binaries with setuid, setgid or file capabilities? Then the spawn option could be a tri-state with the choice of allow, deny and no_new_privs: - elevateprivileges=allow,spawn=allow: legacy for old kernels - elevateprivileges=deny,spawn=allow: can run privileged helpers - elevateprivileges=deny,spawn=deny: cannot run helpers at all - elevateprivileges=deny,spawn=no_new_privs: can run unprivileged helpers only Paolo > Similarly I could > see the qemu ifup scripts, or migrate 'exec' command wanting to elevate > privs via setuid binaries if QEMU was running unprivileged. ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] seccomp: add elevateprivileges argument to command line 2017-03-14 12:13 ` Paolo Bonzini @ 2017-03-14 12:24 ` Daniel P. Berrange 2017-03-14 12:42 ` Eduardo Otubo 2017-03-14 12:32 ` Eduardo Otubo 1 sibling, 1 reply; 20+ messages in thread From: Daniel P. Berrange @ 2017-03-14 12:24 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Eduardo Otubo, qemu-devel, thuth, Andy Lutomirski On Tue, Mar 14, 2017 at 01:13:15PM +0100, Paolo Bonzini wrote: > > > On 14/03/2017 12:52, Daniel P. Berrange wrote: > >>> DEF("sandbox", HAS_ARG, QEMU_OPTION_sandbox, \ > >>> - "-sandbox on[,obsolete=allow] Enable seccomp mode 2 system call filter (default 'off').\n" \ > >>> - " obsolete: Allow obsolete system calls", > >>> + "-sandbox on[,obsolete=allow][,elevateprivileges=deny]\n" \ > >>> + " Enable seccomp mode 2 system call filter (default 'off').\n" \ > >>> + " obsolete: Allow obsolete system calls\n" \ > >>> + " elevateprivileges: avoids Qemu process to elevate its privileges by blacklisting all set*uid|gid system calls", > >> Why allow these by default? > > The goal is that '-sandbox on' should not break *any* QEMU feature. It > > needs to be a safe thing that people can unconditionally turn on without > > thinking about it. > > Sure, but what advantages would it provide if the default blacklist does > not block anything meaningful? At the very least, spawn=deny should > default elevateprivileges to deny too. Yep, having spawn=deny imply elevateprivileges=deny is reasonable IMHO. > I think there should be a list (as small as possible) of features that > are sacrificed by "-sandbox on". That breaks the key goal that '-sandbox on' should never break a valid QEMU configuration, no matter how obscure, and would thus continue to discourage people from turning it on by default. Yes, a bare '-sandbox on' is very loose, but think of it as just being a building block. 90% of the time the user or mgmt app would want to turn on extra flags to lock it down more meaningfully, by explicitly blocking ability to use feature they know won't be needed. > > The QEMU bridge helper requires setuid privs, hence > > elevated privileges needs to be permitted by default. > > QEMU itself should not be getting additional privileges, only the helper > and in turn the helper or ifup scripts can be limited through MAC. The > issue is that seccomp persists across execve. That's true. > Currently, unprivileged users are only allowed to install seccomp > filters if no_new_privs is set. Would it make sense if seccomp filters > without no_new_privs succeeded, except that the filter would not persist > across execve of binaries with setuid, setgid or file capabilities? > > Then the spawn option could be a tri-state with the choice of allow, > deny and no_new_privs: > > - elevateprivileges=allow,spawn=allow: legacy for old kernels > - elevateprivileges=deny,spawn=allow: can run privileged helpers > - elevateprivileges=deny,spawn=deny: cannot run helpers at all > - elevateprivileges=deny,spawn=no_new_privs: can run unprivileged > helpers only That could work, but I think that syntax is making it uneccessarily complex to understand. I don't like how it introduces a semantic dependancy between the elevateprivileges & spawn flags i.e. the interpretation of elevateprivileges=deny, varies according to what you set for spawn= option. I'd be more inclined to make elevateprivileges be a tri-state instead e.g. elevateprivileges=allow|deny|children Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :| ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] seccomp: add elevateprivileges argument to command line 2017-03-14 12:24 ` Daniel P. Berrange @ 2017-03-14 12:42 ` Eduardo Otubo 0 siblings, 0 replies; 20+ messages in thread From: Eduardo Otubo @ 2017-03-14 12:42 UTC (permalink / raw) To: Daniel P. Berrange; +Cc: Paolo Bonzini, qemu-devel, thuth, Andy Lutomirski On Tue, Mar 14, 2017 at 12=24=55PM +0000, Daniel P. Berrange wrote: > On Tue, Mar 14, 2017 at 01:13:15PM +0100, Paolo Bonzini wrote: > > > > > > On 14/03/2017 12:52, Daniel P. Berrange wrote: > > >>> DEF("sandbox", HAS_ARG, QEMU_OPTION_sandbox, \ > > >>> - "-sandbox on[,obsolete=allow] Enable seccomp mode 2 system call filter (default 'off').\n" \ > > >>> - " obsolete: Allow obsolete system calls", > > >>> + "-sandbox on[,obsolete=allow][,elevateprivileges=deny]\n" \ > > >>> + " Enable seccomp mode 2 system call filter (default 'off').\n" \ > > >>> + " obsolete: Allow obsolete system calls\n" \ > > >>> + " elevateprivileges: avoids Qemu process to elevate its privileges by blacklisting all set*uid|gid system calls", > > >> Why allow these by default? > > > The goal is that '-sandbox on' should not break *any* QEMU feature. It > > > needs to be a safe thing that people can unconditionally turn on without > > > thinking about it. > > > > Sure, but what advantages would it provide if the default blacklist does > > not block anything meaningful? At the very least, spawn=deny should > > default elevateprivileges to deny too. > > Yep, having spawn=deny imply elevateprivileges=deny is reasonable IMHO. > > > I think there should be a list (as small as possible) of features that > > are sacrificed by "-sandbox on". > > That breaks the key goal that '-sandbox on' should never break a valid > QEMU configuration, no matter how obscure, and would thus continue to > discourage people from turning it on by default. > > Yes, a bare '-sandbox on' is very loose, but think of it as just being > a building block. 90% of the time the user or mgmt app would want to > turn on extra flags to lock it down more meaningfully, by explicitly > blocking ability to use feature they know won't be needed. > > > > The QEMU bridge helper requires setuid privs, hence > > > elevated privileges needs to be permitted by default. > > > > QEMU itself should not be getting additional privileges, only the helper > > and in turn the helper or ifup scripts can be limited through MAC. The > > issue is that seccomp persists across execve. > > That's true. > > > Currently, unprivileged users are only allowed to install seccomp > > filters if no_new_privs is set. Would it make sense if seccomp filters > > without no_new_privs succeeded, except that the filter would not persist > > across execve of binaries with setuid, setgid or file capabilities? > > > > Then the spawn option could be a tri-state with the choice of allow, > > deny and no_new_privs: > > > > - elevateprivileges=allow,spawn=allow: legacy for old kernels > > - elevateprivileges=deny,spawn=allow: can run privileged helpers > > - elevateprivileges=deny,spawn=deny: cannot run helpers at all > > - elevateprivileges=deny,spawn=no_new_privs: can run unprivileged > > helpers only > > That could work, but I think that syntax is making it uneccessarily > complex to understand. I don't like how it introduces a semantic > dependancy between the elevateprivileges & spawn flags i.e. the > interpretation of elevateprivileges=deny, varies according to what > you set for spawn= option. > > I'd be more inclined to make elevateprivileges be a tri-state instead > e.g. > > elevateprivileges=allow|deny|children > Still weird but better than the combination of elevateprivileges and spawn. Perhaps this has a better semantics? elevateprivileges=allow|deny|no_new_privs -- Eduardo Otubo ProfitBricks GmbH ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] seccomp: add elevateprivileges argument to command line 2017-03-14 12:13 ` Paolo Bonzini 2017-03-14 12:24 ` Daniel P. Berrange @ 2017-03-14 12:32 ` Eduardo Otubo 2017-03-14 12:48 ` Paolo Bonzini 1 sibling, 1 reply; 20+ messages in thread From: Eduardo Otubo @ 2017-03-14 12:32 UTC (permalink / raw) To: Paolo Bonzini; +Cc: Daniel P. Berrange, qemu-devel, thuth, Andy Lutomirski On Tue, Mar 14, 2017 at 01=13=15PM +0100, Paolo Bonzini wrote: > > > On 14/03/2017 12:52, Daniel P. Berrange wrote: > >>> DEF("sandbox", HAS_ARG, QEMU_OPTION_sandbox, \ > >>> - "-sandbox on[,obsolete=allow] Enable seccomp mode 2 system call filter (default 'off').\n" \ > >>> - " obsolete: Allow obsolete system calls", > >>> + "-sandbox on[,obsolete=allow][,elevateprivileges=deny]\n" \ > >>> + " Enable seccomp mode 2 system call filter (default 'off').\n" \ > >>> + " obsolete: Allow obsolete system calls\n" \ > >>> + " elevateprivileges: avoids Qemu process to elevate its privileges by blacklisting all set*uid|gid system calls", > >> Why allow these by default? > > The goal is that '-sandbox on' should not break *any* QEMU feature. It > > needs to be a safe thing that people can unconditionally turn on without > > thinking about it. > > Sure, but what advantages would it provide if the default blacklist does > not block anything meaningful? At the very least, spawn=deny should > default elevateprivileges to deny too. > > I think there should be a list (as small as possible) of features that > are sacrificed by "-sandbox on". Can you give an example of such a list? > > > The QEMU bridge helper requires setuid privs, hence > > elevated privileges needs to be permitted by default. > > QEMU itself should not be getting additional privileges, only the helper > and in turn the helper or ifup scripts can be limited through MAC. The > issue is that seccomp persists across execve. > > Currently, unprivileged users are only allowed to install seccomp > filters if no_new_privs is set. Would it make sense if seccomp filters > without no_new_privs succeeded, except that the filter would not persist > across execve of binaries with setuid, setgid or file capabilities? Yes, it does make sense. Using seccomp_attr_set() and the flag SCMP_FLTATR_CTL_NNP we can disable NO_NEW_PRIVS. > > Then the spawn option could be a tri-state with the choice of allow, > deny and no_new_privs: > > - elevateprivileges=allow,spawn=allow: legacy for old kernels > - elevateprivileges=deny,spawn=allow: can run privileged helpers > - elevateprivileges=deny,spawn=deny: cannot run helpers at all > - elevateprivileges=deny,spawn=no_new_privs: can run unprivileged > helpers only I think it does look interesting to me this model. -- Eduardo Otubo ProfitBricks GmbH ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] seccomp: add elevateprivileges argument to command line 2017-03-14 12:32 ` Eduardo Otubo @ 2017-03-14 12:48 ` Paolo Bonzini 2017-03-14 13:02 ` Daniel P. Berrange 0 siblings, 1 reply; 20+ messages in thread From: Paolo Bonzini @ 2017-03-14 12:48 UTC (permalink / raw) To: Daniel P. Berrange, qemu-devel, thuth, Andy Lutomirski On 14/03/2017 13:32, Eduardo Otubo wrote: > On Tue, Mar 14, 2017 at 01=13=15PM +0100, Paolo Bonzini wrote: >> >> >> On 14/03/2017 12:52, Daniel P. Berrange wrote: >>>>> DEF("sandbox", HAS_ARG, QEMU_OPTION_sandbox, \ >>>>> - "-sandbox on[,obsolete=allow] Enable seccomp mode 2 system call filter (default 'off').\n" \ >>>>> - " obsolete: Allow obsolete system calls", >>>>> + "-sandbox on[,obsolete=allow][,elevateprivileges=deny]\n" \ >>>>> + " Enable seccomp mode 2 system call filter (default 'off').\n" \ >>>>> + " obsolete: Allow obsolete system calls\n" \ >>>>> + " elevateprivileges: avoids Qemu process to elevate its privileges by blacklisting all set*uid|gid system calls", >>>> Why allow these by default? >>> The goal is that '-sandbox on' should not break *any* QEMU feature. It >>> needs to be a safe thing that people can unconditionally turn on without >>> thinking about it. >> >> Sure, but what advantages would it provide if the default blacklist does >> not block anything meaningful? At the very least, spawn=deny should >> default elevateprivileges to deny too. >> >> I think there should be a list (as small as possible) of features that >> are sacrificed by "-sandbox on". > > Can you give an example of such a list? Well, anything that makes "-sandbox on" more than security theater... I would consider it a necessary evil, not a good thing to have such a list. Due to NO_NEW_PRIVS, I think non-migration fork/exec should be on the list, but hopefully nothing else. >>> The QEMU bridge helper requires setuid privs, hence >>> elevated privileges needs to be permitted by default. >> >> QEMU itself should not be getting additional privileges, only the helper >> and in turn the helper or ifup scripts can be limited through MAC. The >> issue is that seccomp persists across execve. >> >> Currently, unprivileged users are only allowed to install seccomp >> filters if no_new_privs is set. Would it make sense if seccomp filters >> without no_new_privs succeeded, except that the filter would not persist >> across execve of binaries with setuid, setgid or file capabilities? > > Yes, it does make sense. Using seccomp_attr_set() and the flag > SCMP_FLTATR_CTL_NNP we can disable NO_NEW_PRIVS. That however in turn requires CAP_SYS_ADMIN. Without kernel changes, it's not possible. Paolo >> >> Then the spawn option could be a tri-state with the choice of allow, >> deny and no_new_privs: >> >> - elevateprivileges=allow,spawn=allow: legacy for old kernels >> - elevateprivileges=deny,spawn=allow: can run privileged helpers >> - elevateprivileges=deny,spawn=deny: cannot run helpers at all >> - elevateprivileges=deny,spawn=no_new_privs: can run unprivileged >> helpers only > > I think it does look interesting to me this model. > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] seccomp: add elevateprivileges argument to command line 2017-03-14 12:48 ` Paolo Bonzini @ 2017-03-14 13:02 ` Daniel P. Berrange 2017-03-14 13:05 ` Paolo Bonzini 0 siblings, 1 reply; 20+ messages in thread From: Daniel P. Berrange @ 2017-03-14 13:02 UTC (permalink / raw) To: Paolo Bonzini; +Cc: qemu-devel, thuth, Andy Lutomirski On Tue, Mar 14, 2017 at 01:48:59PM +0100, Paolo Bonzini wrote: > > > On 14/03/2017 13:32, Eduardo Otubo wrote: > > On Tue, Mar 14, 2017 at 01=13=15PM +0100, Paolo Bonzini wrote: > >> > >> > >> On 14/03/2017 12:52, Daniel P. Berrange wrote: > >>>>> DEF("sandbox", HAS_ARG, QEMU_OPTION_sandbox, \ > >>>>> - "-sandbox on[,obsolete=allow] Enable seccomp mode 2 system call filter (default 'off').\n" \ > >>>>> - " obsolete: Allow obsolete system calls", > >>>>> + "-sandbox on[,obsolete=allow][,elevateprivileges=deny]\n" \ > >>>>> + " Enable seccomp mode 2 system call filter (default 'off').\n" \ > >>>>> + " obsolete: Allow obsolete system calls\n" \ > >>>>> + " elevateprivileges: avoids Qemu process to elevate its privileges by blacklisting all set*uid|gid system calls", > >>>> Why allow these by default? > >>> The goal is that '-sandbox on' should not break *any* QEMU feature. It > >>> needs to be a safe thing that people can unconditionally turn on without > >>> thinking about it. > >> > >> Sure, but what advantages would it provide if the default blacklist does > >> not block anything meaningful? At the very least, spawn=deny should > >> default elevateprivileges to deny too. > >> > >> I think there should be a list (as small as possible) of features that > >> are sacrificed by "-sandbox on". > > > > Can you give an example of such a list? > > Well, anything that makes "-sandbox on" more than security theater... I > would consider it a necessary evil, not a good thing to have such a list. > Due to NO_NEW_PRIVS, I think non-migration fork/exec should be on the > list, but hopefully nothing else. The current semantics of '-sandbox on' are that it is documented to not break any valid QEMU features. By blocking fork/exec, we make a semantic change to the existing behaviour of '-sandbox on'. So any application which has been using '-sandbox on' historically, is at risk of being broken when upgrading to QEMU 2.10. It would force the application to generate a different CLI for new QEMU - ie to avoid being broken they would have to now add elevateprivs=allow, spawn=allow. I think we have to maintain semantic compat with existing usage, which means that '-sandbox on' has to remain security theatre. So if we want a default config to be more restrictive, then I think you realistically have to turn the default parameter into a tri-state instead of bool, ie allow '-sandbox default' as an alternative to '-sandbox on' that was slightly stronger by blocking fork/exec. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :| ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] seccomp: add elevateprivileges argument to command line 2017-03-14 13:02 ` Daniel P. Berrange @ 2017-03-14 13:05 ` Paolo Bonzini 2017-03-14 13:19 ` Daniel P. Berrange 0 siblings, 1 reply; 20+ messages in thread From: Paolo Bonzini @ 2017-03-14 13:05 UTC (permalink / raw) To: Daniel P. Berrange; +Cc: qemu-devel, thuth, Andy Lutomirski On 14/03/2017 14:02, Daniel P. Berrange wrote: >>> Can you give an example of such a list? >> >> Well, anything that makes "-sandbox on" more than security theater... I >> would consider it a necessary evil, not a good thing to have such a list. >> >> Due to NO_NEW_PRIVS, I think non-migration fork/exec should be on the >> list, but hopefully nothing else. > > The current semantics of '-sandbox on' are that it is documented to not > break any valid QEMU features. By blocking fork/exec, we make a semantic > change to the existing behaviour of '-sandbox on'. I don't propose to block fork/exec. I propose not to whitelist setuid, as is the case now too. It wouldn't be backwards-incompatible. > So any application > which has been using '-sandbox on' historically, is at risk of being > broken when upgrading to QEMU 2.10. It would force the application to > generate a different CLI for new QEMU - ie to avoid being broken they > would have to now add elevateprivs=allow, spawn=allow. I think we have > to maintain semantic compat with existing usage, which means that > '-sandbox on' has to remain security theatre. > > So if we want a default config to be more restrictive, then I think you > realistically have to turn the default parameter into a tri-state > instead of bool, ie allow '-sandbox default' as an alternative to > '-sandbox on' that was slightly stronger by blocking fork/exec. Or just print a warning that "-sandbox on" is useless. I guess that would be okay too if we want to be "stronger" than backwards-compatible. Paolo ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 3/5] seccomp: add elevateprivileges argument to command line 2017-03-14 13:05 ` Paolo Bonzini @ 2017-03-14 13:19 ` Daniel P. Berrange 0 siblings, 0 replies; 20+ messages in thread From: Daniel P. Berrange @ 2017-03-14 13:19 UTC (permalink / raw) To: Paolo Bonzini; +Cc: qemu-devel, thuth, Andy Lutomirski On Tue, Mar 14, 2017 at 02:05:32PM +0100, Paolo Bonzini wrote: > > > On 14/03/2017 14:02, Daniel P. Berrange wrote: > >>> Can you give an example of such a list? > >> > >> Well, anything that makes "-sandbox on" more than security theater... I > >> would consider it a necessary evil, not a good thing to have such a list. > >> > >> Due to NO_NEW_PRIVS, I think non-migration fork/exec should be on the > >> list, but hopefully nothing else. > > > > The current semantics of '-sandbox on' are that it is documented to not > > break any valid QEMU features. By blocking fork/exec, we make a semantic > > change to the existing behaviour of '-sandbox on'. > > I don't propose to block fork/exec. I propose not to whitelist setuid, > as is the case now too. It wouldn't be backwards-incompatible. That we don't whitelist setuid is a bug in the current impl vs the intended semantics of '-sandbox on', where we are denying valid features. > > So any application > > which has been using '-sandbox on' historically, is at risk of being > > broken when upgrading to QEMU 2.10. It would force the application to > > generate a different CLI for new QEMU - ie to avoid being broken they > > would have to now add elevateprivs=allow, spawn=allow. I think we have > > to maintain semantic compat with existing usage, which means that > > '-sandbox on' has to remain security theatre. > > > > So if we want a default config to be more restrictive, then I think you > > realistically have to turn the default parameter into a tri-state > > instead of bool, ie allow '-sandbox default' as an alternative to > > '-sandbox on' that was slightly stronger by blocking fork/exec. > > Or just print a warning that "-sandbox on" is useless. I guess that > would be okay too if we want to be "stronger" than backwards-compatible. It isn't totally useless - it is blocking access to a number of system calls that QEMU should never use. eg things like reboot, load_module, etc. In the absence of other security protections it is useless, but if you combine with other mechanisms like DAC or LSM, then the default seccomp rules offer some extra protection. Admittedly not alot, but it is non-zero. The extra opt-in protecton flags then increase the value at cost of killing some features. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :| ^ permalink raw reply [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH 4/5] seccomp: add spawn argument to command line 2017-03-14 11:32 [Qemu-devel] [PATCH 0/5] seccomp: feature refactoring Eduardo Otubo ` (2 preceding siblings ...) 2017-03-14 11:32 ` [Qemu-devel] [PATCH 3/5] seccomp: add elevateprivileges " Eduardo Otubo @ 2017-03-14 11:32 ` Eduardo Otubo 2017-03-14 11:32 ` [Qemu-devel] [PATCH 5/5] seccomp: add resourcecontrol " Eduardo Otubo 2017-03-14 11:40 ` [Qemu-devel] [PATCH 0/5] seccomp: feature refactoring no-reply 5 siblings, 0 replies; 20+ messages in thread From: Eduardo Otubo @ 2017-03-14 11:32 UTC (permalink / raw) To: qemu-devel; +Cc: berrange, thuth This patch adds [,spawn=deny] argument to `-sandbox on' option. It blacklists fork and execve syste calls, avoiding Qemu to spawn new threads or processes. Signed-off-by: Eduardo Otubo <eduardo.otubo@profitbricks.com> --- include/sysemu/seccomp.h | 1 + qemu-options.hx | 7 +++++-- qemu-seccomp.c | 18 ++++++++++++++++++ vl.c | 11 +++++++++++ 4 files changed, 35 insertions(+), 2 deletions(-) diff --git a/include/sysemu/seccomp.h b/include/sysemu/seccomp.h index e6e78d85ce..f1614d6514 100644 --- a/include/sysemu/seccomp.h +++ b/include/sysemu/seccomp.h @@ -17,6 +17,7 @@ #define OBSOLETE 0x0001 #define PRIVILEGED 0x0010 +#define SPAWN 0x0100 #include <seccomp.h> diff --git a/qemu-options.hx b/qemu-options.hx index 47018db5aa..53f4f8cfd2 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -3732,10 +3732,11 @@ Old param mode (ARM only). ETEXI DEF("sandbox", HAS_ARG, QEMU_OPTION_sandbox, \ - "-sandbox on[,obsolete=allow][,elevateprivileges=deny]\n" \ + "-sandbox on[,obsolete=allow][,elevateprivileges=deny][,spawn=deny]" \ " Enable seccomp mode 2 system call filter (default 'off').\n" \ " obsolete: Allow obsolete system calls\n" \ - " elevateprivileges: avoids Qemu process to elevate its privileges by blacklisting all set*uid|gid system calls", + " elevateprivileges: avoids Qemu process to elevate its privileges by blacklisting all set*uid|gid system calls\n" \ + " spawn: avoids Qemu to spawn new threads or processes by blacklisting *fork and execve\n" QEMU_ARCH_ALL) STEXI @item -sandbox @var{arg}[,obsolete=@var{string}] @@ -3747,6 +3748,8 @@ disable it. The default is 'off'. Enable Obsolete system calls @item elevateprivileges=@var{string} Disable set*uid|gid systema calls +@item spawn=@var{string} +Disable *fork and execve @end table ETEXI diff --git a/qemu-seccomp.c b/qemu-seccomp.c index 5aa6590386..4c1f7b41ba 100644 --- a/qemu-seccomp.c +++ b/qemu-seccomp.c @@ -31,6 +31,12 @@ struct QemuSeccompSyscall { uint8_t priority; }; +static const struct QemuSeccompSyscall spawn_syscalls[] = { + { SCMP_SYS(fork), 255 }, + { SCMP_SYS(vfork), 255 }, + { SCMP_SYS(execve), 255 }, +}; + static const struct QemuSeccompSyscall privileged_syscalls[] = { { SCMP_SYS(setuid), 255 }, { SCMP_SYS(setgid), 255 }, @@ -152,6 +158,18 @@ int seccomp_start(uint8_t seccomp_opts) } } + if (seccomp_opts & SPAWN) { + for (i = 0; i < ARRAY_SIZE(spawn_syscalls); i++) { + rc = seccomp_rule_add(ctx, SCMP_ACT_KILL, spawn_syscalls[i].num, 0); + if (rc < 0) { + goto seccomp_return; + } + rc = seccomp_syscall_priority(ctx, spawn_syscalls[i].num, spawn_syscalls[i].priority); + if (rc < 0) { + goto seccomp_return; + } + } + } rc = seccomp_load(ctx); diff --git a/vl.c b/vl.c index d071e240b0..6a6e9a69bf 100644 --- a/vl.c +++ b/vl.c @@ -277,6 +277,10 @@ static QemuOptsList qemu_sandbox_opts = { .name = "elevateprivileges", .type = QEMU_OPT_STRING, }, + { + .name = "spawn", + .type = QEMU_OPT_STRING, + }, { /* end of list */ } }, }; @@ -1056,6 +1060,13 @@ static int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp) } } + value = qemu_opt_get(opts,"spawn"); + if (value) { + if (strcmp(value, "deny") == 0) { + seccomp_opts |= SPAWN; + } + } + if (seccomp_start(seccomp_opts) < 0) { error_report("failed to install seccomp syscall filter " "in the kernel"); -- 2.11.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* [Qemu-devel] [PATCH 5/5] seccomp: add resourcecontrol argument to command line 2017-03-14 11:32 [Qemu-devel] [PATCH 0/5] seccomp: feature refactoring Eduardo Otubo ` (3 preceding siblings ...) 2017-03-14 11:32 ` [Qemu-devel] [PATCH 4/5] seccomp: add spawn " Eduardo Otubo @ 2017-03-14 11:32 ` Eduardo Otubo 2017-03-14 11:40 ` [Qemu-devel] [PATCH 0/5] seccomp: feature refactoring no-reply 5 siblings, 0 replies; 20+ messages in thread From: Eduardo Otubo @ 2017-03-14 11:32 UTC (permalink / raw) To: qemu-devel; +Cc: berrange, thuth This patch adds [,resourcecontrol=deny] to `-sandbox on' option. It blacklists all process affinity and scheduler priority system calls to avoid any bigger of the process. Signed-off-by: Eduardo Otubo <eduardo.otubo@profitbricks.com> --- include/sysemu/seccomp.h | 1 + qemu-options.hx | 5 ++++- qemu-seccomp.c | 26 ++++++++++++++++++++++++++ vl.c | 11 +++++++++++ 4 files changed, 42 insertions(+), 1 deletion(-) diff --git a/include/sysemu/seccomp.h b/include/sysemu/seccomp.h index f1614d6514..c7003dd197 100644 --- a/include/sysemu/seccomp.h +++ b/include/sysemu/seccomp.h @@ -18,6 +18,7 @@ #define OBSOLETE 0x0001 #define PRIVILEGED 0x0010 #define SPAWN 0x0100 +#define RESOURCECTL 0x1000 #include <seccomp.h> diff --git a/qemu-options.hx b/qemu-options.hx index 53f4f8cfd2..5784ffe4b1 100644 --- a/qemu-options.hx +++ b/qemu-options.hx @@ -3732,11 +3732,12 @@ Old param mode (ARM only). ETEXI DEF("sandbox", HAS_ARG, QEMU_OPTION_sandbox, \ - "-sandbox on[,obsolete=allow][,elevateprivileges=deny][,spawn=deny]" \ + "-sandbox on[,obsolete=allow][,elevateprivileges=deny][,spawn=deny][,resourcecontrol=deny]\n" \ " Enable seccomp mode 2 system call filter (default 'off').\n" \ " obsolete: Allow obsolete system calls\n" \ " elevateprivileges: avoids Qemu process to elevate its privileges by blacklisting all set*uid|gid system calls\n" \ " spawn: avoids Qemu to spawn new threads or processes by blacklisting *fork and execve\n" + " resourcecontrol: disable process affinity and schedular priority\n", QEMU_ARCH_ALL) STEXI @item -sandbox @var{arg}[,obsolete=@var{string}] @@ -3750,6 +3751,8 @@ Enable Obsolete system calls Disable set*uid|gid systema calls @item spawn=@var{string} Disable *fork and execve +@item resourcecontrol=@var{string} +Disable process affinity and schedular priority @end table ETEXI diff --git a/qemu-seccomp.c b/qemu-seccomp.c index 4c1f7b41ba..dec47e9a74 100644 --- a/qemu-seccomp.c +++ b/qemu-seccomp.c @@ -31,6 +31,19 @@ struct QemuSeccompSyscall { uint8_t priority; }; +static const struct QemuSeccompSyscall resourcecontrol_syscalls[] = { + { SCMP_SYS(getpriority), 255 }, + { SCMP_SYS(setpriority), 255 }, + { SCMP_SYS(sched_setparam), 255 }, + { SCMP_SYS(sched_getparam), 255 }, + { SCMP_SYS(sched_setscheduler), 255 }, + { SCMP_SYS(sched_getscheduler), 255 }, + { SCMP_SYS(sched_setaffinity), 255 }, + { SCMP_SYS(sched_getaffinity), 255 }, + { SCMP_SYS(sched_get_priority_max), 255 }, + { SCMP_SYS(sched_get_priority_min), 255 }, +}; + static const struct QemuSeccompSyscall spawn_syscalls[] = { { SCMP_SYS(fork), 255 }, { SCMP_SYS(vfork), 255 }, @@ -171,6 +184,19 @@ int seccomp_start(uint8_t seccomp_opts) } } + if (seccomp_opts & RESOURCECTL) { + for (i = 0; i < ARRAY_SIZE(resourcecontrol_syscalls); i++) { + rc = seccomp_rule_add(ctx, SCMP_ACT_KILL, resourcecontrol_syscalls[i].num, 0); + if (rc < 0) { + goto seccomp_return; + } + rc = seccomp_syscall_priority(ctx, resourcecontrol_syscalls[i].num, resourcecontrol_syscalls[i].priority); + if (rc < 0) { + goto seccomp_return; + } + } + } + rc = seccomp_load(ctx); seccomp_return: diff --git a/vl.c b/vl.c index 6a6e9a69bf..3ceffef094 100644 --- a/vl.c +++ b/vl.c @@ -281,6 +281,10 @@ static QemuOptsList qemu_sandbox_opts = { .name = "spawn", .type = QEMU_OPT_STRING, }, + { + .name = "resourcecontrol", + .type = QEMU_OPT_STRING, + }, { /* end of list */ } }, }; @@ -1067,6 +1071,13 @@ static int parse_sandbox(void *opaque, QemuOpts *opts, Error **errp) } } + value = qemu_opt_get(opts,"resourcecontrol"); + if (value) { + if (strcmp(value, "deny") == 0) { + seccomp_opts |= RESOURCECTL; + } + } + if (seccomp_start(seccomp_opts) < 0) { error_report("failed to install seccomp syscall filter " "in the kernel"); -- 2.11.0 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [Qemu-devel] [PATCH 0/5] seccomp: feature refactoring 2017-03-14 11:32 [Qemu-devel] [PATCH 0/5] seccomp: feature refactoring Eduardo Otubo ` (4 preceding siblings ...) 2017-03-14 11:32 ` [Qemu-devel] [PATCH 5/5] seccomp: add resourcecontrol " Eduardo Otubo @ 2017-03-14 11:40 ` no-reply 5 siblings, 0 replies; 20+ messages in thread From: no-reply @ 2017-03-14 11:40 UTC (permalink / raw) To: eduardo.otubo; +Cc: famz, qemu-devel, thuth Hi, This series seems to have some coding style problems. See output below for more information: Type: series Subject: [Qemu-devel] [PATCH 0/5] seccomp: feature refactoring Message-id: 20170314113209.12025-1-eduardo.otubo@profitbricks.com === TEST SCRIPT BEGIN === #!/bin/bash BASE=base n=1 total=$(git log --oneline $BASE.. | wc -l) failed=0 # Useful git options git config --local diff.renamelimit 0 git config --local diff.renames True commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..." if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then failed=1 echo fi n=$((n+1)) done exit $failed === TEST SCRIPT END === Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384 From https://github.com/patchew-project/qemu * [new tag] patchew/1489491125-23648-1-git-send-email-peter.maydell@linaro.org -> patchew/1489491125-23648-1-git-send-email-peter.maydell@linaro.org * [new tag] patchew/20170314113209.12025-1-eduardo.otubo@profitbricks.com -> patchew/20170314113209.12025-1-eduardo.otubo@profitbricks.com Switched to a new branch 'test' 3f499bf seccomp: add resourcecontrol argument to command line 3ffba6f seccomp: add spawn argument to command line a830fdd seccomp: add elevateprivileges argument to command line 54c6390 seccomp: add obsolete argument to command line 7759286 seccomp: changing from whitelist to blacklist === OUTPUT BEGIN === Checking PATCH 1/5: seccomp: changing from whitelist to blacklist... Checking PATCH 2/5: seccomp: add obsolete argument to command line... ERROR: "foo * bar" should be "foo *bar" #146: FILE: vl.c:1039: + const char * value = NULL; ERROR: space required after that ',' (ctx:VxV) #148: FILE: vl.c:1041: + value = qemu_opt_get(opts,"obsolete"); ^ total: 2 errors, 0 warnings, 123 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. Checking PATCH 3/5: seccomp: add elevateprivileges argument to command line... WARNING: line over 80 characters #81: FILE: qemu-seccomp.c:143: + rc = seccomp_rule_add(ctx, SCMP_ACT_KILL, privileged_syscalls[i].num, 0); ERROR: space required after that ',' (ctx:VxV) #116: FILE: vl.c:1052: + value = qemu_opt_get(opts,"elevateprivileges"); ^ total: 1 errors, 1 warnings, 90 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. Checking PATCH 4/5: seccomp: add spawn argument to command line... ERROR: line over 90 characters #79: FILE: qemu-seccomp.c:167: + rc = seccomp_syscall_priority(ctx, spawn_syscalls[i].num, spawn_syscalls[i].priority); ERROR: space required after that ',' (ctx:VxV) #107: FILE: vl.c:1063: + value = qemu_opt_get(opts,"spawn"); ^ total: 2 errors, 0 warnings, 81 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. Checking PATCH 5/5: seccomp: add resourcecontrol argument to command line... WARNING: line over 80 characters #82: FILE: qemu-seccomp.c:189: + rc = seccomp_rule_add(ctx, SCMP_ACT_KILL, resourcecontrol_syscalls[i].num, 0); ERROR: line over 90 characters #86: FILE: qemu-seccomp.c:193: + rc = seccomp_syscall_priority(ctx, resourcecontrol_syscalls[i].num, resourcecontrol_syscalls[i].priority); ERROR: space required after that ',' (ctx:VxV) #115: FILE: vl.c:1074: + value = qemu_opt_get(opts,"resourcecontrol"); ^ total: 2 errors, 1 warnings, 89 lines checked Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS. === OUTPUT END === Test command exited with code: 1 --- Email generated automatically by Patchew [http://patchew.org/]. Please send your feedback to patchew-devel@freelists.org ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2017-03-14 13:19 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-14 11:32 [Qemu-devel] [PATCH 0/5] seccomp: feature refactoring Eduardo Otubo 2017-03-14 11:32 ` [Qemu-devel] [PATCH 1/5] seccomp: changing from whitelist to blacklist Eduardo Otubo 2017-03-14 11:32 ` [Qemu-devel] [PATCH 2/5] seccomp: add obsolete argument to command line Eduardo Otubo 2017-03-14 11:47 ` Paolo Bonzini 2017-03-14 12:01 ` Thomas Huth 2017-03-14 12:22 ` Eduardo Otubo 2017-03-14 11:32 ` [Qemu-devel] [PATCH 3/5] seccomp: add elevateprivileges " Eduardo Otubo 2017-03-14 11:47 ` Paolo Bonzini 2017-03-14 11:52 ` Daniel P. Berrange 2017-03-14 12:13 ` Paolo Bonzini 2017-03-14 12:24 ` Daniel P. Berrange 2017-03-14 12:42 ` Eduardo Otubo 2017-03-14 12:32 ` Eduardo Otubo 2017-03-14 12:48 ` Paolo Bonzini 2017-03-14 13:02 ` Daniel P. Berrange 2017-03-14 13:05 ` Paolo Bonzini 2017-03-14 13:19 ` Daniel P. Berrange 2017-03-14 11:32 ` [Qemu-devel] [PATCH 4/5] seccomp: add spawn " Eduardo Otubo 2017-03-14 11:32 ` [Qemu-devel] [PATCH 5/5] seccomp: add resourcecontrol " Eduardo Otubo 2017-03-14 11:40 ` [Qemu-devel] [PATCH 0/5] seccomp: feature refactoring no-reply
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.