9:23 a.m.
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
9:23 a.m.
New subject: [PATCH v3 1/4] security_apparmor: fix memleaks in AppArmorSetFDLabel
proc and fd_path are allocated but never freed. Fix by using
g_autofree instead.
Fixes: b9757fea30785a92aa95ea675b9bc371e4fb2e8c
Signed-off-by: Georgia Garcia <georgia.garcia(a)canonical.com>
Reviewed-by: Jim Fehlig <jfehlig(a)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
9:23 a.m.
New subject: [PATCH v3 2/4] security: replace uses of label and VIR_FREE by g_autofree
Moving towards full adoption of GLib APIs in the AppArmor code.
Signed-off-by: Georgia Garcia <georgia.garcia(a)canonical.com>
Reviewed-by: Jim Fehlig <jfehlig(a)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
9:23 a.m.
New subject: [PATCH v3 3/4] apparmor: fix UUID specification
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(a)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(a)/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-(a){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
6:04 p.m.
New subject: [PATCH v3 3/4] apparmor: fix UUID specification
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(a)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(a)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(a)/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-(a){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
5:45 a.m.
New subject: [PATCH v3 3/4] apparmor: fix UUID specification
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(a)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(a)suse.com>
Regards,
Jim
11:52 a.m.
New subject: [PATCH v3 3/4] apparmor: fix UUID specification
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(a)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
9:23 a.m.
New subject: [PATCH v3 4/4] virt-aa-helper: store dynamically generated rules
Some rules are generated dynamically during boot and added to the
AppArmor policy. An example of that is macvtap devices that call the
AppArmorSetFDLabel hook to add a rule for the tap device path.
Since this information is dynamic, it is not available in the xml
config, therefore whenever a "Restore" hook is called, the entire
profile is regenerated by virt-aa-helper based only the information
from the VM definition, so the dynamic/runtime information is lost.
This patch stores the dynamically generated rules in a new file called
libvirt-uuid.runtime_files which is included by the AppArmor
policy. This file should exist while the domain is running and should
be reloaded automatically whenever there's a restore operation. These
rules only make sense when the VM is running, so the file is removed
when the VM is shutdown.
Note that there are no hooks for restoring FD labels, so that
information is not removed from the set of rules while the domain is
running.
Closes: https://gitlab.com/libvirt/libvirt/-/issues/692
Signed-off-by: Georgia Garcia <georgia.garcia(a)canonical.com>
---
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
6:29 p.m.
New subject: [PATCH v3 4/4] virt-aa-helper: store dynamically generated rules
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(a)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);
}
7:50 a.m.
New subject: [PATCH v3 4/4] virt-aa-helper: store dynamically generated rules
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(a)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);
> }
12:06 p.m.
New subject: [PATCH v3 4/4] virt-aa-helper: store dynamically generated rules
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 :-)
7:59 a.m.
New subject: [PATCH v3 4/4] virt-aa-helper: store dynamically generated rules
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(a)lists.libvirt.org>
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.
Regards,
Jim
PS: Add Andrea to cc this time :-)
12:13 p.m.
New subject: [PATCH v3 4/4] virt-aa-helper: store dynamically generated rules
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
8:49 a.m.
New subject: [PATCH v3 4/4] virt-aa-helper: store dynamically generated rules
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 :)
[1] https://gitlab.com/libvirt/libvirt/-/issues/692
11:32 a.m.
New subject: [PATCH v3 4/4] virt-aa-helper: store dynamically generated rules
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
3:17 p.m.
New subject: [PATCH v3 4/4] virt-aa-helper: store dynamically generated rules
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(a)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);
}
10
days inactive
19
days old
15 comments
4 participants
participants (4)
-
Andrea Bolognani -
Georgia Garcia -
Jim Fehlig -
Tiago Pasqualini