libvirt users - Jun 2018 - Re: [libvirt] virRandomBits - not very random

On Fri, Jun 01, 2018 at 11:17:44AM +0100, Daniel P. Berrangé
wrote:
>On Wed, May 30, 2018 at 10:21:54PM +0200, Martin Kletzander wrote:
>> On Tue, May 29, 2018 at 10:06:25AM -0400, John Ferlan wrote:
>> >
>> >
>> > On 05/29/2018 09:44 AM, Michal Privoznik wrote:
>> > > On 05/29/2018 03:38 PM, Martin Kletzander wrote:
>> > > > On Fri, May 25, 2018 at 09:37:44AM -0500, Eric Blake
wrote:
>> > > > > On 05/25/2018 09:17 AM, Michal Privoznik wrote:
>> > > > >
>> > > > > > > > We should probably seed it with data
from /dev/urandom, and/or the new
>> > > > > > > > Linux getrandom() syscall (or BSD
equivalent).
>> > > > > >
>> > > > > > I'm not quite sure that right after reboot
there's going to be enough
>> > > > > > entropy. Every service that's starting
wants some random bits. But it's
>> > > > > > probably better than what we have now.
>> > > > >
>> > > > > Here's where we left things last time it came
up:
>> > > > >
>> > > > >
https://www.redhat.com/archives/libvir-list/2014-December/msg00573.html
>> > > > >
>> > > > > If gnutls has an interface that will give us random
bits
>> > > > > (gnutls_key_generate() in 3.0, perhaps), we should
use THAT for all of
>> > > > > our random bits (and forget about a seed), except
when we are mocking
>> > > > > things in our testsuite, and need a deterministic
PRNG from a
>> > > > > deterministic seed.
>> > > > >
>> > > > > If not (including if we are not linked with
gnutls), then we should
>> > > > > prefer the new Linux syscall but fall back to
/dev/urandom for JUST
>> > > > > enough bits for a seed; once we're seeded,
stick with using our existing
>> > > > > PRNG for all future bits (after all, we aren't
trying to generate
>> > > > > cryptographically secure keys using virRandomBits -
and the places where
>> > > > > we DO need crypto-strong randomness such as setting
up TLS migration is
>> > > > > where we are relying on gnutls to provide it rather
than virRandomBits).
>> > > > >
>> > > > > So at this point, it's just a matter of someone
writing the patches.
>> > > > >
>> > > >
>> > > > Actually, do we need to have a fallback at all? 
Can't we just drop all the
>> > > > gross parts of the code the conditionally compile based
on GNUTLS
>> > > > support?  Why
>> > > > don't we have gnutls required?
>> > >
>> > > That's exactly what I'm suggesting in my patches [1].
gnutls is widely
>> > > available (including Linux, Windows, *BSD, Mac Os X).
However, before
>> > > doing that we need to fix virRandomBits() to actually call
gnutls_rnd().
>> > >
>> > > 1:
https://www.redhat.com/archives/libvir-list/2018-May/msg02077.html
>> > >
>> >
>> > I have this faint recollection of one of the CI platform builds
failing
>> > because something in the gnutls* family didn't exist there
when I was
>> > making the changes to add the domain master secret code....  After
a bit
>> > of digging, it seems it was a perhaps a CENTOS6 environment:
>> >
>> >
https://www.redhat.com/archives/libvir-list/2016-April/msg00287.html
>> >
>> > and since IIUC that's not an issue any more....
>> >
>>
>> Oh, cool to know.  Michal also found the patch [1] where Dan switched
the gnutls
>> from being mandatory to making it optional and there is no explanation
for that
>> change in the commit message:
>>
>> [1] f587c27768ee13f5bed6a9262106307b7a124403
>
>Not all usage scenarios in libvirt have required GNUTLS - only the remote
>driver, when using stateful virt drivers. If you're just biulding
libvirt
>for usage with ESX/HyperV/etc, there's no reason you'd want GNUTLS
historically.
>
>Also note when building the setuid libvirt pieces we must never use GNUTLS
>because its library constructors do very bad things leading to CVEs.
>
Good to know, thanks for the info.  I'll need to read up on the gnutls
constructors (is it just gnutls or some other libs as well?) even though I
remember researching something related to it some time ago.  However that
doesn't mean it has to be optional.

