[krbdev.mit.edu #6413] pkinit thread safety with OpenSSL

[Greg Hudson via RT]

#7889 fixes part of the problem, by avoiding using the OpenSSL mutable 
OID table.

The major remaining issue has to do with library initialization.  At 
module initialization time, we call:

    CRYPTO_malloc_init();
    ERR_load_crypto_strings();
    OpenSSL_add_all_algorithms();

We try to do this only once, using a static variable, but we don't use a 
mutex around the static variable, so with threads we could conceivably 
call the functions multiple times in close proximity.  Other uses of 
OpenSSL in the same process could also call these functions at the same 
time as we do, whether or not we used a mutex or pthread_once.

We do not register locking callbacks as described in 
https://www.openssl.org/support/faq.html#PROG1.  In theory, OpenSSL has 
the right to be non-thread-safe in arbitrary ways because of this, 
unless the application or another OpenSSL user registers callbacks.  I 
am not aware of specific problems outside of initialization, but they 
may still exist.

We never call cleanup functions like EVP_cleanup, which is good for 
friendliness to other users of OpenSSL but means OpenSSL could leak heap 
memory if it were repeatedly loaded and unloaded.  Russ has noted that 
this scenario could happen because of PAM loading libkrb5 which then 
loads PKINIT.

CRYPTO_malloc_init is a macro which passes the malloc/realloc/free 
pointers from the caller's namespace into the library.  On a typical 
Unix build, this has no impact, since the library uses 
malloc/realloc/free from its own namespace by default.  On Windows 
(which we currently don't build PKINIT for, but might in the future), 
CRYPTO_malloc_init is supposed to help with the case where the caller 
was built in a different compiler mode from OpenSSL and has a different 
malloc implementation (https://www.openssl.org/support/faq.html#PROG2).  
Obviously this can't work simultaneously for multiple users of OpenSSL 
in the same process if they aren't all compiled in the same mode.

ERR_load_crypto_strings is necessary to produce text output from error 
messages (https://www.openssl.org/support/faq.html#PROG7).  Accesses to 
the OpenSSL global error table during initialization are protected by 
locks if locking callbacks are registered.

OpenSSL_add_all_algorithms is necessary to use ciphers and digests 
(https://www.openssl.org/support/faq.html#PROG8).  As with 
ERR_load_crypto_strings, accesses to the OpenSSL global error table 
during initialization are protected by locks if locking callbacks are 
registered.

We are somewhat limited in how much we can insure thread-safe 
initialization within PKINIT.  Using a k5-platform.h library initializer 
to initialize OpenSSL would help avoid some thread safety issues for the 
specific case where PKINIT is used in multiple threads but the process 
contains no other user of OpenSSL.  Registering our own locking 
callbacks would also work for that specific case, but could be very 
unfriendly to other users of OpenSSL.  Checking first if locking 
callbacks are already registered is itself not thread-safe.

Nico has patches at 
https://github.com/nicowilliams/openssl/commits/thread_safety to address 
thread-safe initialization within OpenSSL, which he believes could be 
accepted upstream if they were cleaned up and submitted.  I can't be 
sure if or when that will happen.

I don't think we can take any responsibility for preventing memory leaks 
on unload, unless OpenSSL adds new APIs for registering and 
unregistering callers like NSS has.  OpenSSL could perhaps address the 
issue through a library finalizer, if it is willing to take on the 
associated portability issues.