This sets up some basic support in libvirtd for dropping privileges
by removing capabilities, or changing uid/gid of the process. It
needed a little movement of existing code to allow us to drop
privileges in between initializing the daemon and initializing the
drivers.
As I mentioned in the first mail, this patch doesn't really improve
security of the daemon, since we keep CAP_DAC_OVERRIDE, CAP_SYS_ADMIN
and CAP_NET_ADMIN. I've put comments inline showing why I chose to
keep/exclude each capability.
I also added a helper to util.c for resolving a name to a gid/uid.
Ignore all the printfs() in the code, those will be removed later
before I submit this again...
qemud/Makefile.am | 5
qemud/qemud.c | 278 ++++++++++++++++++++++++++++++++++++++++++-----
src/libvirt_private.syms | 2
src/util.c | 73 ++++++++++++
src/util.h | 7 +
5 files changed, 339 insertions(+), 26 deletions(-)
Daniel
diff -r 75253f120589 qemud/Makefile.am
--- a/qemud/Makefile.am Mon Jun 22 19:00:54 2009 +0100
+++ b/qemud/Makefile.am Mon Jun 22 20:24:38 2009 +0100
@@ -88,7 +88,7 @@ libvirtd_CFLAGS = \
-I$(top_srcdir)/include -I$(top_builddir)/include \
-I$(top_srcdir)/src \
$(LIBXML_CFLAGS) $(GNUTLS_CFLAGS) $(SASL_CFLAGS) \
- $(POLKIT_CFLAGS) \
+ $(POLKIT_CFLAGS) $(CAPNG_CFLAGS) \
$(WARN_CFLAGS) -DLOCAL_STATE_DIR="\"$(localstatedir)\"" \
$(COVERAGE_CFLAGS) \
-DSYSCONF_DIR="\"$(sysconfdir)\"" \
@@ -104,7 +104,8 @@ libvirtd_LDADD = \
$(LIBXML_LIBS) \
$(GNUTLS_LIBS) \
$(SASL_LIBS) \
- $(POLKIT_LIBS)
+ $(POLKIT_LIBS) \
+ $(CAPNG_LIBS)
if WITH_DRIVER_MODULES
libvirtd_LDADD += ../src/libvirt_driver.la
diff -r 75253f120589 qemud/qemud.c
--- a/qemud/qemud.c Mon Jun 22 19:00:54 2009 +0100
+++ b/qemud/qemud.c Mon Jun 22 20:24:38 2009 +0100
@@ -115,6 +115,14 @@ static int unix_sock_ro_mask = 0666;
#else
+#if HAVE_CAPNG
+#include <cap-ng.h>
+#include <sys/prctl.h>
+
+#define SYSTEM_USER "libvirtd"
+#define SYSTEM_GROUP "libvirtd"
+#endif
+
static gid_t unix_sock_gid = 0; /* Only root by default */
static int unix_sock_rw_mask = 0700; /* Allow user only */
static int unix_sock_ro_mask = 0777; /* Allow world */
@@ -769,9 +777,14 @@ static int qemudInitPaths(struct qemud_s
return ret;
}
+
+/* Server initialization, may run privileged */
static struct qemud_server *qemudInitialize(int sigread) {
struct qemud_server *server;
+ /* Initializes threading, logging, & random number generator */
+ virInitialize();
+
if (VIR_ALLOC(server) < 0) {
VIR_ERROR0(_("Failed to allocate struct qemud_server"));
return NULL;
@@ -796,8 +809,20 @@ static struct qemud_server *qemudInitial
return NULL;
}
- virInitialize();
+ virEventRegisterImpl(virEventAddHandleImpl,
+ virEventUpdateHandleImpl,
+ virEventRemoveHandleImpl,
+ virEventAddTimeoutImpl,
+ virEventUpdateTimeoutImpl,
+ virEventRemoveTimeoutImpl);
+ return server;
+}
+
+
+/* Server initialization unprivileged/lower privileges */
+static void qemudInitializeDrivers(void)
+{
/*
* Note that the order is important: the first ones have a higher
* priority when calling virStateInitialize. We must register
@@ -842,17 +867,6 @@ static struct qemud_server *qemudInitial
oneRegister();
#endif
#endif
-
- virEventRegisterImpl(virEventAddHandleImpl,
- virEventUpdateHandleImpl,
- virEventRemoveHandleImpl,
- virEventAddTimeoutImpl,
- virEventUpdateTimeoutImpl,
- virEventRemoveTimeoutImpl);
-
- virStateInitialize(server->privileged);
-
- return server;
}
static struct qemud_server *qemudNetworkInit(struct qemud_server *server) {
@@ -2694,10 +2708,211 @@ version (const char *argv0)
printf ("%s (%s) %s\n", argv0, PACKAGE_NAME, PACKAGE_VERSION);
}
+#ifdef HAVE_CAPNG
+static void
+qemudPrintCaps(const char *msg)
+{
+ printf("%s\n", msg);
+ capng_print_caps_numeric(CAPNG_PRINT_STDOUT, CAPNG_SELECT_BOTH);
+ printf("Effective: ");
+ capng_print_caps_text(CAPNG_PRINT_STDOUT, CAPNG_EFFECTIVE);
+ printf("\n");
+ printf("Permitted: ");
+ capng_print_caps_text(CAPNG_PRINT_STDOUT, CAPNG_PERMITTED);
+ printf("\n");
+ printf("Inheritable: ");
+ capng_print_caps_text(CAPNG_PRINT_STDOUT, CAPNG_INHERITABLE);
+ printf("\n");
+ printf("Bounding: ");
+ capng_print_caps_text(CAPNG_PRINT_STDOUT, CAPNG_BOUNDING_SET);
+ printf("\n");
+}
+
+
+static int
+qemudDropPrivileges(struct qemud_server *server ATTRIBUTE_UNUSED)
+{
+ uid_t uid;
+ gid_t gid;
+ int ret;
+
+ if (virGetUserID(NULL, SYSTEM_USER, &uid) < 0) {
+ VIR_ERROR("cannot lookup '%s' user", SYSTEM_USER);
+ return -1;
+ }
+ if (virGetGroupID(NULL, SYSTEM_GROUP, &gid) < 0) {
+ VIR_ERROR("cannot lookup '%s' group", SYSTEM_GROUP);
+ return -1;
+ }
+
+ capng_get_caps_process();
+ qemudPrintCaps("Initial");
+
+ capng_clear(CAPNG_SELECT_BOTH);
+
+ /*
+ * The libvirtd daemon needs a hell of alot of capabilties for all
+ * its various functions. This is less than satisfactory.
+ *
+ * We also need to pass some of these capabilities onto children
+ * that we spawn. virExec() needs to help us
+ *
+ * We're going to need to make this more configurable later.
+ */
+
+ if (capng_updatev(CAPNG_ADD,
+ CAPNG_EFFECTIVE|CAPNG_PERMITTED|
+ CAPNG_INHERITABLE|CAPNG_BOUNDING_SET,
+ /* For storage APIs.
+ * XXX these bits need to be configurable too */
+ CAP_CHOWN,
+ CAP_DAC_OVERRIDE,
+ CAP_DAC_READ_SEARCH,
+ CAP_FOWNER,
+ CAP_FSETID,
+
+ /* Need to kill arbitrary qemu/uml/etc processes as non-libvirt UID */
+ CAP_KILL,
+
+ /* Need to spawn qemu/uml as non-libvirt GID/UID */
+ CAP_SETGID,
+ CAP_SETUID,
+
+ /* Need this in order to change our bounding set later */
+ CAP_SETPCAP,
+
+ /* Not even storage APIs should need this */
+ /*CAP_LINUX_IMMUTABLE,*/
+
+ /* No, our ports are all > 1024 */
+ /*CAP_NET_BIND_SERVICE,*/
+
+ /* No need to broadcast/multicast - delegated to avahi over DBus (hopefully) */
+ /*CAP_NET_BROADCAST,*/
+
+ /* Annoyingly need this hugely permissive role to
+ * configure bridges & tap devices
+ * XXX this bit needs to be configurable for sure*/
+ CAP_NET_ADMIN,
+
+ /* Pretty sure we don't need raw sockets */
+ /*CAP_NET_RAW,*/
+
+ /* Xen driver needs to mlock() memory for hypercalls */
+ /* XXX, perhaps its better to just set a ulimit */
+ CAP_IPC_LOCK,
+
+ /*/* No need for IPC stuff */
+ /*CAP_IPC_OWNER,*/
+
+ /* Really don't want to be in loading kernel modules business
+ * Lets mandate pre-load of pcistub/pci-back in future */
+ /*CAP_SYS_MODULE,*/
+
+ /* Apparently needed for /proc/bus/usb, which we need to
+ * give to QEMU */
+ CAP_SYS_RAWIO,
+
+ /* Shouldn't need to chroot */
+ /*CAP_SYS_CHROOT,*/
+
+ /* Definitely don't ptrace anything */
+ /*CAP_SYS_PTRACE,*/
+
+ /* Don't need to mess with accounting */
+ /*CAP_SYS_PACCT,*/
+
+ /* Another annoyingly hugely permissive cap we
+ * need to have to mount storage, SCSI, partitioning, etc
+ * XXX this bit needs to be configurable for sure */
+ CAP_SYS_ADMIN,
+
+ /* Don't want to reboot host ! */
+ /*CAP_SYS_BOOT,*/
+
+ /* Need this to set QEMU CPU affinity */
+ CAP_SYS_NICE,
+
+ /* Don't need to override quotas, etc */
+ /*CAP_SYS_RESOURCE,*/
+
+ /* Don't need to mess with system clock */
+ /*CAP_SYS_TIME,*/
+
+ /* XXX Does tty config include putting it in raw mode ?
+ * If not, kill this */
+ CAP_SYS_TTY_CONFIG,
+
+ /* Unfortunately needed for LXC setup */
+ CAP_MKNOD,
+
+ /* Don't need to take file leases (for now) */
+ /*CAP_LEASE,*/
+
+ /* Don't need to mess with auditing */
+ /*CAP_AUDIT_WRITE,*/
+ /*CAP_AUDIT_CONTROL,*/
+
+ /* XXX Undocumented */
+ /*CAP_SETFCAP,*/
+
+ /* Don't think we nede to override MAC */
+ /*CAP_MAC_OVERRIDE,*/
+ /*CAP_MAC_ADMIN,*/
+
+ -1 /* sentinal */) < 0) {
+ VIR_ERROR0("cannot set capability mask");
+ goto error;
+ }
+
+ /*
+ * If we change ID at this point, then any binaries we execve()
+ * *must* have any neccessary file capabilities set, eg
+ * /bin/mount must have CAP_SYS_ADMIN set on its file. Otherwise
+ * its effective & permitted sets will be initialized all zero.
+ *
+ * If we don't change ID, then we have to further filter the
+ * capabilities in virEec() for each individual command we
+ * run to the bare minimum it needs.
+ *
+ * The former is a nicer world,the latter is current reality
+ * since no distro sets any file capabilities.
+ *
+ * Since libvirtd (currently) needs to keep capabilities like
+ * DAC_OVERRIDE/SYS_ADMIN/NET_ADMIN, the overall security is
+ * not really changed either way.
+ */
+#if 1
+ if ((ret = capng_apply(CAPNG_SELECT_BOTH)) < 0) {
+ VIR_ERROR("cannot apply capabilities %d", ret);
+ return -1;
+ }
+#else
+ if ((ret = capng_change_id(uid, gid,
+ CAPNG_DROP_SUPP_GRP/* | CAPNG_CLEAR_BOUNDING*/)) < 0) {
+ VIR_ERROR("Cannot change id %d %d: %d", uid, gid, ret);
+ return -1;
+ }
+#endif
+
+ capng_get_caps_process();
+ qemudPrintCaps("After apply");
+
+
+ capng_get_caps_process();
+ qemudPrintCaps("After change id");
+ return 0;
+
+error:
+ return -1;
+}
+#else
#ifdef __sun
static int
-qemudSetupPrivs (void)
+qemudDropPrivileges (struct qemud_server *server ATTRIBUTE_UNUSED)
{
+ /* XXX it'd really be preferable to lookup this UID based
+ * on named user */
chown ("/var/run/libvirt", SYSTEM_UID, SYSTEM_UID);
if (__init_daemon_priv (PU_RESETGROUPS | PU_CLEARLIMITSET,
@@ -2715,7 +2930,11 @@ qemudSetupPrivs (void)
return 0;
}
#else
-#define qemudSetupPrivs() 0
+qemudDropPrivileges(struct qemud_server *server ATTRIBUTE_UNUSED)
+{
+ return 0;
+}
+#endif
#endif
/* Print command-line usage. */
@@ -2894,7 +3113,8 @@ int main(int argc, char **argv) {
sigaction(SIGINT, &sig_action, NULL);
sigaction(SIGQUIT, &sig_action, NULL);
sigaction(SIGTERM, &sig_action, NULL);
- sigaction(SIGCHLD, &sig_action, NULL);
+ /* Disabled because we don't rely on this anymore AFAICT */
+ /*sigaction(SIGCHLD, &sig_action, NULL);*/
sig_action.sa_handler = SIG_IGN;
sigaction(SIGPIPE, &sig_action, NULL);
@@ -2911,15 +3131,7 @@ int main(int argc, char **argv) {
}
}
- /* Beyond this point, nothing should rely on using
- * getuid/geteuid() == 0, for privilege level checks.
- * It must all use the flag 'server->privileged'
- * which is also passed into all libvirt stateful
- * drivers
- */
- if (qemudSetupPrivs() < 0)
- goto error2;
-
+ /* Basic state setup, must avoid most libvirt APIs before this point */
if (!(server = qemudInitialize(sigpipe[0]))) {
ret = 2;
goto error2;
@@ -2936,6 +3148,22 @@ int main(int argc, char **argv) {
unix_sock_dir);
}
+ /* Beyond this point, nothing should rely on using
+ * getuid/geteuid() == 0, for privilege level checks.
+ * It must all use the flag 'server->privileged'
+ * which is also passed into all libvirt stateful
+ * drivers
+ */
+ if (server->privileged &&
+ qemudDropPrivileges(server) < 0)
+ goto error2;
+
+
+ /* Load external driver modules, or register builtin modules */
+ qemudInitializeDrivers();
+
+
+ /* A event handler to process incoming signals */
if (virEventAddHandleImpl(sigpipe[0],
VIR_EVENT_HANDLE_READABLE,
qemudDispatchSignalEvent,
@@ -2945,6 +3173,8 @@ int main(int argc, char **argv) {
goto error2;
}
+ virStateInitialize(server->privileged);
+
if (!(server = qemudNetworkInit(server))) {
ret = 2;
goto error2;
diff -r 75253f120589 src/libvirt_private.syms
--- a/src/libvirt_private.syms Mon Jun 22 19:00:54 2009 +0100
+++ b/src/libvirt_private.syms Mon Jun 22 20:24:38 2009 +0100
@@ -348,6 +348,8 @@ virRun;
virSkipSpaces;
virKillProcess;
virGetUserDirectory;
+virGetUserID;
+virGetGroupID;
# uuid.h
diff -r 75253f120589 src/util.c
--- a/src/util.c Mon Jun 22 19:00:54 2009 +0100
+++ b/src/util.c Mon Jun 22 20:24:38 2009 +0100
@@ -55,6 +55,7 @@
#include <netdb.h>
#ifdef HAVE_GETPWUID_R
#include <pwd.h>
+#include <grp.h>
#endif
#include "virterror_internal.h"
@@ -1831,3 +1832,75 @@ char *virGetUserDirectory(virConnectPtr
return ret;
}
#endif
+
+int virGetUserID(virConnectPtr conn,
+ const char *name,
+ uid_t *uid)
+{
+ char *strbuf;
+ struct passwd pwbuf;
+ struct passwd *pw = NULL;
+ size_t strbuflen = sysconf(_SC_GETPW_R_SIZE_MAX);
+
+ if (VIR_ALLOC_N(strbuf, strbuflen) < 0) {
+ virReportOOMError(conn);
+ return -1;
+ }
+
+ /*
+ * From the manpage (terrifying but true):
+ *
+ * ERRORS
+ * 0 or ENOENT or ESRCH or EBADF or EPERM or ...
+ * The given name or uid was not found.
+ */
+ if (getpwnam_r(name, &pwbuf, strbuf, strbuflen, &pw) != 0 || pw == NULL) {
+ virReportSystemError(conn, errno,
+ _("Failed to find user record for name '%s'"),
+ name);
+ VIR_FREE(strbuf);
+ return -1;
+ }
+
+ *uid = pw->pw_uid;
+
+ VIR_FREE(strbuf);
+
+ return 0;
+}
+
+int virGetGroupID(virConnectPtr conn,
+ const char *name,
+ gid_t *gid)
+{
+ char *strbuf;
+ struct group grbuf;
+ struct group *gr = NULL;
+ size_t strbuflen = sysconf(_SC_GETGR_R_SIZE_MAX);
+
+ if (VIR_ALLOC_N(strbuf, strbuflen) < 0) {
+ virReportOOMError(conn);
+ return -1;
+ }
+
+ /*
+ * From the manpage (terrifying but true):
+ *
+ * ERRORS
+ * 0 or ENOENT or ESRCH or EBADF or EPERM or ...
+ * The given name or uid was not found.
+ */
+ if (getgrnam_r(name, &grbuf, strbuf, strbuflen, &gr) != 0 || gr == NULL) {
+ virReportSystemError(conn, errno,
+ _("Failed to find group record for name '%s'"),
+ name);
+ VIR_FREE(strbuf);
+ return -1;
+ }
+
+ *gid = gr->gr_gid;
+
+ VIR_FREE(strbuf);
+
+ return 0;
+}
diff -r 75253f120589 src/util.h
--- a/src/util.h Mon Jun 22 19:00:54 2009 +0100
+++ b/src/util.h Mon Jun 22 20:24:38 2009 +0100
@@ -218,6 +218,13 @@ char *virGetUserDirectory(virConnectPtr
uid_t uid);
#endif
+int virGetUserID(virConnectPtr conn,
+ const char *name,
+ uid_t *uid);
+int virGetGroupID(virConnectPtr conn,
+ const char *name,
+ gid_t *gid);
+
int virRandomInitialize(unsigned int seed);
int virRandom(int max);
--
|: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :|
|: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :|
|: http://autobuild.org -o- http://search.cpan.org/~danberr/ :|
|: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list