bip39, qr: bound the wordlist token read; don't rely on assert for indexing - #401
Open
xanimo wants to merge 4 commits into
Open
bip39, qr: bound the wordlist token read; don't rely on assert for indexing#401xanimo wants to merge 4 commits into
xanimo wants to merge 4 commits into
Conversation
…dexing
get_custom_words() read wordlist tokens with
char word[1024];
while (fscanf(fp, "%s", word) == 1)
with no field width, so a wordlist file containing a whitespace-free
token of 1024 bytes or more wrote past the buffer. Demonstrated, not
inferred: the added test drives a 2000-byte token through it and
AddressSanitizer reports
stack-buffer-overflow ... WRITE of size 2001
The path is reachable through the public API. dogecoin_generate_mnemonic()
takes an optional wordlist filename and a non-NULL value lands here;
passing NULL uses the built-in wordlists and never reaches this code. So
a caller is affected only if it offers a custom wordlist, but a library
cannot assume anything about the files its callers are handed, and
nothing in the API contract bounds token length.
Affected: v0.1.2, v0.1.3, v0.1.4, v0.1.5-pre.
Bounds the conversion with a field width derived from the buffer size, so
the two cannot drift. A width alone would silently split an over-long
token into several words rather than rejecting the file, so the read also
peeks at the next byte: a token that fills the buffer without ending at
whitespace or EOF means the file is malformed and it is refused. Real
BIP39 words are at most 8 characters, so nothing legitimate comes close.
Tests cover the overflow, a token of exactly the maximum length, a
well-formed 2048-word file that must still parse, and a short file that
must still be refused. Verified the test has bite: with only the field
width removed, the suite aborts under ASan instead of failing an
assertion.
Separately, getNumDataCodewords() guarded its table indexing with
assert(0 <= e && e < 4);
and then indexed ECC_CODEWORDS_PER_BLOCK[e][v] and
NUM_ERROR_CORRECTION_BLOCKS[e][v] directly. assert() compiles out under
NDEBUG, so a release build reads out of bounds on an out-of-range ecl
rather than trapping -- the same shape as the /dev/urandom short read
that only tripped an assert. Adds a real bounds check alongside it.
Both were found by the cppcheck job added in dogecoinfoundation#359 (invalidscanf,
negativeIndex). Both findings now clear.
82/82, clean under ASan+LSan.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL's security-extended "Unbounded write" query flagged the strcpy below the fscanf on PR dogecoinfoundation#401. It is a false positive as the code stands: the allocation is sized strlen(word) + 1 from the very string being copied, so it cannot overflow. It was not a false positive before the previous commit. With an unbounded fscanf, word could be smashed and strlen and strcpy would then both run over corrupted memory -- the query was pointing at the sink while the defect was at the source. Measure once, allocate from that length, and memcpy the same length. The behaviour is identical, but the safety argument is now local to these three lines instead of depending on the scanf field width several lines above, which is what the analyser could not see. 82/82, clean under ASan+LSan.
aarch64-linux-optee failed at the positive control: get_custom_words() is inside #ifndef USE_OPTEE because OP-TEE has no file I/O, so the stub returns -1 unconditionally and the "a well-formed 2048-word file still parses" assertion could never hold there. Rather than skip the test on that target, assert the stub's contract: it must fail, and it must not populate the wordlist. A build where the stub began returning success would let a caller believe a custom wordlist had been loaded when none was, which is worth a test of its own. The file-based assertions stay on every platform that has file I/O. 82/82 natively; the USE_OPTEE branch compile-checked separately.
CodeQL flagged an unbounded write at the strcat in get_mnemonic() on PR dogecoinfoundation#401. It is not in the diff, but it is the same attack path, and the first fix did not close it. get_mnemonic() concatenates wordlist entries into a caller-supplied MNEMONIC, which is char[MAX_WORDS_IN_MNEMONIC * MAX_CHARS_IN_MNEMONIC_WORD * HEX_CHARS_PER_BYTE + 1] = 769 bytes. Bounding the loader at 1023 characters stopped the read from overflowing word[] but still let a wordlist supply entries far longer than the assembly can hold: 24 words of 1023 characters is roughly 24 KB into 769 bytes. The overflow moved rather than went away. Bound the loader at MAX_CHARS_IN_MNEMONIC_WORD instead. That is the constant the mnemonic buffer is already sized from, so the loader now enforces the invariant the rest of the file assumes rather than inventing a looser one of its own. Real BIP39 words are at most 8 characters, so nothing legitimate is affected -- the shipped wordlists are unchanged and still parse. The two-level BIP39_STR() expands the constant before stringifying, so the scanf field width follows the bound automatically. Also: the test created its scratch wordlist with fopen(path, "w"), leaving the mode to the umask -- four more CodeQL alerts, and a real if minor issue, since another local user could rewrite the file between creation and read. Creates it 0600 via open()+fdopen() on POSIX. 82/82, clean under ASan+LSan.
This was referenced Aug 5, 2026
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
2 participants
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.
Two findings from the cppcheck job added in #359 —
invalidscanfandnegativeIndex. Both are real. Both now clear.The wordlist read overflows the stack
get_custom_words()read tokens with no field width:A wordlist file containing a whitespace-free token of 1024 bytes or more writes past the buffer. Demonstrated, not inferred — the added test drives a 2000-byte token through it, and with only the field width removed AddressSanitizer reports:
Reachable through the public API.
dogecoin_generate_mnemonic()takes an optional wordlist filename (bip39.h:176); a non-NULL value lands here. Passing NULL uses the built-in wordlists and never reaches this code.Affected: v0.1.2, v0.1.3, v0.1.4, v0.1.5-pre. Not dev-only.
Who is affected
A consumer is affected only if it offers a custom wordlist. I can't enumerate consumers from inside the library, so rather than guess at severity, here is what a downstream needs to check for themselves:
Nothing in the API contract bounds token length, so arbitrary file content is permitted input and the library has to handle it. That's true regardless of whether any particular caller happens to feed it a hostile file.
The fix
Bounds the conversion with a field width derived from the buffer size, so the width and buffer can't drift apart.
A width alone would be a subtler bug than the one it fixes: it silently splits an over-long token into several words instead of rejecting the file. So the read also peeks at the next byte — a token that fills the buffer without ending at whitespace or EOF means the file is malformed, and it's refused. Real BIP39 words are at most 8 characters, so nothing legitimate comes close.
Tests
Verified the test has bite rather than merely passing: with only the field width removed, the suite aborts under ASan instead of failing an assertion.
assertwas load-bearing for table indexinggetNumDataCodewords()guarded its indexing withassert(0 <= e && e < 4)and then indexedECC_CODEWORDS_PER_BLOCK[e][v]andNUM_ERROR_CORRECTION_BLOCKS[e][v]directly.assert()compiles out underNDEBUG, so a release build reads out of bounds on an out-of-rangeeclrather than trapping. Same shape as the/dev/urandomshort read in #382, which also only tripped an assert. Adds a real bounds check alongside the assert.82/82, clean under ASan+LSan. Both cppcheck findings clear.
Note on the cppcheck job itself: it currently fails on
0.1.5-dev— I reproduced it on an untouched checkout. Its header documents a phase-0 policy of "onlyerrorseverity fails the job", but the step named gate on error severity runs--enable=warning --error-exitcode=1, so warnings fail it. After this PR there are 32 findings left: 18invalidPrintfArgType_sint(%dwith unsigned — real but cosmetic), 6 null-check ordering warnings inwallet.c/utils.cworth a look, 4internalAstError(cppcheck 2.7 can't parseHASH_DEL/assert), 2uninitvaron OP-TEETEEC_MEMREF_TEMP_OUTPUTbuffers, and 1literalWithCharPtrCompare— the last three groups being false positives. That's a separate PR; this one only fixes the two real defects.