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/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) { diff --git a/src/random.c b/src/random.c index a17e9ea12..b7e0ee8ae 100644 --- a/src/random.c +++ b/src/random.c @@ -27,6 +27,7 @@ */ #include +#include #include #include @@ -38,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 @@ -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 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 } 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);