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

[Michal Privoznik]

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.

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

> 
> And one thing below:
> 
>> diff --git a/src/util/virrandom.c b/src/util/virrandom.c
>> index 444b0f9802..01cc82a052 100644
>> --- a/src/util/virrandom.c
>> +++ b/src/util/virrandom.c
>> @@ -108,26 +61,14 @@ VIR_ONCE_GLOBAL_INIT(virRandom)
>> uint64_t virRandomBits(int nbits)
>> {
>>     uint64_t ret = 0;
>> -    int32_t bits;
>>
>> -    if (virRandomInitialize() < 0) {
>> +    if (virRandomBytes((unsigned char *) &ret, sizeof(ret)) < 0) {
>>         /* You're already hosed, so this particular non-random value
>>          * isn't any worse.  */
>>         return 0;
> 
> We definitely want to return an error here now IMHO.

I'm not quite sure how to achieve that. The only thing I can think about
is changing virRandomBits() signature. But since it is pre-existing I
think it's safe to do in a follow up patch.

Michal