bip38, tool, bench, random: check dogecoin_random_bytes everywhere - #408
Open
xanimo wants to merge 1 commit into
Open
bip38, tool, bench, random: check dogecoin_random_bytes everywhere#408xanimo wants to merge 1 commit into
xanimo wants to merge 1 commit into
Conversation
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. dogecoinfoundation#382 and dogecoinfoundation#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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Eleven call sites discarded the return value of
dogecoin_random_bytes— six in BIP38, the rest incli/tool.c,src/random.c,src/bench.c, and the OP-TEE and OpenEnclave hosts.This matters most at
bip38.c'sseedb:seedbderivesfactorb, and the private key ispassfactor * 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 isn't correct. A caller that can't distinguish a key from an empty buffer will still use it.
How each site propagates
bip38_encrypt_ec_with_passphrase,dogecoin_bip38_encrypt_from_intermediate,hd_gen_masterbip38_ownerentropy_generate,make_valid_privkeystatic void— nowdogecoin_bool, callers propagatemain()— report and exit non-zeroTwo couldn't change signature:
dogecoin_bip38_generate_lot_sequenceisLIBDOGECOIN_API void. Rather than break the signature it emits lot 0 — which every consumer already rejects (lot == 0is checked at the top of both encrypt paths). So an RNG failure surfaces through validation that already exists.random_seedis reached through avoidfunction pointer infast_random_context. It now leavesrequires_seedset and doesn't 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 took two attempts to prove anything
The first version installed a failing RNG mapper and asserted the encrypt paths refused. It passed even with the
seedbcheck reverted — because the owner-entropy check fails first and shadows it. It was testing the aggregate, not the site.The isolating assertion uses
dogecoin_bip38_encrypt_from_intermediatewith the BIP38 spec intermediate code, so the owner entropy is already fixed and the RNG is reached forseedbalone.With that in place, reverting the
seedbcheck doesn't merely fail an assertion:The code carries on with an unwritten
seedband crashes downstream. Restored: 82/82.Worth noting there are now no unchecked
dogecoin_random_bytescalls left in the tree.This will show red on cppcheck until #403 lands — that job fails on
0.1.5-devitself, not on this branch.