[PATCH 0/5] Fix qemuNodeGetSecurityModel() and some related cleanups
While I'd love to give more context, I can't - I've found these on a stale branch. However, they are still valid and worth of merging. Michal Prívozník (5): qemu: Use virStrcpy in qemuNodeGetSecurityModel() qemu: Obtain @caps only after ACL check in qemuNodeGetSecurityModel qemu: Fix retval if ACL check fails in qemuNodeGetSecurityModel domain_conf: Parse full length of some <seclabel/> attributes use more virStrcpy() and virStrcpyStatic() src/conf/domain_conf.c | 6 ++---- src/libvirt-lxc.c | 5 ++--- src/qemu/qemu_driver.c | 23 +++++++++-------------- src/remote/remote_driver.c | 12 ++++-------- src/security/security_selinux.c | 3 +-- 5 files changed, 18 insertions(+), 31 deletions(-) -- 2.26.2
The code we have there to copy seclabel model or doi can be replaced by virStrcpy() calls which do exactly the same checks. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a376824854..a9e8f660c7 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5930,7 +5930,6 @@ static int qemuNodeGetSecurityModel(virConnectPtr conn, virSecurityModelPtr secmodel) { virQEMUDriverPtr driver = conn->privateData; - char *p; g_autoptr(virCaps) caps = NULL; memset(secmodel, 0, sizeof(*secmodel)); @@ -5946,23 +5945,21 @@ static int qemuNodeGetSecurityModel(virConnectPtr conn, caps->host.secModels[0].model == NULL) return 0; - p = caps->host.secModels[0].model; - if (strlen(p) >= VIR_SECURITY_MODEL_BUFLEN-1) { + if (virStrcpy(secmodel->model, caps->host.secModels[0].model, + VIR_SECURITY_MODEL_BUFLEN) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("security model string exceeds max %d bytes"), - VIR_SECURITY_MODEL_BUFLEN-1); + VIR_SECURITY_MODEL_BUFLEN - 1); return -1; } - strcpy(secmodel->model, p); - p = caps->host.secModels[0].doi; - if (strlen(p) >= VIR_SECURITY_DOI_BUFLEN-1) { + if (virStrcpy(secmodel->doi, caps->host.secModels[0].doi, + VIR_SECURITY_DOI_BUFLEN) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("security DOI string exceeds max %d bytes"), - VIR_SECURITY_DOI_BUFLEN-1); + VIR_SECURITY_DOI_BUFLEN - 1); return -1; } - strcpy(secmodel->doi, p); return 0; } -- 2.26.2
Even though we are getting driver capabilities with refresh=false (so that it is not expensive), we still should do ACL check first because there is no point in bothering with the capabilities if caller doesn't have permissions to call the API. Also, this way the comment makes more sense. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index a9e8f660c7..96ec84bd1c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5934,14 +5934,12 @@ static int qemuNodeGetSecurityModel(virConnectPtr conn, memset(secmodel, 0, sizeof(*secmodel)); - if (!(caps = virQEMUDriverGetCapabilities(driver, false))) - return 0; - if (virNodeGetSecurityModelEnsureACL(conn) < 0) return 0; /* We treat no driver as success, but simply return no data in *secmodel */ - if (caps->host.nsecModels == 0 || + if (!(caps = virQEMUDriverGetCapabilities(driver, false)) || + caps->host.nsecModels == 0 || caps->host.secModels[0].model == NULL) return 0; -- 2.26.2
While previously we returned 0 this is not correct. We have to return a negative value to indicate error. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_driver.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 96ec84bd1c..88324945ef 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -5935,7 +5935,7 @@ static int qemuNodeGetSecurityModel(virConnectPtr conn, memset(secmodel, 0, sizeof(*secmodel)); if (virNodeGetSecurityModelEnsureACL(conn) < 0) - return 0; + return -1; /* We treat no driver as success, but simply return no data in *secmodel */ if (!(caps = virQEMUDriverGetCapabilities(driver, false)) || -- 2.26.2
In virSecurityLabelDefParseXML() we are parsing the <seclabel/> element among with its attributes. Some of the attributes are limited in length (because of virNodeGetSecurityModel()), however some are not. And for the latter ones we don't need to use virXMLPropStringLimit() to parse them. Moreover, using VIR_SECURITY_LABEL_BUFLEN as the limit is wrong - we are not storing the parsed strings into a static buffer of that size rather than checking if the string passes string -> enum conversion. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/conf/domain_conf.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 384710da40..5a8947eeec 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7713,8 +7713,7 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, /* set default value */ seclabel->type = VIR_DOMAIN_SECLABEL_DYNAMIC; - p = virXMLPropStringLimit(ctxt->node, "type", - VIR_SECURITY_LABEL_BUFLEN - 1); + p = virXMLPropString(ctxt->node, "type"); if (p) { seclabel->type = virDomainSeclabelTypeFromString(p); if (seclabel->type <= 0) { @@ -7729,8 +7728,7 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, seclabel->relabel = false; VIR_FREE(p); - p = virXMLPropStringLimit(ctxt->node, "relabel", - VIR_SECURITY_LABEL_BUFLEN-1); + p = virXMLPropString(ctxt->node, "relabel"); if (p) { if (virStringParseYesNo(p, &seclabel->relabel) < 0) { virReportError(VIR_ERR_XML_ERROR, -- 2.26.2
There are a few places where we open code virStrcpy() or virStrcpyStatic(). Call respective functions instead. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt-lxc.c | 5 ++--- src/remote/remote_driver.c | 12 ++++-------- src/security/security_selinux.c | 3 +-- 3 files changed, 7 insertions(+), 13 deletions(-) diff --git a/src/libvirt-lxc.c b/src/libvirt-lxc.c index f6391214be..2a271b74f0 100644 --- a/src/libvirt-lxc.c +++ b/src/libvirt-lxc.c @@ -35,6 +35,7 @@ # include <sys/apparmor.h> #endif #include "vircgroup.h" +#include "virstring.h" #define VIR_FROM_THIS VIR_FROM_NONE @@ -213,7 +214,7 @@ virDomainLxcEnterSecurityLabel(virSecurityModelPtr model, goto error; } - if (strlen((char *) ctx) >= VIR_SECURITY_LABEL_BUFLEN) { + if (virStrcpy(oldlabel->label, ctx, VIR_SECURITY_LABEL_BUFLEN) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("security label exceeds " "maximum length: %d"), @@ -221,8 +222,6 @@ virDomainLxcEnterSecurityLabel(virSecurityModelPtr model, freecon(ctx); goto error; } - - strcpy(oldlabel->label, (char *) ctx); freecon(ctx); if ((oldlabel->enforcing = security_getenforce()) < 0) { diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index b0af3ee88e..1b784e61c7 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -2328,12 +2328,11 @@ remoteDomainGetSecurityLabel(virDomainPtr domain, virSecurityLabelPtr seclabel) } if (ret.label.label_val != NULL) { - if (strlen(ret.label.label_val) >= sizeof(seclabel->label)) { + if (virStrcpyStatic(seclabel->label, ret.label.label_val) < 0) { virReportError(VIR_ERR_RPC, _("security label exceeds maximum: %zu"), sizeof(seclabel->label) - 1); goto cleanup; } - strcpy(seclabel->label, ret.label.label_val); seclabel->enforcing = ret.enforcing; } @@ -2372,13 +2371,12 @@ remoteDomainGetSecurityLabelList(virDomainPtr domain, virSecurityLabelPtr* secla for (i = 0; i < ret.labels.labels_len; i++) { remote_domain_get_security_label_ret *cur = &ret.labels.labels_val[i]; if (cur->label.label_val != NULL) { - if (strlen(cur->label.label_val) >= sizeof((*seclabels)->label)) { + if (virStrcpyStatic((*seclabels)[i].label, cur->label.label_val) < 0) { virReportError(VIR_ERR_RPC, _("security label exceeds maximum: %zd"), sizeof((*seclabels)->label) - 1); VIR_FREE(*seclabels); goto cleanup; } - strcpy((*seclabels)[i].label, cur->label.label_val); (*seclabels)[i].enforcing = cur->enforcing; } } @@ -2444,21 +2442,19 @@ remoteNodeGetSecurityModel(virConnectPtr conn, virSecurityModelPtr secmodel) } if (ret.model.model_val != NULL) { - if (strlen(ret.model.model_val) >= sizeof(secmodel->model)) { + if (virStrcpyStatic(secmodel->model, ret.model.model_val) < 0) { virReportError(VIR_ERR_RPC, _("security model exceeds maximum: %zu"), sizeof(secmodel->model) - 1); goto cleanup; } - strcpy(secmodel->model, ret.model.model_val); } if (ret.doi.doi_val != NULL) { - if (strlen(ret.doi.doi_val) >= sizeof(secmodel->doi)) { + if (virStrcpyStatic(secmodel->doi, ret.doi.doi_val) < 0) { virReportError(VIR_ERR_RPC, _("security doi exceeds maximum: %zu"), sizeof(secmodel->doi) - 1); goto cleanup; } - strcpy(secmodel->doi, ret.doi.doi_val); } rv = 0; diff --git a/src/security/security_selinux.c b/src/security/security_selinux.c index e9cd95916e..2fc6ef2616 100644 --- a/src/security/security_selinux.c +++ b/src/security/security_selinux.c @@ -1209,7 +1209,7 @@ virSecuritySELinuxGetProcessLabel(virSecurityManagerPtr mgr G_GNUC_UNUSED, return -1; } - if (strlen((char *)ctx) >= VIR_SECURITY_LABEL_BUFLEN) { + if (virStrcpy(sec->label, ctx, VIR_SECURITY_LABEL_BUFLEN) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("security label exceeds " "maximum length: %d"), @@ -1218,7 +1218,6 @@ virSecuritySELinuxGetProcessLabel(virSecurityManagerPtr mgr G_GNUC_UNUSED, return -1; } - strcpy(sec->label, (char *)ctx); freecon(ctx); VIR_DEBUG("label=%s", sec->label); -- 2.26.2
On a Monday in 2021, Michal Privoznik wrote:
While I'd love to give more context, I can't - I've found these on a stale branch. However, they are still valid and worth of merging.
Michal Prívozník (5): qemu: Use virStrcpy in qemuNodeGetSecurityModel() qemu: Obtain @caps only after ACL check in qemuNodeGetSecurityModel qemu: Fix retval if ACL check fails in qemuNodeGetSecurityModel domain_conf: Parse full length of some <seclabel/> attributes use more virStrcpy() and virStrcpyStatic()
src/conf/domain_conf.c | 6 ++---- src/libvirt-lxc.c | 5 ++--- src/qemu/qemu_driver.c | 23 +++++++++-------------- src/remote/remote_driver.c | 12 ++++-------- src/security/security_selinux.c | 3 +-- 5 files changed, 18 insertions(+), 31 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Michal Privoznik