Re: [PATCH] security_apparmor: store dynamically generated rules

On Fri, Nov 08, 2024 at 15:58:35 -0300, Georgia Garcia wrote: > Some rules are generated dynamically during boot and added to the > AppArmor policy. An example of that is macvtap devices that call the > AppArmorSetFDLabel hook to add a rule for the tap device path. > > Since this information is dynamic, it is not available in the xml > config, therefore whenever a "Restore" hook is called, the entire > profile is regenerated by virt-aa-helper based only the information > from the VM definition, so the dynamic information is lost. > > This patch stores the dynamically generated rules while the domain is > running and reloads them whenever there's a restore operation. > > Note that there are no hooks for restoring FD labels, so that > information is not removed from the set of rules while the domain is > running. Looking at the implementation I see that you store the additional information in memory only, in a global variable. Note that the libvirt daemons support being restarted for updates, which in this case will lead to the pre-existing behaviour as the memory-stored cache of labels is not persisted to disk. Other components such as the qemu driver persist their state in form of on-disk files which are updated based on update of the internal state so that they can operate seamlessly after restart. I'm not familiar enough with the apparmor security model, so I don't know if there's a possibility to re-construct the data from existing runtime state, which would make things much simpler.

> Closes: https://gitlab.com/libvirt/libvirt/-/issues/692 > Signed-off-by: Georgia Garcia <georgia.garcia(a)canonical.com&gt; > --- > src/security/security_apparmor.c | 208 +++++++++++++++++++++++++++++-- > 1 file changed, 200 insertions(+), 8 deletions(-) > > diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c > index 07e95ec81d..ae5815fb0d 100644 > --- a/src/security/security_apparmor.c > +++ b/src/security/security_apparmor.c > @@ -60,6 +60,24 @@ struct SDPDOP { > virDomainDef *def; > }; > > +typedef struct _virSecurityAppArmorLabel virSecurityAppArmorLabel; > +struct _virSecurityAppArmorLabel { > + GPtrArray *paths; > + char *name; > +}; > + > +typedef struct _virSecurityAppArmorLabelList virSecurityAppArmorLabelList; > +struct _virSecurityAppArmorLabelList { > + GPtrArray *labels; > + virMutex lock; > +}; > + > +virSecurityAppArmorLabelList *labelList = NULL; > + > +static int AppArmorRestorePolicy(virSecurityManager *mgr, > + virDomainDef *def, > + char *seclabel); > + > /* > * profile_status returns '-2' on error, '-1' if not loaded, '0' if loaded > * > @@ -273,6 +291,119 @@ reload_profile(virSecurityManager *mgr, > secdef->imagelabel); > return -1; > } > + if (!append) { > + if (AppArmorRestorePolicy(mgr, def, secdef->imagelabel) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("cannot restore rules from AppArmor profile \'%1$s\'"), > + secdef->imagelabel); > + return -1; > + } > + } > + > + } > + return 0; > +} > + > +static int > +AppArmorInitLabelList(void) > +{ > + if (labelList) > + return 0; > + > + labelList = g_new0(virSecurityAppArmorLabelList, 1); > + labelList->labels = g_ptr_array_new(); > + if (virMutexInit(&labelList->lock) < 0) { > + virReportSystemError(errno, "%s", > + _("Failed to initialize AppArmor mutex")); > + return -1; > + } > + return 0; > +} > + > +static void > +AppArmorFreeLabelList(void) > +{ > + size_t i; > + if (!labelList) > + return; > + > + VIR_WITH_MUTEX_LOCK_GUARD(&labelList->lock) { > + for (i = 0; i < labelList->labels->len; i++) { > + virSecurityAppArmorLabel *label = g_ptr_array_index(labelList->labels, i); > + g_ptr_array_free(label->paths, TRUE); > + g_free(label->name); > + } > + g_ptr_array_free(labelList->labels, TRUE); > + } > + virMutexDestroy(&labelList->lock); > + g_free(labelList); > + labelList = NULL; > +} > + > +static int > +AppArmorAppendPathToLabelList(char *seclabel, > + const char *path) > +{ > + size_t i; > + char *new; > + > + if (!path) > + return 0; > + > + if (!labelList) { > + if (AppArmorInitLabelList() < 0) > + return -1; > + } > + > + VIR_WITH_MUTEX_LOCK_GUARD(&labelList->lock) { > + for (i = 0; i < labelList->labels->len; i++) { > + virSecurityAppArmorLabel *label = g_ptr_array_index(labelList->labels, i); > + if (STREQ(seclabel, label->name)) { > + new = g_strdup(path); > + g_ptr_array_add(label->paths, new); > + } > + } > + } > + return 0; > +} > + > +static int > +AppArmorLoadStoredPath(virSecurityAppArmorLabel *label, > + virSecurityManager *mgr, > + virDomainDef *def, > + char *seclabel) > +{ > + size_t i; > + for (i = 0; i < label->paths->len; i++) { > + if (load_profile(mgr, seclabel, def, g_ptr_array_index(label->paths, i), true) < 0) { 'load_profile' is a rather expensive function it formats the XML and then spawns the virt-aa-helper binary which parses the XML and sets up the profile. This call is nested within two loops over arrays. As said I'm not too familiar but this doesn't seem to be a good idea on the first glance. > + virReportError(VIR_ERR_INTERNAL_ERROR, > + _("cannot update AppArmor profile \'%1$s\' \'%2$s\'"), > + seclabel, (char *)g_ptr_array_index(label->paths, i)); > + return -1; > + } > + } > + return 0; > +} > + > +static int > +AppArmorRestorePolicy(virSecurityManager *mgr, > + virDomainDef *def, > + char *seclabel) > +{ > + size_t i; > + > + if (!labelList) { > + return 0; > + } > + > + VIR_WITH_MUTEX_LOCK_GUARD(&labelList->lock) { > + for (i = 0; i < labelList->labels->len; i++) { > + virSecurityAppArmorLabel *label = g_ptr_array_index(labelList->labels, i); > + if (STREQ(seclabel, label->name)) { > + if (AppArmorLoadStoredPath(label, mgr, def, seclabel) < 0) > + return -1; > + } > + } > } > return 0; > } > @@ -330,12 +461,13 @@ AppArmorSecurityManagerProbe(const char *virtDriver G_GNUC_UNUSED) > static int > AppArmorSecurityManagerOpen(virSecurityManager *mgr G_GNUC_UNUSED) > { > - return 0; > + return AppArmorInitLabelList(); > } > > static int > AppArmorSecurityManagerClose(virSecurityManager *mgr G_GNUC_UNUSED) > { > + AppArmorFreeLabelList(); > return 0; > } > > @@ -364,6 +496,7 @@ AppArmorGenSecurityLabel(virSecurityManager *mgr G_GNUC_UNUSED, > g_autofree char *profile_name = NULL; > virSecurityLabelDef *secdef = virDomainDefGetSecurityLabelDef(def, > SECURITY_APPARMOR_NAME); > + virSecurityAppArmorLabel *label; > > if (!secdef) > return 0; > @@ -404,6 +537,18 @@ AppArmorGenSecurityLabel(virSecurityManager *mgr G_GNUC_UNUSED, > goto err; > } > > + if (!labelList) { > + if (AppArmorInitLabelList() < 0) > + goto err; > + } > + > + label = g_new0(virSecurityAppArmorLabel, 1); > + label->paths = g_ptr_array_new(); > + label->name = g_strdup(profile_name); > + VIR_WITH_MUTEX_LOCK_GUARD(&labelList->lock) { > + g_ptr_array_add(labelList->labels, label); > + } Is it guaranteed that the profile name is unique? The rest of the code strictly operates on the first entry with matching name, and this doesn't check that there are duplicates. In fact it seems to me as if 'labelList' should be a hash table instead of a list as you don't ever need the order of the entries and all the code strictly looks up the appropriate entry via matching the string. > + > return 0; > > err: > @@ -421,6 +566,7 @@ AppArmorSetSecurityAllLabel(virSecurityManager *mgr, > bool chardevStdioLogd G_GNUC_UNUSED, > bool migrated G_GNUC_UNUSED) > { > + int rc; > virSecurityLabelDef *secdef = virDomainDefGetSecurityLabelDef(def, > SECURITY_APPARMOR_NAME); > if (!secdef || !secdef->relabel) > @@ -428,8 +574,15 @@ AppArmorSetSecurityAllLabel(virSecurityManager *mgr, > > /* Reload the profile if incomingPath is specified. Note that > GenSecurityLabel() will have already been run. */ > - if (incomingPath) > - return reload_profile(mgr, def, incomingPath, true); > + if (incomingPath) { > + rc = reload_profile(mgr, def, incomingPath, true); > + if (AppArmorAppendPathToLabelList(secdef->imagelabel, incomingPath) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + "%s", _("Could not store path")); > + return -1; > + } > + return rc; > + } > > return 0; > } > @@ -495,6 +648,7 @@ AppArmorRestoreSecurityAllLabel(virSecurityManager *mgr G_GNUC_UNUSED, > bool migrated G_GNUC_UNUSED, > bool chardevStdioLogd G_GNUC_UNUSED) > { > + size_t i; > int rc = 0; > virSecurityLabelDef *secdef = > virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME); > @@ -508,6 +662,21 @@ AppArmorRestoreSecurityAllLabel(virSecurityManager *mgr G_GNUC_UNUSED, > _("could not remove profile for \'%1$s\'"), > secdef->label); > } > + if (labelList) { > + VIR_WITH_MUTEX_LOCK_GUARD(&labelList->lock) { > + for (i = 0; i < labelList->labels->len; i++) { > + virSecurityAppArmorLabel *label; > + label = g_ptr_array_index(labelList->labels, i); > + if (STREQ(secdef->label, label->name)) { > + g_free(label->name); > + g_ptr_array_free(label->paths, TRUE); the libvirt code base doesn't use the capitalized 'TRUE'. > + g_ptr_array_remove_index(labelList->labels, i); > + break; > + } > + } > + } > + } > + > } > return rc; > } > @@ -1082,15 +1251,26 @@ AppArmorSetPathLabel(virSecurityManager *mgr, > { > int rc = -1; > char *full_path = NULL; > + virSecurityLabelDef *secdef; > + > + secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME); > > if (allowSubtree) { > full_path = g_strdup_printf("%s/{,**}", path); > - rc = reload_profile(mgr, def, full_path, true); > - VIR_FREE(full_path); > } else { > - rc = reload_profile(mgr, def, path, true); > + full_path = g_strdup(path); > + } > + > + rc = reload_profile(mgr, def, full_path, true); > + if (rc == 0) { > + if (AppArmorAppendPathToLabelList(secdef->imagelabel, full_path) < 0) { This function already reports it's own error ... > + virReportError(VIR_ERR_INTERNAL_ERROR, > + "%s", _("Could not store path")); ... but you overwrite it here with a worse message. (multiple occurences). > + rc = -1; > + } > } > > + VIR_FREE(full_path); > return rc; > } > > @@ -1107,6 +1287,7 @@ AppArmorSetFDLabel(virSecurityManager *mgr, > virDomainDef *def, > int fd) > { > + int rc = 0; > char *proc = NULL; > char *fd_path = NULL; This looks like a pre-exissting memleak. both 'proc' and 'fd_path' are leaked. To fix this we now use 'g_autofree' instead of adding a 'err' or other cleanup label. Ideally fix this separately before modifying the function.

> > @@ -1121,10 +1302,21 @@ AppArmorSetFDLabel(virSecurityManager *mgr, > if (virFileResolveLink(proc, &fd_path) < 0) { > /* it's a deleted file, presumably. Ignore? */ > VIR_WARN("could not find path for descriptor %s, skipping", proc); > - return 0; > + goto err; > } > > - return reload_profile(mgr, def, fd_path, true); > + rc = reload_profile(mgr, def, fd_path, true); > + if (rc == 0) { > + if (AppArmorAppendPathToLabelList(secdef->imagelabel, fd_path) < 0) { > + virReportError(VIR_ERR_INTERNAL_ERROR, > + "%s", _("Could not store path")); > + rc = -1; > + } > + } > + g_free(fd_path); > + err: > + g_free(proc); > + return rc; > } > > static char * > -- > 2.34.1 >