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

[Eric Blake]

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.

> +    }
>  #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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 621 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130412/8aebab4c/attachment-0001.sig>