From dc37340a3fe13b4402f022baf44aeee167e3b4d4 Mon Sep 17 00:00:00 2001 From: bluezr Date: Wed, 5 Aug 2026 13:02:51 -0700 Subject: [PATCH 1/4] bip39, qr: bound the wordlist token read; don't rely on assert for indexing 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 #359 (invalidscanf, negativeIndex). Both findings now clear. 82/82, clean under ASan+LSan. --- include/dogecoin/bip39.h | 12 ++++++ src/bip39.c | 24 ++++++++++- src/qr.c | 6 +++ test/bip39_tests.c | 88 ++++++++++++++++++++++++++++++++++++++++ test/unittester.c | 2 + 5 files changed, 130 insertions(+), 2 deletions(-) diff --git a/include/dogecoin/bip39.h b/include/dogecoin/bip39.h index 8aa993f82..7ef6a4fd8 100644 --- a/include/dogecoin/bip39.h +++ b/include/dogecoin/bip39.h @@ -27,6 +27,18 @@ LIBDOGECOIN_API /* number of words in the language wordlist used for mnemonics */ #define LANG_WORD_CNT 2048 +/* + * Longest token get_custom_words() will accept from a wordlist file, and the + * buffer it reads into. BIP39 words are at most 8 characters; this is far + * wider than any real wordlist and exists only to bound the conversion. + * BIP39_STR() stringifies the length for the scanf field width so the width + * and the buffer size cannot drift apart. + */ +#define BIP39_WORD_MAXLEN 1023 +#define BIP39_WORD_BUFSZ (BIP39_WORD_MAXLEN + 1) +#define BIP39_STR_(x) #x +#define BIP39_STR(x) BIP39_STR_(x) + /* Indicates the number of entropy bits supported */ #define MAX_ENTROPY_BITS 256 diff --git a/src/bip39.c b/src/bip39.c index e8c748814..bde957d7d 100644 --- a/src/bip39.c +++ b/src/bip39.c @@ -25,6 +25,7 @@ * OTHER DEALINGS IN THE SOFTWARE. */ +#include #include #include #include @@ -463,7 +464,8 @@ int get_custom_words(const char *filepath, char* wordlist[]) { #ifndef USE_OPTEE /* OPTEE does not support file I/O */ int i = 0; FILE * fp; - char word[1024]; + int c; + char word[BIP39_WORD_BUFSZ]; /* Check that file path is valid */ if (filepath == NULL) { @@ -477,7 +479,25 @@ int get_custom_words(const char *filepath, char* wordlist[]) { return -1; } - while (fscanf(fp, "%s", word) == 1) { + while (fscanf(fp, "%" BIP39_STR(BIP39_WORD_MAXLEN) "s", word) == 1) { + /* + * The width above bounds the write, but on its own it would silently + * split an over-long token into several words rather than rejecting + * the file. Peek at the next byte: if the token filled the buffer and + * did not end at whitespace or EOF, it was longer than any BIP39 word + * can be and the file is malformed. + */ + if (strlen(word) == BIP39_WORD_MAXLEN) { + c = fgetc(fp); + if (c != EOF && !isspace(c)) { + fprintf(stderr, "ERROR: word longer than %d characters\n", BIP39_WORD_MAXLEN); + fclose(fp); + return -1; + } + if (c != EOF) { + ungetc(c, fp); + } + } if (i >= LANG_WORD_CNT) { fprintf(stderr, "ERROR: too many words in file\n"); fclose(fp); diff --git a/src/qr.c b/src/qr.c index 8c05a5daf..75530bd23 100644 --- a/src/qr.c +++ b/src/qr.c @@ -327,6 +327,12 @@ testable void addEccAndInterleave(uint8_t data[], int version, enum qrcodegen_Ec testable int getNumDataCodewords(int version, enum qrcodegen_Ecc ecl) { int v = version, e = (int)ecl; assert(0 <= e && e < 4); + // assert() is compiled out under NDEBUG, and both tables below are indexed + // with e directly, so a release build would read out of bounds on an + // out-of-range ecl rather than trapping. Same class as the /dev/urandom + // short read that only tripped an assert. + if (e < 0 || e >= 4) + return 0; return getNumRawDataModules(v) / 8 - ECC_CODEWORDS_PER_BLOCK [e][v] * NUM_ERROR_CORRECTION_BLOCKS[e][v]; diff --git a/test/bip39_tests.c b/test/bip39_tests.c index 8349f998c..7d04d4b50 100644 --- a/test/bip39_tests.c +++ b/test/bip39_tests.c @@ -1359,3 +1359,91 @@ void test_bip39() u_assert_mem_eq(seed, seed_test, 64); debug_print("%s\n", utils_uint8_to_hex(seed, 64)); } + + +/* + * get_custom_words() read wordlist tokens with fscanf(fp, "%s", word) into a + * fixed 1024-byte stack buffer, with no field width. A wordlist file holding a + * whitespace-free token of 1024 bytes or more smashed the stack. + * + * The path is reachable through the public API: dogecoin_generate_mnemonic() + * takes an optional wordlist filename, and passing a non-NULL one lands here. + * Passing NULL uses the built-in lists and never reaches this code. + * + * Present in v0.1.2, v0.1.3, v0.1.4 and v0.1.5-pre. + * + * Without the fix this test does not merely fail -- it corrupts the stack, and + * under ASan it aborts with a stack-buffer-overflow in get_custom_words. + */ +void test_bip39_custom_wordlist_bounds() +{ + const char* path = "bip39_overlong_wordlist.tmp"; + char* wordlist[LANG_WORD_CNT]; + FILE* fp; + int i; + + dogecoin_mem_zero(wordlist, sizeof(wordlist)); + + /* A token far longer than the 1024-byte buffer, written first so the + rejection happens before any word is allocated. */ + fp = fopen(path, "w"); + u_assert_true(fp != NULL); + for (i = 0; i < 2000; i++) { + fputc('a', fp); + } + fputc('\n', fp); + /* Enough well-formed words after it that a parser which skipped past the + long token would otherwise reach the 2048-word count. */ + for (i = 0; i < LANG_WORD_CNT; i++) { + fprintf(fp, "abandon\n"); + } + fclose(fp); + + /* Must be refused, and must not write past word[]. */ + u_assert_int_eq(get_custom_words(path, wordlist), -1); + /* Nothing was allocated before the rejection. */ + u_assert_true(wordlist[0] == NULL); + remove(path); + + /* A token of exactly the maximum length is still refused, since no BIP39 + word is anywhere near it and accepting it would mean the next read + silently began mid-token. */ + fp = fopen(path, "w"); + u_assert_true(fp != NULL); + for (i = 0; i < BIP39_WORD_MAXLEN + 1; i++) { + fputc('b', fp); + } + fputc('\n', fp); + fclose(fp); + u_assert_int_eq(get_custom_words(path, wordlist), -1); + remove(path); + + /* Positive control: a well-formed 2048-word file still parses, so the + bound did not break legitimate wordlists. */ + fp = fopen(path, "w"); + u_assert_true(fp != NULL); + for (i = 0; i < LANG_WORD_CNT; i++) { + fprintf(fp, "abandon\n"); + } + fclose(fp); + u_assert_int_eq(get_custom_words(path, wordlist), 0); + u_assert_true(wordlist[0] != NULL); + u_assert_str_eq(wordlist[0], "abandon"); + for (i = 0; i < LANG_WORD_CNT; i++) { + free(wordlist[i]); + wordlist[i] = NULL; + } + remove(path); + + /* A file with too few words is still refused. */ + fp = fopen(path, "w"); + u_assert_true(fp != NULL); + fprintf(fp, "abandon\nability\n"); + fclose(fp); + u_assert_int_eq(get_custom_words(path, wordlist), -1); + for (i = 0; i < LANG_WORD_CNT; i++) { + free(wordlist[i]); + wordlist[i] = NULL; + } + remove(path); +} diff --git a/test/unittester.c b/test/unittester.c index 96dd35fae..ee685f62f 100644 --- a/test/unittester.c +++ b/test/unittester.c @@ -44,6 +44,7 @@ extern void test_base64(); extern void test_dit(); extern void test_bip32(); extern void test_bip39(); +extern void test_bip39_custom_wordlist_bounds(); extern void test_bip44(); extern void test_block_header(); extern void test_buffer(); @@ -175,6 +176,7 @@ int main() u_run_test(test_dit); u_run_test(test_bip32); u_run_test(test_bip39); + u_run_test(test_bip39_custom_wordlist_bounds); u_run_test(test_bip44); u_run_test(test_block_header); u_run_test(test_buffer); From 865f5dbabb8c75b6505e2fb5782252e3dd8108c4 Mon Sep 17 00:00:00 2001 From: bluezr Date: Wed, 5 Aug 2026 13:17:02 -0700 Subject: [PATCH 2/4] bip39: copy the word from a measured length rather than strcpy CodeQL's security-extended "Unbounded write" query flagged the strcpy below the fscanf on PR #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. --- src/bip39.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/bip39.c b/src/bip39.c index bde957d7d..12d3ae8b1 100644 --- a/src/bip39.c +++ b/src/bip39.c @@ -465,6 +465,7 @@ int get_custom_words(const char *filepath, char* wordlist[]) { int i = 0; FILE * fp; int c; + size_t wordlen; char word[BIP39_WORD_BUFSZ]; /* Check that file path is valid */ @@ -503,13 +504,18 @@ int get_custom_words(const char *filepath, char* wordlist[]) { fclose(fp); return -1; } - wordlist[i] = malloc(strlen(word) + 1); + /* Size and copy from one measured length rather than strcpy'ing a + string whose bound the compiler cannot see. Equivalent once the read + above is bounded, but it keeps the safety argument local to these + three lines instead of depending on the scanf width further up. */ + wordlen = strlen(word); + wordlist[i] = malloc(wordlen + 1); if (wordlist[i] == NULL) { fprintf(stderr, "ERROR: cannot allocate memory\n"); fclose(fp); return -1; } - strcpy(wordlist[i], word); + memcpy(wordlist[i], word, wordlen + 1); i++; } From 7a68faf5bd92c3ef9d4f85458d6012b7de8168a7 Mon Sep 17 00:00:00 2001 From: bluezr Date: Wed, 5 Aug 2026 13:31:18 -0700 Subject: [PATCH 3/4] test: guard the wordlist bounds test for OP-TEE 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. --- test/bip39_tests.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/bip39_tests.c b/test/bip39_tests.c index 7d04d4b50..386bc3fda 100644 --- a/test/bip39_tests.c +++ b/test/bip39_tests.c @@ -1377,6 +1377,18 @@ void test_bip39() */ void test_bip39_custom_wordlist_bounds() { +#ifdef USE_OPTEE + /* + * OP-TEE has no file I/O, so get_custom_words() is compiled to a stub that + * always fails. Assert that contract rather than skipping: a build where + * the stub started returning success would mean callers could believe a + * wordlist had been loaded when none was. + */ + char* wordlist_stub[LANG_WORD_CNT]; + dogecoin_mem_zero(wordlist_stub, sizeof(wordlist_stub)); + u_assert_int_eq(get_custom_words("unused", wordlist_stub), -1); + u_assert_true(wordlist_stub[0] == NULL); +#else const char* path = "bip39_overlong_wordlist.tmp"; char* wordlist[LANG_WORD_CNT]; FILE* fp; @@ -1446,4 +1458,5 @@ void test_bip39_custom_wordlist_bounds() wordlist[i] = NULL; } remove(path); +#endif /* USE_OPTEE */ } From 0708c880d14f33593348b6bab68089fff7ea8c6e Mon Sep 17 00:00:00 2001 From: bluezr Date: Wed, 5 Aug 2026 13:54:24 -0700 Subject: [PATCH 4/4] bip39: bound wordlist words to what the mnemonic can hold CodeQL flagged an unbounded write at the strcat in get_mnemonic() on PR #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. --- include/dogecoin/bip39.h | 31 +++++++++++++++++++------------ test/bip39_tests.c | 31 +++++++++++++++++++++++++++---- 2 files changed, 46 insertions(+), 16 deletions(-) diff --git a/include/dogecoin/bip39.h b/include/dogecoin/bip39.h index 7ef6a4fd8..24bf7fc30 100644 --- a/include/dogecoin/bip39.h +++ b/include/dogecoin/bip39.h @@ -27,18 +27,6 @@ LIBDOGECOIN_API /* number of words in the language wordlist used for mnemonics */ #define LANG_WORD_CNT 2048 -/* - * Longest token get_custom_words() will accept from a wordlist file, and the - * buffer it reads into. BIP39 words are at most 8 characters; this is far - * wider than any real wordlist and exists only to bound the conversion. - * BIP39_STR() stringifies the length for the scanf field width so the width - * and the buffer size cannot drift apart. - */ -#define BIP39_WORD_MAXLEN 1023 -#define BIP39_WORD_BUFSZ (BIP39_WORD_MAXLEN + 1) -#define BIP39_STR_(x) #x -#define BIP39_STR(x) BIP39_STR_(x) - /* Indicates the number of entropy bits supported */ #define MAX_ENTROPY_BITS 256 @@ -63,6 +51,25 @@ LIBDOGECOIN_API /* Maximum size of a mnemonic phrase string in bytes */ #define MAX_MNEMONIC_STRING_SIZE (MAX_WORDS_IN_MNEMONIC * MAX_CHARS_IN_MNEMONIC_WORD * HEX_CHARS_PER_BYTE) + 1 +/* + * Longest token get_custom_words() will accept from a wordlist file, and the + * buffer it reads into. + * + * This is MAX_CHARS_IN_MNEMONIC_WORD rather than merely "wide enough to read + * safely". get_mnemonic() concatenates wordlist entries into a caller-supplied + * MNEMONIC, which is sized from that same constant. A loader bound any looser + * would accept a word the read survives and the assembly downstream does not, + * so the loader enforces the invariant the rest of the file already assumes. + * Real BIP39 words are at most 8 characters. + * + * BIP39_STR() stringifies the length for the scanf field width, so the width + * and the buffer size cannot drift apart. + */ +#define BIP39_WORD_MAXLEN MAX_CHARS_IN_MNEMONIC_WORD +#define BIP39_WORD_BUFSZ (BIP39_WORD_MAXLEN + 1) +#define BIP39_STR_(x) #x +#define BIP39_STR(x) BIP39_STR_(x) + /* Maximum number of characters in a passphrase */ #define MAX_CHARS_IN_PASSPHRASE 256 diff --git a/test/bip39_tests.c b/test/bip39_tests.c index 386bc3fda..1948b168b 100644 --- a/test/bip39_tests.c +++ b/test/bip39_tests.c @@ -14,6 +14,11 @@ #include #include +#ifndef _WIN32 +#include +#include +#include +#endif #include #include #include @@ -1375,6 +1380,24 @@ void test_bip39() * Without the fix this test does not merely fail -- it corrupts the stack, and * under ASan it aborts with a stack-buffer-overflow in get_custom_words. */ +/* + * Create the scratch wordlist private to this user. plain fopen(path, "w") + * leaves the mode to the process umask, which CodeQL flags and which would let + * another local user rewrite the file between creation and read. + */ +static FILE* wordlist_tmp_open(const char* path) +{ +#ifdef _WIN32 + return fopen(path, "w"); +#else + int fd = open(path, O_CREAT | O_WRONLY | O_TRUNC, S_IRUSR | S_IWUSR); + if (fd < 0) { + return NULL; + } + return fdopen(fd, "w"); +#endif +} + void test_bip39_custom_wordlist_bounds() { #ifdef USE_OPTEE @@ -1398,7 +1421,7 @@ void test_bip39_custom_wordlist_bounds() /* A token far longer than the 1024-byte buffer, written first so the rejection happens before any word is allocated. */ - fp = fopen(path, "w"); + fp = wordlist_tmp_open(path); u_assert_true(fp != NULL); for (i = 0; i < 2000; i++) { fputc('a', fp); @@ -1420,7 +1443,7 @@ void test_bip39_custom_wordlist_bounds() /* A token of exactly the maximum length is still refused, since no BIP39 word is anywhere near it and accepting it would mean the next read silently began mid-token. */ - fp = fopen(path, "w"); + fp = wordlist_tmp_open(path); u_assert_true(fp != NULL); for (i = 0; i < BIP39_WORD_MAXLEN + 1; i++) { fputc('b', fp); @@ -1432,7 +1455,7 @@ void test_bip39_custom_wordlist_bounds() /* Positive control: a well-formed 2048-word file still parses, so the bound did not break legitimate wordlists. */ - fp = fopen(path, "w"); + fp = wordlist_tmp_open(path); u_assert_true(fp != NULL); for (i = 0; i < LANG_WORD_CNT; i++) { fprintf(fp, "abandon\n"); @@ -1448,7 +1471,7 @@ void test_bip39_custom_wordlist_bounds() remove(path); /* A file with too few words is still refused. */ - fp = fopen(path, "w"); + fp = wordlist_tmp_open(path); u_assert_true(fp != NULL); fprintf(fp, "abandon\nability\n"); fclose(fp);