[libvirt] [PATCH] virt-aa-helper: resolve yet to be created paths
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
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
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
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
participants (2)
-
Christian Ehrhardt -
Michal Privoznik