From 0c0e0d0263ac3ecdce6137f6615e06ff0d3f60ca Mon Sep 17 00:00:00 2001 From: "Daniel P. Berrange" Date: Mon, 11 Jan 2010 11:04:40 +0000 Subject: [PATCH] Refactor setup & cleanup of security labels in security driver The current security driver architecture has the following split of logic * domainGenSecurityLabel Allocate the unique label for the domain about to be started * domainGetSecurityLabel Retrieve the current live security label for a process * domainSetSecurityLabel Apply the previously allocated label to the current process Setup all disk image / device labelling * domainRestoreSecurityLabel Restore the original disk image / device labelling. Release the unique label for the domain The 'domainSetSecurityLabel' method is special because it runs in the context of the child process between the fork + exec. This is require in order to set the process label. It is not required in order to label disks/devices though. Having the disk labelling code run in the child process limits what it can do. In particularly libvirtd would like to remember the current disk image label, and only change shared image labels for the first VM to start. This requires use & update of global state in the libvirtd daemon, and thus cannot run in the child process context. The solution is to split domainSetSecurityLabel into two parts, one applies process label, and the other handles disk image labelling. At the same time domainRestoreSecurityLabel is similarly split, just so that it matches the style. Thus the previous 4 methods are replaced by the following 6 new methods * domainGenSecurityLabel Allocate the unique label for the domain about to be started No actual change here. * domainReleaseSecurityLabel Release the unique label for the domain * domainGetSecurityProcessLabel Retrieve the current live security label for a process Merely renamed for clarity. * domainSetSecurityProcessLabel Apply the previously allocated label to the current process * domainRestoreSecurityAllLabel Restore the original disk image / device labelling. * domainSetSecurityAllLabel Setup all disk image / device labelling The SELinux and AppArmour drivers are then updated to comply with this new spec. Notice that the AppArmour driver was actually a little different. It was creating its profile for the disk image and device labels in the 'domainGenSecurityLabel' method, where as the SELinux driver did it in 'domainSetSecurityLabel'. With the new method split, we can have consistency, with both drivers doing that in the domainSetSecurityAllLabel method. NB, the AppArmour changes here haven't been compiled so may not build. --- src/qemu/qemu_driver.c | 21 ++++++--- src/security/security_apparmor.c | 66 ++++++++++++++++++--------- src/security/security_driver.h | 30 ++++++++----- src/security/security_selinux.c | 76 ++++++++++++++++++++++---------- 4 files changed, 129 insertions(+), 64 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 15bda52fff..ad2737a5a2 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2450,8 +2450,14 @@ static int qemudDomainSetSecurityLabel(virConnectPtr conn, struct qemud_driver * int rc = 0; if (driver->securityDriver && - driver->securityDriver->domainSetSecurityLabel && - driver->securityDriver->domainSetSecurityLabel(conn, driver->securityDriver, vm) < 0) + driver->securityDriver->domainSetSecurityAllLabel && + driver->securityDriver->domainSetSecurityAllLabel(conn, vm) < 0) + rc = -1; + + if (rc == 0 && + driver->securityDriver && + driver->securityDriver->domainSetSecurityProcessLabel && + driver->securityDriver->domainSetSecurityProcessLabel(conn, driver->securityDriver, vm) < 0) rc = -1; return rc; @@ -3063,8 +3069,11 @@ static void qemudShutdownVMDaemon(virConnectPtr conn, /* Reset Security Labels */ if (driver->securityDriver && - driver->securityDriver->domainRestoreSecurityLabel) - driver->securityDriver->domainRestoreSecurityLabel(conn, vm); + driver->securityDriver->domainRestoreSecurityAllLabel) + driver->securityDriver->domainRestoreSecurityAllLabel(conn, vm); + if (driver->securityDriver && + driver->securityDriver->domainReleaseSecurityLabel) + driver->securityDriver->domainReleaseSecurityLabel(conn, vm); /* Clear out dynamically assigned labels */ if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_DYNAMIC) { @@ -4632,8 +4641,8 @@ static int qemudDomainGetSecurityLabel(virDomainPtr dom, virSecurityLabelPtr sec * QEMU monitor hasn't seen SIGHUP/ERR on poll(). */ if (virDomainObjIsActive(vm)) { - if (driver->securityDriver && driver->securityDriver->domainGetSecurityLabel) { - if (driver->securityDriver->domainGetSecurityLabel(dom->conn, vm, seclabel) == -1) { + if (driver->securityDriver && driver->securityDriver->domainGetSecurityProcessLabel) { + if (driver->securityDriver->domainGetSecurityProcessLabel(dom->conn, vm, seclabel) == -1) { qemudReportError(dom->conn, dom, NULL, VIR_ERR_INTERNAL_ERROR, "%s", _("Failed to get security label")); goto cleanup; diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 0f9ff95e6a..f2886455a9 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -341,16 +341,6 @@ AppArmorGenSecurityLabel(virConnectPtr conn, virDomainObjPtr vm) if ((profile_name = get_profile_name(conn, vm)) == NULL) return rc; - /* if the profile is not already loaded, then load one */ - if (profile_loaded(profile_name) < 0) { - if (load_profile(conn, profile_name, vm, NULL) < 0) { - virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR, - _("cannot generate AppArmor profile " - "\'%s\'"), profile_name); - goto clean; - } - } - vm->def->seclabel.label = strndup(profile_name, strlen(profile_name)); if (!vm->def->seclabel.label) { virReportOOMError(NULL); @@ -375,7 +365,6 @@ AppArmorGenSecurityLabel(virConnectPtr conn, virDomainObjPtr vm) goto clean; err: - remove_profile(profile_name); VIR_FREE(vm->def->seclabel.label); VIR_FREE(vm->def->seclabel.imagelabel); VIR_FREE(vm->def->seclabel.model); @@ -386,12 +375,33 @@ AppArmorGenSecurityLabel(virConnectPtr conn, virDomainObjPtr vm) return rc; } +static int +AppArmorSetSecurityAllLabel(virConnectPtr conn, virDomainObjPtr vm) +{ + int rc = -1; + + if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC) + return 0; + + /* if the profile is not already loaded, then load one */ + if (profile_loaded(vm->def->seclabel.label) < 0) { + if (load_profile(conn, vm->def->seclabel.label, vm, NULL) < 0) { + virSecurityReportError(conn, VIR_ERR_INTERNAL_ERROR, + _("cannot generate AppArmor profile " + "\'%s\'"), vm->def->seclabel.label); + return -1; + } + } + + return 0; +} + /* Seen with 'virsh dominfo '. This function only called if the VM is * running. */ static int -AppArmorGetSecurityLabel(virConnectPtr conn, - virDomainObjPtr vm, virSecurityLabelPtr sec) +AppArmorGetSecurityProcessLabel(virConnectPtr conn, + virDomainObjPtr vm, virSecurityLabelPtr sec) { int rc = -1; char *profile_name = NULL; @@ -423,7 +433,20 @@ AppArmorGetSecurityLabel(virConnectPtr conn, * more details. Currently called via qemudShutdownVMDaemon. */ static int -AppArmorRestoreSecurityLabel(virConnectPtr conn, virDomainObjPtr vm) +AppArmorReleaseSecurityLabel(virConnectPtr conn, virDomainObjPtr vm) +{ + const virSecurityLabelDefPtr secdef = &vm->def->seclabel; + + VIR_FREE(secdef->model); + VIR_FREE(secdef->label); + VIR_FREE(secdef->imagelabel); + + return 0; +} + + +static int +AppArmorRestoreSecurityAllLabel(virConnectPtr conn, virDomainObjPtr vm) { const virSecurityLabelDefPtr secdef = &vm->def->seclabel; int rc = 0; @@ -434,9 +457,6 @@ AppArmorRestoreSecurityLabel(virConnectPtr conn, virDomainObjPtr vm) _("could not remove profile for \'%s\'"), secdef->label); } - VIR_FREE(secdef->model); - VIR_FREE(secdef->label); - VIR_FREE(secdef->imagelabel); } return rc; } @@ -445,8 +465,8 @@ AppArmorRestoreSecurityLabel(virConnectPtr conn, virDomainObjPtr vm) * LOCAL_STATE_DIR/log/libvirt/qemu/.log */ static int -AppArmorSetSecurityLabel(virConnectPtr conn, - virSecurityDriverPtr drv, virDomainObjPtr vm) +AppArmorSetSecurityProcessLabel(virConnectPtr conn, + virSecurityDriverPtr drv, virDomainObjPtr vm) { const virSecurityLabelDefPtr secdef = &vm->def->seclabel; int rc = -1; @@ -620,9 +640,11 @@ virSecurityDriver virAppArmorSecurityDriver = { .domainRestoreSecurityImageLabel = AppArmorRestoreSecurityImageLabel, .domainGenSecurityLabel = AppArmorGenSecurityLabel, .domainReserveSecurityLabel = AppArmorReserveSecurityLabel, - .domainGetSecurityLabel = AppArmorGetSecurityLabel, - .domainRestoreSecurityLabel = AppArmorRestoreSecurityLabel, - .domainSetSecurityLabel = AppArmorSetSecurityLabel, + .domainReleaseSecurityLabel = AppArmorReleaseSecurityLabel, + .domainGetSecurityProcessLabel = AppArmorGetSecurityProcessLabel, + .domainSetSecurityProcessLabel = AppArmorSetSecurityProcessLabel, + .domainRestoreSecurityAllLabel = AppArmorRestoreSecurityAllLabel, + .domainSetSecurityAllLabel = AppArmorSetSecurityAllLabel, .domainSetSecurityHostdevLabel = AppArmorSetSecurityHostdevLabel, .domainRestoreSecurityHostdevLabel = AppArmorRestoreSecurityHostdevLabel, }; diff --git a/src/security/security_driver.h b/src/security/security_driver.h index 97c900202c..5d2446d246 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -52,15 +52,19 @@ typedef int (*virSecurityDomainRestoreSavedStateLabel) (virConnectPtr conn, typedef int (*virSecurityDomainGenLabel) (virConnectPtr conn, virDomainObjPtr sec); typedef int (*virSecurityDomainReserveLabel) (virConnectPtr conn, - virDomainObjPtr sec); -typedef int (*virSecurityDomainGetLabel) (virConnectPtr conn, - virDomainObjPtr vm, - virSecurityLabelPtr sec); -typedef int (*virSecurityDomainRestoreLabel) (virConnectPtr conn, - virDomainObjPtr vm); -typedef int (*virSecurityDomainSetLabel) (virConnectPtr conn, - virSecurityDriverPtr drv, - virDomainObjPtr vm); + virDomainObjPtr sec); +typedef int (*virSecurityDomainReleaseLabel) (virConnectPtr conn, + virDomainObjPtr sec); +typedef int (*virSecurityDomainSetAllLabel) (virConnectPtr conn, + virDomainObjPtr sec); +typedef int (*virSecurityDomainRestoreAllLabel) (virConnectPtr conn, + virDomainObjPtr vm); +typedef int (*virSecurityDomainGetProcessLabel) (virConnectPtr conn, + virDomainObjPtr vm, + virSecurityLabelPtr sec); +typedef int (*virSecurityDomainSetProcessLabel) (virConnectPtr conn, + virSecurityDriverPtr drv, + virDomainObjPtr vm); typedef int (*virSecurityDomainSecurityVerify) (virConnectPtr conn, virDomainDefPtr def); @@ -73,9 +77,11 @@ struct _virSecurityDriver { virSecurityDomainSetImageLabel domainSetSecurityImageLabel; virSecurityDomainGenLabel domainGenSecurityLabel; virSecurityDomainReserveLabel domainReserveSecurityLabel; - virSecurityDomainGetLabel domainGetSecurityLabel; - virSecurityDomainSetLabel domainSetSecurityLabel; - virSecurityDomainRestoreLabel domainRestoreSecurityLabel; + virSecurityDomainReleaseLabel domainReleaseSecurityLabel; + virSecurityDomainGetProcessLabel domainGetSecurityProcessLabel; + virSecurityDomainSetProcessLabel domainSetSecurityProcessLabel; + virSecurityDomainSetAllLabel domainSetSecurityAllLabel; + virSecurityDomainRestoreAllLabel domainRestoreSecurityAllLabel; virSecurityDomainRestoreHostdevLabel domainRestoreSecurityHostdevLabel; virSecurityDomainSetHostdevLabel domainSetSecurityHostdevLabel; virSecurityDomainSetSavedStateLabel domainSetSavedStateLabel; diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index c48f4c8af5..9f1a644695 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -277,9 +277,9 @@ SELinuxSecurityDriverOpen(virConnectPtr conn, virSecurityDriverPtr drv) } static int -SELinuxGetSecurityLabel(virConnectPtr conn, - virDomainObjPtr vm, - virSecurityLabelPtr sec) +SELinuxGetSecurityProcessLabel(virConnectPtr conn, + virDomainObjPtr vm, + virSecurityLabelPtr sec) { security_context_t ctx; @@ -615,8 +615,8 @@ done: } static int -SELinuxRestoreSecurityLabel(virConnectPtr conn, - virDomainObjPtr vm) +SELinuxRestoreSecurityAllLabel(virConnectPtr conn, + virDomainObjPtr vm) { const virSecurityLabelDefPtr secdef = &vm->def->seclabel; int i; @@ -636,6 +636,19 @@ SELinuxRestoreSecurityLabel(virConnectPtr conn, vm->def->disks[i]) < 0) rc = -1; } + + return rc; +} + +static int +SELinuxReleaseSecurityLabel(virConnectPtr conn ATTRIBUTE_UNUSED, + virDomainObjPtr vm) +{ + const virSecurityLabelDefPtr secdef = &vm->def->seclabel; + + if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC) + return 0; + context_t con = context_new(secdef->label); if (con) { mcsRemove(context_range_get(con)); @@ -646,7 +659,7 @@ SELinuxRestoreSecurityLabel(virConnectPtr conn, VIR_FREE(secdef->label); VIR_FREE(secdef->imagelabel); - return rc; + return 0; } @@ -693,13 +706,12 @@ SELinuxSecurityVerify(virConnectPtr conn, virDomainDefPtr def) } static int -SELinuxSetSecurityLabel(virConnectPtr conn, - virSecurityDriverPtr drv, - virDomainObjPtr vm) +SELinuxSetSecurityProcessLabel(virConnectPtr conn, + virSecurityDriverPtr drv, + virDomainObjPtr vm) { /* TODO: verify DOI */ const virSecurityLabelDefPtr secdef = &vm->def->seclabel; - int i; if (vm->def->seclabel.label == NULL) return 0; @@ -722,18 +734,32 @@ SELinuxSetSecurityLabel(virConnectPtr conn, return -1; } - if (secdef->type == VIR_DOMAIN_SECLABEL_DYNAMIC) { - for (i = 0 ; i < vm->def->ndisks ; i++) { - /* XXX fixme - we need to recursively label the entriy tree :-( */ - if (vm->def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_DIR) - continue; - if (SELinuxSetSecurityImageLabel(conn, vm, vm->def->disks[i]) < 0) - return -1; - } - for (i = 0 ; i < vm->def->nhostdevs ; i++) { - if (SELinuxSetSecurityHostdevLabel(conn, vm, vm->def->hostdevs[i]) < 0) - return -1; + return 0; +} + +static int +SELinuxSetSecurityAllLabel(virConnectPtr conn, + virDomainObjPtr vm) +{ + const virSecurityLabelDefPtr secdef = &vm->def->seclabel; + int i; + + if (secdef->type == VIR_DOMAIN_SECLABEL_STATIC) + return 0; + + for (i = 0 ; i < vm->def->ndisks ; i++) { + /* XXX fixme - we need to recursively label the entire tree :-( */ + if (vm->def->disks[i]->type == VIR_DOMAIN_DISK_TYPE_DIR) { + VIR_WARN("Unable to relabel directory tree %s for disk %s", + vm->def->disks[i]->src, vm->def->disks[i]->dst); + continue; } + if (SELinuxSetSecurityImageLabel(conn, vm, vm->def->disks[i]) < 0) + return -1; + } + for (i = 0 ; i < vm->def->nhostdevs ; i++) { + if (SELinuxSetSecurityHostdevLabel(conn, vm, vm->def->hostdevs[i]) < 0) + return -1; } return 0; @@ -748,9 +774,11 @@ virSecurityDriver virSELinuxSecurityDriver = { .domainRestoreSecurityImageLabel = SELinuxRestoreSecurityImageLabel, .domainGenSecurityLabel = SELinuxGenSecurityLabel, .domainReserveSecurityLabel = SELinuxReserveSecurityLabel, - .domainGetSecurityLabel = SELinuxGetSecurityLabel, - .domainRestoreSecurityLabel = SELinuxRestoreSecurityLabel, - .domainSetSecurityLabel = SELinuxSetSecurityLabel, + .domainReleaseSecurityLabel = SELinuxReleaseSecurityLabel, + .domainGetSecurityProcessLabel = SELinuxGetSecurityProcessLabel, + .domainSetSecurityProcessLabel = SELinuxSetSecurityProcessLabel, + .domainRestoreSecurityAllLabel = SELinuxRestoreSecurityAllLabel, + .domainSetSecurityAllLabel = SELinuxSetSecurityAllLabel, .domainSetSecurityHostdevLabel = SELinuxSetSecurityHostdevLabel, .domainRestoreSecurityHostdevLabel = SELinuxRestoreSecurityHostdevLabel, .domainSetSavedStateLabel = SELinuxSetSavedStateLabel, -- GitLab