tools: split virt-login-shell into two binaries

The virt-login-shell binary is a setuid program that takes
no arguments. When invoked it looks at the invoking uid,
resolves it to a username, and finds an LXC guest with the
same name. It then starts the guest and runs the shell in
side the namespaces of the container.

Given this set of tasks the virt-login-shell binary needs
to connect to libvirtd, make various other libvirt API calls.
This is a problem for setuid binaries as various libraries
that libvirt.so links to are not safe. For example, they have
constructor functions which execute an unknown amount of code
that can be influenced by env variables.

For this reason virt-login-shell doesn't use libvirt.so,
but instead links to a custom, cut down, set of source files
sufficient to be a local client only.

This introduces a problem for integrating glib2 into libvirt
though, as once integrated, there would be no way to build
virt-login-shell without an external dependancy on glib2 and
this is definitely not setuid safe.

To resolve this problem, we split the virt-login-shell binary
into two parts. The first part is setuid and does almost
nothing. It simply records the original uid+gid, and then
invokes the virt-login-shell-helper binary. Crucially when
it does this it completes scrubs all environment variables.
It is thus safe for virt-login-shell-helper to link to the
normal libvirt.so. Any things that constructor functions
do cannot be influenced by user control env vars or cli
args.

Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

diff --git a/.gitignore b/.gitignore
index 727bfdb6ec7517b08bf35fb79a221ee2e4d38a5a..f3193173d690fb1f63d6afac54868dd431b03ba7 100644 (file)
--- a/.gitignore
+++ b/.gitignore
@@ -183,6 +183,7 @@
 /tests/test_conf
 /tools/libvirt-guests.sh
 /tools/virt-login-shell
+/tools/virt-login-shell-helper
 /tools/virsh
 /tools/virsh-*-edit.c
 /tools/virt-admin

diff --git a/cfg.mk b/cfg.mk
index 694ce2076f9164b2fb09e04f3479b6800add29d6..9130b4560b454f004f2582590362b290d158829c 100644 (file)
--- a/cfg.mk
+++ b/cfg.mk
@@ -1203,7 +1203,7 @@ exclude_file_name_regexp--sc_avoid_write = \
 
 exclude_file_name_regexp--sc_bindtextdomain = .*
 
-exclude_file_name_regexp--sc_gettext_init = ^(tests|examples)/
+exclude_file_name_regexp--sc_gettext_init = ^((tests|examples)/|tools/virt-login-shell.c)
 
 exclude_file_name_regexp--sc_copyright_format = \
        ^cfg\.mk$$
@@ -1229,7 +1229,7 @@ exclude_file_name_regexp--sc_prohibit_access_xok = \
        ^(cfg\.mk|src/util/virutil\.c)$$
 
 exclude_file_name_regexp--sc_prohibit_asprintf = \
-  ^(cfg\.mk|bootstrap.conf$$|examples/|src/util/virstring\.[ch]$$|tests/vircgroupmock\.c$$)
+  ^(cfg\.mk|bootstrap.conf$$|examples/|src/util/virstring\.[ch]$$|tests/vircgroupmock\.c|tools/virt-login-shell\.c$$)
 
 exclude_file_name_regexp--sc_prohibit_strdup = \
   ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c|tests/commandhelper\.c$$)
@@ -1256,7 +1256,7 @@ exclude_file_name_regexp--sc_prohibit_newline_at_end_of_diagnostic = \
   ^src/rpc/gendispatch\.pl$$
 
 exclude_file_name_regexp--sc_prohibit_nonreentrant = \
-  ^((po|tests|examples)/|docs/.*(py|js|html\.in)|run.in$$|tools/wireshark/util/genxdrstub\.pl$$)
+  ^((po|tests|examples)/|docs/.*(py|js|html\.in)|run.in$$|tools/wireshark/util/genxdrstub\.pl|tools/virt-login-shell\.c$$)
 
 exclude_file_name_regexp--sc_prohibit_select = \
        ^cfg\.mk$$
@@ -1270,7 +1270,7 @@ exclude_file_name_regexp--sc_prohibit_raw_allocation = \
 exclude_file_name_regexp--sc_prohibit_readlink = \
   ^src/(util/virutil|lxc/lxc_container)\.c$$
 
