Re: libssh multi-threading bug in crypto initialization

[Aris Adamantiadis <aris@xxxxxxxxxxxx>]

Hi Mark,

Thanks for your report. However I wonder if your patch does not conflict
with this :
http://api.libssh.org/master/libssh_tutor_threads.html
We already have a cross-platform and less intrusive way of providing
synchronizations primitives to openssl and gcrypt.
Is there something that your patch fixes that couldn't be fixed with our
threading callbacks ?

(sorry for late reaction)

Thanks,

Aris

Le 21/09/12 00:55, Mark Riordan a écrit :
> Hello.  I recently fixed a bug in libssh's initialization of the OpenSSL
> Cryptographic module that caused crashes in my multithreaded application.
> The fix was for libssh 0.4.8, but I see that the bug appears in 0.5.2, and
> the fix is much the same.
> 
> Failing to get red.libssh.org to work, I'm attaching a zip file containing a
> couple of new source code files.  
> You'll want to add these files to CMakeLists.txt. 
> Also, dh.c needs to be changed to add a call to the new function provided by
> critsec.c.  I haven't patched 0.5.2, but the 0.4.8 patch looks like:
> 
> @@ -55,6 +55,7 @@
>  #include "libssh/session.h"
>  #include "libssh/keys.h"
>  #include "libssh/dh.h"
> +#include "libssh/critsec.h"
> 
>  /* todo: remove it */
>  #include "libssh/string.h"
> @@ -134,6 +135,7 @@
>        return -1;
>      }
>  #elif defined HAVE_LIBCRYPTO
> +       libssh_init_critsec_crypto();
>      p = bignum_new();
>      if (p == NULL) {
>        bignum_free(g);
> 
> The problem is that libcrypto maintains a pool of pseudorandom numbers
> intended to be used by all threads.  Appropriate locking is done in
> libcrypto,
> BUT this locking doesn't work at all unless you provide some non-trivial
> locking initialization in the code that uses libcrypto.  
> If you fail to provide the extra initialization, the resulting lack of
> locking in libcrypto may result in two indices being out-of-sync, resulting
> in -N rather than N being passed as a byte count in a call to memcpy.
> N is typically rather small--for instance, 12.  Doing a memcpy of -12 bytes
> (which looks like over 4 billion when viewed as an unsigned integer)
> is very bad for the health of libssh.
> 
> Regards,
> 
> Mark R
>