Libguestfs - Jun 2017 - Re: [PATCH] libvirt: disallow non-local connections (RHBZ#1347830)

On Tuesday, 27 June 2017 22:56:25 CEST Richard W.M. Jones
wrote:
> 
> Not that I'm opposed to this patch, but there's a bit of history
here:
> 
> https://www.redhat.com/archives/libguestfs/2012-December/msg00120.html
Hm it doesn't say much more about that though, and the solution I
implemented is even less strict than what Dan suggested back then.
> I think it would be good for libvirt to address the "is remote"
issue,
> which libvirt is (in theory) in the best place to address, eg by
> comparing systemd /etc/machine-id on both systems.
I took the approach from what virt-manager does, i.e. consider local
connections whose URI has an empty hostname.

OTOH, currently in libvirt there is still no reliable way to detect
whether some connection is local: the internal calculation of the UUID
for the capabilities may use files and tools which can be read and run
by root only, so the output on the same host changes depending on the
user.
> Then we could use that to deny remote URIs, but probably we wouldn't
> want to deny it completely, but allow a way for callers to bypass the
> check if they know better.
That could be a good idea, how would you expect this bypass to look
like?

-- 
Pino Toscano

On Wed, Jun 28, 2017 at 10:33:48AM +0200, Pino Toscano
wrote:
> On Tuesday, 27 June 2017 22:56:25 CEST Richard W.M. Jones wrote:
> > 
> > Not that I'm opposed to this patch, but there's a bit of
history here:
> > 
> > https://www.redhat.com/archives/libguestfs/2012-December/msg00120.html
> 
> Hm it doesn't say much more about that though, and the solution I
> implemented is even less strict than what Dan suggested back then.
> 
> > I think it would be good for libvirt to address the "is
remote" issue,
> > which libvirt is (in theory) in the best place to address, eg by
> > comparing systemd /etc/machine-id on both systems.
> 
> I took the approach from what virt-manager does, i.e. consider local
> connections whose URI has an empty hostname.
Right, but ::1 and 127.0.0.1 etc are non-empty hostnames which refer
to the local machine.  I really think if we're going to add a check we
should add a correct check, and the only way to do this properly is in
libvirt.
> OTOH, currently in libvirt there is still no reliable way to detect
> whether some connection is local: the internal calculation of the UUID
> for the capabilities may use files and tools which can be read and run
> by root only, so the output on the same host changes depending on the
> user.
Can't we get libvirt to export the machine ID in capabilities?

There is already a //host/uuid field, but as you point out it
is different for different users (WTF?)
> > Then we could use that to deny remote URIs, but probably we
wouldn't
> > want to deny it completely, but allow a way for callers to bypass the
> > check if they know better.
> 
> That could be a good idea, how would you expect this bypass to look
> like?
I'm not sure.  An extra parameter passed in .query_raw I suppose.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org

On Thu, Jun 29, 2017 at 12:17:02PM +0100, Richard W.M. Jones
wrote:
> On Wed, Jun 28, 2017 at 10:33:48AM +0200, Pino Toscano wrote:
> > On Tuesday, 27 June 2017 22:56:25 CEST Richard W.M. Jones wrote:
> > > 
> > > Not that I'm opposed to this patch, but there's a bit of
history here:
> > > 
> > >
https://www.redhat.com/archives/libguestfs/2012-December/msg00120.html
> > 
> > Hm it doesn't say much more about that though, and the solution I
> > implemented is even less strict than what Dan suggested back then.
> > 
> > > I think it would be good for libvirt to address the "is
remote" issue,
> > > which libvirt is (in theory) in the best place to address, eg by
> > > comparing systemd /etc/machine-id on both systems.
> > 
> > I took the approach from what virt-manager does, i.e. consider local
> > connections whose URI has an empty hostname.
> 
> Right, but ::1 and 127.0.0.1 etc are non-empty hostnames which refer
> to the local machine.  I really think if we're going to add a check we
> should add a correct check, and the only way to do this properly is in
> libvirt.
Even 127.0.0.1 might refer to a remote machine if the user has any
kind of SSH tunnelling set going.

Also, even if 127.0.0.1 *is* pointing to the local machine, it is
going to be a libvirtd running as root. If guestfs is running as
non-root, then you're still not going to be able to open disk
images despite it being localhost.

So as described in the old mail thread, I think you'd be better
off mandating that with uid == 0, the URI is qemu:///system
and uid != 0, the URI is qemu:///session, and thus disallow
any connection URIs that can result in different hosts, or
different privilege levels.

> > OTOH, currently in libvirt there is still no reliable way to detect
> > whether some connection is local: the internal calculation of the UUID
> > for the capabilities may use files and tools which can be read and run
> > by root only, so the output on the same host changes depending on the
> > user.
> 
> Can't we get libvirt to export the machine ID in capabilities?
> 
> There is already a //host/uuid field, but as you point out it
> is different for different users (WTF?)
That is supposed to be populated from the SMBIOS product UUID,
with optional override from libvirtd.conf

If we can't read the SMBIOS UUID though, it seems we generate
a random UUID by default. Unfortunately non-root users can't
access the /sys/devices/virtual/dmi/id/product_uuid file
hence session mode libvirtd is reporting random uuids. This
is really awful because the host UUID will change every time
libvirtd starts too.

I think it is restricted access because some insane vendors used
UUID as a "secret" key to access hardware support records for
the host, with no other authentication required.

We should report a separate OS  UUID, populated from the systemd
machine-id file, as a distinct item from the hardware UUID.
> > > Then we could use that to deny remote URIs, but probably we
wouldn't
> > > want to deny it completely, but allow a way for callers to bypass
the
> > > check if they know better.
> > 
> > That could be a good idea, how would you expect this bypass to look
> > like?
> 
> I'm not sure.  An extra parameter passed in .query_raw I suppose.
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 :|