[libvirt] [PATCH] virt-aa-helper: Improve valid_path

[Martin Kletzander]

On Thu, Aug 27, 2015 at 03:04:12AM +0200, Michal Privoznik wrote:
>So, after some movement in virt-aa-helper, I've noticed the
>virt-aa-helper-test failing. I've ran gdb (it took me a while to
>realize how to do that) and this showed up immediately:
>

What was the hard part of running gdb?  Making the virt-aa-helper run
under it during test suite or anything else?

>  Program received signal SIGSEGV, Segmentation fault.
>  strlen () at ../sysdeps/x86_64/strlen.S:106
>  106     ../sysdeps/x86_64/strlen.S: No such file or directory.
>  (gdb) bt
>  #0  strlen () at ../sysdeps/x86_64/strlen.S:106
>  #1  0x0000555555561a13 in array_starts_with (str=0x5555557ce910 "/tmp/tmp.6nI2Fkv0KL/1.img", arr=0x7fffffffd160, size=-1540438016) at security/virt-aa-helper.c:525
>  #2  0x0000555555561d49 in valid_path (path=0x5555557ce910 "/tmp/tmp.6nI2Fkv0KL/1.img", readonly=false) at security/virt-aa-helper.c:617
>  #3  0x0000555555562506 in vah_add_path (buf=0x7fffffffd3e0, path=0x5555557cb910 "/tmp/tmp.6nI2Fkv0KL/1.img", perms=0x555555581585 "rw", recursive=false) at security/virt-aa-helper.c:823
>  #4  0x0000555555562693 in vah_add_file (buf=0x7fffffffd3e0, path=0x5555557cb910 "/tmp/tmp.6nI2Fkv0KL/1.img", perms=0x555555581585 "rw") at security/virt-aa-helper.c:854
>  #5  0x0000555555562918 in add_file_path (disk=0x5555557d4440, path=0x5555557cb910 "/tmp/tmp.6nI2Fkv0KL/1.img", depth=0, opaque=0x7fffffffd3e0) at security/virt-aa-helper.c:931
>  #6  0x00007ffff78f18b1 in virDomainDiskDefForeachPath (disk=0x5555557d4440, ignoreOpenFailure=true, iter=0x5555555628a6 <add_file_path>, opaque=0x7fffffffd3e0) at conf/domain_conf.c:23286
>  #7  0x0000555555562b5f in get_files (ctl=0x7fffffffd670) at security/virt-aa-helper.c:982
>  #8  0x0000555555564100 in vahParseArgv (ctl=0x7fffffffd670, argc=5, argv=0x7fffffffd7e8) at security/virt-aa-helper.c:1277
>  #9  0x00005555555643d6 in main (argc=5, argv=0x7fffffffd7e8) at security/virt-aa-helper.c:1332
>
>So I've taken look at valid_path() because it is obviously
>calling array_starts_with() with malformed @size. And here's the
>result: there are two variables to hold the size of three arrays
>and their value is recalculated before each call of
>array_starts_with(). What if we just use three variables,
>initialize them and do not touch them afterwards?
>
>Signed-off-by: Michal Privoznik <mprivozn at redhat.com>
>---
> src/security/virt-aa-helper.c | 18 ++++++++----------
> 1 file changed, 8 insertions(+), 10 deletions(-)
>
>diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c
>index a78c4c8..b4a8f27 100644
>--- a/src/security/virt-aa-helper.c
>+++ b/src/security/virt-aa-helper.c
>@@ -546,9 +546,6 @@ array_starts_with(const char *str, const char * const *arr, const long size)
> static int
> valid_path(const char *path, const bool readonly)
> {
>-    int npaths;
>-    int nropaths;
>-
>     const char * const restricted[] = {
>         "/bin/",
>         "/etc/",
>@@ -581,6 +578,10 @@ valid_path(const char *path, const bool readonly)
>         "/etc/libvirt-sandbox/services/" /* for virt-sandbox service config */
>     };
>
>+    const int nropaths = ARRAY_CARDINALITY(restricted);
>+    const int nrwpaths = ARRAY_CARDINALITY(restricted_rw);
>+    const int nopaths = ARRAY_CARDINALITY(override);
>+
>     if (path == NULL) {
>         vah_error(NULL, 0, _("bad pathname"));
>         return -1;
>@@ -600,21 +601,18 @@ valid_path(const char *path, const bool readonly)
>         vah_warning(_("path does not exist, skipping file type checks"));
>
>     /* overrides are always allowed */
>-    npaths = sizeof(override)/sizeof(*(override));
>-    if (array_starts_with(path, override, npaths) == 0)
>+    if (array_starts_with(path, override, nopaths) == 0)
>         return 0;
>
>     /* allow read only paths upfront */
>     if (readonly) {
>-        nropaths = sizeof(restricted_rw)/sizeof(*(restricted_rw));
>-        if (array_starts_with(path, restricted_rw, nropaths) == 0)
>+        if (array_starts_with(path, restricted_rw, nrwpaths) == 0)

So if it wasn't readonly, the nropaths was not calculated, ...

>             return 0;
>     }
>
>     /* disallow RW acess to all paths in restricted and restriced_rw */
>-    npaths = sizeof(restricted)/sizeof(*(restricted));
>-    if ((array_starts_with(path, restricted, npaths) == 0
>-        || array_starts_with(path, restricted_rw, nropaths) == 0))

...but it was used here.  I remember that when reviewing I spotted
something like that, but when writing the review I missed it, so I
thought the first look fooled me.  Well, it wasn't the case and this
is of course cleaner.

ACK and safe for freeze.

>+    if ((array_starts_with(path, restricted, nropaths) == 0 ||
>+         array_starts_with(path, restricted_rw, nrwpaths) == 0))
>         return 1;
>
>     return 0;
>--
>2.4.6
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150827/d214cc34/attachment-0001.sig>