Skip the field inversion when just measuring output size.

https://boringssl-review.googlesource.com/c/boringssl/+/41084
inadvertently added a somewhat expensive operation (field inversion) in
the path of EC_POINT_point2oct when passed with buf == NULL. The result
is a caller that calls the function twice, first to measure and then to
serialize, actually ends up doing the field inversion twice.

Fix this by removing the dual-use calling convention from the internal
function and just have a separate function to measure the output size
separately. It's slightly subtle because EC_POINT_point2oct would check
for the point at infinity by way of converting to affine coordinates, so
we do need to repeat that check.

As part of this, add a unit test for
https://boringssl-review.googlesource.com/6488, which rejected the point
at infinity way back.

Change-Id: I3b6c0f95cced9c00489386f064a2c3f0bb1776f8
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/55065
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>

crypto/fipsmodule/ec/ec_test.cc

@@ -891,6 +891,29 @@ TEST_P(ECCurveTest, GPlusMinusG) {
   EXPECT_TRUE(EC_POINT_is_at_infinity(group(), sum.get()));
 }
 
+// Test that we refuse to encode or decode the point at infinity.
+TEST_P(ECCurveTest, EncodeInfinity) {
+  // The point at infinity is encoded as a single zero byte, but we do not
+  // support it.
+  static const uint8_t kInfinity[] = {0};
+  bssl::UniquePtr<EC_POINT> inf(EC_POINT_new(group()));
+  ASSERT_TRUE(inf);
+  EXPECT_FALSE(EC_POINT_oct2point(group(), inf.get(), kInfinity,
+                                  sizeof(kInfinity), nullptr));
+
+  // Encoding it also fails.
+  ASSERT_TRUE(EC_POINT_set_to_infinity(group(), inf.get()));
+  uint8_t buf[128];
+  EXPECT_EQ(
+      0u, EC_POINT_point2oct(group(), inf.get(), POINT_CONVERSION_UNCOMPRESSED,
+                             buf, sizeof(buf), nullptr));
+
+  // Measuring the length of the encoding also fails.
+  EXPECT_EQ(
+      0u, EC_POINT_point2oct(group(), inf.get(), POINT_CONVERSION_UNCOMPRESSED,
+                             nullptr, 0, 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);

crypto/fipsmodule/ec/internal.h

@@ -439,11 +439,18 @@ int ec_get_x_coordinate_as_bytes(const EC_GROUP *group, uint8_t *out,
                                  size_t *out_len, size_t max_out,
                                  const EC_RAW_POINT *p);
 
-// ec_point_to_bytes behaves like |EC_POINT_point2oct| but takes an
-// |EC_AFFINE|.
+// ec_point_byte_len returns the number of bytes in the byte representation of
+// a non-infinity point in |group|, encoded according to |form|, or zero if
+// |form| is invalid.
+size_t ec_point_byte_len(const EC_GROUP *group, point_conversion_form_t form);
+
+// ec_point_to_bytes encodes |point| according to |form| and writes the result
+// |buf|. It returns the size of the output on success or zero on error. At most
+// |max_out| bytes will be written. The buffer should be at least
+// |ec_point_byte_len| long to guarantee success.
 size_t ec_point_to_bytes(const EC_GROUP *group, const EC_AFFINE *point,
                          point_conversion_form_t form, uint8_t *buf,
-                         size_t len);
+                         size_t max_out);
 
 // ec_point_from_uncompressed parses |in| as a point in uncompressed form and
 // sets the result to |out|. It returns one on success and zero if the input was

crypto/fipsmodule/ec/oct.c

@@ -73,9 +73,7 @@
 #include "internal.h"
 
 
-size_t ec_point_to_bytes(const EC_GROUP *group, const EC_AFFINE *point,
-                         point_conversion_form_t form, uint8_t *buf,
-                         size_t len) {
+size_t ec_point_byte_len(const EC_GROUP *group, point_conversion_form_t form) {
   if (form != POINT_CONVERSION_COMPRESSED &&
       form != POINT_CONVERSION_UNCOMPRESSED) {
     OPENSSL_PUT_ERROR(EC, EC_R_INVALID_FORM);
@@ -88,27 +86,30 @@ size_t ec_point_to_bytes(const EC_GROUP *group, const EC_AFFINE *point,
     // Uncompressed points have a second coordinate.
     output_len += field_len;
   }
+  return output_len;
+}
 
-  // if 'buf' is NULL, just return required length
-  if (buf != NULL) {
-    if (len < output_len) {
-      OPENSSL_PUT_ERROR(EC, EC_R_BUFFER_TOO_SMALL);
-      return 0;
-    }
+size_t ec_point_to_bytes(const EC_GROUP *group, const EC_AFFINE *point,
+                         point_conversion_form_t form, uint8_t *buf,
+                         size_t len) {
+  size_t output_len = ec_point_byte_len(group, form);
+  if (len < output_len) {
+    OPENSSL_PUT_ERROR(EC, EC_R_BUFFER_TOO_SMALL);
+    return 0;
+  }
 
-    size_t field_len_out;
-    ec_felem_to_bytes(group, buf + 1, &field_len_out, &point->X);
-    assert(field_len_out == field_len);
+  size_t field_len;
+  ec_felem_to_bytes(group, buf + 1, &field_len, &point->X);
+  assert(field_len == BN_num_bytes(&group->field));
 
-    if (form == POINT_CONVERSION_UNCOMPRESSED) {
-      ec_felem_to_bytes(group, buf + 1 + field_len, &field_len_out, &point->Y);
-      assert(field_len_out == field_len);
-      buf[0] = form;
-    } else {
-      uint8_t y_buf[EC_MAX_BYTES];
-      ec_felem_to_bytes(group, y_buf, &field_len_out, &point->Y);
-      buf[0] = form + (y_buf[field_len_out - 1] & 1);
-    }
+  if (form == POINT_CONVERSION_UNCOMPRESSED) {
+    ec_felem_to_bytes(group, buf + 1 + field_len, &field_len, &point->Y);
+    assert(field_len == BN_num_bytes(&group->field));
+    buf[0] = form;
+  } else {
+    uint8_t y_buf[EC_MAX_BYTES];
+    ec_felem_to_bytes(group, y_buf, &field_len, &point->Y);
+    buf[0] = form + (y_buf[field_len - 1] & 1);
   }
 
   return output_len;
@@ -214,6 +215,15 @@ size_t EC_POINT_point2oct(const EC_GROUP *group, const EC_POINT *point,
     OPENSSL_PUT_ERROR(EC, EC_R_INCOMPATIBLE_OBJECTS);
     return 0;
   }
+  if (buf == NULL) {
+    // When |buf| is NULL, just return the number of bytes that would be
+    // written, without doing an expensive Jacobian-to-affine conversion.
+    if (ec_GFp_simple_is_at_infinity(group, &point->raw)) {
+      OPENSSL_PUT_ERROR(EC, EC_R_POINT_AT_INFINITY);
+      return 0;
+    }
+    return ec_point_byte_len(group, form);
+  }
   EC_AFFINE affine;
   if (!ec_jacobian_to_affine(group, &affine, &point->raw)) {
     return 0;

crypto/trust_token/pmbtoken.c

@@ -123,8 +123,7 @@ err:
 
 static int point_to_cbb(CBB *out, const EC_GROUP *group,
                         const EC_AFFINE *point) {
-  size_t len =
-      ec_point_to_bytes(group, point, POINT_CONVERSION_UNCOMPRESSED, NULL, 0);
+  size_t len = ec_point_byte_len(group, POINT_CONVERSION_UNCOMPRESSED);
   if (len == 0) {
     return 0;
   }

crypto/trust_token/voprf.c

@@ -62,8 +62,7 @@ static int voprf_init_method(VOPRF_METHOD *method, int curve_nid,
 
 static int cbb_add_point(CBB *out, const EC_GROUP *group,
                          const EC_AFFINE *point) {
-  size_t len =
-      ec_point_to_bytes(group, point, POINT_CONVERSION_UNCOMPRESSED, NULL, 0);
+  size_t len = ec_point_byte_len(group,  POINT_CONVERSION_UNCOMPRESSED);
   if (len == 0) {
     return 0;
   }