Re: [libvirt] [PATCH 1/2] apparmor: Allow access to filesystem mounts
On 28.01.2014 15:04, Jamie Strandboge wrote:
On 01/26/2014 03:47 PM, Felix Geyer wrote:
Make virt-aa-helper create rules to allow VMs access to filesystem mounts from the host.
Note that virt-aa-helper access to various parts of the filesystem is generally ok. However, can you be more specific about the problem you're trying to solve? Eg, is there a bug number?
virt-aa-helper doesn't create the appropriate rules to allow qemu access to shared filesystem mounts: http://libvirt.org/formatdomain.html#elementsFilesystems This made it necessary to manually modify the libivrt-<UUID> profile. There is a report about this on the Ubuntu bugtracker: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/943680 Regards, Felix
--- src/security/virt-aa-helper.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index b9282b4..e1f7848 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -578,9 +578,6 @@ valid_path(const char *path, const bool readonly) return -1;
switch (sb.st_mode & S_IFMT) { - case S_IFDIR: - return 1; - break; case S_IFSOCK: return 1; break; @@ -747,7 +744,7 @@ get_definition(vahControl * ctl, const char *xmlStr) }
static int -vah_add_file(virBufferPtr buf, const char *path, const char *perms) +vah_add_path(virBufferPtr buf, const char *path, const char *perms, bool recursive) { char *tmp = NULL; int rc = -1; @@ -788,10 +785,14 @@ vah_add_file(virBufferPtr buf, const char *path, const char *perms) goto cleanup; }
- virBufferAsprintf(buf, " \"%s\" %s,\n", tmp, perms); + virBufferAsprintf(buf, " \"%s%s\" %s,\n", tmp, recursive ? "/**" : "", perms); if (readonly) { virBufferAddLit(buf, " # don't audit writes to readonly files\n"); - virBufferAsprintf(buf, " deny \"%s\" w,\n", tmp); + virBufferAsprintf(buf, " deny \"%s%s\" w,\n", tmp, recursive ? "/**" : ""); + } + if (recursive) { + // allow reading (but not creating) the dir + virBufferAsprintf(buf, " \"%s/\" r,\n", tmp); }
cleanup: @@ -801,6 +802,12 @@ vah_add_file(virBufferPtr buf, const char *path, const char *perms) }
static int +vah_add_file(virBufferPtr buf, const char *path, const char *perms) +{ + return vah_add_path(buf, path, perms, false); +} + +static int vah_add_file_chardev(virBufferPtr buf, const char *path, const char *perms, @@ -1049,6 +1056,13 @@ get_files(vahControl * ctl) } /* switch */ }
+ for (i = 0; i < ctl->def->nfss; i++) { + virDomainFSDefPtr fs = ctl->def->fss[i]; + + if (vah_add_path(&buf, fs->src, fs->readonly ? "r" : "rw", true) != 0) + goto cleanup; + } + if (ctl->newfile) if (vah_add_file(&buf, ctl->newfile, "rw") != 0) goto cleanup;
On 01/28/2014 12:12 PM, Felix Geyer wrote:
On 28.01.2014 15:04, Jamie Strandboge wrote:
On 01/26/2014 03:47 PM, Felix Geyer wrote:
Make virt-aa-helper create rules to allow VMs access to filesystem mounts from the host.
Note that virt-aa-helper access to various parts of the filesystem is generally ok. However, can you be more specific about the problem you're trying to solve? Eg, is there a bug number?
virt-aa-helper doesn't create the appropriate rules to allow qemu access to shared filesystem mounts: http://libvirt.org/formatdomain.html#elementsFilesystems This made it necessary to manually modify the libivrt-<UUID> profile.
There is a report about this on the Ubuntu bugtracker: https://bugs.launchpad.net/ubuntu/+source/libvirt/+bug/943680
Ok, thanks for the extra information.
--- src/security/virt-aa-helper.c | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index b9282b4..e1f7848 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -578,9 +578,6 @@ valid_path(const char *path, const bool readonly) return -1;
switch (sb.st_mode & S_IFMT) { - case S_IFDIR: - return 1; - break; case S_IFSOCK: return 1; break; @@ -747,7 +744,7 @@ get_definition(vahControl * ctl, const char *xmlStr) }
static int -vah_add_file(virBufferPtr buf, const char *path, const char *perms) +vah_add_path(virBufferPtr buf, const char *path, const char *perms, bool recursive) { char *tmp = NULL; int rc = -1; @@ -788,10 +785,14 @@ vah_add_file(virBufferPtr buf, const char *path, const char *perms) goto cleanup; }
- virBufferAsprintf(buf, " \"%s\" %s,\n", tmp, perms); + virBufferAsprintf(buf, " \"%s%s\" %s,\n", tmp, recursive ? "/**" : "", perms); if (readonly) { virBufferAddLit(buf, " # don't audit writes to readonly files\n"); - virBufferAsprintf(buf, " deny \"%s\" w,\n", tmp); + virBufferAsprintf(buf, " deny \"%s%s\" w,\n", tmp, recursive ? "/**" : ""); + } + if (recursive) { + // allow reading (but not creating) the dir + virBufferAsprintf(buf, " \"%s/\" r,\n", tmp); }
cleanup: @@ -801,6 +802,12 @@ vah_add_file(virBufferPtr buf, const char *path, const char *perms) }
static int +vah_add_file(virBufferPtr buf, const char *path, const char *perms) +{ + return vah_add_path(buf, path, perms, false); +} + +static int vah_add_file_chardev(virBufferPtr buf, const char *path, const char *perms, @@ -1049,6 +1056,13 @@ get_files(vahControl * ctl) } /* switch */ }
+ for (i = 0; i < ctl->def->nfss; i++) { + virDomainFSDefPtr fs = ctl->def->fss[i]; + + if (vah_add_path(&buf, fs->src, fs->readonly ? "r" : "rw", true) != 0) + goto cleanup; + } + if (ctl->newfile) if (vah_add_file(&buf, ctl->newfile, "rw") != 0) goto cleanup;
I've not tested this personally, but the patch looks fine from an AppArmor and virt-aa-helper perspective. The only bit I wasn't sure of was this last hunk of the patch-- I'd like upstream to comment on the correctness of (at least) this portion of the patch. I will say that I noticed the SElinux driver doesn't seem to use ctl->def->nfss, etc at all-- perhaps because it assumes the filesystem to be correctly labelled to begin with? (I didn't look any further than security_selinux.c). Conditional 'Acked-By: Jamie Strandboge <jamie@canonical.com>' provided others are ok with this last hunk. -- Jamie Strandboge http://www.ubuntu.com/
participants (2)
-
Felix Geyer -
Jamie Strandboge