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@xxxxxxxxxx>
> > ---
> > 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.
