random: the Windows RNG must report failure, not -1 - #405
Open
xanimo wants to merge 4 commits into
Open
Conversation
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.
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/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.
Stacked on dogecoinfoundation#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 dogecoinfoundation#382 fixes for the POSIX short read -- a failure that reports success in a release build -- on a path dogecoinfoundation#382 does not touch. Both exits now return false and zero the buffer first, matching what dogecoinfoundation#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.
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
1 participant
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.
Stacked on #382, which fixes the POSIX half of the same problem. Review #382 first; this PR's diff collapses to one file once it lands.
The bug
dogecoin_random_bytes_internalreturnsdogecoin_bool— anddogecoin_boolistypedef uint8_t. TheWIN32branch had two failure exits:-1in auint8_tis255, which is true. I compiled it to be certain:So every caller written the obvious way —
— saw success, while
bufstill held whatever was on the stack.This is the same failure shape #382 fixes for the POSIX short read: a failure that reports success in a release build. It's on a path #382 doesn't reach, which is why it needs its own change.
Reachability is narrow — it needs
BCryptGenRandomto be unresolved andCryptGenRandomto fail. On modern Windows that's unlikely. But "unlikely" is doing a lot of work in a key-generation path, and theENOSYSexit is reachable whenever neither provider resolves.The fix
Both exits return
falseand zero the buffer first, matching #382's POSIX behaviour. That second part matters because eleven call sites in this tree discard the return value entirely, including:bip38.c:1368,bip38.c:1474—seedb, which derivesfactorband thence the private keycli/tool.c:144— a 32-byte seedbip38.c:372/378,bip38.c:1185/1186— ownerentropy and lot/sequenceA caller that ignores the result now gets an obviously-unusable all-zero buffer instead of stack residue that looks random enough to spend to.
The success exits return
truerather than1, so the function speaks one vocabulary throughout.On testing — the honest version
The
WIN32branch cannot be reached from a Linux test, and no test here will fail if someone reintroducesreturn -1on that path. I'd rather say that than imply coverage I don't have.What the added test does instead is pin the hazard. It installs an RNG mapper returning
(dogecoin_bool)-1and asserts the caller observes255, that255is true, and that!ris therefore0— so the reasoning behind this change is executable rather than a comment, and a future change to the type or the convention surfaces right here. It also asserts the ordinary contract: a correctly-failing mapper yields exactlyfalse.82/82.
Found during an entropy-path sweep. The remaining item from that sweep, not fixed here: those eleven callers should check the return value. Zeroing the buffer makes ignoring it survivable, not correct.