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

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

Comment (by engstrom):

 Replying to [comment:6 bagder]:
> Right. Can't we just remove memset()? It serves no purpose...

 the EVP_CIPHER structure contains 13 members and 6 of them (block_size,
 key_len, iv_len, init, do_cipher and cleanup) are being explicitly set.
 The memset() takes care of initializing the other 7 members so it's
 important.

 How about this solution - create three static global structures in
 openssl.c along with a function to initialize those structures (e.g.):

 {{{
 #!c
 static EVP_CIPHER aes_ctr_cipher16;
 static EVP_CIPHER aes_ctr_cipher24;
 static EVP_CIPHER aes_ctr_cipher32;

 void
 _libssh2_init_EVP_aes_ctr(void)
 {
     memset(&aes_ctr_cipher16, 0, sizeof(aes_ctr_cipher16));
     aes_ctr_cipher16.block_size = 16;
     aes_ctr_cipher16.key_len = 16;
     aes_ctr_cipher16.iv_len = 16;
     aes_ctr_cipher16.init = aes_ctr_init;
     aes_ctr_cipher16.do_cipher = aes_ctr_do_cipher;
     aes_ctr_cipher16.cleanup = aes_ctr_cleanup;

     memset(&aes_ctr_cipher24, 0, sizeof(aes_ctr_cipher24));
     aes_ctr_cipher24.block_size = 16;
     aes_ctr_cipher24.key_len = 24;
     aes_ctr_cipher24.iv_len = 16;
     aes_ctr_cipher24.init = aes_ctr_init;
     aes_ctr_cipher24.do_cipher = aes_ctr_do_cipher;
     aes_ctr_cipher24.cleanup = aes_ctr_cleanup;

     memset(&aes_ctr_cipher32, 0, sizeof(aes_ctr_cipher32));
     aes_ctr_cipher32.block_size = 16;
     aes_ctr_cipher32.key_len = 32;
     aes_ctr_cipher32.iv_len = 16;
     aes_ctr_cipher32.init = aes_ctr_init;
     aes_ctr_cipher32.do_cipher = aes_ctr_do_cipher;
     aes_ctr_cipher32.cleanup = aes_ctr_cleanup;
 }
 }}}

 Then change the _libssh2_EVP_aes_XXX_ctr() functions to return the address
 of the appropriate structure:

 {{{
 #!c
 const EVP_CIPHER *
 _libssh2_EVP_aes_128_ctr(void)
 {
     return &aes_ctr_cipher16;
 }

 const EVP_CIPHER *
 _libssh2_EVP_aes_192_ctr(void)
 {
     return &aes_ctr_cipher24;
 }

 const EVP_CIPHER *
 _libssh2_EVP_aes_256_ctr(void)
 {
     return &aes_ctr_cipher32;
 }
 }}}

 Then, in openssl.h you can add the prototype for the init function:

 {{{
 #!c
 void _libssh2_init_EVP_aes_ctr(void);
 }}}

 Then in global.c add the following code so that if OpenSSL is used instead
 of libgcrypt the EVP_CIPHER structures get initialized:

 {{{
 #!c
 static void
 _libssh2_init_cipher_structures(void)
 {
 #ifndef LIBSSH2_LIBGCRYPT /* only need to initialize the cipher structures
 if we build with OpenSSL */
 #include "openssl.h"
  _libssh2_init_EVP_aes_ctr();
 #else
   /* No need to initialize any structures if using libgcrypt */
 #endif
 }
 }}}

 Finally in global.c add the call to _libssh2_init_cipher_structures() in
 the libssh2_init() function which should only be called once and therefore
 won't result in one thread changing the value of a structure member right
 before another thread dereferences that structure member:

 {{{
 #!c
 LIBSSH2_API int
 libssh2_init(int flags)
 {
     if (_libssh2_initialized == 0 && !(flags & LIBSSH2_INIT_NO_CRYPTO)) {
         libssh2_crypto_init();
         _libssh2_init_cipher_structures();
     }

     _libssh2_initialized++;
     _libssh2_init_flags |= flags;

     return 0;
 }
 }}}

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