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

Martin Kletzander mkletzan at redhat.com
Fri Jun 1 12:10:56 UTC 2018


On Fri, Jun 01, 2018 at 11:25:35AM +0100, Daniel P. Berrangé wrote:
>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 at redhat.com>
>> > > > ---
>> > > > 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.
>

On FreeBSD it is a symlink to /dev/random (they both behave like getrandom() on
Linux) and I guess on MacOS it is the same.  Random search showed it exists
there.

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

Sure, we can do that.  It's just some crust (more configure checks and
conditional compilations, etc.) in case libvirt would run so early that
/dev/urandom was not properly seeded.  Is there a modern distribution that
doesn't seed /dev/urandom during boot before starting any services?

>
>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 :|
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180601/c52eb1ad/attachment-0001.sig>


More information about the libvir-list mailing list