diff --git a/include/dogecoin/bip39.h b/include/dogecoin/bip39.h index 8aa993f82..24bf7fc30 100644 --- a/include/dogecoin/bip39.h +++ b/include/dogecoin/bip39.h @@ -51,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/src/bip39.c b/src/bip39.c index e8c748814..12d3ae8b1 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,9 @@ 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; + size_t wordlen; + char word[BIP39_WORD_BUFSZ]; /* Check that file path is valid */ if (filepath == NULL) { @@ -477,19 +480,42 @@ 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); 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++; } 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..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 @@ -1359,3 +1364,122 @@ 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. + */ +/* + * 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 + /* + * 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; + 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 = wordlist_tmp_open(path); + 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 = wordlist_tmp_open(path); + 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 = wordlist_tmp_open(path); + 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 = wordlist_tmp_open(path); + 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); +#endif /* USE_OPTEE */ +} 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);