Re: [Qemu-devel] [PATCH 3/5] seccomp: add elevateprivileges argument to command line

[Eduardo Otubo]

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