random: the Windows RNG must report failure, not -1

configure.ac

@@ -260,14 +260,13 @@ AC_CHECK_DECLS([readpassphrase], [AC_DEFINE([HAVE_READPASSPHRASE], [1], [Define
 
 m4_include(m4/macros/with.m4)
 ARG_WITH_SET([random-device], [/dev/urandom], [set the device to read random data from])
-if test "x$random_device" = x"/dev/urandom"; then
-  AC_DEFINE_UNQUOTED([FILE_RANDOM],[1],[Define to 1 to enable random retrieving over filehandle])
-  AC_DEFINE([RANDOM_DEVICE],["/dev/urandom"],[Define to set random file handle])
-fi
-if test "x$random_device" = x"/dev/random"; then
-  AC_DEFINE_UNQUOTED([FILE_RANDOM],[1],[Define to 1 to enable /dev/random as random device])
-  AC_DEFINE([RANDOM_DEVICE],["/dev/random"],[Define to set random file handle])
-fi
+# Define RANDOM_DEVICE for whatever device was configured, not just for the two
+# values that used to be enumerated here. Matching on exact strings meant
+# --with-random-device=/dev/hwrng left RANDOM_DEVICE undefined, which is what
+# pushed src/random.c into hardcoding a path and ignoring this option entirely.
+AS_IF([test "x$random_device" = x],[random_device="/dev/urandom"])
+AC_DEFINE_UNQUOTED([FILE_RANDOM],[1],[Define to 1 to enable random retrieving over filehandle])
+AC_DEFINE_UNQUOTED([RANDOM_DEVICE],["$random_device"],[Device the POSIX entropy fallback reads from])
 
 # Configure Intel AVX2
 if test x$use_intel_avx2 = xyes; then

src/cli/tool.c

@@ -141,7 +141,15 @@ dogecoin_bool hd_gen_master(const dogecoin_chainparams* chain, char* masterkeyhe
 {
     dogecoin_hdnode node;
     uint8_t seed[32];
-    dogecoin_random_bytes(seed, 32, true);
+    /* Propagate an entropy failure instead of deriving a master key from
+       whatever is in seed[]. Every key in the resulting wallet descends from
+       these 32 bytes, so a silent failure here is unrecoverable and invisible:
+       the wallet works, and the keys are guessable. address.c already tests
+       this return value -- it just could never be false. */
+    if (!dogecoin_random_bytes(seed, 32, true)) {
+        dogecoin_mem_zero(seed, 32);
+        return false;
+    }
     dogecoin_hdnode_from_seed(seed, 32, &node);
     dogecoin_mem_zero(seed, 32);
     dogecoin_hdnode_serialize_private(&node, chain, masterkeyhex, strsize);

src/openenclave/host/host.c

@@ -412,7 +412,12 @@ int main(int argc, char* argv[])
                 printf("Shared secret not provided, generating one...\n");
                 // Generate random 20 bytes (40 hex characters)
                 unsigned char random_bytes[TOTP_SECRET_HEX_SIZE / 2];
-                dogecoin_random_bytes(random_bytes, sizeof(random_bytes), 0);
+                /* This becomes a TOTP shared secret; a silent entropy failure
+                   would yield a predictable second factor. */
+                if (!dogecoin_random_bytes(random_bytes, sizeof(random_bytes), 0)) {
+                    fprintf(stderr, "Failed to generate shared secret: no entropy available\n");
+                    goto exit;
+                }
 
                 shared_secret = malloc(TOTP_SECRET_HEX_SIZE + 1);
                 if (!shared_secret) {

src/optee/host/main.c

@@ -775,7 +775,12 @@ int main(int argc, const char* argv[])
                 printf("Shared secret not provided, generating one...\n");
                 // Generate random 20 bytes (40 hex characters)
                 unsigned char random_bytes[TOTP_SECRET_HEX_SIZE / 2];
-                dogecoin_random_bytes(random_bytes, sizeof(random_bytes), 0);
+                /* This becomes a TOTP shared secret; a silent entropy failure
+                   would yield a predictable second factor. */
+                if (!dogecoin_random_bytes(random_bytes, sizeof(random_bytes), 0)) {
+                    fprintf(stderr, "Failed to generate shared secret: no entropy available\n");
+                    goto exit;
+                }
 
                 shared_secret = malloc(TOTP_SECRET_HEX_SIZE + 1);
                 if (!shared_secret) {

src/random.c

@@ -27,6 +27,7 @@
 */
 
 #include <dogecoin/common.h>
+#include <dogecoin/mem.h>
 #include <dogecoin/random.h>
 
 #include <assert.h>
@@ -38,6 +39,15 @@
 #include <stdio.h>
 #include <string.h>
 #include <time.h>
+
+/* The device the POSIX fallback reads entropy from. Normally set by the build:
+   -DRANDOM_DEVICE=... from CMake, or libdogecoin-config.h under autotools.
+   The guard is a backstop for configurations that define neither -- it must
+   never be reached silently in a shipped build, which is why the value it
+   picks is the conservative one rather than a blocking device. */
+#ifndef RANDOM_DEVICE
+#define RANDOM_DEVICE "/dev/urandom"
+#endif
 #if defined _WIN32
 #ifdef _MSC_VER
 #include <win/winunistd.h>
@@ -185,7 +195,7 @@ dogecoin_bool dogecoin_random_bytes_internal(uint8_t* buf, uint32_t len, const u
     InitOnceExecuteOnce(&rng_init_once, rng_initialize_once, NULL, NULL);
     if (BCryptGenRandomFunc != NULL
         && BCryptGenRandomFunc(NULL, buf, len, BCRYPT_USE_SYSTEM_PREFERRED_RNG) == 0 /*STATUS_SUCCESS*/)
-        return 1;
+        return true;
     /* CryptGenRandom, defined in <wincrypt.h>
        <https://docs.microsoft.com/en-us/windows/win32/api/wincrypt/nf-wincrypt-cryptgenrandom>
        works in older releases as well, but is now deprecated.
@@ -194,16 +204,22 @@ dogecoin_bool dogecoin_random_bytes_internal(uint8_t* buf, uint32_t len, const u
     if (crypt_provider_ok) {
         if (!CryptGenRandom(crypt_provider, len, buf)) {
             errno = EIO;
-            return -1;
+            dogecoin_mem_zero(buf, len);
+            return false;
             }
-        return 1;
+        return true;
         }
 # else
     if (BCryptGenRandomFunc(NULL, buf, len, BCRYPT_USE_SYSTEM_PREFERRED_RNG) == 0 /*STATUS_SUCCESS*/)
-        return 1;
+        return true;
 # endif
+    /* No usable RNG at all. This must report failure, not -1: the return type
+       is dogecoin_bool, which is a uint8_t, so -1 arrives at the caller as 255
+       -- a true value. Every caller writing `if (!dogecoin_random_bytes(...))`
+       saw success while buf still held whatever was on the stack. */
     errno = ENOSYS;
-    return -1;
+    dogecoin_mem_zero(buf, len);
+    return false;
 #else
 #if USE_OPENENCLAVE || USE_OPTEE
     if (rng_ptr != NULL)
@@ -213,12 +229,23 @@ dogecoin_bool dogecoin_random_bytes_internal(uint8_t* buf, uint32_t len, const u
 #endif
 
     (void)update_seed; //unused
-    FILE* frand = fopen("/dev/urandom", "r"); // figure out why RANDOM_DEVICE is undeclared here
+    FILE* frand = fopen(RANDOM_DEVICE, "rb");
     if (!frand)
         return false;
     size_t len_read = fread(buf, 1, len, frand);
-    assert(len_read == len);
     fclose(frand);
+    /* A short read must fail the call, not just trip an assert: assert() is
+       compiled out under NDEBUG, which CMake's Release configuration sets by
+       default (-O3 -DNDEBUG). In that build a partial read left the tail of buf
+       holding whatever was already in that memory and still returned true, so a
+       caller would use uninitialised bytes as key material.
+       Zero the buffer as well as returning false, so that a caller which ignores
+       the return value gets an obviously-unusable all-zero key rather than
+       something that looks random enough to spend to. */
+    if (len_read != len) {
+        dogecoin_mem_zero(buf, len);
+        return false;
+    }
     return true;
 #endif
     }

test/random_tests.c

@@ -80,3 +80,69 @@ void test_random()
     // switch back to the default random callback mapper
     dogecoin_rnd_set_mapper_default();
 }
+
+
+/* --- regression: a failing RNG must report false, never a truthy value --- */
+
+static void rnd_noop_init(void) {}
+
+/* Fails correctly: reports false and leaves the buffer alone. */
+static dogecoin_bool rnd_fail_false(uint8_t* buf, uint32_t len, const uint8_t update_seed)
+{
+    (void)buf; (void)len; (void)update_seed;
+    return false;
+}
+
+/* Fails the way the WIN32 branch used to: `return -1` from a function whose
+   return type is dogecoin_bool. */
+static dogecoin_bool rnd_fail_minus_one(uint8_t* buf, uint32_t len, const uint8_t update_seed)
+{
+    (void)buf; (void)len; (void)update_seed;
+    return (dogecoin_bool)-1;
+}
+
+/*
+ * dogecoin_bool is a uint8_t, so `return -1` reaches the caller as 255 -- a
+ * true value. The WIN32 path did exactly that on both of its failure exits
+ * (CryptGenRandom failure, and no RNG provider at all), so every caller
+ * written as `if (!dogecoin_random_bytes(...))` saw success while buf still
+ * held whatever was on the stack.
+ *
+ * The second half of this test pins that hazard as an executable fact rather
+ * than a comment: if the convention or the underlying type ever changes, this
+ * is where it surfaces.
+ */
+void test_random_failure_is_false()
+{
+    dogecoin_rnd_mapper mapper;
+    uint8_t buf[32];
+    dogecoin_bool r;
+
+    /* A correct failure is exactly false, and callers can test it. */
+    mapper.dogecoin_random_init = rnd_noop_init;
+    mapper.dogecoin_random_bytes = rnd_fail_false;
+    dogecoin_rnd_set_mapper(mapper);
+
+    memset(buf, 0xAB, sizeof(buf));
+    r = dogecoin_random_bytes(buf, sizeof(buf), 0);
+    u_assert_int_eq((int)r, 0);
+    u_assert_true(!r);
+
+    /* The trap this change removes: -1 survives as 255 and is truthy, so the
+       caller's `if (!r)` never fires. */
+    mapper.dogecoin_random_bytes = rnd_fail_minus_one;
+    dogecoin_rnd_set_mapper(mapper);
+
+    r = dogecoin_random_bytes(buf, sizeof(buf), 0);
+    u_assert_int_eq((int)r, 255);
+    /* Spelled out rather than u_assert_true(r): that macro compares against 1,
+       and the whole point here is that 255 is not 1 yet is still true. */
+    u_assert_int_eq(r ? 1 : 0, 1);
+    u_assert_int_eq((int)(!r), 0);
+
+    dogecoin_rnd_set_mapper_default();
+
+    /* And the real RNG still succeeds and writes the buffer. */
+    memset(buf, 0, sizeof(buf));
+    u_assert_true(dogecoin_random_bytes(buf, sizeof(buf), 0));
+}

test/unittester.c

@@ -60,6 +60,7 @@ extern void test_memory();
 extern void test_moon();
 extern void test_op_return();
 extern void test_random();
+extern void test_random_failure_is_false();
 extern void test_rmd160();
 extern void test_scrypt();
 extern void test_serialize();
@@ -191,6 +192,7 @@ int main()
     u_run_test(test_moon);
     u_run_test(test_op_return);
     u_run_test(test_random);
+    u_run_test(test_random_failure_is_false);
     u_run_test(test_rmd160);
     u_run_test(test_scrypt);
     u_run_test(test_serialize);