fix(crypto): Improve naming and robustness of SLIP39's T9 mask lookup.

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(

crypto/slip39.c

@@ -27,13 +27,21 @@
 #include <string.h>
 #include "slip39_wordlist.h"
 
-/**
+static uint16_t find(uint16_t prefix, bool find_index);
- * Returns word on position `index`.
- */
-const char* get_word(uint16_t index) { return wordlist[index]; }
 
 /**
- * Finds index of given word, if found.
+ * Returns word at position `index`.
+ */
+const char* get_word(uint16_t index) {
+  if (index >= WORDS_COUNT) {
+    return NULL;
+  }
+
+  return wordlist[index];
+}
+
+/**
+ * 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.
+ * Calculates which buttons on the T9 keyboard can still be pressed after the
- * Returns a 9-bit bitmask, where each bit specifies which buttons
+ * prefix was entered. Returns a 9-bit bitmask, where each bit specifies which
- * can be further pressed (there are still words in this combination).
+ * buttons can be pressed (there are still words in this combination). The least
- * LSB denotes first button.
+ * 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) {
+static uint16_t find(uint16_t prefix, bool find_index) {
-  uint16_t min = prefix;
+  if (prefix == 0) {
-  uint16_t max = 0;
+    return find_index ? 0 : 0x1ff;
-  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;
-
-  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`.
+  // The first digit is sorted so we only need to search up to `max_search`.
-  while (words_button_seq[i] < for_max) {
+  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) {
+      uint8_t digit = (words_button_seq[i] / divider) % 10;
-        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;
       bitmap |= 1 << (digit - 1);
     }
-    i++;
   }
 
-  return bitmap;
+  if (find_index) {
+    // Index not found.
+    return WORDS_COUNT;
+  } else {
+    return bitmap;
+  }
 }

crypto/slip39.h

@@ -22,6 +22,9 @@
  * OTHER DEALINGS IN THE SOFTWARE.
  */
 
+#ifndef __SLIP39_H__
+#define __SLIP39_H__
+
 #include <stdbool.h>
 #include <stdint.h>
 
@@ -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

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[] = {{
+  } vectors[] = {
-                     12,
+      {
-                     0xFD  // 011111101
+          12,
-                 },
+          0xFD  // 011111101
-                 {
+      },
-                     21,
+      {
-                     0xF8  // 011111000
+          21,
-                 },
+          0xF8  // 011111000
-                 {
+      },
-                     75,
+      {
-                     0xAD  // 010101101
+          75,
-                 },
+          0xAD  // 010101101
-                 {
+      },
-                     4,
+      {
-                     0x1F7  // 111110111
+          4,
-                 },
+          0x1F7  // 111110111
-                 {
+      },
-                     738,
+      {
-                     0x6D  // 001101101
+          738,
-                 },
+          0x6D  // 001101101
-                 {
+      },
-                     9,
+      {
-                     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"},
+  } vectors[] = {
-                 {646, "photo"},
+      {7945, "swimming"}, {646, "photo"}, {5, "kernel"},
-                 {5, "kernel"},
+      {34, "either"},     {62, "ocean"},  {0, "academic"},
-                 {34, "either"},
+  };
-                 {62, "ocean"}};
   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);