[PATCH v3 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/apparmor/usr.sbin.virtqemud.in | 6 +- src/security/security_apparmor.c | 84 +++++----- src/security/virt-aa-helper.c | 145 +++++++++--------- 5 files changed, 125 insertions(+), 122 deletions(-) -- 2.43.0
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> Reviewed-by: Jim Fehlig <jfehlig@suse.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 eed0f265d6..ae2175d334 100644 --- a/src/security/security_apparmor.c +++ b/src/security/security_apparmor.c @@ -1109,8 +1109,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.43.0
Moving towards full adoption of GLib APIs in the AppArmor code. Signed-off-by: Georgia Garcia <georgia.garcia@canonical.com> Reviewed-by: Jim Fehlig <jfehlig@suse.com> --- src/security/security_apparmor.c | 42 +++++-------- src/security/virt-aa-helper.c | 100 ++++++++++--------------------- 2 files changed, 46 insertions(+), 96 deletions(-) diff --git a/src/security/security_apparmor.c b/src/security/security_apparmor.c index ae2175d334..91c51f6395 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. @@ -247,8 +238,6 @@ use_apparmor(void) rc = -1; } - cleanup: - VIR_FREE(libvirt_daemon); return rc; } @@ -950,7 +939,8 @@ AppArmorSetChardevLabel(virSecurityManager *mgr, virDomainChrSourceDef *dev_source, bool chardevStdioLogd G_GNUC_UNUSED) { - char *in = NULL, *out = NULL; + g_autofree char *in = NULL; + g_autofree char *out = NULL; int ret = -1; virSecurityLabelDef *secdef; @@ -971,11 +961,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; @@ -995,9 +985,6 @@ AppArmorSetChardevLabel(virSecurityManager *mgr, break; } - done: - VIR_FREE(in); - VIR_FREE(out); return ret; } @@ -1083,12 +1070,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 94a28bf331..1626d5a89c 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; } /* @@ -574,7 +560,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))) { @@ -600,7 +586,6 @@ caps_mockup(vahControl * ctl, const char *xmlStr) ctl->arch = virArchFromHost(); } else { ctl->arch = virArchFromString(arch); - VIR_FREE(arch); } return 0; @@ -685,15 +670,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; @@ -730,7 +715,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); } @@ -754,7 +739,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] == '/') @@ -771,13 +756,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; } @@ -793,36 +771,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 @@ -1473,8 +1443,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; @@ -1517,7 +1487,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)) @@ -1579,7 +1549,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 @@ -1597,7 +1567,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) { @@ -1613,14 +1582,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.43.0
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 +++++-- src/security/apparmor/usr.sbin.virtqemud.in | 6 ++++-- 3 files changed, 13 insertions(+), 5 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 diff --git a/src/security/apparmor/usr.sbin.virtqemud.in b/src/security/apparmor/usr.sbin.virtqemud.in index 42fa4813da..86b23465b6 100644 --- a/src/security/apparmor/usr.sbin.virtqemud.in +++ b/src/security/apparmor/usr.sbin.virtqemud.in @@ -1,5 +1,7 @@ #include <tunables/global> @{LIBVIRT}="libvirt" +@{hextet}=[0-9a-f][0-9a-f][0-9a-f][0-9a-f] +@{UUID}=@{hextet}@{hextet}-@{hextet}-@{hextet}-@{hextet}-@{hextet}@{hextet}@{hextet} profile virtqemud @sbindir@/virtqemud flags=(attach_disconnected) { #include <abstractions/base> @@ -71,7 +73,7 @@ profile virtqemud @sbindir@/virtqemud 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), @@ -109,7 +111,7 @@ profile virtqemud @sbindir@/virtqemud flags=(attach_disconnected) { /etc/libvirt/hooks/** 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.43.0
On 1/7/25 08:23, 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 +++++-- src/security/apparmor/usr.sbin.virtqemud.in | 6 ++++-- 3 files changed, 13 insertions(+), 5 deletions(-)
This patch seems fine to me. Did you notice the issue by code inspection, or does it fix an observed error? If the latter, we should mention it in the commit message. Reviewed-by: Jim Fehlig <jfehlig@suse.com> Regards, Jim
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 diff --git a/src/security/apparmor/usr.sbin.virtqemud.in b/src/security/apparmor/usr.sbin.virtqemud.in index 42fa4813da..86b23465b6 100644 --- a/src/security/apparmor/usr.sbin.virtqemud.in +++ b/src/security/apparmor/usr.sbin.virtqemud.in @@ -1,5 +1,7 @@ #include <tunables/global> @{LIBVIRT}="libvirt" +@{hextet}=[0-9a-f][0-9a-f][0-9a-f][0-9a-f] +@{UUID}=@{hextet}@{hextet}-@{hextet}-@{hextet}-@{hextet}-@{hextet}@{hextet}@{hextet}
profile virtqemud @sbindir@/virtqemud flags=(attach_disconnected) { #include <abstractions/base> @@ -71,7 +73,7 @@ profile virtqemud @sbindir@/virtqemud 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),
@@ -109,7 +111,7 @@ profile virtqemud @sbindir@/virtqemud flags=(attach_disconnected) { /etc/libvirt/hooks/** 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 Tue, 2025-01-07 at 17:04 -0700, Jim Fehlig wrote:
On 1/7/25 08:23, 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 +++++-- src/security/apparmor/usr.sbin.virtqemud.in | 6 ++++-- 3 files changed, 13 insertions(+), 5 deletions(-)
This patch seems fine to me. Did you notice the issue by code inspection, or does it fix an observed error? If the latter, we should mention it in the commit message.
It was indeed by code inspection. Since the rules were broader than needed, we wouldn't see errors related to this in normal libvirt use. I'm just restricting it to what was the intended behavior. Thank you, Georgia
Reviewed-by: Jim Fehlig <jfehlig@suse.com>
Regards, Jim
On 1/8/25 04:45, Georgia Garcia wrote:
On Tue, 2025-01-07 at 17:04 -0700, Jim Fehlig wrote:
On 1/7/25 08:23, 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 +++++-- src/security/apparmor/usr.sbin.virtqemud.in | 6 ++++-- 3 files changed, 13 insertions(+), 5 deletions(-)
This patch seems fine to me. Did you notice the issue by code inspection, or does it fix an observed error? If the latter, we should mention it in the commit message.
It was indeed by code inspection. Since the rules were broader than needed, we wouldn't see errors related to this in normal libvirt use. I'm just restricting it to what was the intended behavior.
Thanks for confirming. I've pushed patches 1-3 since they are independent improvements from the issue you're trying to fix in patch 4. Regards, Jim
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 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); } -- 2.43.0
On 1/7/25 08:23, 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.
Have you considered, or experimented with, adding a "remove file" option to the "replace" mode of virt-aa-helper? Figuring out the short name of the option might be the most difficult part :-P.
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.
I'm not super excited about this approach, but I'm also no apparmor expert. Perhaps my preference for a '--remove-file' option to supplement '--add-file' is not really possible or realistic with the current apparmor integration. Andrea also has some experience with apparmor and its libvirt support. He may have better advice on fixing this issue. Regards, Jim
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 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); }
On Tue, 2025-01-07 at 17:29 -0700, Jim Fehlig wrote:
On 1/7/25 08:23, 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.
Have you considered, or experimented with, adding a "remove file" option to the "replace" mode of virt-aa-helper? Figuring out the short name of the option might be the most difficult part :-P.
I didn't experiment with it because I thought it was a change too drastic to change the behavior of "Restore" to instead of regenerate the policy based on the xml, to read the policy, string match the drive (for example) being removed, remove that entry and rewrite the policy file. By maintaining current behavior for the most part I would also lower the risk of regressions. It might be possible but I'd have to look into more detail into all the "Restore" hooks to say for certain.
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.
I'm not super excited about this approach, but I'm also no apparmor expert. Perhaps my preference for a '--remove-file' option to supplement '--add-file' is not really possible or realistic with the current apparmor integration.
Andrea also has some experience with apparmor and its libvirt support. He may have better advice on fixing this issue.
Since there aren't hooks for removing permissions for files that were created by FD (domainSetSecurityImageFDLabel / domainSetSecurityTapFDLabel) I figured that separating them in a different file was the best approach but I'm open to changing it if it's more appropriate. Any feedback is welcome! Thank you, Georgia
Regards, Jim
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 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); }
On 1/8/25 06:50, Georgia Garcia wrote:
On Tue, 2025-01-07 at 17:29 -0700, Jim Fehlig wrote:
On 1/7/25 08:23, 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.
Have you considered, or experimented with, adding a "remove file" option to the "replace" mode of virt-aa-helper? Figuring out the short name of the option might be the most difficult part :-P.
I didn't experiment with it because I thought it was a change too drastic to change the behavior of "Restore" to instead of regenerate the policy based on the xml, to read the policy, string match the drive (for example) being removed, remove that entry and rewrite the policy file. By maintaining current behavior for the most part I would also lower the risk of regressions. It might be possible but I'd have to look into more detail into all the "Restore" hooks to say for certain.
Given the current behavior of Restore, I share your fear of regressions.
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.
I'm not super excited about this approach, but I'm also no apparmor expert. Perhaps my preference for a '--remove-file' option to supplement '--add-file' is not really possible or realistic with the current apparmor integration.
Andrea also has some experience with apparmor and its libvirt support. He may have better advice on fixing this issue.
Since there aren't hooks for removing permissions for files that were created by FD (domainSetSecurityImageFDLabel / domainSetSecurityTapFDLabel) I figured that separating them in a different file was the best approach but I'm open to changing it if it's more appropriate. Any feedback is welcome!
I'm not against the approach, and indeed it may be the safest way to go. Before I invest time reviewing and testing this patch, let's see if others have comments/suggestions. Regards, Jim PS: Add Andrea to cc this time :-)
Sorry for pinging, but any updates on this? We have a customer waiting for this fix and if this needs to be reworked, we need to start working on it asap to get it merged on time. Thank you, Tiago On Wed, Jan 8, 2025 at 3:07 PM Jim Fehlig via Devel <devel@lists.libvirt.org> wrote:
On Tue, 2025-01-07 at 17:29 -0700, Jim Fehlig wrote:
On 1/7/25 08:23, 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.
Have you considered, or experimented with, adding a "remove file"
"replace" mode of virt-aa-helper? Figuring out the short name of the
On 1/8/25 06:50, Georgia Garcia wrote: option to the option
might be the most difficult part :-P.
I didn't experiment with it because I thought it was a change too drastic to change the behavior of "Restore" to instead of regenerate the policy based on the xml, to read the policy, string match the drive (for example) being removed, remove that entry and rewrite the policy file. By maintaining current behavior for the most part I would also lower the risk of regressions. It might be possible but I'd have to look into more detail into all the "Restore" hooks to say for certain.
Given the current behavior of Restore, I share your fear of regressions.
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.
I'm not super excited about this approach, but I'm also no apparmor
expert.
Perhaps my preference for a '--remove-file' option to supplement '--add-file' is not really possible or realistic with the current apparmor integration.
Andrea also has some experience with apparmor and its libvirt support. He may have better advice on fixing this issue.
Since there aren't hooks for removing permissions for files that were created by FD (domainSetSecurityImageFDLabel / domainSetSecurityTapFDLabel) I figured that separating them in a different file was the best approach but I'm open to changing it if it's more appropriate. Any feedback is welcome!
I'm not against the approach, and indeed it may be the safest way to go. Before I invest time reviewing and testing this patch, let's see if others have comments/suggestions.
Regards, Jim
PS: Add Andrea to cc this time :-)
On Wed, Jan 08, 2025 at 11:06:54AM -0700, Jim Fehlig wrote:
On 1/8/25 06:50, Georgia Garcia wrote:
On Tue, 2025-01-07 at 17:29 -0700, Jim Fehlig wrote:
On 1/7/25 08:23, 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.
Have you considered, or experimented with, adding a "remove file" option to the "replace" mode of virt-aa-helper? Figuring out the short name of the option might be the most difficult part :-P.
I didn't experiment with it because I thought it was a change too drastic to change the behavior of "Restore" to instead of regenerate the policy based on the xml, to read the policy, string match the drive (for example) being removed, remove that entry and rewrite the policy file. By maintaining current behavior for the most part I would also lower the risk of regressions. It might be possible but I'd have to look into more detail into all the "Restore" hooks to say for certain.
Given the current behavior of Restore, I share your fear of regressions.
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.
I'm not super excited about this approach, but I'm also no apparmor expert. Perhaps my preference for a '--remove-file' option to supplement '--add-file' is not really possible or realistic with the current apparmor integration.
Andrea also has some experience with apparmor and its libvirt support. He may have better advice on fixing this issue.
Since there aren't hooks for removing permissions for files that were created by FD (domainSetSecurityImageFDLabel / domainSetSecurityTapFDLabel) I figured that separating them in a different file was the best approach but I'm open to changing it if it's more appropriate. Any feedback is welcome!
I'm not against the approach, and indeed it may be the safest way to go. Before I invest time reviewing and testing this patch, let's see if others have comments/suggestions.
PS: Add Andrea to cc this time :-)
Thanks for looping me in. I'm far from an expert when it comes to AppArmor, but after giving the patch a closer look I share some of Jim's concerns. While the current approach of throwing away all information that is not recorded in the XML is obviously problematic, it seems that what you're implementing here is a workaround for a somewhat narrow failure scenario that doesn't fully address the underlying limitations. Going by the example presented in [1], IIUC your change would make it so the lines needed for macvtap use, specifically "/dev/net/tun" rwk, "/dev/tap82" rwk, would be written to .runtime_files instead of .files. That's good enough to safeguard them from disappearing when disks are unplugged, but what if the macvtap interface itself is? Wouldn't those lines linger around despite being no longer needed? I've also noticed that the lines "/var/lib/libvirt/qemu/domain-33-tap/{,**}" rwk, "/run/libvirt/qemu/channel/33-tap/{,**}" rwk, "/var/lib/libvirt/qemu/domain-33-tap/master-key.aes" rwk, have disappeared, and the lines "/dev/pts/0" rw, "/dev/pts/0" rw, (duplicated?) have appeared. That doesn't seem right, and I wouldn't be surprised if this change could lead to further issues. So I think we really need a --remove-file option that can be used to carefully undo the changes applied by an earlier use of --add-file. Unfortunately this will likely involve a far more significant rework of the AppArmor driver, and we will certainly have to be careful about not introducing regressions in the process, but I'm really not a fan of half measures unless the trade-off is overwhelmingly stacked in their favor... To reiterate, this opinion is based on vague at best familiarity with the AppArmor driver. Don't hesitate to point out why I'm wrong :) [1] https://gitlab.com/libvirt/libvirt/-/issues/692 -- Andrea Bolognani / Red Hat / Virtualization
On Tue, 2025-01-14 at 12:13 -0600, Andrea Bolognani wrote:
On Wed, Jan 08, 2025 at 11:06:54AM -0700, Jim Fehlig wrote:
On 1/8/25 06:50, Georgia Garcia wrote:
On Tue, 2025-01-07 at 17:29 -0700, Jim Fehlig wrote:
On 1/7/25 08:23, 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.
Have you considered, or experimented with, adding a "remove file" option to the "replace" mode of virt-aa-helper? Figuring out the short name of the option might be the most difficult part :-P.
I didn't experiment with it because I thought it was a change too drastic to change the behavior of "Restore" to instead of regenerate the policy based on the xml, to read the policy, string match the drive (for example) being removed, remove that entry and rewrite the policy file. By maintaining current behavior for the most part I would also lower the risk of regressions. It might be possible but I'd have to look into more detail into all the "Restore" hooks to say for certain.
Given the current behavior of Restore, I share your fear of regressions.
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.
I'm not super excited about this approach, but I'm also no apparmor expert. Perhaps my preference for a '--remove-file' option to supplement '--add-file' is not really possible or realistic with the current apparmor integration.
Andrea also has some experience with apparmor and its libvirt support. He may have better advice on fixing this issue.
Since there aren't hooks for removing permissions for files that were created by FD (domainSetSecurityImageFDLabel / domainSetSecurityTapFDLabel) I figured that separating them in a different file was the best approach but I'm open to changing it if it's more appropriate. Any feedback is welcome!
I'm not against the approach, and indeed it may be the safest way to go. Before I invest time reviewing and testing this patch, let's see if others have comments/suggestions.
PS: Add Andrea to cc this time :-)
Thanks for looping me in.
I'm far from an expert when it comes to AppArmor, but after giving the patch a closer look I share some of Jim's concerns.
While the current approach of throwing away all information that is not recorded in the XML is obviously problematic, it seems that what you're implementing here is a workaround for a somewhat narrow failure scenario that doesn't fully address the underlying limitations.
Going by the example presented in [1], IIUC your change would make it so the lines needed for macvtap use, specifically
"/dev/net/tun" rwk, "/dev/tap82" rwk,
would be written to .runtime_files instead of .files. That's good enough to safeguard them from disappearing when disks are unplugged, but what if the macvtap interface itself is? Wouldn't those lines linger around despite being no longer needed?
Yes, they would, and that is the current behavior - if you remove only the macvtap, it will not be removed from .files That's the current limitation because there are no security hooks called when macvtap devices are unplugged. I thought it would be better to be over-permissive (fd permissions linger throughout the runtime of the vm) than over-restrictive to fix the issue given what's available in the security side of libvirt.
I've also noticed that the lines
"/var/lib/libvirt/qemu/domain-33-tap/{,**}" rwk, "/run/libvirt/qemu/channel/33-tap/{,**}" rwk, "/var/lib/libvirt/qemu/domain-33-tap/master-key.aes" rwk,
These rules were also created from a file descriptor like macvtap devices, therefore were deleted when the profile was created based on the xml on a restore.
have disappeared, and the lines
"/dev/pts/0" rw, "/dev/pts/0" rw,
(duplicated?) have appeared. That doesn't seem right, and I wouldn't be surprised if this change could lead to further issues.
So I think we really need a --remove-file option that can be used to carefully undo the changes applied by an earlier use of --add-file.
Unfortunately this will likely involve a far more significant rework of the AppArmor driver, and we will certainly have to be careful about not introducing regressions in the process, but I'm really not a fan of half measures unless the trade-off is overwhelmingly stacked in their favor...
As I said earlier, it would also involve the addition of at least one security hook, impacting all security drivers. But yes, this change would basically involve rewriting the entire AppArmor driver and a part of virt-aa-helper. While I'm not against it, unfortunately I will not be able to dedicate the amount of time needed for such a significant change. Thank you, Georgia
To reiterate, this opinion is based on vague at best familiarity with the AppArmor driver. Don't hesitate to point out why I'm wrong :)
On Wed, Jan 15, 2025 at 11:49:43AM -0300, Georgia Garcia wrote:
On Tue, 2025-01-14 at 12:13 -0600, Andrea Bolognani wrote:
Going by the example presented in [1], IIUC your change would make it so the lines needed for macvtap use, specifically
"/dev/net/tun" rwk, "/dev/tap82" rwk,
would be written to .runtime_files instead of .files. That's good enough to safeguard them from disappearing when disks are unplugged, but what if the macvtap interface itself is? Wouldn't those lines linger around despite being no longer needed?
Yes, they would, and that is the current behavior - if you remove only the macvtap, it will not be removed from .files
That's the current limitation because there are no security hooks called when macvtap devices are unplugged. I thought it would be better to be over-permissive (fd permissions linger throughout the runtime of the vm) than over-restrictive to fix the issue given what's available in the security side of libvirt.
From a functional standpoint sure, leaving rules behind is not a problem. But considering that the whole point of a security driver is to prevent the QEMU process from accessing resources that it shouldn't have access to, I would consider it a pretty serious flaw.
So I think we really need a --remove-file option that can be used to carefully undo the changes applied by an earlier use of --add-file.
Unfortunately this will likely involve a far more significant rework of the AppArmor driver, and we will certainly have to be careful about not introducing regressions in the process, but I'm really not a fan of half measures unless the trade-off is overwhelmingly stacked in their favor...
As I said earlier, it would also involve the addition of at least one security hook, impacting all security drivers. But yes, this change would basically involve rewriting the entire AppArmor driver and a part of virt-aa-helper. While I'm not against it, unfortunately I will not be able to dedicate the amount of time needed for such a significant change.
I haven't looked in detail at how much work adding the ability to remove rules on device unplug would require, but surely "basically rewrite the entire driver" is an overexaggeration? Look, I understand that you probably just want to fix the issue that's affecting your customers then move on with your life, and generally speaking I don't really have a problem with partial fixes that merely get us closer to the solution instead of all the way there. However, the changes you're proposing here alter how the driver operates in a pretty fundamental and, critically, user-visible way. I'm not keen on switching to a new approach while already being aware of the fact that a full fix with require yet another pivot... -- Andrea Bolognani / Red Hat / Virtualization
On 1/7/25 08:23, 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.
In the first sentence, I'd suggest being a bit more explicit and say "added to the domain's AppArmor policy".
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.
Same comment for the first sentence in this paragraph.
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 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)
The comment should be updated to reflect the difference between reload_profile and reload_runtime_profile. Although IMO the names suggest they reload different profiles, when in fact they both reload the domain profile. reload_profile is internal, so fine to just extend it with the additional parameter IMO.
{ 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"
I saved the bikeshedding for last :-). How about "-e | --ephemeral file exists only while domain is running" ? Regards, Jim
"\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); }
participants (4)
-
Andrea Bolognani -
Georgia Garcia -
Jim Fehlig -
Tiago Pasqualini