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>
diff --git a/crypto/fipsmodule/ec/ec_test.cc b/crypto/fipsmodule/ec/ec_test.cc index edcfeaa..c54adb5 100644 --- a/crypto/fipsmodule/ec/ec_test.cc +++ b/crypto/fipsmodule/ec/ec_test.cc
@@ -891,6 +891,29 @@ 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);
diff --git a/crypto/fipsmodule/ec/internal.h b/crypto/fipsmodule/ec/internal.h index f6c8e8a..0d53546 100644 --- a/crypto/fipsmodule/ec/internal.h +++ b/crypto/fipsmodule/ec/internal.h
@@ -439,11 +439,18 @@ 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
diff --git a/crypto/fipsmodule/ec/oct.c b/crypto/fipsmodule/ec/oct.c index ddd0f37..7032635 100644 --- a/crypto/fipsmodule/ec/oct.c +++ b/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 @@ // 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 @@ 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;
diff --git a/crypto/trust_token/pmbtoken.c b/crypto/trust_token/pmbtoken.c index 68d8909..ab09f01 100644 --- a/crypto/trust_token/pmbtoken.c +++ b/crypto/trust_token/pmbtoken.c
@@ -123,8 +123,7 @@ 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; }
diff --git a/crypto/trust_token/voprf.c b/crypto/trust_token/voprf.c index cedee1e..f8e1c4c 100644 --- a/crypto/trust_token/voprf.c +++ b/crypto/trust_token/voprf.c
@@ -62,8 +62,7 @@ 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; }