virExecWithHook is only called from one place, so it always has the same "hook" function (virHookCommand), and the data sent to that function is always a virCommandPtr, so eliminate the function and generic data from the arglist, and replace it with "virCommandPtr cmd". The call to (hook)(data) is replaced with "virHookCommand(cmd)". Finally, virExecWithHook is renamed to virExec. Indentation has been updated only for code that will remain after the next patch, which will remove all other args to virExec (since they are now redundant, as they're all members of virCommandPtr). --- Change from V1: rebased. src/util/vircommand.c | 79 ++++++++++++++++++++++++--------------------------- 1 file changed, 37 insertions(+), 42 deletions(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index db7dbca..02bf50d 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1,7 +1,7 @@ /* * vircommand.c: Child command execution * - * Copyright (C) 2010-2012 Red Hat, Inc. + * Copyright (C) 2010-2013 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -45,7 +45,7 @@ #define VIR_FROM_THIS VIR_FROM_NONE -/* Flags for virExecWithHook */ +/* Flags for virExec */ enum { VIR_EXEC_NONE = 0, VIR_EXEC_NONBLOCK = (1 << 0), @@ -104,6 +104,8 @@ struct _virCommand { unsigned long long capabilities; }; +static int virCommandHook(virCommandPtr cmd); + /* * virCommandFDIsSet: * @fd: FD to test @@ -390,21 +392,19 @@ prepareStdFd(int fd, int std) * VIR_EXEC_NONE : Default function behavior * VIR_EXEC_NONBLOCK : Set child process output fd's as non-blocking * VIR_EXEC_DAEMON : Daemonize the child process - * @hook optional virExecHook function to call prior to exec - * @data data to pass to the hook function + * @cmd virCommandPtr to pass to virCommandHook * @pidfile path to use as pidfile for daemonized process (needs DAEMON flag) * @capabilities capabilities to keep */ static int -virExecWithHook(const char *const*argv, +virExec(virCommandPtr cmd, + const char *const*argv, const char *const*envp, const int *keepfd, int keepfd_size, pid_t *retpid, int infd, int *outfd, int *errfd, unsigned int flags, - virExecHook hook, - void *data, char *pidfile, unsigned long long capabilities) { @@ -417,6 +417,7 @@ virExecWithHook(const char *const*argv, int tmpfd; const char *binary = NULL; int forkRet; + struct sigaction waxon, waxoff; if (argv[0][0] != '/') { if (!(binary = virFindFileInPath(argv[0]))) { @@ -602,33 +603,30 @@ virExecWithHook(const char *const*argv, } } - if (hook) { - /* virFork reset all signal handlers to the defaults. - * This is good for the child process, but our hook - * risks running something that generates SIGPIPE, - * so we need to temporarily block that again - */ - struct sigaction waxon, waxoff; - memset(&waxoff, 0, sizeof(waxoff)); - waxoff.sa_handler = SIG_IGN; - sigemptyset(&waxoff.sa_mask); - memset(&waxon, 0, sizeof(waxon)); - if (sigaction(SIGPIPE, &waxoff, &waxon) < 0) { - virReportSystemError(errno, "%s", - _("Could not disable SIGPIPE")); - goto fork_error; - } + /* virFork reset all signal handlers to the defaults. + * This is good for the child process, but our hook + * risks running something that generates SIGPIPE, + * so we need to temporarily block that again + */ + memset(&waxoff, 0, sizeof(waxoff)); + waxoff.sa_handler = SIG_IGN; + sigemptyset(&waxoff.sa_mask); + memset(&waxon, 0, sizeof(waxon)); + if (sigaction(SIGPIPE, &waxoff, &waxon) < 0) { + virReportSystemError(errno, "%s", + _("Could not disable SIGPIPE")); + goto fork_error; + } - if ((hook)(data) != 0) { - VIR_DEBUG("Hook function failed."); - goto fork_error; - } + if (virCommandHook(cmd) != 0) { + VIR_DEBUG("Hook function failed."); + goto fork_error; + } - if (sigaction(SIGPIPE, &waxon, NULL) < 0) { - virReportSystemError(errno, "%s", - _("Could not re-enable SIGPIPE")); - goto fork_error; - } + if (sigaction(SIGPIPE, &waxon, NULL) < 0) { + virReportSystemError(errno, "%s", + _("Could not re-enable SIGPIPE")); + goto fork_error; } /* The steps above may need todo something privileged, so @@ -712,7 +710,8 @@ virRun(const char *const *argv ATTRIBUTE_UNUSED, } static int -virExecWithHook(const char *const*argv ATTRIBUTE_UNUSED, +virExec(virCommandPtr cmd ATTRIBUTE_UNUSED, + const char *const*argv ATTRIBUTE_UNUSED, const char *const*envp ATTRIBUTE_UNUSED, const int *keepfd ATTRIBUTE_UNUSED, int keepfd_size ATTRIBUTE_UNUSED, @@ -721,15 +720,13 @@ virExecWithHook(const char *const*argv ATTRIBUTE_UNUSED, int *outfd ATTRIBUTE_UNUSED, int *errfd ATTRIBUTE_UNUSED, int flags_unused ATTRIBUTE_UNUSED, - virExecHook hook ATTRIBUTE_UNUSED, - void *data ATTRIBUTE_UNUSED, char *pidfile ATTRIBUTE_UNUSED, unsigned long long capabilities ATTRIBUTE_UNUSED) { /* XXX: Some day we can implement pieces of virCommand/virExec on * top of _spawn() or CreateProcess(), but we can't implement * everything, since mingw completely lacks fork(), so we cannot - * run hook code in the child. */ + * run our own code in the child process. */ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("virExec is not implemented for WIN32")); return -1; @@ -2061,9 +2058,8 @@ virCommandRun(virCommandPtr cmd, int *exitstatus) * Perform all virCommand-specific actions, along with the user hook. */ static int -virCommandHook(void *data) +virCommandHook(virCommandPtr cmd) { - virCommandPtr cmd = data; int res = 0; if (cmd->hook) { @@ -2401,7 +2397,8 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid) VIR_DEBUG("About to run %s", str ? str : cmd->args[0]); VIR_FREE(str); - ret = virExecWithHook((const char *const *)cmd->args, + ret = virExec(cmd, + (const char *const *)cmd->args, (const char *const *)cmd->env, cmd->preserve, cmd->preserve_size, @@ -2410,8 +2407,6 @@ virCommandRunAsync(virCommandPtr cmd, pid_t *pid) cmd->outfdptr, cmd->errfdptr, cmd->flags, - virCommandHook, - cmd, cmd->pidfile, cmd->capabilities); @@ -2549,7 +2544,7 @@ void virCommandAbort(virCommandPtr cmd ATTRIBUTE_UNUSED) { /* Mingw lacks WNOHANG and kill(). But since we haven't ported - * virExecWithHook to mingw yet, there's no process to be killed, + * virExec to mingw yet, there's no process to be killed, * making this implementation trivially correct for now :) */ } #endif -- 1.8.1

--- change from V1: rebased src/util/vircommand.c | 149 +++++++++++++++++++++++--------------------------- 1 file changed, 68 insertions(+), 81 deletions(-) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index b41c78b..ba81c14 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -104,7 +104,7 @@ struct _virCommand { unsigned long long capabilities; }; -static int virCommandHook(virCommandPtr cmd); +static int virCommandHandshakeChild(virCommandPtr cmd); /* * virCommandFDIsSet: @@ -394,7 +394,7 @@ virExec(virCommandPtr cmd) int childerr = -1; int tmpfd; const char *binary = NULL; - int forkRet; + int forkRet, ret; struct sigaction waxon, waxoff; if (cmd->args[0][0] != '/') { @@ -597,11 +597,26 @@ virExec(virCommandPtr cmd) goto fork_error; } - if (virCommandHook(cmd) != 0) { - VIR_DEBUG("Hook function failed."); - goto fork_error; + if (cmd->hook) { + VIR_DEBUG("Run hook %p %p", cmd->hook, cmd->opaque); + ret = cmd->hook(cmd->opaque); + VIR_DEBUG("Done hook %d", ret); + if (ret < 0) + goto fork_error; } + if (cmd->pwd) { + VIR_DEBUG("Running child in %s", cmd->pwd); + if (chdir(cmd->pwd) < 0) { + virReportSystemError(errno, + _("Unable to change to %s"), cmd->pwd); + goto fork_error; + } + } + + if (virCommandHandshakeChild(cmd) < 0) + goto fork_error; + if (sigaction(SIGPIPE, &waxon, NULL) < 0) { virReportSystemError(errno, "%s", _("Could not re-enable SIGPIPE")); @@ -2022,82 +2037,6 @@ virCommandRun(virCommandPtr cmd, int *exitstatus) } -/* - * Perform all virCommand-specific actions, along with the user hook. - */ -static int -virCommandHook(virCommandPtr cmd) -{ - int res = 0; - - if (cmd->hook) { - VIR_DEBUG("Run hook %p %p", cmd->hook, cmd->opaque); - res = cmd->hook(cmd->opaque); - VIR_DEBUG("Done hook %d", res); - } - if (res == 0 && cmd->pwd) { - VIR_DEBUG("Running child in %s", cmd->pwd); - res = chdir(cmd->pwd); - if (res < 0) { - virReportSystemError(errno, - _("Unable to change to %s"), cmd->pwd); - } - } - if (cmd->handshake) { - char c = res < 0 ? '0' : '1'; - int rv; - VIR_DEBUG("Notifying parent for handshake start on %d", - cmd->handshakeWait[1]); - if (safewrite(cmd->handshakeWait[1], &c, sizeof(c)) != sizeof(c)) { - virReportSystemError(errno, "%s", - _("Unable to notify parent process")); - return -1; - } - - /* On failure we pass the error message back to parent, - * so they don't have to dig through stderr logs - */ - if (res < 0) { - virErrorPtr err = virGetLastError(); - const char *msg = err ? err->message : - _("Unknown failure during hook execution"); - size_t len = strlen(msg) + 1; - if (safewrite(cmd->handshakeWait[1], msg, len) != len) { - virReportSystemError(errno, "%s", - _("Unable to send error to parent")); - return -1; - } - return -1; - } - - VIR_DEBUG("Waiting on parent for handshake complete on %d", - cmd->handshakeNotify[0]); - if ((rv = saferead(cmd->handshakeNotify[0], &c, - sizeof(c))) != sizeof(c)) { - if (rv < 0) - virReportSystemError(errno, "%s", - _("Unable to wait on parent process")); - else - virReportSystemError(EIO, "%s", - _("libvirtd quit during handshake")); - return -1; - } - if (c != '1') { - virReportSystemError(EINVAL, - _("Unexpected confirm code '%c' from parent"), - c); - return -1; - } - VIR_FORCE_CLOSE(cmd->handshakeWait[1]); - VIR_FORCE_CLOSE(cmd->handshakeNotify[0]); - } - - VIR_DEBUG("Hook is done %d", res); - - return res; -} - - static void virCommandHandleReadWrite(int watch, int fd, int events, void *opaque) { @@ -2546,6 +2485,54 @@ void virCommandRequireHandshake(virCommandPtr cmd) cmd->handshake = true; } +/* virCommandHandshakeChild: + * + * child side of handshake - called by child process in virExec() to + * indicate to parent that the child process has successfully + * completed its pre-exec initialization. + */ +static int +virCommandHandshakeChild(virCommandPtr cmd) +{ + char c = '1'; + int rv; + + if (!cmd->handshake) + return true; + + VIR_DEBUG("Notifying parent for handshake start on %d", + cmd->handshakeWait[1]); + if (safewrite(cmd->handshakeWait[1], &c, sizeof(c)) != sizeof(c)) { + virReportSystemError(errno, "%s", + _("Unable to notify parent process")); + return -1; + } + + VIR_DEBUG("Waiting on parent for handshake complete on %d", + cmd->handshakeNotify[0]); + if ((rv = saferead(cmd->handshakeNotify[0], &c, + sizeof(c))) != sizeof(c)) { + if (rv < 0) + virReportSystemError(errno, "%s", + _("Unable to wait on parent process")); + else + virReportSystemError(EIO, "%s", + _("libvirtd quit during handshake")); + return -1; + } + if (c != '1') { + virReportSystemError(EINVAL, + _("Unexpected confirm code '%c' from parent"), + c); + return -1; + } + VIR_FORCE_CLOSE(cmd->handshakeWait[1]); + VIR_FORCE_CLOSE(cmd->handshakeNotify[0]); + + VIR_DEBUG("Handshake with parent is done"); + return 0; +} + /** * virCommandHandshakeWait: * @cmd: command to wait on -- 1.8.1

On 02/12/2013 01:15 PM, Laine Stump wrote: Is it common enough to set both gid/uid at once, in order to make this a single function virCommandSetUIDGID? In fact, down at a lower layer in the stack, we pass both ids at once. Hmm, in the chown() case, doing both at once lets you use one syscall instead of two; but in the set*id() functions, it's separate syscalls for uid vs. gid no matter what we do, so I guess it doesn't really matter whether it is two separate calls or one combined call higher up in the stack. But what you have with separate calls works, so I don't mind whether you keep it as-is, to save the hassle of rippling a combined call through the rest of the series. ACK. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

Rather than treating uid:gid of 0:0 as a NOP, we blindly pass that through to the lower layers. However, we *do* check for a requested value of "-1" to mean "don't change this setting". setregid() and setreuid() already interpret -1 as a NOP, so this is just an optimization, but we are also calling getpwuid_r and initgroups, and it's unclear what the former would do with a uid of -1. --- Change from V1: * only bypass uid/gid setting if they are -1 rather than > 0 * cast -1 to ([gu]id_t) when comparing to a [gu]id_t src/util/virutil.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/util/virutil.c b/src/util/virutil.c index 24ba954..0d7db00 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -2687,7 +2687,7 @@ virSetUIDGID(uid_t uid, gid_t gid) int err; char *buf = NULL; - if (gid > 0) { + if (gid != (gid_t)-1) { if (setregid(gid, gid) < 0) { virReportSystemError(err = errno, _("cannot change to '%d' group"), @@ -2696,7 +2696,7 @@ virSetUIDGID(uid_t uid, gid_t gid) } } - if (uid > 0) { + if (uid != (uid_t)-1) { # ifdef HAVE_INITGROUPS struct passwd pwd, *pwd_result; size_t bufsize; -- 1.8.1

Setting the uid/gid of the child process was the only thing done by the hook function in this case, and that can now be done more simply with virCommandSetUID/GID. --- Change from V1: rebased. src/qemu/qemu_capabilities.c | 64 +++++++++++++------------------------------- 1 file changed, 18 insertions(+), 46 deletions(-) diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c index 4efe052..51fc9dc 100644 --- a/src/qemu/qemu_capabilities.c +++ b/src/qemu/qemu_capabilities.c @@ -279,37 +279,10 @@ static const char *virQEMUCapsArchToString(virArch arch) } -struct _virQEMUCapsHookData { - uid_t runUid; - gid_t runGid; -}; -typedef struct _virQEMUCapsHookData virQEMUCapsHookData; -typedef virQEMUCapsHookData *virQEMUCapsHookDataPtr; - -static int virQEMUCapsHook(void * data) -{ - int ret; - virQEMUCapsHookDataPtr hookData = data; - - if (!hookData) { - virReportError(VIR_ERR_INTERNAL_ERROR, "%s", - _("QEMU uid:gid not specified by caller")); - ret = -1; - goto cleanup; - } - - VIR_DEBUG("Switch QEMU uid:gid to %d:%d", - hookData->runUid, hookData->runGid); - ret = virSetUIDGID(hookData->runUid, hookData->runGid); - -cleanup: - return ret; -} - static virCommandPtr virQEMUCapsProbeCommand(const char *qemu, virQEMUCapsPtr qemuCaps, - virQEMUCapsHookDataPtr hookData) + uid_t runUid, gid_t runGid) { virCommandPtr cmd = virCommandNew(qemu); @@ -322,7 +295,8 @@ virQEMUCapsProbeCommand(const char *qemu, virCommandAddEnvPassCommon(cmd); virCommandClearCaps(cmd); - virCommandSetPreExecHook(cmd, virQEMUCapsHook, hookData); + virCommandSetGID(cmd, runGid); + virCommandSetUID(cmd, runUid); return cmd; } @@ -416,7 +390,8 @@ no_memory: } static int -virQEMUCapsProbeMachineTypes(virQEMUCapsPtr qemuCaps, virQEMUCapsHookDataPtr hookData) +virQEMUCapsProbeMachineTypes(virQEMUCapsPtr qemuCaps, + uid_t runUid, gid_t runGid) { char *output; int ret = -1; @@ -433,7 +408,7 @@ virQEMUCapsProbeMachineTypes(virQEMUCapsPtr qemuCaps, virQEMUCapsHookDataPtr hoo return -1; } - cmd = virQEMUCapsProbeCommand(qemuCaps->binary, qemuCaps, hookData); + cmd = virQEMUCapsProbeCommand(qemuCaps->binary, qemuCaps, runUid, runGid); virCommandAddArgList(cmd, "-M", "?", NULL); virCommandSetOutputBuffer(cmd, &output); @@ -572,7 +547,7 @@ cleanup: } static int -virQEMUCapsProbeCPUModels(virQEMUCapsPtr qemuCaps, virQEMUCapsHookDataPtr hookData) +virQEMUCapsProbeCPUModels(virQEMUCapsPtr qemuCaps, uid_t runUid, gid_t runGid) { char *output = NULL; int ret = -1; @@ -590,7 +565,7 @@ virQEMUCapsProbeCPUModels(virQEMUCapsPtr qemuCaps, virQEMUCapsHookDataPtr hookDa return 0; } - cmd = virQEMUCapsProbeCommand(qemuCaps->binary, qemuCaps, hookData); + cmd = virQEMUCapsProbeCommand(qemuCaps->binary, qemuCaps, runUid, runGid); virCommandAddArgList(cmd, "-cpu", "?", NULL); virCommandSetOutputBuffer(cmd, &output); @@ -1601,7 +1576,7 @@ virQEMUCapsParseDeviceStr(virQEMUCapsPtr qemuCaps, const char *str) static int virQEMUCapsExtractDeviceStr(const char *qemu, virQEMUCapsPtr qemuCaps, - virQEMUCapsHookDataPtr hookData) + uid_t runUid, gid_t runGid) { char *output = NULL; virCommandPtr cmd; @@ -1615,7 +1590,7 @@ virQEMUCapsExtractDeviceStr(const char *qemu, * understand '-device name,?', and always exits with status 1 for * the simpler '-device ?', so this function is really only useful * if -help includes "device driver,?". */ - cmd = virQEMUCapsProbeCommand(qemu, qemuCaps, hookData); + cmd = virQEMUCapsProbeCommand(qemu, qemuCaps, runUid, runGid); virCommandAddArgList(cmd, "-device", "?", "-device", "pci-assign,?", @@ -2183,7 +2158,6 @@ virQEMUCapsInitHelp(virQEMUCapsPtr qemuCaps, uid_t runUid, gid_t runGid) char *help = NULL; int ret = -1; const char *tmp; - virQEMUCapsHookData hookData; VIR_DEBUG("qemuCaps=%p", qemuCaps); @@ -2196,9 +2170,7 @@ virQEMUCapsInitHelp(virQEMUCapsPtr qemuCaps, uid_t runUid, gid_t runGid) qemuCaps->arch = virArchFromHost(); } - hookData.runUid = runUid; - hookData.runGid = runGid; - cmd = virQEMUCapsProbeCommand(qemuCaps->binary, NULL, &hookData); + cmd = virQEMUCapsProbeCommand(qemuCaps->binary, NULL, runUid, runGid); virCommandAddArgList(cmd, "-help", NULL); virCommandSetOutputBuffer(cmd, &help); @@ -2227,13 +2199,15 @@ virQEMUCapsInitHelp(virQEMUCapsPtr qemuCaps, uid_t runUid, gid_t runGid) * understands the 0.13.0+ notion of "-device driver,". */ if (virQEMUCapsGet(qemuCaps, QEMU_CAPS_DEVICE) && strstr(help, "-device driver,?") && - virQEMUCapsExtractDeviceStr(qemuCaps->binary, qemuCaps, &hookData) < 0) + virQEMUCapsExtractDeviceStr(qemuCaps->binary, + qemuCaps, runUid, runGid) < 0) { goto cleanup; + } - if (virQEMUCapsProbeCPUModels(qemuCaps, &hookData) < 0) + if (virQEMUCapsProbeCPUModels(qemuCaps, runUid, runGid) < 0) goto cleanup; - if (virQEMUCapsProbeMachineTypes(qemuCaps, &hookData) < 0) + if (virQEMUCapsProbeMachineTypes(qemuCaps, runUid, runGid) < 0) goto cleanup; ret = 0; @@ -2329,7 +2303,6 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, char *monarg = NULL; char *monpath = NULL; char *pidfile = NULL; - virQEMUCapsHookData hookData; char *archstr; pid_t pid = 0; virDomainObj vm; @@ -2383,9 +2356,8 @@ virQEMUCapsInitQMP(virQEMUCapsPtr qemuCaps, NULL); virCommandAddEnvPassCommon(cmd); virCommandClearCaps(cmd); - hookData.runUid = runUid; - hookData.runGid = runGid; - virCommandSetPreExecHook(cmd, virQEMUCapsHook, &hookData); + virCommandSetGID(cmd, runGid); + virCommandSetUID(cmd, runUid); if (virCommandRun(cmd, &status) < 0) goto cleanup; -- 1.8.1

--- Change from V1: * reset uid/gid to -1 rather than 0 when re-using virCommand src/storage/storage_backend.c | 28 ++++++---------------------- 1 file changed, 6 insertions(+), 22 deletions(-) diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index cab72c6..b0ddd46 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -1,7 +1,7 @@ /* * storage_backend.c: internal storage driver backend contract * - * Copyright (C) 2007-2012 Red Hat, Inc. + * Copyright (C) 2007-2013 Red Hat, Inc. * Copyright (C) 2007-2008 Daniel P. Berrange * * This library is free software; you can redistribute it and/or @@ -533,24 +533,6 @@ cleanup: return ret; } -struct hookdata { - virStorageVolDefPtr vol; - bool skip; -}; - -static int virStorageBuildSetUIDHook(void *data) { - struct hookdata *tmp = data; - virStorageVolDefPtr vol = tmp->vol; - - if (tmp->skip) - return 0; - - if (virSetUIDGID(vol->target.perms.uid, vol->target.perms.gid) < 0) - return -1; - - return 0; -} - static int virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, virStorageVolDefPtr vol, virCommandPtr cmd) { @@ -558,7 +540,6 @@ static int virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, gid_t gid; uid_t uid; int filecreated = 0; - struct hookdata data = {vol, false}; if ((pool->def->type == VIR_STORAGE_POOL_NETFS) && (((getuid() == 0) @@ -567,7 +548,8 @@ static int virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, || ((vol->target.perms.gid != -1) && (vol->target.perms.gid != getgid())))) { - virCommandSetPreExecHook(cmd, virStorageBuildSetUIDHook, &data); + virCommandSetUID(cmd, vol->target.perms.uid); + virCommandSetGID(cmd, vol->target.perms.gid); if (virCommandRun(cmd, NULL) == 0) { /* command was successfully run, check if the file was created */ @@ -576,7 +558,9 @@ static int virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, } } - data.skip = true; + /* don't change uid/gid if we retry */ + virCommandSetUID(cmd, -1); + virCommandSetGID(cmd, -1); if (!filecreated) { if (virCommandRun(cmd, NULL) < 0) { -- 1.8.1

This makes it simpler to include the necessary system security driver libraries for a particular system. For this patch, several existing conditional sections from the Makfile were replaced; I'll later be adding SECDRIVER_LIBS to libvirt_util_la_LIBADD, because vircommand.c will be calling a function from $securitylib. --- Change from V1: * initialize SECDRIVER_LIBS to empty * allow both WITH_SECDRIVER_SELINUX and WITH_SECDRIVER_APPARMOR in the same build src/Makefile.am | 33 ++++++++++++--------------------- 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/src/Makefile.am b/src/Makefile.am index d554aa1..12319e0 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -32,6 +32,14 @@ nodist_conf_DATA = THREAD_LIBS = $(LIB_PTHREAD) $(LTLIBMULTITHREAD) +SECDRIVER_LIBS = +if WITH_SECDRIVER_SELINUX +SECDRIVER_LIBS += $(SELINUX_LIBS) +endif +if WITH_SECDRIVER_APPARMOR +SECDRIVER_LIBS += $(APPARMOR_LIBS) +endif + if WITH_NETWORK UUID=$(shell uuidgen 2>/dev/null) endif @@ -987,12 +995,7 @@ if WITH_BLKID libvirt_driver_lxc_impl_la_CFLAGS += $(BLKID_CFLAGS) libvirt_driver_lxc_impl_la_LIBADD += $(BLKID_LIBS) endif -if WITH_SECDRIVER_SELINUX -libvirt_driver_lxc_impl_la_LIBADD += $(SELINUX_LIBS) -endif -if WITH_SECDRIVER_APPARMOR -libvirt_driver_lxc_impl_la_LIBADD += $(APPARMOR_LIBS) -endif +libvirt_driver_lxc_impl_la_LIBADD += $(SECDRIVER_LIBS) libvirt_driver_lxc_impl_la_SOURCES = $(LXC_DRIVER_SOURCES) conf_DATA += lxc/lxc.conf @@ -1159,12 +1162,7 @@ libvirt_driver_storage_impl_la_CFLAGS = \ -I$(top_srcdir)/src/conf $(AM_CFLAGS) libvirt_driver_storage_impl_la_LDFLAGS = $(AM_LDFLAGS) libvirt_driver_storage_impl_la_LIBADD = -if WITH_SECDRIVER_SELINUX -libvirt_driver_storage_impl_la_LIBADD += $(SELINUX_LIBS) -endif -if WITH_SECDRIVER_APPARMOR -libvirt_driver_storage_impl_la_LIBADD += $(APPARMOR_LIBS) -endif +libvirt_driver_storage_impl_la_LIBADD += $(SECDRIVER_LIBS) if WITH_BLKID libvirt_driver_storage_impl_la_CFLAGS += $(BLKID_CFLAGS) libvirt_driver_storage_impl_la_LIBADD += $(BLKID_LIBS) @@ -1277,16 +1275,14 @@ libvirt_la_BUILT_LIBADD += libvirt_security_manager.la libvirt_security_manager_la_CFLAGS = \ -I$(top_srcdir)/src/conf $(AM_CFLAGS) libvirt_security_manager_la_LDFLAGS = $(AM_LDFLAGS) -libvirt_security_manager_la_LIBADD = +libvirt_security_manager_la_LIBADD = $(SECDRIVER_LIBS) if WITH_SECDRIVER_SELINUX libvirt_security_manager_la_SOURCES += $(SECURITY_DRIVER_SELINUX_SOURCES) libvirt_security_manager_la_CFLAGS += $(SELINUX_CFLAGS) -libvirt_security_manager_la_LIBADD += $(SELINUX_LIBS) endif if WITH_SECDRIVER_APPARMOR libvirt_security_manager_la_SOURCES += $(SECURITY_DRIVER_APPARMOR_SOURCES) libvirt_security_manager_la_CFLAGS += $(APPARMOR_CFLAGS) -libvirt_security_manager_la_LIBADD += $(APPARMOR_LIBS) endif # Add all conditional sources just in case... @@ -1944,12 +1940,7 @@ libvirt_lxc_LDADD = \ if WITH_DTRACE_PROBES libvirt_lxc_LDADD += libvirt_probes.lo endif -if WITH_SECDRIVER_SELINUX -libvirt_lxc_LDADD += $(SELINUX_LIBS) -endif -if WITH_SECDRIVER_APPARMOR -libvirt_lxc_LDADD += $(APPARMOR_LIBS) -endif +libvirt_lxc_LDADD += $(SECDRIVER_LIBS) libvirt_lxc_CFLAGS = \ -I$(top_srcdir)/src/conf \ $(AM_CFLAGS) \ -- 1.8.1

virCommand gets two new APIs: virCommandSetSELinuxLabel() and virCommandSetAppArmorProfile(), which both save a copy of a null-terminated string in the virCommand. During virCommandRun, if the string is non-NULL and we've been compiled with AppArmor and/or SELinux security driver support, the appropriate security library function is called for the child process, using the string that was previously set. In the case of SELinux, setexeccon_raw() is called, and for AppArmor, aa_change_profile() is called. This functionality has been added so that users of virCommand can use the upcoming virSecurityManagerSetChildProcessLabel() prior to running a child process, rather than needing to setup a hook function to be called (and in turn call virSecurityManagerSetProcessLabel()) *during* the setup of the child process. --- Change from V1: * V1 had a single API that did double duty for both SELinux and AppArmor (because I didn't realize both could be built in simultaneously). V1 treats each separately, with two different APIs. src/Makefile.am | 3 +- src/libvirt_private.syms | 2 + src/util/vircommand.c | 95 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/vircommand.h | 6 +++ 4 files changed, 105 insertions(+), 1 deletion(-) diff --git a/src/Makefile.am b/src/Makefile.am index 12319e0..0d2f2f8 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -765,7 +765,8 @@ libvirt_util_la_CFLAGS = $(CAPNG_CFLAGS) $(YAJL_CFLAGS) $(LIBNL_CFLAGS) \ $(DBUS_CFLAGS) $(LDEXP_LIBM) libvirt_util_la_LIBADD = $(CAPNG_LIBS) $(YAJL_LIBS) $(LIBNL_LIBS) \ $(THREAD_LIBS) $(AUDIT_LIBS) $(DEVMAPPER_LIBS) \ - $(LIB_CLOCK_GETTIME) $(DBUS_LIBS) $(MSCOM_LIBS) $(LIBXML_LIBS) + $(LIB_CLOCK_GETTIME) $(DBUS_LIBS) $(MSCOM_LIBS) $(LIBXML_LIBS) \ + $(SECDRIVER_LIBS) noinst_LTLIBRARIES += libvirt_conf.la diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 511a686..8241e6f 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -156,6 +156,7 @@ virCommandPreserveFD; virCommandRequireHandshake; virCommandRun; virCommandRunAsync; +virCommandSetAppArmorProfile; virCommandSetErrorBuffer; virCommandSetErrorFD; virCommandSetGID; @@ -165,6 +166,7 @@ virCommandSetOutputBuffer; virCommandSetOutputFD; virCommandSetPidFile; virCommandSetPreExecHook; +virCommandSetSELinuxLabel; virCommandSetUID; virCommandSetWorkingDirectory; virCommandToString; diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 84d275f..9727809 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -33,6 +33,13 @@ # include <cap-ng.h> #endif +#if defined(WITH_SECDRIVER_SELINUX) +# include <selinux/selinux.h> +#endif +#if defined(WITH_SECDRIVER_APPARMOR) +# include <sys/apparmor.h> +#endif + #include "vircommand.h" #include "viralloc.h" #include "virerror.h" @@ -104,6 +111,12 @@ struct _virCommand { uid_t uid; gid_t gid; unsigned long long capabilities; +#if defined(WITH_SECDRIVER_SELINUX) + char *seLinuxLabel; +#endif +#if defined(WITH_SECDRIVER_APPARMOR) + char *appArmorProfile; +#endif }; static int virCommandHandshakeChild(virCommandPtr cmd); @@ -607,6 +620,32 @@ virExec(virCommandPtr cmd) goto fork_error; } +# if defined(WITH_SECDRIVER_SELINUX) + if (cmd->seLinuxLabel) { + VIR_DEBUG("Setting child security label to %s", cmd->seLinuxLabel); + if (setexeccon_raw(cmd->seLinuxLabel) == -1) { + virReportSystemError(errno, + _("unable to set SELinux security context " + "'%s' for '%s'"), + cmd->seLinuxLabel, cmd->args[0]); + if (security_getenforce() == 1) + goto fork_error; + } + } +# endif +# if defined(WITH_SECDRIVER_APPARMOR) + if (cmd->appArmorProfile) { + VIR_DEBUG("Setting child AppArmor profile to %s", cmd->appArmorProfile); + if (aa_change_profile(cmd->appArmorProfile) < 0) { + virReportSystemError(errno, + _("unable to set AppArmor profile '%s' " + "for '%s'"), + cmd->appArmorProfile, cmd->args[0]); + goto fork_error; + } + } +# endif + if (cmd->uid != (uid_t)-1 || cmd->gid != (gid_t)-1) { VIR_DEBUG("Setting child uid:gid to %d:%d", (int)cmd->uid, (int)cmd->gid); @@ -967,6 +1006,56 @@ virCommandAllowCap(virCommandPtr cmd, } +/** + * virCommandSetSELinuxLabel: + * @cmd: the command to modify + * @label: the SELinux label to use for the child process + * + * Saves a copy of @label to use when setting the SELinux context + * label (with setexeccon_raw()) after the child process has been + * started. If SELinux isn't compiled into libvirt, or if label is + * NULL, nothing will be done. + */ +void +virCommandSetSELinuxLabel(virCommandPtr cmd, + const char *label ATTRIBUTE_UNUSED) +{ + if (!cmd || cmd->has_error) + return; + +#if defined(WITH_SECDRIVER_SELINUX) + VIR_FREE(cmd->seLinuxLabel); + if (label && !(cmd->seLinuxLabel = strdup(label))) + cmd->has_error = ENOMEM; +#endif + return; +} + + +/** + * virCommandSetAppArmorProfile: + * @cmd: the command to modify + * @profile: the AppArmor profile to use + * + * Saves a copy of @profile to use when aa_change_profile() after the + * child process has been started. If AppArmor support isn't + * configured into libvirt, or if profile is NULL, nothing will be done. + */ +void +virCommandSetAppArmorProfile(virCommandPtr cmd, + const char *profile ATTRIBUTE_UNUSED) +{ + if (!cmd || cmd->has_error) + return; + +#if defined(WITH_SECDRIVER_APPARMOR) + VIR_FREE(cmd->appArmorProfile); + if (profile && !(cmd->appArmorProfile = strdup(profile))) + cmd->has_error = ENOMEM; +#endif + return; +} + /** * virCommandDaemonize: @@ -2715,6 +2804,12 @@ virCommandFree(virCommandPtr cmd) VIR_FREE(cmd->transfer); VIR_FREE(cmd->preserve); +#if defined(WITH_SECDRIVER_SELINUX) + VIR_FREE(cmd->seLinuxLabel); +#endif +#if defined(WITH_SECDRIVER_APPARMOR) + VIR_FREE(cmd->appArmorProfile); +#endif VIR_FREE(cmd); } diff --git a/src/util/vircommand.h b/src/util/vircommand.h index a4022fa..6c13795 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -70,6 +70,12 @@ void virCommandClearCaps(virCommandPtr cmd); void virCommandAllowCap(virCommandPtr cmd, int capability); +void virCommandSetSELinuxLabel(virCommandPtr cmd, + const char *label); + +void virCommandSetAppArmorProfile(virCommandPtr cmd, + const char *profile); + void virCommandDaemonize(virCommandPtr cmd); void virCommandNonblockingFDs(virCommandPtr cmd); -- 1.8.1

The existing virSecurityManagerSetProcessLabel() API is designed so that it must be called after forking the child process, but before exec'ing the child. Due to the way the virCommand API works, that means it needs to be put in a "hook" function that virCommand is told to call out to at that time. Setting the child process label is a basic enough need when executing any process that virCommand should have a method of doing that. But virCommand must be told what label to set, and only the security driver knows the answer to that question. The new virSecurityManagerSet*Child*ProcessLabel() API is the way to transfer the knowledge about what label to set from the security driver to the virCommand object. It is given a virCommandPtr, and each security driver calls the appropriate virCommand* API to tell virCommand what to do between fork and exec. 1) in the case of the DAC security driver, it calls virCommandSetUID/GID() to set a uid and gid that must be set for the child process. 2) for the SELinux security driver, it calls virCommandSetSELinuxLabel() to save a copy of the char* that will be sent to setexeccon_raw() *after forking the child process*. 3) for the AppArmor security drivers, it calls virCommandSetAppArmorProfile() to save a copy of the char* that will be sent to aa_change_profile() *after forking the child process*. With this new API in place, we will be able to remove virSecurityManagerSetProcessLabel() from any virCommand pre-exec hooks. (Unfortunately, the LXC driver uses clone() rather than virCommand, so it can't take advantage of this new security driver API, meaning that we need to keep around the older virSecurityManagerSetProcessLabel(), at least for now.) --- Change from V1: rebased, Copyright dates simplified. src/libvirt_private.syms | 1 + src/security/security_apparmor.c | 42 +++++++++++++++++++++++++++++++++++++++- src/security/security_dac.c | 24 ++++++++++++++++++++++- src/security/security_driver.h | 6 +++++- src/security/security_manager.c | 13 ++++++++++++- src/security/security_manager.h | 6 +++++- src/security/security_nop.c | 10 +++++++++- src/security/security_selinux.c | 32 ++++++++++++++++++++++++++++++ src/security/security_stack.c | 20 ++++++++++++++++++- 9 files changed, 147 insertions(+), 7 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 8241e6f..dcdcb67 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1068,6 +1068,7 @@ virSecurityManagerRestoreHostdevLabel; virSecurityManagerRestoreImageLabel; virSecurityManagerRestoreSavedStateLabel; virSecurityManagerSetAllLabel; +virSecurityManagerSetChildProcessLabel; virSecurityManagerSetDaemonSocketLabel; virSecurityManagerSetHostdevLabel; virSecurityManagerSetHugepages; diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 532b21b..ddc1fe4 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -1,7 +1,7 @@ /* * AppArmor security driver for libvirt * - * Copyright (C) 2011 Red Hat, Inc. + * Copyright (C) 2011-2013 Red Hat, Inc. * Copyright (C) 2009-2010 Canonical Ltd. * * This library is free software; you can redistribute it and/or @@ -627,6 +627,45 @@ AppArmorSetSecurityProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, return rc; } +/* Called directly by API user prior to virCommandRun(). + * virCommandRun() will then call aa_change_profile() (if a + * cmd->appArmorProfile has been set) *after forking the child + * process*. + */ +static int +AppArmorSetSecurityChildProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr def, + virCommandPtr cmd) +{ + int rc = -1; + char *profile_name = NULL; + const virSecurityLabelDefPtr secdef = + virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME); + + if (!secdef) + goto cleanup; + + if (STRNEQ(SECURITY_APPARMOR_NAME, secdef->model)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("security label driver mismatch: " + "\'%s\' model configured for domain, but " + "hypervisor driver is \'%s\'."), + secdef->model, SECURITY_APPARMOR_NAME); + if (use_apparmor() > 0) + goto cleanup; + } + + if ((profile_name = get_profile_name(def)) == NULL) + goto cleanup; + + virCommandSetAppArmorProfile(cmd, profile_name); + rc = 0; + + cleanup: + VIR_FREE(profile_name); + return rc; +} + static int AppArmorSetSecurityDaemonSocketLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr vm ATTRIBUTE_UNUSED) @@ -927,6 +966,7 @@ virSecurityDriver virAppArmorSecurityDriver = { .domainGetSecurityProcessLabel = AppArmorGetSecurityProcessLabel, .domainSetSecurityProcessLabel = AppArmorSetSecurityProcessLabel, + .domainSetSecurityChildProcessLabel = AppArmorSetSecurityChildProcessLabel, .domainSetSecurityAllLabel = AppArmorSetSecurityAllLabel, .domainRestoreSecurityAllLabel = AppArmorRestoreSecurityAllLabel, diff --git a/src/security/security_dac.c b/src/security/security_dac.c index ae489e2..b115bb0 100644 --- a/src/security/security_dac.c +++ b/src/security/security_dac.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2010-2012 Red Hat, Inc. + * Copyright (C) 2010-2013 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -878,6 +878,27 @@ virSecurityDACSetProcessLabel(virSecurityManagerPtr mgr, static int +virSecurityDACSetChildProcessLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def ATTRIBUTE_UNUSED, + virCommandPtr cmd) +{ + uid_t user; + gid_t group; + virSecurityDACDataPtr priv = virSecurityManagerGetPrivateData(mgr); + + if (virSecurityDACGetIds(def, priv, &user, &group)) + return -1; + + VIR_DEBUG("Setting child to drop privileges of DEF to %u:%u", + (unsigned int) user, (unsigned int) group); + + virCommandSetUID(cmd, user); + virCommandSetGID(cmd, group); + return 0; +} + + +static int virSecurityDACVerify(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr def ATTRIBUTE_UNUSED) { @@ -1072,6 +1093,7 @@ virSecurityDriver virSecurityDriverDAC = { .domainGetSecurityProcessLabel = virSecurityDACGetProcessLabel, .domainSetSecurityProcessLabel = virSecurityDACSetProcessLabel, + .domainSetSecurityChildProcessLabel = virSecurityDACSetChildProcessLabel, .domainSetSecurityAllLabel = virSecurityDACSetSecurityAllLabel, .domainRestoreSecurityAllLabel = virSecurityDACRestoreSecurityAllLabel, diff --git a/src/security/security_driver.h b/src/security/security_driver.h index 6b775ab..cc401e1 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2008, 2010 Red Hat, Inc. + * Copyright (C) 2008, 2010-2013 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -92,6 +92,9 @@ typedef int (*virSecurityDomainGetProcessLabel) (virSecurityManagerPtr mgr, virSecurityLabelPtr sec); typedef int (*virSecurityDomainSetProcessLabel) (virSecurityManagerPtr mgr, virDomainDefPtr def); +typedef int (*virSecurityDomainSetChildProcessLabel) (virSecurityManagerPtr mgr, + virDomainDefPtr def, + virCommandPtr cmd); typedef int (*virSecurityDomainSecurityVerify) (virSecurityManagerPtr mgr, virDomainDefPtr def); typedef int (*virSecurityDomainSetImageFDLabel) (virSecurityManagerPtr mgr, @@ -131,6 +134,7 @@ struct _virSecurityDriver { virSecurityDomainGetProcessLabel domainGetSecurityProcessLabel; virSecurityDomainSetProcessLabel domainSetSecurityProcessLabel; + virSecurityDomainSetChildProcessLabel domainSetSecurityChildProcessLabel; virSecurityDomainSetAllLabel domainSetSecurityAllLabel; virSecurityDomainRestoreAllLabel domainRestoreSecurityAllLabel; diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 50962ba..c621366 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -1,7 +1,7 @@ /* * security_manager.c: Internal security manager API * - * Copyright (C) 2010-2011 Red Hat, Inc. + * Copyright (C) 2010-2013 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -571,6 +571,17 @@ int virSecurityManagerSetProcessLabel(virSecurityManagerPtr mgr, return -1; } +int virSecurityManagerSetChildProcessLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + virCommandPtr cmd) +{ + if (mgr->drv->domainSetSecurityChildProcessLabel) + return mgr->drv->domainSetSecurityChildProcessLabel(mgr, vm, cmd); + + virReportError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + return -1; +} + int virSecurityManagerVerify(virSecurityManagerPtr mgr, virDomainDefPtr def) { diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 8e8accf..711b354 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -1,7 +1,7 @@ /* * security_manager.h: Internal security manager API * - * Copyright (C) 2010-2011 Red Hat, Inc. + * Copyright (C) 2010-2013 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -24,6 +24,7 @@ # define VIR_SECURITY_MANAGER_H__ # include "domain_conf.h" +# include "vircommand.h" typedef struct _virSecurityManager virSecurityManager; typedef virSecurityManager *virSecurityManagerPtr; @@ -103,6 +104,9 @@ int virSecurityManagerGetProcessLabel(virSecurityManagerPtr mgr, virSecurityLabelPtr sec); int virSecurityManagerSetProcessLabel(virSecurityManagerPtr mgr, virDomainDefPtr def); +int virSecurityManagerSetChildProcessLabel(virSecurityManagerPtr mgr, + virDomainDefPtr def, + virCommandPtr cmd); int virSecurityManagerVerify(virSecurityManagerPtr mgr, virDomainDefPtr def); int virSecurityManagerSetImageFDLabel(virSecurityManagerPtr mgr, diff --git a/src/security/security_nop.c b/src/security/security_nop.c index b6eb3f6..2b9767e 100644 --- a/src/security/security_nop.c +++ b/src/security/security_nop.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2010-2011 Red Hat, Inc. + * Copyright (C) 2010-2013 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -157,6 +157,13 @@ static int virSecurityDomainSetProcessLabelNop(virSecurityManagerPtr mgr ATTRIBU return 0; } +static int virSecurityDomainSetChildProcessLabelNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr vm ATTRIBUTE_UNUSED, + virCommandPtr cmd ATTRIBUTE_UNUSED) +{ + return 0; +} + static int virSecurityDomainVerifyNop(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr def ATTRIBUTE_UNUSED) { @@ -207,6 +214,7 @@ virSecurityDriver virSecurityDriverNop = { .domainGetSecurityProcessLabel = virSecurityDomainGetProcessLabelNop, .domainSetSecurityProcessLabel = virSecurityDomainSetProcessLabelNop, + .domainSetSecurityChildProcessLabel = virSecurityDomainSetChildProcessLabelNop, .domainSetSecurityAllLabel = virSecurityDomainSetAllLabelNop, .domainRestoreSecurityAllLabel = virSecurityDomainRestoreAllLabelNop, diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 8173b20..a61e0f0 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1859,6 +1859,37 @@ virSecuritySELinuxSetSecurityProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UN } static int +virSecuritySELinuxSetSecurityChildProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, + virDomainDefPtr def, + virCommandPtr cmd) +{ + /* TODO: verify DOI */ + virSecurityLabelDefPtr secdef; + + secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_SELINUX_NAME); + if (secdef == NULL) + return -1; + + if (secdef->label == NULL) + return 0; + + VIR_DEBUG("label=%s", secdef->label); + if (!STREQ(SECURITY_SELINUX_NAME, secdef->model)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("security label driver mismatch: " + "'%s' model configured for domain, but " + "hypervisor driver is '%s'."), + secdef->model, SECURITY_SELINUX_NAME); + if (security_getenforce() == 1) + return -1; + } + + /* save in cmd to be set after fork/before child process is exec'ed */ + virCommandSetSELinuxLabel(cmd, secdef->label); + return 0; +} + +static int virSecuritySELinuxSetSecurityDaemonSocketLabel(virSecurityManagerPtr mgr ATTRIBUTE_UNUSED, virDomainDefPtr def) { @@ -2261,6 +2292,7 @@ virSecurityDriver virSecurityDriverSELinux = { .domainGetSecurityProcessLabel = virSecuritySELinuxGetSecurityProcessLabel, .domainSetSecurityProcessLabel = virSecuritySELinuxSetSecurityProcessLabel, + .domainSetSecurityChildProcessLabel = virSecuritySELinuxSetSecurityChildProcessLabel, .domainSetSecurityAllLabel = virSecuritySELinuxSetSecurityAllLabel, .domainRestoreSecurityAllLabel = virSecuritySELinuxRestoreSecurityAllLabel, diff --git a/src/security/security_stack.c b/src/security/security_stack.c index e2d0b1d..14d757d 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2010-2011 Red Hat, Inc. + * Copyright (C) 2010-2013 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -367,6 +367,23 @@ virSecurityStackSetProcessLabel(virSecurityManagerPtr mgr, } static int +virSecurityStackSetChildProcessLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm, + virCommandPtr cmd) +{ + virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityStackItemPtr item = priv->itemsHead; + int rc = 0; + + for (; item; item = item->next) { + if (virSecurityManagerSetChildProcessLabel(item->securityManager, vm, cmd) < 0) + rc = -1; + } + + return rc; +} + +static int virSecurityStackGetProcessLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, pid_t pid, @@ -540,6 +557,7 @@ virSecurityDriver virSecurityDriverStack = { .domainGetSecurityProcessLabel = virSecurityStackGetProcessLabel, .domainSetSecurityProcessLabel = virSecurityStackSetProcessLabel, + .domainSetSecurityChildProcessLabel = virSecurityStackSetChildProcessLabel, .domainSetSecurityAllLabel = virSecurityStackSetSecurityAllLabel, .domainRestoreSecurityAllLabel = virSecurityStackRestoreSecurityAllLabel, -- 1.8.1

The qemu driver had been calling virSecurityManagerSetProcessLabel() from a "pre-exec hook" function that is run after the child is forked, but before exec'ing qemu. This is problematic because the uid and gid of the child are set by the security driver, but capabilities are dropped by virCommand - such separation doesn't work; the two operations must be done together or the capabilities do not transfer properly to the child process. This patch switches to using virSecurityManagerSetChildProcessLabel(), which is called prior to virCommandRun() (rather than being called *during* virCommandrun() by the hook function), and doesn't set the UID/GID/security label directly, but instead merely informs virCommand what it should set them all to when the time is appropriate. This lets virCommand choose to do the uid/gid and caps dropping all at the same time if it wants (it does *want* to, but isn't doing so yet; that's for an upcoming patch). --- Change from V1: rebased. src/qemu/qemu_process.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 12e3544..6c70246 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -1,7 +1,7 @@ /* * qemu_process.h: QEMU process management * - * Copyright (C) 2006-2012 Red Hat, Inc. + * Copyright (C) 2006-2013 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -2829,10 +2829,6 @@ static int qemuProcessHook(void *data) if (qemuProcessInitNumaMemoryPolicy(h->vm, h->nodemask) < 0) goto cleanup; - VIR_DEBUG("Setting up security labelling"); - if (virSecurityManagerSetProcessLabel(h->driver->securityManager, h->vm->def) < 0) - goto cleanup; - ret = 0; cleanup: @@ -3972,6 +3968,12 @@ int qemuProcessStart(virConnectPtr conn, virCommandSetPreExecHook(cmd, qemuProcessHook, &hookData); + VIR_DEBUG("Setting up security labelling"); + if (virSecurityManagerSetChildProcessLabel(driver->securityManager, + vm->def, cmd) < 0) { + goto cleanup; + } + virCommandSetOutputFD(cmd, &logfile); virCommandSetErrorFD(cmd, &logfile); virCommandNonblockingFDs(cmd); -- 1.8.1

Normally when a process' uid is changed to non-0, all the capabilities bits are cleared, even those explicitly set with calls to capng_update()/capng_apply() made immediately before setuid. And *after* the process' uid has been changed, it no longer has the necessary privileges to add capabilities back to the process. In order to set a non-0 uid while still maintaining any capabilities bits, it is necessary to either call capng_change_id() (which unfortunately doesn't currently call initgroups to setup auxiliary group membership), or to perform the small amount of calisthenics contained in the new utility function virSetUIDGIDWithCaps(). Another very important difference between the capabilities setting/clearing in virSetUIDGIDWithCaps() and virCommand's virSetCapabilities() (which it will replace in the next patch) is that the new function properly clears the capabilities bounding set, so it will not be possible for a child process to set any new capabilities. A short description of what is done by virSetUIDGIDWithCaps(): 1) clear all capabilities then set all those desired by the caller (in capBits) plus CAP_SETGID, CAP_SETUID, and CAP_SETPCAP (which is needed to change the capabilities bounding set). 2) call prctl(), telling it that we want to maintain current capabilities across an upcoming setuid(). 3) switch to the new uid/gid 4) again call prctl(), telling it we will no longer want capabilities maintained if this process does another setuid(). 5) clear the capabilities that we added to allow us to setuid/setgid/change the bounding set (unless they were also requested by the caller via the virCommand API). Because the modification/maintaining of capabilities is intermingled with setting the uid, this is necessarily done in a single function, rather than having two independent functions. Note that, due to the way that effective capabilities are computed (at time of execve) for a process that has uid != 0, the *file* capabilities of the binary being executed must also have the desired capabilities bit(s) set (see "man 7 capabilities"). This can be done with the "filecap" command. (e.g. "filecap /usr/bin/qemu-kvm sys_rawio"). --- Change from V1: * properly cast when comparing gid/uid, and only short circuit for -1 (not 0) * fix // style comments * add ATTRIBUTE_UNUSED where appropriate for capBits argument. src/libvirt_private.syms | 1 + src/util/virutil.c | 111 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 1 + 3 files changed, 113 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index dcdcb67..d8d5877 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1312,6 +1312,7 @@ virSetDeviceUnprivSGIO; virSetInherit; virSetNonBlock; virSetUIDGID; +virSetUIDGIDWithCaps; virSkipSpaces; virSkipSpacesAndBackslash; virSkipSpacesBackwards; diff --git a/src/util/virutil.c b/src/util/virutil.c index 0d7db00..28fcc2f 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -60,6 +60,7 @@ #endif #if WITH_CAPNG # include <cap-ng.h> +# include <sys/prctl.h> #endif #if defined HAVE_MNTENT_H && defined HAVE_GETMNTENT_R # include <mntent.h> @@ -2990,6 +2991,116 @@ virGetGroupName(gid_t gid ATTRIBUTE_UNUSED) } #endif /* HAVE_GETPWUID_R */ +#if WITH_CAPNG +/* Set the real and effective uid and gid to the given values, while + * maintaining the capabilities indicated by bits in @capBits. return + * 0 on success, -1 on failure (the original system error remains in + * errno). + */ +int +virSetUIDGIDWithCaps(uid_t uid, gid_t gid, unsigned long long capBits) +{ + 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. + */ + + capng_clear(CAPNG_SELECT_BOTH); + + for (ii = 0; ii <= CAP_LAST_CAP; ii++) { + if (capBits & (1ULL << ii)) { + capng_update(CAPNG_ADD, + CAPNG_EFFECTIVE|CAPNG_INHERITABLE| + CAPNG_PERMITTED|CAPNG_BOUNDING_SET, + ii); + } + } + + if (gid != (gid_t)-1 && + !capng_have_capability(CAPNG_EFFECTIVE, CAP_SETGID)) { + need_setgid = true; + capng_update(CAPNG_ADD, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_SETGID); + } + if (uid != (uid_t)-1 && + !capng_have_capability(CAPNG_EFFECTIVE, CAP_SETUID)) { + need_setuid = true; + capng_update(CAPNG_ADD, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_SETUID); + } +# ifdef PR_CAPBSET_DROP + /* If newer kernel, we need also need setpcap to change the bounding set */ + if ((capBits || need_setgid || need_setuid) && + !capng_have_capability(CAPNG_EFFECTIVE, CAP_SETPCAP)) { + need_setpcap = true; + } + if (need_setpcap) + capng_update(CAPNG_ADD, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_SETPCAP); +# endif + + need_prctl = capBits || need_setgid || need_setuid || need_setpcap; + + /* Tell system we want to keep caps across uid change */ + if (need_prctl && prctl(PR_SET_KEEPCAPS, 1, 0, 0, 0)) { + virReportSystemError(errno, "%s", + _("prctl failed to set KEEPCAPS")); + goto cleanup; + } + + /* Change to the temp capabilities */ + if ((capng_ret = capng_apply(CAPNG_SELECT_BOTH)) < 0) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot apply process capabilities %d"), capng_ret); + goto cleanup; + } + + if (virSetUIDGID(uid, gid) < 0) + goto cleanup; + + /* Tell it we are done keeping capabilities */ + if (need_prctl && prctl(PR_SET_KEEPCAPS, 0, 0, 0, 0)) { + virReportSystemError(errno, "%s", + _("prctl failed to reset KEEPCAPS")); + goto cleanup; + } + + /* Drop the caps that allow setuid/gid (unless they were requested) */ + if (need_setgid) + capng_update(CAPNG_DROP, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_SETGID); + if (need_setuid) + capng_update(CAPNG_DROP, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_SETUID); + /* Throw away CAP_SETPCAP so no more changes */ + if (need_setpcap) + capng_update(CAPNG_DROP, CAPNG_EFFECTIVE|CAPNG_PERMITTED, CAP_SETPCAP); + + if (need_prctl && ((capng_ret = capng_apply(CAPNG_SELECT_BOTH)) < 0)) { + virReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot apply process capabilities %d"), capng_ret); + ret = -1; + goto cleanup; + } + + ret = 0; +cleanup: + return ret; +} + +#else +/* + * On platforms without libcapng, the capabilities setting is treated + * as a NOP. + */ + +int +virSetUIDGIDWithCaps(uid_t uid, gid_t gid, + unsigned long long capBits ATTRIBUTE_UNUSED) +{ + return virSetUIDGID(uid, gid); +} +#endif + #if defined HAVE_MNTENT_H && defined HAVE_GETMNTENT_R /* search /proc/mounts for mount point of *type; return pointer to diff --git a/src/util/virutil.h b/src/util/virutil.h index 4201aa1..2dc3403 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -54,6 +54,7 @@ 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 virFileReadLimFD(int fd, int maxlen, char **buf) ATTRIBUTE_RETURN_CHECK; -- 1.8.1

