Re: [PATCH 08/10] virrandom: Make virRandomBits better

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, May 30, 2018 at 03:43:31PM +0200, Martin Kletzander wrote:
> On Wed, May 30, 2018 at 02:16:08PM +0200, Michal Privoznik wrote:
> > On 05/29/2018 03:44 PM, Martin Kletzander wrote:
> > > On Tue, May 29, 2018 at 10:24:44AM +0200, Michal Privoznik wrote:
> > > > Now that we have strong PRNG generator implemented in
> > > > virRandomBytes() let's use that instead of gnulib's random_r.
> > > > 
> > > > Problem with the latter is in way we seed it: current UNIX time
> > > > and libvirtd's PID are not that random as one might think.
> > > > Imagine two hosts booting at the same time. There's a fair chance
> > > > that those hosts spawn libvirtds at the same time and with the
> > > > same PID. This will result in both daemons generating the same
> > > > sequence of say MAC addresses [1].
> > > > 
> > > > 1: https://www.redhat.com/archives/libvirt-users/2018-May/msg00097.html
> > > > 
> > > > Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
> > > > ---
> > > > src/util/virrandom.c | 63
> > > > ++--------------------------------------------------
> > > > 1 file changed, 2 insertions(+), 61 deletions(-)
> > > > 
> > > 
> > > ACK to patches 1-7.  But for this one I'm "concerned" about few things.
> > > 
> > > First of all, just so I don't forget it, random_r can be removed from
> > > bootstrap.conf after this patch, right?
> > 
> > Yes, I was meaning to make that change but then I forgot.
> > 
> > > 
> > > Before this patch, and without gnutls, we wouldn't deplete the entropy
> > > of the
> > > kernel, (even though we're just using /dev/urandom and not /dev/random),
> > > but now
> > > we'd read everything from /dev/urandom.
> > 
> > Unless we are built with gnutls. But I don't see much problem with that.
> > 
> 
> Yeah, it's not that big of a deal, just an extra point for the next thing I
> mentioned below.
> 
> > > 
> > > Last but not least, if we completely drop the non-gnutls variants of
> > > everything,
> > > wouldn't everything be easier anyway?  Like the worrying about entropy
> > > pool in
> > > previous point?
> > 
> > Sure. But requiring gnutls (like I'm suggesting in the cover letter) is
> > orthogonal to these patches IMO.
> > 
> 
> My point was that the fixes might be could be cleaner and shorter, but that "not
> that big of a deal" above would be fixed.  It also makes it kind of relevant.
> 
> Since /dev/urandom cannot be really exhausted in newer Linux kernels (not sure
> for FreeBSD and others), I don't think that's a problem.  We should ensure,
> however, that it is seeded properly.  It might not be when it's early during the
> boot for Linux (although systemd and others seed it explicitly early enough),
> that's what getrandom() is for as it ensures the seeding was done properly.  But
> that's Linux-specific.  FreeBSD will apparently not give you any data until it's
> seeded properly.

/dev/urandom does not even exist on non-Linux hosts AFAIK, so this has the
effect of breaking libvirt on non-Linux hosts when gnutls is disabled.

IMHO we should used be using   getrandom() as the first fallback, and only
then try /dev/urandom or /dev/random if the former doesn't exist


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

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux