[PATCH 0/3] apparmor: Separate seclabel generation and profile loading
See 3/3 for explanation. Michal Prívozník (3): security: Introduce virSecurityDomainLoadProfile() qemu: Call virSecurityManagerLoadProfile() at the end of qemuProcessPrepareDomain() security_apparmor: Move profile loading into .domainLoadProfile callback src/libvirt_private.syms | 1 + src/qemu/qemu_process.c | 7 +++++++ src/qemu/qemu_security.h | 1 + src/security/security_apparmor.c | 31 +++++++++++++++++++++++-------- src/security/security_driver.h | 4 ++++ src/security/security_manager.c | 13 +++++++++++++ src/security/security_manager.h | 2 ++ src/security/security_stack.c | 15 +++++++++++++++ 8 files changed, 66 insertions(+), 8 deletions(-) -- 2.52.0
From: Michal Privoznik <mprivozn@redhat.com> Specifically tailored for AppArmor, so that generating a seclabel and producing profile can be separated. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/libvirt_private.syms | 1 + src/security/security_driver.h | 4 ++++ src/security/security_manager.c | 13 +++++++++++++ src/security/security_manager.h | 2 ++ src/security/security_stack.c | 15 +++++++++++++++ 5 files changed, 35 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4e57e4a8f6..64152c3bbb 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1822,6 +1822,7 @@ virSecurityManagerGetModel; virSecurityManagerGetMountOptions; virSecurityManagerGetNested; virSecurityManagerGetProcessLabel; +virSecurityManagerLoadProfile; virSecurityManagerMoveImageMetadata; virSecurityManagerNew; virSecurityManagerNewDAC; diff --git a/src/security/security_driver.h b/src/security/security_driver.h index b8c5b416e3..d81662dab4 100644 --- a/src/security/security_driver.h +++ b/src/security/security_driver.h @@ -81,6 +81,8 @@ typedef int (*virSecurityDomainReserveLabel) (virSecurityManager *mgr, pid_t pid); typedef int (*virSecurityDomainReleaseLabel) (virSecurityManager *mgr, virDomainDef *sec); +typedef int (*virSecurityDomainLoadProfile) (virSecurityManager *mgr, + virDomainDef *def); typedef int (*virSecurityDomainSetAllLabel) (virSecurityManager *mgr, char *const *sharedFilesystems, virDomainDef *sec, @@ -211,6 +213,8 @@ struct _virSecurityDriver { virSecurityDomainReserveLabel domainReserveSecurityLabel; virSecurityDomainReleaseLabel domainReleaseSecurityLabel; + virSecurityDomainLoadProfile domainLoadProfile; + virSecurityDomainGetProcessLabel domainGetSecurityProcessLabel; virSecurityDomainSetProcessLabel domainSetSecurityProcessLabel; virSecurityDomainSetChildProcessLabel domainSetSecurityChildProcessLabel; diff --git a/src/security/security_manager.c b/src/security/security_manager.c index 5fc4eb4872..87c8b9f3c1 100644 --- a/src/security/security_manager.c +++ b/src/security/security_manager.c @@ -726,6 +726,19 @@ virSecurityManagerReleaseLabel(virSecurityManager *mgr, } +int +virSecurityManagerLoadProfile(virSecurityManager *mgr, + virDomainDef *def) +{ + VIR_LOCK_GUARD lock = virObjectLockGuard(mgr); + + if (!mgr->drv->domainLoadProfile) + return 0; + + return mgr->drv->domainLoadProfile(mgr, def); +} + + static int virSecurityManagerCheckModel(virSecurityManager *mgr, char *secmodel) { diff --git a/src/security/security_manager.h b/src/security/security_manager.h index 068ca4e290..381b614ec1 100644 --- a/src/security/security_manager.h +++ b/src/security/security_manager.h @@ -128,6 +128,8 @@ int virSecurityManagerReserveLabel(virSecurityManager *mgr, pid_t pid); int virSecurityManagerReleaseLabel(virSecurityManager *mgr, virDomainDef *sec); +int virSecurityManagerLoadProfile(virSecurityManager *mgr, + virDomainDef *def); int virSecurityManagerCheckAllLabel(virSecurityManager *mgr, virDomainDef *sec); int virSecurityManagerSetAllLabel(virSecurityManager *mgr, diff --git a/src/security/security_stack.c b/src/security/security_stack.c index 99a68a6053..96b59d159b 100644 --- a/src/security/security_stack.c +++ b/src/security/security_stack.c @@ -280,6 +280,19 @@ virSecurityStackReserveLabel(virSecurityManager *mgr, } +static int +virSecurityStackLoadProfile(virSecurityManager *mgr, + virDomainDef *vm) +{ + int rc = 0; + + if (virSecurityManagerLoadProfile(virSecurityStackGetPrimary(mgr), vm) < 0) + rc = -1; + + return rc; +} + + static int virSecurityStackSetHostdevLabel(virSecurityManager *mgr, virDomainDef *vm, @@ -1070,6 +1083,8 @@ virSecurityDriver virSecurityDriverStack = { .domainReserveSecurityLabel = virSecurityStackReserveLabel, .domainReleaseSecurityLabel = virSecurityStackReleaseLabel, + .domainLoadProfile = virSecurityStackLoadProfile, + .domainGetSecurityProcessLabel = virSecurityStackGetProcessLabel, .domainSetSecurityProcessLabel = virSecurityStackSetProcessLabel, .domainSetSecurityChildProcessLabel = virSecurityStackSetChildProcessLabel, -- 2.52.0
From: Michal Privoznik <mprivozn@redhat.com> So far, this is a NOP as no secdriver implements the callback. But the idea is to separate seclabel generation on profile loading for AppArmor. See next commit. Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/qemu/qemu_process.c | 7 +++++++ src/qemu/qemu_security.h | 1 + 2 files changed, 8 insertions(+) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index a53bb40783..5d5b1b291b 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -7154,6 +7154,13 @@ qemuProcessPrepareDomain(virQEMUDriver *driver, } } + /* Keep this as the last step so that security drivers can + * see all the path generated in steps above. */ + if (!(flags & VIR_QEMU_PROCESS_START_PRETEND)) { + if (qemuSecurityManagerLoadProfile(driver->securityManager, vm->def) < 0) + return -1; + } + return 0; } diff --git a/src/qemu/qemu_security.h b/src/qemu/qemu_security.h index 36663cffde..d540c01f77 100644 --- a/src/qemu/qemu_security.h +++ b/src/qemu/qemu_security.h @@ -137,6 +137,7 @@ int qemuSecurityCommandRun(virQEMUDriver *driver, #define qemuSecurityGetMountOptions virSecurityManagerGetMountOptions #define qemuSecurityGetNested virSecurityManagerGetNested #define qemuSecurityGetProcessLabel virSecurityManagerGetProcessLabel +#define qemuSecurityManagerLoadProfile virSecurityManagerLoadProfile #define qemuSecurityNew virSecurityManagerNew #define qemuSecurityNewDAC virSecurityManagerNewDAC #define qemuSecurityNewStack virSecurityManagerNewStack -- 2.52.0
From: Michal Privoznik <mprivozn@redhat.com> Here's the problem. When starting a QEMU domain, the process is split into several phases. A simplified process looks like this: 1) qemuProcessPrepareDomain() 1a) qemuSecurityGenLabel() 1b) generate run time paths 1c) qemuSecurityManagerLoadProfile() 2) qemuProcessPrepareHost() 2a) qemuSecurityDomainSetPathLabel() /* transitively */ 3) qemuProcessLaunch() 3a) qemuSecuritySetAllLabel() NB, step 2a) also contains helper processes used to set up host, that we want to run in the security context of the domain (e.g. swtpm_setup). This works well for SELinux and DAC because their APIs basically match 1:1 to ours. But it's not that simple with AppArmor. It doesn't contain any profile upfront, so one is generated in step 1a) (among with seclabel). But at that point, the domain definition has no run time info. This can then lead to some paths being left out (for instance, disks which source wasn't translated from storage pool/vol spec into a path). After previous commits, there's this new step 1c) which can actually load the profile. Therefore, move profile loading into .domainLoadProfile callback. Resolves: https://gitlab.com/libvirt/libvirt/-/issues/135 Signed-off-by: Michal Privoznik <mprivozn@redhat.com> --- src/security/security_apparmor.c | 31 +++++++++++++++++++++++-------- 1 file changed, 23 insertions(+), 8 deletions(-) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 68ac39611f..98fad9034d 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -387,23 +387,36 @@ AppArmorGenSecurityLabel(virSecurityManager *mgr G_GNUC_UNUSED, if (!secdef->model) secdef->model = g_strdup(SECURITY_APPARMOR_NAME); - /* Now that we have a label, load the profile into the kernel. */ + return 0; +} + + +static int +AppArmorLoadProfile(virSecurityManager *mgr G_GNUC_UNUSED, + virDomainDef *def) +{ + virSecurityLabelDef *secdef = virDomainDefGetSecurityLabelDef(def, + SECURITY_APPARMOR_NAME); + + if (!secdef) + return 0; + + if ((secdef->type == VIR_DOMAIN_SECLABEL_STATIC) || + (secdef->type == VIR_DOMAIN_SECLABEL_NONE)) { + return 0; + } + if (load_profile(mgr, secdef->label, def, NULL, false) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot load AppArmor profile \'%1$s\'"), secdef->label); - goto err; + return -1; } return 0; - - err: - VIR_FREE(secdef->label); - VIR_FREE(secdef->imagelabel); - VIR_FREE(secdef->model); - return -1; } + static int AppArmorSetSecurityAllLabel(virSecurityManager *mgr, char *const *sharedFilesystems G_GNUC_UNUSED, @@ -1157,6 +1170,8 @@ virSecurityDriver virAppArmorSecurityDriver = { .domainReserveSecurityLabel = AppArmorReserveSecurityLabel, .domainReleaseSecurityLabel = AppArmorReleaseSecurityLabel, + .domainLoadProfile = AppArmorLoadProfile, + .domainGetSecurityProcessLabel = AppArmorGetSecurityProcessLabel, .domainSetSecurityProcessLabel = AppArmorSetSecurityProcessLabel, .domainSetSecurityChildProcessLabel = AppArmorSetSecurityChildProcessLabel, -- 2.52.0
participants (1)
-
Michal Privoznik