From 718c88c961d2ab93c9a0b43ed8b6720cf5d5b512 Mon Sep 17 00:00:00 2001 From: David Benjamin Date: Fri, 23 Mar 2018 16:32:48 -0400 Subject: [PATCH] Fix a bug in p224-64.c. p224_felem_neg does not produce an output within the tight bounds suitable for p224_felem_contract. This was found by inspection of the code. This only affects the final y-coordinate output of arbitrary-point multiplication, so it is a no-op for ECDH and ECDSA. Change-Id: I1d929458d1f21d02cd8e745d2f0f7040a6bb0627 Reviewed-on: https://boringssl-review.googlesource.com/26847 Commit-Queue: David Benjamin Commit-Queue: Adam Langley Reviewed-by: Adam Langley CQ-Verified: CQ bot account: commit-bot@chromium.org --- crypto/fipsmodule/ec/ec_test.cc | 23 +++++++++++++++++++++++ crypto/fipsmodule/ec/p224-64.c | 26 +++++++++----------------- 2 files changed, 32 insertions(+), 17 deletions(-) diff --git a/crypto/fipsmodule/ec/ec_test.cc b/crypto/fipsmodule/ec/ec_test.cc index 0ac2c5bc..d2fc2a55 100644 --- a/crypto/fipsmodule/ec/ec_test.cc +++ b/crypto/fipsmodule/ec/ec_test.cc @@ -674,6 +674,29 @@ TEST_P(ECCurveTest, DoubleSpecialCase) { EXPECT_EQ(0, EC_POINT_cmp(group(), p.get(), two_g.get(), nullptr)); } +// This a regression test for a P-224 bug, but we may as well run it for all +// curves. +TEST_P(ECCurveTest, P224Bug) { + // P = -G + const EC_POINT *g = EC_GROUP_get0_generator(group()); + bssl::UniquePtr p(EC_POINT_dup(g, group())); + ASSERT_TRUE(p); + ASSERT_TRUE(EC_POINT_invert(group(), p.get(), nullptr)); + + // Compute 31 * P + 32 * G = G + bssl::UniquePtr ret(EC_POINT_new(group())); + ASSERT_TRUE(ret); + bssl::UniquePtr bn31(BN_new()), bn32(BN_new()); + ASSERT_TRUE(bn31); + ASSERT_TRUE(bn32); + ASSERT_TRUE(BN_set_word(bn31.get(), 31)); + ASSERT_TRUE(BN_set_word(bn32.get(), 32)); + ASSERT_TRUE(EC_POINT_mul(group(), ret.get(), bn32.get(), p.get(), bn31.get(), + nullptr)); + + EXPECT_EQ(0, EC_POINT_cmp(group(), ret.get(), g, nullptr)); +} + static std::vector AllCurves() { const size_t num_curves = EC_get_builtin_curves(nullptr, 0); std::vector curves(num_curves); diff --git a/crypto/fipsmodule/ec/p224-64.c b/crypto/fipsmodule/ec/p224-64.c index 71a8af0a..028197b6 100644 --- a/crypto/fipsmodule/ec/p224-64.c +++ b/crypto/fipsmodule/ec/p224-64.c @@ -257,23 +257,6 @@ static void p224_felem_sum(p224_felem out, const p224_felem in) { out[3] += in[3]; } -// Get negative value: out = -in -// Assumes in[i] < 2^57 -static void p224_felem_neg(p224_felem out, const p224_felem in) { - static const p224_limb two58p2 = - (((p224_limb)1) << 58) + (((p224_limb)1) << 2); - static const p224_limb two58m2 = - (((p224_limb)1) << 58) - (((p224_limb)1) << 2); - static const p224_limb two58m42m2 = - (((p224_limb)1) << 58) - (((p224_limb)1) << 42) - (((p224_limb)1) << 2); - - // Set to 0 mod 2^224-2^96+1 to ensure out > in - out[0] = two58p2 - in[0]; - out[1] = two58m42m2 - in[1]; - out[2] = two58m2 - in[2]; - out[3] = two58m2 - in[3]; -} - // Subtract field elements: out -= in // Assumes in[i] < 2^57 static void p224_felem_diff(p224_felem out, const p224_felem in) { @@ -513,6 +496,15 @@ static void p224_felem_contract(p224_felem out, const p224_felem in) { out[3] = tmp[3]; } +// Get negative value: out = -in +// Requires in[i] < 2^63, +// ensures out[0] < 2^56, out[1] < 2^56, out[2] < 2^56, out[3] <= 2^56 + 2^16 +static void p224_felem_neg(p224_felem out, const p224_felem in) { + p224_widefelem tmp = {0}; + p224_felem_diff_128_64(tmp, in); + p224_felem_reduce(out, tmp); +} + // Zero-check: returns 1 if input is 0, and 0 otherwise. We know that field // elements are reduced to in < 2^225, so we only need to check three cases: 0, // 2^224 - 2^96 + 1, and 2^225 - 2^97 + 2