On Tue, Feb 09, 2016 at 11:51:57AM +0100, Pino Toscano wrote: > 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 <[email protected]> > 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 ACK. Since the chmod in this case has been folded into the guestfs_int_lazy_make_sockdir function, there should probably be a followup commit to do the same for guestfs_int_lazy_make_tmpdir. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-df lists disk usage of guests without needing to install any software inside the virtual machine. Supports Linux and Windows. http://people.redhat.com/~rjones/virt-df/ _______________________________________________ Libguestfs mailing list [email protected] https://www.redhat.com/mailman/listinfo/libguestfs
