[libvirt] [PATCH] Do more complete initialization of libgcrypt

[Daniel P. Berrange]

On Mon, Apr 15, 2013 at 12:01:56PM +0100, Daniel P. Berrange wrote:
> On Fri, Apr 12, 2013 at 01:31:44PM -0600, Eric Blake wrote:
> > On 04/12/2013 10:27 AM, Daniel P. Berrange wrote:
> > > From: "Daniel P. Berrange" <berrange at redhat.com>
> > > 
> > > If libvirt makes any gcry_control() calls, then this
> > > prevents gnutls for doing any initialization. As such
> > > we must take care to do full initialization of libcrypt
> > > on a par with what gnutls would have done. In particular
> > > we must disable "sec mem" for cases where the user does
> > > not have mlock() permission. We also skip our init of
> > > libgcrypt if something else (ie the app using libvirt)
> > > has beaten us to it.
> > 
> > Wow, libgcrypt is just horrible with regards to its usability.  At any
> > rate, I read
> > 
> > http://www.gnupg.org/documentation/manuals/gcrypt/Multi_002dThreading.html#Multi_002dThreading
> > 
> > http://www.gnupg.org/documentation/manuals/gcrypt/Controlling-the-library.html#index-gcry_005fcontrol-6
> > 
> > and it seems to match with you conversion to a conditional initialization.
> > 
> > Is it worth pointing to
> > https://bugzilla.redhat.com/show_bug.cgi?id=951630 as your rationale?
> > 
> > > 
> > > Signed-off-by: Daniel P. Berrange <berrange at redhat.com>
> > > ---
> > >  src/libvirt.c | 10 ++++++++--
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/src/libvirt.c b/src/libvirt.c
> > > index c5221f5..7c0a873 100644
> > > --- a/src/libvirt.c
> > > +++ b/src/libvirt.c
> > > @@ -409,8 +409,14 @@ virGlobalInit(void)
> > >          goto error;
> > >  
> > >  #ifdef WITH_GNUTLS
> > > -    gcry_control(GCRYCTL_SET_THREAD_CBS, &virTLSThreadImpl);
> > > -    gcry_check_version(NULL);
> > > +    if (gcry_control(GCRYCTL_ANY_INITIALIZATION_P) == 0) {
> > > +        gcry_control(GCRYCTL_SET_THREAD_CBS, &virTLSThreadImpl);
> > > +        gcry_check_version(NULL);
> > 
> > Up to here makes sense.
> > 
> > > +
> > > +        gcry_control(GCRYCTL_DISABLE_SECMEM, NULL, 0);
> > 
> > Makes sense; the manual said most applications don't need secure memory,
> > and that FIPS mode ignores this (so we aren't breaking FIPS systems by
> > weakening anything they depend on).
> > 
> > > +        gcry_control(GCRYCTL_INITIALIZATION_FINISHED, NULL, 0);
> > 
> > Makes sense - we check if anyone has called gcry_check_version
> > (ANY_INITIALIZATION_P above), and if not then we finish full
> > initialization ourselves.  But is there a possible race where one thread
> > could have started basic initialization, and then two threads are
> > competing to finish initialization?
> > 
> > > +        gcry_control(GCRYCTL_ENABLE_QUICK_RANDOM, 0);
> > 
> > The gcrypt manual discourages this one unless you have consulted with a
> > crypto expert; are we violating the assumptions of any top layer
> > application that links with us?  Furthermore, the manual states that
> > ENABLE_QUICK_RANDOM can only be used at initialization time, but we just
> > declared that initialization was finished.
> 
> My rationale here is that I 100% copied the init code from gnutls
> in its file  ./lib/gcrypt/init.c
> 
> My presumption is that the gnutls developers know better than me,
> so I defer to their code as best practice for us to follow.

Actually looking more closely, it seems the QUICK_RANDOM flag is
merely a hack used to make the gnutls/libgcrypt test suite run
more quickly. As such we can leave it out.


Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|