[PATCH 08/12] virSecurityLabelDefParseXML: Don't use 'virXPathStringLimit'

[Peter Krempa]

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 at 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.