8:14 a.m.
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
8:14 a.m.
New subject: [libvirt] [PATCH 1/3] Allow building lxc without virt-login-shell
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
8:23 a.m.
New subject: [libvirt] [PATCH 1/3] Allow building lxc without virt-login-shell
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 :|
8:14 a.m.
New subject: [libvirt] [PATCH 2/3] virt-aa-helper: don't deny writes to readonly mounts
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
2:18 a.m.
New subject: [libvirt] [PATCH 2/3] virt-aa-helper: don't deny writes to readonly mounts
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;
}
}
4:50 p.m.
New subject: [libvirt] [PATCH 2/3] virt-aa-helper: don't deny writes to readonly mounts
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/
8:42 a.m.
New subject: [libvirt] [PATCH 2/3] virt-aa-helper: don't deny writes to readonly mounts
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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
9:02 a.m.
New subject: [libvirt] [PATCH 2/3] virt-aa-helper: don't deny writes to readonly mounts
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(a)redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
>
11:25 a.m.
New subject: [libvirt] [PATCH 2/3] virt-aa-helper: don't deny writes to readonly mounts
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
3233
days inactive
3292
days old
8 comments
5 participants
participants (5)
-
Cedric Bosdonnat -
Cédric Bosdonnat
-
Daniel P. Berrange -
Guido Günther -
Jamie Strandboge