Merge pull request #2636 from bluca/tweetnacl_fd

Problem: tweetnacl on *nix use of /dev/urandom is not thread safe

CMakeLists.txt

@@ -323,6 +323,7 @@ endif ()
 #-----------------------------------------------------------------------------
 if (NOT CMAKE_CROSSCOMPILING)
   zmq_check_sock_cloexec ()
+  zmq_check_o_cloexec ()
   zmq_check_so_keepalive ()
   zmq_check_tcp_keepcnt ()
   zmq_check_tcp_keepidle ()
@@ -330,6 +331,7 @@ if (NOT CMAKE_CROSSCOMPILING)
   zmq_check_tcp_keepalive ()
   zmq_check_tcp_tipc ()
   zmq_check_pthread_setname ()
+  zmq_check_getrandom ()
 endif ()
 
 if (    CMAKE_SYSTEM_NAME MATCHES "Linux"

acinclude.m4

@@ -614,6 +614,31 @@ int main (int argc, char *argv [])
     AS_IF([test "x$libzmq_cv_sock_cloexec" = "xyes"], [$1], [$2])
 }])
 
+dnl ################################################################################
+dnl # LIBZMQ_CHECK_O_CLOEXEC([action-if-found], [action-if-not-found])          #
+dnl # Check if O_CLOEXEC is supported                                           #
+dnl ################################################################################
+AC_DEFUN([LIBZMQ_CHECK_O_CLOEXEC], [{
+    AC_CACHE_CHECK([whether O_CLOEXEC is supported], [libzmq_cv_o_cloexec],
+        [AC_TRY_RUN([/* O_CLOEXEC test */
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+
+int main (int argc, char *argv [])
+{
+    int s = open ("/dev/null", O_CLOEXEC | O_RDONLY);
+    return (s == -1);
+}
+        ],
+        [libzmq_cv_o_cloexec="yes"],
+        [libzmq_cv_o_cloexec="no"],
+        [libzmq_cv_o_cloexec="not during cross-compile"]
+        )]
+    )
+    AS_IF([test "x$libzmq_cv_o_cloexec" = "xyes"], [$1], [$2])
+}])
+
 dnl ################################################################################
 dnl # LIBZMQ_CHECK_EVENTFD_CLOEXEC([action-if-found], [action-if-not-found])          #
 dnl # Check if EFD_CLOEXEC is supported                                           #
@@ -804,6 +829,29 @@ int main (int argc, char *argv [])
     AS_IF([test "x$libzmq_cv_tcp_keepalive" = "xyes"], [$1], [$2])
 }])
 
+dnl ################################################################################
+dnl # LIBZMQ_CHECK_GETRANDOM([action-if-found], [action-if-not-found])  #
+dnl # Checks if getrandom is supported                                  #
+dnl ################################################################################
+AC_DEFUN([LIBZMQ_CHECK_GETRANDOM], [{
+    AC_CACHE_CHECK([whether getrandom is supported], [libzmq_cv_getrandom],
+        [AC_TRY_RUN([/* thread-local storage test */
+#include <sys/random.h>
+
+int main (int argc, char *argv [])
+{
+    char buf[4];
+    getrandom(buf, 4, 0);
+}
+        ],
+        [libzmq_cv_getrandom="yes"],
+        [libzmq_cv_getrandom="no"],
+        [libzmq_cv_getrandom="not during cross-compile"]
+        )]
+    )
+    AS_IF([test "x$libzmq_cv_getrandom" = "xyes"], [$1], [$2])
+}])
+
 dnl ################################################################################
 dnl # LIBZMQ_CHECK_POLLER_KQUEUE([action-if-found], [action-if-not-found])         #
 dnl # Checks kqueue polling system                                                 #

builds/cmake/Modules/ZMQSourceRunChecks.cmake

@@ -31,6 +31,23 @@ int main(int argc, char *argv [])
     ZMQ_HAVE_EVENTFD_CLOEXEC)
 endmacro()
 
+macro(zmq_check_o_cloexec)
+  message(STATUS "Checking whether O_CLOEXEC is supported")
+  check_c_source_runs(
+    "
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+
+int main(int argc, char *argv [])
+{
+    int s = open ("/dev/null", O_CLOEXEC | O_RDONLY);
+    return (s == -1);
+}
+"
+    ZMQ_HAVE_O_CLOEXEC)
+endmacro()
+
 # TCP keep-alives Checks.
 
 macro(zmq_check_so_keepalive)
