On Mon, Nov 22, 2021 at 21:01:58 +0100, Tim Wiederhake wrote:
On Mon, 2021-11-22 at 18:12 +0100, Peter Krempa wrote:
virXPathStringLimit doesn't give callers a way to differentiate between the queried XPath being empty and the length limit being exceeded.
This means that callers are either overwriting the error message or ignoring it altogether.
Move the length checks into the caller.
Signed-off-by: Peter Krempa <pkrempa@redhat.com> --- src/conf/domain_conf.c | 22 ++++++++++++++-------- 1 file changed, 14 insertions(+), 8 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index ee44bbbd4b..bd9da0744d 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -7871,9 +7871,9 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, if (seclabel->type == VIR_DOMAIN_SECLABEL_STATIC || (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && seclabel->type != VIR_DOMAIN_SECLABEL_NONE)) { - seclabel->label = virXPathStringLimit("string(./label[1])", - VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (!seclabel->label) { + seclabel->label = virXPathString("string(./label[1])", ctxt); + if (!seclabel->label || + strlen(seclabel->label) >= VIR_SECURITY_LABEL_BUFLEN - 1) {
What is your opinion on using strnlen instead? Not necessarily for security reasons, but since we do not care for the exact string length if it is above a certain size, we can return early.
Yeah, selling it as security is impossible because we already have at least two copies of the XML in memory. As a optimization sure, but let's see whether it doesn't make the code too bulky as the lenght constant appears twice in the expression.
virReportError(VIR_ERR_XML_ERROR, "%s", _("security label is missing")); return NULL; @@ -7884,9 +7884,10 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt, if (seclabel->relabel && (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) && seclabel->type != VIR_DOMAIN_SECLABEL_NONE)) { - seclabel->imagelabel = virXPathStringLimit("string(./imagelabel[1])", - VIR_SECURITY_LABEL_BUFLEN-1, ctxt); - if (!seclabel->imagelabel) { + seclabel->imagelabel = virXPathString("string(./imagelabel[1])", ctxt); + + if (!seclabel->imagelabel || + strlen(seclabel->imagelabel) >= VIR_SECURITY_LABEL_BUFLEN - 1) { virReportError(VIR_ERR_XML_ERROR, "%s", _("security imagelabel is missing")); return NULL; @@ -7895,8 +7896,13 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
/* Only parse baselabel for dynamic label type */ if (seclabel->type == VIR_DOMAIN_SECLABEL_DYNAMIC) { - seclabel->baselabel = virXPathStringLimit("string(./baselabel[1])", - VIR_SECURITY_LABEL_BUFLEN-1, ctxt); + seclabel->baselabel = virXPathString("string(./baselabel[1])", ctxt); + + if (seclabel->baselabel && + strlen(seclabel->baselabel) >= VIR_SECURITY_LABEL_BUFLEN - 1) { + g_free(seclabel->baselabel); + seclabel->baselabel = NULL; + }
g_clear_pointer?
Yeah, I've actually used it in later patch.