[libvirt] This patch fixes up a previous patch to work in containers
It also adds the ability to pass in privileged field into Security Manager so that writing to /run/setrans only attempted on privileged machines [PATCH] libvirt writes an mcs translation file to /run/setrans
From: Dan Walsh <dwalsh@redhat.com> mcstransd is a translation tool that can translate MCS Labels into human understandable code. I have patched it to watch for translation files in the /run/setrans directory. This allows us to run commands like ps -eZ and see system_u:system_r:svirt_t:Fedora18 rather then system_u:system_r:svirt_t:s0:c1,c2. When used with containers it would make an easy way to list all processes within a container using ps -eZ | grep Fedora18 Pass in privileged field into Security Manager so this is only attempted on privileged machines --- src/lxc/lxc_container.c | 8 +++--- src/lxc/lxc_controller.c | 2 +- src/lxc/lxc_driver.c | 3 ++- src/qemu/qemu_conf.c | 2 ++ src/qemu/qemu_conf.h | 1 + src/qemu/qemu_driver.c | 9 ++++--- src/security/security_manager.c | 29 +++++++++++++++------ src/security/security_manager.h | 7 +++-- src/security/security_selinux.c | 57 ++++++++++++++++++++++++++++++++++++++++- 9 files changed, 98 insertions(+), 20 deletions(-) diff --git a/src/lxc/lxc_container.c b/src/lxc/lxc_container.c index 48ccc09..cb6ae6a 100644 --- a/src/lxc/lxc_container.c +++ b/src/lxc/lxc_container.c @@ -1829,6 +1829,10 @@ static int lxcContainerSetupPivotRoot(virDomainDefPtr vmDef, if (lxcContainerPopulateDevices(ttyPaths, nttyPaths) < 0) goto cleanup; + VIR_DEBUG("Setting up security labeling"); + if (virSecurityManagerSetProcessLabel(securityDriver, vmDef) < 0) + goto cleanup; + /* Sets up any non-root mounts from guest config */ if (lxcContainerMountAllFS(vmDef, sec_mount_options) < 0) goto cleanup; @@ -2027,10 +2031,6 @@ static int lxcContainerChild(void *data) goto cleanup; } - VIR_DEBUG("Setting up security labeling"); - if (virSecurityManagerSetProcessLabel(argv->securityDriver, vmDef) < 0) - goto cleanup; - if (lxcContainerSetStdio(argv->monitor, ttyfd, argv->handshakefd) < 0) { goto cleanup; } diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 730236e..5a4c809 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -1732,7 +1732,7 @@ int main(int argc, char *argv[]) if (!(ctrl->securityManager = virSecurityManagerNew(securityDriver, LXC_DRIVER_NAME, - false, false, false))) + false, false, false, true))) goto cleanup; if (ctrl->def->seclabels) { diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 997f81d..a80cc9b 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1358,7 +1358,8 @@ lxcSecurityInit(virLXCDriverPtr driver) LXC_DRIVER_NAME, false, driver->securityDefaultConfined, - driver->securityRequireConfined); + driver->securityRequireConfined, + true); if (!mgr) goto error; diff --git a/src/qemu/qemu_conf.c b/src/qemu/qemu_conf.c index 098e49c..b92a27b 100644 --- a/src/qemu/qemu_conf.c +++ b/src/qemu/qemu_conf.c @@ -250,6 +250,7 @@ virQEMUDriverConfigPtr virQEMUDriverConfigNew(bool privileged) cfg->securityDefaultConfined = true; cfg->securityRequireConfined = false; + cfg->securityPrivileged = privileged; cfg->keepAliveInterval = 5; cfg->keepAliveCount = 5; @@ -399,6 +400,7 @@ int virQEMUDriverConfigLoadFile(virQEMUDriverConfigPtr cfg, GET_VALUE_BOOL("security_default_confined", cfg->securityDefaultConfined); GET_VALUE_BOOL("security_require_confined", cfg->securityRequireConfined); + GET_VALUE_BOOL("security_privileged", cfg->securityPrivileged); GET_VALUE_BOOL("spice_tls", cfg->spiceTLS); GET_VALUE_STR("spice_tls_x509_cert_dir", cfg->spiceTLSx509certdir); diff --git a/src/qemu/qemu_conf.h b/src/qemu/qemu_conf.h index df0791e..06824ae 100644 --- a/src/qemu/qemu_conf.h +++ b/src/qemu/qemu_conf.h @@ -138,6 +138,7 @@ struct _virQEMUDriverConfig { char **securityDriverNames; bool securityDefaultConfined; bool securityRequireConfined; + bool securityPrivileged; char *saveImageFormat; char *dumpImageFormat; diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 203cc6d..7a68e8a 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -322,7 +322,8 @@ qemuSecurityInit(virQEMUDriverPtr driver) QEMU_DRIVER_NAME, cfg->allowDiskFormatProbing, cfg->securityDefaultConfined, - cfg->securityRequireConfined))) + cfg->securityRequireConfined, + cfg->securityPrivileged))) goto error; if (!stack) { if (!(stack = virSecurityManagerNewStack(mgr))) @@ -339,7 +340,8 @@ qemuSecurityInit(virQEMUDriverPtr driver) QEMU_DRIVER_NAME, cfg->allowDiskFormatProbing, cfg->securityDefaultConfined, - cfg->securityRequireConfined))) + cfg->securityRequireConfined, + cfg->securityPrivileged))) goto error; if (!(stack = virSecurityManagerNewStack(mgr))) goto error; @@ -353,7 +355,8 @@ qemuSecurityInit(virQEMUDriverPtr driver) cfg->allowDiskFormatProbing, cfg->securityDefaultConfined, cfg->securityRequireConfined, - cfg->dynamicOwnership))) + cfg->dynamicOwnership, + cfg->securityPrivileged))) goto error; if (!stack) { if (!(stack = virSecurityManagerNewStack(mgr))) diff --git a/src/security/security_manager.c b/src/security/security_manager.c index f7c5c2e..12b4d55 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -41,6 +41,7 @@ struct _virSecurityManager { bool allowDiskFormatProbing; bool defaultConfined; bool requireConfined; + bool privileged; const char *virtDriver; void *privateData; }; @@ -66,7 +67,8 @@ static virSecurityManagerPtr virSecurityManagerNewDriver(virSecurityDriverPtr dr const char *virtDriver, bool allowDiskFormatProbing, bool defaultConfined, - bool requireConfined) + bool requireConfined, + bool privileged) { virSecurityManagerPtr mgr; char *privateData; @@ -75,10 +77,10 @@ static virSecurityManagerPtr virSecurityManagerNewDriver(virSecurityDriverPtr dr return NULL; VIR_DEBUG("drv=%p (%s) virtDriver=%s allowDiskFormatProbing=%d " - "defaultConfined=%d requireConfined=%d", + "defaultConfined=%d requireConfined=%d privileged=%d", drv, drv->name, virtDriver, allowDiskFormatProbing, defaultConfined, - requireConfined); + requireConfined, privileged); if (VIR_ALLOC_N(privateData, drv->privateDataLen) < 0) { virReportOOMError(); @@ -94,6 +96,7 @@ static virSecurityManagerPtr virSecurityManagerNewDriver(virSecurityDriverPtr dr mgr->allowDiskFormatProbing = allowDiskFormatProbing; mgr->defaultConfined = defaultConfined; mgr->requireConfined = requireConfined; + mgr->privileged = privileged; mgr->virtDriver = virtDriver; mgr->privateData = privateData; @@ -112,7 +115,8 @@ virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary) virSecurityManagerGetDriver(primary), virSecurityManagerGetAllowDiskFormatProbing(primary), virSecurityManagerGetDefaultConfined(primary), - virSecurityManagerGetRequireConfined(primary)); + virSecurityManagerGetRequireConfined(primary), + virSecurityManagerGetPrivileged(primary)); if (!mgr) return NULL; @@ -136,14 +140,16 @@ virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver, bool allowDiskFormatProbing, bool defaultConfined, bool requireConfined, - bool dynamicOwnership) + bool dynamicOwnership, + bool privileged) { virSecurityManagerPtr mgr = virSecurityManagerNewDriver(&virSecurityDriverDAC, virtDriver, allowDiskFormatProbing, defaultConfined, - requireConfined); + requireConfined, + privileged); if (!mgr) return NULL; @@ -159,7 +165,8 @@ virSecurityManagerPtr virSecurityManagerNew(const char *name, const char *virtDriver, bool allowDiskFormatProbing, bool defaultConfined, - bool requireConfined) + bool requireConfined, + bool privileged) { virSecurityDriverPtr drv = virSecurityDriverLookup(name, virtDriver); if (!drv) @@ -189,7 +196,8 @@ virSecurityManagerPtr virSecurityManagerNew(const char *name, virtDriver, allowDiskFormatProbing, defaultConfined, - requireConfined); + requireConfined, + privileged); } @@ -278,6 +286,11 @@ bool virSecurityManagerGetRequireConfined(virSecurityManagerPtr mgr) return mgr->requireConfined; } +bool virSecurityManagerGetPrivileged(virSecurityManagerPtr mgr) +{ + return mgr->privileged; +} + int virSecurityManagerRestoreImageLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, virDomainDiskDefPtr disk) diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 711b354..4d936a3 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -33,7 +33,8 @@ virSecurityManagerPtr virSecurityManagerNew(const char *name, const char *virtDriver, bool allowDiskFormatProbing, bool defaultConfined, - bool requireConfined); + bool requireConfined, + bool privileged); virSecurityManagerPtr virSecurityManagerNewStack(virSecurityManagerPtr primary); int virSecurityManagerStackAddNested(virSecurityManagerPtr stack, @@ -45,7 +46,8 @@ virSecurityManagerPtr virSecurityManagerNewDAC(const char *virtDriver, bool allowDiskFormatProbing, bool defaultConfined, bool requireConfined, - bool dynamicOwnership); + bool dynamicOwnership, + bool privileged); void virSecurityManagerPreFork(virSecurityManagerPtr mgr); void virSecurityManagerPostFork(virSecurityManagerPtr mgr); @@ -58,6 +60,7 @@ const char *virSecurityManagerGetModel(virSecurityManagerPtr mgr); bool virSecurityManagerGetAllowDiskFormatProbing(virSecurityManagerPtr mgr); bool virSecurityManagerGetDefaultConfined(virSecurityManagerPtr mgr); bool virSecurityManagerGetRequireConfined(virSecurityManagerPtr mgr); +bool virSecurityManagerGetPrivileged(virSecurityManagerPtr mgr); int virSecurityManagerRestoreImageLabel(virSecurityManagerPtr mgr, virDomainDefPtr def, diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 5d108b9..c416666 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -83,6 +83,57 @@ virSecuritySELinuxRestoreSecurityTPMFileLabelInt(virSecurityManagerPtr mgr, virDomainTPMDefPtr tpm); +static int +virSecuritySELinuxAddMCSFile(const char *name, + const char *label) +{ + int ret = -1; + char *tmp = NULL; + context_t con = NULL; + + if (virAsprintf(&tmp, "%s/%s", SELINUX_TRANS_DIR, name) < 0) { + virReportOOMError(); + return -1; + } + if (!(con = context_new(label))) { + virReportSystemError(errno, "%s", + _("unable to allocate security context")); + goto cleanup; + } + if (virFileWriteStr(tmp, context_range_get(con), S_IRUSR|S_IWUSR) < 0) { + virReportSystemError(errno, + _("unable to create MCS file %s"), tmp); + goto cleanup; + } + ret = 0; + +cleanup: + VIR_FREE(tmp); + context_free(con); + return ret; +} + +static int +virSecuritySELinuxRemoveMCSFile(const char *name) +{ + char *tmp = NULL; + int ret = -1; + if (virAsprintf(&tmp, "%s/%s", SELINUX_TRANS_DIR, name) < 0) { + virReportOOMError(); + return -1; + } + if (unlink(tmp) < 0 && errno != ENOENT) { + virReportSystemError(errno, + _("Unable to remove MCS file %s"), tmp); + goto cleanup; + } + ret = 0; + +cleanup: + VIR_FREE(tmp); + return ret; +} + /* * Returns 0 on success, 1 if already reserved, or -1 on fatal error */ @@ -1953,7 +2004,7 @@ virSecuritySELinuxReleaseSecurityLabel(virSecurityManagerPtr mgr, } VIR_FREE(secdef->imagelabel); - return 0; + return virSecuritySELinuxRemoveMCSFile(def->name); } @@ -2047,10 +2098,14 @@ virSecuritySELinuxSetSecurityProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UN return -1; } + if (virSecurityManagerGetPrivileged(mgr) && (virSecuritySELinuxAddMCSFile(def->name, secdef->label) < 0)) + return -1; + if (setexeccon_raw(secdef->label) == -1) { virReportSystemError(errno, _("unable to set security context '%s'"), secdef->label); + virSecuritySELinuxRemoveMCSFile(def->name); if (security_getenforce() == 1) return -1; } -- 1.8.2.1
On Tue, May 21, 2013 at 09:12:49AM -0400, dwalsh@redhat.com wrote:
From: Dan Walsh <dwalsh@redhat.com>
mcstransd is a translation tool that can translate MCS Labels into human understandable code. I have patched it to watch for translation files in the /run/setrans directory. This allows us to run commands like ps -eZ and see system_u:system_r:svirt_t:Fedora18 rather then system_u:system_r:svirt_t:s0:c1,c2. When used with containers it would make an easy way to list all processes within a container using ps -eZ | grep Fedora18
Pass in privileged field into Security Manager so this is only attempted on privileged machines
Did you actually test this patch, because it doesn't work at all ? An LXC guest fails to start: 2013-05-21 16:26:30.894+0000: 1: error : virSecuritySELinuxAddMCSFile:107 : unable to create MCS file /var/run/setrans/busy: No such file or directory If I create that directory inside the container, it at least starts, but doesn't have any effect because you're trying to write to /var/run directory inside the container, rather than in the host. With a QEMU guest this does nothing at all, because the QEMU driver uses virSecurityManagerSetChildProcessLabel instead of virSecurityManagerSetProcessLabel so this new code simply never runs. Trying todo this from the virSecurityManagerSetProcessLabel method is just wrong. As I said last time, virSecurityManagerGenProcessLabel is a better place IMHO.
diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index 5d108b9..c416666 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -83,6 +83,57 @@ virSecuritySELinuxRestoreSecurityTPMFileLabelInt(virSecurityManagerPtr mgr, virDomainTPMDefPtr tpm);
+static int +virSecuritySELinuxAddMCSFile(const char *name, + const char *label) +{ + int ret = -1; + char *tmp = NULL; + context_t con = NULL; + + if (virAsprintf(&tmp, "%s/%s", SELINUX_TRANS_DIR, name) < 0) {
SELINUX_TRANS_DIR doesn't appear to exist in any libselinux package prior to Fedora 19, so this breaks the build on all RHEL distros and Fedora < 18. This code needs to be made conditional on this constant existing in the headers.
+ virReportOOMError(); + return -1; + } + if (!(con = context_new(label))) { + virReportSystemError(errno, "%s", + _("unable to allocate security context")); + goto cleanup; + } + if (virFileWriteStr(tmp, context_range_get(con), S_IRUSR|S_IWUSR) < 0) { + virReportSystemError(errno, + _("unable to create MCS file %s"), tmp); + goto cleanup; + } + ret = 0; + +cleanup: + VIR_FREE(tmp); + context_free(con); + return ret; +} + +static int +virSecuritySELinuxRemoveMCSFile(const char *name) +{ + char *tmp = NULL; + int ret = -1; + if (virAsprintf(&tmp, "%s/%s", SELINUX_TRANS_DIR, name) < 0) { + virReportOOMError(); + return -1; + } + if (unlink(tmp) < 0 && errno != ENOENT) { + virReportSystemError(errno, + _("Unable to remove MCS file %s"), tmp); + goto cleanup; + } + ret = 0; + +cleanup: + VIR_FREE(tmp); + return ret; +} + /* * Returns 0 on success, 1 if already reserved, or -1 on fatal error */ @@ -1953,7 +2004,7 @@ virSecuritySELinuxReleaseSecurityLabel(virSecurityManagerPtr mgr, } VIR_FREE(secdef->imagelabel);
- return 0; + return virSecuritySELinuxRemoveMCSFile(def->name); }
@@ -2047,10 +2098,14 @@ virSecuritySELinuxSetSecurityProcessLabel(virSecurityManagerPtr mgr ATTRIBUTE_UN return -1; }
+ if (virSecurityManagerGetPrivileged(mgr) && (virSecuritySELinuxAddMCSFile(def->name, secdef->label) < 0)) + return -1;
As I said last time, failure to create the MCS file should not be treated as a fatal error IMHO. 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 :|
participants (2)
-
Daniel P. Berrange -
dwalsh＠redhat.com