Problem: calling randombytes_close with libsodium can crash Contexts in other threads

[minrk]

Problem: When using libsodium, calling zmq::random_close() closes a shared resources that may still be in use.

See #4241 for details and repro, but this is called unconditionally on every curve_keypair() and every context destructor, since #3001. Closing any context and/or issuing a new curve keypair can crash another context if it is working with curve-enabled sockets at the time, as it is still a consumer of a now-closed resource. libsodium will try to reinitialize the source, so it doesn't crash every time; instead, certain race conditions must be met to result in a crash.

Solution: Do not close this shared resource while it is still in use, as suggested by the libsodium documentation.

For implementations using /dev/[u]random, this can leave up to one leftover FD per process (restores #2632, but should be bounded to one).

If we want to call randombytes_close(), we must restore the use of refcounting in #2636 when using libsodium to avoid crashes.

closes #4241

[bluca]

Unfortunately this is not a solution, because then we go back to leaking file descriptors, which trigger a cascade of other problems (selinux errors, etc etc)

[minrk]

OK, then the fix is to revert #3001

[bluca]

Eh I wish - that breaks a whole other host of cases due to C++ broken global initialization mechanisms...

[minrk]

I don't fully understand how the refcounting broke things, but to be correct, libsodium must have a refcounter to protect the call to randombytes_close(), just like tweetnacl. I don't know how to make it properly global to avoid #2991, but this means refcounting is clearly required with libsodium, unfortunately.

As it is now, one must ensure that once a context is active dealing with curve messages, no other contexts can be terminated and no new keypairs issued.

[bluca]

It's explained in the comments - it's not possible to do refcounting via globals in C++. The only solution is to get a kernel with getrandom(), everything else fixes one things and breaks another, which is not acceptable, as we really need to keep things stable.

[minrk]

The only solution is to get a kernel with getrandom()

That's not something folks usually have control over, so not really a solution.

everything else fixes one things and breaks another, which is not acceptable, as we really need to keep things stable.

Yeah, I get that. Even fixing wrong behavior can break things.

WDYT about a runtime and/or compile-time opt-out of the random_close? When getrandom is available, removing it changes nothing. When it's not, you trade a crash for a (bounded to one process-wide) leak. If the leak were opt-in, it seems reasonable to expect the calling code to manage closing the shared resource, in the rare cases where that's useful.

The 'wrong' behavior of the leak is easy to workaround (close the FD) and of little consequence (leak up to one FD in the lifetime of the process), whereas the current behavior is not possible to workaround except with "get a different kernel".

[bluca]

The only solution is to get a kernel with getrandom()

That's not something folks usually have control over, so not really a solution.

everything else fixes one things and breaks another, which is not acceptable, as we really need to keep things stable.

Yeah, I get that. Even fixing wrong behavior can break things.

WDYT about a runtime and/or compile-time opt-out of the random_close? When getrandom is available, removing it changes nothing. When it's not, you trade a crash for a (bounded to one process-wide) leak. If the leak were opt-in, it seems reasonable to expect the calling code to manage closing the shared resource, in the rare cases where that's useful.

The 'wrong' behavior of the leak is easy to workaround (close the FD) and of little consequence (leak up to one FD in the lifetime of the process), whereas the current behavior is not possible to workaround except with "get a different kernel".

A build option would be fine. Note that the leak is not just cosmetic, things like selinux will raise an audit and so on.

[minrk]

I added an opt-in to cmake and autotools (my m4 is rusty, so I hope it's right). As you might guess, I would prefer the non-crashing behavior be the default, but I assume no change by default is the right choice for consistency.

I wonder if a runtime zmq_random_open() / zmq_random_close() that allowed applications to call sodium_init / randombytes_close() and take it out of the hands of zmq::random_open/close that have refcounting troubles would be best?

things like selinux will raise an audit and so on.

Do you have a link to more information on that? If I have application code that does:

zmq_term() // doesn't call randombytes_close() anymore
randombytes_close()

would that avoid the audit issue, or no? I don't have experience with selinux, so don't know exactly what's being audited there.

[bluca]

If you want to list the explanation, please be precise "not threadsafe without getrandom()"

[minrk]

Do you think it's better to restore the refcounting instead of disabling entirely (i.e. exactly reverting #3001 if the new flag is set)?

[bluca]

Do you think it's better to restore the refcounting instead o disabling entirely (i.e. exactly reverting #3001 if the new flag is set)?

Current patch is best - the workaround to call randombytes_close manually is trivial, while safely dealing with the global init shenanigans is extremely convoluted, if at all possible