-exclude_file_name_regexp--sc_prohibit_setuid = ^src/util/virutil\.c$$
+exclude_file_name_regexp--sc_prohibit_setuid = ^src/util/virutil\.c|tools/virt-login-shell\.c$$
 
 exclude_file_name_regexp--sc_prohibit_sprintf = \
   ^(cfg\.mk|docs/hacking\.html\.in|.*\.stp|.*\.pl)$$
@@ -1317,7 +1317,7 @@ exclude_file_name_regexp--sc_prohibit_unsigned_pid = \
   ^(include/libvirt/.*\.h|src/(qemu/qemu_driver\.c|driver-hypervisor\.h|libvirt(-[a-z]*)?\.c|.*\.x|util/vir(polkit|systemd)\.c)|tests/virpolkittest\.c|tools/virsh-domain\.c)$$
 
 exclude_file_name_regexp--sc_prohibit_getenv = \
-  ^tests/.*\.[ch]$$
+  ^tests/.*\.[ch]|tools/virt-login-shell\.c$$
 
 exclude_file_name_regexp--sc_avoid_attribute_unused_in_header = \
   ^(src/util/virlog\.h|src/network/bridge_driver\.h)$$

diff --git a/libvirt.spec.in b/libvirt.spec.in
index 045c0fed1a6046617f56e1445306f3278903c7f8..6f96fbec33d3ddce2afb1d07c566dd5e1d7ff8d7 100644 (file)
--- a/libvirt.spec.in
+++ b/libvirt.spec.in
@@ -1847,6 +1847,7 @@ exit 0
 %if %{with_lxc}
 %files login-shell
 %attr(4750, root, virtlogin) %{_bindir}/virt-login-shell
+%{_libexecdir}/virt-login-shell-helper
 %config(noreplace) %{_sysconfdir}/libvirt/virt-login-shell.conf
 %{_mandir}/man1/virt-login-shell.1*
 %endif

diff --git a/tools/Makefile.am b/tools/Makefile.am
index 125540d31309fb047c43ea5a73db296b0d153bc8..3d9461ba65a09d2a344fae5c14a58837dd40417c 100644 (file)
--- a/tools/Makefile.am
+++ b/tools/Makefile.am
@@ -105,6 +105,7 @@ endif WITH_SANLOCK
 if WITH_LOGIN_SHELL
 conf_DATA += virt-login-shell.conf
 bin_PROGRAMS += virt-login-shell
+libexec_PROGRAMS = virt-login-shell-helper
 man1_MANS += virt-login-shell.1
 endif WITH_LOGIN_SHELL
 
