Fix typo in point_add. Rather than writing the answer into the output, it wrote it into some awkwardly-named temporaries. Thanks to Daniel Hirche for reporting this issue! Bug: chromium:825273 (cherry picked from commit 5fca61391822252baf3dc37529ba02f6d7611acf) Change-Id: I315087f5e7414118a6f70bf4aed76325aa4f76a0
diff --git a/crypto/fipsmodule/ec/ec_test.cc b/crypto/fipsmodule/ec/ec_test.cc index 923ce1e..55f4fa2 100644 --- a/crypto/fipsmodule/ec/ec_test.cc +++ b/crypto/fipsmodule/ec/ec_test.cc
@@ -28,8 +28,9 @@ #include <openssl/nid.h> #include <openssl/obj.h> -#include "../bn/internal.h" #include "../../test/test_util.h" +#include "../bn/internal.h" +#include "internal.h" // kECKeyWithoutPublic is an ECPrivateKey with the optional publicKey field @@ -359,7 +360,18 @@ EC_KEY_set_public_key(key.get(), EC_GROUP_get0_generator(p256.get()))); } -class ECCurveTest : public testing::TestWithParam<EC_builtin_curve> {}; +class ECCurveTest : public testing::TestWithParam<EC_builtin_curve> { + public: + const EC_GROUP *group() const { return group_.get(); } + + void SetUp() override { + group_.reset(EC_GROUP_new_by_curve_name(GetParam().nid)); + ASSERT_TRUE(group_); + } + + private: + bssl::UniquePtr<EC_GROUP> group_; +}; TEST_P(ECCurveTest, SetAffine) { // Generate an EC_KEY. @@ -367,9 +379,8 @@ ASSERT_TRUE(key); ASSERT_TRUE(EC_KEY_generate_key(key.get())); - const EC_GROUP *const group = EC_KEY_get0_group(key.get()); - EXPECT_TRUE( - EC_POINT_is_on_curve(group, EC_KEY_get0_public_key(key.get()), nullptr)); + EXPECT_TRUE(EC_POINT_is_on_curve(group(), EC_KEY_get0_public_key(key.get()), + nullptr)); // Get the public key's coordinates. bssl::UniquePtr<BIGNUM> x(BN_new()); @@ -379,30 +390,30 @@ bssl::UniquePtr<BIGNUM> p(BN_new()); ASSERT_TRUE(p); EXPECT_TRUE(EC_POINT_get_affine_coordinates_GFp( - group, EC_KEY_get0_public_key(key.get()), x.get(), y.get(), nullptr)); + group(), EC_KEY_get0_public_key(key.get()), x.get(), y.get(), nullptr)); EXPECT_TRUE( - EC_GROUP_get_curve_GFp(group, p.get(), nullptr, nullptr, nullptr)); + EC_GROUP_get_curve_GFp(group(), p.get(), nullptr, nullptr, nullptr)); // Points on the curve should be accepted. - auto point = bssl::UniquePtr<EC_POINT>(EC_POINT_new(group)); + auto point = bssl::UniquePtr<EC_POINT>(EC_POINT_new(group())); ASSERT_TRUE(point); - EXPECT_TRUE(EC_POINT_set_affine_coordinates_GFp(group, point.get(), x.get(), + EXPECT_TRUE(EC_POINT_set_affine_coordinates_GFp(group(), point.get(), x.get(), y.get(), nullptr)); // Subtract one from |y| to make the point no longer on the curve. EXPECT_TRUE(BN_sub(y.get(), y.get(), BN_value_one())); // Points not on the curve should be rejected. - bssl::UniquePtr<EC_POINT> invalid_point(EC_POINT_new(group)); + bssl::UniquePtr<EC_POINT> invalid_point(EC_POINT_new(group())); ASSERT_TRUE(invalid_point); - EXPECT_FALSE(EC_POINT_set_affine_coordinates_GFp(group, invalid_point.get(), + EXPECT_FALSE(EC_POINT_set_affine_coordinates_GFp(group(), invalid_point.get(), x.get(), y.get(), nullptr)); // Coordinates out of range should be rejected. EXPECT_TRUE(BN_add(y.get(), y.get(), BN_value_one())); EXPECT_TRUE(BN_add(y.get(), y.get(), p.get())); - EXPECT_FALSE(EC_POINT_set_affine_coordinates_GFp(group, invalid_point.get(), + EXPECT_FALSE(EC_POINT_set_affine_coordinates_GFp(group(), invalid_point.get(), x.get(), y.get(), nullptr)); EXPECT_FALSE( EC_KEY_set_public_key_affine_coordinates(key.get(), x.get(), y.get())); @@ -420,57 +431,52 @@ ASSERT_TRUE(key); ASSERT_TRUE(EC_KEY_generate_key(key.get())); - const EC_GROUP *const group = EC_KEY_get0_group(key.get()); - - bssl::UniquePtr<EC_POINT> p1(EC_POINT_new(group)); + bssl::UniquePtr<EC_POINT> p1(EC_POINT_new(group())); ASSERT_TRUE(p1); ASSERT_TRUE(EC_POINT_copy(p1.get(), EC_KEY_get0_public_key(key.get()))); - bssl::UniquePtr<EC_POINT> p2(EC_POINT_new(group)); + bssl::UniquePtr<EC_POINT> p2(EC_POINT_new(group())); ASSERT_TRUE(p2); ASSERT_TRUE(EC_POINT_copy(p2.get(), EC_KEY_get0_public_key(key.get()))); - bssl::UniquePtr<EC_POINT> double_p1(EC_POINT_new(group)); + bssl::UniquePtr<EC_POINT> double_p1(EC_POINT_new(group())); ASSERT_TRUE(double_p1); bssl::UniquePtr<BN_CTX> ctx(BN_CTX_new()); ASSERT_TRUE(ctx); - ASSERT_TRUE(EC_POINT_dbl(group, double_p1.get(), p1.get(), ctx.get())); + ASSERT_TRUE(EC_POINT_dbl(group(), double_p1.get(), p1.get(), ctx.get())); - bssl::UniquePtr<EC_POINT> p1_plus_p2(EC_POINT_new(group)); + bssl::UniquePtr<EC_POINT> p1_plus_p2(EC_POINT_new(group())); ASSERT_TRUE(p1_plus_p2); ASSERT_TRUE( - EC_POINT_add(group, p1_plus_p2.get(), p1.get(), p2.get(), ctx.get())); + EC_POINT_add(group(), p1_plus_p2.get(), p1.get(), p2.get(), ctx.get())); EXPECT_EQ(0, - EC_POINT_cmp(group, double_p1.get(), p1_plus_p2.get(), ctx.get())) + EC_POINT_cmp(group(), double_p1.get(), p1_plus_p2.get(), ctx.get())) << "A+A != 2A"; } TEST_P(ECCurveTest, MulZero) { - bssl::UniquePtr<EC_GROUP> group(EC_GROUP_new_by_curve_name(GetParam().nid)); - ASSERT_TRUE(group); - - bssl::UniquePtr<EC_POINT> point(EC_POINT_new(group.get())); + bssl::UniquePtr<EC_POINT> point(EC_POINT_new(group())); ASSERT_TRUE(point); bssl::UniquePtr<BIGNUM> zero(BN_new()); ASSERT_TRUE(zero); BN_zero(zero.get()); - ASSERT_TRUE(EC_POINT_mul(group.get(), point.get(), zero.get(), nullptr, - nullptr, nullptr)); + ASSERT_TRUE(EC_POINT_mul(group(), point.get(), zero.get(), nullptr, nullptr, + nullptr)); - EXPECT_TRUE(EC_POINT_is_at_infinity(group.get(), point.get())) + EXPECT_TRUE(EC_POINT_is_at_infinity(group(), point.get())) << "g * 0 did not return point at infinity."; // Test that zero times an arbitrary point is also infinity. The generator is // used as the arbitrary point. - bssl::UniquePtr<EC_POINT> generator(EC_POINT_new(group.get())); + bssl::UniquePtr<EC_POINT> generator(EC_POINT_new(group())); ASSERT_TRUE(generator); - ASSERT_TRUE(EC_POINT_mul(group.get(), generator.get(), BN_value_one(), - nullptr, nullptr, nullptr)); - ASSERT_TRUE(EC_POINT_mul(group.get(), point.get(), nullptr, generator.get(), + ASSERT_TRUE(EC_POINT_mul(group(), generator.get(), BN_value_one(), nullptr, + nullptr, nullptr)); + ASSERT_TRUE(EC_POINT_mul(group(), point.get(), nullptr, generator.get(), zero.get(), nullptr)); - EXPECT_TRUE(EC_POINT_is_at_infinity(group.get(), point.get())) + EXPECT_TRUE(EC_POINT_is_at_infinity(group(), point.get())) << "p * 0 did not return point at infinity."; } @@ -480,39 +486,32 @@ // 5.6.2.3.2. (Though all our curves have cofactor one, so this check isn't // useful.) TEST_P(ECCurveTest, MulOrder) { - bssl::UniquePtr<EC_GROUP> group(EC_GROUP_new_by_curve_name(GetParam().nid)); - ASSERT_TRUE(group); - // Test that g × order = ∞. - bssl::UniquePtr<EC_POINT> point(EC_POINT_new(group.get())); + bssl::UniquePtr<EC_POINT> point(EC_POINT_new(group())); ASSERT_TRUE(point); - ASSERT_TRUE(EC_POINT_mul(group.get(), point.get(), - EC_GROUP_get0_order(group.get()), nullptr, nullptr, - nullptr)); + ASSERT_TRUE(EC_POINT_mul(group(), point.get(), EC_GROUP_get0_order(group()), + nullptr, nullptr, nullptr)); - EXPECT_TRUE(EC_POINT_is_at_infinity(group.get(), point.get())) + EXPECT_TRUE(EC_POINT_is_at_infinity(group(), point.get())) << "g * order did not return point at infinity."; // Test that p × order = ∞, for some arbitrary p. bssl::UniquePtr<BIGNUM> forty_two(BN_new()); ASSERT_TRUE(forty_two); ASSERT_TRUE(BN_set_word(forty_two.get(), 42)); - ASSERT_TRUE(EC_POINT_mul(group.get(), point.get(), forty_two.get(), nullptr, + ASSERT_TRUE(EC_POINT_mul(group(), point.get(), forty_two.get(), nullptr, nullptr, nullptr)); - ASSERT_TRUE(EC_POINT_mul(group.get(), point.get(), nullptr, point.get(), - EC_GROUP_get0_order(group.get()), nullptr)); + ASSERT_TRUE(EC_POINT_mul(group(), point.get(), nullptr, point.get(), + EC_GROUP_get0_order(group()), nullptr)); - EXPECT_TRUE(EC_POINT_is_at_infinity(group.get(), point.get())) + EXPECT_TRUE(EC_POINT_is_at_infinity(group(), point.get())) << "p * order did not return point at infinity."; } // Test that |EC_POINT_mul| works with out-of-range scalars. The operation will // not be constant-time, but we'll compute the right answer. TEST_P(ECCurveTest, MulOutOfRange) { - bssl::UniquePtr<EC_GROUP> group(EC_GROUP_new_by_curve_name(GetParam().nid)); - ASSERT_TRUE(group); - - bssl::UniquePtr<BIGNUM> n_minus_one(BN_dup(EC_GROUP_get0_order(group.get()))); + bssl::UniquePtr<BIGNUM> n_minus_one(BN_dup(EC_GROUP_get0_order(group()))); ASSERT_TRUE(n_minus_one); ASSERT_TRUE(BN_sub_word(n_minus_one.get(), 1)); @@ -526,81 +525,74 @@ ASSERT_TRUE(BN_set_word(seven.get(), 7)); bssl::UniquePtr<BIGNUM> ten_n_plus_seven( - BN_dup(EC_GROUP_get0_order(group.get()))); + BN_dup(EC_GROUP_get0_order(group()))); ASSERT_TRUE(ten_n_plus_seven); ASSERT_TRUE(BN_mul_word(ten_n_plus_seven.get(), 10)); ASSERT_TRUE(BN_add_word(ten_n_plus_seven.get(), 7)); - bssl::UniquePtr<EC_POINT> point1(EC_POINT_new(group.get())), - point2(EC_POINT_new(group.get())); + bssl::UniquePtr<EC_POINT> point1(EC_POINT_new(group())), + point2(EC_POINT_new(group())); ASSERT_TRUE(point1); ASSERT_TRUE(point2); - ASSERT_TRUE(EC_POINT_mul(group.get(), point1.get(), n_minus_one.get(), - nullptr, nullptr, nullptr)); - ASSERT_TRUE(EC_POINT_mul(group.get(), point2.get(), minus_one.get(), nullptr, + ASSERT_TRUE(EC_POINT_mul(group(), point1.get(), n_minus_one.get(), nullptr, nullptr, nullptr)); - EXPECT_EQ(0, EC_POINT_cmp(group.get(), point1.get(), point2.get(), nullptr)) + ASSERT_TRUE(EC_POINT_mul(group(), point2.get(), minus_one.get(), nullptr, + nullptr, nullptr)); + EXPECT_EQ(0, EC_POINT_cmp(group(), point1.get(), point2.get(), nullptr)) << "-1 * G and (n-1) * G did not give the same result"; - ASSERT_TRUE(EC_POINT_mul(group.get(), point1.get(), seven.get(), nullptr, - nullptr, nullptr)); - ASSERT_TRUE(EC_POINT_mul(group.get(), point2.get(), ten_n_plus_seven.get(), + ASSERT_TRUE(EC_POINT_mul(group(), point1.get(), seven.get(), nullptr, nullptr, + nullptr)); + ASSERT_TRUE(EC_POINT_mul(group(), point2.get(), ten_n_plus_seven.get(), nullptr, nullptr, nullptr)); - EXPECT_EQ(0, EC_POINT_cmp(group.get(), point1.get(), point2.get(), nullptr)) + EXPECT_EQ(0, EC_POINT_cmp(group(), point1.get(), point2.get(), nullptr)) << "7 * G and (10n + 7) * G did not give the same result"; } // Test that 10×∞ + G = G. TEST_P(ECCurveTest, Mul) { - bssl::UniquePtr<EC_GROUP> group(EC_GROUP_new_by_curve_name(GetParam().nid)); - ASSERT_TRUE(group); - bssl::UniquePtr<EC_POINT> p(EC_POINT_new(group.get())); + bssl::UniquePtr<EC_POINT> p(EC_POINT_new(group())); ASSERT_TRUE(p); - bssl::UniquePtr<EC_POINT> result(EC_POINT_new(group.get())); + bssl::UniquePtr<EC_POINT> result(EC_POINT_new(group())); ASSERT_TRUE(result); bssl::UniquePtr<BIGNUM> n(BN_new()); ASSERT_TRUE(n); - ASSERT_TRUE(EC_POINT_set_to_infinity(group.get(), p.get())); + ASSERT_TRUE(EC_POINT_set_to_infinity(group(), p.get())); ASSERT_TRUE(BN_set_word(n.get(), 10)); // First check that 10×∞ = ∞. - ASSERT_TRUE(EC_POINT_mul(group.get(), result.get(), nullptr, p.get(), n.get(), - nullptr)); - EXPECT_TRUE(EC_POINT_is_at_infinity(group.get(), result.get())); + ASSERT_TRUE( + EC_POINT_mul(group(), result.get(), nullptr, p.get(), n.get(), nullptr)); + EXPECT_TRUE(EC_POINT_is_at_infinity(group(), result.get())); // Now check that 10×∞ + G = G. - const EC_POINT *generator = EC_GROUP_get0_generator(group.get()); - ASSERT_TRUE(EC_POINT_mul(group.get(), result.get(), BN_value_one(), p.get(), + const EC_POINT *generator = EC_GROUP_get0_generator(group()); + ASSERT_TRUE(EC_POINT_mul(group(), result.get(), BN_value_one(), p.get(), n.get(), nullptr)); - EXPECT_EQ(0, EC_POINT_cmp(group.get(), result.get(), generator, nullptr)); + EXPECT_EQ(0, EC_POINT_cmp(group(), result.get(), generator, nullptr)); } -#if !defined(BORINGSSL_SHARED_LIBRARY) TEST_P(ECCurveTest, MulNonMinimal) { - bssl::UniquePtr<EC_GROUP> group(EC_GROUP_new_by_curve_name(GetParam().nid)); - ASSERT_TRUE(group); - bssl::UniquePtr<BIGNUM> forty_two(BN_new()); ASSERT_TRUE(forty_two); ASSERT_TRUE(BN_set_word(forty_two.get(), 42)); // Compute g × 42. - bssl::UniquePtr<EC_POINT> point(EC_POINT_new(group.get())); + bssl::UniquePtr<EC_POINT> point(EC_POINT_new(group())); ASSERT_TRUE(point); - ASSERT_TRUE(EC_POINT_mul(group.get(), point.get(), forty_two.get(), nullptr, + ASSERT_TRUE(EC_POINT_mul(group(), point.get(), forty_two.get(), nullptr, nullptr, nullptr)); // Compute it again with a non-minimal 42, much larger than the scalar. ASSERT_TRUE(bn_resize_words(forty_two.get(), 64)); - bssl::UniquePtr<EC_POINT> point2(EC_POINT_new(group.get())); + bssl::UniquePtr<EC_POINT> point2(EC_POINT_new(group())); ASSERT_TRUE(point2); - ASSERT_TRUE(EC_POINT_mul(group.get(), point2.get(), forty_two.get(), nullptr, + ASSERT_TRUE(EC_POINT_mul(group(), point2.get(), forty_two.get(), nullptr, nullptr, nullptr)); - EXPECT_EQ(0, EC_POINT_cmp(group.get(), point.get(), point2.get(), nullptr)); + EXPECT_EQ(0, EC_POINT_cmp(group(), point.get(), point2.get(), nullptr)); } -#endif // BORINGSSL_SHARED_LIBRARY // Test that EC_KEY_set_private_key rejects invalid values. TEST_P(ECCurveTest, SetInvalidPrivateKey) { @@ -622,40 +614,56 @@ } TEST_P(ECCurveTest, IgnoreOct2PointReturnValue) { - bssl::UniquePtr<EC_GROUP> group(EC_GROUP_new_by_curve_name(GetParam().nid)); - ASSERT_TRUE(group); - bssl::UniquePtr<BIGNUM> forty_two(BN_new()); ASSERT_TRUE(forty_two); ASSERT_TRUE(BN_set_word(forty_two.get(), 42)); // Compute g × 42. - bssl::UniquePtr<EC_POINT> point(EC_POINT_new(group.get())); + bssl::UniquePtr<EC_POINT> point(EC_POINT_new(group())); ASSERT_TRUE(point); - ASSERT_TRUE(EC_POINT_mul(group.get(), point.get(), forty_two.get(), nullptr, + ASSERT_TRUE(EC_POINT_mul(group(), point.get(), forty_two.get(), nullptr, nullptr, nullptr)); // Serialize the point. - size_t serialized_len = - EC_POINT_point2oct(group.get(), point.get(), - POINT_CONVERSION_UNCOMPRESSED, nullptr, 0, nullptr); + size_t serialized_len = EC_POINT_point2oct( + group(), point.get(), POINT_CONVERSION_UNCOMPRESSED, nullptr, 0, nullptr); ASSERT_NE(0u, serialized_len); std::vector<uint8_t> serialized(serialized_len); - ASSERT_EQ(serialized_len, - EC_POINT_point2oct(group.get(), point.get(), - POINT_CONVERSION_UNCOMPRESSED, serialized.data(), - serialized_len, nullptr)); + ASSERT_EQ( + serialized_len, + EC_POINT_point2oct(group(), point.get(), POINT_CONVERSION_UNCOMPRESSED, + serialized.data(), serialized_len, nullptr)); // Create a serialized point that is not on the curve. serialized[serialized_len - 1]++; - ASSERT_FALSE(EC_POINT_oct2point(group.get(), point.get(), serialized.data(), + ASSERT_FALSE(EC_POINT_oct2point(group(), point.get(), serialized.data(), serialized.size(), nullptr)); // After a failure, |point| should have been set to the generator to defend // against code that doesn't check the return value. - ASSERT_EQ(0, EC_POINT_cmp(group.get(), point.get(), - EC_GROUP_get0_generator(group.get()), nullptr)); + ASSERT_EQ(0, EC_POINT_cmp(group(), point.get(), + EC_GROUP_get0_generator(group()), nullptr)); +} + +TEST_P(ECCurveTest, DoubleSpecialCase) { + const EC_POINT *g = EC_GROUP_get0_generator(group()); + + bssl::UniquePtr<EC_POINT> two_g(EC_POINT_new(group())); + ASSERT_TRUE(two_g); + ASSERT_TRUE(EC_POINT_dbl(group(), two_g.get(), g, nullptr)); + + bssl::UniquePtr<EC_POINT> p(EC_POINT_new(group())); + ASSERT_TRUE(p); + ASSERT_TRUE(EC_POINT_mul(group(), p.get(), BN_value_one(), g, BN_value_one(), + nullptr)); + EXPECT_EQ(0, EC_POINT_cmp(group(), p.get(), two_g.get(), nullptr)); + + EC_SCALAR one; + ASSERT_TRUE(ec_bignum_to_scalar(group(), &one, BN_value_one())); + ASSERT_TRUE( + ec_point_mul_scalar_public(group(), p.get(), &one, g, &one, nullptr)); + EXPECT_EQ(0, EC_POINT_cmp(group(), p.get(), two_g.get(), nullptr)); } static std::vector<EC_builtin_curve> AllCurves() {
diff --git a/crypto/fipsmodule/ec/internal.h b/crypto/fipsmodule/ec/internal.h index e1a3e71..ee1eb95 100644 --- a/crypto/fipsmodule/ec/internal.h +++ b/crypto/fipsmodule/ec/internal.h
@@ -180,8 +180,8 @@ // ec_bignum_to_scalar converts |in| to an |EC_SCALAR| and writes it to // |*out|. It returns one on success and zero if |in| is out of range. -int ec_bignum_to_scalar(const EC_GROUP *group, EC_SCALAR *out, - const BIGNUM *in); +OPENSSL_EXPORT int ec_bignum_to_scalar(const EC_GROUP *group, EC_SCALAR *out, + const BIGNUM *in); // ec_bignum_to_scalar_unchecked behaves like |ec_bignum_to_scalar| but does not // check |in| is fully reduced. @@ -204,9 +204,9 @@ // ec_point_mul_scalar_public performs the same computation as // ec_point_mul_scalar. It further assumes that the inputs are public so // there is no concern about leaking their values through timing. -int ec_point_mul_scalar_public(const EC_GROUP *group, EC_POINT *r, - const EC_SCALAR *g_scalar, const EC_POINT *p, - const EC_SCALAR *p_scalar, BN_CTX *ctx); +OPENSSL_EXPORT int ec_point_mul_scalar_public( + const EC_GROUP *group, EC_POINT *r, const EC_SCALAR *g_scalar, + const EC_POINT *p, const EC_SCALAR *p_scalar, BN_CTX *ctx); // ec_compute_wNAF writes the modified width-(w+1) Non-Adjacent Form (wNAF) of // |scalar| to |out| and returns one on success or zero on internal error. |out|
diff --git a/third_party/fiat/p256.c b/third_party/fiat/p256.c index f8ad0bf..82f3608 100644 --- a/third_party/fiat/p256.c +++ b/third_party/fiat/p256.c
@@ -1120,7 +1120,7 @@ limb_t yneq = fe_nz(r); if (!xneq && !yneq && z1nz && z2nz) { - point_double(x_out, y_out, z_out, x1, y1, z1); + point_double(x3, y3, z3, x1, y1, z1); return; }