Skip to content

Commit a7185da

Browse files
davidbenDominic Dams
davidben
authored and
Dominic Dams
committed
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> (cherry picked from commit da663b7ca86d70f7da979f9a48d2238ca5762bdd)
1 parent a783e46 commit a7185da

5 files changed

Lines changed: 66 additions & 28 deletions

File tree

‎crypto/fipsmodule/ec/ec_test.cc‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1730,6 +1730,29 @@ TEST_P(ECCurveTest, GPlusMinusG) {
17301730
EXPECT_TRUE(EC_POINT_is_at_infinity(group(), sum.get()));
17311731
}
17321732

1733+
// Test that we refuse to encode or decode the point at infinity.
1734+
TEST_P(ECCurveTest, EncodeInfinity) {
1735+
// The point at infinity is encoded as a single zero byte, but we do not
1736+
// support it.
1737+
static const uint8_t kInfinity[] = {0};
1738+
bssl::UniquePtr<EC_POINT> inf(EC_POINT_new(group()));
1739+
ASSERT_TRUE(inf);
1740+
EXPECT_FALSE(EC_POINT_oct2point(group(), inf.get(), kInfinity,
1741+
sizeof(kInfinity), nullptr));
1742+
1743+
// Encoding it also fails.
1744+
ASSERT_TRUE(EC_POINT_set_to_infinity(group(), inf.get()));
1745+
uint8_t buf[128];
1746+
EXPECT_EQ(
1747+
0u, EC_POINT_point2oct(group(), inf.get(), POINT_CONVERSION_UNCOMPRESSED,
1748+
buf, sizeof(buf), nullptr));
1749+
1750+
// Measuring the length of the encoding also fails.
1751+
EXPECT_EQ(
1752+
0u, EC_POINT_point2oct(group(), inf.get(), POINT_CONVERSION_UNCOMPRESSED,
1753+
nullptr, 0, nullptr));
1754+
}
1755+
17331756
static std::vector<EC_builtin_curve> AllCurves() {
17341757
const size_t num_curves = EC_get_builtin_curves(nullptr, 0);
17351758
std::vector<EC_builtin_curve> curves(num_curves);

‎crypto/fipsmodule/ec/internal.h‎

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -449,11 +449,18 @@ int ec_get_x_coordinate_as_bytes(const EC_GROUP *group, uint8_t *out,
449449
size_t *out_len, size_t max_out,
450450
const EC_RAW_POINT *p);
451451

452-
// ec_point_to_bytes behaves like |EC_POINT_point2oct| but takes an
453-
// |EC_AFFINE|.
452+
// ec_point_byte_len returns the number of bytes in the byte representation of
453+
// a non-infinity point in |group|, encoded according to |form|, or zero if
454+
// |form| is invalid.
455+
size_t ec_point_byte_len(const EC_GROUP *group, point_conversion_form_t form);
456+
457+
// ec_point_to_bytes encodes |point| according to |form| and writes the result
458+
// |buf|. It returns the size of the output on success or zero on error. At most
459+
// |max_out| bytes will be written. The buffer should be at least
460+
// |ec_point_byte_len| long to guarantee success.
454461
size_t ec_point_to_bytes(const EC_GROUP *group, const EC_AFFINE *point,
455462
point_conversion_form_t form, uint8_t *buf,
456-
size_t len);
463+
size_t max_out);
457464

458465
// ec_point_from_uncompressed parses |in| as a point in uncompressed form and
459466
// sets the result to |out|. It returns one on success and zero if the input was

‎crypto/fipsmodule/ec/oct.c‎

Lines changed: 31 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -73,9 +73,7 @@
7373
#include "internal.h"
7474

7575

