[PATCH v2 0/4] fix AppArmor policy restore for runtime 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 patchset fixes that by storing these rules in a different file called libvirt-uuid.runtime_files, which is included by libvirt-uuid.files that already exists. It also includes other fixes like memory leaks, adoption of the GLib API in the apparmor files and a fix on the AppArmor policy that incorrectly applies apparmor policy syntax. Georgia Garcia (4): security_apparmor: fix memleaks in AppArmorSetFDLabel security: replace uses of label and VIR_FREE by g_autofree apparmor: fix UUID specification virt-aa-helper: store dynamically generated rules .../usr.lib.libvirt.virt-aa-helper.in | 5 +- src/security/apparmor/usr.sbin.libvirtd.in | 7 +- src/security/security_apparmor.c | 83 +++++----- src/security/virt-aa-helper.c | 145 +++++++++--------- 4 files changed, 120 insertions(+), 120 deletions(-) -- 2.34.1
proc and fd_path are allocated but never freed. Fix by using g_autofree instead. Fixes: b9757fea30785a92aa95ea675b9bc371e4fb2e8c Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com> --- src/security/security_apparmor.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 07e95ec81d..7092724563 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -1107,8 +1107,8 @@ AppArmorSetFDLabel(virSecurityManager *mgr, virDomainDef *def, int fd) { - char *proc = NULL; - char *fd_path = NULL; + g_autofree char *proc = NULL; + g_autofree char *fd_path = NULL; virSecurityLabelDef *secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME); -- 2.34.1
On 11/13/24 07:28, Georgia Garcia wrote:
proc and fd_path are allocated but never freed. Fix by using g_autofree instead.
Fixes: b9757fea30785a92aa95ea675b9bc371e4fb2e8c Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com> --- src/security/security_apparmor.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Jim Fehlig <jfehlig@suse.com> Regards, Jim
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 07e95ec81d..7092724563 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -1107,8 +1107,8 @@ AppArmorSetFDLabel(virSecurityManager *mgr, virDomainDef *def, int fd) { - char *proc = NULL; - char *fd_path = NULL; + g_autofree char *proc = NULL; + g_autofree char *fd_path = NULL;
virSecurityLabelDef *secdef = virDomainDefGetSecurityLabelDef(def, SECURITY_APPARMOR_NAME);
Moving towards full adoption of GLib APIs in the AppArmor code. Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com> --- src/security/security_apparmor.c | 41 ++++--------- src/security/virt-aa-helper.c | 100 ++++++++++--------------------- 2 files changed, 45 insertions(+), 96 deletions(-) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 7092724563..9e578b2526 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -115,37 +115,28 @@ profile_loaded(const char *str) static int profile_status_file(const char *str) { - char *profile = NULL; - char *content = NULL; - char *tmp = NULL; - int rc = -1; + g_autofree char *profile = NULL; + g_autofree char *content = NULL; + g_autofree char *tmp = NULL; int len; profile = g_strdup_printf("%s/%s", APPARMOR_DIR "/libvirt", str); if (!virFileExists(profile)) - goto failed; + return -1; if ((len = virFileReadAll(profile, MAX_FILE_LEN, &content)) < 0) { virReportSystemError(errno, _("Failed to read \'%1$s\'"), profile); - goto failed; + return -1; } /* create string that is ' <str> flags=(complain)\0' */ tmp = g_strdup_printf(" %s flags=(complain)", str); if (strstr(content, tmp) != NULL) - rc = 0; - else - rc = 1; - - failed: - VIR_FREE(tmp); - VIR_FREE(profile); - VIR_FREE(content); - - return rc; + return 0; + return 1; } /* @@ -218,7 +209,7 @@ static int use_apparmor(void) { int rc = -1; - char *libvirt_daemon = NULL; + g_autofree char *libvirt_daemon = NULL; if (virFileResolveLink("/proc/self/exe", &libvirt_daemon) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -232,7 +223,7 @@ use_apparmor(void) return 1; if (access(APPARMOR_PROFILES_PATH, R_OK) != 0) - goto cleanup; + return rc; /* First check profile status using full binary path. If that fails * check using profile name. @@ -245,8 +236,6 @@ use_apparmor(void) rc = -1; } - cleanup: - VIR_FREE(libvirt_daemon); return rc; } @@ -948,7 +937,7 @@ AppArmorSetChardevLabel(virSecurityManager *mgr, virDomainChrSourceDef *dev_source, bool chardevStdioLogd G_GNUC_UNUSED) { - char *in = NULL, *out = NULL; + g_autofree char *in = NULL, *out = NULL; int ret = -1; virSecurityLabelDef *secdef; @@ -969,11 +958,11 @@ AppArmorSetChardevLabel(virSecurityManager *mgr, out = g_strdup_printf("%s.out", dev_source->data.file.path); if (virFileExists(in)) { if (reload_profile(mgr, def, in, true) < 0) - goto done; + return ret; } if (virFileExists(out)) { if (reload_profile(mgr, def, out, true) < 0) - goto done; + return ret; } ret = reload_profile(mgr, def, dev_source->data.file.path, true); break; @@ -993,9 +982,6 @@ AppArmorSetChardevLabel(virSecurityManager *mgr, break; } - done: - VIR_FREE(in); - VIR_FREE(out); return ret; } @@ -1081,12 +1067,11 @@ AppArmorSetPathLabel(virSecurityManager *mgr, bool allowSubtree) { int rc = -1; - char *full_path = NULL; + g_autofree char *full_path = NULL; 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); } diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 067a17f331..601f2d2581 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -146,9 +146,8 @@ vah_info(const char *str) static int parserCommand(const char *profile_name, const char cmd) { - int result = -1; char flag[3]; - char *profile; + g_autofree char *profile = NULL; int status; int ret; @@ -163,7 +162,7 @@ parserCommand(const char *profile_name, const char cmd) if (!virFileExists(profile)) { vah_error(NULL, 0, _("profile does not exist")); - goto cleanup; + return -1; } else { const char * const argv[] = { "/sbin/apparmor_parser", flag, profile, NULL @@ -175,23 +174,18 @@ parserCommand(const char *profile_name, const char cmd) (WIFEXITED(status) && WEXITSTATUS(status) != 0)) { if (ret != 0) { vah_error(NULL, 0, _("failed to run apparmor_parser")); - goto cleanup; + return -1; } else if (cmd == 'R' && WIFEXITED(status) && WEXITSTATUS(status) == 234) { vah_warning(_("unable to unload already unloaded profile")); } else { vah_error(NULL, 0, _("apparmor_parser exited with error")); - goto cleanup; + return -1; } } } - result = 0; - - cleanup: - VIR_FREE(profile); - - return result; + return 0; } /* @@ -201,18 +195,17 @@ static int update_include_file(const char *include_file, const char *included_files, bool append) { - int rc = -1; int plen, flen = 0; int fd; - char *pcontent = NULL; - char *existing = NULL; + g_autofree char *pcontent = NULL; + g_autofree char *existing = NULL; const char *warning = "# DO NOT EDIT THIS FILE DIRECTLY. IT IS MANAGED BY LIBVIRT.\n"; if (virFileExists(include_file)) { flen = virFileReadAll(include_file, MAX_FILE_LEN, &existing); if (flen < 0) - return rc; + return -1; } if (append && virFileExists(include_file)) @@ -223,38 +216,31 @@ update_include_file(const char *include_file, const char *included_files, plen = strlen(pcontent); if (plen > MAX_FILE_LEN) { vah_error(NULL, 0, _("invalid length for new profile")); - goto cleanup; + return -1; } /* only update the disk profile if it is different */ if (flen > 0 && flen == plen && STREQLEN(existing, pcontent, plen)) { - rc = 0; - goto cleanup; + return 0; } /* write the file */ if ((fd = open(include_file, O_CREAT | O_TRUNC | O_WRONLY, 0644)) == -1) { vah_error(NULL, 0, _("failed to create include file")); - goto cleanup; + return -1; } if (safewrite(fd, pcontent, plen) < 0) { /* don't write the '\0' */ VIR_FORCE_CLOSE(fd); vah_error(NULL, 0, _("failed to write to profile")); - goto cleanup; + return -1; } if (VIR_CLOSE(fd) != 0) { vah_error(NULL, 0, _("failed to close or write to profile")); - goto cleanup; + return -1; } - rc = 0; - - cleanup: - VIR_FREE(pcontent); - VIR_FREE(existing); - - return rc; + return 0; } /* @@ -572,7 +558,7 @@ caps_mockup(vahControl * ctl, const char *xmlStr) { g_autoptr(xmlDoc) xml = NULL; g_autoptr(xmlXPathContext) ctxt = NULL; - char *arch; + g_autofree char *arch = NULL; if (!(xml = virXMLParse(NULL, xmlStr, _("(domain_definition)"), "domain", &ctxt, NULL, false))) { @@ -598,7 +584,6 @@ caps_mockup(vahControl * ctl, const char *xmlStr) ctl->arch = virArchFromHost(); } else { ctl->arch = virArchFromString(arch); - VIR_FREE(arch); } return 0; @@ -683,15 +668,15 @@ get_definition(vahControl * ctl, const char *xmlStr) static int vah_add_path(virBuffer *buf, const char *path, const char *perms, bool recursive) { - char *tmp = NULL; int rc = -1; bool readonly = true; bool explicit_deny_rule = true; char *sub = NULL; - char *perms_new = NULL; - char *pathdir = NULL; - char *pathtmp = NULL; - char *pathreal = NULL; + g_autofree char *tmp = NULL; + g_autofree char *perms_new = NULL; + g_autofree char *pathdir = NULL; + g_autofree char *pathtmp = NULL; + g_autofree char *pathreal = NULL; if (path == NULL) return rc; @@ -728,7 +713,7 @@ vah_add_path(virBuffer *buf, const char *path, const char *perms, bool recursive if ((pathreal = realpath(pathdir, NULL)) == NULL) { vah_error(NULL, 0, pathdir); vah_error(NULL, 0, _("could not find realpath")); - goto cleanup; + return rc; } tmp = g_strdup_printf("%s%s", pathreal, pathtmp); } @@ -752,7 +737,7 @@ vah_add_path(virBuffer *buf, const char *path, const char *perms, bool recursive vah_error(NULL, 0, path); vah_error(NULL, 0, _("skipped restricted file")); } - goto cleanup; + return rc; } if (tmp[strlen(tmp) - 1] == '/') @@ -769,13 +754,6 @@ vah_add_path(virBuffer *buf, const char *path, const char *perms, bool recursive virBufferAsprintf(buf, " \"%s/\" r,\n", tmp); } - cleanup: - VIR_FREE(pathdir); - VIR_FREE(pathtmp); - VIR_FREE(pathreal); - VIR_FREE(perms_new); - VIR_FREE(tmp); - return rc; } @@ -791,36 +769,28 @@ vah_add_file_chardev(virBuffer *buf, const char *perms, const int type) { - char *pipe_in; - char *pipe_out; - int rc = -1; + g_autofree char *pipe_in = NULL; + g_autofree char *pipe_out = NULL; if (type == VIR_DOMAIN_CHR_TYPE_PIPE) { /* add the pipe input */ pipe_in = g_strdup_printf("%s.in", path); if (vah_add_file(buf, pipe_in, perms) != 0) - goto clean_pipe_in; + return -1; /* add the pipe output */ pipe_out = g_strdup_printf("%s.out", path); if (vah_add_file(buf, pipe_out, perms) != 0) - goto clean_pipe_out; - - rc = 0; - clean_pipe_out: - VIR_FREE(pipe_out); - clean_pipe_in: - VIR_FREE(pipe_in); + return -1; } else { /* add the file */ if (vah_add_file(buf, path, perms) != 0) return -1; - rc = 0; } - return rc; + return 0; } static int @@ -1467,8 +1437,8 @@ main(int argc, char **argv) vahControl _ctl = { 0 }; vahControl *ctl = &_ctl; int rc = -1; - char *profile = NULL; - char *include_file = NULL; + g_autofree char *profile = NULL; + g_autofree char *include_file = NULL; off_t size; bool purged = 0; @@ -1511,7 +1481,7 @@ main(int argc, char **argv) if (ctl->cmd == 'D') unlink(include_file); } else if (ctl->cmd == 'c' || ctl->cmd == 'r') { - char *included_files = NULL; + g_autofree char *included_files = NULL; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER; if (ctl->cmd == 'c' && virFileExists(profile)) @@ -1573,7 +1543,7 @@ main(int argc, char **argv) /* create the profile from TEMPLATE */ if (ctl->cmd == 'c' || purged) { - char *tmp = NULL; + g_autofree char *tmp = NULL; #if defined(WITH_APPARMOR_3) const char *ifexists = "if exists "; #else @@ -1591,7 +1561,6 @@ main(int argc, char **argv) vah_error(ctl, 0, _("could not create profile")); unlink(include_file); } - VIR_FREE(tmp); } if (rc == 0 && !ctl->dryrun) { @@ -1607,14 +1576,9 @@ main(int argc, char **argv) unlink(profile); } } - cleanup: - VIR_FREE(included_files); } - + cleanup: vahDeinit(ctl); - VIR_FREE(profile); - VIR_FREE(include_file); - exit(rc == 0 ? EXIT_SUCCESS : EXIT_FAILURE); } -- 2.34.1
On 11/13/24 07:28, Georgia Garcia wrote:
Moving towards full adoption of GLib APIs in the AppArmor code.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com> --- src/security/security_apparmor.c | 41 ++++--------- src/security/virt-aa-helper.c | 100 ++++++++++--------------------- 2 files changed, 45 insertions(+), 96 deletions(-)
diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index 7092724563..9e578b2526 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -115,37 +115,28 @@ profile_loaded(const char *str) static int profile_status_file(const char *str) { - char *profile = NULL; - char *content = NULL; - char *tmp = NULL; - int rc = -1; + g_autofree char *profile = NULL; + g_autofree char *content = NULL; + g_autofree char *tmp = NULL; int len;
profile = g_strdup_printf("%s/%s", APPARMOR_DIR "/libvirt", str);
if (!virFileExists(profile)) - goto failed; + return -1;
if ((len = virFileReadAll(profile, MAX_FILE_LEN, &content)) < 0) { virReportSystemError(errno, _("Failed to read \'%1$s\'"), profile); - goto failed; + return -1; }
/* create string that is ' <str> flags=(complain)\0' */ tmp = g_strdup_printf(" %s flags=(complain)", str);
if (strstr(content, tmp) != NULL) - rc = 0; - else - rc = 1; - - failed: - VIR_FREE(tmp); - VIR_FREE(profile); - VIR_FREE(content); - - return rc; + return 0; + return 1; }
/* @@ -218,7 +209,7 @@ static int use_apparmor(void) { int rc = -1; - char *libvirt_daemon = NULL; + g_autofree char *libvirt_daemon = NULL;
if (virFileResolveLink("/proc/self/exe", &libvirt_daemon) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -232,7 +223,7 @@ use_apparmor(void) return 1;
if (access(APPARMOR_PROFILES_PATH, R_OK) != 0) - goto cleanup; + return rc;
/* First check profile status using full binary path. If that fails * check using profile name. @@ -245,8 +236,6 @@ use_apparmor(void) rc = -1; }
- cleanup: - VIR_FREE(libvirt_daemon); return rc; }
@@ -948,7 +937,7 @@ AppArmorSetChardevLabel(virSecurityManager *mgr, virDomainChrSourceDef *dev_source, bool chardevStdioLogd G_GNUC_UNUSED) { - char *in = NULL, *out = NULL; + g_autofree char *in = NULL, *out = NULL;
While touching this code, we can follow the preference* of one variable per line. I can make that small tweak before pushing. Otherwise, the patch looks good! Reviewed-by: Jim Fehlig <jfehlig@suse.com> Regards, Jim * I _think_ that's the project preference, and vaguely recall others mentioning it in the past :-).
int ret = -1; virSecurityLabelDef *secdef;
@@ -969,11 +958,11 @@ AppArmorSetChardevLabel(virSecurityManager *mgr, out = g_strdup_printf("%s.out", dev_source->data.file.path); if (virFileExists(in)) { if (reload_profile(mgr, def, in, true) < 0) - goto done; + return ret; } if (virFileExists(out)) { if (reload_profile(mgr, def, out, true) < 0) - goto done; + return ret; } ret = reload_profile(mgr, def, dev_source->data.file.path, true); break; @@ -993,9 +982,6 @@ AppArmorSetChardevLabel(virSecurityManager *mgr, break; }
- done: - VIR_FREE(in); - VIR_FREE(out); return ret; }
@@ -1081,12 +1067,11 @@ AppArmorSetPathLabel(virSecurityManager *mgr, bool allowSubtree) { int rc = -1; - char *full_path = NULL; + g_autofree char *full_path = NULL;
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); } diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 067a17f331..601f2d2581 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -146,9 +146,8 @@ vah_info(const char *str) static int parserCommand(const char *profile_name, const char cmd) { - int result = -1; char flag[3]; - char *profile; + g_autofree char *profile = NULL; int status; int ret;
@@ -163,7 +162,7 @@ parserCommand(const char *profile_name, const char cmd)
if (!virFileExists(profile)) { vah_error(NULL, 0, _("profile does not exist")); - goto cleanup; + return -1; } else { const char * const argv[] = { "/sbin/apparmor_parser", flag, profile, NULL @@ -175,23 +174,18 @@ parserCommand(const char *profile_name, const char cmd) (WIFEXITED(status) && WEXITSTATUS(status) != 0)) { if (ret != 0) { vah_error(NULL, 0, _("failed to run apparmor_parser")); - goto cleanup; + return -1; } else if (cmd == 'R' && WIFEXITED(status) && WEXITSTATUS(status) == 234) { vah_warning(_("unable to unload already unloaded profile")); } else { vah_error(NULL, 0, _("apparmor_parser exited with error")); - goto cleanup; + return -1; } } }
- result = 0; - - cleanup: - VIR_FREE(profile); - - return result; + return 0; }
/* @@ -201,18 +195,17 @@ static int update_include_file(const char *include_file, const char *included_files, bool append) { - int rc = -1; int plen, flen = 0; int fd; - char *pcontent = NULL; - char *existing = NULL; + g_autofree char *pcontent = NULL; + g_autofree char *existing = NULL; const char *warning = "# DO NOT EDIT THIS FILE DIRECTLY. IT IS MANAGED BY LIBVIRT.\n";
if (virFileExists(include_file)) { flen = virFileReadAll(include_file, MAX_FILE_LEN, &existing); if (flen < 0) - return rc; + return -1; }
if (append && virFileExists(include_file)) @@ -223,38 +216,31 @@ update_include_file(const char *include_file, const char *included_files, plen = strlen(pcontent); if (plen > MAX_FILE_LEN) { vah_error(NULL, 0, _("invalid length for new profile")); - goto cleanup; + return -1; }
/* only update the disk profile if it is different */ if (flen > 0 && flen == plen && STREQLEN(existing, pcontent, plen)) { - rc = 0; - goto cleanup; + return 0; }
/* write the file */ if ((fd = open(include_file, O_CREAT | O_TRUNC | O_WRONLY, 0644)) == -1) { vah_error(NULL, 0, _("failed to create include file")); - goto cleanup; + return -1; }
if (safewrite(fd, pcontent, plen) < 0) { /* don't write the '\0' */ VIR_FORCE_CLOSE(fd); vah_error(NULL, 0, _("failed to write to profile")); - goto cleanup; + return -1; }
if (VIR_CLOSE(fd) != 0) { vah_error(NULL, 0, _("failed to close or write to profile")); - goto cleanup; + return -1; } - rc = 0; - - cleanup: - VIR_FREE(pcontent); - VIR_FREE(existing); - - return rc; + return 0; }
/* @@ -572,7 +558,7 @@ caps_mockup(vahControl * ctl, const char *xmlStr) { g_autoptr(xmlDoc) xml = NULL; g_autoptr(xmlXPathContext) ctxt = NULL; - char *arch; + g_autofree char *arch = NULL;
if (!(xml = virXMLParse(NULL, xmlStr, _("(domain_definition)"), "domain", &ctxt, NULL, false))) { @@ -598,7 +584,6 @@ caps_mockup(vahControl * ctl, const char *xmlStr) ctl->arch = virArchFromHost(); } else { ctl->arch = virArchFromString(arch); - VIR_FREE(arch); }
return 0; @@ -683,15 +668,15 @@ get_definition(vahControl * ctl, const char *xmlStr) static int vah_add_path(virBuffer *buf, const char *path, const char *perms, bool recursive) { - char *tmp = NULL; int rc = -1; bool readonly = true; bool explicit_deny_rule = true; char *sub = NULL; - char *perms_new = NULL; - char *pathdir = NULL; - char *pathtmp = NULL; - char *pathreal = NULL; + g_autofree char *tmp = NULL; + g_autofree char *perms_new = NULL; + g_autofree char *pathdir = NULL; + g_autofree char *pathtmp = NULL; + g_autofree char *pathreal = NULL;
if (path == NULL) return rc; @@ -728,7 +713,7 @@ vah_add_path(virBuffer *buf, const char *path, const char *perms, bool recursive if ((pathreal = realpath(pathdir, NULL)) == NULL) { vah_error(NULL, 0, pathdir); vah_error(NULL, 0, _("could not find realpath")); - goto cleanup; + return rc; } tmp = g_strdup_printf("%s%s", pathreal, pathtmp); } @@ -752,7 +737,7 @@ vah_add_path(virBuffer *buf, const char *path, const char *perms, bool recursive vah_error(NULL, 0, path); vah_error(NULL, 0, _("skipped restricted file")); } - goto cleanup; + return rc; }
if (tmp[strlen(tmp) - 1] == '/') @@ -769,13 +754,6 @@ vah_add_path(virBuffer *buf, const char *path, const char *perms, bool recursive virBufferAsprintf(buf, " \"%s/\" r,\n", tmp); }
- cleanup: - VIR_FREE(pathdir); - VIR_FREE(pathtmp); - VIR_FREE(pathreal); - VIR_FREE(perms_new); - VIR_FREE(tmp); - return rc; }
@@ -791,36 +769,28 @@ vah_add_file_chardev(virBuffer *buf, const char *perms, const int type) { - char *pipe_in; - char *pipe_out; - int rc = -1; + g_autofree char *pipe_in = NULL; + g_autofree char *pipe_out = NULL;
if (type == VIR_DOMAIN_CHR_TYPE_PIPE) { /* add the pipe input */ pipe_in = g_strdup_printf("%s.in", path);
if (vah_add_file(buf, pipe_in, perms) != 0) - goto clean_pipe_in; + return -1;
/* add the pipe output */ pipe_out = g_strdup_printf("%s.out", path);
if (vah_add_file(buf, pipe_out, perms) != 0) - goto clean_pipe_out; - - rc = 0; - clean_pipe_out: - VIR_FREE(pipe_out); - clean_pipe_in: - VIR_FREE(pipe_in); + return -1; } else { /* add the file */ if (vah_add_file(buf, path, perms) != 0) return -1; - rc = 0; }
- return rc; + return 0; }
static int @@ -1467,8 +1437,8 @@ main(int argc, char **argv) vahControl _ctl = { 0 }; vahControl *ctl = &_ctl; int rc = -1; - char *profile = NULL; - char *include_file = NULL; + g_autofree char *profile = NULL; + g_autofree char *include_file = NULL; off_t size; bool purged = 0;
@@ -1511,7 +1481,7 @@ main(int argc, char **argv) if (ctl->cmd == 'D') unlink(include_file); } else if (ctl->cmd == 'c' || ctl->cmd == 'r') { - char *included_files = NULL; + g_autofree char *included_files = NULL; g_auto(virBuffer) buf = VIR_BUFFER_INITIALIZER;
if (ctl->cmd == 'c' && virFileExists(profile)) @@ -1573,7 +1543,7 @@ main(int argc, char **argv)
/* create the profile from TEMPLATE */ if (ctl->cmd == 'c' || purged) { - char *tmp = NULL; + g_autofree char *tmp = NULL; #if defined(WITH_APPARMOR_3) const char *ifexists = "if exists "; #else @@ -1591,7 +1561,6 @@ main(int argc, char **argv) vah_error(ctl, 0, _("could not create profile")); unlink(include_file); } - VIR_FREE(tmp); }
if (rc == 0 && !ctl->dryrun) { @@ -1607,14 +1576,9 @@ main(int argc, char **argv) unlink(profile); } } - cleanup: - VIR_FREE(included_files); } - + cleanup: vahDeinit(ctl);
- VIR_FREE(profile); - VIR_FREE(include_file); - exit(rc == 0 ? EXIT_SUCCESS : EXIT_FAILURE); }
On 1/7/25 01:56, Jim Fehlig via Devel wrote:
On 11/13/24 07:28, Georgia Garcia wrote:
Moving towards full adoption of GLib APIs in the AppArmor code.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com> --- src/security/security_apparmor.c | 41 ++++--------- src/security/virt-aa-helper.c | 100 ++++++++++--------------------- 2 files changed, 45 insertions(+), 96 deletions(-)
diff --git a/src/security/security_apparmor.c b/src/security/ security_apparmor.c index 7092724563..9e578b2526 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -115,37 +115,28 @@ profile_loaded(const char *str) static int profile_status_file(const char *str) { - char *profile = NULL; - char *content = NULL; - char *tmp = NULL; - int rc = -1; + g_autofree char *profile = NULL; + g_autofree char *content = NULL; + g_autofree char *tmp = NULL; int len; profile = g_strdup_printf("%s/%s", APPARMOR_DIR "/libvirt", str); if (!virFileExists(profile)) - goto failed; + return -1; if ((len = virFileReadAll(profile, MAX_FILE_LEN, &content)) < 0) { virReportSystemError(errno, _("Failed to read \'%1$s\'"), profile); - goto failed; + return -1; } /* create string that is ' <str> flags=(complain)\0' */ tmp = g_strdup_printf(" %s flags=(complain)", str); if (strstr(content, tmp) != NULL) - rc = 0; - else - rc = 1; - - failed: - VIR_FREE(tmp); - VIR_FREE(profile); - VIR_FREE(content); - - return rc; + return 0; + return 1; } /* @@ -218,7 +209,7 @@ static int use_apparmor(void) { int rc = -1; - char *libvirt_daemon = NULL; + g_autofree char *libvirt_daemon = NULL; if (virFileResolveLink("/proc/self/exe", &libvirt_daemon) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, @@ -232,7 +223,7 @@ use_apparmor(void) return 1; if (access(APPARMOR_PROFILES_PATH, R_OK) != 0) - goto cleanup; + return rc; /* First check profile status using full binary path. If that fails * check using profile name. @@ -245,8 +236,6 @@ use_apparmor(void) rc = -1; } - cleanup: - VIR_FREE(libvirt_daemon); return rc; } @@ -948,7 +937,7 @@ AppArmorSetChardevLabel(virSecurityManager *mgr, virDomainChrSourceDef *dev_source, bool chardevStdioLogd G_GNUC_UNUSED) { - char *in = NULL, *out = NULL; + g_autofree char *in = NULL, *out = NULL;
While touching this code, we can follow the preference* of one variable per line. I can make that small tweak before pushing.
Otherwise, the patch looks good!
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
Regards, Jim
* I _think_ that's the project preference, and vaguely recall others mentioning it in the past :-).
It is, because it leads to smaller diffs in the future. Please do make the change before pushing these. Michal
There is a common misconception when writing AppArmor policy that [0-9]* applies * to the [0-9] class, but that's not the case. For this example, [0-9]* matches a single digit followed by any number of characters except for / Create a UUID variable that uses the following format 8-4-4-4-12. Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com> --- src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in | 5 ++++- src/security/apparmor/usr.sbin.libvirtd.in | 7 +++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in index 44645c6989..90a8b7072c 100644 --- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in +++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in @@ -1,5 +1,8 @@ #include <tunables/global> +@{hextet}=[0-9a-f][0-9a-f][0-9a-f][0-9a-f] +@{UUID}=@{hextet}@{hextet}-@{hextet}-@{hextet}-@{hextet}-@{hextet}@{hextet}@{hextet} + profile virt-aa-helper @libexecdir@/virt-aa-helper { #include <abstractions/base> #include <abstractions/openssl> @@ -44,7 +47,7 @@ profile virt-aa-helper @libexecdir@/virt-aa-helper { /{usr/,}{s,}bin/apparmor_parser Ux, @sysconfdir@/apparmor.d/libvirt/* r, - @sysconfdir@/apparmor.d/libvirt/libvirt-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]* rw, + @sysconfdir@/apparmor.d/libvirt/libvirt-@{UUID}* rw, # for backingstore -- allow access to non-hidden files in @{HOME} as well # as storage pools diff --git a/src/security/apparmor/usr.sbin.libvirtd.in b/src/security/apparmor/usr.sbin.libvirtd.in index 70e586895f..3659ddc219 100644 --- a/src/security/apparmor/usr.sbin.libvirtd.in +++ b/src/security/apparmor/usr.sbin.libvirtd.in @@ -1,4 +1,7 @@ #include <tunables/global> + +@{hextet}=[0-9a-f][0-9a-f][0-9a-f][0-9a-f] +@{UUID}=@{hextet}@{hextet}-@{hextet}-@{hextet}-@{hextet}-@{hextet}@{hextet}@{hextet} @{LIBVIRT}="libvirt" profile libvirtd @sbindir@/libvirtd flags=(attach_disconnected) { @@ -72,7 +75,7 @@ profile libvirtd @sbindir@/libvirtd flags=(attach_disconnected) { signal (send) set=("term") peer=libvirtd//qemu_bridge_helper, # allow connect with openGraphicsFD, direction reversed in newer versions - unix (send, receive) type=stream addr=none peer=(label=libvirt-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*), + unix (send, receive) type=stream addr=none peer=(label=libvirt-@{UUID}), # unconfined also required if guests run without security module unix (send, receive) type=stream addr=none peer=(label=unconfined), @@ -115,7 +118,7 @@ profile libvirtd @sbindir@/libvirtd flags=(attach_disconnected) { /etc/xen/scripts/** rmix, # allow changing to our UUID-based named profiles - change_profile -> @{LIBVIRT}-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*, + change_profile -> @{LIBVIRT}-@{UUID}, /usr/{lib,lib64,lib/qemu,libexec,libexec/qemu}/qemu-bridge-helper Cx -> qemu_bridge_helper, # child profile for bridge helper process -- 2.34.1
On 11/13/24 07:28, Georgia Garcia wrote:
There is a common misconception when writing AppArmor policy that [0-9]* applies * to the [0-9] class, but that's not the case. For this example, [0-9]* matches a single digit followed by any number of characters except for /
Create a UUID variable that uses the following format 8-4-4-4-12.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com> --- src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in | 5 ++++- src/security/apparmor/usr.sbin.libvirtd.in | 7 +++++-- 2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in index 44645c6989..90a8b7072c 100644 --- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in +++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in @@ -1,5 +1,8 @@ #include <tunables/global>
+@{hextet}=[0-9a-f][0-9a-f][0-9a-f][0-9a-f] +@{UUID}=@{hextet}@{hextet}-@{hextet}-@{hextet}-@{hextet}-@{hextet}@{hextet}@{hextet} + profile virt-aa-helper @libexecdir@/virt-aa-helper { #include <abstractions/base> #include <abstractions/openssl> @@ -44,7 +47,7 @@ profile virt-aa-helper @libexecdir@/virt-aa-helper { /{usr/,}{s,}bin/apparmor_parser Ux,
@sysconfdir@/apparmor.d/libvirt/* r, - @sysconfdir@/apparmor.d/libvirt/libvirt-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]* rw, + @sysconfdir@/apparmor.d/libvirt/libvirt-@{UUID}* rw,
# for backingstore -- allow access to non-hidden files in @{HOME} as well # as storage pools diff --git a/src/security/apparmor/usr.sbin.libvirtd.in b/src/security/apparmor/usr.sbin.libvirtd.in index 70e586895f..3659ddc219 100644 --- a/src/security/apparmor/usr.sbin.libvirtd.in +++ b/src/security/apparmor/usr.sbin.libvirtd.in
The changes here are also needed in usr.sbin.virtqemud.in. Regards, Jim
@@ -1,4 +1,7 @@ #include <tunables/global> + +@{hextet}=[0-9a-f][0-9a-f][0-9a-f][0-9a-f] +@{UUID}=@{hextet}@{hextet}-@{hextet}-@{hextet}-@{hextet}-@{hextet}@{hextet}@{hextet} @{LIBVIRT}="libvirt"
profile libvirtd @sbindir@/libvirtd flags=(attach_disconnected) { @@ -72,7 +75,7 @@ profile libvirtd @sbindir@/libvirtd flags=(attach_disconnected) { signal (send) set=("term") peer=libvirtd//qemu_bridge_helper,
# allow connect with openGraphicsFD, direction reversed in newer versions - unix (send, receive) type=stream addr=none peer=(label=libvirt-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*), + unix (send, receive) type=stream addr=none peer=(label=libvirt-@{UUID}), # unconfined also required if guests run without security module unix (send, receive) type=stream addr=none peer=(label=unconfined),
@@ -115,7 +118,7 @@ profile libvirtd @sbindir@/libvirtd flags=(attach_disconnected) { /etc/xen/scripts/** rmix,
# allow changing to our UUID-based named profiles - change_profile -> @{LIBVIRT}-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*, + change_profile -> @{LIBVIRT}-@{UUID},
/usr/{lib,lib64,lib/qemu,libexec,libexec/qemu}/qemu-bridge-helper Cx -> qemu_bridge_helper, # child profile for bridge helper process
On Mon, 2025-01-06 at 17:59 -0700, Jim Fehlig wrote:
On 11/13/24 07:28, Georgia Garcia wrote:
There is a common misconception when writing AppArmor policy that [0-9]* applies * to the [0-9] class, but that's not the case. For this example, [0-9]* matches a single digit followed by any number of characters except for /
Create a UUID variable that uses the following format 8-4-4-4-12.
Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com> --- src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in | 5 ++++- src/security/apparmor/usr.sbin.libvirtd.in | 7 +++++-- 2 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in index 44645c6989..90a8b7072c 100644 --- a/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in +++ b/src/security/apparmor/usr.lib.libvirt.virt-aa-helper.in @@ -1,5 +1,8 @@ #include <tunables/global>
+@{hextet}=[0-9a-f][0-9a-f][0-9a-f][0-9a-f] +@{UUID}=@{hextet}@{hextet}-@{hextet}-@{hextet}-@{hextet}-@{hextet}@{hextet}@{hextet} + profile virt-aa-helper @libexecdir@/virt-aa-helper { #include <abstractions/base> #include <abstractions/openssl> @@ -44,7 +47,7 @@ profile virt-aa-helper @libexecdir@/virt-aa-helper { /{usr/,}{s,}bin/apparmor_parser Ux,
@sysconfdir@/apparmor.d/libvirt/* r, - @sysconfdir@/apparmor.d/libvirt/libvirt-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]* rw, + @sysconfdir@/apparmor.d/libvirt/libvirt-@{UUID}* rw,
# for backingstore -- allow access to non-hidden files in @{HOME} as well # as storage pools diff --git a/src/security/apparmor/usr.sbin.libvirtd.in b/src/security/apparmor/usr.sbin.libvirtd.in index 70e586895f..3659ddc219 100644 --- a/src/security/apparmor/usr.sbin.libvirtd.in +++ b/src/security/apparmor/usr.sbin.libvirtd.in
The changes here are also needed in usr.sbin.virtqemud.in.
Thanks for catching that!
Regards, Jim
@@ -1,4 +1,7 @@ #include <tunables/global> + +@{hextet}=[0-9a-f][0-9a-f][0-9a-f][0-9a-f] +@{UUID}=@{hextet}@{hextet}-@{hextet}-@{hextet}-@{hextet}-@{hextet}@{hextet}@{hextet} @{LIBVIRT}="libvirt"
profile libvirtd @sbindir@/libvirtd flags=(attach_disconnected) { @@ -72,7 +75,7 @@ profile libvirtd @sbindir@/libvirtd flags=(attach_disconnected) { signal (send) set=("term") peer=libvirtd//qemu_bridge_helper,
# allow connect with openGraphicsFD, direction reversed in newer versions - unix (send, receive) type=stream addr=none peer=(label=libvirt-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*), + unix (send, receive) type=stream addr=none peer=(label=libvirt-@{UUID}), # unconfined also required if guests run without security module unix (send, receive) type=stream addr=none peer=(label=unconfined),
@@ -115,7 +118,7 @@ profile libvirtd @sbindir@/libvirtd flags=(attach_disconnected) { /etc/xen/scripts/** rmix,
# allow changing to our UUID-based named profiles - change_profile -> @{LIBVIRT}-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*-[0-9a-f]*, + change_profile -> @{LIBVIRT}-@{UUID},
/usr/{lib,lib64,lib/qemu,libexec,libexec/qemu}/qemu-bridge-helper Cx -> qemu_bridge_helper, # child profile for bridge helper process
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@canonical.com> --- 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 9e578b2526..28f263ddd4 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, @@ -243,10 +246,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); @@ -256,7 +260,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); @@ -266,6 +270,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) { @@ -386,7 +402,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); @@ -418,7 +434,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; } @@ -1071,9 +1087,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; @@ -1109,7 +1125,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 601f2d2581..98cf9411ea 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")); @@ -1350,10 +1352,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': @@ -1390,6 +1393,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; @@ -1439,9 +1445,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]); @@ -1473,13 +1486,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; @@ -1507,6 +1523,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) { @@ -1529,11 +1546,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) { @@ -1544,11 +1570,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) { @@ -1560,6 +1587,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); } } @@ -1572,6 +1600,7 @@ main(int argc, char **argv) /* cleanup */ if (rc != 0) { unlink(include_file); + unlink(include_runtime_file); if (ctl->cmd == 'c') unlink(profile); } -- 2.34.1
On Wed, 2024-11-13 at 11:28 -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/runtime information is lost.
This patchset fixes that by storing these rules in a different file called libvirt-uuid.runtime_files, which is included by libvirt-uuid.files that already exists. It also includes other fixes like memory leaks, adoption of the GLib API in the apparmor files and a fix on the AppArmor policy that incorrectly applies apparmor policy syntax.
Georgia Garcia (4): security_apparmor: fix memleaks in AppArmorSetFDLabel security: replace uses of label and VIR_FREE by g_autofree apparmor: fix UUID specification virt-aa-helper: store dynamically generated rules
.../usr.lib.libvirt.virt-aa-helper.in | 5 +- src/security/apparmor/usr.sbin.libvirtd.in | 7 +- src/security/security_apparmor.c | 83 +++++----- src/security/virt-aa-helper.c | 145 +++++++++--------- 4 files changed, 120 insertions(+), 120 deletions(-)
Friendly ping
I backported this to Noble and Jammy libvirt ubuntu packages and confirm that this fixes the issue. Is there anything we can do to get this merged?
participants (4)
-
Georgia Garcia -
Jim Fehlig -
Michal Prívozník -
tiago.pasqualini＠canonical.com