zeromq / libzmq

ZeroMQ core engine in C++, implements ZMTP/3.1
https://www.zeromq.org
Mozilla Public License 2.0
9.46k stars 2.34k forks source link

Misuse of libsodium::randombytes_close() #4634

Open axelriet opened 7 months ago

axelriet commented 7 months ago

When built with ENABLE_LIBSODIUM_RANDOMBYTES_CLOSE zmq::random_close() calls zmq::manage_random(false) which in turn calls randombytes_close() which deinitializes the RNG in the crypto library.

zmq::random_close() is called from zmq::ctx_t::~ctx_t() as well as from zmq::zmq_curve_keypair() and zmq_curve_public(), however nothing says that randombytes_close() is idempotent, and calling either of zmq::zmq_curve_keypair() and zmq_curve_public() or destroying a (secondary) context - for example - will happily deinitialize the RNG multiple times and regardless of the existence of an active context, which might keep using it.

The calls to sodium_init() and randombytes_close() must be protected by a refcount, and the former must only called once on the first invocation of zmq::manage_random(true), while the latter must only be called once on the last invocation of zmq::manage_random(false).

t-b commented 3 months ago

I don't think sodium_init in the latest version needs a refcount, the documentation states in 1:

sodium_init() initializes the library and should be called before any other function provided by Sodium. It is safe to call this function more than once and from different threads – subsequent calls won’t have any effects.

But the use of randombytes_close is wrong in multiple ways:

  1. The documentation of zmq::random_open/random_close states:
//  [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 ();

which is not the case when looking at the implementation.

  1. The documentation of randombytes_close says [2]:

Explicitly calling this function is almost never required.

  1. The documentation of randombytes_stir states

The randombytes_stir() function reseeds the pseudo-random number generator if it supports this operation. Calling this function is not required with the default generator, even after a fork() call, unless the descriptor for /dev/urandom was closed using randombytes_close().

So calling randombytes_close actively harms when forking.

ZMQ_LIBSODIUM_RANDOMBYTES_CLOSE is enabled by default in configure.ac/CMake when available.

t-b commented 3 months ago

This is a recent regression: ff47aeb7 (Problem: no permission to relicense tweetnacl integration, 2023-02-18)

axelriet commented 3 months ago

What needs to be counted are the calls to manage_random. I did it in my fork like this: https://github.com/axelriet/libzmq/commit/2700a87adf227ec9d54720fa9caa22f76692a752