[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH 2/3] Set basic capabilities needed for libvirtd



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 :|


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]