[libvirt] [PATCH 0/3] Misc fixes
Hi all, Here are a few patches without strong connection together. The first one only allows us not to package virt-login-shell even with lxc driver enabled. The other ones are related to mounts security. I'm wondering if changing the default dropped capabilities in the lxc driver is acceptable... dropping sys_admin makes sense, but it can introduce incompatibilities for users needing it as they will need to explicitely enable it. Cédric Bosdonnat (3): Allow building lxc without virt-login-shell virt-aa-helper: don't deny writes to readonly mounts lxc: drop sys_admin caps by default configure.ac | 14 ++++++++++++++ src/lxc/lxc_container.c | 1 + src/security/virt-aa-helper.c | 5 ++++- tools/Makefile.am | 12 ++++++------ 4 files changed, 25 insertions(+), 7 deletions(-) -- 2.1.4
Add a configure option to disable virt-login-shell build even if lxc is enabled. --- configure.ac | 14 ++++++++++++++ tools/Makefile.am | 12 ++++++------ 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/configure.ac b/configure.ac index f481c50..c766351 100644 --- a/configure.ac +++ b/configure.ac @@ -1074,6 +1074,19 @@ if test "$with_lxc" = "yes" ; then fi AM_CONDITIONAL([WITH_LXC], [test "$with_lxc" = "yes"]) +AC_ARG_WITH([login_shell], + [AS_HELP_STRING([--with-login-shell], + [build virt-login-shell @<:@default=yes@:>@])]) +m4_divert_text([DEFAULTS], [with_login_shell=yes]) + +if test "$with_lxc" != "yes" ; then + with_login_shell="no" +fi +if test "$with_login_shell" ; then + AC_DEFINE_UNQUOTED([WITH_LOGIN_SHELL], 1, [whether virt-login-shell is built]) +fi +AM_CONDITIONAL([WITH_LOGIN_SHELL], [test "$with_login_shell" = "yes"]) + dnl dnl Checks for the Parallels driver dnl @@ -2974,6 +2987,7 @@ AC_MSG_NOTICE([ Init script: $with_init_script]) AC_MSG_NOTICE([Char device locks: $with_chrdev_lock_files]) AC_MSG_NOTICE([ Default Editor: $DEFAULT_EDITOR]) AC_MSG_NOTICE([ Loader/NVRAM: $with_loader_nvram]) +AC_MSG_NOTICE([ virt-login-shell: $with_login_shell]) AC_MSG_NOTICE([]) AC_MSG_NOTICE([Developer Tools]) AC_MSG_NOTICE([]) diff --git a/tools/Makefile.am b/tools/Makefile.am index d5638d9..d005035 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -71,12 +71,12 @@ sbin_SCRIPTS = virt-sanlock-cleanup DISTCLEANFILES += virt-sanlock-cleanup endif WITH_SANLOCK -if WITH_LXC +if WITH_LOGIN_SHELL conf_DATA += virt-login-shell.conf bin_PROGRAMS += virt-login-shell -else ! WITH_LXC +else ! WITH_LOGIN_SHELL EXTRA_DIST += virt-login-shell.conf -endif ! WITH_LXC +endif ! WITH_LOGIN_SHELL dist_man1_MANS = \ @@ -84,11 +84,11 @@ dist_man1_MANS = \ virt-pki-validate.1 \ virt-xml-validate.1 \ virsh.1 -if WITH_LXC +if WITH_LOGIN_SHELL dist_man1_MANS += virt-login-shell.1 -else ! WITH_LXC +else ! WITH_LOGIN_SHELL EXTRA_DIST += virt-login-shell.1 -endif ! WITH_LXC +endif ! WITH_LOGIN_SHELL if WITH_SANLOCK dist_man8_MANS = virt-sanlock-cleanup.8 endif WITH_SANLOCK -- 2.1.4
On Tue, Nov 17, 2015 at 03:14:50PM +0100, Cédric Bosdonnat wrote:
Add a configure option to disable virt-login-shell build even if lxc is enabled. --- configure.ac | 14 ++++++++++++++ tools/Makefile.am | 12 ++++++------ 2 files changed, 20 insertions(+), 6 deletions(-)
ACK
diff --git a/configure.ac b/configure.ac index f481c50..c766351 100644 --- a/configure.ac +++ b/configure.ac @@ -1074,6 +1074,19 @@ if test "$with_lxc" = "yes" ; then fi AM_CONDITIONAL([WITH_LXC], [test "$with_lxc" = "yes"])
+AC_ARG_WITH([login_shell], + [AS_HELP_STRING([--with-login-shell], + [build virt-login-shell @<:@default=yes@:>@])]) +m4_divert_text([DEFAULTS], [with_login_shell=yes]) + +if test "$with_lxc" != "yes" ; then + with_login_shell="no" +fi +if test "$with_login_shell" ; then + AC_DEFINE_UNQUOTED([WITH_LOGIN_SHELL], 1, [whether virt-login-shell is built]) +fi +AM_CONDITIONAL([WITH_LOGIN_SHELL], [test "$with_login_shell" = "yes"]) + dnl dnl Checks for the Parallels driver dnl @@ -2974,6 +2987,7 @@ AC_MSG_NOTICE([ Init script: $with_init_script]) AC_MSG_NOTICE([Char device locks: $with_chrdev_lock_files]) AC_MSG_NOTICE([ Default Editor: $DEFAULT_EDITOR]) AC_MSG_NOTICE([ Loader/NVRAM: $with_loader_nvram]) +AC_MSG_NOTICE([ virt-login-shell: $with_login_shell]) AC_MSG_NOTICE([]) AC_MSG_NOTICE([Developer Tools]) AC_MSG_NOTICE([])
Minor nitpick - can you create an m4/virt-login-shell.m4 to hold the code and just call it from configure.ac Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
There is no need to deny writes on a readonly mount: write still won't be accepted, even if the user remounts the folder as RW in the guest as qemu sets the 9p mount as ro. This deny rule was leading to problems for example with readonly /: The qemu process had to write to a bunch of files in / like logs, sockets, etc. This deny rule was also preventing auditing of these denials, making it harder to debug. --- src/security/virt-aa-helper.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 5de56e5..a2d7226 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1127,7 +1127,10 @@ get_files(vahControl * ctl) ctl->def->fss[i]->src) { virDomainFSDefPtr fs = ctl->def->fss[i]; - if (vah_add_path(&buf, fs->src, fs->readonly ? "r" : "rw", true) != 0) + /* We don't need to add deny rw rules for readonly mounts, + * this can only lead to troubles when mounting / readonly. + */ + if (vah_add_path(&buf, fs->src, "rw", true) != 0) goto cleanup; } } -- 2.1.4
Hi all, Has that patch been skipped in the review process? -- Cedric On Tue, 2015-11-17 at 15:14 +0100, Cédric Bosdonnat wrote:
There is no need to deny writes on a readonly mount: write still won't be accepted, even if the user remounts the folder as RW in the guest as qemu sets the 9p mount as ro.
This deny rule was leading to problems for example with readonly /: The qemu process had to write to a bunch of files in / like logs, sockets, etc. This deny rule was also preventing auditing of these denials, making it harder to debug. --- src/security/virt-aa-helper.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 5de56e5..a2d7226 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1127,7 +1127,10 @@ get_files(vahControl * ctl) ctl->def->fss[i]->src) { virDomainFSDefPtr fs = ctl->def->fss[i];
- if (vah_add_path(&buf, fs->src, fs->readonly ? "r" : "rw", true) != 0) + /* We don't need to add deny rw rules for readonly mounts, + * this can only lead to troubles when mounting / readonly. + */ + if (vah_add_path(&buf, fs->src, "rw", true) != 0) goto cleanup; } }
On 11/26/2015 02:18 AM, Cedric Bosdonnat wrote:
Hi all,
Has that patch been skipped in the review process?
-- Cedric
On Tue, 2015-11-17 at 15:14 +0100, Cédric Bosdonnat wrote:
There is no need to deny writes on a readonly mount: write still won't be accepted, even if the user remounts the folder as RW in the guest as qemu sets the 9p mount as ro.
This deny rule was leading to problems for example with readonly /: The qemu process had to write to a bunch of files in / like logs, sockets, etc. This deny rule was also preventing auditing of these denials, making it harder to debug. --- src/security/virt-aa-helper.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 5de56e5..a2d7226 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1127,7 +1127,10 @@ get_files(vahControl * ctl) ctl->def->fss[i]->src) { virDomainFSDefPtr fs = ctl->def->fss[i];
- if (vah_add_path(&buf, fs->src, fs->readonly ? "r" : "rw", true) != 0) + /* We don't need to add deny rw rules for readonly mounts, + * this can only lead to troubles when mounting / readonly. + */ + if (vah_add_path(&buf, fs->src, "rw", true) != 0) goto cleanup; } }
NAK (though it seems this was already committed-- I apologize I missed this when it was initially discussed). The logic before was if fs->readonly was set, add an 'r' rule (which happened to also add a 'deny w' rule) and if fs->readonly is not set, add an 'rw' rule. This patch unconditionally changes that logic and adds a 'rw' regardless of fs->readonly which is wrong. Instead, adjust vah_add_path() to conditionally not add the deny rule. One way to do this is use 'R' for skipping the explicit deny write rule: static int vah_add_path(virBufferPtr buf, ...) { ... /*bool readonly = true;*/ bool explicit_deny_write = true; ... if (strchr(perms, 'w') != NULL || strchr(perms, 'R') != NULL) explicit_deny_write = false; ... /*if (readonly) {*/ if (explicit_deny_write) { virBufferAddLit(buf, " # don't audit writes to readonly files\n"); virBufferAsprintf(buf, " deny \"%s%s\" w,\n", tmp, recursive ? "/**" : ""); } ... } ... static int get_files(vahControl * ctl) { ... /* We don't need explicit deny write rules for readonly mounts, * this can only lead to troubles when mounting / readonly. */ if (vah_add_path(&buf, fs->src, fs->readonly ? "R" : "rw", true) != 0) goto cleanup; I left out the bit about converting the 'R' in perms to 'r' before adding the rule for clarity of approach but this would have to be done too since 'R' isn't a valid permission in apparmor. Also, with this change it would be best to add a comment before the vah_add_path() function to say what valid values of perms are and that 'R' is special. There are certainly other ways of conditionally adding the deny rule. -- Jamie Strandboge http://www.ubuntu.com/
Hi, On Tue, Nov 17, 2015 at 03:14:51PM +0100, Cédric Bosdonnat wrote:
There is no need to deny writes on a readonly mount: write still won't be accepted, even if the user remounts the folder as RW in the guest as qemu sets the 9p mount as ro.
Wouldn't a security whole in qemu possibly allow to circumvent this and isn't this type of exploit the thing we want to guard against in the apparmor proiles?
This deny rule was leading to problems for example with readonly /: The qemu process had to write to a bunch of files in / like logs, sockets, etc. This deny rule was also preventing auditing of these denials, making it harder to debug.
So you're mapping a host directory as '/' into the guest or what was the exact setup? Cheers, -- Guido
--- src/security/virt-aa-helper.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 5de56e5..a2d7226 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1127,7 +1127,10 @@ get_files(vahControl * ctl) ctl->def->fss[i]->src) { virDomainFSDefPtr fs = ctl->def->fss[i];
- if (vah_add_path(&buf, fs->src, fs->readonly ? "r" : "rw", true) != 0) + /* We don't need to add deny rw rules for readonly mounts, + * this can only lead to troubles when mounting / readonly. + */ + if (vah_add_path(&buf, fs->src, "rw", true) != 0) goto cleanup; } } -- 2.1.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, 2015-11-26 at 15:42 +0100, Guido Günther wrote:
Hi, On Tue, Nov 17, 2015 at 03:14:51PM +0100, Cédric Bosdonnat wrote:
There is no need to deny writes on a readonly mount: write still won't be accepted, even if the user remounts the folder as RW in the guest as qemu sets the 9p mount as ro.
Wouldn't a security whole in qemu possibly allow to circumvent this and isn't this type of exploit the thing we want to guard against in the apparmor proiles?
This deny rule was leading to problems for example with readonly /: The qemu process had to write to a bunch of files in / like logs, sockets, etc. This deny rule was also preventing auditing of these denials, making it harder to debug.
So you're mapping a host directory as '/' into the guest or what was the exact setup?
Yes, `virt-sandbox /bin/sh` will readonly mount the host / as / in the guest. This will result in a 'deny /** w' rule that prevents writing to several files. As the deny rules have precedence over the allow ones, this rule will be the one applied for the logs and other files we need to write to. -- Cedric
Cheers, -- Guido
--- src/security/virt-aa-helper.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/src/security/virt-aa-helper.c b/src/security/virt-aa-helper.c index 5de56e5..a2d7226 100644 --- a/src/security/virt-aa-helper.c +++ b/src/security/virt-aa-helper.c @@ -1127,7 +1127,10 @@ get_files(vahControl * ctl) ctl->def->fss[i]->src) { virDomainFSDefPtr fs = ctl->def->fss[i];
- if (vah_add_path(&buf, fs->src, fs->readonly ? "r" : "rw", true) != 0) + /* We don't need to add deny rw rules for readonly mounts, + * this can only lead to troubles when mounting / readonly. + */ + if (vah_add_path(&buf, fs->src, "rw", true) != 0) goto cleanup; } } -- 2.1.4
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Nov 26, 2015 at 04:02:03PM +0100, Cedric Bosdonnat wrote:
On Thu, 2015-11-26 at 15:42 +0100, Guido Günther wrote:
Hi, On Tue, Nov 17, 2015 at 03:14:51PM +0100, Cédric Bosdonnat wrote:
There is no need to deny writes on a readonly mount: write still won't be accepted, even if the user remounts the folder as RW in the guest as qemu sets the 9p mount as ro.
Wouldn't a security whole in qemu possibly allow to circumvent this and isn't this type of exploit the thing we want to guard against in the apparmor proiles?
This deny rule was leading to problems for example with readonly /: The qemu process had to write to a bunch of files in / like logs, sockets, etc. This deny rule was also preventing auditing of these denials, making it harder to debug.
So you're mapping a host directory as '/' into the guest or what was the exact setup?
Yes, `virt-sandbox /bin/sh` will readonly mount the host / as / in the guest. This will result in a 'deny /** w' rule that prevents writing to several files. As the deny rules have precedence over the allow ones, this rule will be the one applied for the logs and other files we need to write to.
I see. Since I don't see any other nice solution mild "ACK" since I'm not a apparmor expert. Cheers, -- Guido
participants (5)
-
Cedric Bosdonnat -
Cédric Bosdonnat
-
Daniel P. Berrange -
Guido Günther -
Jamie Strandboge