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

19 Jan 2021

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@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?
...
+
+    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. 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.

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


    