On Tue, Feb 12, 2013 at 2:15 PM, Laine Stump <laine(a)laine.org&gt; wrote: The following error bisect's down to this commit when running out of my local checkout for testing. 2013-02-16 05:16:55.102+0000: 29992: error : virCommandWait:2270 : internal error Child process (LC_ALL=C LD_LIBRARY_PATH=/home/cardoe/work/libvirt/src/.libs PATH=/usr/local/bin:/usr/bin:/bin:/opt/bin:/usr/x86_64-pc-linux-gnu/gcc-bin/4.6.3:/usr/games/bin HOME=/home/cardoe USER=cardoe LOGNAME=cardoe /usr/bin/qemu-kvm -help) unexpected exit status 1: libvir: error : internal error cannot apply process capabilities -1 -- Doug Goldstein

On Sat, Feb 16, 2013 at 4:53 PM, Laine Stump <laine(a)laine.org&gt; wrote: Oops. Guess that would have been helpful to include. Its Gentoo btw, not Debian. Its in the first call. I guess the exit message is overriding the original line number in the error message. 2013-02-17 18:08:03.696+0000: 21164: debug : virExec:641 : Setting child uid:gid to 0:0 with caps 0 2013-02-17 18:08:03.696+0000: 21164: error : virSetUIDGIDWithCaps:3055 : internal error cannot apply process capabilities -1 -- Doug Goldstein