From b7891cf74762e1522d033b3b8bac0387b1a41340 Mon Sep 17 00:00:00 2001 From: bluezr Date: Sat, 1 Aug 2026 10:25:00 -0700 Subject: [PATCH 1/4] random: fail closed on a short read from /dev/urandom (CWE-330) dogecoin_random_bytes_internal() read the requested length with fread() and checked the result with size_t len_read = fread(buf, 1, len, frand); assert(len_read == len); fclose(frand); return true; assert() is removed by NDEBUG, and CMake's Release configuration defines it by default -- CMAKE_C_FLAGS_RELEASE is "-O3 -DNDEBUG", confirmed reaching the compile line for this file, and CMakeLists.txt selects Release whenever the source tree is not a git checkout, which is the normal case for anyone building from a tarball. So in a release build a short read left the tail of buf holding whatever was already in that memory and returned true anyway. Callers use this for seeds, private keys and nonces, so the result is key material that is partly uninitialised while looking entirely successful to the caller. Reduced to a standalone reproduction of the same code path with a deliberately short read: debug (assert live) : Aborted, exit 134 release (-DNDEBUG) : returned=1 bytes_actually_from_rng=8/32 tail_uninitialised=24 Check the length explicitly and return false. Also zero the buffer on failure, so that a caller which ignores the return value is handed an obviously unusable all-zero key rather than something random enough to look spendable. fopen mode is "rb" rather than "r" for correctness on platforms where the distinction is real. Verification is by the reproduction above and by inspection: forcing a genuine short read from /dev/urandom in-tree would need fault injection around fread(), which the current structure does not allow without restructuring code that should not be churned for testability alone. Not reachable through dogecoin_rnd_set_mapper() overrides, and unrelated to any feature branch -- this is the default RNG on every POSIX build. 78/78. --- src/random.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/src/random.c b/src/random.c index a17e9ea12..5e279c5dc 100644 --- a/src/random.c +++ b/src/random.c @@ -27,6 +27,7 @@ */ #include +#include #include #include @@ -213,12 +214,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("/dev/urandom", "rb"); // figure out why RANDOM_DEVICE is undeclared here 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 } From c3f3e2db867794cd3f2b8b432ae14788e836b85b Mon Sep 17 00:00:00 2001 From: bluezr Date: Sat, 1 Aug 2026 10:31:19 -0700 Subject: [PATCH 2/4] tool, enclave hosts: propagate entropy failures to the caller Audit of every dogecoin_random_bytes() call site that produces key material. Most already fail closed -- dogecoin_privkey_gen (key.c), mnemonic entropy (bip39.c), wallet seed creation (wallet.c) and the ecc blinding seed all check and return. Three did not. hd_gen_master() is the worst of them: it is public API, it generates the 32-byte seed every key in an HD wallet descends from, and it discarded the return value and then returned true unconditionally. address.c already writes if (!hd_gen_master(chain, hd_privkey_master_local, sizeof(...))) so the caller was checking a value that could never be false. A wallet built after an entropy failure would look completely healthy while every address in it was derivable by anyone. This is the failure mode that is invisible to testing: keys derive, addresses format, transactions sign and confirm. Only the entropy source shows it. The openenclave and optee hosts both generate a TOTP shared secret the same way. A silent failure there is a predictable second factor, so both now report and bail instead of continuing. src/bench.c is left alone deliberately: it is a benchmark, its key is never used for funds, and its retry loop would reject a degenerate value anyway. Noting it rather than changing a signature for no security gain. 78/78. --- src/cli/tool.c | 10 +++++++++- src/openenclave/host/host.c | 7 ++++++- src/optee/host/main.c | 7 ++++++- 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/src/cli/tool.c b/src/cli/tool.c index a8ad4319e..0472b98ad 100644 --- a/src/cli/tool.c +++ b/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); diff --git a/src/openenclave/host/host.c b/src/openenclave/host/host.c index 78d0bffc4..f3f05ada3 100644 --- a/src/openenclave/host/host.c +++ b/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) { diff --git a/src/optee/host/main.c b/src/optee/host/main.c index 53653ee29..a0a093126 100644 --- a/src/optee/host/main.c +++ b/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) { From a694580fdeb06c8394531b424ebf2e9e6b6eb82f Mon Sep 17 00:00:00 2001 From: bluezr Date: Sun, 2 Aug 2026 14:19:41 -0700 Subject: [PATCH 3/4] random: honour --with-random-device instead of hardcoding the path src/random.c read "/dev/urandom" as a literal, with the comment "figure out why RANDOM_DEVICE is undeclared here". Chasing that comment turned up a real bug rather than a stale note. RANDOM_DEVICE resolves fine under CMake, which passes -DRANDOM_DEVICE="/dev/urandom" via ADD_DEFINITIONS to every target. Under autotools it was defined only when the configured value matched one of two hardcoded strings: if test "x$random_device" = x"/dev/urandom"; then AC_DEFINE(...) if test "x$random_device" = x"/dev/random"; then AC_DEFINE(...) so --with-random-device=/dev/hwrng left RANDOM_DEVICE undefined and any use of it failed to compile. Hardcoding the path made that build work, at the cost of silently ignoring the option: a user who configured --with-random-device=/dev/random still got /dev/urandom, with no warning. An operator deliberately choosing a blocking entropy source did not get one. configure.ac now defines RANDOM_DEVICE from whatever value was configured, so any device works. src/random.c uses the macro and carries an #ifndef fallback to /dev/urandom so no configuration fails to compile; the fallback picks the conservative value rather than a blocking device, since reaching it means the build did not say. Verified rather than assumed: - substituting the macro compiles clean under CMake, which is what the comment claimed was impossible - cmake -DRANDOM_DEVICE=/dev/random puts /dev/random in random.c.o, where the old code emitted /dev/urandom regardless - configure --with-random-device=/dev/hwrng, previously the case that left the macro undefined, now emits #define RANDOM_DEVICE "/dev/hwrng" Note for reviewers: FILE_RANDOM is defined here and read nowhere in the tree. Left in place rather than widening this change, but it is dead and worth removing separately. --- configure.ac | 15 +++++++-------- src/random.c | 11 ++++++++++- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/configure.ac b/configure.ac index a3c801458..ec83a051d 100644 --- a/configure.ac +++ b/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 diff --git a/src/random.c b/src/random.c index 5e279c5dc..3f8b69b4a 100644 --- a/src/random.c +++ b/src/random.c @@ -39,6 +39,15 @@ #include #include #include + +/* 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 @@ -214,7 +223,7 @@ dogecoin_bool dogecoin_random_bytes_internal(uint8_t* buf, uint32_t len, const u #endif (void)update_seed; //unused - FILE* frand = fopen("/dev/urandom", "rb"); // 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); From 7c826a304168371fa8e824ab288aa2e4bcd22438 Mon Sep 17 00:00:00 2001 From: bluezr Date: Thu, 6 Aug 2026 10:52:09 -0700 Subject: [PATCH 4/4] random: the Windows RNG must report failure, not -1 Stacked on #382, which fixes the POSIX half of the same problem. dogecoin_random_bytes_internal returns dogecoin_bool, and dogecoin_bool is a uint8_t. The WIN32 branch had two failure exits that returned -1: errno = EIO; return -1; /* CryptGenRandom failed */ errno = ENOSYS; return -1; /* no RNG provider at all */ -1 in a uint8_t is 255, which is true. Every caller written as if (!dogecoin_random_bytes(buf, len, 0)) { ...handle failure... } saw success, while buf still held whatever was on the stack. That is the same failure shape #382 fixes for the POSIX short read -- a failure that reports success in a release build -- on a path #382 does not touch. Both exits now return false and zero the buffer first, matching what #382 does on POSIX: eleven call sites in this tree discard the return value entirely, including bip38.c's seedb (which derives the private key) and tool.c's 32-byte seed, so a caller that ignores the result gets an obviously-unusable all-zero buffer rather than stack residue that looks random enough to spend to. The success exits return true rather than 1, so the function speaks one vocabulary throughout. On testing: the WIN32 branch cannot be reached from a Linux test, and no test here will fail if someone reintroduces `return -1` on that path. What the added test does instead is pin the hazard itself. It installs an RNG mapper that returns (dogecoin_bool)-1 and asserts the caller observes 255, that 255 is true, and that `!r` is therefore 0 -- so the reasoning behind this change is executable rather than a comment, and a future change to the type or the convention surfaces here. It also asserts the ordinary contract: a correctly-failing mapper yields exactly false. 82/82. --- src/random.c | 16 +++++++---- test/random_tests.c | 66 +++++++++++++++++++++++++++++++++++++++++++++ test/unittester.c | 2 ++ 3 files changed, 79 insertions(+), 5 deletions(-) diff --git a/src/random.c b/src/random.c index 3f8b69b4a..b7e0ee8ae 100644 --- a/src/random.c +++ b/src/random.c @@ -195,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 works in older releases as well, but is now deprecated. @@ -204,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) diff --git a/test/random_tests.c b/test/random_tests.c index 81df140ef..02c1b2974 100644 --- a/test/random_tests.c +++ b/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)); +} diff --git a/test/unittester.c b/test/unittester.c index 96dd35fae..2523af811 100644 --- a/test/unittester.c +++ b/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);