@@ -228,3 +245,19 @@ int main(int argc, char *argv [])
     ZMQ_HAVE_PTHREAD_SET_NAME)
   set(CMAKE_REQUIRED_FLAGS ${SAVE_CMAKE_REQUIRED_FLAGS})
 endmacro()
+
+
+macro(zmq_check_getrandom)
+  message(STATUS "Checking whether getrandom is supported")
+  check_c_source_runs(
+    "
+#include <sys/random.h>
+
+int main (int argc, char *argv [])
+{
+    char buf[4];
+    getrandom(buf, 4, 0);
+}
+"
+    ZMQ_HAVE_GETRANDOM)
+endmacro()

configure.ac

@@ -662,6 +662,12 @@ LIBZMQ_CHECK_SOCK_CLOEXEC([
         [Whether SOCK_CLOEXEC is defined and functioning.])
     ])
 
+LIBZMQ_CHECK_O_CLOEXEC([
+    AC_DEFINE([ZMQ_HAVE_O_CLOEXEC],
+        [1],
+        [Whether O_CLOEXEC is defined and functioning.])
+    ])
+
 # TCP keep-alives Checks.
 LIBZMQ_CHECK_SO_KEEPALIVE([
     AC_DEFINE([ZMQ_HAVE_SO_KEEPALIVE],
@@ -693,6 +699,12 @@ LIBZMQ_CHECK_TCP_KEEPALIVE([
         [Whether TCP_KEEPALIVE is supported.])
     ])
 
+LIBZMQ_CHECK_GETRANDOM([
+    AC_DEFINE([ZMQ_HAVE_GETRANDOM],
+        [1],
+        [Whether getrandom is supported.])
+    ])
+
 AM_CONDITIONAL(HAVE_FORK, test "x$ac_cv_func_fork" = "xyes")
 
 if test "x$cross_compiling" = "xyes"; then

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 <vmci_sockets.h>
@@ -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;

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;
     };
 
 }

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) && !defined (ZMQ_HAVE_GETRANDOM))
+    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) && !defined (ZMQ_HAVE_GETRANDOM))
+    scoped_lock_t locker (random_sync);
+    --random_refcount;
+
+    if (random_refcount == 0) {
+        randombytes_close ();
+    }
+#endif
+}
+

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

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,32 +898,45 @@ int randombytes_close(void)
     return rc;
 }
 
+int sodium_init (void)
+{
+    return 0;
+}
+
 #else
 
+#include <unistd.h>
+#include <assert.h>
+
+#ifdef ZMQ_HAVE_GETRANDOM
+#include <sys/random.h>
+#else
 #include <sys/types.h>
 #include <sys/stat.h>
 #include <fcntl.h>
-#include <unistd.h>
 
 static int fd = -1;
+#endif
 
 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);
-        }
-    }
+#ifndef ZMQ_HAVE_GETRANDOM
+    //  Require that random_open has already been called, to avoid
+    //  race conditions.
+    assert (fd != -1);
+#endif
     while (xlen > 0) {
         if (xlen < 1048576)
             i = xlen;
         else
             i = 1048576;
 
+#ifdef ZMQ_HAVE_GETRANDOM
+        i = getrandom (x, i);
+#else
         i = read(fd,x,i);
+#endif
         if (i < 1) {
             sleep (1);
             continue;
@@ -933,16 +946,43 @@ 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;
+#ifndef ZMQ_HAVE_GETRANDOM
     if (fd != -1 && close(fd) == 0) {
         fd = -1;
         rc = 0;
     }
+#endif // ZMQ_HAVE_GETRANDOM
     return rc;
 }
 
+//  Do not call manually! Use random_open from random.hpp
+int sodium_init (void)
+{
+#ifndef ZMQ_HAVE_GETRANDOM
+    if (fd == -1) {
+        for (;;) {
+            int flags = O_RDONLY;
+#ifdef ZMQ_HAVE_O_CLOEXEC
+            flags |= O_CLOEXEC;
+#endif
+            fd = open ("/dev/urandom", flags);
+            if (fd != -1)
+                break;
+            sleep (1);
+        }
+#if !defined ZMQ_HAVE_O_CLOEXEC && defined FD_CLOEXEC
+        int rc = fcntl (fd, F_SETFD, FD_CLOEXEC);
+        assert (rc != -1);
+#endif
+    }
+#endif // ZMQ_HAVE_GETRANDOM
+    return 0;
+}
+
 #endif
 
 #endif

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);

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 <assert.h>
 #include <new>
 #include <stdint.h>
@@ -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;