[PATCH 0/3] security: Don't error out on seclabels of type='none'
*** BLURB HERE *** Michal Prívozník (3): conf: Fix seclabel type parsing wrt default value security: Rewrite virSecurityManagerCheckModel() to use g_autofree security: Don't error out on seclabels of type='none' src/conf/domain_conf.c | 13 +++++------ src/security/security_manager.c | 41 ++++++++++++++++++++++++--------- 2 files changed, 36 insertions(+), 18 deletions(-) -- 2.52.0
From: Michal Privoznik <mprivozn@redhat.com> Prior to v7.10.0-rc1~26 seclabels defaulted to VIR_DOMAIN_SECLABEL_DYNAMIC (type='dynamic'). But after switching the parser to virXMLPropEnum() the type is overwritten to VIR_DOMAIN_SECLABEL_DEFAULT because the first thing that the helper function does is to set variable that holds the result to zero. Switch to virXMLPropEnumDefault() to restore the previous behavior. Fixes: f7ff8556ad9ba8d81408e31649071941a6a849a3 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 84425ff39d..b1ee519ff6 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7076,14 +7076,13 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, if (!(seclabel = virSecurityLabelDefNew(model))) return NULL; - /* set default value */ - seclabel->type = VIR_DOMAIN_SECLABEL_DYNAMIC; - - if (virXMLPropEnum(ctxt->node, "type", - virDomainSeclabelTypeFromString, - VIR_XML_PROP_NONZERO, - &seclabel->type) < 0) + if (virXMLPropEnumDefault(ctxt->node, "type", + virDomainSeclabelTypeFromString, + VIR_XML_PROP_NONZERO, + &seclabel->type, + VIR_DOMAIN_SECLABEL_DYNAMIC) < 0) { return NULL; + } if (seclabel->type == VIR_DOMAIN_SECLABEL_STATIC || seclabel->type == VIR_DOMAIN_SECLABEL_NONE) -- 2.52.0
On Thu, Mar 26, 2026 at 12:51:23 +0100, Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@redhat.com>
Prior to v7.10.0-rc1~26 seclabels defaulted to
Given that it's ~5 years since I broke this ...
VIR_DOMAIN_SECLABEL_DYNAMIC (type='dynamic'). But after switching the parser to virXMLPropEnum() the type is overwritten to VIR_DOMAIN_SECLABEL_DEFAULT because the first thing that the helper function does is to set variable that holds the result to zero. Switch to virXMLPropEnumDefault() to restore the previous behavior.
.... I wonder what the implications actually are (I didn't check). The commit message explains what happened w.r.t the behaviour of virXMLPropEnum() but don't detail what the effect of the change was so I wonder why we didn't see any problem with this or why we now want to change it back besides "it was like that before". Did you happen to figure out what the change actually impacted?
On 3/31/26 14:57, Peter Krempa wrote:
On Thu, Mar 26, 2026 at 12:51:23 +0100, Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@redhat.com>
Prior to v7.10.0-rc1~26 seclabels defaulted to
Given that it's ~5 years since I broke this ...
VIR_DOMAIN_SECLABEL_DYNAMIC (type='dynamic'). But after switching the parser to virXMLPropEnum() the type is overwritten to VIR_DOMAIN_SECLABEL_DEFAULT because the first thing that the helper function does is to set variable that holds the result to zero. Switch to virXMLPropEnumDefault() to restore the previous behavior.
.... I wonder what the implications actually are (I didn't check). The commit message explains what happened w.r.t the behaviour of virXMLPropEnum() but don't detail what the effect of the change was so I wonder why we didn't see any problem with this or why we now want to change it back besides "it was like that before".
Did you happen to figure out what the change actually impacted?
Yeah, I was thinking about that too. But I think we're on the safe side because of extensive checks the function does. I mean, the moment you want relabel='yes' you also need either type='static' or type='dynamic'. These then get properly formatted into saved domain XML. There's only one corner case (through which I encountered this bug). In the original bug report, Rich specified "<seclabel model="selinux" relabel="no"/>". I did not realize it was disk's seclabel and thought it's the top level domain seclabel. Now, our RNG doesn't consider this valid, nevertheless, ignoring XML validation error, I've realized the seclabel was parsed and stored in virDomainDef, yet not formatted back (thanks to short circuit at the very beginning of virSecurityLabelDefFormat()). Long story short, the only case in which we can have "troubles" is when users do NOT want us to use certain secdriver AND they pass incomplete XML that our RNG rejects. Michal
From: Michal Privoznik <mprivozn@redhat.com> Let's use automatic memory freeing inside of virSecurityManagerCheckModel() as it will simplify future commits. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_manager.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 5fc4eb4872..f2f3bb4f19 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -729,9 +729,8 @@ virSecurityManagerReleaseLabel(virSecurityManager *mgr, static int virSecurityManagerCheckModel(virSecurityManager *mgr, char *secmodel) { - int ret = -1; + g_autofree virSecurityManager **sec_managers = NULL; size_t i; - virSecurityManager **sec_managers = NULL; if (STREQ_NULLABLE(secmodel, "none")) return 0; @@ -741,17 +740,14 @@ static int virSecurityManagerCheckModel(virSecurityManager *mgr, for (i = 0; sec_managers[i]; i++) { if (STREQ_NULLABLE(secmodel, sec_managers[i]->drv->name)) { - ret = 0; - goto cleanup; + return 0; } } virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Security driver model '%1$s' is not available"), secmodel); - cleanup: - VIR_FREE(sec_managers); - return ret; + return -1; } -- 2.52.0
On Thu, Mar 26, 2026 at 12:51:24 +0100, Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@redhat.com>
Let's use automatic memory freeing inside of virSecurityManagerCheckModel() as it will simplify future commits.
Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_manager.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-)
Reviewed-by: Peter Krempa <pkrempa@redhat.com>
From: Michal Privoznik <mprivozn@redhat.com> Ever since of commit v1.2.13-rc1~66 the model attribute of a <seclabel/> is validated against secdriver names enabled. In nearly all cases this is something users want so that domain XML does not claim to set seclabels of a model that's not enabled. However, consider the following seclabel: <seclabel type='none' model='selinux'/> It tells us to not bother setting selinux labels on given domain. A mgmt app might format this into domain XML if it sees selinux is disabled on the host. But if that's the case, selinux driver is not loaded and this virSecurityManagerCheckModel() doesn't find it and reports an error. Well, the error doesn't need to be reported as we will just ignore selinux as each driver callback checks if relabel is false (which it is for type='none'). This is true for other secdrivers too. Resolves: https://redhat.atlassian.net/browse/RHEL-156689 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_manager.c | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-) diff --git a/src/security/security_manager.c b/src/security/security_manager.c index f2f3bb4f19..7023ac2db8 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -727,7 +727,8 @@ virSecurityManagerReleaseLabel(virSecurityManager *mgr, static int virSecurityManagerCheckModel(virSecurityManager *mgr, - char *secmodel) + char *secmodel, + bool relabel) { g_autofree virSecurityManager **sec_managers = NULL; size_t i; @@ -744,6 +745,19 @@ static int virSecurityManagerCheckModel(virSecurityManager *mgr, } } + if (relabel == false) { + const char * const knownModels[] = { + "none", "apparmor", "dac", "selinux" + }; + + for (i = 0; i < G_N_ELEMENTS(knownModels); i++) { + if (STREQ_NULLABLE(secmodel, knownModels[i])) { + VIR_INFO("Ignoring seclabel with model %s and relabel=no", secmodel); + return 0; + } + } + } + virReportError(VIR_ERR_CONFIG_UNSUPPORTED, _("Security driver model '%1$s' is not available"), secmodel); @@ -758,8 +772,11 @@ virSecurityManagerCheckDomainLabel(virSecurityManager *mgr, size_t i; for (i = 0; i < def->nseclabels; i++) { - if (virSecurityManagerCheckModel(mgr, def->seclabels[i]->model) < 0) + if (virSecurityManagerCheckModel(mgr, + def->seclabels[i]->model, + def->seclabels[i]->relabel) < 0) { return -1; + } } return 0; @@ -773,8 +790,11 @@ virSecurityManagerCheckDiskLabel(virSecurityManager *mgr, size_t i; for (i = 0; i < disk->src->nseclabels; i++) { - if (virSecurityManagerCheckModel(mgr, disk->src->seclabels[i]->model) < 0) + if (virSecurityManagerCheckModel(mgr, + disk->src->seclabels[i]->model, + disk->src->seclabels[i]->relabel) < 0) { return -1; + } } return 0; @@ -788,8 +808,11 @@ virSecurityManagerCheckChardevLabel(virSecurityManager *mgr, size_t i; for (i = 0; i < dev->source->nseclabels; i++) { - if (virSecurityManagerCheckModel(mgr, dev->source->seclabels[i]->model) < 0) + if (virSecurityManagerCheckModel(mgr, + dev->source->seclabels[i]->model, + dev->source->seclabels[i]->relabel) < 0) { return -1; + } } return 0; -- 2.52.0
I tested the series and it resolves the issue, thus: Tested-by: Richard W.M. Jones <rjones@redhat.com> - - - I wanted to clarify how we are using <seclabel/> since the original description on RHEL-156689 was wrong (I've updated it there). We put this in the main body of the XML: <seclabel type="none" model="selinux"/> We put this into some <disk/> sections: <disk device="disk" type="file"> <source file="scratch1.img"> <seclabel model="selinux" relabel="no"/> </source> I was concerned that in your comment you said this second usage of <seclabel/> is wrong. However the documentation seems to contradict this (although the documentation is not very clear). Is the second usage above correct or not? There's no simple way to check the XML we generate against the RNG, so if libvirt accepts it then we won't find out if it's technically wrong. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
On Thu, Mar 26, 2026 at 12:51:25 +0100, Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@redhat.com>
Ever since of commit v1.2.13-rc1~66 the model attribute of a <seclabel/> is validated against secdriver names enabled. In nearly all cases this is something users want so that domain XML does not claim to set seclabels of a model that's not enabled. However, consider the following seclabel:
<seclabel type='none' model='selinux'/>
It tells us to not bother setting selinux labels on given domain. A mgmt app might format this into domain XML if it sees selinux is disabled on the host. But if that's the case, selinux driver is not loaded and this virSecurityManagerCheckModel() doesn't find it and reports an error.
Well, the error doesn't need to be reported as we will just ignore selinux as each driver callback checks if relabel is false (which it is for type='none'). This is true for other secdrivers too.
Resolves: https://redhat.atlassian.net/browse/RHEL-156689 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_manager.c | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-)
diff --git a/src/security/security_manager.c b/src/security/security_manager.c index f2f3bb4f19..7023ac2db8 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -727,7 +727,8 @@ virSecurityManagerReleaseLabel(virSecurityManager *mgr,
static int virSecurityManagerCheckModel(virSecurityManager *mgr, - char *secmodel) + char *secmodel, + bool relabel) { g_autofree virSecurityManager **sec_managers = NULL; size_t i; @@ -744,6 +745,19 @@ static int virSecurityManagerCheckModel(virSecurityManager *mgr, } }
+ if (relabel == false) { + const char * const knownModels[] = { + "none", "apparmor", "dac", "selinux" + };
I don't like the hardcoded list of security drivers
+ + for (i = 0; i < G_N_ELEMENTS(knownModels); i++) {
If you NULL-terminate the list you can use g_strv_contains instead of the loop.
+ if (STREQ_NULLABLE(secmodel, knownModels[i])) { + VIR_INFO("Ignoring seclabel with model %s and relabel=no", secmodel); + return 0; + }
I was considering if this should behave differently if the secdriver is explicitly disabled rather than disabled either by autoprobe or not even compiled in. Since for the existing secdrivers we disable them by disabling "relabel" (e.g. there are no necessary steps to avoid confinment) I guess this is okay. IMO to avoid the hardcoded list you IMO should ignore any label requesting disabling of given security driver if the driver is not enabled. That includes unknown ones. I don't think there's a need to report error for those.
On 3/31/26 14:52, Peter Krempa wrote:
On Thu, Mar 26, 2026 at 12:51:25 +0100, Michal Privoznik via Devel wrote:
From: Michal Privoznik <mprivozn@redhat.com>
Ever since of commit v1.2.13-rc1~66 the model attribute of a <seclabel/> is validated against secdriver names enabled. In nearly all cases this is something users want so that domain XML does not claim to set seclabels of a model that's not enabled. However, consider the following seclabel:
<seclabel type='none' model='selinux'/>
It tells us to not bother setting selinux labels on given domain. A mgmt app might format this into domain XML if it sees selinux is disabled on the host. But if that's the case, selinux driver is not loaded and this virSecurityManagerCheckModel() doesn't find it and reports an error.
Well, the error doesn't need to be reported as we will just ignore selinux as each driver callback checks if relabel is false (which it is for type='none'). This is true for other secdrivers too.
Resolves: https://redhat.atlassian.net/browse/RHEL-156689 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_manager.c | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-)
diff --git a/src/security/security_manager.c b/src/security/security_manager.c index f2f3bb4f19..7023ac2db8 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -727,7 +727,8 @@ virSecurityManagerReleaseLabel(virSecurityManager *mgr,
static int virSecurityManagerCheckModel(virSecurityManager *mgr, - char *secmodel) + char *secmodel, + bool relabel) { g_autofree virSecurityManager **sec_managers = NULL; size_t i; @@ -744,6 +745,19 @@ static int virSecurityManagerCheckModel(virSecurityManager *mgr, } }
+ if (relabel == false) { + const char * const knownModels[] = { + "none", "apparmor", "dac", "selinux" + };
I don't like the hardcoded list of security drivers
Yeah, me neither. But there's hardly going to be a new one.
+ + for (i = 0; i < G_N_ELEMENTS(knownModels); i++) {
If you NULL-terminate the list you can use g_strv_contains instead of the loop.
+ if (STREQ_NULLABLE(secmodel, knownModels[i])) { + VIR_INFO("Ignoring seclabel with model %s and relabel=no", secmodel); + return 0; + }
I was considering if this should behave differently if the secdriver is explicitly disabled rather than disabled either by autoprobe or not even compiled in.
Since for the existing secdrivers we disable them by disabling "relabel" (e.g. there are no necessary steps to avoid confinment) I guess this is okay.
IMO to avoid the hardcoded list you IMO should ignore any label requesting disabling of given security driver if the driver is not enabled. That includes unknown ones. I don't think there's a need to report error for those.
Fair enough. I've got stuck on a corner case where I set the following seclabel: <seclabel type='none' model='MyOwnModel'/> and it was formatted back into the live XML. But maybe there's nothing wrong with that. Alright, for completeness, let me post v2 which drops knownModels[]. Michal
On a Thursday in 2026, Michal Privoznik via Devel wrote:
*** BLURB HERE ***
Michal Prívozník (3): conf: Fix seclabel type parsing wrt default value security: Rewrite virSecurityManagerCheckModel() to use g_autofree security: Don't error out on seclabels of type='none'
src/conf/domain_conf.c | 13 +++++------ src/security/security_manager.c | 41 ++++++++++++++++++++++++--------- 2 files changed, 36 insertions(+), 18 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (5)
-
Ján Tomko -
Michal Privoznik -
Michal Prívozník
-
Peter Krempa -
Richard W.M. Jones