[Libvir] [RFC PATCH] Solaris least privilege
In the interests of giving a 'heads-up' I'm posting this patch. It implements least-privilege on Solaris. The basic idea is that all libvirt clients are forced to go through libvirtd, which verifies a particular privilege. virtd itself runs with enough privilege to interact with Xen. This patch is: - not to be applied :) - only against 0.4.0 - subject to further change - not yet reviewed, not even by myself (properly) Nonetheless, comments are more than welcome. regards john --- libvirt-0.4.0/qemud/remote.c 2007-12-12 05:30:49.000000000 -0800 +++ libvirt-new/qemud/remote.c 2008-04-10 12:52:18.059618661 -0700 @@ -434,6 +434,15 @@ remoteDispatchOpen (struct qemud_server flags = args->flags; if (client->readonly) flags |= VIR_CONNECT_RO; +#ifdef __sun + /* + * On Solaris, all clients are forced to go via virtd. As a result, + * virtd must indicate it really does want to connect to the + * hypervisor. + */ + name = "xen:///"; +#endif + client->conn = flags & VIR_CONNECT_RO ? virConnectOpenReadOnly (name) --- libvirt-0.4.0/src/libvirt.c 2007-12-17 13:51:09.000000000 -0800 +++ libvirt-new/src/libvirt.c 2008-04-16 08:46:28.767087199 -0700 @@ -34,6 +34,7 @@ #include "uuid.h" #include "test.h" +#include "xen_internal.h" #include "xen_unified.h" #include "remote_internal.h" #include "qemu_driver.h" @@ -202,8 +203,16 @@ virInitialize(void) if (qemudRegister() == -1) return -1; #endif #ifdef WITH_XEN + /* + * On Solaris, only initialize Xen if we're libvirtd. + */ +#ifdef __sun + if (geteuid() != 0 && xenHavePrivilege() && + xenUnifiedRegister () == -1) return -1; +#else if (xenUnifiedRegister () == -1) return -1; #endif +#endif #ifdef WITH_OPENVZ if (openvzRegister() == -1) return -1; #endif @@ -525,6 +534,16 @@ do_open (const char *name, if (STREQ (name, "xen://")) name = "xen:///"; +#ifdef __sun + /* + * If we're not libvirtd, force us to go via the daemon. + */ + if (geteuid() == 0 || !xenHavePrivilege()) + name = "remote+unix:///"; + else if (STREQ (name, "xen:///") && xenUnifiedRegister () == -1) + return NULL; +#endif + if (!initialized) if (virInitialize() < 0) return NULL; --- libvirt-0.4.0/qemud/qemud.c 2007-12-12 05:30:49.000000000 -0800 +++ libvirt-new/qemud/qemud.c 2008-04-17 17:16:47.075258251 -0700 @@ -60,6 +60,25 @@ #include "mdns.h" #endif +#ifdef __sun +#include <ucred.h> +#include <priv.h> + +#ifndef PRIV_VIRT_MANAGE +#define PRIV_VIRT_MANAGE ((const char *)"virt_manage") +#endif + +#ifndef PRIV_XVM_CONTROL +#define PRIV_XVM_CONTROL ((const char *)"xvm_control") +#endif + +#define PU_RESETGROUPS 0x0001 /* Remove supplemental groups */ +#define PU_CLEARLIMITSET 0x0008 /* L=0 */ + +extern int __init_daemon_priv(int, uid_t, gid_t, ...); + +#endif + static int godaemon = 0; /* -d: Be a daemon */ static int verbose = 0; /* -v: Verbose mode */ static int timeout = -1; /* -t: Shutdown timeout */ @@ -668,11 +687,13 @@ static int qemudInitPaths(struct qemud_s unlink(sockname); +#ifndef __sun if (snprintf (roSockname, maxlen, "%s/run/libvirt/libvirt-sock-ro", LOCAL_STATE_DIR) >= maxlen) goto snprintf_error; unlink(roSockname); +#endif if (snprintf(server->logDir, PATH_MAX, "%s/log/libvirt/", LOCAL_STATE_DIR) >= PATH_MAX) goto snprintf_error; @@ -1033,6 +1054,29 @@ static int qemudDispatchServer(struct qe return -1; } +#ifdef __sun + { + ucred_t *ucred = NULL; + const priv_set_t *privs; + + if (getpeerucred (fd, &ucred) == -1 || + (privs = ucred_getprivset (ucred, PRIV_EFFECTIVE)) == NULL) { + if (ucred != NULL) + ucred_free (ucred); + close (fd); + return -1; + } + + if (!priv_ismember (privs, PRIV_VIRT_MANAGE)) { + ucred_free (ucred); + close (fd); + return -1; + } + + ucred_free (ucred); + } +#endif /* __sun */ + /* Disable Nagle. Unix sockets will ignore this. */ setsockopt (fd, IPPROTO_TCP, TCP_NODELAY, (void *)&no_slow_start, sizeof no_slow_start); @@ -1864,6 +1908,10 @@ remoteReadConfigFile (struct qemud_serve if (auth_unix_rw == REMOTE_AUTH_POLKIT) unix_sock_rw_mask = 0777; #endif +#ifdef __sun + unix_sock_rw_mask = 0666; +#endif + if (remoteConfigGetAuth(conf, "auth_unix_ro", &auth_unix_ro, filename) < 0) return -1; if (remoteConfigGetAuth(conf, "auth_tcp", &auth_tcp, filename) < 0) @@ -1955,6 +2003,32 @@ remoteReadConfigFile (struct qemud_serve return -1; } +#ifdef __sun +static void +qemudSetupPrivs (struct qemud_server *server) +{ + chown ("/var/run/libvirt", 60, 60); + chmod ("/var/run/libvirt", 0755); + chown ("/var/run/libvirt/libvirt-sock", 60, 60); + chmod ("/var/run/libvirt/libvirt-sock", 0666); + chown (server->logDir, 60, 60); + + if (__init_daemon_priv (PU_RESETGROUPS | PU_CLEARLIMITSET, + 60, 60, PRIV_XVM_CONTROL, NULL)) { + fprintf (stderr, "additional privileges are required\n"); + exit (1); + } + + if (priv_set (PRIV_OFF, PRIV_ALLSETS, PRIV_FILE_LINK_ANY, PRIV_PROC_INFO, + PRIV_PROC_SESSION, PRIV_PROC_EXEC, PRIV_PROC_FORK, NULL)) { + fprintf (stderr, "failed to set reduced privileges\n"); + exit (1); + } +} +#else +#define qemudSetupPrivs +#endif + /* Print command-line usage. */ static void usage (const char *argv0) @@ -2139,6 +2213,8 @@ int main(int argc, char **argv) { goto error2; } + qemudSetupPrivs(server); + qemudRunLoop(server); qemudCleanup(server); --- libvirt-0.4.0/src/xs_internal.c 2007-12-14 07:33:11.000000000 -0800 +++ libvirt-new/src/xs_internal.c 2008-04-10 09:54:38.676077954 -0700 @@ -31,7 +31,7 @@ #include "driver.h" #include "xen_unified.h" #include "xs_internal.h" -#include "xen_internal.h" /* for xenHypervisorCheckID */ +#include "xen_internal.h" #ifdef __linux__ #define XEN_HYPERVISOR_SOCKET "/proc/xen/privcmd" @@ -344,11 +344,11 @@ xenStoreOpen(virConnectPtr conn, if (priv->xshandle == NULL) { /* - * not being able to connect via the socket as a normal user - * is rather normal, this should fallback to the proxy (or - * remote) mechanism. + * not being able to connect via the socket as an unprivileged + * user is rather normal, this should fallback to the proxy (or + * remote) mechanism. */ - if (getuid() == 0) { + if (!xenHavePrivilege()) { virXenStoreError(NULL, VIR_ERR_NO_XEN, _("failed to connect to Xen Store")); } --- libvirt-0.4.0/src/xen_internal.c 2007-12-12 05:30:49.000000000 -0800 +++ libvirt-new/src/xen_internal.c 2008-04-10 10:28:41.363704244 -0700 @@ -28,6 +28,15 @@ #include <errno.h> #include <sys/utsname.h> +#ifdef __sun +#include <priv.h> + +#ifndef PRIV_XVM_CONTROL +#define PRIV_XVM_CONTROL ((const char *)"xvm_control") +#endif + +#endif + #include "xs_internal.h" #include "stats_linux.h" #include "xend_internal.h" @@ -3259,6 +3268,21 @@ xenHypervisorGetVcpuMax(virDomainPtr dom return maxcpu; } +/** + * xenHavePrivilege() + * + * Return true if the current process should be able to connect to Xen. + */ +int +xenHavePrivilege() +{ +#ifdef __sun + return priv_ineffect (PRIV_XVM_CONTROL); +#else + return getuid () == 0; +#endif +} + #endif /* WITH_XEN */ /* * vim: set tabstop=4: --- libvirt-0.4.0/src/xen_internal.h 2007-12-05 12:33:02.000000000 -0800 +++ libvirt-new/src/xen_internal.h 2008-04-10 09:34:16.379186656 -0700 @@ -98,6 +98,9 @@ int xenHypervisorNodeGetCellsFreeMemory( unsigned long long *freeMems, int startCell, int maxCells); + +int xenHavePrivilege(void); + #ifdef __cplusplus } #endif --- libvirt-0.4.0/src/xen_unified.c 2007-12-12 05:30:49.000000000 -0800 +++ libvirt-new/src/xen_unified.c 2008-04-10 15:10:53.653298774 -0700 @@ -266,8 +266,8 @@ xenUnifiedOpen (virConnectPtr conn, xmlU priv->xendConfigVersion > 2) continue; - /* Ignore proxy for root */ - if (i == XEN_UNIFIED_PROXY_OFFSET && getuid() == 0) + /* Ignore proxy if we have privilege */ + if (i == XEN_UNIFIED_PROXY_OFFSET && xenHavePrivilege()) continue; if (drivers[i]->open) { @@ -282,10 +282,10 @@ xenUnifiedOpen (virConnectPtr conn, xmlU #endif } - /* If as root, then all drivers must succeed. - If non-root, then only proxy must succeed */ + /* If privileged, then all drivers must succeed. + If unprivileged, then only proxy must succeed */ if (!priv->opened[i] && - (getuid() == 0 || i == XEN_UNIFIED_PROXY_OFFSET)) { + (xenHavePrivilege() || i == XEN_UNIFIED_PROXY_OFFSET)) { for (j = 0; j < i; ++j) if (priv->opened[j]) drivers[j]->close (conn); free (priv); @@ -1266,6 +1266,12 @@ static virDriver xenUnifiedDriver = { int xenUnifiedRegister (void) { + static int driver_priority = 0; + static int xen_initialized = 0; + + if (xen_initialized) + return driver_priority; + /* Ignore failures here. */ (void) xenHypervisorInit (); (void) xenProxyInit (); @@ -1273,7 +1279,9 @@ xenUnifiedRegister (void) (void) xenStoreInit (); (void) xenXMInit (); - return virRegisterDriver (&xenUnifiedDriver); + driver_priority = virRegisterDriver (&xenUnifiedDriver); + xen_initialized = 1; + return driver_priority; } #endif /* WITH_XEN */ --- libvirt-0.4.0/src/xend_internal.c 2007-12-17 15:05:27.000000000 -0800 +++ libvirt-new/src/xend_internal.c 2008-04-10 09:45:10.262989682 -0700 @@ -42,7 +42,7 @@ #include "uuid.h" #include "xen_unified.h" #include "xend_internal.h" -#include "xen_internal.h" /* for DOM0_INTERFACE_VERSION */ +#include "xen_internal.h" #include "xs_internal.h" /* To extract VNC port & Serial console TTY */ /* required for cpumap_t */ @@ -235,7 +235,7 @@ do_connect(virConnectPtr xend) * is rather normal, this should fallback to the proxy (or * remote) mechanism. */ - if ((getuid() == 0) || (xend->flags & VIR_CONNECT_RO)) { + if (xenHavePrivilege() || (xend->flags & VIR_CONNECT_RO)) { virXendError(xend, VIR_ERR_INTERNAL_ERROR, "failed to connect to xend"); } --- libvirt-0.4.0/src/virsh.c 2007-12-07 07:00:48.000000000 -0800 +++ libvirt-new/src/virsh.c 2008-04-16 08:44:35.668718168 -0700 @@ -36,6 +36,7 @@ #include <sys/stat.h> #include <inttypes.h> #include <test.h> +#include <signal.h> #include <libxml/parser.h> #include <libxml/tree.h> @@ -50,6 +51,7 @@ #include "console.h" static char *progname; +static int sigpipe; #ifndef TRUE #define TRUE 1 @@ -202,9 +204,6 @@ typedef struct __vshControl { virConnectPtr conn; /* connection to hypervisor (MAY BE NULL) */ vshCmd *cmd; /* the current command */ char *cmdstr; /* string with command */ -#ifndef __MINGW32__ - uid_t uid; /* process owner */ -#endif /* __MINGW32__ */ int imode; /* interactive mode? */ int quiet; /* quiet mode */ int debug; /* print debug messages? */ @@ -464,12 +463,8 @@ static vshCmdOptDef opts_console[] = { static int cmdConsole(vshControl * ctl, vshCmd * cmd) { - xmlDocPtr xml = NULL; - xmlXPathObjectPtr obj = NULL; - xmlXPathContextPtr ctxt = NULL; virDomainPtr dom; int ret = FALSE; - char *doc; if (!vshConnectionUsability(ctl, ctl->conn, TRUE)) return FALSE; @@ -477,37 +472,16 @@ cmdConsole(vshControl * ctl, vshCmd * cm if (!(dom = vshCommandOptDomain(ctl, cmd, "domain", NULL))) return FALSE; - doc = virDomainGetXMLDesc(dom, 0); - if (!doc) - goto cleanup; - - xml = xmlReadDoc((const xmlChar *) doc, "domain.xml", NULL, - XML_PARSE_NOENT | XML_PARSE_NONET | - XML_PARSE_NOWARNING); - free(doc); - if (!xml) - goto cleanup; - ctxt = xmlXPathNewContext(xml); - if (!ctxt) - goto cleanup; + ret = vshConsole(dom); - obj = xmlXPathEval(BAD_CAST "string(/domain/devices/console/@tty)", ctxt); - if ((obj != NULL) && ((obj->type == XPATH_STRING) && - (obj->stringval != NULL) && (obj->stringval[0] != 0))) { - if (vshRunConsole((const char *)obj->stringval) == 0) - ret = TRUE; - } else { + if (ret == 0) { vshPrintExtra(ctl, _("No console available for domain\n")); + } else if (ret == -1) { + fprintf(stderr, _("unable to open console: %s\n"), + strerror(errno)); } - xmlXPathFreeObject(obj); - cleanup: - if (ctxt) - xmlXPathFreeContext(ctxt); - if (xml) - xmlFreeDoc(xml); - virDomainFree(dom); - return ret; + return ret == 1; } #else /* __MINGW32__ */ @@ -4523,22 +4497,11 @@ vshInit(vshControl * ctl) if (ctl->conn) return FALSE; -#ifndef __MINGW32__ - ctl->uid = getuid(); -#endif - vshOpenLogFile(ctl); /* set up the library error handler */ virSetErrorFunc(NULL, virshErrorHandler); -#ifndef __MINGW32__ - /* Force a non-root, Xen connection to readonly */ - if ((ctl->name == NULL || - !strcasecmp(ctl->name, "xen")) && ctl->uid != 0) - ctl->readonly = 1; -#endif - ctl->conn = virConnectOpenAuth(ctl->name, virConnectAuthPtrDefault, ctl->readonly ? VIR_CONNECT_RO : 0); @@ -5021,12 +4984,21 @@ vshParseArgv(vshControl * ctl, int argc, return TRUE; } -int +static void sigpipe_handler(int sig) +{ + sigpipe = 1; + /* + * Force readline() to exit. + */ + close(STDIN_FILENO); +} + main(int argc, char **argv) { vshControl _ctl, *ctl = &_ctl; char *defaultConn; int ret = TRUE; + struct sigaction sig_action; if (!setlocale(LC_ALL, "")) { perror("setlocale"); @@ -5059,6 +5031,12 @@ main(int argc, char **argv) exit(EXIT_FAILURE); } + sig_action.sa_handler = sigpipe_handler; + sig_action.sa_flags = 0; + sigemptyset(&sig_action.sa_mask); + + sigaction(SIGPIPE, &sig_action, NULL); + if (!vshInit(ctl)) { vshDeinit(ctl); exit(EXIT_FAILURE); @@ -5098,6 +5076,13 @@ main(int argc, char **argv) fputc('\n', stdout); /* line break after alone prompt */ } + /* + * If the connection over a socket failed abruptly, it's probably + * due to not having the right privileges. + */ + if (sigpipe) + vshError(ctl, TRUE, _("failed to connect (insufficient privileges?)")); + vshDeinit(ctl); exit(ret ? EXIT_SUCCESS : EXIT_FAILURE); } --- libvirt-0.4.0/src/console.c 2007-12-07 07:00:48.000000000 -0800 +++ libvirt-new/src/console.c 2008-04-16 16:34:15.533322609 -0700 @@ -34,6 +34,11 @@ #include <errno.h> #include <unistd.h> #include <signal.h> +#include <stdlib.h> + +#include <libxml/parser.h> +#include <libxml/tree.h> +#include <libxml/xpath.h> #include "console.h" #include "internal.h" @@ -59,8 +64,10 @@ cfmakeraw (struct termios *attr) } #endif /* !HAVE_CFMAKERAW */ -int vshRunConsole(const char *tty) { - int ttyfd, ret = -1; +static int +vshDirectConsole(int ttyfd) +{ + int ret = -1; struct termios ttyattr, rawattr; void (*old_sigquit)(int); void (*old_sigterm)(int); @@ -68,14 +75,6 @@ int vshRunConsole(const char *tty) { void (*old_sighup)(int); void (*old_sigpipe)(int); - - /* We do not want this to become the controlling TTY */ - if ((ttyfd = open(tty, O_NOCTTY | O_RDWR)) < 0) { - fprintf(stderr, _("unable to open tty %s: %s\n"), - tty, strerror(errno)); - return -1; - } - /* Put STDIN into raw mode so that stuff typed does not echo to the screen (the TTY reads will result in it being echoed back already), and @@ -174,7 +173,7 @@ int vshRunConsole(const char *tty) { } } done: - ret = 0; + ret = 1; cleanup: @@ -195,6 +194,106 @@ int vshRunConsole(const char *tty) { return ret; } +/* + * Attempt to access the domain via 'xenconsole', which may have + * additional privilege to reach the console. + */ +static int +vshXenConsole(int id) +{ + char arg[100]; + char *argv[3]; + pid_t pid; + + if ((pid = fork()) < 0) { + return -1; + } else if (pid) { + int wstat; + + if (waitpid(pid, &wstat, 0) < 0) + return -1; + + if (WIFSIGNALED(wstat)) + return -1; + if (WIFEXITED(wstat) && WEXITSTATUS(wstat)) + return -1; + + return 1; + } + + /* child */ + + if (snprintf(arg, 100, "%d", id) < 0) + return -1; + + argv[0] = "/usr/lib/xen/bin/xenconsole"; + argv[1] = arg; + argv[2] = NULL; + + if (execv("/usr/lib/xen/bin/xenconsole", argv)) + fprintf(stderr, _("failed to run xenconsole: %s\n"), + strerror(errno)); + _exit(1); +} + +/* + * Returns 0 if no console is available, 1 if the console was accessed + * successfully, or -1 on error. + */ +int vshConsole(virDomainPtr dom) +{ + xmlDocPtr xml = NULL; + xmlXPathObjectPtr obj = NULL; + xmlXPathContextPtr ctxt = NULL; + char *doc; + int ttyfd; + int ret = -1; + + doc = virDomainGetXMLDesc(dom, 0); + if (!doc) + goto cleanup; + + xml = xmlReadDoc((const xmlChar *) doc, "domain.xml", NULL, + XML_PARSE_NOENT | XML_PARSE_NONET | + XML_PARSE_NOWARNING); + free(doc); + if (!xml) + goto cleanup; + ctxt = xmlXPathNewContext(xml); + if (!ctxt) + goto cleanup; + + obj = xmlXPathEval(BAD_CAST "string(/domain/devices/console/@tty)", ctxt); + if (obj == NULL || obj->type != XPATH_STRING || obj->stringval == NULL || + obj->stringval[0] == '\0') { + ret = 0; + goto cleanup; + } + + /* We do not want this to become the controlling TTY */ + ttyfd = open((char *)obj->stringval, O_NOCTTY | O_RDWR); + + if (ttyfd != -1) { + ret = vshDirectConsole(ttyfd); + goto cleanup; + } else if (errno != EACCES) { + fprintf(stderr, _("unable to open tty %s: %s\n"), + obj->stringval, strerror(errno)); + } else { + ret = vshXenConsole(virDomainGetID(dom)); + } + + cleanup: + if (obj) + xmlXPathFreeObject(obj); + if (ctxt) + xmlXPathFreeContext(ctxt); + if (xml) + xmlFreeDoc(xml); + virDomainFree(dom); + return ret; +} + #endif /* !__MINGW32__ */ /* --- libvirt-0.4.0/src/console.h 2007-12-07 07:00:48.000000000 -0800 +++ libvirt-new/src/console.h 2008-04-16 08:49:20.165071296 -0700 @@ -25,11 +25,13 @@ #ifndef __MINGW32__ +#include "libvirt/libvirt.h" + #ifdef __cplusplus extern "C" { #endif - int vshRunConsole(const char *tty); +int vshConsole(virDomainPtr dom); #ifdef __cplusplus }
On Thu, Apr 24, 2008 at 01:48:08PM +0100, John Levon wrote:
In the interests of giving a 'heads-up' I'm posting this patch. It implements least-privilege on Solaris. The basic idea is that all libvirt clients are forced to go through libvirtd, which verifies a particular privilege. virtd itself runs with enough privilege to interact with Xen.
This patch is:
- not to be applied :) - only against 0.4.0 - subject to further change - not yet reviewed, not even by myself (properly)
Nonetheless, comments are more than welcome.
Hi John, in general the idea of removing all those geteid() == 0 and replacing them like xenHavePrivilege() is a good one. The patch includes stuff which is not strictly related like the virsh console cleanup which should be separated. Also it seems you use some socket auth extensions to detect the uid of the other process, we do that already in qemud/qemud.c see function qemudGetSocketIdentity() , maybe we should abstract that in the util.c module and provide the _sun version there. I didn't fully understood some of the checks on the socket paths but that was separated under #ifdef _sun so that looks system specific. in a nutshell, good idea but let's try to make this as generic as possible :-) Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Thu, Apr 24, 2008 at 09:54:19AM -0400, Daniel Veillard wrote:
in general the idea of removing all those geteid() == 0 and replacing them like xenHavePrivilege() is a good one. The patch includes stuff which is not strictly related like the virsh console cleanup which should be separated.
Sure, at merge time everything will be split up appropriately. BTW, it is related very much: only xenconsole has privilege to connect to Xen consoles.
Also it seems you use some socket auth extensions to detect the uid of the other process, we do that already in qemud/qemud.c see function qemudGetSocketIdentity() , maybe we should abstract that in the util.c module and provide the _sun version there.
It's not about UID but privilege. The Identity stuff is only used under HAVE_POLKIT, so I'm not sure there's much commonality that can be abstracted. Can you describe further what you would expect it to look like? regards john
On Thu, Apr 24, 2008 at 03:04:56PM +0100, John Levon wrote:
On Thu, Apr 24, 2008 at 09:54:19AM -0400, Daniel Veillard wrote:
Also it seems you use some socket auth extensions to detect the uid of the other process, we do that already in qemud/qemud.c see function qemudGetSocketIdentity() , maybe we should abstract that in the util.c module and provide the _sun version there.
It's not about UID but privilege. The Identity stuff is only used under HAVE_POLKIT, so I'm not sure there's much commonality that can be abstracted. Can you describe further what you would expect it to look like?
okay then I misunderstood that part of the patch, Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Thu, Apr 24, 2008 at 03:04:56PM +0100, John Levon wrote:
On Thu, Apr 24, 2008 at 09:54:19AM -0400, Daniel Veillard wrote:
in general the idea of removing all those geteid() == 0 and replacing them like xenHavePrivilege() is a good one. The patch includes stuff which is not strictly related like the virsh console cleanup which should be separated.
Sure, at merge time everything will be split up appropriately. BTW, it is related very much: only xenconsole has privilege to connect to Xen consoles.
In that case we should definitel split the 'virsh console' impl out into a separate binary, so we can use the non-Xen specific codebase and stil maintain your privilege separation.
Also it seems you use some socket auth extensions to detect the uid of the other process, we do that already in qemud/qemud.c see function qemudGetSocketIdentity() , maybe we should abstract that in the util.c module and provide the _sun version there.
It's not about UID but privilege. The Identity stuff is only used under HAVE_POLKIT, so I'm not sure there's much commonality that can be abstracted. Can you describe further what you would expect it to look like?
Although we don't use the qemudGetSocketIdentity() anyway other than under the POLKIT code, this may change in the future, so it'd just be convenient to have a Solaris impl there. We can change the #if HAVE_POLKIT to be #ifdef HAVE_POLKIT || __sun, so the method is available to the privilege checking code too. Dan -- |: Red Hat, Engineering, Boston -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 :|
On Thu, Apr 24, 2008 at 01:48:08PM +0100, John Levon wrote:
In the interests of giving a 'heads-up' I'm posting this patch. It implements least-privilege on Solaris. The basic idea is that all libvirt clients are forced to go through libvirtd, which verifies a particular privilege. virtd itself runs with enough privilege to interact with Xen.
This is interesting work. Work around privilege levels / security is probably one of the most interesting & important aspects for future libvirt development. On Linux obviously this likely involves SELinux and its interesting to see that Solaris is also working on a MAC based on the FLASK model. Any work we can do in the realm of privilege separation using existing UNIX apis is also useful.
--- libvirt-0.4.0/qemud/remote.c 2007-12-12 05:30:49.000000000 -0800 +++ libvirt-new/qemud/remote.c 2008-04-10 12:52:18.059618661 -0700 @@ -434,6 +434,15 @@ remoteDispatchOpen (struct qemud_server flags = args->flags; if (client->readonly) flags |= VIR_CONNECT_RO;
+#ifdef __sun + /* + * On Solaris, all clients are forced to go via virtd. As a result, + * virtd must indicate it really does want to connect to the + * hypervisor. + */ + name = "xen:///"; +#endif +
I'm pretty sure we can come up with a way todo this without having to have the #ifdef __sun here. AFAICT, you're trying to stop the remote driver in the daemon re-entrantly calling back into the daemon. If so, then we can make this more generic, by just having a global flag to disable the remote driver entirely when inside the daemon.
--- libvirt-0.4.0/src/libvirt.c 2007-12-17 13:51:09.000000000 -0800 +++ libvirt-new/src/libvirt.c 2008-04-16 08:46:28.767087199 -0700 @@ -34,6 +34,7 @@
#include "uuid.h" #include "test.h" +#include "xen_internal.h" #include "xen_unified.h" #include "remote_internal.h" #include "qemu_driver.h" @@ -202,8 +203,16 @@ virInitialize(void) if (qemudRegister() == -1) return -1; #endif #ifdef WITH_XEN + /* + * On Solaris, only initialize Xen if we're libvirtd. + */ +#ifdef __sun + if (geteuid() != 0 && xenHavePrivilege() && + xenUnifiedRegister () == -1) return -1; +#else if (xenUnifiedRegister () == -1) return -1; #endif +#endif
As Daniel suggests, we should write some kind of generic helper function in the util.c file, so we can isolate the #ifdef __sun in a single place rather than repeating it.
--- libvirt-0.4.0/qemud/qemud.c 2007-12-12 05:30:49.000000000 -0800 +++ libvirt-new/qemud/qemud.c 2008-04-17 17:16:47.075258251 -0700 @@ -60,6 +60,25 @@ #include "mdns.h" #endif
+#ifdef __sun +#include <ucred.h> +#include <priv.h> + +#ifndef PRIV_VIRT_MANAGE +#define PRIV_VIRT_MANAGE ((const char *)"virt_manage") +#endif + +#ifndef PRIV_XVM_CONTROL +#define PRIV_XVM_CONTROL ((const char *)"xvm_control") +#endif + +#define PU_RESETGROUPS 0x0001 /* Remove supplemental groups */ +#define PU_CLEARLIMITSET 0x0008 /* L=0 */ + +extern int __init_daemon_priv(int, uid_t, gid_t, ...); + +#endif + static int godaemon = 0; /* -d: Be a daemon */ static int verbose = 0; /* -v: Verbose mode */ static int timeout = -1; /* -t: Shutdown timeout */ @@ -668,11 +687,13 @@ static int qemudInitPaths(struct qemud_s
unlink(sockname);
+#ifndef __sun if (snprintf (roSockname, maxlen, "%s/run/libvirt/libvirt-sock-ro", LOCAL_STATE_DIR) >= maxlen) goto snprintf_error;
unlink(roSockname); +#endif
if (snprintf(server->logDir, PATH_MAX, "%s/log/libvirt/", LOCAL_STATE_DIR) >= PATH_MAX) goto snprintf_error; @@ -1033,6 +1054,29 @@ static int qemudDispatchServer(struct qe return -1; }
+#ifdef __sun + { + ucred_t *ucred = NULL; + const priv_set_t *privs; + + if (getpeerucred (fd, &ucred) == -1 || + (privs = ucred_getprivset (ucred, PRIV_EFFECTIVE)) == NULL) { + if (ucred != NULL) + ucred_free (ucred); + close (fd); + return -1; + } + + if (!priv_ismember (privs, PRIV_VIRT_MANAGE)) { + ucred_free (ucred); + close (fd); + return -1; + } + + ucred_free (ucred); + } +#endif /* __sun */ +
After the 0.4.0 release I refactored the damon code to have a generic function for getting socket peer credentials. So we can move this code into the qemud.c file in the qemudGetSocketIdentity() method. Its good to have a Solaris impl for this API.
/* Disable Nagle. Unix sockets will ignore this. */ setsockopt (fd, IPPROTO_TCP, TCP_NODELAY, (void *)&no_slow_start, sizeof no_slow_start); @@ -1864,6 +1908,10 @@ remoteReadConfigFile (struct qemud_serve if (auth_unix_rw == REMOTE_AUTH_POLKIT) unix_sock_rw_mask = 0777; #endif +#ifdef __sun + unix_sock_rw_mask = 0666; +#endif +
We can probably use 0666 even on Linux - there's no compelling reason why we need to have 0777 with execute permission - read & write should be sufficient I believe.
+#ifdef __sun +static void +qemudSetupPrivs (struct qemud_server *server) +{ + chown ("/var/run/libvirt", 60, 60); + chmod ("/var/run/libvirt", 0755); + chown ("/var/run/libvirt/libvirt-sock", 60, 60); + chmod ("/var/run/libvirt/libvirt-sock", 0666); + chown (server->logDir, 60, 60);
The thing that concerns me here, is that we're getting a bit of a disconnect between the config file and the impl. The user can already set these things via the cofnig file, so hardcoding specific values is not so pleasant. I'm wondering if its practical to have this privilege separation stuff be toggled via a config file setting and avoid hardcoding this soo much.
+/** + * xenHavePrivilege() + * + * Return true if the current process should be able to connect to Xen. + */ +int +xenHavePrivilege() +{ +#ifdef __sun + return priv_ineffect (PRIV_XVM_CONTROL); +#else + return getuid () == 0; +#endif +} +
As mentioned earlier, we probably want to move this into the util.c file and have the privilege name passed in as a parameter.
+/* + * Attempt to access the domain via 'xenconsole', which may have + * additional privilege to reach the console. + */ +static int +vshXenConsole(int id)
+{ + char arg[100]; + char *argv[3]; + pid_t pid; + + if ((pid = fork()) < 0) { + return -1; + } else if (pid) { + int wstat; + + if (waitpid(pid, &wstat, 0) < 0) + return -1; + + if (WIFSIGNALED(wstat)) + return -1; + if (WIFEXITED(wstat) && WEXITSTATUS(wstat)) + return -1; + + return 1; + } + + /* child */ + + if (snprintf(arg, 100, "%d", id) < 0) + return -1; + + argv[0] = "/usr/lib/xen/bin/xenconsole"; + argv[1] = arg; + argv[2] = NULL; + + if (execv("/usr/lib/xen/bin/xenconsole", argv))
Clearly this has to die. The virsh console command implements exactly the same functionality as xenconsole without being xen specific. I'm guessing you're doing this because you need the console in a separate process from virsh, so it can run with different privileges. If so, we should probably split 'virsh console' out into a separate standlone binary/command 'virt-console'. This will let you get the privilege separation without relying on Xen specific commands. So in summary, this is interesting work. I've no objections to the general principle of what the patch is trying to achieve - we'll just need to iterate over it a few times to try and better isolate the solaris specific #ifdefs and/or make them more generically applicable and avoid the xen specific console bits. It'd be useful for other people's understanding if there was a short doc giving the 1000ft overview of the different components and their relative privileges Regards, Dan. -- |: Red Hat, Engineering, Boston -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 :|
On Thu, Apr 24, 2008 at 03:25:05PM +0100, Daniel P. Berrange wrote:
--- libvirt-0.4.0/qemud/remote.c 2007-12-12 05:30:49.000000000 -0800 +++ libvirt-new/qemud/remote.c 2008-04-10 12:52:18.059618661 -0700 @@ -434,6 +434,15 @@ remoteDispatchOpen (struct qemud_server flags = args->flags; if (client->readonly) flags |= VIR_CONNECT_RO;
+#ifdef __sun + /* + * On Solaris, all clients are forced to go via virtd. As a result, + * virtd must indicate it really does want to connect to the + * hypervisor. + */ + name = "xen:///"; +#endif +
I'm pretty sure we can come up with a way todo this without having to have the #ifdef __sun here. AFAICT, you're trying to stop the remote driver in the daemon re-entrantly calling back into the daemon. If so, then we can make this more generic, by just having a global flag to disable the remote driver entirely when inside the daemon.
That's right. Such a flag sounds sensible to me, I wasn't sure over the rules of what the daemon can actually use (i.e. private interfaces to libvirt core).
--- libvirt-0.4.0/src/libvirt.c 2007-12-17 13:51:09.000000000 -0800 +++ libvirt-new/src/libvirt.c 2008-04-16 08:46:28.767087199 -0700 @@ -34,6 +34,7 @@
#include "uuid.h" #include "test.h" +#include "xen_internal.h" #include "xen_unified.h" #include "remote_internal.h" #include "qemu_driver.h" @@ -202,8 +203,16 @@ virInitialize(void) if (qemudRegister() == -1) return -1; #endif #ifdef WITH_XEN + /* + * On Solaris, only initialize Xen if we're libvirtd. + */ +#ifdef __sun + if (geteuid() != 0 && xenHavePrivilege() && + xenUnifiedRegister () == -1) return -1; +#else if (xenUnifiedRegister () == -1) return -1; #endif +#endif
As Daniel suggests, we should write some kind of generic helper function in the util.c file, so we can isolate the #ifdef __sun in a single place rather than repeating it.
I can do that, sure.
After the 0.4.0 release I refactored the damon code to have a generic function for getting socket peer credentials. So we can move this code into the qemud.c file in the qemudGetSocketIdentity() method. Its good to have a Solaris impl for this API.
But this only returns the UID right? Or are you saying there's a generic function for "can connect"? If so, we could certainly use that. I don't see how we could use qemudGetSocketIdentity() (though we could certainly *implement* it).
/* Disable Nagle. Unix sockets will ignore this. */ setsockopt (fd, IPPROTO_TCP, TCP_NODELAY, (void *)&no_slow_start, sizeof no_slow_start); @@ -1864,6 +1908,10 @@ remoteReadConfigFile (struct qemud_serve if (auth_unix_rw == REMOTE_AUTH_POLKIT) unix_sock_rw_mask = 0777; #endif +#ifdef __sun + unix_sock_rw_mask = 0666; +#endif +
We can probably use 0666 even on Linux - there's no compelling reason why we need to have 0777 with execute permission - read & write should be sufficient I believe.
This hunk is enforcing 0666 even without Polkit on Solaris. So I can fix the 0777 too, but we do need this hunk.
+#ifdef __sun +static void +qemudSetupPrivs (struct qemud_server *server) +{ + chown ("/var/run/libvirt", 60, 60); + chmod ("/var/run/libvirt", 0755); + chown ("/var/run/libvirt/libvirt-sock", 60, 60); + chmod ("/var/run/libvirt/libvirt-sock", 0666); + chown (server->logDir, 60, 60);
The thing that concerns me here, is that we're getting a bit of a disconnect between the config file and the impl. The user can already set these things via the cofnig file, so hardcoding specific values is not so pleasant. I'm wondering if its practical to have this privilege separation stuff be toggled via a config file setting and avoid hardcoding this soo much.
We don't even ship the config file, as there's nothing in it that we want to make configurable on Solaris. We don't have the same issue that Linux does, where it needs to define a single set of sources that applies to whole bunch of disparate environments. We just have the Solaris ecosystem, and it's always configured and locked down in the same way. When there's genuine configuration values that users need for something enabled on Solaris, it will be in SMF anyway, so will need re-working on the config backend in libvirt.
+/** + * xenHavePrivilege() + * + * Return true if the current process should be able to connect to Xen. + */ +int +xenHavePrivilege() +{ +#ifdef __sun + return priv_ineffect (PRIV_XVM_CONTROL); +#else + return getuid () == 0; +#endif +}
As mentioned earlier, we probably want to move this into the util.c file and have the privilege name passed in as a parameter.
Could you explain further how you see this working?
+/* + * Attempt to access the domain via 'xenconsole', which may have + * additional privilege to reach the console. + */ +static int +vshXenConsole(int id)
Clearly this has to die.
It's not at all clear to me. virsh console is already a wart, and I don't see the problem in a pragmatic approach here, given it only ever tries the direct approach as a fallback.
The virsh console command implements exactly the same functionality as xenconsole without being xen specific.
I'm guessing you're doing this because you need the console in a separate process from virsh, so it can run with different privileges.
Yes. In particular it needs to run setuid root and: - get the pts from xenstore (needs PRIV_XVM_CONTROL) - open the pty (requires all privileges) - drop all privileges before continuing
If so, we should probably split 'virsh console' out into a separate standlone binary/command 'virt-console'. This will let you get the privilege separation without relying on Xen specific commands.
This just adds a whole bunch of extra code to no clear advantage?
So in summary, this is interesting work. I've no objections to the general principle of what the patch is trying to achieve - we'll just need to iterate over it a few times to try and better isolate the solaris specific
Sure. Just as a warning, it will probably be quite some time before I'm able to find time to get this updated and in a mergable state.
It'd be useful for other people's understanding if there was a short doc giving the 1000ft overview of the different components and their relative privileges
I have a detailed document ready for architecture review. When it comes up on the ARC (http://www.opensolaris.org/os/community/arc/) I'll forward it on. regards, john
On Thu, Apr 24, 2008 at 03:42:35PM +0100, John Levon wrote:
On Thu, Apr 24, 2008 at 03:25:05PM +0100, Daniel P. Berrange wrote:
--- libvirt-0.4.0/qemud/remote.c 2007-12-12 05:30:49.000000000 -0800 +++ libvirt-new/qemud/remote.c 2008-04-10 12:52:18.059618661 -0700 @@ -434,6 +434,15 @@ remoteDispatchOpen (struct qemud_server flags = args->flags; if (client->readonly) flags |= VIR_CONNECT_RO;
+#ifdef __sun + /* + * On Solaris, all clients are forced to go via virtd. As a result, + * virtd must indicate it really does want to connect to the + * hypervisor. + */ + name = "xen:///"; +#endif +
I'm pretty sure we can come up with a way todo this without having to have the #ifdef __sun here. AFAICT, you're trying to stop the remote driver in the daemon re-entrantly calling back into the daemon. If so, then we can make this more generic, by just having a global flag to disable the remote driver entirely when inside the daemon.
That's right. Such a flag sounds sensible to me, I wasn't sure over the rules of what the daemon can actually use (i.e. private interfaces to libvirt core).
It is fuzzy & full of trickery :-) If you need to detect whether you are inside teh daemon or not, then you need to register a state driver with the virRegisterStateDriver(). The 'init' method in that will be run when inside the daemon thus enabling you to toggle the flag.
After the 0.4.0 release I refactored the damon code to have a generic function for getting socket peer credentials. So we can move this code into the qemud.c file in the qemudGetSocketIdentity() method. Its good to have a Solaris impl for this API.
But this only returns the UID right? Or are you saying there's a generic function for "can connect"? If so, we could certainly use that. I don't see how we could use qemudGetSocketIdentity() (though we could certainly *implement* it).
I'd just like to see if have an impl for Solaris - whether you happen to use it or not, is separate issue. Some people may be taking libvirt directly & building/deploying it themselves on Solaris, outside the context of your confined model, so it may turn out to be useful.
We can probably use 0666 even on Linux - there's no compelling reason why we need to have 0777 with execute permission - read & write should be sufficient I believe.
This hunk is enforcing 0666 even without Polkit on Solaris. So I can fix the 0777 too, but we do need this hunk.
Ah yes, ignore me here
+#ifdef __sun +static void +qemudSetupPrivs (struct qemud_server *server) +{ + chown ("/var/run/libvirt", 60, 60); + chmod ("/var/run/libvirt", 0755); + chown ("/var/run/libvirt/libvirt-sock", 60, 60); + chmod ("/var/run/libvirt/libvirt-sock", 0666); + chown (server->logDir, 60, 60);
The thing that concerns me here, is that we're getting a bit of a disconnect between the config file and the impl. The user can already set these things via the cofnig file, so hardcoding specific values is not so pleasant. I'm wondering if its practical to have this privilege separation stuff be toggled via a config file setting and avoid hardcoding this soo much.
We don't even ship the config file, as there's nothing in it that we want to make configurable on Solaris. We don't have the same issue that Linux does, where it needs to define a single set of sources that applies to whole bunch of disparate environments. We just have the Solaris ecosystem, and it's always configured and locked down in the same way.
When there's genuine configuration values that users need for something enabled on Solaris, it will be in SMF anyway, so will need re-working on the config backend in libvirt.
So you're not making use of any of the remote management features like TLS and SASL/Kerberos ? I'd think at least users need the config to switch between which auth scheme they'd like to use, because the choice of TLS vs Kerberos is really a deployment question for admins.
+/** + * xenHavePrivilege() + * + * Return true if the current process should be able to connect to Xen. + */ +int +xenHavePrivilege() +{ +#ifdef __sun + return priv_ineffect (PRIV_XVM_CONTROL); +#else + return getuid () == 0; +#endif +}
As mentioned earlier, we probably want to move this into the util.c file and have the privilege name passed in as a parameter.
Could you explain further how you see this working?
Something like int virHavePrivilege(const char *); Pass in PRIV_XEN_CONTROL, and on Solaris have that check with your prvilege code, and on Linux just have it ignore the param and check the UID. It is likely we'll do more fine grained checks based on named privileges on Linux too
+/* + * Attempt to access the domain via 'xenconsole', which may have + * additional privilege to reach the console. + */ +static int +vshXenConsole(int id)
Clearly this has to die.
It's not at all clear to me. virsh console is already a wart, and I don't see the problem in a pragmatic approach here, given it only ever tries the direct approach as a fallback.
Well, xm console is the true wart since it is needlessly design to only work with Xen, when it could have trivially been portable. Thus we have virsh console which is portable - fetching the PTY from the XML document instead.
The virsh console command implements exactly the same functionality as xenconsole without being xen specific.
I'm guessing you're doing this because you need the console in a separate process from virsh, so it can run with different privileges.
Yes. In particular it needs to run setuid root and:
- get the pts from xenstore (needs PRIV_XVM_CONTROL)
That's already obtainable from the XML doc & since you run the Xen driver within the daemon, it should have the PRIV_XVM_CONTROL privilege already.
- open the pty (requires all privileges) - drop all privileges before continuing
If so, we should probably split 'virsh console' out into a separate standlone binary/command 'virt-console'. This will let you get the privilege separation without relying on Xen specific commands.
This just adds a whole bunch of extra code to no clear advantage?
There's no extra code because virsh console already exists and isn't going away because it offers a superset of what xenconsole does, by virtue of being portable Dan. -- |: Red Hat, Engineering, Boston -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 :|
On Thu, Apr 24, 2008 at 03:59:26PM +0100, Daniel P. Berrange wrote:
I'd just like to see if have an impl for Solaris - whether you happen to use it or not, is separate issue. Some people may be taking libvirt
Fair enough.
When there's genuine configuration values that users need for something enabled on Solaris, it will be in SMF anyway, so will need re-working on the config backend in libvirt.
So you're not making use of any of the remote management features like TLS and SASL/Kerberos ? I'd think at least users need the config to switch between which auth scheme they'd like to use, because the choice of TLS vs Kerberos is really a deployment question for admins.
Today, we're not. When we do, as I said, we'll need libvirt changes to understand SMF configuration instead of the config file, so we'll have to fix up things there.
+int +xenHavePrivilege() +{ +#ifdef __sun + return priv_ineffect (PRIV_XVM_CONTROL); +#else + return getuid () == 0; +#endif +}
As mentioned earlier, we probably want to move this into the util.c file and have the privilege name passed in as a parameter.
Could you explain further how you see this working?
Something like
int virHavePrivilege(const char *);
Pass in PRIV_XEN_CONTROL, and on Solaris have that check with your prvilege code, and on Linux just have it ignore the param and check the UID. It is likely we'll do more fine grained checks based on named privileges on Linux too
I'm not sure I like this. It's pushing out a Solaris-specific detail ("PRIV_XVM_CONTROL") to every user. Furthermore, I'm not sure about it being generic either: PRIV_XVM_CONTROL wouldn't apply to qemu on Solaris, or zones, or whatever.
This just adds a whole bunch of extra code to no clear advantage?
There's no extra code because virsh console already exists and isn't going away because it offers a superset of what xenconsole does, by virtue of being portable
Sure there is: - code to spawn and run virt-console - all the current virsh console code in virt-console - the least-priv implementation in virt-console, duplicating xenconsole Note the latter is going to be Xen-specific (indeed, Xen on Solaris specific). There's no way I can avoid that. Anyway, if you're set on this, I'm not entirely averse to it, I'm just not sure I see the point. regards, john
participants (3)
-
Daniel P. Berrange -
Daniel Veillard -
John Levon