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

[Pino Toscano]

On Thursday, 29 June 2017 13:32:10 CEST Daniel P. Berrange wrote:
> 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.

That's why Rich suggested a way to override this check, and still allow
to open the disks of the connection specified.

> 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.

NACK -- you can perfectly open local disks, if their permissions is not
restricted for root, and non-users have the permissions to access the
system libvirt.  The above suggestion is way too strict, and actually
brings no advantage over my patch.

> > > 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.

Yes, I know this (I briefly mentioned that in one of my earlier emails),
I checked that with Peter some time ago.  Also, unless you expose that
in the capabilities for the "test" driver as well, it is not much
useful anyway, since you cannot compare that UUID with anything else.

Or, if libvirt would have a way to know whether a connection is local,
then fiddling with UUIDs would not be needed.

-- 
Pino Toscano
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part.
URL: <http://listman.redhat.com/archives/libguestfs/attachments/20170629/6204f713/attachment.sig>