[libvirt] [PATCH] util: fix clear_emulator_capabilities=0
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. src/util/vircommand.c | 4 +++- src/util/virutil.c | 16 ++++++++++------ src/util/virutil.h | 3 ++- 3 files changed, 15 insertions(+), 8 deletions(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index f672101..08d3d29 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -639,8 +639,10 @@ virExec(virCommandPtr cmd) cmd->capabilities || (cmd->flags & VIR_EXEC_CLEAR_CAPS)) { VIR_DEBUG("Setting child uid:gid to %d:%d with caps %llx", (int)cmd->uid, (int)cmd->gid, cmd->capabilities); - if (virSetUIDGIDWithCaps(cmd->uid, cmd->gid, cmd->capabilities) < 0) + if (virSetUIDGIDWithCaps(cmd->uid, cmd->gid, cmd->capabilities, + (cmd->flags & VIR_EXEC_CLEAR_CAPS)) < 0) { goto fork_error; + } } if (cmd->pwd) { diff --git a/src/util/virutil.c b/src/util/virutil.c index 4605c78..634b7e3 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -2998,18 +2998,21 @@ virGetGroupName(gid_t gid ATTRIBUTE_UNUSED) * errno). */ int -virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long long capBits) +virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long long capBits, + bool clearExistingCaps) { int ii, capng_ret, ret = -1; bool need_setgid = false, need_setuid = false; bool need_prctl = false, need_setpcap = false; - /* First drop all caps except those in capBits + the extra ones we - * need to change uid/gid and change the capabilities bounding - * set. + /* 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)) + capng_clear(CAPNG_SELECT_BOTH); for (ii = 0; ii <= CAP_LAST_CAP; ii++) { if (capBits & (1ULL << ii)) { @@ -3095,7 +3098,8 @@ cleanup: int virSetUIDGIDWithCaps(uid_t uid, gid_t gid, - unsigned long long capBits ATTRIBUTE_UNUSED) + unsigned long long capBits ATTRIBUTE_UNUSED, + bool clearExistingCaps ATTRIBUTE_UNUSED) { return virSetUIDGID(uid, gid); } diff --git a/src/util/virutil.h b/src/util/virutil.h index 2dc3403..665ad78 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -54,7 +54,8 @@ int virPipeReadUntilEOF(int outfd, int errfd, char **outbuf, char **errbuf); int virSetUIDGID(uid_t uid, gid_t gid); -int virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long long capBits); +int virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long long capBits, + bool clearExistingCaps); int virFileReadLimFD(int fd, int maxlen, char **buf) ATTRIBUTE_RETURN_CHECK; -- 1.8.1.4
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? ACK with those problems addressed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Wed, Mar 13, 2013 at 04:09:44PM -0600, 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 :)
Nah, we can continue to rebase Fedora 19 until either Beta release or the Virtualization test day. So we have at least one more release rebase possible. 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 :|
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.
On 03/14/2013 12:26 PM, Laine Stump wrote:
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.
Okay, I pushed it after adding "!!" to the bool arg and fixing the "1" to "-1". I also pushed the patch to the v1.0.3-maint branch.
On 03/14/2013 01:09 PM, Laine Stump wrote:
- 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).
'uid != -1' isn't portable; it has to be 'uid != (uid_t)-1'.
ACK with those problems addressed.
Okay, I pushed it after adding "!!" to the bool arg and fixing the "1" to "-1". I also pushed the patch to the v1.0.3-maint branch.
Guess we'll need to post the trivial followup patches to both places as well, then. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Daniel P. Berrange -
Eric Blake -
Laine Stump