From c073d68a2dccba2944e6352bd929f52b307ae297 Mon Sep 17 00:00:00 2001 From: Andrew Kozlik Date: Thu, 18 Apr 2019 14:35:18 +0200 Subject: [PATCH] crypto/shamir: Improve error handling. --- .../modtrezorcrypto/modtrezorcrypto-shamir.h | 16 ++++--- crypto/shamir.c | 41 ++++++++++++++--- crypto/shamir.h | 13 +++--- crypto/tests/test_check.c | 45 ++++++++++++++++--- 4 files changed, 90 insertions(+), 25 deletions(-) diff --git a/core/embed/extmod/modtrezorcrypto/modtrezorcrypto-shamir.h b/core/embed/extmod/modtrezorcrypto/modtrezorcrypto-shamir.h index d2ec88f894..a1589af271 100644 --- a/core/embed/extmod/modtrezorcrypto/modtrezorcrypto-shamir.h +++ b/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]; - const uint8_t *share_values[MAX_SHARE_COUNT]; + uint8_t share_indices[SHAMIR_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, - share_count, value_len); - vstr_cut_tail_bytes(&vstr, vstr_len(&vstr) - value_len); + if (shamir_interpolate((uint8_t *)vstr.buf, x_uint8, share_indices, + share_values, share_count, value_len) != true) { + 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, diff --git a/crypto/shamir.c b/crypto/shamir.c index 21e712faf7..6e25700a5c 100644 --- a/crypto/shamir.c +++ b/crypto/shamir.c @@ -36,8 +36,8 @@ */ #include "shamir.h" -#include #include +#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; } diff --git a/crypto/shamir.h b/crypto/shamir.h index 16a37dc360..2a4242301e 100644 --- a/crypto/shamir.h +++ b/crypto/shamir.h @@ -29,6 +29,7 @@ #ifndef __SHAMIR_H__ #define __SHAMIR_H__ +#include #include #include @@ -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_values: Points to an array of y_1, ... , y_m, where each y_i is an + * share_indices: Points to the array of integers x_1, ... , x_m. + * 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); diff --git a/crypto/tests/test_check.c b/crypto/tests/test_check.c index e9ddb14688..7ca2e59307 100644 --- a/crypto/tests/test_check.c +++ b/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, - vectors[i].share_indices, share_values, - vectors[i].share_count, vectors[i].len); - ck_assert_mem_eq(result, vectors[i].result, vectors[i].len); + ck_assert_int_eq(shamir_interpolate(result, vectors[i].result_index, + vectors[i].share_indices, share_values, + vectors[i].share_count, vectors[i].len), + vectors[i].ret); + if (vectors[i].ret == true) { + ck_assert_mem_eq(result, vectors[i].result, vectors[i].len); + } } } END_TEST