Do you think there are people who would be bothered by the requirement of
gnutls?  I'm sure most of them have gnutls anyway.  Some of them even need
to
have, for example if you have a look at virCryptoHashBuf() its stub returns -1
and it is used in ESX.  If you compile without GNUTLS, all the functions that
require it will just fail and it's not something that is used rarely.

We can we make it required only if you are building with qemu or remote or esx
or basically something that requires it.  Encryption of RAW volumes for example.
Or lock driver.  But I don't think there are some who run the builds
themselves
and doesn't want gnutls on their system.  I can't think of a reason for
having
it and not linking libvirt with it.
>
>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:01:03PM +0200, Martin Kletzander
wrote:
> On Fri, Jun 01, 2018 at 11:17:44AM +0100, Daniel P. Berrangé wrote:
> > On Wed, May 30, 2018 at 10:21:54PM +0200, Martin Kletzander wrote:
> > > On Tue, May 29, 2018 at 10:06:25AM -0400, John Ferlan wrote:
> > > >
> > > >
> > > > On 05/29/2018 09:44 AM, Michal Privoznik wrote:
> > > > > On 05/29/2018 03:38 PM, Martin Kletzander wrote:
> > > > > > On Fri, May 25, 2018 at 09:37:44AM -0500, Eric
Blake wrote:
> > > > > > > On 05/25/2018 09:17 AM, Michal Privoznik
wrote:
> > > > > > >
> > > > > > > > > > We should probably seed it
with data from /dev/urandom, and/or the new
> > > > > > > > > > Linux getrandom() syscall (or
BSD equivalent).
> > > > > > > >
> > > > > > > > I'm not quite sure that right after
reboot there's going to be enough
> > > > > > > > entropy. Every service that's
starting wants some random bits. But it's
> > > > > > > > probably better than what we have now.
> > > > > > >
> > > > > > > Here's where we left things last time it
came up:
> > > > > > >
> > > > > > >
https://www.redhat.com/archives/libvir-list/2014-December/msg00573.html
> > > > > > >
> > > > > > > If gnutls has an interface that will give us
random bits
> > > > > > > (gnutls_key_generate() in 3.0, perhaps), we
should use THAT for all of
> > > > > > > our random bits (and forget about a seed),
except when we are mocking
> > > > > > > things in our testsuite, and need a
deterministic PRNG from a
> > > > > > > deterministic seed.
> > > > > > >
> > > > > > > If not (including if we are not linked with
gnutls), then we should
> > > > > > > prefer the new Linux syscall but fall back to
/dev/urandom for JUST
> > > > > > > enough bits for a seed; once we're
seeded, stick with using our existing
> > > > > > > PRNG for all future bits (after all, we
aren't trying to generate
> > > > > > > cryptographically secure keys using
virRandomBits - and the places where
> > > > > > > we DO need crypto-strong randomness such as
setting up TLS migration is
> > > > > > > where we are relying on gnutls to provide it
rather than virRandomBits).
> > > > > > >
> > > > > > > So at this point, it's just a matter of
someone writing the patches.
> > > > > > >
> > > > > >
> > > > > > Actually, do we need to have a fallback at all? 
Can't we just drop all the
> > > > > > gross parts of the code the conditionally compile
based on GNUTLS
> > > > > > support?  Why
> > > > > > don't we have gnutls required?
> > > > >
> > > > > That's exactly what I'm suggesting in my
patches [1]. gnutls is widely
> > > > > available (including Linux, Windows, *BSD, Mac Os X).
However, before
> > > > > doing that we need to fix virRandomBits() to actually
call gnutls_rnd().
> > > > >
> > > > > 1:
https://www.redhat.com/archives/libvir-list/2018-May/msg02077.html
> > > > >
> > > >
> > > > I have this faint recollection of one of the CI platform
builds failing
> > > > because something in the gnutls* family didn't exist
there when I was
> > > > making the changes to add the domain master secret code.... 
After a bit
> > > > of digging, it seems it was a perhaps a CENTOS6 environment:
> > > >
> > > >
https://www.redhat.com/archives/libvir-list/2016-April/msg00287.html
> > > >
> > > > and since IIUC that's not an issue any more....
> > > >
> > > 
> > > Oh, cool to know.  Michal also found the patch [1] where Dan
switched the gnutls
> > > from being mandatory to making it optional and there is no
explanation for that
> > > change in the commit message:
> > > 
> > > [1] f587c27768ee13f5bed6a9262106307b7a124403
> > 
> > Not all usage scenarios in libvirt have required GNUTLS - only the
remote
> > driver, when using stateful virt drivers. If you're just biulding
libvirt
> > for usage with ESX/HyperV/etc, there's no reason you'd want
GNUTLS historically.
> > 
> > Also note when building the setuid libvirt pieces we must never use
GNUTLS
> > because its library constructors do very bad things leading to CVEs.
> > 
> 
> Good to know, thanks for the info.  I'll need to read up on the gnutls
> constructors (is it just gnutls or some other libs as well?) even though I
> remember researching something related to it some time ago.  However that
> doesn't mean it has to be optional.
> 
> Do you think there are people who would be bothered by the requirement of
> gnutls?  I'm sure most of them have gnutls anyway.  Some of them even
need to
> have, for example if you have a look at virCryptoHashBuf() its stub returns
-1
> and it is used in ESX.  If you compile without GNUTLS, all the functions
that
> require it will just fail and it's not something that is used rarely.
> 
> We can we make it required only if you are building with qemu or remote or
esx
> or basically something that requires it.  Encryption of RAW volumes for
example.
> Or lock driver.  But I don't think there are some who run the builds
themselves
> and doesn't want gnutls on their system.  I can't think of a reason
for having
> it and not linking libvirt with it.
I should clarify - I've no objection to making GNUTLS mandatory at configure
time - just that we need to continue to have WITH_GNUTLS conditionals in some
parts of the code in order to support the setuid build without GNUTLS. So we
might be able to remove some of the WITH_GNUTLS bits, but not all of them.

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

