On Wed, Mar 7, 2018 at 12:45 PM, Michal Privoznik <mprivozn@xxxxxxxxxx> wrote:
On 03/01/2018 03:53 PM, Christian Ehrhardt wrote:
> In certain cases a xml contains paths that do not yet exist, but
> are valid as qemu will create them later on - for example
> vhostuser mode=server sockets.
>
> In any such cases so far the check to virFileExists failed and due to
> that the paths stayed non-resolved in regard to symlinks.
>
> But for apparmor those non-resolved rules are non functional as they
> are evaluated after resolving any symlinks.
>
> Therefore for non-existent files and partially non-existent paths
> resolve as much as possible to get valid rules.
>
> Example:
> <interface type='vhostuser'>
> <model type='virtio'/>
> <source type='unix'
> path='/var/run/symlinknet'
> mode='server'/>
Don't be afraid to write this on one line. Firstly, the 80 character
line is just a soft limit, secondly I like to see verbatim text
unchanged. But feel free to keep it as it is. Your call.
I guess this can be written simpler:
> </interface>
>
> Got rendered as:
> "/var/run/symlinknet" rw,
>
> But correct with "/var/run" being a symlink to "/run" is:
> "/run/symlinknet" rw,
>
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com >
> ---
> src/security/virt-aa-helper.c | 45 ++++++++++++++++++++++++++++++++++++-------
> 1 file changed, 38 insertions(+), 7 deletions(-)
>
> diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper. c
> index ff0068c..91bc339 100644
> --- a/src/security/virt-aa-helper.c
> +++ b/src/security/virt-aa-helper.c
> @@ -41,6 +41,7 @@
> #include "viralloc.h"
> #include "vircommand.h"
> #include "virlog.h"
> +#include "dirname.h"
> #include "driver.h"
>
> #include "security_driver.h"
> @@ -752,6 +753,9 @@ vah_add_path(virBufferPtr buf, const char *path, const char *perms, bool recursi
> bool explicit_deny_rule = true;
> char *sub = NULL;
> char *perms_new = NULL;
> + char *pathdir = NULL;
> + char *pathtmp = NULL;
> + char *pathreal = NULL;
>
> if (path == NULL)
> return rc;
> @@ -766,14 +770,38 @@ vah_add_path(virBufferPtr buf, const char *path, const char *perms, bool recursi
> return 0;
> }
>
> - if (virFileExists(path)) {
> - if ((tmp = realpath(path, NULL)) == NULL) {
> - vah_error(NULL, 0, path);
> - vah_error(NULL, 0, _("could not find realpath for disk"));
> - return rc;
> + /* files might be created by qemu later on and not exist right now.
> + * But realpath needs a valid path to work on, therefore:
> + * 1. walk the path to find longest valid path
> + * 2. get the realpath of that valid path
> + * 3. re-combine the realpath with the remaining suffix
> + * Note: A totally non existent path is used as-is
> + */
> + if ((pathdir = mdir_name(path)) == NULL)
> + goto cleanup;
> + while (!virFileExists(pathdir)) {
> + if (VIR_STRDUP_QUIET(pathtmp, pathdir) < 0)
> + goto cleanup;
> + VIR_FREE(pathdir);
> + if ((pathdir = mdir_name(pathtmp)) == NULL)
> + goto cleanup;
> + VIR_FREE(pathtmp);
> + }
if ((pathdir = mdir_name(path)) == NULL)
goto cleanup;
while (!virFileExists(pathdir)) {
if ((pathtmp = mdir_name(pathdir)) == NULL)
goto cleanup;
VIR_FREE(pathdir);
VIR_STEAL_PTR(pathdir, pathtmp);
}
ACK whether you decide to go with my way (untested though) or what you
have. Sorry for the delay.
Thanks, the usage of steal makes this indeed much nicer.
I played through all the cases in my mind and think it is safe against leaks just as the former version was.
Also tests with various XMLs were good.
Also another round of checks was good, so pushing in the new version with your ack.
Thanks for the review Michal!
Michal
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
