Re: [PATCH v3 4/4] virt-aa-helper: store dynamically generated rules

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/runtime information is lost.

This patch stores the dynamically generated rules in a new file called libvirt-uuid.runtime_files which is included by the AppArmor policy. This file should exist while the domain is running and should be reloaded automatically whenever there's a restore operation. These rules only make sense when the VM is running, so the file is removed when the VM is shutdown.

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. Closes: https://gitlab.com/libvirt/libvirt/-/issues/692 Signed-off-by: Georgia Garcia <georgia.garcia(a)canonical.com&gt; --- src/security/security_apparmor.c | 38 +++++++++++++++++++-------- src/security/virt-aa-helper.c | 45 ++++++++++++++++++++++++++------ 2 files changed, 64 insertions(+), 19 deletions(-) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 91c51f6395..907b01577c 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -147,7 +147,8 @@ load_profile(virSecurityManager *mgr G_GNUC_UNUSED, const char *profile, virDomainDef *def, const char *fn, - bool append) + bool append, + bool runtime) { bool create = true; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; @@ -173,6 +174,8 @@ load_profile(virSecurityManager *mgr G_GNUC_UNUSED, } else { virCommandAddArgList(cmd, "-f", fn, NULL); } + if (runtime) + virCommandAddArgList(cmd, "-t", NULL); } virCommandAddEnvFormat(cmd, @@ -245,10 +248,11 @@ use_apparmor(void) * NULL. */ static int -reload_profile(virSecurityManager *mgr, - virDomainDef *def, - const char *fn, - bool append) +reload_runtime_profile(virSecurityManager *mgr, + virDomainDef *def, + const char *fn, + bool append, + bool runtime) { virSecurityLabelDef *secdef = virDomainDefGetSecurityLabelDef( def, SECURITY_APPARMOR_NAME); @@ -258,7 +262,7 @@ reload_profile(virSecurityManager *mgr, /* Update the profile only if it is loaded */ if (profile_loaded(secdef->imagelabel) >= 0) { - if (load_profile(mgr, secdef->imagelabel, def, fn, append) < 0) { + if (load_profile(mgr, secdef->imagelabel, def, fn, append, runtime) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot update AppArmor profile \'%1$s\'"), secdef->imagelabel); @@ -268,6 +272,18 @@ reload_profile(virSecurityManager *mgr, return 0; } +/* reload the profile, adding read/write file specified by fn if it is not + * NULL. + */ +static int +reload_profile(virSecurityManager *mgr, + virDomainDef *def, + const char *fn, + bool append) +{ + return reload_runtime_profile(mgr, def, fn, append, false); +} + static int AppArmorSetSecurityHostdevLabelHelper(const char *file, void *opaque) { @@ -388,7 +404,7 @@ AppArmorGenSecurityLabel(virSecurityManager *mgr G_GNUC_UNUSED, secdef->model = g_strdup(SECURITY_APPARMOR_NAME); /* Now that we have a label, load the profile into the kernel. */ - if (load_profile(mgr, secdef->label, def, NULL, false) < 0) { + if (load_profile(mgr, secdef->label, def, NULL, false, false) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, _("cannot load AppArmor profile \'%1$s\'"), secdef->label); @@ -420,7 +436,7 @@ 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); + return reload_runtime_profile(mgr, def, incomingPath, true, true); return 0; } @@ -1074,9 +1090,9 @@ AppArmorSetPathLabel(virSecurityManager *mgr, if (allowSubtree) { full_path = g_strdup_printf("%s/{,**}", path); - rc = reload_profile(mgr, def, full_path, true); + rc = reload_runtime_profile(mgr, def, full_path, true, true); } else { - rc = reload_profile(mgr, def, path, true); + rc = reload_runtime_profile(mgr, def, path, true, true); } return rc; @@ -1112,7 +1128,7 @@ AppArmorSetFDLabel(virSecurityManager *mgr, return 0; } - return reload_profile(mgr, def, fd_path, true); + return reload_runtime_profile(mgr, def, fd_path, true, true); } static char * diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 1626d5a89c..3a217fa3d1 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -71,6 +71,7 @@ typedef struct { virArch arch; /* machine architecture */ char *newfile; /* newly added file */ bool append; /* append to .files instead of rewrite */ + bool runtime; /* file should be added to .runtime_files */ } vahControl; static int @@ -110,6 +111,7 @@ vah_usage(void) " Extra File:\n" " -f | --add-file <file> add file to a profile generated from XML\n" " -F | --append-file <file> append file to an existing profile\n" + " -t | --runtime file is valid only during runtime\n" "\n"), progname); puts(_("This command is intended to be used by libvirtd and not used directly.\n")); @@ -1356,10 +1358,11 @@ vahParseArgv(vahControl * ctl, int argc, char **argv) { "replace", 0, 0, 'r' }, { "remove", 0, 0, 'R' }, { "uuid", 1, 0, 'u' }, + { "runtime", 0, 0, 't' }, { 0, 0, 0, 0 }, }; - while ((arg = getopt_long(argc, argv, "acdDhrRH:b:u:p:f:F:", opt, + while ((arg = getopt_long(argc, argv, "acdDhrRH:b:u:p:f:F:t", opt, &idx)) != -1) { switch (arg) { case 'a': @@ -1396,6 +1399,9 @@ vahParseArgv(vahControl * ctl, int argc, char **argv) PROFILE_NAME_SIZE) < 0) vah_error(ctl, 1, _("error copying UUID")); break; + case 't': + ctl->runtime = true; + break; default: vah_error(ctl, 1, _("unsupported option")); break; @@ -1445,9 +1451,16 @@ main(int argc, char **argv) int rc = -1; g_autofree char *profile = NULL; g_autofree char *include_file = NULL; + g_autofree char *include_runtime_file = NULL; off_t size; bool purged = 0; +#if defined(WITH_APPARMOR_3) + const char *ifexists = "if exists "; +#else + const char *ifexists = ""; +#endif + if (virGettextInitialize() < 0 || virErrorInitialize() < 0) { fprintf(stderr, _("%1$s: initialization failed\n"), argv[0]); @@ -1479,13 +1492,16 @@ main(int argc, char **argv) profile = g_strdup_printf("%s/%s", APPARMOR_DIR "/libvirt", ctl->uuid); include_file = g_strdup_printf("%s/%s.files", APPARMOR_DIR "/libvirt", ctl->uuid); + include_runtime_file = g_strdup_printf("%s/%s.runtime_files", APPARMOR_DIR "/libvirt", ctl->uuid); if (ctl->cmd == 'a') { rc = parserLoad(ctl->uuid); } else if (ctl->cmd == 'R' || ctl->cmd == 'D') { rc = parserRemove(ctl->uuid); - if (ctl->cmd == 'D') + if (ctl->cmd == 'D') { unlink(include_file); + unlink(include_runtime_file); + } } else if (ctl->cmd == 'c' || ctl->cmd == 'r') { g_autofree char *included_files = NULL; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; @@ -1513,6 +1529,7 @@ main(int argc, char **argv) if (vah_add_file(&buf, ctl->newfile, "rwk") != 0) goto cleanup; } else { + virBufferAsprintf(&buf, " #include %s<libvirt/%s.runtime_files>\n", ifexists, ctl->uuid); if (ctl->def->virtType == VIR_DOMAIN_VIRT_QEMU || ctl->def->virtType == VIR_DOMAIN_VIRT_KQEMU || ctl->def->virtType == VIR_DOMAIN_VIRT_KVM) { @@ -1535,11 +1552,20 @@ main(int argc, char **argv) /* (re)create the include file using included_files */ if (ctl->dryrun) { - vah_info(include_file); + if (ctl->runtime) + vah_info(include_runtime_file); + else + vah_info(include_file); vah_info(included_files); rc = 0; } else if (ctl->def->virtType == VIR_DOMAIN_VIRT_LXC) { rc = 0; + } else if (ctl->runtime) { + /* runtime should only update include_runtime_file */ + if ((rc = update_include_file(include_runtime_file, + included_files, + ctl->append)) != 0) + goto cleanup; } else if ((rc = update_include_file(include_file, included_files, ctl->append)) != 0) { @@ -1550,11 +1576,12 @@ main(int argc, char **argv) /* create the profile from TEMPLATE */ if (ctl->cmd == 'c' || purged) { g_autofree char *tmp = NULL; -#if defined(WITH_APPARMOR_3) - const char *ifexists = "if exists "; -#else - const char *ifexists = ""; -#endif + + /* ideally libvirt-uuid.files and + * libvirt-uuid.runtime_files should be in libvirt-uuid.d/ + * and the directory should be included instead, but how + * to deal with running domains when the libvirt-uuid + * profile is not recreated? */ tmp = g_strdup_printf(" #include %s<libvirt/%s.files>\n", ifexists, ctl->uuid); if (ctl->dryrun) { @@ -1566,6 +1593,7 @@ main(int argc, char **argv) ctl->def->virtType)) != 0) { vah_error(ctl, 0, _("could not create profile")); unlink(include_file); + unlink(include_runtime_file); } } @@ -1578,6 +1606,7 @@ main(int argc, char **argv) /* cleanup */ if (rc != 0) { unlink(include_file); + unlink(include_runtime_file); if (ctl->cmd == 'c') unlink(profile); }