commit 2044181e01261b2790dc83a5cc6bbc40e61f4385
author Adam Langley <alangley@gmail.com> Sun Feb 04 11:20:14 2018 -0800
committer CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Mon Feb 05 02:03:29 2018 +0000
tree c12e64871091630e5d807098e18623a13af65705
parent a3123910509c5ae5f8b8475bdeed1944a5a42fab

Set output point to the generator when not on the curve.

Processing off-curve points is sufficiently dangerous to worry about
code that doesn't check the return value of
|EC_POINT_set_affine_coordinates| and |EC_POINT_oct2point|. While we
have integrated on-curve checks into these functions, code that ignores
the return value will still be able to work with an invalid point
because it's already been installed in the output by the time the check
is done.

Instead, in the event of an off-curve point, set the output point to the
generator, which is certainly on the curve and hopefully safe.

Change-Id: Ibc73dceb2d8d21920e07c4f6def2c8249cb78ca0
Reviewed-on: https://boringssl-review.googlesource.com/25724
Commit-Queue: David Benjamin <davidben@google.com>
Reviewed-by: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>

crypto/fipsmodule/ec/ec.c

diff --git a/crypto/fipsmodule/ec/ec.c b/crypto/fipsmodule/ec/ec.c
index b55beb6..616df16 100644
--- a/crypto/fipsmodule/ec/ec.c
+++ b/crypto/fipsmodule/ec/ec.c
@@ -766,6 +766,9 @@
   }
 
   if (!EC_POINT_is_on_curve(group, point, ctx)) {
+    // In the event of an error, defend against the caller not checking the
+    // return value by setting a known safe value: the base point.
+    EC_POINT_copy(point, EC_GROUP_get0_generator(group));
     OPENSSL_PUT_ERROR(EC, EC_R_POINT_IS_NOT_ON_CURVE);
     return 0;
   }

crypto/fipsmodule/ec/ec_test.cc

diff --git a/crypto/fipsmodule/ec/ec_test.cc b/crypto/fipsmodule/ec/ec_test.cc
index 85dc8f2..25c7783 100644
--- a/crypto/fipsmodule/ec/ec_test.cc
+++ b/crypto/fipsmodule/ec/ec_test.cc
@@ -599,6 +599,41 @@
   ERR_clear_error();
 }
 
+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()));
+  ASSERT_TRUE(point);
+  ASSERT_TRUE(EC_POINT_mul(group.get(), 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);
+  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));
+
+  // 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(),
+                                  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));
+}
+
 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);