[libvirt] [PATCH 0/6] security: Add check for invalid security models and duplicates
Add a check for invalid security models in per-device configuration as well as a check for seclabel duplicates in per-device configuration Erik Skultety (6): security: Add check for valid security model conf: fix a minor typo conf: forbid seclabel duplicates for domain devices schema: allow multiple seclabel for devices in domaincommon.rng test: add test to check for device seclabel duplicates po: add security_stack.c into POTFILES.in docs/schemas/domaincommon.rng | 16 ++++---- po/POTFILES.in | 1 + src/conf/domain_conf.c | 11 ++++- src/security/security_manager.c | 5 +++ src/security/security_manager.h | 1 + src/security/security_stack.c | 48 +++++++++++++++++++++- .../qemuxml2argv-seclabel-device-multiple.xml | 35 ++++++++++++++++ 7 files changed, 106 insertions(+), 11 deletions(-) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-device-multiple.xml -- 1.9.3
We do have a check for valid per-domain security model, however we still do permit an invalid security model for a <disk> type device. This patch introduces a new function virSecurityStackCheckDiskLabels 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/security/security_manager.c | 5 +++++ src/security/security_manager.h | 1 + src/security/security_stack.c | 48 +++++++++++++++++++++++++++++++++++++++-- 3 files changed, 52 insertions(+), 2 deletions(-) diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 302f54d..8eacf0c 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -262,6 +262,11 @@ virSecurityManagerGetDriver(virSecurityManagerPtr mgr) return mgr->virtDriver; } +const char * +virSecurityManagerGetDriverName(virSecurityManagerPtr mgr) +{ + return mgr->drv->name; +} const char * virSecurityManagerGetDOI(virSecurityManagerPtr mgr) diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 156f882..4626c4b 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -70,6 +70,7 @@ void virSecurityManagerPostFork(virSecurityManagerPtr mgr); void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr); const char *virSecurityManagerGetDriver(virSecurityManagerPtr mgr); +const char *virSecurityManagerGetDriverName(virSecurityManagerPtr mgr); const char *virSecurityManagerGetDOI(virSecurityManagerPtr mgr); const char *virSecurityManagerGetModel(virSecurityManagerPtr mgr); const char *virSecurityManagerGetBaseLabel(virSecurityManagerPtr mgr, int virtType); diff --git a/src/security/security_stack.c b/src/security/security_stack.c index 1ded57b..75d8b96 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -301,7 +301,46 @@ virSecurityStackRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr, return rc; } +static int +virSecurityStackCheckSecurityDiskLabels(virSecurityManagerPtr mgr, + virDomainDefPtr vm) +{ + virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityStackItemPtr item = priv->itemsHead; + size_t i, j; + bool sec_model_valid = false; + + for (i = 0; i < vm->ndisks; i++) { + virStorageSourcePtr src = vm->disks[i]->src; + for (j = 0; j < src->nseclabels; j++) { + const char *sec_model = src->seclabels[j]->model; + + if (STREQ_NULLABLE(sec_model, "none")) { + sec_model_valid = true; + continue; + } + + sec_model_valid = false; + for (; item; item = item->next) { + const char *drv_name = virSecurityManagerGetDriverName(mgr); + + if (STREQ_NULLABLE(sec_model, drv_name)) { + sec_model_valid = true; + break; + } + } + if (!sec_model_valid) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("No security driver matches security model " + "'%s'"), + sec_model); + return -1; + } + } + } + return 0; +} static int virSecurityStackSetSecurityAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, @@ -309,13 +348,18 @@ virSecurityStackSetSecurityAllLabel(virSecurityManagerPtr mgr, { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; - int rc = 0; + int rc = -1; + + if (virSecurityStackCheckSecurityDiskLabels(mgr, vm) < 0) + goto error; for (; item; item = item->next) { if (virSecurityManagerSetAllLabel(item->securityManager, vm, stdin_path) < 0) - rc = -1; + goto error; } + rc = 0; + error: return rc; } -- 1.9.3
On Fri, Feb 06, 2015 at 07:13:23PM +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 <disk> type device. This patch introduces a new function virSecurityStackCheckDiskLabels 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/security/security_manager.c | 5 +++++ src/security/security_manager.h | 1 + src/security/security_stack.c | 48 +++++++++++++++++++++++++++++++++++++++-- 3 files changed, 52 insertions(+), 2 deletions(-)
diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 302f54d..8eacf0c 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -262,6 +262,11 @@ virSecurityManagerGetDriver(virSecurityManagerPtr mgr) return mgr->virtDriver; }
+const char * +virSecurityManagerGetDriverName(virSecurityManagerPtr mgr) +{ + return mgr->drv->name; +}
const char * virSecurityManagerGetDOI(virSecurityManagerPtr mgr) diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 156f882..4626c4b 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -70,6 +70,7 @@ void virSecurityManagerPostFork(virSecurityManagerPtr mgr); void *virSecurityManagerGetPrivateData(virSecurityManagerPtr mgr);
const char *virSecurityManagerGetDriver(virSecurityManagerPtr mgr); +const char *virSecurityManagerGetDriverName(virSecurityManagerPtr mgr); const char *virSecurityManagerGetDOI(virSecurityManagerPtr mgr); const char *virSecurityManagerGetModel(virSecurityManagerPtr mgr); const char *virSecurityManagerGetBaseLabel(virSecurityManagerPtr mgr, int virtType); diff --git a/src/security/security_stack.c b/src/security/security_stack.c index 1ded57b..75d8b96 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -301,7 +301,46 @@ virSecurityStackRestoreSecurityHostdevLabel(virSecurityManagerPtr mgr, return rc; }
+static int +virSecurityStackCheckSecurityDiskLabels(virSecurityManagerPtr mgr, + virDomainDefPtr vm) +{ + virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); + virSecurityStackItemPtr item = priv->itemsHead; + size_t i, j; + bool sec_model_valid = false;
The function would be nicer without this variable, assuming success by default and returning an error right away if we did not find a driver.
+ + for (i = 0; i < vm->ndisks; i++) {
Character devices can have seclabels too, see virDomainChrSourceDefFormat.
+ virStorageSourcePtr src = vm->disks[i]->src; + for (j = 0; j < src->nseclabels; j++) { + const char *sec_model = src->seclabels[j]->model; + + if (STREQ_NULLABLE(sec_model, "none")) { + sec_model_valid = true;
This value is never read again.
+ continue; + } + + sec_model_valid = false; + for (; item; item = item->next) { + const char *drv_name = virSecurityManagerGetDriverName(mgr); + + if (STREQ_NULLABLE(sec_model, drv_name)) { + sec_model_valid = true; + break; + } + }
virSecurityDriverLookup can be used here, after we've special-cased the "none" driver.
+ if (!sec_model_valid) { + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, + _("No security driver matches security model " + "'%s'"), + sec_model); + return -1; + } + } + }
+ return 0; +} static int virSecurityStackSetSecurityAllLabel(virSecurityManagerPtr mgr, virDomainDefPtr vm, @@ -309,13 +348,18 @@ virSecurityStackSetSecurityAllLabel(virSecurityManagerPtr mgr, { virSecurityStackDataPtr priv = virSecurityManagerGetPrivateData(mgr); virSecurityStackItemPtr item = priv->itemsHead; - int rc = 0; + int rc = -1; + + if (virSecurityStackCheckSecurityDiskLabels(mgr, vm) < 0) + goto error;
Putting this check into the SecurityManager would make it work for drivers not using the stack (LXC). In QEMU driver, this function (SetSecurityAllLabel) is called after the fork, but we can do the check much earlier - maybe in virSecurityManagerGenLabel where we already check the per-domain labels. Jan
--- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4251b13..a36dace 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5213,7 +5213,7 @@ virSecurityLabelDefsParseXML(virDomainDefPtr def, for (j = 0; j < i; j++) { if (STREQ_NULLABLE(seclabel->model, def->seclabels[j]->model)) { virReportError(VIR_ERR_XML_DETAIL, - _("seclablel for model %s is already provided"), + _("seclabel for model %s is already provided"), seclabel->model); virSecurityLabelDefFree(seclabel); goto error; -- 1.9.3
On Fri, Feb 06, 2015 at 07:13:24PM +0100, Erik Skultety wrote:
--- src/conf/domain_conf.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
ACK and pushed. Jan
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 4251b13..a36dace 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5213,7 +5213,7 @@ virSecurityLabelDefsParseXML(virDomainDefPtr def, for (j = 0; j < i; j++) { if (STREQ_NULLABLE(seclabel->model, def->seclabels[j]->model)) { virReportError(VIR_ERR_XML_DETAIL, - _("seclablel for model %s is already provided"), + _("seclabel for model %s is already provided"), seclabel->model); virSecurityLabelDefFree(seclabel); goto error; -- 1.9.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index a36dace..c43aded 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -5323,6 +5323,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; } -- 1.9.3
In our RNG schema we do allow multiple seclabels per-domain, bu don't allow this for devices, yet we neither have a check in our XML parser, nor in a post-parse callback. As one of my previous patches in this series added that check to the XML parser, let's allow multiple seclabels for devices then --- docs/schemas/domaincommon.rng | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) 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> -- 1.9.3
On Fri, Feb 06, 2015 at 07:13:26PM +0100, Erik Skultety wrote:
In our RNG schema we do allow multiple seclabels per-domain, bu don't allow this for devices, yet we neither have a check in our XML parser, nor in a post-parse callback. As one of my previous patches in this series added that check to the XML parser, let's allow multiple seclabels for devices then --- docs/schemas/domaincommon.rng | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
A test case for qemuxml2xmltest (which will also automatically be tested by domainschematest) testing multiple labels would be nice. Jan
--- .../qemuxml2argv-seclabel-device-multiple.xml | 35 ++++++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-device-multiple.xml diff --git a/tests/qemuxml2argvdata/qemuxml2argv-seclabel-device-multiple.xml b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-device-multiple.xml new file mode 100644 index 0000000..0ee6768 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-device-multiple.xml @@ -0,0 +1,35 @@ +<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='yes'> + <label>system_u:system_r:svirt_custom_t:s0:c192,c392</label> + </seclabel> + <seclabel model='selinux' relabel='yes'> + <label>system_u:system_r:svirt_custom_t:s0:c194,c398</label> + </seclabel> + </source> + <backingStore/> + <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> -- 1.9.3
This is only tested by domainschematest. It should be added to either qemuxml2xmltest or qemuxml2argvtest and squashed in together with the commit adding this check. Jan On Fri, Feb 06, 2015 at 07:13:27PM +0100, Erik Skultety wrote:
--- .../qemuxml2argv-seclabel-device-multiple.xml | 35 ++++++++++++++++++++++ 1 file changed, 35 insertions(+) create mode 100644 tests/qemuxml2argvdata/qemuxml2argv-seclabel-device-multiple.xml
diff --git a/tests/qemuxml2argvdata/qemuxml2argv-seclabel-device-multiple.xml b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-device-multiple.xml new file mode 100644 index 0000000..0ee6768 --- /dev/null +++ b/tests/qemuxml2argvdata/qemuxml2argv-seclabel-device-multiple.xml @@ -0,0 +1,35 @@ +<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='yes'> + <label>system_u:system_r:svirt_custom_t:s0:c192,c392</label> + </seclabel> + <seclabel model='selinux' relabel='yes'> + <label>system_u:system_r:svirt_custom_t:s0:c194,c398</label> + </seclabel> + </source> + <backingStore/> + <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> -- 1.9.3
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Previous patches of these series introduced a new check which might endup reporting an error. In that case POTFILES.in had to be updated, so that syntax-check could pass successfully --- po/POTFILES.in | 1 + 1 file changed, 1 insertion(+) diff --git a/po/POTFILES.in b/po/POTFILES.in index 3064037..339009c 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -147,6 +147,7 @@ src/security/security_dac.c src/security/security_driver.c src/security/security_manager.c src/security/security_selinux.c +src/security/security_stack.c src/security/virt-aa-helper.c src/storage/parthelper.c src/storage/storage_backend.c -- 1.9.3
On 02/06/2015 11:13 AM, Erik Skultety wrote:
Add a check for invalid security models in per-device configuration as well as a check for seclabel duplicates in per-device configuration
Erik Skultety (6): security: Add check for valid security model conf: fix a minor typo conf: forbid seclabel duplicates for domain devices schema: allow multiple seclabel for devices in domaincommon.rng test: add test to check for device seclabel duplicates po: add security_stack.c into POTFILES.in
docs/schemas/domaincommon.rng | 16 ++++---- po/POTFILES.in | 1 +
Patch 6 should be squashed into the patch that triggered the syntax-check failure condition, not standalone. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 02/06/2015 08:04 PM, Eric Blake wrote:
On 02/06/2015 11:13 AM, Erik Skultety wrote:
Add a check for invalid security models in per-device configuration as well as a check for seclabel duplicates in per-device configuration
Erik Skultety (6): security: Add check for valid security model conf: fix a minor typo conf: forbid seclabel duplicates for domain devices schema: allow multiple seclabel for devices in domaincommon.rng test: add test to check for device seclabel duplicates po: add security_stack.c into POTFILES.in
docs/schemas/domaincommon.rng | 16 ++++---- po/POTFILES.in | 1 +
Patch 6 should be squashed into the patch that triggered the syntax-check failure condition, not standalone.
Yeah, I realized that the second I sent the patches. I was thinking maybe reordering, but now that you said that, sure, squashing is a better idea. I'll wait for some reviews first and then I'll fix this as well, thank you Eric. Erik
participants (3)
-
Eric Blake -
Erik Skultety -
Ján Tomko