On Fri, Jun 01, 2018 at 01:25:26PM +0100, Daniel P. Berrangé
wrote:
>On Fri, Jun 01, 2018 at 02:01:03PM +0200, Martin Kletzander wrote:
>> On Fri, Jun 01, 2018 at 11:17:44AM +0100, Daniel P. Berrangé wrote:
>> > On Wed, May 30, 2018 at 10:21:54PM +0200, Martin Kletzander wrote:
>> > > On Tue, May 29, 2018 at 10:06:25AM -0400, John Ferlan wrote:
>> > > >
>> > > >
>> > > > On 05/29/2018 09:44 AM, Michal Privoznik wrote:
>> > > > > On 05/29/2018 03:38 PM, Martin Kletzander wrote:
>> > > > > > On Fri, May 25, 2018 at 09:37:44AM -0500, Eric
Blake wrote:
>> > > > > > > On 05/25/2018 09:17 AM, Michal Privoznik
wrote:
>> > > > > > >
>> > > > > > > > > > We should probably seed it
with data from /dev/urandom, and/or the new
>> > > > > > > > > > Linux getrandom() syscall
(or BSD equivalent).
>> > > > > > > >
>> > > > > > > > I'm not quite sure that right
after reboot there's going to be enough
>> > > > > > > > entropy. Every service that's
starting wants some random bits. But it's
>> > > > > > > > probably better than what we have
now.
>> > > > > > >
>> > > > > > > Here's where we left things last time
it came up:
>> > > > > > >
>> > > > > > >
https://www.redhat.com/archives/libvir-list/2014-December/msg00573.html
>> > > > > > >
>> > > > > > > If gnutls has an interface that will give
us random bits
>> > > > > > > (gnutls_key_generate() in 3.0, perhaps),
we should use THAT for all of
>> > > > > > > our random bits (and forget about a
seed), except when we are mocking
>> > > > > > > things in our testsuite, and need a
deterministic PRNG from a
>> > > > > > > deterministic seed.
>> > > > > > >
>> > > > > > > If not (including if we are not linked
with gnutls), then we should
>> > > > > > > prefer the new Linux syscall but fall
back to /dev/urandom for JUST
>> > > > > > > enough bits for a seed; once we're
seeded, stick with using our existing
>> > > > > > > PRNG for all future bits (after all, we
aren't trying to generate
>> > > > > > > cryptographically secure keys using
virRandomBits - and the places where
>> > > > > > > we DO need crypto-strong randomness such
as setting up TLS migration is
>> > > > > > > where we are relying on gnutls to provide
it rather than virRandomBits).
>> > > > > > >
>> > > > > > > So at this point, it's just a matter
of someone writing the patches.
>> > > > > > >
>> > > > > >
>> > > > > > Actually, do we need to have a fallback at
all?  Can't we just drop all the
>> > > > > > gross parts of the code the conditionally
compile based on GNUTLS
>> > > > > > support?  Why
>> > > > > > don't we have gnutls required?
>> > > > >
>> > > > > That's exactly what I'm suggesting in my
patches [1]. gnutls is widely
>> > > > > available (including Linux, Windows, *BSD, Mac Os
X). However, before
>> > > > > doing that we need to fix virRandomBits() to
actually call gnutls_rnd().
>> > > > >
>> > > > > 1:
https://www.redhat.com/archives/libvir-list/2018-May/msg02077.html
>> > > > >
>> > > >
>> > > > I have this faint recollection of one of the CI platform
builds failing
>> > > > because something in the gnutls* family didn't exist
there when I was
>> > > > making the changes to add the domain master secret
code....  After a bit
>> > > > of digging, it seems it was a perhaps a CENTOS6
environment:
>> > > >
>> > > >
https://www.redhat.com/archives/libvir-list/2016-April/msg00287.html
>> > > >
>> > > > and since IIUC that's not an issue any more....
>> > > >
>> > >
>> > > Oh, cool to know.  Michal also found the patch [1] where Dan
switched the gnutls
>> > > from being mandatory to making it optional and there is no
explanation for that
>> > > change in the commit message:
>> > >
>> > > [1] f587c27768ee13f5bed6a9262106307b7a124403
>> >
>> > Not all usage scenarios in libvirt have required GNUTLS - only the
remote
>> > driver, when using stateful virt drivers. If you're just
biulding libvirt
>> > for usage with ESX/HyperV/etc, there's no reason you'd
want GNUTLS historically.
>> >
>> > Also note when building the setuid libvirt pieces we must never
use GNUTLS
>> > because its library constructors do very bad things leading to
CVEs.
>> >
>>
>> Good to know, thanks for the info.  I'll need to read up on the
gnutls
>> constructors (is it just gnutls or some other libs as well?) even
though I
>> remember researching something related to it some time ago.  However
that
>> doesn't mean it has to be optional.
>>
>> Do you think there are people who would be bothered by the requirement
of
>> gnutls?  I'm sure most of them have gnutls anyway.  Some of them
even need to
>> have, for example if you have a look at virCryptoHashBuf() its stub
returns -1
>> and it is used in ESX.  If you compile without GNUTLS, all the
functions that
>> require it will just fail and it's not something that is used
rarely.
>>
>> We can we make it required only if you are building with qemu or remote
or esx
>> or basically something that requires it.  Encryption of RAW volumes for
example.
>> Or lock driver.  But I don't think there are some who run the
builds themselves
>> and doesn't want gnutls on their system.  I can't think of a
reason for having
>> it and not linking libvirt with it.
>
>I should clarify - I've no objection to making GNUTLS mandatory at
configure
>time - just that we need to continue to have WITH_GNUTLS conditionals in
some
>parts of the code in order to support the setuid build without GNUTLS. So we
>might be able to remove some of the WITH_GNUTLS bits, but not all of them.
>
Oh, OK.  Great.
>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
:|