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@redhat.com>
---
src/util/virrandom.c | 63 ++--------------------------------------------------
1 file changed, 2 insertions(+), 61 deletions(-)
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
@@ -49,53 +49,6 @@ VIR_LOG_INIT("util.random");
#define RANDOM_SOURCE "/dev/urandom"
-/* The algorithm of virRandomBits relies on gnulib's guarantee that
- * 'random_r' matches the POSIX requirements on 'random' of being
- * evenly distributed among exactly [0, 2**31) (that is, we always get
- * exactly 31 bits). While this happens to be the value of RAND_MAX
- * on glibc, note that POSIX only requires RAND_MAX to be tied to the
- * weaker 'rand', so there are platforms where RAND_MAX is smaller
- * than the range of 'random_r'. For the results to be evenly
- * distributed among up to 64 bits, we also rely on the period of
- * 'random_r' to be at least 2**64, which POSIX only guarantees for
- * 'random' if you use 256 bytes of state. */
-enum {
- RANDOM_BITS_PER_ITER = 31,
- RANDOM_BITS_MASK = (1U << RANDOM_BITS_PER_ITER) - 1,
- RANDOM_STATE_SIZE = 256,
-};
-
-static char randomState[RANDOM_STATE_SIZE];
-static struct random_data randomData;
-static virMutex randomLock = VIR_MUTEX_INITIALIZER;
-
-
-static int
-virRandomOnceInit(void)
-{
- unsigned int seed = time(NULL) ^ getpid();
-
-#if 0
- /* Normally we want a decent seed. But if reproducible debugging
- * of a fixed pseudo-random sequence is ever required, uncomment
- * this block to let an environment variable force the seed. */
- const char *debug = virGetEnvBlockSUID("VIR_DEBUG_RANDOM_SEED");
-
- if (debug && virStrToLong_ui(debug, NULL, 0, &seed) < 0)
- return -1;
-#endif
-
- if (initstate_r(seed,
- randomState,
- sizeof(randomState),
- &randomData) < 0)
- return -1;
-
- return 0;
-}
-
-VIR_ONCE_GLOBAL_INIT(virRandom)
-
/**
* virRandomBits:
* @nbits: Number of bits of randommess required
@@ -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;
}
- virMutexLock(&randomLock);
-
- while (nbits > RANDOM_BITS_PER_ITER) {
- random_r(&randomData, &bits);
- ret = (ret << RANDOM_BITS_PER_ITER) | (bits & RANDOM_BITS_MASK);
- nbits -= RANDOM_BITS_PER_ITER;
- }
-
- random_r(&randomData, &bits);
- ret = (ret << nbits) | (bits & ((1 << nbits) - 1));
-
- virMutexUnlock(&randomLock);
+ ret &= (1U << nbits) - 1;
return ret;
}
--
2.16.1
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 05/29/2018 03:24 AM, 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@redhat.com>
> ---
> src/util/virrandom.c | 63 ++--------------------------------------------------
> 1 file changed, 2 insertions(+), 61 deletions(-)
>
> -static int
> -virRandomOnceInit(void)
> -{
> - unsigned int seed = time(NULL) ^ getpid();
> -
> -#if 0
> - /* Normally we want a decent seed. But if reproducible debugging
> - * of a fixed pseudo-random sequence is ever required, uncomment
> - * this block to let an environment variable force the seed. */
> - const char *debug = virGetEnvBlockSUID("VIR_DEBUG_RANDOM_SEED");
> -
> - if (debug && virStrToLong_ui(debug, NULL, 0, &seed) < 0)
> - return -1;
Are we sure we don't need the ability to quickly compile in a
deterministic replacement for debug in the future? I suppose there's
always git history, but this particular #if 0 was left in place for a
reason, where completely removing it makes it harder to realize that
such debugging is even possible.
--
Eric Blake, Principal Software Engineer
Red Hat, Inc. +1-919-301-3266
Virtualization: qemu.org | libvirt.org
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
On 05/29/2018 10:32 PM, Eric Blake wrote:
> On 05/29/2018 03:24 AM, 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@redhat.com>
>> ---
>> src/util/virrandom.c | 63
>> ++--------------------------------------------------
>> 1 file changed, 2 insertions(+), 61 deletions(-)
>>
>
>> -static int
>> -virRandomOnceInit(void)
>> -{
>> - unsigned int seed = time(NULL) ^ getpid();
>> -
>> -#if 0
>> - /* Normally we want a decent seed. But if reproducible debugging
>> - * of a fixed pseudo-random sequence is ever required, uncomment
>> - * this block to let an environment variable force the seed. */
>> - const char *debug = virGetEnvBlockSUID("VIR_DEBUG_RANDOM_SEED");
>> -
>> - if (debug && virStrToLong_ui(debug, NULL, 0, &seed) < 0)
>> - return -1;
>
> Are we sure we don't need the ability to quickly compile in a
> deterministic replacement for debug in the future? I suppose there's
> always git history, but this particular #if 0 was left in place for a
> reason, where completely removing it makes it harder to realize that
> such debugging is even possible.
>
Well, what we can now do is to cook a mock library that would implement
gnutls_rnd() to return some deterministic number without any need to
recompile libvirt at all. Therefore I don't think we should leave #if 0
in. And frankly, until I looked into this file I did not even know we
have such option.
Michal
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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?
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.
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?
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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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.
Anyway, I got a bit caught up there. I also learned something new and I think
the patch can be used like this. We might also ditch gnutls if we use
getrandom() on Linux before using /dev/urandom. That should be fine if we want
to take the other approach. But it looks like gnutls will be needed anyway,
so...
>>
>> 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.
>
I thought of it differently. The way it failed before was during
initialization, once per the daemon running for example, and then all the calls
to virRandomBytes were returning 0 all the time. This way (although I have no
idea when gnutls_rnd can fail) it can be returning fine random numbers and then,
out of nowhere, return 0 few times, then continue with random numbers.
I just wanted so that we both understand what the change here is. Since we're
logging the error already, maybe just resetting it is fine. Or actually it
could be left there.
So ACK if you remove random_r from bootstrap.conf.--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
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@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. 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
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@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 :| -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Fri, Jun 01, 2018 at 02:10:56PM +0200, Martin Kletzander wrote: > 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@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. Ah that's good to know - I guess they added the symlink due to software having assumptions from linux world :-) 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@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
© 2016 - 2025 Red Hat, Inc.