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

On 04/12/2013 10:27 AM, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; > > 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.htm... http://www.gnupg.org/documentation/manuals/gcrypt/Controlling-the-library... 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(a)redhat.com&gt; > --- > 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.

I'm not a gcrypt expert, so assuming you can explain the things I questioned above, then I don't mind this patch going in as-is. But it doesn't make me feel any better about gcrypt when it is this hard to set it up correctly for use in a multi-threaded library; and it doesn't help when their manual describes the setup but doesn't offer any sample code to copy from. Crypto library authors have sure made life tough on everyone.