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

@@ -766,6 +766,9 @@ int EC_POINT_set_affine_coordinates_GFp(const EC_GROUP *group, EC_POINT *point,
   }
 
   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

@@ -599,6 +599,41 @@ TEST_P(ECCurveTest, SetInvalidPrivateKey) {
   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);