Re: [libvirt] [PATCH] util: fix clear_emulator_capabilities=0

On 03/13/2013 06:09 PM, Eric Blake wrote: > On 03/13/2013 01:37 PM, Laine Stump wrote: >> My commit 7a2e845a865dc7fa82d2393ea2a770cfc8cf00b4 (and its >> prerequisites) managed to effectively ignore the >> clear_emulator_capabilities setting in qemu.conf (visible in the code >> as the VIR_EXEC_CLEAR_CAPS flag when qemu is being exec'ed), with the >> result that the capabilities are always cleared regardless of the >> qemu.conf setting. This patch fixes it by passing the flag through to >> virSetUIDGIDWithCaps(), which uses it to decide whether or not to >> clear existing capabilities before adding in those that were >> requested. >> >> Note that the existing capabilities are *always* cleared if the new >> process is going to run as non-root, since the whole point of running >> non-root is to have the capabilities removed (it's still possible to >> add back individual capabilities as needed though). >> --- >> This will need to be backported to v1.0.3-maint. > Yeah, now that Fedora 19 has branched and settled on 1.0.3 as its > starting point, it looks like v1.0.3-maint will be getting lots of fixes :) > >> + if (virSetUIDGIDWithCaps(cmd->uid, cmd->gid, cmd->capabilities, >> + (cmd->flags & VIR_EXEC_CLEAR_CAPS)) < 0) { > While gnulib guarantees that we have <stdbool.h>, it also states that we > cannot rely on C99 rules for slamming random integers into 1 when > converting into a bool context (especially true for C89 compilers using > gnulib's emulation, but apparently there are also buggy C99 compilers > that miscompile things). This should use '(cmd->flags & > VIR_EXEC_CLEAR_CAPS) != 0' (or !! if you don't like != 0), just to be safe. > >> + /* First drop all caps (unless the requested uid is "unchanged" or >> + * root and clearExistingCaps wasn't requested), then add back >> + * those in capBits + the extra ones we need to change uid/gid and >> + * change the capabilities bounding set. >> */ >> >> - capng_clear(CAPNG_SELECT_BOTH); >> + if (clearExistingCaps || (uid != 1 && uid != 0)) > Did you mean uid != 0? Well, actually I meant "uid != -1 && uid != 0" (and I had to look at it 5 times to see that the negative sign was missing). I want to clear the existing caps if: A) clearExistingCaps is true (duh) or B) The new uid will be different, and it will be something other than root > ACK with those problems addressed. >