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.0On 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);
> }
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 :-)
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 :)
>
>
> [1] https://gitlab.com/libvirt/libvirt/-/issues/692
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
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 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 :-) >
© 2016 - 2025 Red Hat, Inc.