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 0x55561a13 in array_starts_with (str=0x557ce910
/tmp/tmp.6nI2Fkv0KL/1.img, arr=0x7fffd160, size=-1540438016) at
security/virt-aa-helper.c:525
#2 0x55561d49 in valid_path (path=0x557ce910
/tmp/tmp.6nI2Fkv0KL/1.img, readonly=false) at security/virt-aa-helper.c:617
#3 0x55562506 in vah_add_path (buf=0x7fffd3e0, path=0x557cb910
/tmp/tmp.6nI2Fkv0KL/1.img, perms=0x55581585 rw, recursive=false) at
security/virt-aa-helper.c:823
#4 0x55562693 in vah_add_file (buf=0x7fffd3e0, path=0x557cb910
/tmp/tmp.6nI2Fkv0KL/1.img, perms=0x55581585 rw) at
security/virt-aa-helper.c:854
#5 0x55562918 in add_file_path (disk=0x557d4440, path=0x557cb910
/tmp/tmp.6nI2Fkv0KL/1.img, depth=0, opaque=0x7fffd3e0) at
security/virt-aa-helper.c:931
#6 0x778f18b1 in virDomainDiskDefForeachPath (disk=0x557d4440,
ignoreOpenFailure=true, iter=0x555628a6 add_file_path,
opaque=0x7fffd3e0) at conf/domain_conf.c:23286
#7 0x55562b5f in get_files (ctl=0x7fffd670) at
security/virt-aa-helper.c:982
#8 0x55564100 in vahParseArgv (ctl=0x7fffd670, argc=5,
argv=0x7fffd7e8) at security/virt-aa-helper.c:1277
#9 0x555643d6 in main (argc=5, argv=0x7fffd7e8) 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 mpriv...@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@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
signature.asc
Description: PGP signature
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
