[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

[libvirt] [PATCH] Move load of AppArmor profile to GenLabel()



Commit 12317957ecd6c37a2fb16275dcdeeacfe25c517 introduced an incompatible
architectural change for the AppArmor security driver. Specifically,
virSecurityManagerSetAllLabel() is now called much later in
src/qemu/qemu_process.c:qemuProcessStart(). Previously, SetAllLabel() was
called immediately after GenLabel() such that after the dynamic label (profile
name) was generated, SetAllLabel() would be called to create and load the
AppArmor profile into the kernel before qemuProcessHook() was executed. With
12317957ecd6c37a2fb16275dcdeeacfe25c517, qemuProcessHook() is now called
before SetAllLabel(), such that aa_change_profile() ends up being called
before the AppArmor profile is loaded into the kernel (via ProcessLabel() in
qemuProcessHook()).

This patch addresses the change by making GenLabel() load the AppArmor
profile into the kernel after the label (profile name) is generated.
SetAllLabel() is then adjusted to only reload_profile() and append stdin_fn to
the profile when it is specified. This also makes the AppArmor driver work
like its SELinux counterpart with regard to SetAllLabel() and stdin_fn.

Passes its part of 'check' and 'syntax-check' (though
libvirt_net_rpc_la-virnetmessage.lo failed to compile and po_check
failed the syntax check for unrelated reasons).

Prior to writing this patch, the issue was discussed on IRC with Daniel
Berrange and Eric Blake. Other discussed alternatives were to do a 2
stage qemuProcessHook(), where ProcessLabel() was only called in the
second stage after the handshake. In attempting this, it still didn't
address the fact that aa_change_profile() was being called on an
unloaded profile so it was abandoned. Another idea was to create a
second private API call that the AppArmor driver would use to load the
profile that would be a no-op for the SElinux driver. I started to go
this route, but in the end it was both unneeded and too complicated once
I realized I could simply load the profile in GenLabel() and still use
SetAllLabel() to reload the profile when stdin_path was specified. The
current fix is implemented wholly within the AppArmor driver and I think
much cleaner.

Reference: https://launchpad.net/bugs/795800

-- 
Jamie Strandboge             | http://www.canonical.com
Author: Jamie Strandboge <jamie canonical com>
Description: Move load of AppArmor profile to GenLabel()
 Commit 12317957ecd6c37a2fb16275dcdeeacfe25c517 introduced an incompatible
 architectural change for the AppArmor security driver. Specifically,
 virSecurityManagerSetAllLabel() is now called much later in
 src/qemu/qemu_process.c:qemuProcessStart(). Previously, SetAllLabel() was
 called immediately after GenLabel() such that after the dynamic label (profile
 name) was generated, SetAllLabel() would be called to create and load the
 AppArmor profile into the kernel before qemuProcessHook() was executed. With
 12317957ecd6c37a2fb16275dcdeeacfe25c517, qemuProcessHook() is now called
 before SetAllLabel(), such that aa_change_profile() ends up being called
 before the AppArmor profile is loaded into the kernel (via ProcessLabel() in
 qemuProcessHook()).
 .
 This patch addresses the change by making GenLabel() load the AppArmor
 profile into the kernel after the label (profile name) is generated.
 SetAllLabel() is then adjusted to only reload_profile() and append stdin_fn to
 the profile when it is specified. This also makes the AppArmor driver work
 like its SELinux counterpart with regard to SetAllLabel() and stdin_fn.
Bug-Ubuntu: https://launchpad.net/bugs/801569

diff -Naurp libvirt.orig/src/security/security_apparmor.c libvirt/src/security/security_apparmor.c
--- libvirt.orig/src/security/security_apparmor.c	2011-06-24 09:04:41.000000000 -0500
+++ libvirt/src/security/security_apparmor.c	2011-06-24 09:28:29.000000000 -0500
@@ -429,6 +429,14 @@ AppArmorGenSecurityLabel(virSecurityMana
         goto err;
     }
 
+    /* Now that we have a label, load the profile into the kernel. */
+    if (load_profile(mgr, vm->def->seclabel.label, vm, NULL, false) < 0) {
+        virSecurityReportError(VIR_ERR_INTERNAL_ERROR,
+                               _("cannot load AppArmor profile "
+                               "\'%s\'"), vm->def->seclabel.label);
+        goto err;
+    }
+
     rc = 0;
     goto clean;
 
@@ -450,16 +458,10 @@ AppArmorSetSecurityAllLabel(virSecurityM
     if (vm->def->seclabel.type == VIR_DOMAIN_SECLABEL_STATIC)
         return 0;
 
-    /* if the profile is not already loaded, then load one */
-    if (profile_loaded(vm->def->seclabel.label) < 0) {
-        if (load_profile(mgr, vm->def->seclabel.label, vm, stdin_path,
-                         false) < 0) {
-            virSecurityReportError(VIR_ERR_INTERNAL_ERROR,
-                                   _("cannot generate AppArmor profile "
-                                   "\'%s\'"), vm->def->seclabel.label);
-            return -1;
-        }
-    }
+    /* Reload the profile if stdin_path is specified. Note that
+       GenSecurityLabel() will have already been run. */
+    if (stdin_path)
+        return reload_profile(mgr, vm, stdin_path, true);
 
     return 0;
 }

Attachment: signature.asc
Description: This is a digitally signed message part


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]