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@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 :|