diff --git a/src/ctx.cpp b/src/ctx.cpp index 9dcdeca2..c96ab0f7 100644 --- a/src/ctx.cpp +++ b/src/ctx.cpp @@ -1,5 +1,5 @@ /* - Copyright (c) 2007-2016 Contributors as noted in the AUTHORS file + Copyright (c) 2007-2017 Contributors as noted in the AUTHORS file This file is part of libzmq, the ZeroMQ core engine in C++. @@ -45,12 +45,7 @@ #include "pipe.hpp" #include "err.hpp" #include "msg.hpp" - -#if defined (ZMQ_USE_TWEETNACL) -# include "tweetnacl.h" -#elif defined (ZMQ_USE_LIBSODIUM) -# include "sodium.h" -#endif +#include "random.hpp" #ifdef ZMQ_HAVE_VMCI #include @@ -91,15 +86,8 @@ zmq::ctx_t::ctx_t () : vmci_family = -1; #endif - scoped_lock_t locker(crypto_sync); -#if defined (ZMQ_USE_TWEETNACL) - // allow opening of /dev/urandom - unsigned char tmpbytes[4]; - randombytes(tmpbytes, 4); -#elif defined (ZMQ_USE_LIBSODIUM) - int rc = sodium_init (); - zmq_assert (rc != -1); -#endif + // Initialise crypto library, if needed. + zmq::random_open (); } bool zmq::ctx_t::check_tag () @@ -131,11 +119,8 @@ zmq::ctx_t::~ctx_t () // corresponding io_thread/socket objects. free (slots); - // If we've done any Curve encryption, we may have a file handle - // to /dev/urandom open that needs to be cleaned up. -#ifdef ZMQ_HAVE_CURVE - randombytes_close (); -#endif + // De-initialise crypto library, if needed. + zmq::random_close (); // Remove the tag, so that the object is considered dead. tag = ZMQ_CTX_TAG_VALUE_BAD; diff --git a/src/ctx.hpp b/src/ctx.hpp index 953b7941..934a444b 100644 --- a/src/ctx.hpp +++ b/src/ctx.hpp @@ -1,5 +1,5 @@ /* - Copyright (c) 2007-2016 Contributors as noted in the AUTHORS file + Copyright (c) 2007-2017 Contributors as noted in the AUTHORS file This file is part of libzmq, the ZeroMQ core engine in C++. @@ -233,8 +233,6 @@ namespace zmq int vmci_family; mutex_t vmci_sync; #endif - - mutex_t crypto_sync; }; } diff --git a/src/random.cpp b/src/random.cpp index 0180fd34..13e47129 100644 --- a/src/random.cpp +++ b/src/random.cpp @@ -1,5 +1,5 @@ /* - Copyright (c) 2007-2016 Contributors as noted in the AUTHORS file + Copyright (c) 2007-2017 Contributors as noted in the AUTHORS file This file is part of libzmq, the ZeroMQ core engine in C++. @@ -37,6 +37,14 @@ #include "random.hpp" #include "stdint.hpp" #include "clock.hpp" +#include "mutex.hpp" +#include "macros.hpp" + +#if defined (ZMQ_USE_TWEETNACL) +#include "tweetnacl.h" +#elif defined (ZMQ_USE_LIBSODIUM) +#include "sodium.h" +#endif void zmq::seed_random () { @@ -57,3 +65,54 @@ uint32_t zmq::generate_random () return high | low; } +// When different threads have their own context the file descriptor +// variable is shared and is subject to race conditions in tweetnacl, +// that lead to file descriptors leaks. In long-running programs with +// ephemeral threads this is a problem as it accumulates. +// thread-local storage cannot be used to initialise the file descriptor +// as it is perfectly legal to share a context among many threads, each +// of which might call curve APIs. +// Also libsodium documentation specifically states that sodium_init +// must not be called concurrently from multiple threads, for the +// same reason. Inspecting the code also reveals that the close API is +// not thread safe. +// The context class cannot be used with static variables as the curve +// utility APIs like zmq_curve_keypair also call into the crypto +// library. +// The safest solution for all use cases therefore is to have a global, +// static lock to serialize calls into an initialiser and a finaliser, +// using refcounts to make sure that a thread does not close the library +// while another is still using it. +static unsigned int random_refcount = 0; +static zmq::mutex_t random_sync; + +void zmq::random_open (void) +{ +#if defined (ZMQ_USE_LIBSODIUM) || \ + (defined (ZMQ_USE_TWEETNACL) && !defined (ZMQ_HAVE_WINDOWS)) + scoped_lock_t locker (random_sync); + + if (random_refcount == 0) { + int rc = sodium_init (); + zmq_assert (rc != -1); + } + + ++random_refcount; +#else + LIBZMQ_UNUSED (random_refcount); +#endif +} + +void zmq::random_close (void) +{ +#if defined (ZMQ_USE_LIBSODIUM) || \ + (defined (ZMQ_USE_TWEETNACL) && !defined (ZMQ_HAVE_WINDOWS)) + scoped_lock_t locker (random_sync); + --random_refcount; + + if (random_refcount == 0) { + randombytes_close (); + } +#endif +} + diff --git a/src/random.hpp b/src/random.hpp index 023ddf2d..11d9cedb 100644 --- a/src/random.hpp +++ b/src/random.hpp @@ -1,5 +1,5 @@ /* - Copyright (c) 2007-2016 Contributors as noted in the AUTHORS file + Copyright (c) 2007-2017 Contributors as noted in the AUTHORS file This file is part of libzmq, the ZeroMQ core engine in C++. @@ -41,6 +41,12 @@ namespace zmq // Generates random value. uint32_t generate_random (); + // [De-]Initialise crypto library, if needed. + // Serialised and refcounted, so that it can be called + // from multiple threads, each with its own context, and from + // the various zmq_utils curve functions safely. + void random_open (); + void random_close (); } #endif diff --git a/src/tweetnacl.c b/src/tweetnacl.c index 14e6803b..8ade6a63 100644 --- a/src/tweetnacl.c +++ b/src/tweetnacl.c @@ -1,5 +1,5 @@ /* - Copyright (c) 2016 Contributors as noted in the AUTHORS file + Copyright (c) 2016-2017 Contributors as noted in the AUTHORS file This file is part of libzmq, the ZeroMQ core engine in C++. @@ -898,26 +898,27 @@ int randombytes_close(void) return rc; } +int sodium_init (void) +{ + return 0; +} + #else #include #include #include #include +#include static int fd = -1; void randombytes (unsigned char *x,unsigned long long xlen) { int i; - if (fd == -1) { - for (;;) { - fd = open("/dev/urandom",O_RDONLY); - if (fd != -1) - break; - sleep (1); - } - } + // Require that random_open has already been called, to avoid + // race conditions. + assert (fd != -1); while (xlen > 0) { if (xlen < 1048576) i = xlen; @@ -934,6 +935,7 @@ void randombytes (unsigned char *x,unsigned long long xlen) } } +// Do not call manually! Use random_close from random.hpp int randombytes_close (void) { int rc = -1; @@ -944,6 +946,20 @@ int randombytes_close (void) return rc; } +// Do not call manually! Use random_open from random.hpp +int sodium_init (void) +{ + if (fd == -1) { + for (;;) { + fd = open("/dev/urandom",O_RDONLY); + if (fd != -1) + break; + sleep (1); + } + } + return 0; +} + #endif #endif diff --git a/src/tweetnacl.h b/src/tweetnacl.h index eedcc999..95f8b2a5 100644 --- a/src/tweetnacl.h +++ b/src/tweetnacl.h @@ -1,5 +1,5 @@ /* - Copyright (c) 2016 Contributors as noted in the AUTHORS file + Copyright (c) 2016-2017 Contributors as noted in the AUTHORS file This file is part of libzmq, the ZeroMQ core engine in C++. @@ -52,7 +52,10 @@ typedef i64 gf[16]; extern "C" { #endif void randombytes (unsigned char *, unsigned long long); +// Do not call manually! Use random_close from random.hpp int randombytes_close (void); +// Do not call manually! Use random_open from random.hpp +int sodium_init (void); int crypto_box_keypair(u8 *y,u8 *x); int crypto_box_afternm(u8 *c,const u8 *m,u64 d,const u8 *n,const u8 *k); diff --git a/src/zmq_utils.cpp b/src/zmq_utils.cpp index 47b09e37..f3eec0e2 100644 --- a/src/zmq_utils.cpp +++ b/src/zmq_utils.cpp @@ -1,5 +1,5 @@ /* - Copyright (c) 2007-2016 Contributors as noted in the AUTHORS file + Copyright (c) 2007-2017 Contributors as noted in the AUTHORS file This file is part of libzmq, the ZeroMQ core engine in C++. @@ -35,6 +35,7 @@ #include "thread.hpp" #include "atomic_counter.hpp" #include "atomic_ptr.hpp" +#include "random.hpp" #include #include #include @@ -217,10 +218,14 @@ int zmq_curve_keypair (char *z85_public_key, char *z85_secret_key) uint8_t public_key [32]; uint8_t secret_key [32]; + zmq::random_open (); + int res = crypto_box_keypair (public_key, secret_key); zmq_z85_encode (z85_public_key, public_key, 32); zmq_z85_encode (z85_secret_key, secret_key, 32); + zmq::random_close (); + return res; #else (void) z85_public_key, (void) z85_secret_key; @@ -246,6 +251,8 @@ int zmq_curve_public (char *z85_public_key, const char *z85_secret_key) uint8_t public_key[32]; uint8_t secret_key[32]; + zmq::random_open (); + if (zmq_z85_decode (secret_key, z85_secret_key) == NULL) return -1; @@ -253,6 +260,8 @@ int zmq_curve_public (char *z85_public_key, const char *z85_secret_key) crypto_scalarmult_base (public_key, secret_key); zmq_z85_encode (z85_public_key, public_key, 32); + zmq::random_close (); + return 0; #else (void) z85_public_key, (void) z85_secret_key;