Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Misuse of libsodium::randombytes_close() #4634

Open
axelriet opened this issue Nov 29, 2023 · 3 comments
Open

Misuse of libsodium::randombytes_close() #4634

axelriet opened this issue Nov 29, 2023 · 3 comments

Comments

@axelriet
Copy link
Contributor

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
Copy link
Contributor

t-b commented Mar 21, 2024

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
Copy link
Contributor

t-b commented Mar 21, 2024

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

@axelriet
Copy link
Contributor Author

What needs to be counted are the calls to manage_random. I did it in my fork like this: axelriet@2700a87

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants