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

[SHREE DUTH AWASTHI]

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_INITIALIZATION_FINISHED, 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 at redhat.com> wrote:

> 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 --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20130413/46807dbf/attachment-0001.htm>