[libvirt] [PATCH v2 0/4] security: Add check for invalid security models and duplicates
Erik Skultety (4): security: Refactor virSecurityManagerGenLabel security: introduce virSecurityManagerCheckAllLabel function conf: forbid seclabel duplicates for domain devices schema: allow multiple seclabel for devices in domaincommon.rng docs/schemas/domaincommon.rng | 16 +-- src/conf/domain_conf.c | 9 ++ src/libvirt_private.syms | 1 + src/qemu/qemu_process.c | 6 + src/security/security_manager.c | 146 ++++++++++++++++++--- src/security/security_manager.h | 2 + .../qemuxml2argv-seclabel-device-duplicates.xml | 33 +++++ .../qemuxml2argv-seclabel-device-multiple.xml | 32 +++++ tests/qemuxml2argvtest.c | 1 + tests/qemuxml2xmltest.c | 1 + 10 files changed, 220 insertions(+), 27 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-device-duplicates.xml create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-device-multiple.xml -- 1.9.3
Before we generate a security label (security driver with dynamic labeling) for a domain, we first check for domain's security model validity. We should also check devices' security model as well, therefore it might be better to move this chunk of code in a separate function which would check both the domain's security model and devices' security model. This function would of course be called right before we try to generate a security label in qemuProcessStart/qemuProcessAttach --- src/security/security_manager.c | 20 +------------------- 1 file changed, 1 insertion(+), 19 deletions(-) diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 302f54d..000bc82 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -576,33 +576,15 @@ virSecurityManagerGenLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm) { int ret = -1; - size_t i, j; + size_t i; virSecurityManagerPtr* sec_managers = NULL; virSecurityLabelDefPtr seclabel; bool generated = false; - if (mgr == NULL || mgr->drv == NULL) - return ret; - if ((sec_managers = virSecurityManagerGetNested(mgr)) == NULL) return ret; virObjectLock(mgr); - for (i = 0; i < vm->nseclabels; i++) { - if (!vm->seclabels[i]->model) - continue; - - for (j = 0; sec_managers[j]; j++) - if (STREQ(vm->seclabels[i]->model, sec_managers[j]->drv->name)) - break; - - if (!sec_managers[j]) { - virReportError(VIR_ERR_CONFIG_UNSUPPORTED, - _("Unable to find security driver for label %s"), - vm->seclabels[i]->model); - goto cleanup; - } - } for (i = 0; sec_managers[i]; i++) { generated = false; -- 1.9.3
On Tue, Feb 10, 2015 at 05:17:33PM +0100, Erik Skultety wrote:
Before we generate a security label (security driver with dynamic labeling) for a domain, we first check for domain's security model validity. We should also check devices' security model as well, therefore it might be better to move this chunk of code in a separate function which would check both the domain's security model and devices' security model.
The addition of this chunk should be a part of this commit. This way it seems it just disappeared.
This function would of course be called right before we try to generate a security label in qemuProcessStart/qemuProcessAttach --- src/security/security_manager.c | 20 +------------------- 1 file changed, 1 insertion(+), 19 deletions(-)
diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 302f54d..000bc82 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -576,33 +576,15 @@ virSecurityManagerGenLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm) { int ret = -1; - size_t i, j; + size_t i; virSecurityManagerPtr* sec_managers = NULL; virSecurityLabelDefPtr seclabel; bool generated = false;
- if (mgr == NULL || mgr->drv == NULL) - return ret; -
Can either of these conditions be true here? If so, we should leave the check here (possibly add an error message), because GetNested will dereference them. If not, they should be cleaned up in a separate patch. Jan
- if (mgr == NULL || mgr->drv == NULL) - return ret; -
Can either of these conditions be true here? If so, we should leave the check here (possibly add an error message), because GetNested will dereference them.
If not, they should be cleaned up in a separate patch.
Jan
Well, the thing is, this was the first function in qemuProcessStart that touched the seclabel structures as well as the security manager and no other function called after this one didn't really need a check like this, because GenLabel would have returned -1 in case of NULL pointer, thus jumping to cleanup. Now, as I introduced a new function that is called before GenLabel, I think it might be correct to move this check up to this CheckAllLabel function and the original meaning remains, i.e. in case of NULL pointer CheckAllLabel returns -1 (I'll fix the return code from 0 to -1 which is clearly wrong and this must be just a typo, but you get the point...) followed by qemuProcessStart jumping to cleanup. Erik
We do have a check for valid per-domain security model, however we still do permit an invalid security model for a domain's device (those which are specified with <source> element). This patch introduces a new function virSecurityManagerCheckAllLabel which compares user specified security model against currently registered security drivers. That being said, it also permits 'none' being specified as a device security model. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1165485 --- src/libvirt_private.syms | 1 + src/qemu/qemu_process.c | 6 ++ src/security/security_manager.c | 126 ++++++++++++++++++++++++++++++++++++++++ src/security/security_manager.h | 2 + 4 files changed, 135 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3b4ff41..1b1d891 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -953,6 +953,7 @@ virSecurityDriverLookup; # security/security_manager.h +virSecurityManagerCheckAllLabel; virSecurityManagerClearSocketLabel; virSecurityManagerGenLabel; virSecurityManagerGetBaseLabel; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index d5df60d..66ae779 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4428,6 +4428,10 @@ int qemuProcessStart(virConnectPtr conn, NULL) < 0) goto cleanup; + VIR_DEBUG("Checking domain and device security labels"); + if (virSecurityManagerCheckAllLabel(driver->securityManager, vm->def) < 0) + goto cleanup; + /* If you are using a SecurityDriver with dynamic labelling, then generate a security label for isolation */ VIR_DEBUG("Generating domain security label (if required)"); @@ -5424,6 +5428,8 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED, } } + if (virSecurityManagerCheckAllLabel(driver->securityManager, vm->def) < 0) + goto error; if (virSecurityManagerGenLabel(driver->securityManager, vm->def) < 0) goto error; diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 000bc82..32bc9fe 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -685,6 +685,132 @@ virSecurityManagerReleaseLabel(virSecurityManagerPtr mgr, } +static int +virSecurityManagerCheckSecurityDiskLabel(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDiskDefPtr disk, + void *opaque) +{ + size_t i, j; + virSecurityManagerPtr mgr = opaque; + virSecurityManagerPtr *sec_managers = NULL; + + if ((sec_managers = virSecurityManagerGetNested(mgr)) == NULL) + return 0; + + for (i = 0; i < disk->src->nseclabels; i++) { + if (STREQ_NULLABLE(disk->src->seclabels[i]->model, "none")) + continue; + + for (j = 0; sec_managers[j]; j++) { + if (STREQ_NULLABLE(disk->src->seclabels[i]->model, + sec_managers[j]->drv->name)) { + break; + } + } + if (!sec_managers[j]) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unable to find security driver for model %s"), + disk->src->seclabels[i]->model); + return -1; + } + } + + return 0; +} + + +static int +virSecurityManagerCheckSecurityChardevLabel(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainChrDefPtr chrdev, + void *opaque) +{ + size_t i, j; + virSecurityManagerPtr mgr = opaque; + virSecurityManagerPtr *sec_managers = NULL; + + if ((sec_managers = virSecurityManagerGetNested(mgr)) == NULL) + return 0; + + for (i = 0; i < chrdev->nseclabels; i++) { + if (STREQ_NULLABLE(chrdev->seclabels[i]->model, "none")) + continue; + + for (j = 0; sec_managers[j]; j++) { + if (STREQ_NULLABLE(chrdev->seclabels[i]->model, + sec_managers[j]->drv->name)) { + break; + } + } + if (!sec_managers[j]) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unable to find security driver for model %s"), + chrdev->seclabels[i]->model); + return -1; + } + } + + return 0; +} + + +static int +virSecurityManagerCheckSecurityChardevCallback(virDomainDefPtr def, + virDomainChrDefPtr dev, + void *opaque) +{ + return virSecurityManagerCheckSecurityChardevLabel(def, dev, opaque); +} + + +int virSecurityManagerCheckAllLabel(virSecurityManagerPtr mgr, + virDomainDefPtr vm) +{ + size_t i, j; + virSecurityManagerPtr *sec_managers = NULL; + + if (mgr == NULL || mgr->drv == NULL) + return 0; + + if ((sec_managers = virSecurityManagerGetNested(mgr)) == NULL) + return 0; + + /* first check per-domain seclabels */ + for (i = 0; i < vm->nseclabels; i++) { + if (STREQ_NULLABLE(vm->seclabels[i]->model, "none")) + continue; + + for (j = 0; sec_managers[j]; j++) { + if (STREQ_NULLABLE(vm->seclabels[i]->model, + sec_managers[j]->drv->name)) { + break; + } + } + if (!sec_managers[j]) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unable to find security driver for model %s"), + vm->seclabels[i]->model); + return -1; + } + } + + /* second check per-device seclabels */ + for (i = 0; i < vm->ndisks; i++) { + if (virSecurityManagerCheckSecurityDiskLabel(vm, + vm->disks[i], + mgr) < 0) + return -1; + } + + if (virDomainChrDefForeach(vm, + true, + virSecurityManagerCheckSecurityChardevCallback, + mgr) < 0) + return -1; + + return 0; +} + + int virSecurityManagerSetAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 156f882..13468db 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -111,6 +111,8 @@ int virSecurityManagerReserveLabel(virSecurityManagerPtr mgr, pid_t pid); int virSecurityManagerReleaseLabel(virSecurityManagerPtr mgr, virDomainDefPtr sec); +int virSecurityManagerCheckAllLabel(virSecurityManagerPtr mgr, + virDomainDefPtr sec); int virSecurityManagerSetAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr sec, const char *stdin_path); -- 1.9.3
On Tue, Feb 10, 2015 at 05:17:34PM +0100, Erik Skultety wrote:
We do have a check for valid per-domain security model, however we still do permit an invalid security model for a domain's device (those which are specified with <source> element). This patch introduces a new function virSecurityManagerCheckAllLabel which compares user specified security model against currently registered security drivers. That being said, it also permits 'none' being specified as a device security model.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1165485 --- src/libvirt_private.syms | 1 + src/qemu/qemu_process.c | 6 ++
missing src/lxc/lxc_process.c
src/security/security_manager.c | 126 ++++++++++++++++++++++++++++++++++++++++ src/security/security_manager.h | 2 + 4 files changed, 135 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 3b4ff41..1b1d891 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -953,6 +953,7 @@ virSecurityDriverLookup;
# security/security_manager.h +virSecurityManagerCheckAllLabel; virSecurityManagerClearSocketLabel; virSecurityManagerGenLabel; virSecurityManagerGetBaseLabel; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index d5df60d..66ae779 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4428,6 +4428,10 @@ int qemuProcessStart(virConnectPtr conn, NULL) < 0) goto cleanup;
+ VIR_DEBUG("Checking domain and device security labels"); + if (virSecurityManagerCheckAllLabel(driver->securityManager, vm->def) < 0) + goto cleanup; + /* If you are using a SecurityDriver with dynamic labelling, then generate a security label for isolation */ VIR_DEBUG("Generating domain security label (if required)"); @@ -5424,6 +5428,8 @@ int qemuProcessAttach(virConnectPtr conn ATTRIBUTE_UNUSED, } }
+ if (virSecurityManagerCheckAllLabel(driver->securityManager, vm->def) < 0) + goto error; if (virSecurityManagerGenLabel(driver->securityManager, vm->def) < 0) goto error;
diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 000bc82..32bc9fe 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -685,6 +685,132 @@ virSecurityManagerReleaseLabel(virSecurityManagerPtr mgr, }
+static int +virSecurityManagerCheckSecurityDiskLabel(virDomainDefPtr def ATTRIBUTE_UNUSED, + virDomainDiskDefPtr disk, + void *opaque) +{ + size_t i, j; + virSecurityManagerPtr mgr = opaque; + virSecurityManagerPtr *sec_managers = NULL; + + if ((sec_managers = virSecurityManagerGetNested(mgr)) == NULL) + return 0; + + for (i = 0; i < disk->src->nseclabels; i++) {
+ if (STREQ_NULLABLE(disk->src->seclabels[i]->model, "none")) + continue; + + for (j = 0; sec_managers[j]; j++) { + if (STREQ_NULLABLE(disk->src->seclabels[i]->model, + sec_managers[j]->drv->name)) { + break;
If you continue; here (or return success if it's in a separate function), the error message below can be unconditional.
+ } + } + if (!sec_managers[j]) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("Unable to find security driver for model %s"), + disk->src->seclabels[i]->model); + return -1; + }
This hunk of code is used three times, would be better as a separate function. Jan
Parser checks for per-domain seclabel duplicates, so it would be nice if it checked for per-device seclabel duplicates the same way Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1165485 --- src/conf/domain_conf.c | 9 ++++++ .../qemuxml2argv-seclabel-device-duplicates.xml | 33 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 43 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-device-duplicates.xml diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 6a57d80..7f242f1 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5421,6 +5421,15 @@ virSecurityDeviceLabelDefParseXML(virSecurityDeviceLabelDefPtr **seclabels_rtn, break; } } + + /* check for duplicate seclabels */ + for (j = 0; j < i; j++) { + if (STREQ_NULLABLE(model, seclabels[j]->model)) { + virReportError(VIR_ERR_XML_DETAIL, + _("seclabel for model %s is already provided"), model); + goto error; + } + } seclabels[i]->model = model; } diff --git a/tests/qemuxml2argvdata/qemuxml2argv-seclabel-device-duplicates.xml b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-device-duplicates.xml new file mode 100644 index 0000000..0ba26b6 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-device-duplicates.xml @@ -0,0 +1,33 @@ +<domain type='qemu' id='1'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'> + <seclabel model='selinux' relabel='no'/> + <seclabel model='selinux' relabel='no'/> + <seclabel model='selinux' relabel='no'/> + </source> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> + <seclabel type='none' relabel='no'/> + <seclabel type='dynamic' model='dac' relabel='yes'/> +</domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index f864c2a..395cab3 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -1302,6 +1302,7 @@ mymain(void) DO_TEST("seclabel-none", QEMU_CAPS_NAME); DO_TEST("seclabel-dac-none", QEMU_CAPS_NAME); DO_TEST_PARSE_ERROR("seclabel-multiple", QEMU_CAPS_NAME); + DO_TEST_PARSE_ERROR("seclabel-device-duplicates", QEMU_CAPS_NAME); DO_TEST("pseries-basic", QEMU_CAPS_CHARDEV, QEMU_CAPS_DEVICE, QEMU_CAPS_NODEFCONFIG); -- 1.9.3
On Tue, Feb 10, 2015 at 05:17:35PM +0100, Erik Skultety wrote:
Parser checks for per-domain seclabel duplicates, so it would be nice if it checked for per-device seclabel duplicates the same way
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1165485 --- src/conf/domain_conf.c | 9 ++++++ .../qemuxml2argv-seclabel-device-duplicates.xml | 33 ++++++++++++++++++++++ tests/qemuxml2argvtest.c | 1 + 3 files changed, 43 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-device-duplicates.xml
This fails domainschematest because multiple labels are only allowed in patch 4. I swapped them and pushed both. Jan
In our RNG schema we do allow multiple (different) seclabels per-domain, but don't allow this for devices, yet we neither have a check in our XML parser, nor in a post-parse callback. In that case we should allow multiple (different) seclabels for devices as well. --- docs/schemas/domaincommon.rng | 16 +++++------ .../qemuxml2argv-seclabel-device-multiple.xml | 32 ++++++++++++++++++++++ tests/qemuxml2xmltest.c | 1 + 3 files changed, 41 insertions(+), 8 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-device-multiple.xml diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index d467dce..b1f4eaa 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -1344,9 +1344,9 @@ <optional> <ref name="storageStartupPolicy"/> </optional> - <optional> + <zeroOrMore> <ref name='devSeclabel'/> - </optional> + </zeroOrMore> </element> </optional> </interleave> @@ -1367,9 +1367,9 @@ <optional> <ref name="storageStartupPolicy"/> </optional> - <optional> + <zeroOrMore> <ref name='devSeclabel'/> - </optional> + </zeroOrMore> </element> </optional> </interleave> @@ -1497,9 +1497,9 @@ <optional> <ref name="storageStartupPolicy"/> </optional> - <optional> + <zeroOrMore> <ref name='devSeclabel'/> - </optional> + </zeroOrMore> </element> </optional> </interleave> @@ -3195,9 +3195,9 @@ <optional> <attribute name="slave"/> </optional> - <optional> + <zeroOrMore> <ref name='devSeclabel'/> - </optional> + </zeroOrMore> </element> </zeroOrMore> <optional> diff --git a/tests/qemuxml2argvdata/qemuxml2argv-seclabel-device-multiple.xml b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-device-multiple.xml new file mode 100644 index 0000000..ce7f4f7 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-device-multiple.xml @@ -0,0 +1,32 @@ +<domain type='qemu'> + <name>QEMUGuest1</name> + <uuid>c7a5fdbd-edaf-9455-926a-d65c16db1809</uuid> + <memory unit='KiB'>219100</memory> + <currentMemory unit='KiB'>219100</currentMemory> + <vcpu placement='static' cpuset='1-4,8-20,525'>1</vcpu> + <os> + <type arch='i686' machine='pc'>hvm</type> + <boot dev='hd'/> + </os> + <clock offset='utc'/> + <on_poweroff>destroy</on_poweroff> + <on_reboot>restart</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/bin/qemu</emulator> + <disk type='block' device='disk'> + <source dev='/dev/HostVG/QEMUGuest1'> + <seclabel model='selinux' relabel='yes'> + <label>system_u:system_r:svirt_custom_t:s0:c192,c392</label> + </seclabel> + <seclabel model='dac' relabel='no'/> + </source> + <target dev='hda' bus='ide'/> + <address type='drive' controller='0' bus='0' target='0' unit='0'/> + </disk> + <controller type='usb' index='0'/> + <controller type='ide' index='0'/> + <controller type='pci' index='0' model='pci-root'/> + <memballoon model='virtio'/> + </devices> +</domain> diff --git a/tests/qemuxml2xmltest.c b/tests/qemuxml2xmltest.c index d3dfd9e..cc29083 100644 --- a/tests/qemuxml2xmltest.c +++ b/tests/qemuxml2xmltest.c @@ -337,6 +337,7 @@ mymain(void) DO_TEST_DIFFERENT("seclabel-none"); DO_TEST("seclabel-dac-none"); DO_TEST("seclabel-dynamic-none"); + DO_TEST("seclabel-device-multiple"); DO_TEST_FULL("seclabel-dynamic-none-relabel", true, WHEN_INACTIVE); DO_TEST("numad-static-vcpu-no-numatune"); DO_TEST("disk-scsi-lun-passthrough-sgio"); -- 1.9.3
participants (2)
-
Erik Skultety -
Ján Tomko