Commit e015a0f8 authored by Luca Boccassi's avatar Luca Boccassi

Problem: fd leak in tweetnacl with one ctx per thread

Solution: add a crypto [de-]initialiser, refcounted and serialised
through critical sections.
This is necessary as utility APIs such as zmq_curve_keypair also
call into the sodium/tweetnacl libraries and need the initialisation
outside of the zmq context.
Also the libsodium documentation explicitly says that sodium_init
must not be called concurrently from multiple threads, which could
have happened until now. Also the randombytes_close function does
not appear to be thread safe either.
This change guarantees that the library is initialised only once at
any given time across the whole program.
Fixes #2632
parent a7bf010e
/*
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;
......
/*
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;
};
}
......
/*
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
}
/*
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
/*
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 <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
#include <assert.h>
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
/*
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);
......
/*
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;
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment