Content-Type: text/plain Content-Disposition: inline Content-Transfer-Encoding: binary MIME-Version: 1.0 X-Mailer: MIME-tools 5.427 (Entity 5.427) Subject: pkinit thread safety with OpenSSL RT-Send-CC: X-RT-Original-Encoding: iso-8859-1 Content-Length: 3644 #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.