From: libssh2 Trac <trac_at_libssh2.stuge.se>
Date: Wed, 28 Sep 2011 22:10:13 -0000

#229: static const EVP_CIPHER * make_ctr_evp (size_t keylen) in openssl.c
not threadsafe
-----------------------+--------------------
  Reporter: engstrom | Owner: bagder
      Type: defect | Status: closed
  Priority: normal | Milestone: 1.2.9
Component: API | Version: 1.3.0
Resolution: fixed | Keywords:
Blocked By: | Blocks:
-----------------------+--------------------

Comment (by engstrom):

Replying to [comment:4 bagder]:
> In [24afd0fc723ade2a0769ff5f3d43b9621d9fdd25/libssh2]:
> {{{
> #!CommitTicketReference repository="libssh2"
revision="24afd0fc723ade2a0769ff5f3d43b9621d9fdd25"
> openssl: don't init static structs differently
>
> make_ctr_evp() is changed to take a struct pointer, and then each
> _libssh2_EVP_aes_[keylen]_ctr function is made to pass in their own
> static struct
>
> Reported by: John Engstrom
> Fixes #229
> }}}

bagder, thanks for the fix but it doesn't solve all the problems. You
correctly identified that trying to use the same copy of the static
structure for three different key sizes will not work. However, there is
another problem that still exists with your fix - the static structure
passed in to make_ctr_evp() is getting memset() so that all members of the
structure are zeroed out.

Let's take an example program that has two threads which both call
libssh2_session_startup() at the same time. Thread 1 happens to get
scheduled first and calls down into _libssh2_cipher_init() - here's an
example of thread 1's stack trace:

   [1] _libssh2_cipher_init(), line 178 in "openssl.c"
   [2] crypt_init(), line 87 in "crypt.c"
   [3] diffie_hellman_sha1(), line 487 in "kex.c"
   [4] kex_method_diffie_hellman_group14_sha1_key_exchange(), line 796 in
"kex.c"
   [5] _libssh2_kex_exchange(), line 1757 in "kex.c"
   [6] session_startup(), line 709 in "session.c"
   [7] libssh2_session_handshake(), line 787 in "session.c"
   [8] libssh2_session_startup(), line 806 in "session.c"

In _libssh2_cipher_init() at line 178 of openssl.c is the following line:
EVP_CipherInit(h, algo(), secret, iv, encrypt);

algo() is a function pointer that is set to call
_libssh2_EVP_aes_128_ctr(). After calling _libssh2_EVP_aes_128_ctr() at
line 178 of openssl.c the OpenSSL function EVP_CipherInit() is called and
the address of the static structure that _libssh2_EVP_aes_128_ctr()
returned to us is passed as a parameter to EVP_CipherInit(). After
running some of the EVP_CipherInit() function but not all of it the
operating system scheduler decides that thread 1 has used up its
allocation of CPU for now and that thread get suspended and thread 2
starts up. Thread 1's callstack now looks like this:

   [1] EVP_CipherInit(), line 90 in "evp_enc.c" <----- NOTE: This is an
OpenSSL function
   [2] _libssh2_cipher_init(), line 178 in "openssl.c"
   [3] crypt_init(), line 87 in "crypt.c"
   [4] diffie_hellman_sha1(), line 487 in "kex.c"
   [5] kex_method_diffie_hellman_group14_sha1_key_exchange(), line 796 in
"kex.c"
   [6] _libssh2_kex_exchange(), line 1757 in "kex.c"
   [7] session_startup(), line 709 in "session.c"
   [8] libssh2_session_handshake(), line 787 in "session.c"
   [9] libssh2_session_startup(), line 806 in "session.c"

Thead 2 also calls into libssh2_session_startup() but with a different
session that was created. The call stack for thread 2 looks like the call
stack for thread 1 did before the call to EVP_CipherInit:

Thread 2 calls _libssh2_EVP_aes_128_ctr() which calls into make_ctr_evp()
which calls memset() on the static structure whose address is currently
being used by the call to OpenSSL's EVP_CipherInit() in thread 1. Because
it's a busy computer and the OS scheduler is capricious thread 2 gets
suspended right after the call to memset() and thread 1 resumes execution.
Within the call to EVP_CipherInit() the init routine that's part of the
static structure that got memset to 0 by thread 2 is called. When calling
a function pointer at the address of 0 occurs the program crashes and core
is dumped.

It's the memset() to 0 that is also a problem in addition to the setting
of the key size that you already identified. By creating three different
static structures and statically initializing them
(e.g. static EVP_CIPHER aes_ctr_cipher16 = {0, 16, 16, 16, 0,
aes_ctr_init, aes_ctr_do_cipher, aes_ctr_cleanup, 0, NULL, NULL, NULL,
NULL};
static EVP_CIPHER aes_ctr_cipher24 = {0, 16, 24, 16, 0, aes_ctr_init,
aes_ctr_do_cipher, aes_ctr_cleanup, 0, NULL, NULL, NULL, NULL};
static EVP_CIPHER aes_ctr_cipher32 = {0, 16, 32, 16, 0, aes_ctr_init,
aes_ctr_do_cipher, aes_ctr_cleanup, 0, NULL, NULL, NULL, NULL};)
we set their values at program load time before main() is called and
therefore the function pointers contained in those structures are never 0.

One way to accomplish this is by changing _libssh2_EVP_aes_128_ctr() from:

_libssh2_EVP_aes_128_ctr(void)
{
static EVP_CIPHER aes_ctr_cipher;
return make_ctr_evp (16, &aes_ctr_cipher);
}

To:

_libssh2_EVP_aes_128_ctr(void)
{
static EVP_CIPHER aes_ctr_cipher16={0, 16, 16, 16, 0, aes_ctr_init,
aes_ctr_do_cipher, aes_ctr_cleanup, 0, NULL, NULL, NULL, NULL};
return &aes_ctr_cipher16;
}

Thanks for taking the time to look at this bug - please let me know if
there's anything more info that I can provide.

-- 
Ticket URL: <http://trac.libssh2.org/ticket/229#comment:5>
libssh2 <http://trac.libssh2.org/>
C library for writing portable SSH2 clients
_______________________________________________
libssh2-devel http://cool.haxx.se/cgi-bin/mailman/listinfo/libssh2-devel

Received on 2011-09-29

Re: [libssh2] #229: static const EVP_CIPHER * make_ctr_evp (size_t keylen) in openssl.c not threadsafe