From bcb659e00eadd5f657ba0ac324b35ba75d99c15a Mon Sep 17 00:00:00 2001
From: Min RK <benjaminrk@gmail.com>
Date: Fri, 13 Aug 2021 16:11:29 +0200
Subject: [PATCH] Problem: calling randombytes_close with libsodium can crash
 Contexts in other threads (#4242)

* add opt-out for randombytes_close

Problem: randombytes_close is either a no-op or unsafe when a Context is running.

Unfortunately, there does not appear to be a single always correct choice,
so let builders pick between two not-great options.

Opting out can leak an FD on /dev/urandom which may need to be closed explicitly.
However, with the default behavior,
using multiple contexts with CURVE can crash with no application-level workaround available.

randombytes_close is not threadsafe and calling it while still in use by a Context can cause a crash.

For implementations using /dev/[u]random, this can leave up to one leftover FD per process.

The libsodium docs suggest that this function rarely needs to be called explicitly.
---
 CMakeLists.txt |  4 ++++
 configure.ac   | 20 ++++++++++++++++++++
 src/random.cpp |  5 +++++
 3 files changed, 29 insertions(+)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index da975d25..788b20d8 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -258,6 +258,7 @@ endif()
 
 option(WITH_LIBSODIUM "Use libsodium instead of built-in tweetnacl" ON)
 option(WITH_LIBSODIUM_STATIC "Use static libsodium library" OFF)
+option(ENABLE_LIBSODIUM_RANDOMBYTES_CLOSE "Automatically close libsodium randombytes. Not threadsafe without getrandom()" ON)
 option(ENABLE_CURVE "Enable CURVE security" ON)
 
 if(ENABLE_CURVE)
@@ -271,6 +272,9 @@ if(ENABLE_CURVE)
       endif()
       set(ZMQ_USE_LIBSODIUM 1)
       set(ZMQ_HAVE_CURVE 1)
+      if (ENABLE_LIBSODIUM_RANDOMBYTES_CLOSE)
+        set(ZMQ_LIBSODIUM_RANDOMBYTES_CLOSE 1)
+      endif()
     else()
       message(
         WARNING
diff --git a/configure.ac b/configure.ac
index 7c798e86..6af4cadc 100644
--- a/configure.ac
+++ b/configure.ac
@@ -550,6 +550,20 @@ AS_IF([test "x$with_libsodium" = "xyes"], [
 AC_ARG_ENABLE([curve],
     [AS_HELP_STRING([--disable-curve], [disable CURVE security [default=no]])])
 
+AC_ARG_ENABLE(
+    [libsodium_randombytes_close],
+    [AS_HELP_STRING(
+        [--disable-libsodium_randombytes_close],
+        [Do not call libsodium randombytes_close() when terminating contexts.
+        If disabled, may leave one FD open on /dev/urandom
+        until randombytes_close() is called explicitly,
+        but fixes a crash when multiple contexts are used with CURVE.
+        Has no effect when getrandom() is available. [default=enabled]]
+    )],
+    [],
+    [enable_libsodium_randombytes_close=yes]
+)
+
 if test "x$enable_curve" = "xno"; then
     curve_library=""
     AC_MSG_NOTICE([CURVE security is disabled])
@@ -558,6 +572,12 @@ elif test "x$with_libsodium" = "xyes"; then
     AC_MSG_NOTICE([Using libsodium for CURVE security])
     AC_DEFINE(ZMQ_HAVE_CURVE, [1], [Using curve encryption])
     AC_DEFINE(ZMQ_USE_LIBSODIUM, [1], [Using libsodium for curve encryption])
+    if test "x$enable_libsodium_randombytes_close" = "xyes"; then
+        AC_DEFINE(ZMQ_LIBSODIUM_RANDOMBYTES_CLOSE, [1], [Automatically close libsodium randombytes. Not threadsafe without getrandom()])
+    else
+        AC_MSG_NOTICE([Disabling libsodium randombytes_close(). randombytes_close() may need to be called in applcation code.])
+    fi
+
     curve_library="libsodium"
     enable_curve="yes"
 
diff --git a/src/random.cpp b/src/random.cpp
index 17c3537d..4700aa58 100644
--- a/src/random.cpp
+++ b/src/random.cpp
@@ -151,8 +151,13 @@ static void manage_random (bool init_)
     if (init_) {
         int rc = sodium_init ();
         zmq_assert (rc != -1);
+#if defined(ZMQ_LIBSODIUM_RANDOMBYTES_CLOSE)
     } else {
+        // randombytes_close either a no-op or not threadsafe
+        // doing this without refcounting can cause crashes
+        // if called while a context is active
         randombytes_close ();
+#endif
     }
 #else
     LIBZMQ_UNUSED (init_);