src/security/virt-aa-helper.c | 45 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 38 insertions(+), 7 deletions(-)
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'/>
</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 (strlen(pathdir) == 1) {
+ /* nothing of the path does exist yet */
+ if (VIR_STRDUP_QUIET(tmp, path) < 0)
+ goto cleanup;
+ } else {
+ if (VIR_STRDUP_QUIET(pathtmp, path+strlen(pathdir)) < 0)
+ goto cleanup;
+ if ((pathreal = realpath(pathdir, NULL)) == NULL) {
+ vah_error(NULL, 0, pathdir);
+ vah_error(NULL, 0, _("could not find realpath"));
+ goto cleanup;
}
- } else if (VIR_STRDUP_QUIET(tmp, path) < 0) {
- return rc;
+ if (virAsprintfQuiet(&tmp, "%s%s", pathreal, pathtmp) < 0)
+ goto cleanup;
}
if (VIR_STRDUP_QUIET(perms_new, perms) < 0)
@@ -814,6 +842,9 @@ vah_add_path(virBufferPtr buf, const char *path, const char *perms, bool recursi
}
cleanup:
+ VIR_FREE(pathdir);
+ VIR_FREE(pathtmp);
+ VIR_FREE(pathreal);
VIR_FREE(perms_new);
VIR_FREE(tmp);
--
2.7.4
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Ping, I'd appreciate if somebody would find the time to review as I'd like
to backport to 4.0 in Ubuntu soon.
On Thu, Mar 1, 2018 at 3:53 PM, Christian Ehrhardt <
christian.ehrhardt@canonical.com> 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'/>
> </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 (strlen(pathdir) == 1) {
> + /* nothing of the path does exist yet */
> + if (VIR_STRDUP_QUIET(tmp, path) < 0)
> + goto cleanup;
> + } else {
> + if (VIR_STRDUP_QUIET(pathtmp, path+strlen(pathdir)) < 0)
> + goto cleanup;
> + if ((pathreal = realpath(pathdir, NULL)) == NULL) {
> + vah_error(NULL, 0, pathdir);
> + vah_error(NULL, 0, _("could not find realpath"));
> + goto cleanup;
> }
> - } else if (VIR_STRDUP_QUIET(tmp, path) < 0) {
> - return rc;
> + if (virAsprintfQuiet(&tmp, "%s%s", pathreal, pathtmp) < 0)
> + goto cleanup;
> }
>
> if (VIR_STRDUP_QUIET(perms_new, perms) < 0)
> @@ -814,6 +842,9 @@ vah_add_path(virBufferPtr buf, const char *path, const
> char *perms, bool recursi
> }
>
> cleanup:
> + VIR_FREE(pathdir);
> + VIR_FREE(pathtmp);
> + VIR_FREE(pathreal);
> VIR_FREE(perms_new);
> VIR_FREE(tmp);
>
> --
> 2.7.4
>
>
--
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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.
> </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);
> + }
I guess this can be written simpler:
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.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On Wed, Mar 7, 2018 at 12:45 PM, Michal Privoznik <mprivozn@redhat.com>
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.
>
> > </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);
> > + }
>
> I guess this can be written simpler:
>
> 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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2024 Red Hat, Inc.