[libvirt] [PATCH] Do more complete initialization of libgcrypt
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. 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); + + gcry_control(GCRYCTL_DISABLE_SECMEM, NULL, 0); + gcry_control(GCRYCTL_INITIALIZATION_FINISHED, NULL, 0); + gcry_control(GCRYCTL_ENABLE_QUICK_RANDOM, 0); + } #endif virLogSetFromEnv(); -- 1.8.1.4
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 http://www.gnupg.org/documentation/manuals/gcrypt/Multi_002dThreading.html#M... http://www.gnupg.org/documentation/manuals/gcrypt/Controlling-the-library.ht... 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
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@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
http://www.gnupg.org/documentation/manuals/gcrypt/Multi_002dThreading.html#M...
http://www.gnupg.org/documentation/manuals/gcrypt/Controlling-the-library.ht...
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
On 04/13/2013 04:54 AM, SHREE DUTH AWASTHI wrote:
Hi Daniel,
Thanks a lot for the patch, yes it is working as expected.
[Please don't top-post on technical lists]
# 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 ?
This list rejects html mail. Thus, we don't see any red-colored lines in your message.
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
The only difference between this and what Dan posted appears to be the #ifdef WITH_GNUTLS; was your patch prepared against libvirt.git? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
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@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#M...
http://www.gnupg.org/documentation/manuals/gcrypt/Controlling-the-library.ht...
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.
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 :|
On Mon, Apr 15, 2013 at 12:01:56PM +0100, Daniel P. Berrange wrote:
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@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#M...
http://www.gnupg.org/documentation/manuals/gcrypt/Controlling-the-library.ht...
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.
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.
Actually looking more closely, it seems the QUICK_RANDOM flag is merely a hack used to make the gnutls/libgcrypt test suite run more quickly. As such we can leave it out. 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 :|
participants (3)
-
Daniel P. Berrange -
Eric Blake -
SHREE DUTH AWASTHI