[libvirt] [PATCH 1/5] util: simplify virSetUIDGIDWithCaps

[Daniel P. Berrange]

On Wed, Mar 27, 2013 at 04:36:38PM -0600, Eric Blake wrote:
> On 03/25/2013 08:25 AM, Paolo Bonzini wrote:
> > The need_prctl variable is not really needed.  If it is false,
> > capng_apply will be called twice with the same set, causing
> > a little extra work but no problem.  This keeps the code a bit
> > simpler.
> > 
> > It is also clearer to invoke capng_apply(CAPNG_SELECT_BOUNDS)
> > separately, to make sure it is done while we have CAP_SETPCAP.
> > 
> > Signed-off-by: Paolo Bonzini <pbonzini at redhat.com>
> > ---
> >  src/util/virutil.c | 18 +++++++++++-------
> >  1 file changed, 11 insertions(+), 7 deletions(-)
> 
> >      /* Change to the temp capabilities */
> > -    if ((capng_ret = capng_apply(CAPNG_SELECT_BOTH)) < 0) {
> > +    if ((capng_ret = capng_apply(CAPNG_SELECT_CAPS)) < 0) {
> 
> Beforehand, we limited both caps and bounding set, with an overlarge
> set, now you are limiting just caps...
> 
> >          virReportError(VIR_ERR_INTERNAL_ERROR,
> >                         _("cannot apply process capabilities %d"), capng_ret);
> >          goto cleanup;
> > @@ -3063,12 +3061,18 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long long capBits,
> >          goto cleanup;
> >  
> >      /* Tell it we are done keeping capabilities */
> > -    if (need_prctl && prctl(PR_SET_KEEPCAPS, 0, 0, 0, 0)) {
> > +    if (prctl(PR_SET_KEEPCAPS, 0, 0, 0, 0)) {
> >          virReportSystemError(errno, "%s",
> >                               _("prctl failed to reset KEEPCAPS"));
> >          goto cleanup;
> >      }
> >  
> > +    /* Set bounding set while we have CAP_SETPCAP.  Unfortunately we cannot
> > +     * do this if we failed to get the capability above, so ignore the
> > +     * return value.
> > +     */
> > +    capng_apply(CAPNG_SELECT_BOUNDS);
> 
> ...and then separately limiting bounds, but still while having an
> overlarge set.
> 
> > +
> >      /* Drop the caps that allow setuid/gid (unless they were requested) */
> >      if (need_setgid)
> >          capng_update(CAPNG_DROP, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_SETGID);
> > @@ -3078,7 +3082,7 @@ virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long long capBits,
> >      if (need_setpcap)
> >          capng_update(CAPNG_DROP, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_SETPCAP);
> 
> Here, the set is now pruned to size...
> 
> >  
> > -    if (need_prctl && ((capng_ret = capng_apply(CAPNG_SELECT_BOTH)) < 0)) {
> > +    if (((capng_ret = capng_apply(CAPNG_SELECT_CAPS)) < 0)) {
> 
> ...but you are now only limiting caps, not the bounding set.  Is that
> correct?

This method is derived from code in libcap-ng capng_change_id. Paulo's
changes actually make the libvirt code closer to what capng_change_id
does, so I think it is OK.

> Does this need to be considered for 1.0.4, or can we delay it to
> post-release?

I think we can wait for this whole series - it is really feature
work rather than critical bugfix.

Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|