crypto/shamir: Improve error handling.

core/embed/extmod/modtrezorcrypto/modtrezorcrypto-shamir.h

@@ -23,7 +23,7 @@
 
 #include "shamir.h"
 
-#define MAX_SHARE_COUNT 16
+#define SHAMIR_MAX_SHARE_COUNT 16
 
 /// def interpolate(shares, x) -> bytes:
 ///     '''
@@ -40,12 +40,12 @@ mp_obj_t mod_trezorcrypto_shamir_interpolate(mp_obj_t shares, mp_obj_t x) {
   size_t share_count;
   mp_obj_t *share_items;
   mp_obj_get_array(shares, &share_count, &share_items);
-  if (share_count < 1 || share_count > MAX_SHARE_COUNT) {
+  if (share_count < 1 || share_count > SHAMIR_MAX_SHARE_COUNT) {
     mp_raise_ValueError("Invalid number of shares.");
   }
   uint8_t x_uint8 = trezor_obj_get_uint8(x);
-  uint8_t share_indices[MAX_SHARE_COUNT];
+  uint8_t share_indices[SHAMIR_MAX_SHARE_COUNT];
-  const uint8_t *share_values[MAX_SHARE_COUNT];
+  const uint8_t *share_values[SHAMIR_MAX_SHARE_COUNT];
   size_t value_len = 0;
   for (int i = 0; i < share_count; ++i) {
     mp_obj_t *share;
@@ -66,9 +66,11 @@ mp_obj_t mod_trezorcrypto_shamir_interpolate(mp_obj_t shares, mp_obj_t x) {
   }
   vstr_t vstr;
   vstr_init_len(&vstr, value_len);
-  shamir_interpolate((uint8_t *)vstr.buf, x_uint8, share_indices, share_values,
+  if (shamir_interpolate((uint8_t *)vstr.buf, x_uint8, share_indices,
-                     share_count, value_len);
+                         share_values, share_count, value_len) != true) {
-  vstr_cut_tail_bytes(&vstr, vstr_len(&vstr) - value_len);
+    vstr_clear(&vstr);
+    mp_raise_ValueError("Share indices must be pairwise distinct.");
+  }
   return mp_obj_new_str_from_vstr(&mp_type_bytes, &vstr);
 }
 STATIC MP_DEFINE_CONST_FUN_OBJ_2(mod_trezorcrypto_shamir_interpolate_obj,

crypto/shamir.c

@@ -36,8 +36,8 @@
  */
 
 #include "shamir.h"
-#include <stdio.h>
 #include <string.h>
+#include "memzero.h"
 
 static void bitslice(uint32_t r[8], const uint8_t *x, size_t len) {
   size_t bit_idx, arr_idx;
@@ -254,18 +254,29 @@ static void gf256_inv(uint32_t r[8], uint32_t x[8]) {
   gf256_mul(r, r, y);  // r = x^254
 }
 
-void shamir_interpolate(uint8_t *result, uint8_t result_index,
+bool shamir_interpolate(uint8_t *result, uint8_t result_index,
                         const uint8_t *share_indices,
                         const uint8_t **share_values, uint8_t share_count,
                         size_t len) {
   size_t i, j;
-  uint32_t xs[share_count][8], ys[share_count][8];
   uint32_t x[8];
-  uint32_t denom[8], tmp[8];
+  uint32_t xs[share_count][8];
+  uint32_t ys[share_count][8];
   uint32_t num[8] = {~0}; /* num is the numerator (=1) */
+  uint32_t denom[8];
+  uint32_t tmp[8];
   uint32_t secret[8] = {0};
+  bool ret = true;
 
-  if (len > SHAMIR_MAX_LEN) return;
+  if (len > SHAMIR_MAX_LEN) return false;
+
+  /* The code below assumes that none of the share_indices are equal to
+   * result_index. We need to treat that as a special case. */
+  for (i = 0; i < share_count; i++)
+    if (share_indices[i] == result_index) {
+      memcpy(result, share_values[i], len);
+      return true;
+    }
 
   /* Collect the x and y values */
   for (i = 0; i < share_count; i++) {
@@ -290,10 +301,28 @@ void shamir_interpolate(uint8_t *result, uint8_t result_index,
       gf256_add(tmp, xs[j]);
       gf256_mul(denom, denom, tmp);
     }
+    if ((denom[0] | denom[1] | denom[2] | denom[3] | denom[4] | denom[5] |
+         denom[6] | denom[7]) == 0) {
+      /* The share_indices are not unique. */
+      ret = false;
+      break;
+    }
     gf256_inv(tmp, denom);      /* inverted denominator */
     gf256_mul(tmp, tmp, num);   /* basis polynomial */
     gf256_mul(tmp, tmp, ys[i]); /* scaled coefficient */
     gf256_add(secret, tmp);
   }
-  unbitslice(result, secret, len);
+
+  if (ret == true) {
+    unbitslice(result, secret, len);
+  }
+
+  memzero(x, sizeof(x));
+  memzero(xs, sizeof(xs));
+  memzero(ys, sizeof(ys));
+  memzero(num, sizeof(num));
+  memzero(denom, sizeof(denom));
+  memzero(tmp, sizeof(tmp));
+  memzero(secret, sizeof(secret));
+  return ret;
 }

crypto/shamir.h

@@ -29,6 +29,7 @@
 #ifndef __SHAMIR_H__
 #define __SHAMIR_H__
 
+#include <stdbool.h>
 #include <stddef.h>
 #include <stdint.h>
 
@@ -36,16 +37,18 @@
 
 /*
  * Computes f(x) given the Shamir shares (x_1, f(x_1)), ... , (x_m, f(x_m)).
+ * The x coordinates of the shares must be pairwise distinct. Returns true on
+ * success, otherwise false.
  * result: Array of length len where the evaluations of the polynomials in x
  *     will be written.
  * result_index: The x coordinate of the result.
- * share_indices: Points to an array of integers x_1, ... , x_m.
+ * share_indices: Points to the array of integers x_1, ... , x_m.
- * share_values: Points to an array of y_1, ... , y_m, where each y_i is an
+ * share_values: Points to the array of y_1, ... , y_m, where each y_i is an
  *     array of bytes of length len representing the evaluations of the
  *     polynomials in x_i.
  * share_count: The number of shares m.
- * len: The length of the result array and each of the y_1, ... , y_m arrays.
+ * len: The length of the result array and of each of the y_1, ... , y_m arrays.
-
+ *
  * The number of shares used to compute the result may be larger than the
  * required threshold.
  *
@@ -58,7 +61,7 @@
  * This function treats `shares_values`, `share_indices` and `result` as secret
  * values. `share_count` is treated as a public value (for performance reasons).
  */
-void shamir_interpolate(uint8_t *result, uint8_t result_index,
+bool shamir_interpolate(uint8_t *result, uint8_t result_index,
                         const uint8_t *share_indices,
                         const uint8_t **share_values, uint8_t share_count,
                         size_t len);

crypto/tests/test_check.c

@@ -5067,6 +5067,7 @@ START_TEST(test_shamir) {
     const uint8_t share_values[SHAMIR_MAX_COUNT][SHAMIR_MAX_LEN];
     uint8_t share_count;
     size_t len;
+    bool ret;
   } vectors[] = {{{7,   151, 168, 57,  186, 104, 218, 21, 209, 96,  106,
                    152, 252, 35,  210, 208, 43,  47,  13, 21,  142, 122,
                    24,  42,  149, 192, 95,  24,  240, 24, 148, 110},
@@ -5078,7 +5079,8 @@ START_TEST(test_shamir) {
                        24,  42,  149, 192, 95,  24,  240, 24, 148, 110},
                   },
                   1,
-                  32},
+                  32,
+                  true},
 
                  {{53},
                   255,
@@ -5102,7 +5104,32 @@ START_TEST(test_shamir) {
                       {175},
                   },
                   16,
-                  1},
+                  1,
+                  true},
+
+                 {{91, 188, 226, 91, 254, 197, 225},
+                  1,
+                  {5, 1, 10},
+                  {
+                      {129, 18, 104, 86, 236, 73, 176},
+                      {91, 188, 226, 91, 254, 197, 225},
+                      {69, 53, 151, 204, 224, 37, 19},
+                  },
+                  3,
+                  7,
+                  true},
+
+                 {{0},
+                  255,
+                  {3, 12, 3},
+                  {
+                      {100, 176, 99, 142, 115, 192, 138},
+                      {54, 139, 99, 172, 29, 137, 58},
+                      {216, 119, 222, 40, 87, 25, 147},
+                  },
+                  3,
+                  7,
+                  false},
 
                  {{163, 120, 30, 243, 179, 172, 196, 137, 119, 17},
                   3,
@@ -5111,7 +5138,8 @@ START_TEST(test_shamir) {
                    {121, 9, 79, 98, 132, 164, 9, 165, 19, 230},
                    {86, 52, 173, 138, 189, 223, 122, 102, 248, 157}},
                   3,
-                  10}};
+                  10,
+                  true}};
 
   for (size_t i = 0; i < (sizeof(vectors) / sizeof(*vectors)); ++i) {
     uint8_t result[SHAMIR_MAX_LEN];
@@ -5119,10 +5147,13 @@ START_TEST(test_shamir) {
     for (size_t j = 0; j < vectors[i].share_count; ++j) {
       share_values[j] = vectors[i].share_values[j];
     }
-    shamir_interpolate(result, vectors[i].result_index,
+    ck_assert_int_eq(shamir_interpolate(result, vectors[i].result_index,
-                       vectors[i].share_indices, share_values,
+                                        vectors[i].share_indices, share_values,
-                       vectors[i].share_count, vectors[i].len);
+                                        vectors[i].share_count, vectors[i].len),
-    ck_assert_mem_eq(result, vectors[i].result, vectors[i].len);
+                     vectors[i].ret);
+    if (vectors[i].ret == true) {
+      ck_assert_mem_eq(result, vectors[i].result, vectors[i].len);
+    }
   }
 }
 END_TEST