76-
size_t ec_point_to_bytes(const EC_GROUP *group, const EC_AFFINE *point,
77-
point_conversion_form_t form, uint8_t *buf,
78-
size_t len) {
76+
size_t ec_point_byte_len(const EC_GROUP *group, point_conversion_form_t form) {
7977
if (form != POINT_CONVERSION_COMPRESSED &&
8078
form != POINT_CONVERSION_UNCOMPRESSED) {
8179
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,
8886
// Uncompressed points have a second coordinate.
8987
output_len += field_len;
9088
}
89+
return output_len;
90+
}
9191

92-
// if 'buf' is NULL, just return required length
93-
if (buf != NULL) {
94-
if (len < output_len) {
95-
OPENSSL_PUT_ERROR(EC, EC_R_BUFFER_TOO_SMALL);
96-
return 0;
97-
}
92+
size_t ec_point_to_bytes(const EC_GROUP *group, const EC_AFFINE *point,
93+
point_conversion_form_t form, uint8_t *buf,
94+
size_t len) {
95+
size_t output_len = ec_point_byte_len(group, form);
96+
if (len < output_len) {
97+
OPENSSL_PUT_ERROR(EC, EC_R_BUFFER_TOO_SMALL);
98+
return 0;
99+
}
98100

99-
size_t field_len_out;
100-
ec_felem_to_bytes(group, buf + 1, &field_len_out, &point->X);
101-
assert(field_len_out == field_len);
101+
size_t field_len;
102+
ec_felem_to_bytes(group, buf + 1, &field_len, &point->X);
103+
assert(field_len == BN_num_bytes(&group->field));
102104

103-
if (form == POINT_CONVERSION_UNCOMPRESSED) {
104-
ec_felem_to_bytes(group, buf + 1 + field_len, &field_len_out, &point->Y);
105-
assert(field_len_out == field_len);
106-
buf[0] = form;
107-
} else {
108-
uint8_t y_buf[EC_MAX_BYTES];
109-
ec_felem_to_bytes(group, y_buf, &field_len_out, &point->Y);
110-
buf[0] = form + (y_buf[field_len_out - 1] & 1);
111-
}
105+
if (form == POINT_CONVERSION_UNCOMPRESSED) {
106+
ec_felem_to_bytes(group, buf + 1 + field_len, &field_len, &point->Y);
107+
assert(field_len == BN_num_bytes(&group->field));
108+
buf[0] = form;
109+
} else {
110+
uint8_t y_buf[EC_MAX_BYTES];
111+
ec_felem_to_bytes(group, y_buf, &field_len, &point->Y);
112+
buf[0] = form + (y_buf[field_len - 1] & 1);
112113
}
113114

114115
return output_len;
@@ -214,6 +215,15 @@ size_t EC_POINT_point2oct(const EC_GROUP *group, const EC_POINT *point,
214215
OPENSSL_PUT_ERROR(EC, EC_R_INCOMPATIBLE_OBJECTS);
215216
return 0;
216217
}
218+
if (buf == NULL) {
219+
// When |buf| is NULL, just return the number of bytes that would be
220+
// written, without doing an expensive Jacobian-to-affine conversion.
221+
if (ec_GFp_simple_is_at_infinity(group, &point->raw)) {
222+
OPENSSL_PUT_ERROR(EC, EC_R_POINT_AT_INFINITY);
223+
return 0;
224+
}
225+
return ec_point_byte_len(group, form);
226+
}
217227
EC_AFFINE affine;
218228
if (!ec_jacobian_to_affine(group, &affine, &point->raw)) {
219229
return 0;

‎crypto/trust_token/pmbtoken.c‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -123,8 +123,7 @@ static int derive_scalar_from_secret(const PMBTOKEN_METHOD *method,
123123

124124
static int point_to_cbb(CBB *out, const EC_GROUP *group,
125125
const EC_AFFINE *point) {
126-
size_t len =
127-
ec_point_to_bytes(group, point, POINT_CONVERSION_UNCOMPRESSED, NULL, 0);
126+
size_t len = ec_point_byte_len(group, POINT_CONVERSION_UNCOMPRESSED);
128127
if (len == 0) {
129128
return 0;
130129
}

‎crypto/trust_token/voprf.c‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,8 +62,7 @@ static int voprf_init_method(VOPRF_METHOD *method, int curve_nid,
6262

6363
static int cbb_add_point(CBB *out, const EC_GROUP *group,
6464
const EC_AFFINE *point) {
65-
size_t len =
66-
ec_point_to_bytes(group, point, POINT_CONVERSION_UNCOMPRESSED, NULL, 0);
65+
size_t len = ec_point_byte_len(group, POINT_CONVERSION_UNCOMPRESSED);
6766
if (len == 0) {
6867
return 0;
6968
}

0 commit comments

Comments
 (0)