[PATCH] apparmor: let image label setting loop over backing files

[Christian Ehrhardt]

On Tue, Jan 19, 2021 at 11:43 AM Peter Krempa <pkrempa at redhat.com> wrote:
>
> On Tue, Jan 19, 2021 at 11:23:16 +0100, Christian Ehrhardt wrote:
> > When adding a rule for an image file and that image file has a chain
> > of backing files then we need to add a rule for each of those files.
> >
> > To get that iterate over the backing file chain the same way as
> > dac/selinux already do and add a label for each.
> >
> > Fixes: https://gitlab.com/libvirt/libvirt/-/issues/118
> >
> > Signed-off-by: Christian Ehrhardt <christian.ehrhardt at canonical.com>
> > ---
> >  src/security/security_apparmor.c | 39 ++++++++++++++++++++++----------
> >  1 file changed, 27 insertions(+), 12 deletions(-)
> >
> > diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c
> > index 29f0956d22..1f309c0c9f 100644
> > --- a/src/security/security_apparmor.c
> > +++ b/src/security/security_apparmor.c
> > @@ -756,22 +756,13 @@ AppArmorRestoreInputLabel(virSecurityManagerPtr mgr,
> >
> >  /* Called when hotplugging */
> >  static int
> > -AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
> > -                              virDomainDefPtr def,
> > -                              virStorageSourcePtr src,
> > -                              virSecurityDomainImageLabelFlags flags G_GNUC_UNUSED)
> > +AppArmorSetSecurityImageLabelInternal(virSecurityManagerPtr mgr,
> > +                                      virDomainDefPtr def,
> > +                                      virStorageSourcePtr src)
> >  {
> > -    virSecurityLabelDefPtr secdef;
> >      g_autofree char *vfioGroupDev = NULL;
> >      const char *path;
> >
> > -    secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME);
> > -    if (!secdef || !secdef->relabel)
> > -        return 0;
> > -
> > -    if (!secdef->imagelabel)
> > -        return 0;
> > -
> >      if (src->type == VIR_STORAGE_TYPE_NVME) {
> >          const virStorageSourceNVMeDef *nvme = src->nvme;
> >
> > @@ -797,6 +788,30 @@ AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
> >      return reload_profile(mgr, def, path, true);
> >  }
> >
> > +static int
> > +AppArmorSetSecurityImageLabel(virSecurityManagerPtr mgr,
> > +                              virDomainDefPtr def,
> > +                              virStorageSourcePtr src,
> > +                              virSecurityDomainImageLabelFlags flags G_GNUC_UNUSED)
> > +{
> > +    virSecurityLabelDefPtr secdef;
> > +    virStorageSourcePtr n;
> > +
> > +    secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME);
> > +    if (!secdef || !secdef->relabel)
> > +        return 0;
> > +
> > +    if (!secdef->imagelabel)
> > +        return 0;
>
> So apparmor doesn't support per-image security labels?

This was present before, it just got moved as part of this change.
IIRC for apparmor that is only generated once in AppArmorGenSecurityLabel
and later on only used to check if the struct is ok (if it would be NULL that
would indicate a non initialized element).

If I'm missing some further hidden meaning of "imagelabel" please let me know.

> > +
> > +    for (n = src; virStorageSourceIsBacking(n); n = n->backingStore) {
> > +        if (AppArmorSetSecurityImageLabelInternal(mgr, def, n) < 0)
>
> It feels a bit suboptimal to fork+exec the aahelper for every single
> image.

I agree, but right now virt-aa-helper has no interface to take multiple
files in one pass.
Furthermore that also calls apparmor_parser which also could be done once.
Furthermore on the "append a rule" use case IMHO aa-helper isn't
doing much of it's original task - it becomes a overly huge "append a line and
call the parser".

I agree that we maybe should batch this. But when we touch it for that - at
least for the "append a rule" use case we might not want to use
virt-aa-helper at all.
But then this becomes a much bigger rewrite with moving e.g. the knowledge how
to get from UUID->filepath into libvirt to be usable from
virt-aa-helper (for other tasks)
but also from the labeling calls. A few more might move, like the
apparmor_parser call.

I've taken a note that it would be good to at least try and POC that, but TBH
that list is growing recently without much draining it :-)

But for the change presented I'd like to keep it to the issue that
we'd want to fix right now.

> The selinux/dac drivers collect the list of things to do before
> forking when we are in the transaction mode (or do just chown/selinux
> labelling, which is trivial)
>
> Given that this is usually on an expensive code path, it's probably okay
> for now though.

Thanks!

> Reviewed-by: Peter Krempa <pkrempa at redhat.com>
>
>
> > +            return -1;
> > +    }
> > +
> > +    return 0;
> > +}
> > +
> >  static int
> >  AppArmorSecurityVerify(virSecurityManagerPtr mgr G_GNUC_UNUSED,
> >                         virDomainDefPtr def)
> > --
> > 2.30.0
> >
>


-- 
Christian Ehrhardt
Staff Engineer, Ubuntu Server
Canonical Ltd