Re: [Libguestfs] [PATCH v2 2/2] New API: get-sockdir

On Monday 08 February 2016 19:34:10 Richard W.M. Jones wrote: > On Wed, Feb 03, 2016 at 01:17:42PM +0100, Pino Toscano wrote: > > Introduce a new read-only API to get a path where to store temporary > > sockets: this is different from tmpdir, as we need short paths for > > sockets (due to sockaddr_un::sun_path), and it is either > > XDG_RUNTIME_DIR if set, or /tmp; adapt guestfs_int_create_socketname > > to create sockets in that location. > > > > Furthermore, print sockdir and XDG_RUNTIME_DIR in test-tool for > > debugging. > > As you saw, there were a few problems with this patch. However I also > found something more fundamental. > > On machines where XDG_RUNTIME_DIR is set to /run/user/$UID, it fails > badly when run as root: > > Original error from libvirt: internal error: process exited while > connecting to monitor: 2016-02-08T19:17:42.375986Z qemu-system-x86_64: > -chardev > socket,id=charserial0,path=/run/user/0/libguestfsdittS9/console.sock: > Failed to connect socket: Permission denied [code=1 int1=-1] > > This is because libvirt runs the appliance as qemu.qemu, which cannot > access /run/user/0 (mode 0700). > > This is the default configuration when accessing a remote machine > using `ssh root@remote virt-tool ...' This is not due to the work itself, but to the limitations coming from other parties involved (libvirt in this case). > I think we should drop all references to XDG_RUNTIME_DIR (as I noted > before, I don't have a high regard for freedesktop pseudo-standards, > which I believe are just a way for some people to "policy launder" > junk into Linux). There isn't any needed to scrap everything at the first issue, there actually is a way to make this work better. Also, not having high regards does not automatically mean that things are obnoxious, not that there isn't any middle ground possible. > The attached patch does that. Note the get-sockdir function now > returns the hard-coded value "/tmp", which may or may not be a good > idea. NACK for me. Attached there is a (IMHO) better approach to the issue found with libvirt: since only root needs particular changes, then limit them only for this user. After all, we all recommend using libguestfs and its tools as normal user as much as possible, right? So we shouldn't make the common user case worse only for non-recommended use cases. Thanks, -- Pino Toscano

>From 772f649e595d202bdb67f05aeb62157c1104be89 Mon Sep 17 00:00:00 2001 From: Pino Toscano <ptoscano(a)redhat.com&gt; Date: Tue, 9 Feb 2016 11:36:23 +0100 Subject: [PATCH] lib: fix sockdir for root When running as root libvirt will launch qemu as qemu.qemu, which will not be able to read inside the socket directory in case it is set as XDG_RUNTIME_DIR under /run/user/0. Since normal users don't need particular extra access permissions for their sockdirs, restrict /tmp as only possible sockdir for root, changing the permissions only for this user (and making this operation fatal). Fixes commit 55202a4d49a101392148d79cb2e1591428db2681 and commit 7453952d2456272624f154082e4fc4244af8e507. --- src/launch.c | 6 ------ src/tmpdirs.c | 35 +++++++++++++++++++++++++++++++---- 2 files changed, 31 insertions(+), 10 deletions(-) diff --git a/src/launch.c b/src/launch.c index 9e9960a..9273c58 100644 --- a/src/launch.c +++ b/src/launch.c @@ -428,12 +428,6 @@ guestfs_int_create_socketname (guestfs_h *g, const char *filename, if (guestfs_int_lazy_make_sockdir (g) == -1) return -1; - /* Allow qemu (which may be running as qemu.qemu) to read the socket - * temporary directory. (RHBZ#610880). - */ - if (chmod (g->sockdir, 0755) == -1) - warning (g, "chmod: %s: %m (ignored)", g->sockdir); - if (strlen (g->sockdir) + 1 + strlen (filename) > UNIX_PATH_MAX-1) { error (g, _("socket path too long: %s/%s"), g->sockdir, filename); return -1; diff --git a/src/tmpdirs.c b/src/tmpdirs.c index e66ab9c..76bf1c5 100644 --- a/src/tmpdirs.c +++ b/src/tmpdirs.c @@ -23,6 +23,7 @@ #include <sys/types.h> #include <sys/stat.h> #include <libintl.h> +#include <unistd.h> #include "ignore-value.h" @@ -130,11 +131,20 @@ char * guestfs_impl_get_sockdir (guestfs_h *g) { const char *str; + uid_t euid = geteuid (); - if (g->env_runtimedir) - str = g->env_runtimedir; - else + if (euid == 0) { + /* Use /tmp exclusively for root, as otherwise qemu (running as + * qemu.qemu when launched by libvirt) will not be able to access + * the directory. + */ str = "/tmp"; + } else { + if (g->env_runtimedir) + str = g->env_runtimedir; + else + str = "/tmp"; + } return safe_strdup (g, str); } @@ -168,7 +178,24 @@ guestfs_int_lazy_make_tmpdir (guestfs_h *g) int guestfs_int_lazy_make_sockdir (guestfs_h *g) { - return lazy_make_tmpdir (g, guestfs_get_sockdir, &g->sockdir); + int ret; + uid_t euid = geteuid (); + + ret = lazy_make_tmpdir (g, guestfs_get_sockdir, &g->sockdir); + if (ret == -1) + return ret; + + if (euid == 0) { + /* Allow qemu (which may be running as qemu.qemu) to read the socket + * temporary directory. (RHBZ#610880). + */ + if (chmod (g->sockdir, 0755) == -1) { + perrorf (g, "chmod: %s", g->sockdir); + return -1; + } + } + + return ret; } /* Recursively remove a temporary directory. If removal fails, just