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 <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>
Reviewed-by: Adam Langley <agl@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
diff --git a/crypto/fipsmodule/ec/ec_test.cc b/crypto/fipsmodule/ec/ec_test.cc
index 0ac2c5b..d2fc2a5 100644
--- a/crypto/fipsmodule/ec/ec_test.cc
+++ b/crypto/fipsmodule/ec/ec_test.cc
@@ -674,6 +674,29 @@
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<EC_POINT> 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<EC_POINT> ret(EC_POINT_new(group()));
+ ASSERT_TRUE(ret);
+ bssl::UniquePtr<BIGNUM> 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<EC_builtin_curve> AllCurves() {
const size_t num_curves = EC_get_builtin_curves(nullptr, 0);
std::vector<EC_builtin_curve> curves(num_curves);
diff --git a/crypto/fipsmodule/ec/p224-64.c b/crypto/fipsmodule/ec/p224-64.c
index 71a8af0..028197b 100644
--- a/crypto/fipsmodule/ec/p224-64.c
+++ b/crypto/fipsmodule/ec/p224-64.c
@@ -257,23 +257,6 @@
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 @@
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
