From 7fc185127971767062eca71d4335288fab85e6b4 Mon Sep 17 00:00:00 2001 From: Andrew Kozlik Date: Wed, 2 Dec 2020 17:05:16 +0100 Subject: [PATCH] fix(crypto): Improve naming and robustness of SLIP39's T9 mask lookup. --- .../modtrezorcrypto/modtrezorcrypto-slip39.h | 7 +- crypto/slip39.c | 99 ++++++++++--------- crypto/slip39.h | 7 +- crypto/tests/test_check.c | 79 +++++++++------ 4 files changed, 107 insertions(+), 85 deletions(-) diff --git a/core/embed/extmod/modtrezorcrypto/modtrezorcrypto-slip39.h b/core/embed/extmod/modtrezorcrypto/modtrezorcrypto-slip39.h index 9adef554e..a8f3d01c1 100644 --- a/core/embed/extmod/modtrezorcrypto/modtrezorcrypto-slip39.h +++ b/core/embed/extmod/modtrezorcrypto/modtrezorcrypto-slip39.h @@ -54,11 +54,10 @@ STATIC mp_obj_t mod_trezorcrypto_slip39_button_sequence_to_word(mp_obj_t _prefix) { uint16_t prefix = mp_obj_get_int(_prefix); - if (prefix < 1 || prefix > 9999) { - mp_raise_ValueError( - "Invalid button prefix (range between 1 and 9999 is allowed)"); - } const char *word = button_sequence_to_word(prefix); + if (word == NULL) { + mp_raise_ValueError("Invalid button prefix"); + } return mp_obj_new_str_copy(&mp_type_str, (const uint8_t *)word, strlen(word)); } STATIC MP_DEFINE_CONST_FUN_OBJ_1( diff --git a/crypto/slip39.c b/crypto/slip39.c index 9258e637f..b9bd5c3f4 100644 --- a/crypto/slip39.c +++ b/crypto/slip39.c @@ -27,13 +27,21 @@ #include #include "slip39_wordlist.h" +static uint16_t find(uint16_t prefix, bool find_index); + /** - * Returns word on position `index`. + * Returns word at position `index`. */ -const char* get_word(uint16_t index) { return wordlist[index]; } +const char* get_word(uint16_t index) { + if (index >= WORDS_COUNT) { + return NULL; + } + + return wordlist[index]; +} /** - * Finds index of given word, if found. + * Finds the index of a given word. * Returns true on success and stores result in `index`. */ bool word_index(uint16_t* index, const char* word, uint8_t word_length) { @@ -57,80 +65,77 @@ bool word_index(uint16_t* index, const char* word, uint8_t word_length) { } /** - * Calculates which buttons still can be pressed after some already were. - * Returns a 9-bit bitmask, where each bit specifies which buttons - * can be further pressed (there are still words in this combination). - * LSB denotes first button. + * Calculates which buttons on the T9 keyboard can still be pressed after the + * prefix was entered. Returns a 9-bit bitmask, where each bit specifies which + * buttons can be pressed (there are still words in this combination). The least + * significant bit corresponds to the first button. * * Example: 110000110 - second, third, eighth and ninth button still can be * pressed. */ -uint16_t compute_mask(uint16_t prefix) { return find(prefix, false); } +uint16_t slip39_word_completion_mask(uint16_t prefix) { + return find(prefix, false); +} /** - * Converts sequence to word index. + * Returns the first word matching the button sequence prefix or NULL if no + * match is found. */ const char* button_sequence_to_word(uint16_t prefix) { - return wordlist[find(prefix, true)]; + return get_word(find(prefix, true)); } /** * Computes mask if find_index is false. - * Finds the first word index that suits the prefix otherwise. + * Otherwise finds the first word index that matches the prefix. Returns + * WORDS_COUNT if no match is found. */ -uint16_t find(uint16_t prefix, bool find_index) { - uint16_t min = prefix; - uint16_t max = 0; - uint16_t for_max = 0; - uint8_t multiplier = 0; - uint8_t divider = 0; - uint16_t bitmap = 0; - uint8_t digit = 0; - uint16_t i = 0; +static uint16_t find(uint16_t prefix, bool find_index) { + if (prefix == 0) { + return find_index ? 0 : 0x1ff; + } - max = prefix + 1; - while (min < 1000) { - min = min * 10; - max = max * 10; - multiplier++; + // Determine the range of sequences [min, max), which have the given prefix. + uint16_t min = prefix; + uint16_t max = prefix + 1; + uint16_t divider = 1; + while (max <= 1000) { + min *= 10; + max *= 10; + divider *= 10; } + divider /= 10; // Four char prefix -> the mask is zero - if (!multiplier && !find_index) { + if (!divider && !find_index) { return 0; } - for_max = min - (min % 1000) + 1000; // We can't use binary search because the numbers are not sorted. // They are sorted using the words' alphabet (so we can use the index). // Example: axle (1953), beam (1315) - // The first digit is sorted so we can loop only upto `for_max`. - while (words_button_seq[i] < for_max) { + // The first digit is sorted so we only need to search up to `max_search`. + uint16_t max_search = min - (min % 1000) + 1000; + uint16_t bitmap = 0; + for (uint16_t i = 0; i < WORDS_COUNT; i++) { + if (words_button_seq[i] >= max_search) { + break; + } + if (words_button_seq[i] >= min && words_button_seq[i] < max) { if (find_index) { return i; } - switch (multiplier) { - case 1: - divider = 1; - break; - case 2: - divider = 10; - break; - case 3: - divider = 100; - break; - default: - divider = 1; - break; - } - - digit = (words_button_seq[i] / divider) % 10; + uint8_t digit = (words_button_seq[i] / divider) % 10; bitmap |= 1 << (digit - 1); } - i++; } - return bitmap; + if (find_index) { + // Index not found. + return WORDS_COUNT; + } else { + return bitmap; + } } diff --git a/crypto/slip39.h b/crypto/slip39.h index ba314303a..0bc24fa74 100644 --- a/crypto/slip39.h +++ b/crypto/slip39.h @@ -22,6 +22,9 @@ * OTHER DEALINGS IN THE SOFTWARE. */ +#ifndef __SLIP39_H__ +#define __SLIP39_H__ + #include #include @@ -29,8 +32,8 @@ const char* get_word(uint16_t index); bool word_index(uint16_t* index, const char* word, uint8_t word_length); -uint16_t compute_mask(uint16_t prefix); +uint16_t slip39_word_completion_mask(uint16_t prefix); const char* button_sequence_to_word(uint16_t prefix); -uint16_t find(uint16_t prefix, bool find_index); +#endif diff --git a/crypto/tests/test_check.c b/crypto/tests/test_check.c index 5bace3c96..cd99b2c93 100644 --- a/crypto/tests/test_check.c +++ b/crypto/tests/test_check.c @@ -5263,36 +5263,50 @@ START_TEST(test_slip39_word_index) { } END_TEST -START_TEST(test_slip39_compute_mask) { +START_TEST(test_slip39_word_completion_mask) { static const struct { const uint16_t prefix; const uint16_t expected_mask; - } vectors[] = {{ - 12, - 0xFD // 011111101 - }, - { - 21, - 0xF8 // 011111000 - }, - { - 75, - 0xAD // 010101101 - }, - { - 4, - 0x1F7 // 111110111 - }, - { - 738, - 0x6D // 001101101 - }, - { - 9, - 0x6D // 001101101 - }}; + } vectors[] = { + { + 12, + 0xFD // 011111101 + }, + { + 21, + 0xF8 // 011111000 + }, + { + 75, + 0xAD // 010101101 + }, + { + 4, + 0x1F7 // 111110111 + }, + { + 738, + 0x6D // 001101101 + }, + { + 9, + 0x6D // 001101101 + }, + { + 0, + 0x1FF // 111111111 + }, + { + 9999, + 0x00 // 000000000 + }, + { + 20000, + 0x00 // 000000000 + }, + }; for (size_t i = 0; i < (sizeof(vectors) / sizeof(*vectors)); i++) { - uint16_t mask = compute_mask(vectors[i].prefix); + uint16_t mask = slip39_word_completion_mask(vectors[i].prefix); ck_assert_int_eq(mask, vectors[i].expected_mask); } } @@ -5302,15 +5316,16 @@ START_TEST(test_slip39_sequence_to_word) { static const struct { const uint16_t prefix; const char *expected_word; - } vectors[] = {{7945, "swimming"}, - {646, "photo"}, - {5, "kernel"}, - {34, "either"}, - {62, "ocean"}}; + } vectors[] = { + {7945, "swimming"}, {646, "photo"}, {5, "kernel"}, + {34, "either"}, {62, "ocean"}, {0, "academic"}, + }; for (size_t i = 0; i < (sizeof(vectors) / sizeof(*vectors)); i++) { const char *word = button_sequence_to_word(vectors[i].prefix); ck_assert_str_eq(word, vectors[i].expected_word); } + ck_assert_ptr_eq(button_sequence_to_word(9999), NULL); + ck_assert_ptr_eq(button_sequence_to_word(20000), NULL); } END_TEST @@ -8904,7 +8919,7 @@ Suite *test_suite(void) { tc = tcase_create("slip39"); tcase_add_test(tc, test_slip39_get_word); tcase_add_test(tc, test_slip39_word_index); - tcase_add_test(tc, test_slip39_compute_mask); + tcase_add_test(tc, test_slip39_word_completion_mask); tcase_add_test(tc, test_slip39_sequence_to_word); suite_add_tcase(s, tc);