[libvirt] [PATCH] fix parsing security labels from virt-aa-helper

[Guido Günther]

Hi Christian,

On Mon, Oct 31, 2016 at 11:32:44AM +0100, Christian Ehrhardt wrote:
> When parsing labels virt-aa-helper does no more pass
> VIR_DOMAIN_DEF_PARSE_INACTIVE due to dfbc9a83 that tried to mitigate the
> changes of a89f05ba. For those it had to switch from
> VIR_DOMAIN_DEF_PARSE_INACTIVE to active since we need the domain id
> (ctl->def->id) as it is part of the socket path now which is needed for the
> aa profile.
> 
> But that turned out to break non apparmor seclabels as well as apparmor
> seclabels in xmls without labels.
> In those cases due to VIR_DOMAIN_DEF_PARSE_INACTIVE now not set anymore
> virSecurityLabelDefParseXML insists on finding labels. Cases:
> - non-apparmor seclabel - virt-aa-helper breaks
> - apparmor seclabel without labels on a defined domain - virt-aa-helper breaks
> This was not spotted due to labels getting dynamically created on definition.
> So "define, start, stop" works. But "define, edit (add label), start" does not.
> 
> Now turning back on VIR_DOMAIN_DEF_PARSE_INACTIVE would cause the old bug, so
> we have to differ those more fine grained. This is done by the new flag
> VIR_DOMAIN_DEF_PARSE_SKIP_ACTIVE_LABEL which is like
> VIR_DOMAIN_DEF_PARSE_INACTIVE but only for the security labels.
> So far only set by virt-aa-helper.
> 
> Testcase with virt-aa-helper on xml file:
>  virt-aa-helper -d -r -p 0 -u libvirt-<uuid> < your-guest.xml
>    virt-aa-helper: error: could not parse XML
>    virt-aa-helper: error: could not get VM definition
> (That should have printed a valid apparmor profile)

This should be shortened and clarified (see the other part of the
thread). IMHO the root cause is that we parse the active domain XML but
the live part of the seclabel is not filled in yet.

> 
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt at canonical.com>
> ---
>  src/conf/domain_conf.c        | 6 ++++--
>  src/conf/domain_conf.h        | 3 +++
>  src/security/virt-aa-helper.c | 1 +
>  3 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
> index 03506cb..9eb7883 100644
> --- a/src/conf/domain_conf.c
> +++ b/src/conf/domain_conf.c
> @@ -6626,7 +6626,8 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
>       * if the 'live' VM XML is requested
>       */
>      if (seclabel->type == VIR_DOMAIN_SECLABEL_STATIC ||
> -        (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) &&
> +        (!(flags & (VIR_DOMAIN_DEF_PARSE_SKIP_ACTIVE_LABEL |
> +                    VIR_DOMAIN_DEF_PARSE_INACTIVE)) &&
>           seclabel->type != VIR_DOMAIN_SECLABEL_NONE)) {
>          p = virXPathStringLimit("string(./label[1])",
>                                  VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
> @@ -6642,7 +6643,8 @@ virSecurityLabelDefParseXML(xmlXPathContextPtr ctxt,
>  
>      /* Only parse imagelabel, if requested live XML with relabeling */
>      if (seclabel->relabel &&
> -        (!(flags & VIR_DOMAIN_DEF_PARSE_INACTIVE) &&
> +        (!(flags & (VIR_DOMAIN_DEF_PARSE_SKIP_ACTIVE_LABEL |
> +                    VIR_DOMAIN_DEF_PARSE_INACTIVE)) &&
>           seclabel->type != VIR_DOMAIN_SECLABEL_NONE)) {
>          p = virXPathStringLimit("string(./imagelabel[1])",
>                                  VIR_SECURITY_LABEL_BUFLEN-1, ctxt);
> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> index 24aa79c..90693c6 100644
> --- a/src/conf/domain_conf.hp
> +++ b/src/conf/domain_conf.h
> @@ -2657,6 +2657,9 @@ typedef enum {
>      VIR_DOMAIN_DEF_PARSE_ABI_UPDATE = 1 << 9,
>      /* skip definition validation checks meant to be executed on define time only */
>      VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE = 1 << 10,
> +    /* in regard to security labels, skip parts of the XML that would only be
> +     * present in an active libvirt XML. */
> +    VIR_DOMAIN_DEF_PARSE_SKIP_ACTIVE_LABEL        = 1 << 11,

/* skip parsing of seclabel */
VIR_DOMAIN_DEF_PARSE_SKIP_SECLABEL        = 1 << 11,

is IMHO shorter and I would then change the code to skip the whole
seclabel parsing since it's of no need for virt-aa-helper.

Another possibility is to not introduce a new flag but filter out
seclabels in virt-aa-helper before parsing the XML without cluttering
domain_conf.c even more for this special case.

Cheers,
 -- Guido

>  } virDomainDefParseFlags;
>  
>  typedef enum {
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
> index 77eeaff..0ca4c83 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -705,6 +705,7 @@ get_definition(vahControl * ctl, const char *xmlStr)
>  
>      ctl->def = virDomainDefParseString(xmlStr,
>                                         ctl->caps, ctl->xmlopt, NULL,
> +                                       VIR_DOMAIN_DEF_PARSE_SKIP_ACTIVE_LABEL |
>                                         VIR_DOMAIN_DEF_PARSE_SKIP_VALIDATE);
>  
>      if (ctl->def == NULL) {
> -- 
> 2.7.4
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>