Use |crypto_word_t| and |size_t| more consistently in ECC scalar recoding. Use |crypto_word_t| as the type for secret values in scalar recoding. Use |size_t| as the type of array indexes in scalar recoding. Use explicit casts where a larger type is (losslessly) truncated to a smaller type. With this change, |uint64_t| is no longer used in the p256.c when building in 32-bit mode, |unsigned| is not used in any of the affected modules, and |uint8_t| and |char| are no longer used for secret values in the ECC recoding. When given the choice of doing non-array-indexing arithmetic (e.g. shifts) on |size_t| values or |crypto_word_t| values, prefer doing it on |crypto_word_t| values. More generally, try to use |size_t| only for sizes and array indexes. This is part of a bigger project to minimize the use of types other than |crypto_word_t| for secret values. This is also part of a larger project make the ECC code more consistent. Avoid changing the loop indexing in the P-256 scalar multiplication from |int| to |size_t|. The P-224 code does use |size_t| but it is less clear than the P-256 code where |i - 1| results in a negative/underflowed value when |i| is zero. Change-Id: I78cb404455c2340a4f8c9688d36c0d425bfcc50b Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/41685 Reviewed-by: David Benjamin <davidben@google.com> Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/fipsmodule/ec/internal.h b/crypto/fipsmodule/ec/internal.h index 836d3ca..18aabb0 100644 --- a/crypto/fipsmodule/ec/internal.h +++ b/crypto/fipsmodule/ec/internal.h
@@ -703,7 +703,8 @@ int ec_GFp_mont_felem_from_bytes(const EC_GROUP *group, EC_FELEM *out, const uint8_t *in, size_t len); -void ec_GFp_nistp_recode_scalar_bits(uint8_t *sign, uint8_t *digit, uint8_t in); +void ec_GFp_nistp_recode_scalar_bits(crypto_word_t *sign, crypto_word_t *digit, + crypto_word_t in); const EC_METHOD *EC_GFp_nistp224_method(void); const EC_METHOD *EC_GFp_nistp256_method(void);
diff --git a/crypto/fipsmodule/ec/p224-64.c b/crypto/fipsmodule/ec/p224-64.c index c106de0..da4308c 100644 --- a/crypto/fipsmodule/ec/p224-64.c +++ b/crypto/fipsmodule/ec/p224-64.c
@@ -866,7 +866,7 @@ } // p224_get_bit returns the |i|th bit in |in| -static char p224_get_bit(const p224_felem_bytearray in, size_t i) { +static crypto_word_t p224_get_bit(const p224_felem_bytearray in, size_t i) { if (i >= 224) { return 0; } @@ -977,13 +977,13 @@ // Add every 5 doublings. if (i % 5 == 0) { - uint64_t bits = p224_get_bit(scalar->bytes, i + 4) << 5; + crypto_word_t bits = p224_get_bit(scalar->bytes, i + 4) << 5; bits |= p224_get_bit(scalar->bytes, i + 3) << 4; bits |= p224_get_bit(scalar->bytes, i + 2) << 3; bits |= p224_get_bit(scalar->bytes, i + 1) << 2; bits |= p224_get_bit(scalar->bytes, i) << 1; bits |= p224_get_bit(scalar->bytes, i - 1); - uint8_t sign, digit; + crypto_word_t sign, digit; ec_GFp_nistp_recode_scalar_bits(&sign, &digit, bits); // Select the point to add or subtract. @@ -1022,7 +1022,7 @@ } // First, look 28 bits upwards. - uint64_t bits = p224_get_bit(scalar->bytes, i + 196) << 3; + crypto_word_t bits = p224_get_bit(scalar->bytes, i + 196) << 3; bits |= p224_get_bit(scalar->bytes, i + 140) << 2; bits |= p224_get_bit(scalar->bytes, i + 84) << 1; bits |= p224_get_bit(scalar->bytes, i + 28); @@ -1080,14 +1080,15 @@ // Add multiples of the generator. if (i <= 27) { // First, look 28 bits upwards. - uint64_t bits = p224_get_bit(g_scalar->bytes, i + 196) << 3; + crypto_word_t bits = p224_get_bit(g_scalar->bytes, i + 196) << 3; bits |= p224_get_bit(g_scalar->bytes, i + 140) << 2; bits |= p224_get_bit(g_scalar->bytes, i + 84) << 1; bits |= p224_get_bit(g_scalar->bytes, i + 28); + size_t index = (size_t)bits; p224_point_add(nq[0], nq[1], nq[2], nq[0], nq[1], nq[2], 1 /* mixed */, - g_p224_pre_comp[1][bits][0], g_p224_pre_comp[1][bits][1], - g_p224_pre_comp[1][bits][2]); + g_p224_pre_comp[1][index][0], g_p224_pre_comp[1][index][1], + g_p224_pre_comp[1][index][2]); assert(!skip); // Second, look at the current position. @@ -1095,20 +1096,21 @@ bits |= p224_get_bit(g_scalar->bytes, i + 112) << 2; bits |= p224_get_bit(g_scalar->bytes, i + 56) << 1; bits |= p224_get_bit(g_scalar->bytes, i); + index = (size_t)bits; p224_point_add(nq[0], nq[1], nq[2], nq[0], nq[1], nq[2], 1 /* mixed */, - g_p224_pre_comp[0][bits][0], g_p224_pre_comp[0][bits][1], - g_p224_pre_comp[0][bits][2]); + g_p224_pre_comp[0][index][0], g_p224_pre_comp[0][index][1], + g_p224_pre_comp[0][index][2]); } // Incorporate |p_scalar| every 5 doublings. if (i % 5 == 0) { - uint64_t bits = p224_get_bit(p_scalar->bytes, i + 4) << 5; + crypto_word_t bits = p224_get_bit(p_scalar->bytes, i + 4) << 5; bits |= p224_get_bit(p_scalar->bytes, i + 3) << 4; bits |= p224_get_bit(p_scalar->bytes, i + 2) << 3; bits |= p224_get_bit(p_scalar->bytes, i + 1) << 2; bits |= p224_get_bit(p_scalar->bytes, i) << 1; bits |= p224_get_bit(p_scalar->bytes, i - 1); - uint8_t sign, digit; + crypto_word_t sign, digit; ec_GFp_nistp_recode_scalar_bits(&sign, &digit, bits); // Select the point to add or subtract.
diff --git a/crypto/fipsmodule/ec/p256-x86_64.c b/crypto/fipsmodule/ec/p256-x86_64.c index 973130f..29ae193 100644 --- a/crypto/fipsmodule/ec/p256-x86_64.c +++ b/crypto/fipsmodule/ec/p256-x86_64.c
@@ -50,8 +50,8 @@ // Recode window to a signed digit, see |ec_GFp_nistp_recode_scalar_bits| in // util.c for details -static unsigned booth_recode_w5(unsigned in) { - unsigned s, d; +static crypto_word_t booth_recode_w5(crypto_word_t in) { + crypto_word_t s, d; s = ~((in >> 5) - 1); d = (1 << 6) - in - 1; @@ -61,8 +61,8 @@ return (d << 1) + (s & 1); } -static unsigned booth_recode_w7(unsigned in) { - unsigned s, d; +static crypto_word_t booth_recode_w7(crypto_word_t in) { + crypto_word_t s, d; s = ~((in >> 7) - 1); d = (1 << 8) - in - 1; @@ -194,8 +194,8 @@ assert(p_scalar != NULL); assert(group->field.width == P256_LIMBS); - static const unsigned kWindowSize = 5; - static const unsigned kMask = (1 << (5 /* kWindowSize */ + 1)) - 1; + static const size_t kWindowSize = 5; + static const crypto_word_t kMask = (1 << (5 /* kWindowSize */ + 1)) - 1; // A |P256_POINT| is (3 * 32) = 96 bytes, and the 64-byte alignment should // add no more than 63 bytes of overhead. Thus, |table| should require @@ -232,17 +232,17 @@ BN_ULONG tmp[P256_LIMBS]; alignas(32) P256_POINT h; - unsigned index = 255; - unsigned wvalue = p_str[(index - 1) / 8]; + size_t index = 255; + crypto_word_t wvalue = p_str[(index - 1) / 8]; wvalue = (wvalue >> ((index - 1) % 8)) & kMask; ecp_nistz256_select_w5(r, table, booth_recode_w5(wvalue) >> 1); while (index >= 5) { if (index != 255) { - unsigned off = (index - 1) / 8; + size_t off = (index - 1) / 8; - wvalue = p_str[off] | p_str[off + 1] << 8; + wvalue = (crypto_word_t)p_str[off] | (crypto_word_t)p_str[off + 1] << 8; wvalue = (wvalue >> ((index - 1) % 8)) & kMask; wvalue = booth_recode_w5(wvalue); @@ -283,21 +283,22 @@ P256_POINT_AFFINE a; } p256_point_union_t; -static unsigned calc_first_wvalue(unsigned *index, const uint8_t p_str[33]) { - static const unsigned kWindowSize = 7; - static const unsigned kMask = (1 << (7 /* kWindowSize */ + 1)) - 1; +static crypto_word_t calc_first_wvalue(size_t *index, const uint8_t p_str[33]) { + static const size_t kWindowSize = 7; + static const crypto_word_t kMask = (1 << (7 /* kWindowSize */ + 1)) - 1; *index = kWindowSize; - unsigned wvalue = (p_str[0] << 1) & kMask; + crypto_word_t wvalue = (p_str[0] << 1) & kMask; return booth_recode_w7(wvalue); } -static unsigned calc_wvalue(unsigned *index, const uint8_t p_str[33]) { - static const unsigned kWindowSize = 7; - static const unsigned kMask = (1 << (7 /* kWindowSize */ + 1)) - 1; +static crypto_word_t calc_wvalue(size_t *index, const uint8_t p_str[33]) { + static const size_t kWindowSize = 7; + static const crypto_word_t kMask = (1 << (7 /* kWindowSize */ + 1)) - 1; - const unsigned off = (*index - 1) / 8; - unsigned wvalue = p_str[off] | p_str[off + 1] << 8; + const size_t off = (*index - 1) / 8; + crypto_word_t wvalue = + (crypto_word_t)p_str[off] | (crypto_word_t)p_str[off + 1] << 8; wvalue = (wvalue >> ((*index - 1) % 8)) & kMask; *index += kWindowSize; @@ -325,8 +326,8 @@ p_str[32] = 0; // First window - unsigned index = 0; - unsigned wvalue = calc_first_wvalue(&index, p_str); + size_t index = 0; + crypto_word_t wvalue = calc_first_wvalue(&index, p_str); ecp_nistz256_select_w7(&p.a, ecp_nistz256_precomputed[0], wvalue >> 1); ecp_nistz256_neg(p.p.Z, p.p.Y); @@ -370,8 +371,8 @@ p_str[32] = 0; // First window - unsigned index = 0; - unsigned wvalue = calc_first_wvalue(&index, p_str); + size_t index = 0; + size_t wvalue = calc_first_wvalue(&index, p_str); // Convert |p| from affine to Jacobian coordinates. We set Z to zero if |p| // is infinity and |ONE| otherwise. |p| was computed from the table, so it
diff --git a/crypto/fipsmodule/ec/p256.c b/crypto/fipsmodule/ec/p256.c index 9355ac1..9f5694c 100644 --- a/crypto/fipsmodule/ec/p256.c +++ b/crypto/fipsmodule/ec/p256.c
@@ -67,7 +67,7 @@ static void fiat_p256_copy(fiat_p256_limb_t out[FIAT_P256_NLIMBS], const fiat_p256_limb_t in1[FIAT_P256_NLIMBS]) { - for (int i = 0; i < FIAT_P256_NLIMBS; i++) { + for (size_t i = 0; i < FIAT_P256_NLIMBS; i++) { out[i] = in1[i]; } } @@ -393,7 +393,7 @@ } // fiat_p256_get_bit returns the |i|th bit in |in| -static char fiat_p256_get_bit(const uint8_t *in, int i) { +static crypto_word_t fiat_p256_get_bit(const uint8_t *in, int i) { if (i < 0 || i >= 256) { return 0; } @@ -498,20 +498,20 @@ // do other additions every 5 doublings if (i % 5 == 0) { - uint64_t bits = fiat_p256_get_bit(scalar->bytes, i + 4) << 5; + crypto_word_t bits = fiat_p256_get_bit(scalar->bytes, i + 4) << 5; bits |= fiat_p256_get_bit(scalar->bytes, i + 3) << 4; bits |= fiat_p256_get_bit(scalar->bytes, i + 2) << 3; bits |= fiat_p256_get_bit(scalar->bytes, i + 1) << 2; bits |= fiat_p256_get_bit(scalar->bytes, i) << 1; bits |= fiat_p256_get_bit(scalar->bytes, i - 1); - uint8_t sign, digit; + crypto_word_t sign, digit; ec_GFp_nistp_recode_scalar_bits(&sign, &digit, bits); // select the point to add or subtract, in constant time. - fiat_p256_select_point(digit, 17, (const fiat_p256_felem(*)[3])p_pre_comp, - tmp); + fiat_p256_select_point((fiat_p256_limb_t)digit, 17, + (const fiat_p256_felem(*)[3])p_pre_comp, tmp); fiat_p256_opp(ftmp, tmp[1]); // (X, -Y, Z) is the negative point. - fiat_p256_cmovznz(tmp[1], sign, tmp[1], ftmp); + fiat_p256_cmovznz(tmp[1], (fiat_p256_limb_t)sign, tmp[1], ftmp); if (!skip) { fiat_p256_point_add(nq[0], nq[1], nq[2], nq[0], nq[1], nq[2], @@ -543,12 +543,13 @@ } // First, look 32 bits upwards. - uint64_t bits = fiat_p256_get_bit(scalar->bytes, i + 224) << 3; + crypto_word_t bits = fiat_p256_get_bit(scalar->bytes, i + 224) << 3; bits |= fiat_p256_get_bit(scalar->bytes, i + 160) << 2; bits |= fiat_p256_get_bit(scalar->bytes, i + 96) << 1; bits |= fiat_p256_get_bit(scalar->bytes, i + 32); // Select the point to add, in constant time. - fiat_p256_select_point_affine(bits, 15, fiat_p256_g_pre_comp[1], tmp); + fiat_p256_select_point_affine((fiat_p256_limb_t)bits, 15, + fiat_p256_g_pre_comp[1], tmp); if (!skip) { fiat_p256_point_add(nq[0], nq[1], nq[2], nq[0], nq[1], nq[2], @@ -566,7 +567,8 @@ bits |= fiat_p256_get_bit(scalar->bytes, i + 64) << 1; bits |= fiat_p256_get_bit(scalar->bytes, i); // Select the point to add, in constant time. - fiat_p256_select_point_affine(bits, 15, fiat_p256_g_pre_comp[0], tmp); + fiat_p256_select_point_affine((fiat_p256_limb_t)bits, 15, + fiat_p256_g_pre_comp[0], tmp); fiat_p256_point_add(nq[0], nq[1], nq[2], nq[0], nq[1], nq[2], 1 /* mixed */, tmp[0], tmp[1], tmp[2]); } @@ -613,14 +615,15 @@ // constant-time lookup. if (i <= 31) { // First, look 32 bits upwards. - uint64_t bits = fiat_p256_get_bit(g_scalar->bytes, i + 224) << 3; + crypto_word_t bits = fiat_p256_get_bit(g_scalar->bytes, i + 224) << 3; bits |= fiat_p256_get_bit(g_scalar->bytes, i + 160) << 2; bits |= fiat_p256_get_bit(g_scalar->bytes, i + 96) << 1; bits |= fiat_p256_get_bit(g_scalar->bytes, i + 32); if (bits != 0) { + size_t index = (size_t)(bits - 1); fiat_p256_point_add(ret[0], ret[1], ret[2], ret[0], ret[1], ret[2], - 1 /* mixed */, fiat_p256_g_pre_comp[1][bits - 1][0], - fiat_p256_g_pre_comp[1][bits - 1][1], + 1 /* mixed */, fiat_p256_g_pre_comp[1][index][0], + fiat_p256_g_pre_comp[1][index][1], fiat_p256_one); skip = 0; } @@ -631,9 +634,10 @@ bits |= fiat_p256_get_bit(g_scalar->bytes, i + 64) << 1; bits |= fiat_p256_get_bit(g_scalar->bytes, i); if (bits != 0) { + size_t index = (size_t)(bits - 1); fiat_p256_point_add(ret[0], ret[1], ret[2], ret[0], ret[1], ret[2], - 1 /* mixed */, fiat_p256_g_pre_comp[0][bits - 1][0], - fiat_p256_g_pre_comp[0][bits - 1][1], + 1 /* mixed */, fiat_p256_g_pre_comp[0][index][0], + fiat_p256_g_pre_comp[0][index][1], fiat_p256_one); skip = 0; } @@ -642,7 +646,7 @@ int digit = p_wNAF[i]; if (digit != 0) { assert(digit & 1); - int idx = digit < 0 ? (-digit) >> 1 : digit >> 1; + size_t idx = (size_t)(digit < 0 ? (-digit) >> 1 : digit >> 1); fiat_p256_felem *y = &p_pre_comp[idx][1], tmp; if (digit < 0) { fiat_p256_opp(tmp, p_pre_comp[idx][1]);
diff --git a/crypto/fipsmodule/ec/simple_mul.c b/crypto/fipsmodule/ec/simple_mul.c index 127e2b3..0e6384e 100644 --- a/crypto/fipsmodule/ec/simple_mul.c +++ b/crypto/fipsmodule/ec/simple_mul.c
@@ -108,7 +108,7 @@ if (i > 0) { window |= bn_is_bit_set_words(scalar->words, width, i - 1); } - uint8_t sign, digit; + crypto_word_t sign, digit; ec_GFp_nistp_recode_scalar_bits(&sign, &digit, window); // Select the entry in constant-time. @@ -121,7 +121,7 @@ // Negate if necessary. EC_FELEM neg_Y; ec_felem_neg(group, &neg_Y, &out->Y); - BN_ULONG sign_mask = sign; + crypto_word_t sign_mask = sign; sign_mask = 0u - sign_mask; ec_felem_select(group, &out->Y, sign_mask, &neg_Y, &out->Y); }
diff --git a/crypto/fipsmodule/ec/util.c b/crypto/fipsmodule/ec/util.c index 4f39f18..c4323f2 100644 --- a/crypto/fipsmodule/ec/util.c +++ b/crypto/fipsmodule/ec/util.c
@@ -240,9 +240,9 @@ // P-384: ...01110011; w = 2, 5, 6, 7 are okay // P-256: ...01010001; w = 5, 7 are okay // P-224: ...00111101; w = 3, 4, 5, 6 are okay -void ec_GFp_nistp_recode_scalar_bits(uint8_t *sign, uint8_t *digit, - uint8_t in) { - uint8_t s, d; +void ec_GFp_nistp_recode_scalar_bits(crypto_word_t *sign, crypto_word_t *digit, + crypto_word_t in) { + crypto_word_t s, d; s = ~((in >> 5) - 1); /* sets all bits to MSB(in), 'in' seen as * 6-bit value */