[libvirt] [PATCH 00/14] Fix flaw in virt-login-shell CVE-2013-4400
From: "Daniel P. Berrange" <berrange@redhat.com> The following series of patches have been pushed to master as a fix for CVE-2013-4400. The first four patches are the core fix. The remaining 10 patches are preventative measures to help avoid further problems in the future. I will be pushing at least the first 4 patches to stable branches and any of the further patches if I find they apply without merge problems. Daniel P. Berrange (14): Add helpers for getting env vars in a setuid environment Only allow 'stderr' log output when running setuid (CVE-2013-4400) Close all non-stdio FDs in virt-login-shell (CVE-2013-4400) Don't link virt-login-shell against libvirt.so (CVE-2013-4400) Set a sane $PATH for virt-login-shell Make virCommand env handling robust in setuid env Remove all direct use of getenv Block all use of getenv with syntax-check Only allow the UNIX transport in remote driver when setuid Don't allow remote driver daemon autostart when running setuid Add stub getegid impl for platforms lacking it Remove (nearly) all use of getuid()/getgid() Block all use of libvirt.so in setuid programs Move virt-login-shell into libvirt-login-shell sub-RPM Makefile.am | 1 + bootstrap.conf | 1 + cfg.mk | 8 ++++ config-post.h | 44 ++++++++++++++++++ configure.ac | 1 + daemon/Makefile.am | 1 + daemon/libvirtd.c | 2 +- examples/domain-events/events-c/Makefile.am | 3 +- examples/hellolibvirt/Makefile.am | 2 +- examples/openauth/Makefile.am | 2 +- gnulib/lib/Makefile.am | 2 +- libvirt.spec.in | 28 +++++++---- python/Makefile.am | 1 + src/Makefile.am | 72 +++++++++++++++++++++++++++++ src/driver.c | 3 +- src/libvirt.c | 54 +++++++++++++++------- src/libvirt_private.syms | 6 ++- src/locking/lock_daemon.c | 6 +-- src/locking/lock_driver_lockd.c | 6 +-- src/locking/lock_manager.c | 2 +- src/lxc/lxc_controller.c | 2 +- src/lxc/lxc_driver.c | 4 +- src/lxc/lxc_process.c | 2 +- src/qemu/qemu_command.c | 8 ++-- src/qemu/qemu_driver.c | 6 +-- src/remote/remote_driver.c | 37 +++++++++++---- src/rpc/virnetsocket.c | 16 +++---- src/rpc/virnettlscontext.c | 4 +- src/storage/storage_backend.c | 4 +- src/storage/storage_backend_fs.c | 4 +- src/storage/storage_backend_logical.c | 2 +- src/util/virauth.c | 2 +- src/util/vircommand.c | 50 +++++++++++++++----- src/util/vircommand.h | 8 +++- src/util/virfile.c | 23 +++++---- src/util/viridentity.c | 8 ++-- src/util/virlog.c | 18 ++++++-- src/util/virrandom.c | 2 +- src/util/virstoragefile.c | 2 +- src/util/virutil.c | 47 +++++++++++++++++-- src/util/virutil.h | 8 ++++ src/vbox/vbox_XPCOMCGlue.c | 2 +- src/vbox/vbox_driver.c | 2 +- src/vbox/vbox_tmpl.c | 6 +-- tests/commandtest.c | 8 ++-- tests/qemumonitortestutils.c | 2 +- tests/virnetsockettest.c | 4 +- tools/Makefile.am | 9 +++- tools/virsh.c | 18 ++++---- tools/virt-login-shell.c | 14 ++++++ 50 files changed, 430 insertions(+), 137 deletions(-) create mode 100644 config-post.h -- 1.8.3.1
From: "Daniel P. Berrange" <berrange@redhat.com> Care must be taken accessing env variables when running setuid. Introduce a virGetEnvAllowSUID for env vars which are safe to use in a setuid environment, and another virGetEnvBlockSUID for vars which are not safe. Also add a virIsSUID helper method for any other non-env var code to use. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- bootstrap.conf | 1 + src/libvirt_private.syms | 3 +++ src/util/virutil.c | 39 +++++++++++++++++++++++++++++++++++++++ src/util/virutil.h | 4 ++++ 4 files changed, 47 insertions(+) diff --git a/bootstrap.conf b/bootstrap.conf index 8b37217..d24a714 100644 --- a/bootstrap.conf +++ b/bootstrap.conf @@ -93,6 +93,7 @@ recv regex random_r sched +secure_getenv send setenv setsockopt diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 84c1c28..9be6f32 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1859,6 +1859,8 @@ virFindFCHostCapableVport; virFormatIntDecimal; virGetDeviceID; virGetDeviceUnprivSGIO; +virGetEnvAllowSUID; +virGetEnvBlockSUID; virGetFCHostNameByWWN; virGetGroupID; virGetGroupList; @@ -1877,6 +1879,7 @@ virIndexToDiskName; virIsCapableFCHost; virIsCapableVport; virIsDevMapperDevice; +virIsSUID; virManageVport; virParseNumber; virParseOwnershipIds; diff --git a/src/util/virutil.c b/src/util/virutil.c index 854c0ad..7e7c1c2 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -2131,3 +2131,42 @@ cleanup: return rc; } + + +/** + * virGetEnvBlockSUID: + * @name: the environment variable name + * + * Obtain an environment variable which is unsafe to + * use when running setuid. If running setuid, a NULL + * value will be returned + */ +const char *virGetEnvBlockSUID(const char *name) +{ + return secure_getenv(name); +} + + +/** + * virGetEnvBlockSUID: + * @name: the environment variable name + * + * Obtain an environment variable which is safe to + * use when running setuid. The value will be returned + * even when running setuid + */ +const char *virGetEnvAllowSUID(const char *name) +{ + return getenv(name); +} + + +/** + * virIsSUID: + * Return a true value if running setuid. Does not + * check for elevated capabilities bits. + */ +bool virIsSUID(void) +{ + return getuid() != geteuid(); +} diff --git a/src/util/virutil.h b/src/util/virutil.h index 9f6fb20..e8765b2 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -172,4 +172,8 @@ int virCompareLimitUlong(unsigned long long a, unsigned long long b); int virParseOwnershipIds(const char *label, uid_t *uidPtr, gid_t *gidPtr); +const char *virGetEnvBlockSUID(const char *name); +const char *virGetEnvAllowSUID(const char *name); +bool virIsSUID(void); + #endif /* __VIR_UTIL_H__ */ -- 1.8.3.1
From: "Daniel P. Berrange" <berrange@redhat.com> We must not allow file/syslog/journald log outputs when running setuid since they can be abused to do bad things. In particular the 'file' output can be used to overwrite files. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/virlog.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/src/util/virlog.c b/src/util/virlog.c index 8a688fd..997d582 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1324,6 +1324,9 @@ int virLogPriorityFromSyslog(int priority ATTRIBUTE_UNUSED) * Multiple output can be defined in a single @output, they just need to be * separated by spaces. * + * If running in setuid mode, then only the 'stderr' output will + * be allowed + * * Returns the number of output parsed and installed or -1 in case of error */ int @@ -1335,6 +1338,7 @@ virLogParseOutputs(const char *outputs) virLogPriority prio; int ret = -1; int count = 0; + bool isSUID = virIsSUID(); if (cur == NULL) return -1; @@ -1354,6 +1358,8 @@ virLogParseOutputs(const char *outputs) if (virLogAddOutputToStderr(prio) == 0) count++; } else if (STREQLEN(cur, "syslog", 6)) { + if (isSUID) + goto cleanup; cur += 6; if (*cur != ':') goto cleanup; @@ -1371,6 +1377,8 @@ virLogParseOutputs(const char *outputs) VIR_FREE(name); #endif /* HAVE_SYSLOG_H */ } else if (STREQLEN(cur, "file", 4)) { + if (isSUID) + goto cleanup; cur += 4; if (*cur != ':') goto cleanup; @@ -1391,6 +1399,8 @@ virLogParseOutputs(const char *outputs) VIR_FREE(name); VIR_FREE(abspath); } else if (STREQLEN(cur, "journald", 8)) { + if (isSUID) + goto cleanup; cur += 8; #if USE_JOURNALD if (virLogAddOutputToJournald(prio) == 0) -- 1.8.3.1
From: "Daniel P. Berrange" <berrange@redhat.com> We don't want to inherit any FDs in the new namespace except for the stdio FDs. Explicitly close them all, just in case some do not have the close-on-exec flag set. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tools/virt-login-shell.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tools/virt-login-shell.c b/tools/virt-login-shell.c index c754ae4..0196950 100644 --- a/tools/virt-login-shell.c +++ b/tools/virt-login-shell.c @@ -313,6 +313,18 @@ main(int argc, char **argv) if (cpid == 0) { pid_t ccpid; + int openmax = sysconf(_SC_OPEN_MAX); + int fd; + if (openmax < 0) { + virReportSystemError(errno, "%s", + _("sysconf(_SC_OPEN_MAX) failed")); + return EXIT_FAILURE; + } + for (fd = 3; fd < openmax; fd++) { + int tmpfd = fd; + VIR_MASS_CLOSE(tmpfd); + } + /* Fork once because we don't want to affect * virt-login-shell's namespace itself */ -- 1.8.3.1
On 10/21/2013 07:12 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
We don't want to inherit any FDs in the new namespace except for the stdio FDs. Explicitly close them all, just in case some do not have the close-on-exec flag set.
Not only did this fix the CVE, it made virt-login-shell completely useless. :( Is anyone actually using this program?
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tools/virt-login-shell.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)
diff --git a/tools/virt-login-shell.c b/tools/virt-login-shell.c index c754ae4..0196950 100644 --- a/tools/virt-login-shell.c +++ b/tools/virt-login-shell.c @@ -313,6 +313,18 @@ main(int argc, char **argv)
Adding some additional context: if ((nfdlist = virDomainLxcOpenNamespace(dom, &fdlist, 0)) < 0) goto cleanup; ...
if (cpid == 0) { pid_t ccpid;
+ int openmax = sysconf(_SC_OPEN_MAX); + int fd; + if (openmax < 0) { + virReportSystemError(errno, "%s", + _("sysconf(_SC_OPEN_MAX) failed")); + return EXIT_FAILURE; + } + for (fd = 3; fd < openmax; fd++) { + int tmpfd = fd; + VIR_MASS_CLOSE(tmpfd); + } + /* Fork once because we don't want to affect * virt-login-shell's namespace itself */
and more context: if (nfdlist > 0) { if (virDomainLxcEnterNamespace(dom, nfdlist, fdlist, NULL, NULL, 0) < 0) Oops. We just closed every fd larger than stderr, which means fdlist is now useless, and virDomainLxcEnterNamespace is guaranteed to fail, which means we are guaranteed to not switch namespaces. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> The libvirt.so library has far too many library deps to allow linking against it from setuid programs. Those libraries can do stuff in __attribute__((constructor) functions which is not setuid safe. The virt-login-shell needs to link directly against individual files that it uses, with all library deps turned off except for libxml2 and libselinux. Create a libvirt-setuid-rpc-client.la library which is linked to by virt-login-shell. A config-post.h file allows this library to disable all external deps except libselinux and libxml2. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- Makefile.am | 1 + config-post.h | 44 ++++++++++++++++++ configure.ac | 1 + daemon/Makefile.am | 1 + examples/domain-events/events-c/Makefile.am | 3 +- examples/hellolibvirt/Makefile.am | 2 +- examples/openauth/Makefile.am | 2 +- gnulib/lib/Makefile.am | 2 +- python/Makefile.am | 1 + src/Makefile.am | 72 +++++++++++++++++++++++++++++ src/libvirt.c | 36 +++++++++------ tools/Makefile.am | 9 +++- 12 files changed, 153 insertions(+), 21 deletions(-) create mode 100644 config-post.h diff --git a/Makefile.am b/Makefile.am index f3b5cd2..192a378 100644 --- a/Makefile.am +++ b/Makefile.am @@ -31,6 +31,7 @@ XML_EXAMPLES = \ test/*.xml storage/*.xml))) EXTRA_DIST = \ + config-post.h \ ChangeLog-old \ libvirt.spec libvirt.spec.in \ mingw-libvirt.spec.in \ diff --git a/config-post.h b/config-post.h new file mode 100644 index 0000000..d371e8c --- /dev/null +++ b/config-post.h @@ -0,0 +1,44 @@ +/* + * Copyright (C) 2013 Red Hat, Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2.1 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library. If not, see + * <http://www.gnu.org/licenses/>. + */ + +/* + * Since virt-login-shell will be setuid, we must do everything + * we can to avoid linking to other libraries. Many of them do + * unsafe things in functions marked __atttribute__((constructor)). + * The only way avoid to avoid such deps is to re-compile the + * functions with the code in question disabled, and for that we + * must override the main config.h rules. Hence this file :-( + */ + +#ifdef LIBVIRT_SETUID_RPC_CLIENT +# undef HAVE_LIBDEVMAPPER_H +# undef HAVE_LIBNL +# undef HAVE_LIBNL3 +# undef HAVE_LIBSASL2 +# undef WITH_CAPNG +# undef WITH_CURL +# undef WITH_DTRACE_PROBES +# undef WITH_GNUTLS +# undef WITH_MACVTAP +# undef WITH_NUMACTL +# undef WITH_SASL +# undef WITH_SSH2 +# undef WITH_VIRTUALPORT +# undef WITH_YAJL +# undef WITH_YAJL2 +#endif diff --git a/configure.ac b/configure.ac index 1993fab..1c5b168 100644 --- a/configure.ac +++ b/configure.ac @@ -20,6 +20,7 @@ AC_INIT([libvirt], [1.1.3], [libvir-list@redhat.com], [], [http://libvirt.org]) AC_CONFIG_SRCDIR([src/libvirt.c]) AC_CONFIG_AUX_DIR([build-aux]) AC_CONFIG_HEADERS([config.h]) +AH_BOTTOM([#include <config-post.h>]) AC_CONFIG_MACRO_DIR([m4]) dnl Make automake keep quiet about wildcards & other GNUmake-isms; also keep dnl quiet about the fact that we intentionally cater to automake 1.9 diff --git a/daemon/Makefile.am b/daemon/Makefile.am index 46c766c..e5c5db8 100644 --- a/daemon/Makefile.am +++ b/daemon/Makefile.am @@ -18,6 +18,7 @@ INCLUDES = \ -I$(top_builddir)/gnulib/lib -I$(top_srcdir)/gnulib/lib \ + -I$(top_srcdir) \ -I$(top_builddir)/include -I$(top_srcdir)/include \ -I$(top_builddir)/src -I$(top_srcdir)/src \ -I$(top_srcdir)/src/util \ diff --git a/examples/domain-events/events-c/Makefile.am b/examples/domain-events/events-c/Makefile.am index 0646aee..86500a0 100644 --- a/examples/domain-events/events-c/Makefile.am +++ b/examples/domain-events/events-c/Makefile.am @@ -15,7 +15,8 @@ ## <http://www.gnu.org/licenses/>. INCLUDES = -I$(top_builddir)/include -I$(top_srcdir)/include \ - -I$(top_builddir)/gnulib/lib -I$(top_srcdir)/gnulib/lib + -I$(top_builddir)/gnulib/lib -I$(top_srcdir)/gnulib/lib \ + -I$(top_srcdir) noinst_PROGRAMS = event-test event_test_CFLAGS = $(WARN_CFLAGS) event_test_SOURCES = event-test.c diff --git a/examples/hellolibvirt/Makefile.am b/examples/hellolibvirt/Makefile.am index 060cc71..55ea972 100644 --- a/examples/hellolibvirt/Makefile.am +++ b/examples/hellolibvirt/Makefile.am @@ -14,7 +14,7 @@ ## License along with this library. If not, see ## <http://www.gnu.org/licenses/>. -INCLUDES = -I$(top_builddir)/include -I$(top_srcdir)/include +INCLUDES = -I$(top_builddir)/include -I$(top_srcdir)/include -I$(top_srcdir) noinst_PROGRAMS = hellolibvirt hellolibvirt_CFLAGS = $(WARN_CFLAGS) hellolibvirt_SOURCES = hellolibvirt.c diff --git a/examples/openauth/Makefile.am b/examples/openauth/Makefile.am index 1eb23fc..7bb8604 100644 --- a/examples/openauth/Makefile.am +++ b/examples/openauth/Makefile.am @@ -14,7 +14,7 @@ ## License along with this library. If not, see ## <http://www.gnu.org/licenses/>. -INCLUDES = -I$(top_builddir)/include -I$(top_srcdir)/include +INCLUDES = -I$(top_builddir)/include -I$(top_srcdir)/include -I$(top_srcdir) noinst_PROGRAMS = openauth openauth_CFLAGS = $(WARN_CFLAGS) openauth_SOURCES = openauth.c diff --git a/gnulib/lib/Makefile.am b/gnulib/lib/Makefile.am index e27c658..f098e82 100644 --- a/gnulib/lib/Makefile.am +++ b/gnulib/lib/Makefile.am @@ -27,4 +27,4 @@ noinst_LTLIBRARIES = include gnulib.mk -INCLUDES = $(GETTEXT_CPPFLAGS) +INCLUDES = -I$(top_srcdir) $(GETTEXT_CPPFLAGS) diff --git a/python/Makefile.am b/python/Makefile.am index f327300..c9c2a8b 100644 --- a/python/Makefile.am +++ b/python/Makefile.am @@ -20,6 +20,7 @@ INCLUDES = \ $(PYTHON_INCLUDES) \ -I$(top_builddir)/gnulib/lib \ -I$(top_srcdir)/gnulib/lib \ + -I$(top_srcdir) \ -I$(top_builddir)/src \ -I$(top_srcdir)/src \ -I$(top_srcdir)/src/util \ diff --git a/src/Makefile.am b/src/Makefile.am index a9b6d58..9dab4df 100644 --- a/src/Makefile.am +++ b/src/Makefile.am @@ -21,6 +21,7 @@ # that actually use them. Also keep GETTEXT_CPPFLAGS at the end. INCLUDES = -I../gnulib/lib \ -I$(top_srcdir)/gnulib/lib \ + -I$(top_srcdir) \ -I../include \ -I$(top_srcdir)/include \ -I$(top_srcdir)/src/util \ @@ -1966,6 +1967,77 @@ libvirt_lxc_la_LDFLAGS = \ libvirt_lxc_la_CFLAGS = $(AM_CFLAGS) libvirt_lxc_la_LIBADD = libvirt.la $(CYGWIN_EXTRA_LIBADD) +# Since virt-login-shell will be setuid, we must do everything +# we can to avoid linking to other libraries. Many of them do +# unsafe things in functions marked __atttribute__((constructor)). +# This library is built to include the bare minimum required to +# have a RPC client for local UNIX socket access only. We use +# the ../config-post.h header to disable all external deps that +# we don't want +if WITH_LXC +noinst_LTLIBRARIES += libvirt-setuid-rpc-client.la + +libvirt_setuid_rpc_client_la_SOURCES = \ + util/viralloc.c \ + util/virbitmap.c \ + util/virbuffer.c \ + util/vircommand.c \ + util/virconf.c \ + util/virerror.c \ + util/virevent.c \ + util/vireventpoll.c \ + util/virfile.c \ + util/virhash.c \ + util/virhashcode.c \ + util/virjson.c \ + util/virlog.c \ + util/virobject.c \ + util/virpidfile.c \ + util/virprocess.c \ + util/virrandom.c \ + util/virsocketaddr.c \ + util/virstoragefile.c \ + util/virstring.c \ + util/virtime.c \ + util/virthread.c \ + util/virtypedparam.c \ + util/viruri.c \ + util/virutil.c \ + util/viruuid.c \ + conf/domain_event.c \ + rpc/virnetsocket.c \ + rpc/virnetsocket.h \ + rpc/virnetmessage.h \ + rpc/virnetmessage.c \ + rpc/virkeepalive.c \ + rpc/virkeepalive.h \ + rpc/virnetclient.c \ + rpc/virnetclientprogram.c \ + rpc/virnetclientstream.c \ + rpc/virnetprotocol.c \ + remote/remote_driver.c \ + remote/remote_protocol.c \ + remote/qemu_protocol.c \ + remote/lxc_protocol.c \ + datatypes.c \ + libvirt.c \ + libvirt-lxc.c \ + $(NULL) + +libvirt_setuid_rpc_client_la_LDFLAGS = \ + $(AM_LDFLAGS) \ + $(LIBXML_LIBS) \ + $(SELINUX_LIBS) \ + $(NULL) +libvirt_setuid_rpc_client_la_CFLAGS = \ + -DLIBVIRT_SETUID_RPC_CLIENT \ + -I$(top_srcdir)/src/conf \ + -I$(top_srcdir)/src/rpc \ + $(AM_CFLAGS) \ + $(SELINUX_CFLAGS) \ + $(NULL) +endif WITH_LXC + lockdriverdir = $(libdir)/libvirt/lock-driver lockdriver_LTLIBRARIES = diff --git a/src/libvirt.c b/src/libvirt.c index 7fa675a..7ceec30 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -446,40 +446,46 @@ virGlobalInit(void) goto error; /* + * Note we must avoid everything except 'remote' driver + * for virt-login-shell usage + */ +#ifndef LIBVIRT_SETUID_RPC_CLIENT + /* * Note that the order is important: the first ones have a higher * priority when calling virConnectOpen. */ -#ifdef WITH_TEST +# ifdef WITH_TEST if (testRegister() == -1) goto error; -#endif -#ifdef WITH_OPENVZ +# endif +# ifdef WITH_OPENVZ if (openvzRegister() == -1) goto error; -#endif -#ifdef WITH_VMWARE +# endif +# ifdef WITH_VMWARE if (vmwareRegister() == -1) goto error; -#endif -#ifdef WITH_PHYP +# endif +# ifdef WITH_PHYP if (phypRegister() == -1) goto error; -#endif -#ifdef WITH_ESX +# endif +# ifdef WITH_ESX if (esxRegister() == -1) goto error; -#endif -#ifdef WITH_HYPERV +# endif +# ifdef WITH_HYPERV if (hypervRegister() == -1) goto error; -#endif -#ifdef WITH_XENAPI +# endif +# ifdef WITH_XENAPI if (xenapiRegister() == -1) goto error; -#endif -#ifdef WITH_PARALLELS +# endif +# ifdef WITH_PARALLELS if (parallelsRegister() == -1) goto error; +# endif #endif #ifdef WITH_REMOTE if (remoteRegister() == -1) diff --git a/tools/Makefile.am b/tools/Makefile.am index d8c912f..256a8f3 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -153,6 +153,11 @@ virt_host_validate_CFLAGS = \ $(COVERAGE_CFLAGS) \ $(NULL) +# Since virt-login-shell will be setuid, we must do everything +# we can to avoid linking to other libraries. Many of them do +# unsafe things in functions marked __atttribute__((constructor)). +# This we statically link to a library containing only the minimal +# libvirt client code, not libvirt.so itself. virt_login_shell_SOURCES = \ virt-login-shell.c @@ -163,11 +168,11 @@ virt_login_shell_LDFLAGS = \ virt_login_shell_LDADD = \ $(STATIC_BINARIES) \ $(PIE_LDFLAGS) \ - ../src/libvirt.la \ - ../src/libvirt-lxc.la \ + ../src/libvirt-setuid-rpc-client.la \ ../gnulib/lib/libgnu.la virt_login_shell_CFLAGS = \ + -DLIBVIRT_SETUID_RPC_CLIENT \ $(WARN_CFLAGS) \ $(PIE_CFLAGS) \ $(COVERAGE_CFLAGS) -- 1.8.3.1
From: "Daniel P. Berrange" <berrange@redhat.com> The virt-login-shell binary shouldn't need to execute programs relying on $PATH, but just in case set a fixed $PATH value of /bin:/usr/bin Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- tools/virt-login-shell.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tools/virt-login-shell.c b/tools/virt-login-shell.c index 0196950..758d1af 100644 --- a/tools/virt-login-shell.c +++ b/tools/virt-login-shell.c @@ -212,6 +212,8 @@ main(int argc, char **argv) return EXIT_FAILURE; } + setenv("PATH", "/bin:/usr/bin", 1); + virSetErrorFunc(NULL, NULL); virSetErrorLogPriorityFunc(NULL); -- 1.8.3.1
From: "Daniel P. Berrange" <berrange@redhat.com> When running setuid, we must be careful about what env vars we allow commands to inherit from us. Replace the virCommandAddEnvPass function with two new ones which do filtering virCommandAddEnvPassAllowSUID virCommandAddEnvPassBlockSUID And make virCommandAddEnvPassCommon use the appropriate ones Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libvirt_private.syms | 3 ++- src/lxc/lxc_process.c | 2 +- src/qemu/qemu_command.c | 8 ++++---- src/rpc/virnetsocket.c | 16 ++++++++-------- src/util/vircommand.c | 50 +++++++++++++++++++++++++++++++++++++----------- src/util/vircommand.h | 8 ++++++-- tests/commandtest.c | 8 ++++---- 7 files changed, 64 insertions(+), 31 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 9be6f32..f1f817c 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1048,7 +1048,8 @@ virCommandAddArgSet; virCommandAddEnvBuffer; virCommandAddEnvFormat; virCommandAddEnvPair; -virCommandAddEnvPass; +virCommandAddEnvPassAllowSUID; +virCommandAddEnvPassBlockSUID; virCommandAddEnvPassCommon; virCommandAddEnvString; virCommandAllowCap; diff --git a/src/lxc/lxc_process.c b/src/lxc/lxc_process.c index f088e8e..c51c4d5 100644 --- a/src/lxc/lxc_process.c +++ b/src/lxc/lxc_process.c @@ -740,7 +740,7 @@ virLXCProcessBuildControllerCmd(virLXCDriverPtr driver, cmd = virCommandNew(vm->def->emulator); /* The controller may call ip command, so we have to retain PATH. */ - virCommandAddEnvPass(cmd, "PATH"); + virCommandAddEnvPassBlockSUID(cmd, "PATH", "/bin:/usr/bin"); virCommandAddEnvFormat(cmd, "LIBVIRT_DEBUG=%d", virLogGetDefaultPriority()); diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index 814f368..63e235d 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7141,7 +7141,7 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg, * security issues and might not work when using VNC. */ if (cfg->vncAllowHostAudio) - virCommandAddEnvPass(cmd, "QEMU_AUDIO_DRV"); + virCommandAddEnvPassBlockSUID(cmd, "QEMU_AUDIO_DRV", NULL); else virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=none"); @@ -7396,8 +7396,8 @@ qemuBuildGraphicsCommandLine(virQEMUDriverConfigPtr cfg, * use QEMU's host audio drivers, possibly SDL too * User can set these two before starting libvirtd */ - virCommandAddEnvPass(cmd, "QEMU_AUDIO_DRV"); - virCommandAddEnvPass(cmd, "SDL_AUDIODRIVER"); + virCommandAddEnvPassBlockSUID(cmd, "QEMU_AUDIO_DRV", NULL); + virCommandAddEnvPassBlockSUID(cmd, "SDL_AUDIODRIVER", NULL); /* New QEMU has this flag to let us explicitly ask for * SDL graphics. This is better than relying on the @@ -7847,7 +7847,7 @@ qemuBuildCommandLine(virConnectPtr conn, virCommandAddArg(cmd, "-nographic"); if (cfg->nogfxAllowHostAudio) - virCommandAddEnvPass(cmd, "QEMU_AUDIO_DRV"); + virCommandAddEnvPassBlockSUID(cmd, "QEMU_AUDIO_DRV", NULL); else virCommandAddEnvString(cmd, "QEMU_AUDIO_DRV=none"); } diff --git a/src/rpc/virnetsocket.c b/src/rpc/virnetsocket.c index 2e50f8c..b2ebefe 100644 --- a/src/rpc/virnetsocket.c +++ b/src/rpc/virnetsocket.c @@ -127,9 +127,9 @@ static int virNetSocketForkDaemon(const char *binary) NULL); virCommandAddEnvPassCommon(cmd); - virCommandAddEnvPass(cmd, "XDG_CACHE_HOME"); - virCommandAddEnvPass(cmd, "XDG_CONFIG_HOME"); - virCommandAddEnvPass(cmd, "XDG_RUNTIME_DIR"); + virCommandAddEnvPassBlockSUID(cmd, "XDG_CACHE_HOME", NULL); + virCommandAddEnvPassBlockSUID(cmd, "XDG_CONFIG_HOME", NULL); + virCommandAddEnvPassBlockSUID(cmd, "XDG_RUNTIME_DIR", NULL); virCommandClearCaps(cmd); virCommandDaemonize(cmd); ret = virCommandRun(cmd, NULL); @@ -680,11 +680,11 @@ int virNetSocketNewConnectSSH(const char *nodename, cmd = virCommandNew(binary ? binary : "ssh"); virCommandAddEnvPassCommon(cmd); - virCommandAddEnvPass(cmd, "KRB5CCNAME"); - virCommandAddEnvPass(cmd, "SSH_AUTH_SOCK"); - virCommandAddEnvPass(cmd, "SSH_ASKPASS"); - virCommandAddEnvPass(cmd, "DISPLAY"); - virCommandAddEnvPass(cmd, "XAUTHORITY"); + virCommandAddEnvPassBlockSUID(cmd, "KRB5CCNAME", NULL); + virCommandAddEnvPassBlockSUID(cmd, "SSH_AUTH_SOCK", NULL); + virCommandAddEnvPassBlockSUID(cmd, "SSH_ASKPASS", NULL); + virCommandAddEnvPassBlockSUID(cmd, "DISPLAY", NULL); + virCommandAddEnvPassBlockSUID(cmd, "XAUTHORITY", NULL); virCommandClearCaps(cmd); if (service) diff --git a/src/util/vircommand.c b/src/util/vircommand.c index 7413269..8dcf9e7 100644 --- a/src/util/vircommand.c +++ b/src/util/vircommand.c @@ -1246,21 +1246,49 @@ virCommandAddEnvBuffer(virCommandPtr cmd, virBufferPtr buf) /** - * virCommandAddEnvPass: + * virCommandAddEnvPassAllowSUID: * @cmd: the command to modify * @name: the name to look up in current environment * * Pass an environment variable to the child * using current process' value + * + * Allow to be passed even if setuid + */ +void +virCommandAddEnvPassAllowSUID(virCommandPtr cmd, const char *name) +{ + const char *value; + if (!cmd || cmd->has_error) + return; + + value = virGetEnvAllowSUID(name); + if (value) + virCommandAddEnvPair(cmd, name, value); +} + + +/** + * virCommandAddEnvPassBlockSUID: + * @cmd: the command to modify + * @name: the name to look up in current environment + * @defvalue: value to return if running setuid, may be NULL + * + * Pass an environment variable to the child + * using current process' value. + * + * Do not pass if running setuid */ void -virCommandAddEnvPass(virCommandPtr cmd, const char *name) +virCommandAddEnvPassBlockSUID(virCommandPtr cmd, const char *name, const char *defvalue) { - char *value; + const char *value; if (!cmd || cmd->has_error) return; - value = getenv(name); + value = virGetEnvBlockSUID(name); + if (!value) + value = defvalue; if (value) virCommandAddEnvPair(cmd, name, value); } @@ -1286,13 +1314,13 @@ virCommandAddEnvPassCommon(virCommandPtr cmd) virCommandAddEnvPair(cmd, "LC_ALL", "C"); - virCommandAddEnvPass(cmd, "LD_PRELOAD"); - virCommandAddEnvPass(cmd, "LD_LIBRARY_PATH"); - virCommandAddEnvPass(cmd, "PATH"); - virCommandAddEnvPass(cmd, "HOME"); - virCommandAddEnvPass(cmd, "USER"); - virCommandAddEnvPass(cmd, "LOGNAME"); - virCommandAddEnvPass(cmd, "TMPDIR"); + virCommandAddEnvPassBlockSUID(cmd, "LD_PRELOAD", NULL); + virCommandAddEnvPassBlockSUID(cmd, "LD_LIBRARY_PATH", NULL); + virCommandAddEnvPassBlockSUID(cmd, "PATH", "/bin:/usr/bin"); + virCommandAddEnvPassBlockSUID(cmd, "HOME", NULL); + virCommandAddEnvPassAllowSUID(cmd, "USER"); + virCommandAddEnvPassAllowSUID(cmd, "LOGNAME"); + virCommandAddEnvPassBlockSUID(cmd, "TMPDIR", NULL); } /** diff --git a/src/util/vircommand.h b/src/util/vircommand.h index c619e06..e977f93 100644 --- a/src/util/vircommand.h +++ b/src/util/vircommand.h @@ -99,8 +99,12 @@ void virCommandAddEnvString(virCommandPtr cmd, void virCommandAddEnvBuffer(virCommandPtr cmd, virBufferPtr buf); -void virCommandAddEnvPass(virCommandPtr cmd, - const char *name) ATTRIBUTE_NONNULL(2); +void virCommandAddEnvPassBlockSUID(virCommandPtr cmd, + const char *name, + const char *defvalue) ATTRIBUTE_NONNULL(2); + +void virCommandAddEnvPassAllowSUID(virCommandPtr cmd, + const char *name) ATTRIBUTE_NONNULL(2); void virCommandAddEnvPassCommon(virCommandPtr cmd); diff --git a/tests/commandtest.c b/tests/commandtest.c index 4780cf9..08e9e21 100644 --- a/tests/commandtest.c +++ b/tests/commandtest.c @@ -294,8 +294,8 @@ static int test6(const void *unused ATTRIBUTE_UNUSED) { virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); - virCommandAddEnvPass(cmd, "DISPLAY"); - virCommandAddEnvPass(cmd, "DOESNOTEXIST"); + virCommandAddEnvPassBlockSUID(cmd, "DISPLAY", NULL); + virCommandAddEnvPassBlockSUID(cmd, "DOESNOTEXIST", NULL); if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); @@ -319,8 +319,8 @@ static int test7(const void *unused ATTRIBUTE_UNUSED) virCommandPtr cmd = virCommandNew(abs_builddir "/commandhelper"); virCommandAddEnvPassCommon(cmd); - virCommandAddEnvPass(cmd, "DISPLAY"); - virCommandAddEnvPass(cmd, "DOESNOTEXIST"); + virCommandAddEnvPassBlockSUID(cmd, "DISPLAY", NULL); + virCommandAddEnvPassBlockSUID(cmd, "DOESNOTEXIST", NULL); if (virCommandRun(cmd, NULL) < 0) { virErrorPtr err = virGetLastError(); -- 1.8.3.1
From: "Daniel P. Berrange" <berrange@redhat.com> Unconditional use of getenv is not secure in setuid env. While not all libvirt code runs in a setuid env (since much of it only exists inside libvirtd) this is not always clear to developers. So make all the code paranoid, even if it only ever runs inside libvirtd. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- daemon/libvirtd.c | 2 +- src/driver.c | 3 ++- src/libvirt.c | 2 +- src/locking/lock_daemon.c | 4 ++-- src/locking/lock_driver_lockd.c | 2 +- src/locking/lock_manager.c | 2 +- src/lxc/lxc_driver.c | 4 ++-- src/remote/remote_driver.c | 4 ++-- src/rpc/virnettlscontext.c | 4 ++-- src/util/virauth.c | 2 +- src/util/virfile.c | 7 +++++-- src/util/virlog.c | 8 ++++---- src/util/virrandom.c | 2 +- src/util/virutil.c | 8 ++++---- src/vbox/vbox_XPCOMCGlue.c | 2 +- src/vbox/vbox_tmpl.c | 4 ++-- tools/virsh.c | 18 +++++++++--------- 17 files changed, 41 insertions(+), 37 deletions(-) diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c index 99c0342..aef1546 100644 --- a/daemon/libvirtd.c +++ b/daemon/libvirtd.c @@ -991,7 +991,7 @@ static int migrateProfile(void) goto cleanup; } - config_home = getenv("XDG_CONFIG_HOME"); + config_home = virGetEnvBlockSUID("XDG_CONFIG_HOME"); if (config_home && config_home[0] != '\0') { if (VIR_STRDUP(xdg_dir, config_home) < 0) goto cleanup; diff --git a/src/driver.c b/src/driver.c index a08dd34..ab2a253 100644 --- a/src/driver.c +++ b/src/driver.c @@ -27,6 +27,7 @@ #include "driver.h" #include "viralloc.h" #include "virlog.h" +#include "virutil.h" #include "configmake.h" #include "virstring.h" @@ -43,7 +44,7 @@ static const char *moddir = NULL; void virDriverModuleInitialize(const char *defmoddir) { - const char *custommoddir = getenv("LIBVIRT_DRIVER_DIR"); + const char *custommoddir = virGetEnvBlockSUID("LIBVIRT_DRIVER_DIR"); if (custommoddir) moddir = custommoddir; else if (defmoddir) diff --git a/src/libvirt.c b/src/libvirt.c index 7ceec30..0f8d79a 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -1094,7 +1094,7 @@ virConnectGetDefaultURI(virConfPtr conf, { int ret = -1; virConfValuePtr value = NULL; - char *defname = getenv("LIBVIRT_DEFAULT_URI"); + const char *defname = virGetEnvBlockSUID("LIBVIRT_DEFAULT_URI"); if (defname && *defname) { VIR_DEBUG("Using LIBVIRT_DEFAULT_URI '%s'", defname); *name = defname; diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index 5f675ef..cef5bd6 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -605,7 +605,7 @@ virLockDaemonSetupNetworkingSystemD(virNetServerPtr srv) VIR_DEBUG("Setting up networking from systemd"); - if (!(pidstr = getenv("LISTEN_PID"))) { + if (!(pidstr = virGetEnvAllowSUID("LISTEN_PID"))) { VIR_DEBUG("No LISTEN_FDS from systemd"); return 0; } @@ -621,7 +621,7 @@ virLockDaemonSetupNetworkingSystemD(virNetServerPtr srv) return 0; } - if (!(fdstr = getenv("LISTEN_FDS"))) { + if (!(fdstr = virGetEnvAllowSUID("LISTEN_FDS"))) { VIR_DEBUG("No LISTEN_FDS from systemd"); return 0; } diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index b115799..86ce2d8 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -84,7 +84,7 @@ static virLockManagerLockDaemonDriverPtr driver = NULL; static const char * virLockManagerLockDaemonFindDaemon(void) { - const char *customDaemon = getenv("VIRTLOCKD_PATH"); + const char *customDaemon = virGetEnvBlockSUID("VIRTLOCKD_PATH"); if (customDaemon) return customDaemon; diff --git a/src/locking/lock_manager.c b/src/locking/lock_manager.c index aff54c0..f09c168 100644 --- a/src/locking/lock_manager.c +++ b/src/locking/lock_manager.c @@ -135,7 +135,7 @@ virLockManagerPluginPtr virLockManagerPluginNew(const char *name, void *handle = NULL; virLockDriverPtr driver; virLockManagerPluginPtr plugin = NULL; - const char *moddir = getenv("LIBVIRT_LOCK_MANAGER_PLUGIN_DIR"); + const char *moddir = virGetEnvBlockSUID("LIBVIRT_LOCK_MANAGER_PLUGIN_DIR"); char *modfile = NULL; char *configFile = NULL; diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 870e4a9..61a90ca 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -1362,14 +1362,14 @@ static int lxcStateInitialize(bool privileged, void *opaque ATTRIBUTE_UNUSED) { virCapsPtr caps = NULL; - char *ld; + const char *ld; virLXCDriverConfigPtr cfg = NULL; /* Valgrind gets very annoyed when we clone containers, so * disable LXC when under valgrind * XXX remove this when valgrind is fixed */ - ld = getenv("LD_PRELOAD"); + ld = virGetEnvBlockSUID("LD_PRELOAD"); if (ld && strstr(ld, "vgpreload")) { VIR_INFO("Running under valgrind, disabling driver"); return 0; diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 115d0bc..759383e 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -187,7 +187,7 @@ remoteFindDaemonPath(void) NULL }; size_t i; - const char *customDaemon = getenv("LIBVIRTD_PATH"); + const char *customDaemon = virGetEnvBlockSUID("LIBVIRTD_PATH"); if (customDaemon) return customDaemon; @@ -955,7 +955,7 @@ remoteConnectOpen(virConnectPtr conn, { struct private_data *priv; int ret, rflags = 0; - const char *autostart = getenv("LIBVIRT_AUTOSTART"); + const char *autostart = virGetEnvBlockSUID("LIBVIRT_AUTOSTART"); if (inside_daemon && (!conn->uri || (conn->uri && !conn->uri->server))) return VIR_DRV_OPEN_DECLINED; diff --git a/src/rpc/virnettlscontext.c b/src/rpc/virnettlscontext.c index 7cee27c..cd69794 100644 --- a/src/rpc/virnettlscontext.c +++ b/src/rpc/virnettlscontext.c @@ -710,7 +710,7 @@ static virNetTLSContextPtr virNetTLSContextNew(const char *cacert, bool isServer) { virNetTLSContextPtr ctxt; - char *gnutlsdebug; + const char *gnutlsdebug; int err; if (virNetTLSContextInitialize() < 0) @@ -719,7 +719,7 @@ static virNetTLSContextPtr virNetTLSContextNew(const char *cacert, if (!(ctxt = virObjectLockableNew(virNetTLSContextClass))) return NULL; - if ((gnutlsdebug = getenv("LIBVIRT_GNUTLS_DEBUG")) != NULL) { + if ((gnutlsdebug = virGetEnvAllowSUID("LIBVIRT_GNUTLS_DEBUG")) != NULL) { int val; if (virStrToLong_i(gnutlsdebug, NULL, 10, &val) < 0) val = 10; diff --git a/src/util/virauth.c b/src/util/virauth.c index f58797c..e66cbf5 100644 --- a/src/util/virauth.c +++ b/src/util/virauth.c @@ -42,7 +42,7 @@ virAuthGetConfigFilePathURI(virURIPtr uri, { int ret = -1; size_t i; - const char *authenv = getenv("LIBVIRT_AUTH_FILE"); + const char *authenv = virGetEnvBlockSUID("LIBVIRT_AUTH_FILE"); char *userdir = NULL; *path = NULL; diff --git a/src/util/virfile.c b/src/util/virfile.c index f6fd2a8..bc607a5 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1426,6 +1426,7 @@ virFileIsLink(const char *linkpath) char * virFindFileInPath(const char *file) { + const char *origpath = NULL; char *path = NULL; char *pathiter; char *pathseg; @@ -1454,9 +1455,11 @@ virFindFileInPath(const char *file) } /* copy PATH env so we can tweak it */ - path = getenv("PATH"); + origpath = virGetEnvBlockSUID("PATH"); + if (!origpath) + origpath = "/bin:/usr/bin"; - if (VIR_STRDUP_QUIET(path, path) <= 0) + if (VIR_STRDUP_QUIET(path, origpath) <= 0) return NULL; /* for each path segment, append the file to search for and test for diff --git a/src/util/virlog.c b/src/util/virlog.c index 997d582..5b7e7b0 100644 --- a/src/util/virlog.c +++ b/src/util/virlog.c @@ -1637,18 +1637,18 @@ virLogParseDefaultPriority(const char *priority) void virLogSetFromEnv(void) { - char *debugEnv; + const char *debugEnv; if (virLogInitialize() < 0) return; - debugEnv = getenv("LIBVIRT_DEBUG"); + debugEnv = virGetEnvAllowSUID("LIBVIRT_DEBUG"); if (debugEnv && *debugEnv) virLogParseDefaultPriority(debugEnv); - debugEnv = getenv("LIBVIRT_LOG_FILTERS"); + debugEnv = virGetEnvAllowSUID("LIBVIRT_LOG_FILTERS"); if (debugEnv && *debugEnv) virLogParseFilters(debugEnv); - debugEnv = getenv("LIBVIRT_LOG_OUTPUTS"); + debugEnv = virGetEnvAllowSUID("LIBVIRT_LOG_OUTPUTS"); if (debugEnv && *debugEnv) virLogParseOutputs(debugEnv); } diff --git a/src/util/virrandom.c b/src/util/virrandom.c index b0912f4..491a3af 100644 --- a/src/util/virrandom.c +++ b/src/util/virrandom.c @@ -66,7 +66,7 @@ virRandomOnceInit(void) /* Normally we want a decent seed. But if reproducible debugging * of a fixed pseudo-random sequence is ever required, uncomment * this block to let an environment variable force the seed. */ - const char *debug = getenv("VIR_DEBUG_RANDOM_SEED"); + const char *debug = virGetEnvBlockSUID("VIR_DEBUG_RANDOM_SEED"); if (debug && virStrToLong_ui(debug, NULL, 0, &seed) < 0) return -1; diff --git a/src/util/virutil.c b/src/util/virutil.c index 7e7c1c2..7e24b4a 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -782,7 +782,7 @@ virGetUserDirectoryByUID(uid_t uid) static char *virGetXDGDirectory(const char *xdgenvname, const char *xdgdefdir) { - const char *path = getenv(xdgenvname); + const char *path = virGetEnvBlockSUID(xdgenvname); char *ret = NULL; char *home = NULL; @@ -810,7 +810,7 @@ char *virGetUserCacheDirectory(void) char *virGetUserRuntimeDirectory(void) { - const char *path = getenv("XDG_RUNTIME_DIR"); + const char *path = virGetEnvBlockSUID("XDG_RUNTIME_DIR"); if (!path || !path[0]) { return virGetUserCacheDirectory(); @@ -1143,7 +1143,7 @@ virGetUserDirectoryByUID(uid_t uid ATTRIBUTE_UNUSED) const char *dir; char *ret; - dir = getenv("HOME"); + dir = virGetEnvBlockSUID("HOME"); /* Only believe HOME if it is an absolute path and exists */ if (dir) { @@ -1163,7 +1163,7 @@ virGetUserDirectoryByUID(uid_t uid ATTRIBUTE_UNUSED) if (!dir) /* USERPROFILE is probably the closest equivalent to $HOME? */ - dir = getenv("USERPROFILE"); + dir = virGetEnvBlockSUID("USERPROFILE"); if (VIR_STRDUP(ret, dir) < 0) return NULL; diff --git a/src/vbox/vbox_XPCOMCGlue.c b/src/vbox/vbox_XPCOMCGlue.c index 10ddcf8..8652e3a 100644 --- a/src/vbox/vbox_XPCOMCGlue.c +++ b/src/vbox/vbox_XPCOMCGlue.c @@ -201,7 +201,7 @@ VBoxCGlueInit(unsigned int *version) "/usr/local/lib/VirtualBox", "/Applications/VirtualBox.app/Contents/MacOS" }; - const char *home = getenv("VBOX_APP_HOME"); + const char *home = virGetEnvBlockSUID("VBOX_APP_HOME"); /* If the user specifies the location, try only that. */ if (home != NULL) { diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index cf34f5c..c994067 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -2212,7 +2212,6 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) { vboxIID iid = VBOX_IID_INITIALIZER; int gotAllABoutDef = -1; nsresult rc; - char *tmp; /* Flags checked by virDomainDefFormat */ @@ -2484,8 +2483,9 @@ static char *vboxDomainGetXMLDesc(virDomainPtr dom, unsigned int flags) { } } else if ((vrdpPresent != 1) && (totalPresent == 0) && (VIR_ALLOC_N(def->graphics, 1) >= 0)) { if (VIR_ALLOC(def->graphics[def->ngraphics]) >= 0) { + const char *tmp; def->graphics[def->ngraphics]->type = VIR_DOMAIN_GRAPHICS_TYPE_DESKTOP; - tmp = getenv("DISPLAY"); + tmp = virGetEnvBlockSUID("DISPLAY"); if (VIR_STRDUP(def->graphics[def->ngraphics]->data.desktop.display, tmp) < 0) { /* just don't go to cleanup yet as it is ok to have * display as NULL diff --git a/tools/virsh.c b/tools/virsh.c index 6842ed8..a76229a 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -233,7 +233,7 @@ virshErrorHandler(void *unused ATTRIBUTE_UNUSED, virErrorPtr error) { virFreeError(last_error); last_error = virSaveLastError(); - if (getenv("VIRSH_DEBUG") != NULL) + if (virGetEnvAllowSUID("VIRSH_DEBUG") != NULL) virDefaultErrorFunc(error); } @@ -670,7 +670,7 @@ vshEditWriteToTempFile(vshControl *ctl, const char *doc) int fd; char ebuf[1024]; - tmpdir = getenv("TMPDIR"); + tmpdir = virGetEnvBlockSUID("TMPDIR"); if (!tmpdir) tmpdir = "/tmp"; if (virAsprintf(&ret, "%s/virshXXXXXX.xml", tmpdir) < 0) { vshError(ctl, "%s", _("out of memory")); @@ -717,9 +717,9 @@ vshEditFile(vshControl *ctl, const char *filename) int outfd = STDOUT_FILENO; int errfd = STDERR_FILENO; - editor = getenv("VISUAL"); + editor = virGetEnvBlockSUID("VISUAL"); if (!editor) - editor = getenv("EDITOR"); + editor = virGetEnvBlockSUID("EDITOR"); if (!editor) editor = "vi"; /* could be cruel & default to ed(1) here */ @@ -2367,11 +2367,11 @@ vshEventLoop(void *opaque) static void vshInitDebug(vshControl *ctl) { - char *debugEnv; + const char *debugEnv; if (ctl->debug == VSH_DEBUG_DEFAULT) { /* log level not set from commandline, check env variable */ - debugEnv = getenv("VIRSH_DEBUG"); + debugEnv = virGetEnvAllowSUID("VIRSH_DEBUG"); if (debugEnv) { int debug; if (virStrToLong_i(debugEnv, NULL, 10, &debug) < 0 || @@ -2386,7 +2386,7 @@ vshInitDebug(vshControl *ctl) if (ctl->logfile == NULL) { /* log file not set from cmdline */ - debugEnv = getenv("VIRSH_LOG_FILE"); + debugEnv = virGetEnvBlockSUID("VIRSH_LOG_FILE"); if (debugEnv && *debugEnv) { ctl->logfile = vshStrdup(ctl, debugEnv); vshOpenLogFile(ctl); @@ -3232,7 +3232,7 @@ int main(int argc, char **argv) { vshControl _ctl, *ctl = &_ctl; - char *defaultConn; + const char *defaultConn; bool ret = true; memset(ctl, 0, sizeof(vshControl)); @@ -3279,7 +3279,7 @@ main(int argc, char **argv) else progname++; - if ((defaultConn = getenv("VIRSH_DEFAULT_CONNECT_URI"))) { + if ((defaultConn = virGetEnvBlockSUID("VIRSH_DEFAULT_CONNECT_URI"))) { ctl->name = vshStrdup(ctl, defaultConn); } -- 1.8.3.1
From: "Daniel P. Berrange" <berrange@redhat.com> The use of getenv is typically insecure, and we want people to use our wrappers, to force them to think about setuid needs. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- cfg.mk | 8 ++++++++ src/util/virutil.c | 4 ++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/cfg.mk b/cfg.mk index 3e44439..56821e2 100644 --- a/cfg.mk +++ b/cfg.mk @@ -859,6 +859,11 @@ sc_prohibit_unbounded_arrays_in_rpc: halt='Arrays in XDR must have a upper limit set for <NNN>' \ $(_sc_search_regexp) +sc_prohibit_getenv: + @prohibit='\b(secure_)?getenv *\(' \ + exclude='exempt from syntax-check' \ + halt='Use virGetEnv{Allow,Block}SUID instead of getenv' \ + $(_sc_search_regexp) # We don't use this feature of maint.mk. prev_version_file = /dev/null @@ -1028,3 +1033,6 @@ exclude_file_name_regexp--sc_prohibit_include_public_headers_brackets = \ exclude_file_name_regexp--sc_prohibit_int_ijk = \ ^(src/remote_protocol-structs|src/remote/remote_protocol.x|cfg.mk|include/)$ + +exclude_file_name_regexp--sc_prohibit_getenv = \ + ^tests/.*\.[ch]$$ diff --git a/src/util/virutil.c b/src/util/virutil.c index 7e24b4a..87cc2e7 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -2143,7 +2143,7 @@ cleanup: */ const char *virGetEnvBlockSUID(const char *name) { - return secure_getenv(name); + return secure_getenv(name); /* exempt from syntax-check-rules */ } @@ -2157,7 +2157,7 @@ const char *virGetEnvBlockSUID(const char *name) */ const char *virGetEnvAllowSUID(const char *name) { - return getenv(name); + return getenv(name); /* exempt from syntax-check-rules */ } -- 1.8.3.1
From: "Daniel P. Berrange" <berrange@redhat.com> We don't know enough about quality of external libraries used for non-UNIX transports, nor do we want to spawn external commands when setuid. Restrict to the bare minimum which is UNIX transport for local usage. Users shouldn't need to be running setuid if connecting to remote hypervisors in any case. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libvirt.c | 6 ++++++ src/remote/remote_driver.c | 14 ++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/src/libvirt.c b/src/libvirt.c index 0f8d79a..aec5d80 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -1135,6 +1135,12 @@ do_open(const char *name, if (name && name[0] == '\0') name = NULL; + if (!name && virIsSUID()) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("An explicit URI must be provided when setuid")); + goto failed; + } + /* * If no URI is passed, then check for an environment string if not * available probe the compiled in drivers to find a default hypervisor diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index 759383e..c0e508a 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -488,6 +488,20 @@ doRemoteOpen(virConnectPtr conn, transport = trans_unix; } + /* + * We don't want to be executing external programs in setuid mode, + * so this rules out 'ext' and 'ssh' transports. Exclude libssh + * and tls too, since we're not confident the libraries are safe + * for setuid usage. Just allow UNIX sockets, since that does + * not require any external libraries or command execution + */ + if (virIsSUID() && + transport != trans_unix) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("Only Unix socket URI transport is allowed in setuid mode")); + return VIR_DRV_OPEN_ERROR; + } + /* Local variables which we will initialize. These can * get freed in the failed: path. */ -- 1.8.3.1
From: "Daniel P. Berrange" <berrange@redhat.com> We don't want setuid programs automatically spawning libvirtd, so disable any use of autostart when setuid. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/remote/remote_driver.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index c0e508a..b3ab3e6 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -431,7 +431,7 @@ doRemoteOpen(virConnectPtr conn, trans_tcp, } transport; #ifndef WIN32 - const char *daemonPath; + const char *daemonPath = NULL; #endif /* We handle *ALL* URIs here. The caller has rejected any @@ -713,7 +713,8 @@ doRemoteOpen(virConnectPtr conn, VIR_DEBUG("Proceeding with sockname %s", sockname); } - if (!(daemonPath = remoteFindDaemonPath())) { + if ((flags & VIR_DRV_OPEN_REMOTE_AUTOSTART) && + !(daemonPath = remoteFindDaemonPath())) { virReportError(VIR_ERR_INTERNAL_ERROR, _("Unable to locate libvirtd daemon in %s " "(to override, set $LIBVIRTD_PATH to the " @@ -997,8 +998,9 @@ remoteConnectOpen(virConnectPtr conn, getuid() > 0) { VIR_DEBUG("Auto-spawn user daemon instance"); rflags |= VIR_DRV_OPEN_REMOTE_USER; - if (!autostart || - STRNEQ(autostart, "0")) + if (!virIsSUID() && + (!autostart || + STRNEQ(autostart, "0"))) rflags |= VIR_DRV_OPEN_REMOTE_AUTOSTART; } @@ -1014,8 +1016,9 @@ remoteConnectOpen(virConnectPtr conn, if (getuid() > 0) { VIR_DEBUG("Auto-spawn user daemon instance"); rflags |= VIR_DRV_OPEN_REMOTE_USER; - if (!autostart || - STRNEQ(autostart, "0")) + if (!virIsSUID() && + (!autostart || + STRNEQ(autostart, "0"))) rflags |= VIR_DRV_OPEN_REMOTE_AUTOSTART; } #endif -- 1.8.3.1
From: "Daniel P. Berrange" <berrange@redhat.com> We already have stubs for getuid, geteuid, getgid but not for getegid. Something in gnulib already does a check for it during configure, so we already have the HAVE_GETEGID macro defined. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/util/virutil.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/util/virutil.h b/src/util/virutil.h index e8765b2..2229c73 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -108,6 +108,10 @@ static inline int geteuid (void) { return 0; } static inline int getgid (void) { return 0; } # endif +# ifndef HAVE_GETEGID +static inline int getegid (void) { return 0; } +# endif + char *virGetHostname(void); char *virGetUserDirectory(void); -- 1.8.3.1
From: "Daniel P. Berrange" <berrange@redhat.com> Most of the usage of getuid()/getgid() is in cases where we are considering what privileges we have. As such the code should be using the effective IDs, not real IDs. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libvirt.c | 2 +- src/locking/lock_daemon.c | 2 +- src/locking/lock_driver_lockd.c | 4 ++-- src/lxc/lxc_controller.c | 2 +- src/qemu/qemu_driver.c | 6 +++--- src/remote/remote_driver.c | 4 ++-- src/storage/storage_backend.c | 4 ++-- src/storage/storage_backend_fs.c | 4 ++-- src/storage/storage_backend_logical.c | 2 +- src/util/virfile.c | 16 ++++++++-------- src/util/viridentity.c | 8 ++++---- src/util/virstoragefile.c | 2 +- src/vbox/vbox_driver.c | 2 +- src/vbox/vbox_tmpl.c | 2 +- tests/qemumonitortestutils.c | 2 +- tests/virnetsockettest.c | 4 ++-- 16 files changed, 33 insertions(+), 33 deletions(-) diff --git a/src/libvirt.c b/src/libvirt.c index aec5d80..96d8fdc 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -133,7 +133,7 @@ static int virConnectAuthGainPolkit(const char *privilege) { int status; int ret = -1; - if (getuid() == 0) + if (geteuid() == 0) return 0; cmd = virCommandNewArgList(POLKIT_AUTH, "--obtain", privilege, NULL); diff --git a/src/locking/lock_daemon.c b/src/locking/lock_daemon.c index cef5bd6..35ccb4e 100644 --- a/src/locking/lock_daemon.c +++ b/src/locking/lock_daemon.c @@ -1167,7 +1167,7 @@ int main(int argc, char **argv) { {0, 0, 0, 0} }; - privileged = getuid() == 0; + privileged = geteuid() == 0; if (setlocale(LC_ALL, "") == NULL || bindtextdomain(PACKAGE, LOCALEDIR) == NULL || diff --git a/src/locking/lock_driver_lockd.c b/src/locking/lock_driver_lockd.c index 86ce2d8..f3b9467 100644 --- a/src/locking/lock_driver_lockd.c +++ b/src/locking/lock_driver_lockd.c @@ -302,7 +302,7 @@ virLockManagerLockDaemonConnect(virLockManagerPtr lock, { virNetClientPtr client; - if (!(client = virLockManagerLockDaemonConnectionNew(getuid() == 0, program))) + if (!(client = virLockManagerLockDaemonConnectionNew(geteuid() == 0, program))) return NULL; if (virLockManagerLockDaemonConnectionRegister(lock, @@ -331,7 +331,7 @@ static int virLockManagerLockDaemonSetupLockspace(const char *path) memset(&args, 0, sizeof(args)); args.path = (char*)path; - if (!(client = virLockManagerLockDaemonConnectionNew(getuid() == 0, &program))) + if (!(client = virLockManagerLockDaemonConnectionNew(geteuid() == 0, &program))) return -1; if (virNetClientProgramCall(program, diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index aa30d8e..c8f68c0 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -2362,7 +2362,7 @@ int main(int argc, char *argv[]) goto cleanup; } - if (getuid() != 0) { + if (geteuid() != 0) { fprintf(stderr, "%s: must be run as the 'root' user\n", argv[0]); goto cleanup; } diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 4977b12..c613967 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -2793,8 +2793,8 @@ qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid, unsigned int vfoflags = 0; int fd = -1; int path_shared = virStorageFileIsSharedFS(path); - uid_t uid = getuid(); - gid_t gid = getgid(); + uid_t uid = geteuid(); + gid_t gid = getegid(); /* path might be a pre-existing block dev, in which case * we need to skip the create step, and also avoid unlink @@ -2834,7 +2834,7 @@ qemuOpenFileAs(uid_t fallback_uid, gid_t fallback_gid, qemu user is non-root, just set a flag to bypass security driver shenanigans, and retry the operation after doing setuid to qemu user */ - if ((fd != -EACCES && fd != -EPERM) || fallback_uid == getuid()) + if ((fd != -EACCES && fd != -EPERM) || fallback_uid == geteuid()) goto error; /* On Linux we can also verify the FS-type of the directory. */ diff --git a/src/remote/remote_driver.c b/src/remote/remote_driver.c index b3ab3e6..7181949 100644 --- a/src/remote/remote_driver.c +++ b/src/remote/remote_driver.c @@ -995,7 +995,7 @@ remoteConnectOpen(virConnectPtr conn, (strstr(conn->uri->scheme, "+unix") != NULL)) && (STREQ(conn->uri->path, "/session") || STRPREFIX(conn->uri->scheme, "test+")) && - getuid() > 0) { + geteuid() > 0) { VIR_DEBUG("Auto-spawn user daemon instance"); rflags |= VIR_DRV_OPEN_REMOTE_USER; if (!virIsSUID() && @@ -1013,7 +1013,7 @@ remoteConnectOpen(virConnectPtr conn, if (!conn->uri) { VIR_DEBUG("Auto-probe remote URI"); #ifndef __sun - if (getuid() > 0) { + if (geteuid() > 0) { VIR_DEBUG("Auto-spawn user daemon instance"); rflags |= VIR_DRV_OPEN_REMOTE_USER; if (!virIsSUID() && diff --git a/src/storage/storage_backend.c b/src/storage/storage_backend.c index c21e6ea..aa2ed28 100644 --- a/src/storage/storage_backend.c +++ b/src/storage/storage_backend.c @@ -556,11 +556,11 @@ virStorageBackendCreateExecCommand(virStoragePoolObjPtr pool, bool filecreated = false; if ((pool->def->type == VIR_STORAGE_POOL_NETFS) - && (((getuid() == 0) + && (((geteuid() == 0) && (vol->target.perms.uid != (uid_t) -1) && (vol->target.perms.uid != 0)) || ((vol->target.perms.gid != (gid_t) -1) - && (vol->target.perms.gid != getgid())))) { + && (vol->target.perms.gid != getegid())))) { virCommandSetUID(cmd, vol->target.perms.uid); virCommandSetGID(cmd, vol->target.perms.gid); diff --git a/src/storage/storage_backend_fs.c b/src/storage/storage_backend_fs.c index f9b0326..7ff950b 100644 --- a/src/storage/storage_backend_fs.c +++ b/src/storage/storage_backend_fs.c @@ -785,9 +785,9 @@ virStorageBackendFileSystemBuild(virConnectPtr conn ATTRIBUTE_UNUSED, /* Reflect the actual uid and gid to the config. */ if (pool->def->target.perms.uid == (uid_t) -1) - pool->def->target.perms.uid = getuid(); + pool->def->target.perms.uid = geteuid(); if (pool->def->target.perms.gid == (gid_t) -1) - pool->def->target.perms.gid = getgid(); + pool->def->target.perms.gid = getegid(); if (flags != 0) { ret = virStorageBackendMakeFileSystem(pool, flags); diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 81ee4a4..10966cc 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -726,7 +726,7 @@ virStorageBackendLogicalCreateVol(virConnectPtr conn, goto error; /* We can only chown/grp if root */ - if (getuid() == 0) { + if (geteuid() == 0) { if (fchown(fd, vol->target.perms.uid, vol->target.perms.gid) < 0) { virReportSystemError(errno, _("cannot set file owner '%s'"), diff --git a/src/util/virfile.c b/src/util/virfile.c index bc607a5..3a9980c 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -1535,8 +1535,8 @@ virFileAccessibleAs(const char *path, int mode, gid_t *groups; int ngroups; - if (uid == getuid() && - gid == getgid()) + if (uid == geteuid() && + gid == getegid()) return access(path, mode); ngroups = virGetGroupList(uid, gid, &groups); @@ -1828,9 +1828,9 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, /* allow using -1 to mean "current value" */ if (uid == (uid_t) -1) - uid = getuid(); + uid = geteuid(); if (gid == (gid_t) -1) - gid = getgid(); + gid = getegid(); /* treat absence of both flags as presence of both for simpler * calling. */ @@ -1838,7 +1838,7 @@ virFileOpenAs(const char *path, int openflags, mode_t mode, flags |= VIR_FILE_OPEN_NOFORK|VIR_FILE_OPEN_FORK; if ((flags & VIR_FILE_OPEN_NOFORK) - || (getuid() != 0) + || (geteuid() != 0) || ((uid == 0) && (gid == 0))) { if ((fd = open(path, openflags, mode)) < 0) { @@ -1949,12 +1949,12 @@ virDirCreate(const char *path, /* allow using -1 to mean "current value" */ if (uid == (uid_t) -1) - uid = getuid(); + uid = geteuid(); if (gid == (gid_t) -1) - gid = getgid(); + gid = getegid(); if ((!(flags & VIR_DIR_CREATE_AS_UID)) - || (getuid() != 0) + || (geteuid() != 0) || ((uid == 0) && (gid == 0)) || ((flags & VIR_DIR_CREATE_ALLOW_EXIST) && (stat(path, &st) >= 0))) { return virDirCreateNoFork(path, mode, uid, gid, flags); diff --git a/src/util/viridentity.c b/src/util/viridentity.c index f681f85..4f5127c 100644 --- a/src/util/viridentity.c +++ b/src/util/viridentity.c @@ -157,14 +157,14 @@ virIdentityPtr virIdentityGetSystem(void) virAsprintf(&processtime, "%llu", timestamp) < 0) goto cleanup; - if (!(username = virGetUserName(getuid()))) + if (!(username = virGetUserName(geteuid()))) goto cleanup; - if (virAsprintf(&userid, "%d", (int)getuid()) < 0) + if (virAsprintf(&userid, "%d", (int)geteuid()) < 0) goto cleanup; - if (!(groupname = virGetGroupName(getgid()))) + if (!(groupname = virGetGroupName(getegid()))) goto cleanup; - if (virAsprintf(&groupid, "%d", (int)getgid()) < 0) + if (virAsprintf(&groupid, "%d", (int)getegid()) < 0) goto cleanup; #if WITH_SELINUX diff --git a/src/util/virstoragefile.c b/src/util/virstoragefile.c index 48d5fbb..1b4d4cf 100644 --- a/src/util/virstoragefile.c +++ b/src/util/virstoragefile.c @@ -572,7 +572,7 @@ virFindBackingFile(const char *start, bool start_is_dir, const char *path, goto cleanup; } - if (virFileAccessibleAs(combined, F_OK, getuid(), getgid()) < 0) { + if (virFileAccessibleAs(combined, F_OK, geteuid(), getegid()) < 0) { virReportSystemError(errno, _("Cannot access backing file '%s'"), combined); diff --git a/src/vbox/vbox_driver.c b/src/vbox/vbox_driver.c index 9d07574..4978913 100644 --- a/src/vbox/vbox_driver.c +++ b/src/vbox/vbox_driver.c @@ -153,7 +153,7 @@ static virDrvOpenStatus vboxConnectOpen(virConnectPtr conn, virConnectAuthPtr auth ATTRIBUTE_UNUSED, unsigned int flags) { - uid_t uid = getuid(); + uid_t uid = geteuid(); virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR); diff --git a/src/vbox/vbox_tmpl.c b/src/vbox/vbox_tmpl.c index c994067..37ca651 100644 --- a/src/vbox/vbox_tmpl.c +++ b/src/vbox/vbox_tmpl.c @@ -999,7 +999,7 @@ static virDrvOpenStatus vboxConnectOpen(virConnectPtr conn, unsigned int flags) { vboxGlobalData *data = NULL; - uid_t uid = getuid(); + uid_t uid = geteuid(); virCheckFlags(VIR_CONNECT_RO, VIR_DRV_OPEN_ERROR); diff --git a/tests/qemumonitortestutils.c b/tests/qemumonitortestutils.c index 2aefabc..60ffe90 100644 --- a/tests/qemumonitortestutils.c +++ b/tests/qemumonitortestutils.c @@ -789,7 +789,7 @@ qemuMonitorCommonTestNew(virDomainXMLOptionPtr xmlopt, goto error; } - if (virNetSocketNewListenUNIX(path, 0700, getuid(), getgid(), + if (virNetSocketNewListenUNIX(path, 0700, geteuid(), getegid(), &test->server) < 0) goto error; diff --git a/tests/virnetsockettest.c b/tests/virnetsockettest.c index b3e12f9..eda95bc 100644 --- a/tests/virnetsockettest.c +++ b/tests/virnetsockettest.c @@ -220,7 +220,7 @@ static int testSocketUNIXAccept(const void *data ATTRIBUTE_UNUSED) if (virAsprintf(&path, "%s/test.sock", tmpdir) < 0) goto cleanup; - if (virNetSocketNewListenUNIX(path, 0700, -1, getgid(), &lsock) < 0) + if (virNetSocketNewListenUNIX(path, 0700, -1, getegid(), &lsock) < 0) goto cleanup; if (virNetSocketListen(lsock, 0) < 0) @@ -270,7 +270,7 @@ static int testSocketUNIXAddrs(const void *data ATTRIBUTE_UNUSED) if (virAsprintf(&path, "%s/test.sock", tmpdir) < 0) goto cleanup; - if (virNetSocketNewListenUNIX(path, 0700, -1, getgid(), &lsock) < 0) + if (virNetSocketNewListenUNIX(path, 0700, -1, getegid(), &lsock) < 0) goto cleanup; if (STRNEQ(virNetSocketLocalAddrString(lsock), "127.0.0.1;0")) { -- 1.8.3.1
From: "Daniel P. Berrange" <berrange@redhat.com> Avoid people introducing security flaws in their apps by forbidding the use of libvirt.so in setuid programs, with a check in virInitialize. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libvirt.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/libvirt.c b/src/libvirt.c index 96d8fdc..d76e537 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -409,6 +409,14 @@ virGlobalInit(void) virErrorInitialize() < 0) goto error; +#ifndef IN_VIRT_LOGIN_SHELL + if (virIsSUID()) { + virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("libvirt.so is not safe to use from setuid programs")); + goto error; + } +#endif + #ifdef WITH_GNUTLS_GCRYPT /* * This sequence of API calls it copied exactly from -- 1.8.3.1
On 10/21/2013 07:12 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Avoid people introducing security flaws in their apps by forbidding the use of libvirt.so in setuid programs, with a check in virInitialize.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- src/libvirt.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/src/libvirt.c b/src/libvirt.c index 96d8fdc..d76e537 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -409,6 +409,14 @@ virGlobalInit(void) virErrorInitialize() < 0) goto error;
+#ifndef IN_VIRT_LOGIN_SHELL
Oops. This spelling is from an earlier version of your patch series. But in the version you committed, patch 4/14 (commit 3e2f27e1) named it the more generic LIBVIRT_SETUID_RPC_CLIENT. Which means IN_VIRT_LOGIN_SHELL is never defined,...
+ if (virIsSUID()) {
...so virt-login-shell happily reports that it is setuid...
+ virReportError(VIR_ERR_INTERNAL_ERROR, "%s", + _("libvirt.so is not safe to use from setuid programs"));
...and we have killed it. Two separate killers in our CVE fix - not a good track record on testing things ;( -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
From: "Daniel P. Berrange" <berrange@redhat.com> Many people will not want the setuid virt-login-shell binary installed by default, so move it into a separate sub-RPM named libvirt-login-shell. This RPM is only generated if LXC is enabled Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- libvirt.spec.in | 28 +++++++++++++++++++--------- 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/libvirt.spec.in b/libvirt.spec.in index 7abc315..fb4d46f 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1107,6 +1107,18 @@ Requires: cyrus-sasl-md5 Shared libraries and client binaries needed to access to the virtualization capabilities of recent versions of Linux (and other OSes). +%if %{with_lxc} +%package login-shell +Summary: Login shell for connecting users to an LXC container +Group: Development/Libraries +Requires: %{name}-client = %{version}-%{release} + +%description login-shell +Provides the set-uid virt-login-shell binary that is used to +connect a user to an LXC container when they login, by switching +namespaces. +%endif + %package devel Summary: Libraries, includes, etc. to compile with the libvirt library Group: Development/Libraries @@ -2015,23 +2027,14 @@ fi %doc COPYING COPYING.LESSER %config(noreplace) %{_sysconfdir}/libvirt/libvirt.conf -%if %{with_lxc} -%config(noreplace) %{_sysconfdir}/libvirt/virt-login-shell.conf -%endif %{_mandir}/man1/virsh.1* %{_mandir}/man1/virt-xml-validate.1* %{_mandir}/man1/virt-pki-validate.1* %{_mandir}/man1/virt-host-validate.1* -%if %{with_lxc} -%{_mandir}/man1/virt-login-shell.1* -%endif %{_bindir}/virsh %{_bindir}/virt-xml-validate %{_bindir}/virt-pki-validate %{_bindir}/virt-host-validate -%if %{with_lxc} -%attr(4755, root, root) %{_bindir}/virt-login-shell -%endif %{_libdir}/lib*.so.* %if %{with_dtrace} @@ -2075,6 +2078,13 @@ fi %config(noreplace) %{_sysconfdir}/sasl2/libvirt.conf %endif +%if %{with_lxc} +%files login-shell +%attr(4755, root, root) %{_bindir}/virt-login-shell +%config(noreplace) %{_sysconfdir}/libvirt/virt-login-shell.conf +%{_mandir}/man1/virt-login-shell.1* +%endif + %files devel %defattr(-, root, root) -- 1.8.3.1
participants (2)
-
Daniel P. Berrange -
Eric Blake