On 04/12/2013 10:27 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange(a)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.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>
---
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.
+ }
#endif
virLogSetFromEnv();
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.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library
http://libvirt.org