[libvirt] Add patches to allow users to join running containers.

[PATCH 1/2] Add virGetUserDirectoryByUID to retrieve users homedir [PATCH 2/2] virt-login-shell joins users into lxc container. This patch implements most of the changes suggested by Dan Berrange and Eric Blake. Some replies to suggested changes. Removed mingw-libvirt.spec.in changes since virt lxc probably can not be supported in Windows. Not sure if I need to make changes so my code will not build on that platform. Did not make the changes to install virt-login-shell as 4755 automatically. I guess I want a more firm, make that change request... I did not make a helper function to parse a list of strings out of conf file. The getuid and getgid calls return the user that executed the program, when the app is setuid geteuid and getegid return "0". I believe getuid and getgid are correct. Added virt-login-shell --help, not sure what --program would do? The program is hard coded to LXC because there is no way that I know of for a ZZ process to join a running qemu instance. I have heard back from one security review from Miloslav Trmac, who had similar comments as Eric.

From: Dan Walsh <dwalsh@redhat.com> This function is needed for virt-login-shell. Also modify virGirUserDirectory to use the new function, to simplify the code. --- src/libvirt_private.syms | 1 + src/util/virutil.c | 9 +++++++-- src/util/virutil.h | 1 + 3 files changed, 9 insertions(+), 2 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index d9615ea..201b2cb 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2044,6 +2044,7 @@ virGetUnprivSGIOSysfsPath; virGetUserCacheDirectory; virGetUserConfigDirectory; virGetUserDirectory; +virGetUserDirectoryByUID; virGetUserID; virGetUserName; virGetUserRuntimeDirectory; diff --git a/src/util/virutil.c b/src/util/virutil.c index adcdb3c..99812ca 100644 --- a/src/util/virutil.c +++ b/src/util/virutil.c @@ -750,13 +750,18 @@ static char *virGetGroupEnt(gid_t gid) return ret; } -char *virGetUserDirectory(void) +char *virGetUserDirectoryByUID(uid_t uid) { char *ret; - virGetUserEnt(geteuid(), NULL, NULL, &ret); + virGetUserEnt(uid, NULL, NULL, &ret); return ret; } +char *virGetUserDirectory(void) +{ + return virGetUserDirectoryByUID(geteuid()); +} + static char *virGetXDGDirectory(const char *xdgenvname, const char *xdgdefdir) { const char *path = getenv(xdgenvname); diff --git a/src/util/virutil.h b/src/util/virutil.h index 526c610..4b06992 100644 --- a/src/util/virutil.h +++ b/src/util/virutil.h @@ -111,6 +111,7 @@ static inline int getgid (void) { return 0; } char *virGetHostname(void); char *virGetUserDirectory(void); +char *virGetUserDirectoryByUID(uid_t uid); char *virGetUserConfigDirectory(void); char *virGetUserCacheDirectory(void); char *virGetUserRuntimeDirectory(void); -- 1.8.3.1

On 07/30/2013 01:55 PM, dwalsh@redhat.com wrote:
From: Dan Walsh <dwalsh@redhat.com>
Subject line was a little long; I trimmed it to: util: add virGetUserDirectoryByUID
This function is needed for virt-login-shell. Also modify virGirUserDirectory to use the new function, to simplify the code. --- src/libvirt_private.syms | 1 + src/util/virutil.c | 9 +++++++-- src/util/virutil.h | 1 + 3 files changed, 9 insertions(+), 2 deletions(-)
Failed to build on mingw; here's what I squashed in, then pushed: diff --git i/src/util/virutil.c w/src/util/virutil.c index 99812ca..aef63c7 100644 --- i/src/util/virutil.c +++ w/src/util/virutil.c @@ -647,6 +647,11 @@ cleanup: return result; } +char *virGetUserDirectory(void) +{ + return virGetUserDirectoryByUID(geteuid()); +} + #ifdef HAVE_GETPWUID_R /* Look up fields from the user database for the given user. On * error, set errno, report the error, and return -1. */ @@ -757,11 +762,6 @@ char *virGetUserDirectoryByUID(uid_t uid) return ret; } -char *virGetUserDirectory(void) -{ - return virGetUserDirectoryByUID(geteuid()); -} - static char *virGetXDGDirectory(const char *xdgenvname, const char *xdgdefdir) { const char *path = getenv(xdgenvname); @@ -1097,8 +1097,11 @@ virGetWin32DirectoryRoot(char **path) char * -virGetUserDirectory(void) +virGetUserDirectoryByUID(uid_t uid ATTRIBUTE_UNUSED) { + /* Since Windows lacks setuid, and since we already fake + * geteuid(), we can safely assume that this is only called when + * querying about the current user */ const char *dir; char *ret; @@ -1182,7 +1185,7 @@ virGetUserRuntimeDirectory(void) # else /* !HAVE_GETPWUID_R && !WIN32 */ char * -virGetUserDirectory(void) +virGetUserDirectoryByUID(uid_t uid ATTRIBUTE_UNUSED) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("virGetUserDirectory is not available")); -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

From: Dan Walsh <dwalsh@redhat.com> Openshift wants to have their gears stuck into a container when they login to the system. virt-login-shell will join a running gear with the username of the person running it, or attempt to start the container if it is not running. (Currently containers do not exist if they are not running, so I can not test this feature. But the code is there). This tool needs to be setuid since joining a container (nsjoin) requires privs. The root user is not allowed to execute this command. When this tool is run by a normal user it will only join the "users" container. Only users who are listed as valid_users in /etc/libvirt/virt-login-shell.conf are allowed to join containers using this tool. By default no users are allowed. --- .gitignore | 1 + libvirt.spec.in | 3 + po/POTFILES.in | 1 + tools/Makefile.am | 30 +++- tools/virt-login-shell.c | 363 ++++++++++++++++++++++++++++++++++++++++++++ tools/virt-login-shell.conf | 26 ++++ tools/virt-login-shell.pod | 62 ++++++++ 7 files changed, 485 insertions(+), 1 deletion(-) create mode 100644 tools/virt-login-shell.c create mode 100644 tools/virt-login-shell.conf create mode 100644 tools/virt-login-shell.pod diff --git a/.gitignore b/.gitignore index 4c79de3..b09463a 100644 --- a/.gitignore +++ b/.gitignore @@ -214,6 +214,7 @@ /tools/libvirt-guests.init /tools/libvirt-guests.service /tools/libvirt-guests.sh +/tools/virt-login-shell /tools/virsh /tools/virsh-*-edit.c /tools/virt-*-validate diff --git a/libvirt.spec.in b/libvirt.spec.in index a3a831f..e69e87d 100644 --- a/libvirt.spec.in +++ b/libvirt.spec.in @@ -1974,14 +1974,17 @@ fi %doc AUTHORS ChangeLog.gz NEWS README COPYING COPYING.LESSER TODO %config(noreplace) %{_sysconfdir}/libvirt/libvirt.conf +%config(noreplace) %{_sysconfdir}/libvirt/virt-login-shell.conf %{_mandir}/man1/virsh.1* %{_mandir}/man1/virt-xml-validate.1* %{_mandir}/man1/virt-pki-validate.1* %{_mandir}/man1/virt-host-validate.1* +%{_mandir}/man1/virt-login-shell.1* %{_bindir}/virsh %{_bindir}/virt-xml-validate %{_bindir}/virt-pki-validate %{_bindir}/virt-host-validate +%attr(4755, root, root) %{_bindir}/virt-login-shell %{_libdir}/lib*.so.* %if %{with_dtrace} diff --git a/po/POTFILES.in b/po/POTFILES.in index 1fd84af..884b70a 100644 --- a/po/POTFILES.in +++ b/po/POTFILES.in @@ -231,3 +231,4 @@ tools/virt-host-validate-common.c tools/virt-host-validate-lxc.c tools/virt-host-validate-qemu.c tools/virt-host-validate.c +tools/virt-login-shell.c diff --git a/tools/Makefile.am b/tools/Makefile.am index 644a86d..00c582a 100644 --- a/tools/Makefile.am +++ b/tools/Makefile.am @@ -37,6 +37,7 @@ EXTRA_DIST = \ virt-pki-validate.in \ virt-sanlock-cleanup.in \ virt-sanlock-cleanup.8 \ + virt-login-shell.pod \ virsh.pod \ libvirt-guests.sysconf \ virsh-edit.c \ @@ -52,8 +53,11 @@ EXTRA_DIST = \ DISTCLEANFILES = +confdir = $(sysconfdir)/libvirt +conf_DATA = virt-login-shell.conf + bin_SCRIPTS = virt-xml-validate virt-pki-validate -bin_PROGRAMS = virsh virt-host-validate +bin_PROGRAMS = virsh virt-host-validate virt-login-shell libexec_SCRIPTS = libvirt-guests.sh if WITH_SANLOCK @@ -65,6 +69,7 @@ dist_man1_MANS = \ virt-host-validate.1 \ virt-pki-validate.1 \ virt-xml-validate.1 \ + virt-login-shell.1 \ virsh.1 if WITH_SANLOCK dist_man8_MANS = virt-sanlock-cleanup.8 @@ -128,6 +133,24 @@ virt_host_validate_CFLAGS = \ $(COVERAGE_CFLAGS) \ $(NULL) +virt_login_shell_SOURCES = \ + virt-login-shell.conf \ + virt-login-shell.c + +virt_login_shell_LDFLAGS = $(COVERAGE_LDFLAGS) +virt_login_shell_LDADD = \ + $(STATIC_BINARIES) \ + $(PIE_LDFLAGS) \ + $(RELRO_LDFLAGS) \ + ../src/libvirt.la \ + ../src/libvirt-lxc.la \ + ../gnulib/lib/libgnu.la + +virt_login_shell_CFLAGS = \ + $(WARN_CFLAGS) \ + $(PIE_CFLAGS) \ + $(COVERAGE_CFLAGS) + virsh_SOURCES = \ console.c console.h \ virsh.c virsh.h \ @@ -189,6 +212,11 @@ virsh_win_icon.$(OBJEXT): virsh_win_icon.rc --output-format coff --output $@ endif +virt-login-shell.1: virt-login-shell.pod $(top_srcdir)/configure.ac + $(AM_V_GEN)$(POD2MAN) $< $(srcdir)/$@ \ + && if grep 'POD ERROR' $(srcdir)/$@ ; then \ + rm $(srcdir)/$@; exit 1; fi + virsh.1: virsh.pod $(top_srcdir)/configure.ac $(AM_V_GEN)$(POD2MAN) $< $(srcdir)/$@ \ && if grep 'POD ERROR' $(srcdir)/$@ ; then \ diff --git a/tools/virt-login-shell.c b/tools/virt-login-shell.c new file mode 100644 index 0000000..862e90e --- /dev/null +++ b/tools/virt-login-shell.c @@ -0,0 +1,363 @@ +/* + * virt-login-shell.c: a shell to connect to a container + * + * 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/>. + * + * Daniel Walsh <dwalsh@redhat.com> + */ +#include <config.h> + +#include <stdarg.h> +#include <getopt.h> +#include <stdio.h> +#include <errno.h> +#include <stdlib.h> +#include <fnmatch.h> + +#include "internal.h" +#include "virerror.h" +#include "virconf.h" +#include "virutil.h" +#include "virfile.h" +#include "virprocess.h" +#include "configmake.h" +#include "virstring.h" +#include "viralloc.h" +#include "vircommand.h" +#define VIR_FROM_THIS VIR_FROM_NONE + +static ssize_t nfdlist = 0; +static int *fdlist = NULL; +static const char *conf_file = SYSCONFDIR "/libvirt/virt-login-shell.conf"; + +static void virLoginShellFini(virConnectPtr conn, virDomainPtr dom) +{ + size_t i; + + for (i = 0; i < nfdlist; i++) + VIR_FORCE_CLOSE(fdlist[i]); + VIR_FREE(fdlist); + nfdlist = 0; + if (dom) + virDomainFree(dom); + if (conn) + virConnectClose(conn); +} + +static int virLoginShellAllowedUser(virConfPtr conf, + uid_t uid, + gid_t gid, + const char *name) +{ + virConfValuePtr p; + int ret = -1; + char *ptr = NULL; + size_t i; + gid_t *groups = NULL; + char *gname = NULL; + + p = virConfGetValue(conf, "allowed_users"); + if (p && p->type == VIR_CONF_LIST) { + virConfValuePtr pp; + + /* Calc length and check items */ + for (pp = p->list; pp; pp = pp->next) { + if (pp->type != VIR_CONF_STRING) { + virReportSystemError(EINVAL, "%s", _("shell must be a list of strings\n")); + goto cleanup; + } else { + /* + If string begins with a % this indicates a linux group. + Check to see if the user is in the Linux Group. + */ + if (pp->str[0] == '%') { + ptr = &pp->str[1]; + if (!ptr) + continue; + if (virGetGroupList(uid, gid, &groups) < 0) { + goto cleanup; + } + for (i = 0; groups[i]; i++) { + if (!(gname = virGetGroupName(groups[i]))) + continue; + if (fnmatch(ptr, gname, 0) == 0) { + ret = 0; + goto cleanup; + } + VIR_FREE(gname); + } + VIR_FREE(groups); + continue; + } + if (fnmatch(pp->str, name, 0) == 0) { + ret = 0; + goto cleanup; + } + } + } + } + virReportSystemError(EPERM, _("%s not listed as an allowed_users in %s"), name, conf_file); + errno = EPERM; +cleanup: + VIR_FREE(gname); + VIR_FREE(groups); + return ret; +} + +static char **virLoginShellGetShellArgv(virConfPtr conf) +{ + size_t i; + char **shargv=NULL; + virConfValuePtr p; + + p = virConfGetValue(conf, "shell"); + if (!p) + return virStringSplit("/bin/sh -l", " ", 3); + + if (p && p->type == VIR_CONF_LIST) { + size_t len; + virConfValuePtr pp; + + /* Calc length and check items */ + for (len = 0, pp = p->list; pp; len++, pp = pp->next) { + if (pp->type != VIR_CONF_STRING) { + virReportSystemError(EINVAL, "%s", _("shell must be a list of strings\n")); + goto error; + } + } + + if (VIR_ALLOC_N(shargv, len + 1) < 0) + goto error; + for (i = 0, pp = p->list; pp; i++, pp = pp->next) { + if (VIR_STRDUP(shargv[i], pp->str) < 0) + goto error; + } + shargv[len] = NULL; + } + return shargv; +error: + virStringFreeList(shargv); + return NULL; +} + +static char *progname; + +/* + * Print usage + */ +static void +usage(void) +{ + fprintf(stdout, _("\n" + "%s is a privileged program that allows non root users \n" + "specified in %s to join a Linux container \n" + "with a matching user name and launch a shell. \n" + "\n%s [options]\n\n" + " options:\n" + " -h | --help this help:\n\n"), progname, conf_file, progname); + return; +} + +int +main(int argc, char **argv) +{ + virConfPtr conf = NULL; + const char *login_shell_path = conf_file; + pid_t cpid; + int ret = EXIT_FAILURE; + int status; + int status2; + uid_t uid = getuid(); + gid_t gid = getgid(); + char *name = NULL; + char **shargv = NULL; + virSecurityModelPtr secmodel = NULL; + virSecurityLabelPtr seclabel = NULL; + virDomainPtr dom = NULL; + virConnectPtr conn = NULL; + char *homedir = NULL; + int arg; + int longindex = -1; + + struct option opt[] = { + {"help", no_argument, NULL, 'h'}, + {NULL, 0, NULL, 0} + }; + virReportSystemError(1, "%s %d %s", "Test1", argc, argv[0]); + if (virInitialize() < 0) { + fprintf(stderr, _("Failed to initialize libvirt Error Handling")); + return EXIT_FAILURE; + } + virReportSystemError(EINVAL, "%s", "Test2"); + + if (virErrorInitialize() < 0) { + fprintf(stderr, _("Failed to initialize libvirt Error Handling")); + return EXIT_FAILURE; + } + virSetErrorFunc(NULL, NULL); + virSetErrorLogPriorityFunc(NULL); + + progname = argv[0]; + if (!setlocale(LC_ALL, "")) { + perror("setlocale"); + /* failure to setup locale is not fatal */ + } + if (!bindtextdomain(PACKAGE, LOCALEDIR)) { + perror("bindtextdomain"); + return ret; + } + if (!textdomain(PACKAGE)) { + perror("textdomain"); + return ret; + } + + /* The only option we support is help + */ + while ((arg = getopt_long(argc, argv, "h", opt, &longindex)) != -1) { + switch (arg) { + case 'h': + usage(); + exit(EXIT_SUCCESS); + break; + } + } + + if (argc > optind) { + virReportSystemError(EINVAL, _("%s takes no options"), progname); + errno = EINVAL; + goto cleanup; + } + + if (uid == 0) { + virReportSystemError(EPERM, _("%s must be run by non root users"), progname); + + errno = EPERM; + goto cleanup; + } + + name = virGetUserName(uid); + if (!name) + goto cleanup; + + homedir = virGetUserDirectoryByUID(uid); + if (!homedir) + goto cleanup; + + if (!(conf = virConfReadFile(login_shell_path, 0))) + goto cleanup; + + if (virLoginShellAllowedUser(conf, uid, gid, name) < 0) + goto cleanup; + + if (!(shargv = virLoginShellGetShellArgv(conf))) + goto cleanup; + + conn = virConnectOpen("lxc:///"); + if (!conn) + goto cleanup; + + dom = virDomainLookupByName(conn, name); + if (!dom) + goto cleanup; + + if (!virDomainIsActive(dom) && virDomainCreate(dom)) { + virErrorPtr last_error; + last_error = virGetLastError(); + if (last_error->code != VIR_ERR_OPERATION_INVALID) { + virReportSystemError(last_error->code,_("Can't create %s container: %s\n"), name, virGetLastErrorMessage()); + goto cleanup; + } + } + + if ((nfdlist = virDomainLxcOpenNamespace(dom, &fdlist, 0)) < 0) + goto cleanup; + if (VIR_ALLOC(secmodel) < 0) + goto cleanup; + if (VIR_ALLOC(seclabel) < 0) + goto cleanup; + if (virNodeGetSecurityModel(conn, secmodel) < 0) + goto cleanup; + if (virDomainGetSecurityLabel(dom, seclabel) < 0) + goto cleanup; + + if (virFork(&cpid) < 0) + goto cleanup; + + if (cpid == 0) { + gid_t *groups = NULL; + int ngroups; + pid_t ccpid; + + /* Fork once because we don't want to affect + * virt-login-shell's namespace itself + */ + if (virSetUIDGID(0, 0, NULL, 0) < 0) + return EXIT_FAILURE; + + if (virDomainLxcEnterSecurityLabel(secmodel, + seclabel, + NULL, + 0) < 0) + return EXIT_FAILURE; + + if (nfdlist > 0) { + if (virDomainLxcEnterNamespace(dom, + nfdlist, + fdlist, + NULL, + NULL, + 0) < 0) + return EXIT_FAILURE; + } + + if ((ngroups = virGetGroupList(uid, gid, &groups)) < 0) + return EXIT_FAILURE; + + ret = virSetUIDGID(uid, gid, groups, ngroups); + VIR_FREE(groups); + if (ret < 0) + return EXIT_FAILURE; + + if (virFork(&ccpid) < 0) + return EXIT_FAILURE; + + if (ccpid == 0) { + if (chdir(homedir) < 0) { + virReportSystemError(errno, _("Unable chdir(%s)"), homedir); + return EXIT_FAILURE; + } + if (execv(shargv[0], (char *const*) shargv) < 0) { + virReportSystemError(errno, _("Unable exec shell %s"), shargv[0]); + return -errno; + } + } + return virProcessWait(ccpid, &status2); + } + ret = virProcessWait(cpid, &status); + +cleanup: + virConfFree(conf); + virLoginShellFini(conn, dom); + virStringFreeList(shargv); + VIR_FREE(name); + VIR_FREE(homedir); + VIR_FREE(seclabel); + VIR_FREE(secmodel); + if (ret) + virDispatchError(NULL); + return ret; +} diff --git a/tools/virt-login-shell.conf b/tools/virt-login-shell.conf new file mode 100644 index 0000000..835fd3f --- /dev/null +++ b/tools/virt-login-shell.conf @@ -0,0 +1,26 @@ +# Master configuration file for the virt-login-shell program. +# All settings described here are optional - if omitted, sensible +# defaults are used. + +# By default, virt-login-shell will connect you to a container running +# with the /bin/sh program. Modify the shell variable if you want your +# users to run a different shell or a setup container when joining a +# container. Shell commands must be a list of commands/options separated by +# comma and delimited by square brackets. Defaults to: /bin/sh -l. +# Modify and uncomment the following to modify the login shell. +# shell = [ "/bin/sh", "-l" ] + +# allowed_users specifies the user names of all users that are allowed to +# execute virt-login-shell. You can specify the users as a comma +# separated list of usernames or user groups. +# The list of names support glob syntax. +# To disallow all users (default) +# allowed_users = [] +# If you do not specify any names (default) then no one is allowed +# to use this executable. +# To allow fred and joe only +# allowed_users = ["fred", "joe"] +# To allow all users within a specific group prefix the group name with %. +# allowed_users = ["%engineers"] +# To allow all users specify the following +# allowed_users = [ "*" ] diff --git a/tools/virt-login-shell.pod b/tools/virt-login-shell.pod new file mode 100644 index 0000000..0cd35cf --- /dev/null +++ b/tools/virt-login-shell.pod @@ -0,0 +1,62 @@ +=head1 NAME + +virt-login-shell - tool to execute a shell within a container matching the users name + +=head1 SYNOPSIS + +B<virt-login-shell> + +=head1 DESCRIPTION + +The B<virt-login-shell> program is setuid shell that is used to join +an LXC container that matches the users name. If the container is not +running virt-login-shell will attempt to start the container. +virt-sandbox-shell is not allowed to be run by root. Normal users will get +added to a container that matches their username, if it exists. And they are +configured in /etc/libvirt/virt-login-shell.conf. + +The basic structure of most virt-login-shell usage is: + + virt-login-shell + +=head1 CONFIG + +By default, virt-login-shell will execute the /bin/sh program for the user. +You can modify this behaviour by defining the shell variable in /etc/libvirt/virt-login-shell.conf. + +eg. shell = [ "/bin/ksh", "--login"] + +By default no users are allowed to user virt-login-shell, if you want to allow +certain users to use virt-login-shell, you need to modify the allowed_users variable in /etc/libvirt/virt-login-shell.conf. + +eg. allowed_users = [ "tom", "dick", "harry" ] + +=head1 BUGS + +Report any bugs discovered to the libvirt community via the mailing +list C<http://libvirt.org/contact.html> or bug tracker C<http://libvirt.org/bugs.html>. +Alternatively report bugs to your software distributor / vendor. + +=head1 AUTHORS + + Please refer to the AUTHORS file distributed with libvirt. + + Daniel Walsh <dwalsh at redhat dot com> + +=head1 COPYRIGHT + +Copyright (C) 2013 Red Hat, Inc., and the authors listed in the +libvirt AUTHORS file. + +=head1 LICENSE + +virt-login-shell is distributed under the terms of the GNU LGPL v2+. +This is free software; see the source for copying conditions. There +is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR +PURPOSE + +=head1 SEE ALSO + +L<virsh(1)>, L<http://www.libvirt.org/> + +=cut -- 1.8.3.1

On Tue, Jul 30, 2013 at 03:55:45PM -0400, dwalsh@redhat.com wrote:
From: Dan Walsh <dwalsh@redhat.com>
Openshift wants to have their gears stuck into a container when they login to the system. virt-login-shell will join a running gear with the username of the person running it, or attempt to start the container if it is not running. (Currently containers do not exist if they are not running, so I can not test this feature. But the code is there).
This tool needs to be setuid since joining a container (nsjoin) requires privs. The root user is not allowed to execute this command. When this tool is run by a normal user it will only join the "users" container.
Only users who are listed as valid_users in /etc/libvirt/virt-login-shell.conf are allowed to join containers using this tool. By default no users are allowed. --- +static ssize_t nfdlist = 0; +static int *fdlist = NULL; +static const char *conf_file = SYSCONFDIR "/libvirt/virt-login-shell.conf"; + +static void virLoginShellFini(virConnectPtr conn, virDomainPtr dom) +{ + size_t i; + + for (i = 0; i < nfdlist; i++) + VIR_FORCE_CLOSE(fdlist[i]); + VIR_FREE(fdlist); + nfdlist = 0; + if (dom) + virDomainFree(dom); + if (conn) + virConnectClose(conn);
There are lots of tabs used for indent here and later in this file. Please check with 'make syntax-check'.
+static int virLoginShellAllowedUser(virConfPtr conf, + uid_t uid, + gid_t gid, + const char *name) +{ + virConfValuePtr p; + int ret = -1; + char *ptr = NULL; + size_t i; + gid_t *groups = NULL; + char *gname = NULL; + + p = virConfGetValue(conf, "allowed_users"); + if (p && p->type == VIR_CONF_LIST) { + virConfValuePtr pp; + + /* Calc length and check items */ + for (pp = p->list; pp; pp = pp->next) { + if (pp->type != VIR_CONF_STRING) { + virReportSystemError(EINVAL, "%s", _("shell must be a list of strings\n")); + goto cleanup; + } else { + /* + If string begins with a % this indicates a linux group. + Check to see if the user is in the Linux Group. + */ + if (pp->str[0] == '%') { + ptr = &pp->str[1]; + if (!ptr) + continue; + if (virGetGroupList(uid, gid, &groups) < 0) { + goto cleanup; + } + for (i = 0; groups[i]; i++) { + if (!(gname = virGetGroupName(groups[i]))) + continue; + if (fnmatch(ptr, gname, 0) == 0) { + ret = 0; + goto cleanup; + } + VIR_FREE(gname); + } + VIR_FREE(groups); + continue; + } + if (fnmatch(pp->str, name, 0) == 0) { + ret = 0; + goto cleanup; + } + } + } + } + virReportSystemError(EPERM, _("%s not listed as an allowed_users in %s"), name, conf_file); + errno = EPERM;
No need to set errno if you using virReportSystemError().
+cleanup: + VIR_FREE(gname); + VIR_FREE(groups); + return ret; +} +
+int +main(int argc, char **argv) +{
+ + struct option opt[] = { + {"help", no_argument, NULL, 'h'}, + {NULL, 0, NULL, 0} + }; + virReportSystemError(1, "%s %d %s", "Test1", argc, argv[0]);
Accidental debug line left in I presume
+ if (virInitialize() < 0) { + fprintf(stderr, _("Failed to initialize libvirt Error Handling")); + return EXIT_FAILURE; + } + virReportSystemError(EINVAL, "%s", "Test2");
And here
+ + if (virErrorInitialize() < 0) { + fprintf(stderr, _("Failed to initialize libvirt Error Handling")); + return EXIT_FAILURE; + }
Actually no need to call virErrorInitialize() - virInitialize takes care of that for you
+ if (uid == 0) { + virReportSystemError(EPERM, _("%s must be run by non root users"), progname); + + errno = EPERM;
No need to set errno.
+ if (virFork(&cpid) < 0) + goto cleanup; + + if (cpid == 0) { + gid_t *groups = NULL; + int ngroups; + pid_t ccpid; + + /* Fork once because we don't want to affect + * virt-login-shell's namespace itself + */ + if (virSetUIDGID(0, 0, NULL, 0) < 0) + return EXIT_FAILURE; + + if (virDomainLxcEnterSecurityLabel(secmodel, + seclabel, + NULL, + 0) < 0) + return EXIT_FAILURE; + + if (nfdlist > 0) { + if (virDomainLxcEnterNamespace(dom, + nfdlist, + fdlist, + NULL, + NULL, + 0) < 0) + return EXIT_FAILURE; + } + + if ((ngroups = virGetGroupList(uid, gid, &groups)) < 0) + return EXIT_FAILURE;
virGetGroupList isn't safe to call after fork(). Need to move it earlier in this function.
+ + ret = virSetUIDGID(uid, gid, groups, ngroups); + VIR_FREE(groups); + if (ret < 0) + return EXIT_FAILURE; + + if (virFork(&ccpid) < 0) + return EXIT_FAILURE; + + if (ccpid == 0) { + if (chdir(homedir) < 0) { + virReportSystemError(errno, _("Unable chdir(%s)"), homedir); + return EXIT_FAILURE; + } + if (execv(shargv[0], (char *const*) shargv) < 0) { + virReportSystemError(errno, _("Unable exec shell %s"), shargv[0]); + return -errno; + } + } + return virProcessWait(ccpid, &status2); + } + ret = virProcessWait(cpid, &status); +
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 :|

On 07/31/2013 12:57 PM, Daniel P. Berrange wrote:
+ if (virFork(&cpid) < 0) + goto cleanup; + ... + } + + if ((ngroups = virGetGroupList(uid, gid, &groups)) < 0) + return EXIT_FAILURE;
virGetGroupList isn't safe to call after fork(). Need to move it earlier in this function.
Technically, it is only unsafe to call after fork() if the parent process was multi-threaded. But as virt-login-shell is single-threaded, we can get away with it here. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Wed, Jul 31, 2013 at 01:15:51PM -0600, Eric Blake wrote:
On 07/31/2013 12:57 PM, Daniel P. Berrange wrote:
+ if (virFork(&cpid) < 0) + goto cleanup; + ... + } + + if ((ngroups = virGetGroupList(uid, gid, &groups)) < 0) + return EXIT_FAILURE;
virGetGroupList isn't safe to call after fork(). Need to move it earlier in this function.
Technically, it is only unsafe to call after fork() if the parent process was multi-threaded. But as virt-login-shell is single-threaded, we can get away with it here.
I work on the assumption that all programs are multi-threaded, or will be at some point in their future, even if the developer doesn't think they are/willbe. You never know when some function/library you call may secretly create a thread behind your back. For example, virsh was single threaded, until we put in the special background thread to run the event loop in. 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 :|

On 07/30/2013 01:55 PM, dwalsh@redhat.com wrote:
From: Dan Walsh <dwalsh@redhat.com>
Openshift wants to have their gears stuck into a container when they login to the system. virt-login-shell will join a running gear with the username of the person running it, or attempt to start the container if it is not running. (Currently containers do not exist if they are not running, so I can not test this feature. But the code is there).
This tool needs to be setuid since joining a container (nsjoin) requires privs. The root user is not allowed to execute this command. When this tool is run by a normal user it will only join the "users" container.
Only users who are listed as valid_users in /etc/libvirt/virt-login-shell.conf are allowed to join containers using this tool. By default no users are allowed.
Problem. This is how things get installed: # ls -ld /etc/libvirt/ /etc/libvirt/virt-login-shell.conf /bin/virt-login-shell -rwsr-x---. 1 root virtlogin 891744 Dec 4 01:37 /bin/virt-login-shell drwx------. 6 root root 4096 Dec 23 13:22 /etc/libvirt/ -rw-r--r--. 1 root root 1244 Dec 23 13:22 /etc/libvirt/virt-login-shell.conf But looking at main():
+ + if (uid == 0) { + virReportSystemError(EPERM, _("%s must be run by non root users"), progname); + + errno = EPERM; + goto cleanup; + }
So root cannot run this program...
+ + if (!(conf = virConfReadFile(login_shell_path, 0))) + goto cleanup;
...and non-root invariably fails here, since login_shell_path (/etc/libvirt/virt-login-shell.conf) is buried inside a directory that is not searchable by either root or virtlogin. How on earth did you test this program? It flat out doesn't work, unless we change our installation permissions. Never mind that we also broke it while trying to fix CVE-2013-4400 - even with that damage fixed, this shell is completely worthless out of the box for all released versions of libvirt. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 12/23/2013 03:12 PM, Eric Blake wrote:
Only users who are listed as valid_users in /etc/libvirt/virt-login-shell.conf are allowed to join containers using this tool. By default no users are allowed.
Problem. This is how things get installed:
# ls -ld /etc/libvirt/ /etc/libvirt/virt-login-shell.conf /bin/virt-login-shell -rwsr-x---. 1 root virtlogin 891744 Dec 4 01:37 /bin/virt-login-shell drwx------. 6 root root 4096 Dec 23 13:22 /etc/libvirt/ -rw-r--r--. 1 root root 1244 Dec 23 13:22 /etc/libvirt/virt-login-shell.conf
+ if (!(conf = virConfReadFile(login_shell_path, 0))) + goto cleanup;
...and non-root invariably fails here, since login_shell_path (/etc/libvirt/virt-login-shell.conf) is buried inside a directory that is not searchable by either root or virtlogin.
Ah, I see - non-root fails here if run unprivileged (such as under gdb), but when run setuid it has the permissions of root and can read the file just fine. So this is a case where we are really relying on ALL of the setuid power, rather than one where we could use capability labeling on the binary rather than a full-blown setuid, making it harder to minimize the power of the binary on systems that try to avoid setuid by use of caps. It's also making my life much tougher to try and debug the other bugs in this program. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 12/23/2013 03:17 PM, Eric Blake wrote:
+ if (!(conf = virConfReadFile(login_shell_path, 0))) + goto cleanup;
...and non-root invariably fails here, since login_shell_path (/etc/libvirt/virt-login-shell.conf) is buried inside a directory that is not searchable by either root or virtlogin.
Ah, I see - non-root fails here if run unprivileged (such as under gdb), but when run setuid it has the permissions of root and can read the file just fine.
Then again, when run as setuid, it's not even getting past virInitialize(). :( At least I managed to figure out how to debug things: I recompiled with a sleep() at the beginning, gave my just-compiled binary the same setuid permissions as the installed binary, and then attach gdb (as root, since non-root can't ptrace a running setuid binary for obvious reasons). So I suspect that the failure in virInitialize() is yet more fallout from the CVE-2013-4400 patches being untested. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 12/23/2013 05:44 PM, Eric Blake wrote:
On 12/23/2013 03:17 PM, Eric Blake wrote:
+ if (!(conf = virConfReadFile(login_shell_path, 0))) + goto cleanup;
...and non-root invariably fails here, since login_shell_path (/etc/libvirt/virt-login-shell.conf) is buried inside a directory that is not searchable by either root or virtlogin.
Ah, I see - non-root fails here if run unprivileged (such as under gdb), but when run setuid it has the permissions of root and can read the file just fine.
Maybe need to give it cap_dac_read_search? /* Overrides all DAC restrictions regarding read and search on files and directories, including ACL restrictions if [_POSIX_ACL] is defined. Excluding DAC access covered by CAP_LINUX_IMMUTABLE. */ #define CAP_DAC_READ_SEARCH 2
Then again, when run as setuid, it's not even getting past virInitialize(). :(
At least I managed to figure out how to debug things: I recompiled with a sleep() at the beginning, gave my just-compiled binary the same setuid permissions as the installed binary, and then attach gdb (as root, since non-root can't ptrace a running setuid binary for obvious reasons). So I suspect that the failure in virInitialize() is yet more fallout from the CVE-2013-4400 patches being untested.
-----BEGIN PGP SIGNATURE----- Version: GnuPG v1 Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iEYEARECAAYFAlLFgzgACgkQrlYvE4MpobNyiACfRJWSEAnfiKooQS7ujZnkmAiq +JIAoLmKB5nZl+Nj6QSHww870OOZJhK/ =4uBh -----END PGP SIGNATURE-----

On 01/02/2014 08:18 AM, Daniel J Walsh wrote:
On 12/23/2013 05:44 PM, Eric Blake wrote:
On 12/23/2013 03:17 PM, Eric Blake wrote:
+ if (!(conf = virConfReadFile(login_shell_path, 0))) + goto cleanup;
...and non-root invariably fails here, since login_shell_path (/etc/libvirt/virt-login-shell.conf) is buried inside a directory that is not searchable by either root or virtlogin.
Ah, I see - non-root fails here if run unprivileged (such as under gdb), but when run setuid it has the permissions of root and can read the file just fine.
Maybe need to give it cap_dac_read_search?
/* Overrides all DAC restrictions regarding read and search on files and directories, including ACL restrictions if [_POSIX_ACL] is defined. Excluding DAC access covered by CAP_LINUX_IMMUTABLE. */
#define CAP_DAC_READ_SEARCH 2
Nah, I was able to fix the issue without needing any more caps: https://www.redhat.com/archives/libvir-list/2013-December/msg01243.html -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (4)
-
Daniel J Walsh
-
Daniel P. Berrange
-
dwalsh@redhat.com
-
Eric Blake