[libvirt] [PATCH 0/4] Fix private /dev file access issues
Several heads were banged against several walls while trying to figure this out. No animals were harmed in the writing of this series, though. Andrea Bolognani (4): configure: Move ACL checks to a separate file configure: Make ACL mandatory when building the QEMU driver spec: Require libacl and its headers qemu: Call chmod() after mknod() configure.ac | 12 +++--------- libvirt.spec.in | 4 ++++ m4/virt-acl.m4 | 39 +++++++++++++++++++++++++++++++++++++++ src/qemu/qemu_domain.c | 10 ++++++++++ 4 files changed, 56 insertions(+), 9 deletions(-) create mode 100644 m4/virt-acl.m4 -- 2.7.4
--- configure.ac | 12 +++--------- m4/virt-acl.m4 | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 9 deletions(-) create mode 100644 m4/virt-acl.m4 diff --git a/configure.ac b/configure.ac index a995a05..ed49daa 100644 --- a/configure.ac +++ b/configure.ac @@ -271,6 +271,7 @@ LIBVIRT_ARG_VIRTUALPORT LIBVIRT_ARG_WIRESHARK LIBVIRT_ARG_YAJL +LIBVIRT_CHECK_ACL LIBVIRT_CHECK_APPARMOR LIBVIRT_CHECK_ATOMIC LIBVIRT_CHECK_ATTR @@ -325,19 +326,11 @@ dnl Availability of various common headers (non-fatal if missing). AC_CHECK_HEADERS([pwd.h regex.h sys/un.h \ sys/poll.h syslog.h mntent.h net/ethernet.h linux/magic.h \ sys/un.h sys/syscall.h sys/sysctl.h netinet/tcp.h ifaddrs.h \ - libtasn1.h sys/ucred.h sys/mount.h sys/acl.h]) + libtasn1.h sys/ucred.h sys/mount.h]) dnl Check whether endian provides handy macros. AC_CHECK_DECLS([htole64], [], [], [[#include <endian.h>]]) AC_CHECK_FUNCS([stat stat64 __xstat __xstat64 lstat lstat64 __lxstat __lxstat64]) -ACL_CFLAGS="" -ACL_LIBS="" -if test "x$ac_cv_header_sys_acl_h:x$with_linux" = "xyes:xyes" ; then - ACL_LIBS="-lacl" -fi -AC_SUBST([ACL_CFLAGS]) -AC_SUBST([ACL_LIBS]) - AC_CHECK_TYPE([struct ifreq], [AC_DEFINE([HAVE_STRUCT_IFREQ],[1], [Defined if struct ifreq exists in net/if.h])], @@ -951,6 +944,7 @@ LIBVIRT_RESULT_DRIVER_MODULES AC_MSG_NOTICE([]) AC_MSG_NOTICE([Libraries]) AC_MSG_NOTICE([]) +LIBVIRT_RESULT_ACL LIBVIRT_RESULT_APPARMOR LIBVIRT_RESULT_ATTR LIBVIRT_RESULT_AUDIT diff --git a/m4/virt-acl.m4 b/m4/virt-acl.m4 new file mode 100644 index 0000000..f7d1c6d --- /dev/null +++ b/m4/virt-acl.m4 @@ -0,0 +1,35 @@ +dnl ACL support +dnl +dnl Copyright (C) 2017 Red Hat, Inc. +dnl +dnl This library is free software; you can redistribute it and/or +dnl modify it under the terms of the GNU Lesser General Public +dnl License as published by the Free Software Foundation; either +dnl version 2.1 of the License, or (at your option) any later version. +dnl +dnl This library is distributed in the hope that it will be useful, +dnl but WITHOUT ANY WARRANTY; without even the implied warranty of +dnl MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +dnl Lesser General Public License for more details. +dnl +dnl You should have received a copy of the GNU Lesser General Public +dnl License along with this library. If not, see +dnl <http://www.gnu.org/licenses/>. +dnl + +AC_DEFUN([LIBVIRT_CHECK_ACL], [ + + AC_CHECK_HEADERS([sys/acl.h]) + + ACL_CFLAGS="" + ACL_LIBS="" + if test "x$ac_cv_header_sys_acl_h:x$with_linux" = "xyes:xyes"; then + ACL_LIBS="-lacl" + fi + AC_SUBST([ACL_CFLAGS]) + AC_SUBST([ACL_LIBS]) +]) + +AC_DEFUN([LIBVIRT_RESULT_ACL], [ + LIBVIRT_RESULT_LIB([ACL]) +]) -- 2.7.4
On 02/14/2017 04:33 PM, Andrea Bolognani wrote:
--- configure.ac | 12 +++--------- m4/virt-acl.m4 | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 9 deletions(-) create mode 100644 m4/virt-acl.m4
ACK Michal
When we're building a private /dev for the isolated QEMU process, we want to be able to replicate the contents of the original /dev as closely as possible, including ACLs. To ensure that's always possible, make ACL support mandatory when the QEMU driver is enabled. Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1421036 --- m4/virt-acl.m4 | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/m4/virt-acl.m4 b/m4/virt-acl.m4 index f7d1c6d..7a8b8e5 100644 --- a/m4/virt-acl.m4 +++ b/m4/virt-acl.m4 @@ -21,6 +21,10 @@ AC_DEFUN([LIBVIRT_CHECK_ACL], [ AC_CHECK_HEADERS([sys/acl.h]) + if test "x$ac_cv_header_sys_acl_h:x$with_qemu" = "xno:xyes"; then + AC_MSG_ERROR([Unable to find <sys/acl.h>, required by qemu driver]) + fi + ACL_CFLAGS="" ACL_LIBS="" if test "x$ac_cv_header_sys_acl_h:x$with_linux" = "xyes:xyes"; then -- 2.7.4
On Tue, Feb 14, 2017 at 04:33:09PM +0100, Andrea Bolognani wrote:
When we're building a private /dev for the isolated QEMU process, we want to be able to replicate the contents of the original /dev as closely as possible, including ACLs.
To ensure that's always possible, make ACL support mandatory when the QEMU driver is enabled.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1421036 --- m4/virt-acl.m4 | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/m4/virt-acl.m4 b/m4/virt-acl.m4 index f7d1c6d..7a8b8e5 100644 --- a/m4/virt-acl.m4 +++ b/m4/virt-acl.m4 @@ -21,6 +21,10 @@ AC_DEFUN([LIBVIRT_CHECK_ACL], [
AC_CHECK_HEADERS([sys/acl.h])
+ if test "x$ac_cv_header_sys_acl_h:x$with_qemu" = "xno:xyes"; then + AC_MSG_ERROR([Unable to find <sys/acl.h>, required by qemu driver]) + fi
I understand the desire to simplify the code by assuming the libacl APIs always exist, but we shouldn't do it this way. Instead we should follow our normal practice of creating a src/util/viracl.{c,h} file which wrap the APIs and providing no-op stubs which just raise an error when the library is missing at build time. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
On 02/14/2017 04:40 PM, Daniel P. Berrange wrote:
On Tue, Feb 14, 2017 at 04:33:09PM +0100, Andrea Bolognani wrote:
When we're building a private /dev for the isolated QEMU process, we want to be able to replicate the contents of the original /dev as closely as possible, including ACLs.
To ensure that's always possible, make ACL support mandatory when the QEMU driver is enabled.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1421036 --- m4/virt-acl.m4 | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/m4/virt-acl.m4 b/m4/virt-acl.m4 index f7d1c6d..7a8b8e5 100644 --- a/m4/virt-acl.m4 +++ b/m4/virt-acl.m4 @@ -21,6 +21,10 @@ AC_DEFUN([LIBVIRT_CHECK_ACL], [
AC_CHECK_HEADERS([sys/acl.h])
+ if test "x$ac_cv_header_sys_acl_h:x$with_qemu" = "xno:xyes"; then + AC_MSG_ERROR([Unable to find <sys/acl.h>, required by qemu driver]) + fi
I understand the desire to simplify the code by assuming the libacl APIs always exist, but we shouldn't do it this way. Instead we should follow our normal practice of creating a src/util/viracl.{c,h} file which wrap the APIs and providing no-op stubs which just raise an error when the library is missing at build time.
Unfortunately, it's more complicated than that. What if your distro has getfacl/setfacl but missing the header files (libacl-devel)? In that case libvirt is built without ACL support. Now imagine you have a device, say /dev/kvm with 0777 and ACL entry that denies specific group/user. If we build without ACL support, and use namespaces we will in fact allow that group/user to start a domain because we just create a device with 0777 perms and do not copy ACL entries. Or vice versa - I explicitly allow user under which I'm running in ACLs to /dev/kvm while it's perms are 0660. Lacking libacl headers would render me unable to use namespaces. Michal
On Tue, Feb 14, 2017 at 04:55:05PM +0100, Michal Privoznik wrote:
On 02/14/2017 04:40 PM, Daniel P. Berrange wrote:
On Tue, Feb 14, 2017 at 04:33:09PM +0100, Andrea Bolognani wrote:
When we're building a private /dev for the isolated QEMU process, we want to be able to replicate the contents of the original /dev as closely as possible, including ACLs.
To ensure that's always possible, make ACL support mandatory when the QEMU driver is enabled.
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1421036 --- m4/virt-acl.m4 | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/m4/virt-acl.m4 b/m4/virt-acl.m4 index f7d1c6d..7a8b8e5 100644 --- a/m4/virt-acl.m4 +++ b/m4/virt-acl.m4 @@ -21,6 +21,10 @@ AC_DEFUN([LIBVIRT_CHECK_ACL], [
AC_CHECK_HEADERS([sys/acl.h])
+ if test "x$ac_cv_header_sys_acl_h:x$with_qemu" = "xno:xyes"; then + AC_MSG_ERROR([Unable to find <sys/acl.h>, required by qemu driver]) + fi
I understand the desire to simplify the code by assuming the libacl APIs always exist, but we shouldn't do it this way. Instead we should follow our normal practice of creating a src/util/viracl.{c,h} file which wrap the APIs and providing no-op stubs which just raise an error when the library is missing at build time.
Unfortunately, it's more complicated than that. What if your distro has getfacl/setfacl but missing the header files (libacl-devel)? In that case libvirt is built without ACL support. Now imagine you have a device, say /dev/kvm with 0777 and ACL entry that denies specific group/user. If we build without ACL support, and use namespaces we will in fact allow that group/user to start a domain because we just create a device with 0777 perms and do not copy ACL entries.
Or vice versa - I explicitly allow user under which I'm running in ACLs to /dev/kvm while it's perms are 0660. Lacking libacl headers would render me unable to use namespaces.
Your platform has libacl available so it is not difficult to fix that by building with libacl support. We print out the configure summary precisely so users can see if there's any libraries they forgot to install which might be useful. Mandating libacl will prevent use of the QEMU driver on platforms lacking the libacl library. I see various bug reports indicating portability problems for libacl wrt other platforms, in particular OS-X. So IMHO it is not acceptable to make it a mandatory requirement, when there's no good reason for that aside beyond helping people who forget to install -devel library packages Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
On Tue, 2017-02-14 at 16:00 +0000, Daniel P. Berrange wrote:
Your platform has libacl available so it is not difficult to fix that by building with libacl support. We print out the configure summary precisely so users can see if there's any libraries they forgot to install which might be useful. Mandating libacl will prevent use of the QEMU driver on platforms lacking the libacl library. I see various bug reports indicating portability problems for libacl wrt other platforms, in particular OS-X. So IMHO it is not acceptable to make it a mandatory requirement, when there's no good reason for that aside beyond helping people who forget to install -devel library packages
FreeBSD and other have <sys/acl.h>, it's just not part of a separate package but of libc itself. Not sure about macOS, but it being a BSD derivative I expect it wouldn't be too different. On the other hand, we really only care about having the ACL APIs when we are isolating QEMU, which only happens of Linux due to the namespaces requirement... So maybe we could have it as a strict requirement on Linux only, and as an optional dependency on other platforms? -- Andrea Bolognani / Red Hat / Virtualization
On Tue, Feb 14, 2017 at 05:14:33PM +0100, Andrea Bolognani wrote:
On Tue, 2017-02-14 at 16:00 +0000, Daniel P. Berrange wrote:
Your platform has libacl available so it is not difficult to fix that by building with libacl support. We print out the configure summary precisely so users can see if there's any libraries they forgot to install which might be useful. Mandating libacl will prevent use of the QEMU driver on platforms lacking the libacl library. I see various bug reports indicating portability problems for libacl wrt other platforms, in particular OS-X. So IMHO it is not acceptable to make it a mandatory requirement, when there's no good reason for that aside beyond helping people who forget to install -devel library packages
FreeBSD and other have <sys/acl.h>, it's just not part of a separate package but of libc itself. Not sure about macOS, but it being a BSD derivative I expect it wouldn't be too different.
On the other hand, we really only care about having the ACL APIs when we are isolating QEMU, which only happens of Linux due to the namespaces requirement... So maybe we could have it as a strict requirement on Linux only, and as an optional dependency on other platforms?
IMHO it'd be better to just disable the namespace code at build time if we don't have libacl rather than adding mandatory build deps. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
On Tue, 2017-02-14 at 16:20 +0000, Daniel P. Berrange wrote:
On the other hand, we really only care about having the ACL APIs when we are isolating QEMU, which only happens of Linux due to the namespaces requirement... So maybe we could have it as a strict requirement on Linux only, and as an optional dependency on other platforms? IMHO it'd be better to just disable the namespace code at build time if we don't have libacl rather than adding mandatory build deps.
I'm afraid that might lead to people forgetting to install libacl-devel[1] on Linux and ending up with less security than expected / desired as a result. Moreover, we're talking about a package which is literally 35k in size: I would be way more inclined to pay the price in increased code complexity if we were not dealing with what will basically end up as a rounding error on any reasonable hypervisor host. Not to mention systemd depends on it, so it will be part of the core package set on most modern Linux distributions. [1] I know I did while trying to figure this bug out ;) -- Andrea Bolognani / Red Hat / Virtualization
On Tue, Feb 14, 2017 at 05:47:27PM +0100, Andrea Bolognani wrote:
On Tue, 2017-02-14 at 16:20 +0000, Daniel P. Berrange wrote:
On the other hand, we really only care about having the ACL APIs when we are isolating QEMU, which only happens of Linux due to the namespaces requirement... So maybe we could have it as a strict requirement on Linux only, and as an optional dependency on other platforms? IMHO it'd be better to just disable the namespace code at build time if we don't have libacl rather than adding mandatory build deps.
I'm afraid that might lead to people forgetting to install libacl-devel[1] on Linux and ending up with less security than expected / desired as a result.
You can make the same argument about many other libraries we have optional dependancies against, libcapng, libselinux, apparmour, etc. Our general policy is for libraries to be optional and I don't see a reason for this to be a different case
[1] I know I did while trying to figure this bug out ;)
If we disabled namespace support when libacl is missing at build time you would have noticed quite quickly that you weren't using namespaces. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
On Tue, 2017-02-14 at 16:59 +0000, Daniel P. Berrange wrote:
[1] I know I did while trying to figure this bug out ;) If we disabled namespace support when libacl is missing at build time you would have noticed quite quickly that you weren't using namespaces.
Not sure how quickly you'd notice that, it's not like we would print a big fat warning either at configure time or when starting a guest, so unless you were looking for it specifically it would be entirely silent: the guest would start anyway, it just wouldn't be isolated. But okay, let's leave making sure all optional dependencies are in place to users, and most importantly distributions. As you say, it's consistent with how we deal with other external libraries. Michal, can you please take care of cooking up a patch to disable namespaces when ACL support is not available? -- Andrea Bolognani / Red Hat / Virtualization
Now that they're required by configure, we should make sure they are available when building the rpm and when installing it. --- libvirt.spec.in | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/libvirt.spec.in b/libvirt.spec.in index e8c272b..ca569e3 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -340,6 +340,8 @@ BuildRequires: polkit-devel >= 0.93 # For mount/umount in FS driver BuildRequires: util-linux %if %{with_qemu} +# For managing ACLs +BuildRequires: libacl-devel # From QEMU RPMs BuildRequires: /usr/bin/qemu-img %else @@ -645,6 +647,8 @@ Requires: xz %if 0%{?fedora} >= 24 Requires: systemd-container %endif +# For managing ACLs +Requires: libacl %description daemon-driver-qemu The qemu driver plugin for the libvirtd daemon, providing -- 2.7.4
On 02/14/2017 04:33 PM, Andrea Bolognani wrote:
Now that they're required by configure, we should make sure they are available when building the rpm and when installing it. --- libvirt.spec.in | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/libvirt.spec.in b/libvirt.spec.in index e8c272b..ca569e3 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -340,6 +340,8 @@ BuildRequires: polkit-devel >= 0.93 # For mount/umount in FS driver BuildRequires: util-linux %if %{with_qemu} +# For managing ACLs +BuildRequires: libacl-devel # From QEMU RPMs BuildRequires: /usr/bin/qemu-img %else @@ -645,6 +647,8 @@ Requires: xz %if 0%{?fedora} >= 24 Requires: systemd-container %endif +# For managing ACLs +Requires: libacl
This shouldn't be needed. If we are building with libacl-devel, we are also linking with libacl and thus rpm will put it as a requirement. ACK if you drop this hunk. Michal
On Tue, 2017-02-14 at 17:39 +0100, Michal Privoznik wrote:
@@ -645,6 +647,8 @@ Requires: xz %if 0%{?fedora} >= 24 Requires: systemd-container %endif +# For managing ACLs +Requires: libacl
This shouldn't be needed. If we are building with libacl-devel, we are also linking with libacl and thus rpm will put it as a requirement.
So it does! Looks like I learned a new trick today :)
ACK if you drop this hunk.
I've pushed this after dropping the hunk and tweaking the commit message a bit. Same with the other patches in the series, except of course for 2/4. Thanks! -- Andrea Bolognani / Red Hat / Virtualization
mknod() is affected my the current umask, so we're not guaranteed the newly-created device node will have the right permissions. Call chmod(), which is not affected by the current umask, immediately afterwards to solve the issue. --- src/qemu/qemu_domain.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index f62bf8f..3adec5c 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -7152,6 +7152,16 @@ qemuDomainCreateDeviceRecursive(const char *device, } goto cleanup; } + + /* Set the file permissions again: mknod() is affected by the + * current umask, and as such might not have set them correctly */ + if (create && + chmod(devicePath, sb.st_mode) < 0) { + virReportSystemError(errno, + _("Failed to set permissions for device %s"), + devicePath); + goto cleanup; + } } if (!create) { -- 2.7.4
On 02/14/2017 04:33 PM, Andrea Bolognani wrote:
mknod() is affected my the current umask, so we're not guaranteed the newly-created device node will have the right permissions.
Call chmod(), which is not affected by the current umask, immediately afterwards to solve the issue. --- src/qemu/qemu_domain.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
ACK Michal
participants (3)
-
Andrea Bolognani -
Daniel P. Berrange -
Michal Privoznik