bip39, qr: bound the wordlist token read; don't rely on assert for indexing

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
 

src/bip39.c

@@ -25,6 +25,7 @@
  * OTHER DEALINGS IN THE SOFTWARE.
  */
 
+#include <ctype.h>
 #include <stdio.h>
 #include <bip39/index.h>
 #include <dogecoin/bip39.h>
@@ -463,7 +464,9 @@
 #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 @@
         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++;
     }
 
@@ -680,7 +706,7 @@
             }
             else {
                 /* Concatenate the word from the wordlist to the mnemonic */
                 strcat(mnemonic, word);
 
                 /* update mnemonic_size with the length of the mnemonic */
                 *mnemonic_size += strlen(word);

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];

test/bip39_tests.c

@@ -14,6 +14,11 @@
 #include <dogecoin/mem.h>
 #include <dogecoin/utils.h>
 
+#ifndef _WIN32
+#include <fcntl.h>
+#include <sys/stat.h>
+#include <unistd.h>
+#endif
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
@@ -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 */
+}

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);