From 26a956cdff1007e1b213677a5b7678adfc793e9c Mon Sep 17 00:00:00 2001 From: bluezr Date: Thu, 6 Aug 2026 12:48:19 -0700 Subject: [PATCH] bip38, tool, bench, random: check dogecoin_random_bytes everywhere Eleven call sites discarded the return value of dogecoin_random_bytes: six in BIP38, the rest in cli/tool.c, src/random.c, src/bench.c and the OP-TEE and OpenEnclave hosts. This matters most at bip38.c's seedb: seedb derives factorb, and the private key is passfactor * factorb mod N. With the return ignored, an RNG failure produced a key from whatever was already in the buffer. #382 and #405 make that survivable by zeroing the buffer on failure, but survivable is not the same as correct -- a caller that cannot tell the difference between a key and an empty buffer will still use it. Where the containing function already returned a status, it now checks and propagates: bip38_encrypt_ec_with_passphrase, dogecoin_bip38_encrypt_from_intermediate, and hd_gen_master. bip38_ownerentropy_generate was static void, so it becomes static dogecoin_bool and both callers propagate. make_valid_privkey in bench.c likewise. The OP-TEE and OpenEnclave calls sit directly in main(), so they report and exit non-zero. Two could not be changed that way. dogecoin_bip38_generate_lot_sequence is LIBDOGECOIN_API and returns void. Rather than break the signature it now emits lot 0, which every consumer of these values already rejects -- `lot == 0` is checked at the top of both encrypt paths -- so an RNG failure surfaces there instead of yielding a lot and sequence derived from an unwritten buffer. random_seed is reached through a void function pointer in fast_random_context. On failure it now leaves requires_seed set and does not key the RNG, so a later call retries rather than the context coming up keyed with an all-zero seed and reporting itself ready. The test installs an RNG mapper that always fails and checks the callers refuse. It took two attempts to make it prove anything: the first version passed even with the seedb check reverted, because the owner-entropy check fails first and shadows it. The isolating assertion uses dogecoin_bip38_encrypt_from_intermediate with the BIP38 spec intermediate code, so the owner entropy is fixed and the RNG is reached for seedb alone. With that in place, reverting the seedb check does not merely fail an assertion -- the suite segfaults, because the code carries on with an unwritten seedb. 82/82. --- src/bench.c | 17 ++++---- src/bip38.c | 48 +++++++++++++++++----- src/cli/tool.c | 5 ++- src/openenclave/host/host.c | 5 ++- src/optee/host/main.c | 5 ++- src/random.c | 10 ++++- test/random_tests.c | 82 +++++++++++++++++++++++++++++++++++++ test/unittester.c | 2 + 8 files changed, 152 insertions(+), 22 deletions(-) diff --git a/src/bench.c b/src/bench.c index 0517a6a58..66dad6452 100644 --- a/src/bench.c +++ b/src/bench.c @@ -672,13 +672,16 @@ static void sphincs_shake_128f_commit_bench(benchmark_context *ctx) { /* ---- secp256k1 via ECC module (no direct secp includes) ---- */ -static void make_valid_privkey(uint8_t sk[32]) { +static dogecoin_bool make_valid_privkey(uint8_t sk[32]) { /* try random bytes until valid */ while (1) { - dogecoin_random_bytes(sk, 32, 0); - if (dogecoin_ecc_verify_privatekey(sk)) return; + if (!dogecoin_random_bytes(sk, 32, 0)) { + dogecoin_mem_zero(sk, 32); + return false; + } + if (dogecoin_ecc_verify_privatekey(sk)) return true; sk[0] ^= 0x01; /* nudge */ - if (dogecoin_ecc_verify_privatekey(sk)) return; + if (dogecoin_ecc_verify_privatekey(sk)) return true; } } @@ -688,7 +691,7 @@ static void make_msg32(const uint8_t *src, uint8_t out32[32]) { static void secp_keypair_bench(benchmark_context *ctx) { /* keypair = generate priv + derive compressed pub */ - uint8_t sk[32]; make_valid_privkey(sk); + uint8_t sk[32]; if (!make_valid_privkey(sk)) return; uint8_t pk[33]; size_t pklen = 33; dogecoin_ecc_get_pubkey(sk, pk, &pklen, true); ctx->end = gettimedouble(); ctx->endCycles = perf_cpucycles(); @@ -699,7 +702,7 @@ static void secp_sign_bench(benchmark_context *ctx) { /* reuse one key for stable sign throughput */ static int primed = 0; static uint8_t sk[32]; - if (!primed) { make_valid_privkey(sk); primed = 1; } + if (!primed) { if (!make_valid_privkey(sk)) return; primed = 1; } uint8_t msg32[32]; make_msg32(ctx->input, msg32); uint256_t msg_hash; memcpy(msg_hash, msg32, 32); @@ -718,7 +721,7 @@ static void secp_verify_bench(benchmark_context *ctx) { static unsigned char sigder[80]; static size_t siglen = 0; if (!primed) { - uint8_t sk[32]; make_valid_privkey(sk); + uint8_t sk[32]; if (!make_valid_privkey(sk)) return; pklen = 33; dogecoin_ecc_get_pubkey(sk, pk, &pklen, true); make_msg32(ctx->input, msg32); memcpy(msg_hash, msg32, 32); diff --git a/src/bip38.c b/src/bip38.c index 7ec060718..fa4f956d1 100644 --- a/src/bip38.c +++ b/src/bip38.c @@ -361,7 +361,7 @@ static dogecoin_bool bip38_ec_derived_key( BIP38_SCRYPT_DERIVED_SIZE); } -static void bip38_ownerentropy_generate( +static dogecoin_bool bip38_ownerentropy_generate( dogecoin_bool use_lot_sequence, uint32_t lot, uint32_t sequence, @@ -369,14 +369,19 @@ static void bip38_ownerentropy_generate( { if (use_lot_sequence) { uint32_t lotsequence = lot * 4096u + sequence; - dogecoin_random_bytes(ownerentropy_out, 4, 1); + if (!dogecoin_random_bytes(ownerentropy_out, 4, 1)) { + return false; + } ownerentropy_out[4] = (uint8_t)((lotsequence >> 24) & 0xff); ownerentropy_out[5] = (uint8_t)((lotsequence >> 16) & 0xff); ownerentropy_out[6] = (uint8_t)((lotsequence >> 8) & 0xff); ownerentropy_out[7] = (uint8_t)(lotsequence & 0xff); } else { - dogecoin_random_bytes(ownerentropy_out, 8, 1); + if (!dogecoin_random_bytes(ownerentropy_out, 8, 1)) { + return false; + } } + return true; } static dogecoin_bool bip38_parse_intermediate_code( @@ -1182,8 +1187,17 @@ void dogecoin_bip38_generate_lot_sequence( return; } - dogecoin_random_bytes((uint8_t*)&lot_raw, sizeof(lot_raw), 1); - dogecoin_random_bytes((uint8_t*)&sequence_raw, sizeof(sequence_raw), 1); + /* This is LIBDOGECOIN_API and returns void, so it cannot report failure + directly without breaking the signature. Emit lot 0 instead: every + caller of these values already rejects it (`lot == 0` at the top of both + encrypt paths), so an RNG failure surfaces there rather than producing a + lot and sequence derived from an unwritten buffer. */ + if (!dogecoin_random_bytes((uint8_t*)&lot_raw, sizeof(lot_raw), 1) + || !dogecoin_random_bytes((uint8_t*)&sequence_raw, sizeof(sequence_raw), 1)) { + *lot_out = 0; + *sequence_out = 0; + return; + } *lot_out = (lot_raw % BIP38_LOT_MAX) + 1u; *sequence_out = sequence_raw % (BIP38_SEQUENCE_MAX + 1u); } @@ -1352,7 +1366,9 @@ static dogecoin_bool bip38_encrypt_ec_with_passphrase( if (ownerentropy_override) { memcpy(ownerentropy, ownerentropy_override, 8); } else { - bip38_ownerentropy_generate(use_lot_sequence, lot, sequence, ownerentropy); + if (!bip38_ownerentropy_generate(use_lot_sequence, lot, sequence, ownerentropy)) { + return false; + } } if (!bip38_derive_passfactor(passphrase, ownerentropy, use_lot_sequence, passfactor)) { @@ -1364,8 +1380,12 @@ static dogecoin_bool bip38_encrypt_ec_with_passphrase( if (seedb_override) { memcpy(seedb, seedb_override, BIP38_SEEDB_LEN); - } else { - dogecoin_random_bytes(seedb, BIP38_SEEDB_LEN, 1); + } else if (!dogecoin_random_bytes(seedb, BIP38_SEEDB_LEN, 1)) { + /* seedb derives factorb, and the private key is passfactor * factorb + mod N. Continuing here would mint a key from whatever the RNG left + in the buffer. */ + dogecoin_mem_zero(seedb, BIP38_SEEDB_LEN); + return false; } chain = bip38_chain_from_address_hint(address_hint); @@ -1417,7 +1437,9 @@ dogecoin_bool dogecoin_bip38_generate_intermediate_code( if (ownerentropy_override) { memcpy(ownerentropy, ownerentropy_override, 8); } else { - bip38_ownerentropy_generate(use_lot_sequence, lot, sequence, ownerentropy); + if (!bip38_ownerentropy_generate(use_lot_sequence, lot, sequence, ownerentropy)) { + return false; + } } if (!bip38_derive_passfactor(passphrase, ownerentropy, use_lot_sequence, passfactor)) { @@ -1470,8 +1492,12 @@ dogecoin_bool dogecoin_bip38_encrypt_from_intermediate( if (seedb_override) { memcpy(seedb, seedb_override, BIP38_SEEDB_LEN); - } else { - dogecoin_random_bytes(seedb, BIP38_SEEDB_LEN, 1); + } else if (!dogecoin_random_bytes(seedb, BIP38_SEEDB_LEN, 1)) { + /* seedb derives factorb, and the private key is passfactor * factorb + mod N. Continuing here would mint a key from whatever the RNG left + in the buffer. */ + dogecoin_mem_zero(seedb, BIP38_SEEDB_LEN); + return false; } chain = bip38_chain_from_address_hint(address_chain_hint); diff --git a/src/cli/tool.c b/src/cli/tool.c index a8ad4319e..bf4f06e5f 100644 --- a/src/cli/tool.c +++ b/src/cli/tool.c @@ -141,7 +141,10 @@ dogecoin_bool hd_gen_master(const dogecoin_chainparams* chain, char* masterkeyhe { dogecoin_hdnode node; uint8_t seed[32]; - dogecoin_random_bytes(seed, 32, true); + 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..95bf862d8 100644 --- a/src/openenclave/host/host.c +++ b/src/openenclave/host/host.c @@ -412,7 +412,10 @@ 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); + if (!dogecoin_random_bytes(random_bytes, sizeof(random_bytes), 0)) { + fprintf(stderr, "failed to obtain entropy\n"); + return 1; + } 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..14ad3f053 100644 --- a/src/optee/host/main.c +++ b/src/optee/host/main.c @@ -775,7 +775,10 @@ 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); + if (!dogecoin_random_bytes(random_bytes, sizeof(random_bytes), 0)) { + fprintf(stderr, "failed to obtain entropy\n"); + return 1; + } shared_secret = malloc(TOTP_SECRET_HEX_SIZE + 1); if (!shared_secret) { diff --git a/src/random.c b/src/random.c index a17e9ea12..fbe9eec6c 100644 --- a/src/random.c +++ b/src/random.c @@ -229,7 +229,15 @@ void random_seed(struct fast_random_context* this) dogecoin_random_init(); uint256_t seed; dogecoin_mem_zero(seed, 32); - dogecoin_random_bytes(seed, 32, 0); + if (!dogecoin_random_bytes(seed, 32, 0)) { + /* random_seed is reached through a void function pointer in + fast_random_context, so it cannot report failure without changing + that interface. Leave requires_seed set and do not key the RNG: a + later call re-enters here and tries again, rather than the context + coming up keyed with an all-zero seed and looking ready. */ + dogecoin_mem_zero(seed, 32); + return; + } this->rng->setkey(this->rng, seed, 32); this->requires_seed = false; } diff --git a/test/random_tests.c b/test/random_tests.c index 81df140ef..a7c4ef282 100644 --- a/test/random_tests.c +++ b/test/random_tests.c @@ -14,6 +14,7 @@ #include +#include #include #include @@ -80,3 +81,84 @@ void test_random() // switch back to the default random callback mapper dogecoin_rnd_set_mapper_default(); } + + +/* --- regression: a failing RNG must not yield key material --- */ + +static void rng_prop_init(void) {} + +static dogecoin_bool rng_prop_fail(uint8_t* buf, uint32_t len, const uint8_t update_seed) +{ + (void)buf; (void)len; (void)update_seed; + return false; +} + +/* + * Eleven call sites discarded dogecoin_random_bytes' return value, six of them + * in BIP38 -- including seedb, which derives factorb and thence the private + * key. With the return ignored, an RNG failure produced a key from whatever was + * already in the buffer. + * + * Driving the real failure path is not possible from a test, so this installs a + * mapper that always fails and checks the callers behave: the encrypt paths + * must refuse rather than mint a key. + */ +void test_random_failure_propagates() +{ + dogecoin_rnd_mapper mapper; + uint8_t privkey[DOGECOIN_ECKEY_PKEY_LENGTH]; + char encrypted[128]; + char confirmation[128]; + size_t enc_sz, conf_sz; + uint32_t lot = 12345, sequence = 42; + + mapper.dogecoin_random_init = rng_prop_init; + mapper.dogecoin_random_bytes = rng_prop_fail; + dogecoin_rnd_set_mapper(mapper); + + /* EC-multiplied encryption reaches seedb and the owner entropy. It must + fail rather than return a key derived from an unwritten buffer. */ + enc_sz = sizeof(encrypted); + conf_sz = sizeof(confirmation); + u_assert_int_eq((int)dogecoin_bip38_encrypt_ec_multiplied( + "passphrase", true, false, 0, 0, "DGYrGxANmgjcoZ9xJWncHr6fuA6Y1ZQ56Y", + privkey, encrypted, &enc_sz, confirmation, &conf_sz), 0); + + /* Same with lot/sequence, which additionally goes through the owner + entropy path. */ + enc_sz = sizeof(encrypted); + conf_sz = sizeof(confirmation); + u_assert_int_eq((int)dogecoin_bip38_encrypt_ec_multiplied( + "passphrase", true, true, 100000, 7, "DGYrGxANmgjcoZ9xJWncHr6fuA6Y1ZQ56Y", + privkey, encrypted, &enc_sz, confirmation, &conf_sz), 0); + + /* + * Isolate the seedb site. encrypt_from_intermediate takes a ready-made + * intermediate code -- the BIP38 spec vector -- so the owner entropy is + * already fixed and the RNG is reached for seedb alone. Without the check + * at that site this call still returns a key, because nothing upstream + * fails first. + */ + enc_sz = sizeof(encrypted); + conf_sz = sizeof(confirmation); + u_assert_int_eq((int)dogecoin_bip38_encrypt_from_intermediate( + "passphrasepxFy57B9v8HtUsszJYKReoNDV6VHjUSGt8EVJmux9n1J3Ltf1gRxyDGXqnf9qm", + true, NULL, "DGYrGxANmgjcoZ9xJWncHr6fuA6Y1ZQ56Y", + encrypted, &enc_sz, confirmation, &conf_sz), 0); + + /* dogecoin_bip38_generate_lot_sequence returns void and is public API, so + it signals failure with lot 0 -- a value every consumer of these already + rejects. */ + dogecoin_bip38_generate_lot_sequence(&lot, &sequence); + u_assert_uint32_eq(lot, 0U); + u_assert_uint32_eq(sequence, 0U); + + dogecoin_rnd_set_mapper_default(); + + /* And with the real RNG restored, the same call succeeds. */ + enc_sz = sizeof(encrypted); + conf_sz = sizeof(confirmation); + u_assert_int_eq((int)dogecoin_bip38_encrypt_ec_multiplied( + "passphrase", true, false, 0, 0, "DGYrGxANmgjcoZ9xJWncHr6fuA6Y1ZQ56Y", + privkey, encrypted, &enc_sz, confirmation, &conf_sz), 1); +} diff --git a/test/unittester.c b/test/unittester.c index 96dd35fae..c486e2a03 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_propagates(); 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_propagates); u_run_test(test_rmd160); u_run_test(test_scrypt); u_run_test(test_serialize);