[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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

Hi Daniel,
Thanks a lot for the patch, yes it is working as expected.
# virsh -c qemu://localhost/system version
Compiled against library: libvirt 0.10.2
Using library: libvirt 0.10.2
Using API: QEMU 0.10.2
Running hypervisor: QEMU 0.14.1
But can you please clarify my below query.
Do we really need the red coloured lines as well ?
Our patch :
In function virInitialize()
+ #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);
+        gcry_control(GCRYCTL_DISABLE_SECMEM, NULL, 0);
+        gcry_control(GCRYCTL_ENABLE_QUICK_RANDOM, 0);
+    }
+ #endif
 Thanks and Regards
 Shree Duth Awasthi.

On Fri, Apr 12, 2013 at 9:31 PM, Eric Blake <eblake redhat com> wrote:
On 04/12/2013 10:27 AM, Daniel P. Berrange wrote:
> From: "Daniel P. Berrange" <berrange 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



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 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

[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]