Skip Menu |
 

From: Ken Raeburn <raeburn@MIT.EDU>
To: krb5-bugs@MIT.EDU
Subject: pkinit thread safety
Date: Thu, 12 Mar 2009 17:36:16 -0400
Download (untitled) / with headers
text/plain 1.5KiB


Begin forwarded message:

Show quoted text
> From: Mark Phalan <Mark.Phalan@Sun.COM>
> Date: March 12, 2009 17:13:40 EDT
> To: Ken Raeburn <raeburn@mit.edu>
> Cc: Nikhil Mishra <nikhilm@gs-lab.com>, krbdev@mit.edu
> Subject: Re: Is MIT kerberos thread safe ??
> X-Spam-Score: 0.00
>
>
> On 12 Mar 2009, at 17:43, Ken Raeburn wrote:
>
>> On Mar 12, 2009, at 08:55, Nikhil Mishra wrote:
>>> As the subject says , Is MIT kerberos thread safe ?
>>> My device is a high performance network appliance and
>>> I need to analyze threadsafe-ness of MIT kerberos library.
>>
>> The 1.6.x releases should be thread-safe provided certain objects are
>> not shared across threads for simultaneous use, primarily the
>> Kerberos
>> and GSSAPI context types. Various other objects, especially the
>> simpler ones like krb5_data and krb5_principal, and most of the
>> structures exposed in our API, can be shared as long as both uses are
>> read-only. Some more complex, opaque types like krb5_ccache, and
>> most
>> if not all internal static data, have internal locking performed
>> within the libraries, so that they can be used from multiple threads
>> without corruption.
>>
>> Unfortunately, we don't have documentation written up on *exactly*
>> what can be shared across threads and when, but "never share
>> contexts,
>> and share other stuff only as inputs not outputs" is a good
>> guideline.
>>
>> And, all the above said, there could of course be bugs; if you run
>> into anything, we'd like to know.
>
>
> The PKINIT plugin is a problem if it is used in multiple threads due
> to its use of OpenSSL.
>
> -M
Subject: pkinit thread safety with OpenSSL
Download (untitled) / with headers
text/plain 3.5KiB
#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.
From: ghudson@mit.edu
Subject: git commit

In PKINIT, use library initializer for OpenSSL

Use a library initializer to prevent multiple threads using PKINIT
from concurently initializing OpenSSL functions. For cases where
MT-safety is not assured by registering OpenSSL locking callbacks,
this significantly lowers the odds of crashes caused by races in
OpenSSL initialization. (If OpenSSL initialization functions are
called by some other thread directly, crashes are still possible.)

[ghudson@mit.edu: simplify code changes and commit message]

https://github.com/krb5/krb5/commit/d49e9f0e14adb24e6fe129080c54a0571a39611b
Author: Tomas Kuthan <tkuthan@gmail.com>
Committer: Greg Hudson <ghudson@mit.edu>
Commit: d49e9f0e14adb24e6fe129080c54a0571a39611b
Branch: master
src/plugins/preauth/pkinit/pkinit_crypto_openssl.c | 25 ++++++++------------
1 files changed, 10 insertions(+), 15 deletions(-)