Fix randapi UBSAN bug, signed T:min map to >= 0

The undefined behavior is unary negation of T:min of a signed type
attempting to get a positive value of the same signed type.

This commit adds a unit test that exposes the original bug and well as
a fix for it.

Signed-off-by: Mark Deric <jmark@openvpn.net>

openvpn/random/randapi.hpp

@@ -25,6 +25,7 @@
 
 #include <string>
 #include <cstdint>
+#include <type_traits>
 
 #include <openvpn/common/size.hpp>
 #include <openvpn/common/rc.hpp>
@@ -72,8 +73,12 @@ class RandomAPI : public RC<thread_unsafe_refcount>
     T rand_get_positive()
     {
         T ret = rand_get<T>();
-        if (ret < 0)
-            ret = -ret;
+        if constexpr (std::is_signed_v<T>)
+        {
+            // maps (T:min, -1) to (0, T:max) which is fine for random generation
+            ret &= std::numeric_limits<T>::max();
+        }
+
         return ret;
     }
 

test/unittests/CMakeLists.txt

@@ -56,6 +56,7 @@ add_executable(coreUnitTests
         test_path.cpp
         test_pktid.cpp
         test_prefixlen.cpp
+        test_randapi.cpp
         test_rc.cpp
         test_route.cpp
 	test_reliable.cpp

test/unittests/test_randapi.cpp

@@ -0,0 +1,77 @@
+#include <iostream>
+#include "test_common.h"
+
+#include <openvpn/random/randapi.hpp>
+
+using namespace openvpn;
+
+template <typename IntegralT>
+class IntegralMin : public RandomAPI
+{
+  public:
+    OPENVPN_EXCEPTION(s_min_error);
+
+    typedef RCPtr<IntegralMin> Ptr;
+
+    // Random algorithm name
+    std::string name() const override
+    {
+        return "IntegralMin";
+    }
+
+    // Return true if algorithm is crypto-strength
+    bool is_crypto() const override
+    {
+        return false;
+    }
+
+    // Fill buffer with minimum value
+    void rand_bytes(unsigned char *buf, size_t size) override
+    {
+        if (!rand_bytes_noexcept(buf, size))
+            throw s_min_error("rand_bytes failed");
+    }
+
+    // Like rand_bytes, but don't throw exception.
+    // Return true on successs, false on fail.
+    bool rand_bytes_noexcept(unsigned char *buf, size_t size) override
+    {
+        if (size < sizeof(IntegralT))
+            return false;
+        IntegralT *int_ptr = reinterpret_cast<IntegralT *>(buf);
+        *int_ptr = std::numeric_limits<IntegralT>::min();
+        return true;
+    }
+
+    IntegralT get_result()
+    {
+        return rand_get_positive<IntegralT>();
+    }
+};
+
+template <typename IntegralT>
+void randapi_signed_min_test(const std::string &test_name)
+{
+    IntegralMin<IntegralT> s_min;
+
+    IntegralT result = s_min.get_result();
+
+    EXPECT_EQ(result, 0) << "fails for \"" << test_name << "\" test";
+}
+
+#define RANDAPI_SIGNED_MIN_TEST(test)         \
+    do                                        \
+    {                                         \
+        randapi_signed_min_test<test>(#test); \
+    } while (0)
+
+
+TEST(misc, randapi_signed_min)
+{
+    RANDAPI_SIGNED_MIN_TEST(signed char);
+    RANDAPI_SIGNED_MIN_TEST(unsigned char);
+    RANDAPI_SIGNED_MIN_TEST(int32_t);
+    RANDAPI_SIGNED_MIN_TEST(uint32_t);
+    RANDAPI_SIGNED_MIN_TEST(int64_t);
+    RANDAPI_SIGNED_MIN_TEST(uint64_t);
+}