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@xxxxxxxxxx> > > > > 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@xxxxxxxxxx> > > --- > > 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. > > 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. GNUTLS 3.x has ditched gcrypt, so hopefully libnettle is better in this respect. 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 :| -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list