@@ -192,25 +193,25 @@ virt_host_validate_CFLAGS = \
                $(AM_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 will be setuid, and must not link to anything
+# except glibc. It wil scrub the environment and then invoke the
+# real virt-login-shell-helper binary.
 virt_login_shell_SOURCES = \
+               virt-login-shell.c
+
+virt_login_shell_helper_SOURCES = \
                virt-login-shell-helper.c
 
-virt_login_shell_LDFLAGS = \
+virt_login_shell_helper_LDFLAGS = \
                $(AM_LDFLAGS) \
                $(PIE_LDFLAGS) \
                $(NULL)
-virt_login_shell_LDADD = \
-               $(STATIC_BINARIES) \
-               ../src/libvirt-setuid-rpc-client.la \
+virt_login_shell_helper_LDADD = \
+               ../src/libvirt.la \
+               ../src/libvirt-lxc.la \
                ../gnulib/lib/libgnu.la
 
-virt_login_shell_CFLAGS = \
-               -DLIBVIRT_SETUID_RPC_CLIENT \
+virt_login_shell_helper_CFLAGS = \
                $(AM_CFLAGS) \
                $(NULL)
 

diff --git a/tools/virt-login-shell-helper.c b/tools/virt-login-shell-helper.c
index f06eb1464af438c747ed467bafd0ff4902afa1e8..8dc3bedaa0f1f02b78f29d896f2717ada3853bcc 100644 (file)
--- a/tools/virt-login-shell-helper.c
+++ b/tools/virt-login-shell-helper.c
@@ -157,8 +157,10 @@ main(int argc, char **argv)
     pid_t cpid = -1;
     int ret = EXIT_CANCELED;
     int status;
-    uid_t uid = getuid();
-    gid_t gid = getgid();
+    unsigned long long uidval;
+    unsigned long long gidval;
+    uid_t uid;
+    gid_t gid;
     char *name = NULL;
     char **shargv = NULL;
     size_t shargvlen = 0;
@@ -199,6 +201,16 @@ main(int argc, char **argv)
     if (virGettextInitialize() < 0)
         return ret;
 
+    if (geteuid() != 0) {
+        fprintf(stderr, _("%s: must be run as root\n"), argv[0]);
+        return ret;
+    }
+
+    if (getuid() != 0) {
+        fprintf(stderr, _("%s: must not be run setuid root\n"), argv[0]);
+        return ret;
+    }
+
     while ((arg = getopt_long(argc, argv, "hVc:", opt, &longindex)) != -1) {
         switch (arg) {
         case 'h':
@@ -220,17 +232,29 @@ main(int argc, char **argv)
         }
     }
 
-    if (argc > optind) {
-        virReportSystemError(EINVAL, _("%s takes no options"), progname);
+    if (optind != (argc - 2)) {
+        virReportSystemError(EINVAL, _("%s expects UID and GID parameters"), progname);
         goto cleanup;
     }
 
-    if (uid == 0) {
-        virReportSystemError(EPERM, _("%s must be run by non root users"),
-                             progname);
+    if (virStrToLong_ull(argv[optind], NULL, 10, &uidval) < 0 ||
+        ((uid_t)uidval) != uidval) {
+        virReportSystemError(EINVAL, _("%s cannot parse UID '%s'"),
+                             progname, argv[optind]);
         goto cleanup;
     }
 
+    optind++;
+    if (virStrToLong_ull(argv[optind], NULL, 10, &gidval) < 0 ||
+        ((gid_t)gidval) != gidval) {
+        virReportSystemError(EINVAL, _("%s cannot parse GID '%s'"),
+                             progname, argv[optind]);
+        goto cleanup;
+    }
+
+    uid = (uid_t)uidval;
+    gid = (gid_t)gidval;
+
     name = virGetUserName(uid);
     if (!name)
         goto cleanup;

diff --git a/tools/virt-login-shell.c b/tools/virt-login-shell.c
new file mode 100644 (file)
index 0000000..41d0b34
--- /dev/null
+++ b/tools/virt-login-shell.c
@@ -0,0 +1,84 @@
+/*
+ * virt-login-shell.c: a setuid shell to connect to a container
+ *
+ * Copyright (C) 2019 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/>.
+ */
+
+#include <config.h>
+
+#include <unistd.h>
+#include <sys/types.h>
+#include <assert.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <errno.h>
+#include <string.h>
+
+#include "configmake.h"
+#include "intprops.h"
+
+int main(int argc, char **argv) {
+    char uidstr[INT_BUFSIZE_BOUND(uid_t)];
+    char gidstr[INT_BUFSIZE_BOUND(gid_t)];
+    const char *const newargv[] = {
+        LIBEXECDIR "/virt-login-shell-helper",
+        uidstr,
+        gidstr,
+        NULL,
+    };
+    char *newenv[] = {
+        NULL,
+        NULL,
+    };
+    char *term = getenv("TERM");
+
+    if (getuid() == 0 || getgid() == 0) {
+        fprintf(stderr, "%s: must not be run as root\n", argv[0]);
+        exit(EXIT_FAILURE);
+    }
+
+    if (geteuid() != 0) {
+        fprintf(stderr, "%s: must be run as setuid root\n", argv[0]);
+        exit(EXIT_FAILURE);
+    }
+
+    if (argc != 1) {
+        fprintf(stderr, "%s: no arguments expected\n", argv[0]);
+        exit(EXIT_FAILURE);
+    }
+
+    if (term &&
+        asprintf(&(newenv[0]), "TERM=%s", term) < 0) {
+        fprintf(stderr, "%s: cannot set TERM env variable: %s\n",
+                argv[0], strerror(errno));
+        exit(EXIT_FAILURE);
+    }
+
+    assert(snprintf(uidstr, sizeof(uidstr), "%d", getuid()) < sizeof(uidstr));
+    assert(snprintf(gidstr, sizeof(gidstr), "%d", getgid()) < sizeof(gidstr));
+
+    if (setuid(0) < 0) {
+        fprintf(stderr, "%s: unable to set real UID to root: %s\n",
+                argv[0], strerror(errno));
+        exit(EXIT_FAILURE);
+    }
+
+    execve(newargv[0], (char *const*)newargv, newenv);
+    fprintf(stderr, "%s: failed to run %s/virt-login-shell-helper: %s\n",
+            argv[0], LIBEXECDIR, strerror(errno));
+    exit(EXIT_FAILURE);
+}