On Mon, Feb 17, 2025 at 11:31:32AM +0000, Richard W.M. Jones wrote:
> On Mon, Feb 17, 2025 at 02:14:49AM -0500, Laine Stump wrote:
> > ======
> >
> > I'm sending this as an RFC just because what it's doing feels kind of
> > dirty - directly examining XDG_RUNTIME_DIR seems like an "end run"
> > around the Glib API. If anyone has a better idea of what to do, please
> > give details :-)
> >
> > ======
> >
> > When running unprivileged (i.e. not as root, but as a regular user),
> > libvirt calls g_get_user_runtime_dir() (from Glib) to get the name of
> > a directory where status files can be saved. This is a directory that
> > is 1) writeable by the current user, and 2) will remain there until
> > the host reboots, but then 3) be erased after the reboot. This is used
> > for pidfiles, sockets created to communicate between processes, status
> > XML of active domains, etc.
> >
> > Normally g_get_user_runtime_dir() returns the setting of
> > XDG_RUNTIME_DIR in the user's environment; usually this is set to
> > /run/user/${UID} (e.g. /run/user/1000) - that directory is created
> > when a user first logs in and is owned by the user, but is cleared out
> > when the system reboots (more specifically, this directory usually
> > resides in a tmpfs, and so disappears when that tmpfs is unmounted).
> >
> > But sometimes XDG_RUNTIME_DIR isn't set in the user's environment. In
> > that case, g_get_user_runtime_dir() returns ${HOME}/.config
> > (e.g. /home/laine/.config). This directory fulfills the first 2
> > criteria above, but fails the 3rd. This isn't just some pedantic
> > complaint - libvirt actually depends on the directory being cleared
> > out during a reboot - otherwise it might think that stale status files
> > are indicating active guests when in fact the guests were shutdown
> > during the reboot).
> >
> > In my opinion this behavior is a bug in Glib - see the requirements
> > for XDG_RUNTIME in the FreeDesktop documentation here:
> >
> > https://specifications.freedesktop.org/basedir-spec/latest/#variables
> >
> > but they've documented the behavior as proper in the Glib docs for
> > g_get_user_runtime_dir(), and so likely will consider it not a bug.
> >
> > Beyond that, aside from failing the "must be cleared out during a reboot"
> > requirement, use of $HOME/.cache in this way also disturbs SELinux,
> > which gives an AVC denial when libvirt (or passt) tries to create a
> > file or socket in that directory (the SELinux policy permits use of
> > /run/user/$UID, but not of $HOME/.config). We *could* add that to the
> > SELinux policy, but since the glib behavior doesn't
> >
> > All of the above is a very long leadup to the functionality in this
> > patch: rather than blindly accepting the path returned from
> > g_get_user_runtime_dir(), we first check if XDG_RUNTIME_DIR is set; if
> > it isn't set then we look to see if /run/user/$UID exists and is
> > writable by this user, if so we use *that* as the directory for our
> > status files. Otherwise (both when XDG_RUNTIME_DIR is set, and when
> > /run/user/$UID isn't usable) we fallback to just using the path
> > returned by g_get_user_runtime_dir() - that isn't perfect, but it's
> > what we were doing before, so at least it's not any worse.
> >
> > Resolves: https://issues.redhat.com/browse/RHEL-70222
> > Signed-off-by: Laine Stump <laine@xxxxxxxxxx>
> > ---
> > src/util/virutil.c | 22 ++++++++++++++++++++++
> > 1 file changed, 22 insertions(+)
> >
> > diff --git a/src/util/virutil.c b/src/util/virutil.c
> > index 2abcb282fe..4c7f4b62bc 100644
> > --- a/src/util/virutil.c
> > +++ b/src/util/virutil.c
> > @@ -538,6 +538,28 @@ char *virGetUserRuntimeDirectory(void)
> > #ifdef WIN32
> > return g_strdup(g_get_user_runtime_dir());
> > #else
> > + /* tl;dr - if XDG_RUNTIME_DIR is set, use g_get_user_runtime_dir().
> > + * if not set, then see if /run/user/$UID works
> > + * if so, use that, else fallback to g_get_user_runtime_dir()
> > + *
> > + * this is done because the directory returned by
> > + * g_get_user_runtime_dir() when XDG_RUNTIME_DIR isn't set is
> > + * "suboptimal" (it's a location that is owned by the user, but
> > + * isn't erased when the user completely logs out)
> > + */
> > +
> > + if (!getenv("XDG_RUNTIME_DIR")) {
> > + g_autofree char *runtime_dir = NULL;
> > + struct stat sb;
> > +
> > + runtime_dir = g_strdup_printf("/run/user/%d", getuid());
>
> I'm wondering if this should be geteuid. (I'm not completely clear
> myself, I guess it depends on whether this code could be a called in a
> setuid / dropped privileges case, and if so what directory should be
> used.)
We explicitly block such usage
if (getuid() != geteuid() ||
getgid() != getegid()) {
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("libvirt.so is not safe to use from setuid/setgid programs"));
goto error;
}
It is unsafe because too many things we link to have ELF constructors
that are untrustworthy.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
