Clean up EC_POINT to byte conversions.

With the allocations and BN_CTX gone, ECDH and point2oct are much, much
shorter.

Bug: 242
Change-Id: I3421822e94100f7eb2f5f2373df7fb3b3311365e
Reviewed-on: https://boringssl-review.googlesource.com/c/33071
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Adam Langley <agl@google.com>

crypto/ecdh_extra/ecdh_extra.c

@@ -69,7 +69,6 @@
 #include <limits.h>
 #include <string.h>
 
-#include <openssl/bn.h>
 #include <openssl/digest.h>
 #include <openssl/err.h>
 #include <openssl/mem.h>
@@ -78,10 +77,10 @@
 #include "../internal.h"
 
 
-int ECDH_compute_key(void *out, size_t outlen, const EC_POINT *pub_key,
+int ECDH_compute_key(void *out, size_t out_len, const EC_POINT *pub_key,
                      const EC_KEY *priv_key,
                      void *(*kdf)(const void *in, size_t inlen, void *out,
-                                  size_t *outlen)) {
+                                  size_t *out_len)) {
   if (priv_key->priv_key == NULL) {
     OPENSSL_PUT_ERROR(ECDH, ECDH_R_NO_PRIVATE_VALUE);
     return -1;
@@ -93,74 +92,33 @@ int ECDH_compute_key(void *out, size_t outlen, const EC_POINT *pub_key,
     return -1;
   }
 
-  BN_CTX *ctx = BN_CTX_new();
-  if (ctx == NULL) {
-    return -1;
-  }
-  BN_CTX_start(ctx);
-
-  int ret = -1;
-  size_t buflen = 0;
-  uint8_t *buf = NULL;
-
-  EC_POINT *tmp = EC_POINT_new(group);
-  if (tmp == NULL) {
-    OPENSSL_PUT_ERROR(ECDH, ERR_R_MALLOC_FAILURE);
-    goto err;
-  }
-
-  if (!ec_point_mul_scalar(group, &tmp->raw, NULL, &pub_key->raw, priv)) {
-    OPENSSL_PUT_ERROR(ECDH, ECDH_R_POINT_ARITHMETIC_FAILURE);
-    goto err;
-  }
-
-  BIGNUM *x = BN_CTX_get(ctx);
-  if (!x) {
-    OPENSSL_PUT_ERROR(ECDH, ERR_R_MALLOC_FAILURE);
-    goto err;
-  }
-
-  if (!EC_POINT_get_affine_coordinates_GFp(group, tmp, x, NULL, ctx)) {
+  EC_RAW_POINT shared_point;
+  uint8_t buf[EC_MAX_BYTES];
+  size_t buf_len;
+  if (!ec_point_mul_scalar(group, &shared_point, NULL, &pub_key->raw, priv) ||
+      !ec_point_get_affine_coordinate_bytes(group, buf, NULL, &buf_len,
+                                            sizeof(buf), &shared_point)) {
     OPENSSL_PUT_ERROR(ECDH, ECDH_R_POINT_ARITHMETIC_FAILURE);
-    goto err;
-  }
-
-  buflen = (EC_GROUP_get_degree(group) + 7) / 8;
-  buf = OPENSSL_malloc(buflen);
-  if (buf == NULL) {
-    OPENSSL_PUT_ERROR(ECDH, ERR_R_MALLOC_FAILURE);
-    goto err;
-  }
-
-  if (!BN_bn2bin_padded(buf, buflen, x)) {
-    OPENSSL_PUT_ERROR(ECDH, ERR_R_INTERNAL_ERROR);
-    goto err;
+    return -1;
   }
 
   if (kdf != NULL) {
-    if (kdf(buf, buflen, out, &outlen) == NULL) {
+    if (kdf(buf, buf_len, out, &out_len) == NULL) {
       OPENSSL_PUT_ERROR(ECDH, ECDH_R_KDF_FAILED);
-      goto err;
+      return -1;
     }
   } else {
     // no KDF, just copy as much as we can
-    if (buflen < outlen) {
-      outlen = buflen;
+    if (buf_len < out_len) {
+      out_len = buf_len;
     }
-    OPENSSL_memcpy(out, buf, outlen);
+    OPENSSL_memcpy(out, buf, out_len);
   }
 
-  if (outlen > INT_MAX) {
+  if (out_len > INT_MAX) {
     OPENSSL_PUT_ERROR(ECDH, ERR_R_OVERFLOW);
-    goto err;
+    return -1;
   }
 
-  ret = (int)outlen;
-
-err:
-  OPENSSL_free(buf);
-  EC_POINT_free(tmp);
-  BN_CTX_end(ctx);
-  BN_CTX_free(ctx);
-  return ret;
+  return (int)out_len;
 }

crypto/fipsmodule/ec/ec.c

@@ -973,6 +973,37 @@ int ec_get_x_coordinate_as_scalar(const EC_GROUP *group, EC_SCALAR *out,
   return 1;
 }
 
+int ec_point_get_affine_coordinate_bytes(const EC_GROUP *group, uint8_t *out_x,
+                                         uint8_t *out_y, size_t *out_len,
+                                         size_t max_out,
+                                         const EC_RAW_POINT *p) {
+  size_t len = BN_num_bytes(&group->field);
+  assert(len <= EC_MAX_BYTES);
+  if (max_out < len) {
+    OPENSSL_PUT_ERROR(EC, EC_R_BUFFER_TOO_SMALL);
+    return 0;
+  }
+
+  EC_FELEM x, y;
+  if (!group->meth->point_get_affine_coordinates(
+          group, p, out_x == NULL ? NULL : &x, out_y == NULL ? NULL : &y)) {
+    return 0;
+  }
+
+  if (out_x != NULL) {
+    for (size_t i = 0; i < len; i++) {
+      out_x[i] = x.bytes[len - i - 1];
+    }
+  }
+  if (out_y != NULL) {
+    for (size_t i = 0; i < len; i++) {
+      out_y[i] = y.bytes[len - i - 1];
+    }
+  }
+  *out_len = len;
+  return 1;
+}
+
 void EC_GROUP_set_asn1_flag(EC_GROUP *group, int flag) {}
 
 const EC_METHOD *EC_GROUP_method_of(const EC_GROUP *group) {

crypto/fipsmodule/ec/internal.h

@@ -354,6 +354,17 @@ int ec_cmp_x_coordinate(const EC_GROUP *group, const EC_RAW_POINT *p,
 int ec_get_x_coordinate_as_scalar(const EC_GROUP *group, EC_SCALAR *out,
                                   const EC_RAW_POINT *p);
 
+// ec_point_get_affine_coordinate_bytes writes |p|'s affine coordinates to
+// |out_x| and |out_y|, each of which must have at must |max_out| bytes. It sets
+// |*out_len| to the number of bytes written in each buffer. Coordinates are
+// written big-endian and zero-padded to the size of the field.
+//
+// Either of |out_x| or |out_y| may be NULL to omit that coordinate. This
+// function returns one on success and zero on failure.
+int ec_point_get_affine_coordinate_bytes(const EC_GROUP *group, uint8_t *out_x,
+                                         uint8_t *out_y, size_t *out_len,
+                                         size_t max_out, const EC_RAW_POINT *p);
+
 // ec_field_element_to_scalar reduces |r| modulo |group->order|. |r| must
 // previously have been reduced modulo |group->field|.
 int ec_field_element_to_scalar(const EC_GROUP *group, BIGNUM *r);

crypto/fipsmodule/ec/oct.c

@@ -74,22 +74,18 @@
 
 
 static size_t ec_GFp_simple_point2oct(const EC_GROUP *group,
-                                      const EC_POINT *point,
+                                      const EC_RAW_POINT *point,
                                       point_conversion_form_t form,
-                                      uint8_t *buf, size_t len, BN_CTX *ctx) {
-  size_t ret = 0;
-  BN_CTX *new_ctx = NULL;
-  int used_ctx = 0;
-
-  if ((form != POINT_CONVERSION_COMPRESSED) &&
-      (form != POINT_CONVERSION_UNCOMPRESSED)) {
+                                      uint8_t *buf, size_t len) {
+  if (form != POINT_CONVERSION_COMPRESSED &&
+      form != POINT_CONVERSION_UNCOMPRESSED) {
     OPENSSL_PUT_ERROR(EC, EC_R_INVALID_FORM);
-    goto err;
+    return 0;
   }
 
-  if (EC_POINT_is_at_infinity(group, point)) {
+  if (ec_GFp_simple_is_at_infinity(group, point)) {
     OPENSSL_PUT_ERROR(EC, EC_R_POINT_AT_INFINITY);
-    goto err;
+    return 0;
   }
 
   const size_t field_len = BN_num_bytes(&group->field);
@@ -103,64 +99,31 @@ static size_t ec_GFp_simple_point2oct(const EC_GROUP *group,
   if (buf != NULL) {
     if (len < output_len) {
       OPENSSL_PUT_ERROR(EC, EC_R_BUFFER_TOO_SMALL);
-      goto err;
-    }
-
-    if (ctx == NULL) {
-      ctx = new_ctx = BN_CTX_new();
-      if (ctx == NULL) {
-        goto err;
-      }
+      return 0;
     }
 
-    BN_CTX_start(ctx);
-    used_ctx = 1;
-    BIGNUM *x = BN_CTX_get(ctx);
-    BIGNUM *y = BN_CTX_get(ctx);
-    if (y == NULL) {
-      goto err;
+    uint8_t y_buf[EC_MAX_BYTES];
+    size_t field_len_out;
+    if (!ec_point_get_affine_coordinate_bytes(
+            group, buf + 1 /* x */,
+            form == POINT_CONVERSION_COMPRESSED ? y_buf : buf + 1 + field_len,
+            &field_len_out, field_len, point)) {
+      return 0;
     }
 
-    if (!EC_POINT_get_affine_coordinates_GFp(group, point, x, y, ctx)) {
-      goto err;
+    if (field_len_out != field_len) {
+      OPENSSL_PUT_ERROR(EC, ERR_R_INTERNAL_ERROR);
+      return 0;
     }
 
-    if ((form == POINT_CONVERSION_COMPRESSED) &&
-        BN_is_odd(y)) {
-      buf[0] = form + 1;
+    if (form == POINT_CONVERSION_COMPRESSED) {
+      buf[0] = form + (y_buf[field_len - 1] & 1);
     } else {
       buf[0] = form;
     }
-    size_t i = 1;
-
-    if (!BN_bn2bin_padded(buf + i, field_len, x)) {
-      OPENSSL_PUT_ERROR(EC, ERR_R_INTERNAL_ERROR);
-      goto err;
-    }
-    i += field_len;
-
-    if (form == POINT_CONVERSION_UNCOMPRESSED) {
-      if (!BN_bn2bin_padded(buf + i, field_len, y)) {
-        OPENSSL_PUT_ERROR(EC, ERR_R_INTERNAL_ERROR);
-        goto err;
-      }
-      i += field_len;
-    }
-
-    if (i != output_len) {
-      OPENSSL_PUT_ERROR(EC, ERR_R_INTERNAL_ERROR);
-      goto err;
-    }
   }
 
-  ret = output_len;
-
-err:
-  if (used_ctx) {
-    BN_CTX_end(ctx);
-  }
-  BN_CTX_free(new_ctx);
-  return ret;
+  return output_len;
 }
 
 static int ec_GFp_simple_oct2point(const EC_GROUP *group, EC_POINT *point,
@@ -263,7 +226,7 @@ size_t EC_POINT_point2oct(const EC_GROUP *group, const EC_POINT *point,
     OPENSSL_PUT_ERROR(EC, EC_R_INCOMPATIBLE_OBJECTS);
     return 0;
   }
-  return ec_GFp_simple_point2oct(group, point, form, buf, len, ctx);
+  return ec_GFp_simple_point2oct(group, &point->raw, form, buf, len);
 }
 
 int EC_POINT_set_compressed_coordinates_GFp(const EC_GROUP *group,

crypto/fipsmodule/ecdh/ecdh.c

@@ -66,10 +66,8 @@
 
 #include <openssl/ecdh.h>
 
-#include <limits.h>
 #include <string.h>
 
-#include <openssl/bn.h>
 #include <openssl/ec.h>
 #include <openssl/ec_key.h>
 #include <openssl/err.h>
@@ -92,49 +90,14 @@ int ECDH_compute_key_fips(uint8_t *out, size_t out_len, const EC_POINT *pub_key,
     return 0;
   }
 
-  BN_CTX *ctx = BN_CTX_new();
-  if (ctx == NULL) {
-    return 0;
-  }
-  BN_CTX_start(ctx);
-
-  int ret = 0;
-  size_t buflen = 0;
-  uint8_t *buf = NULL;
-
-  EC_POINT *shared_point = EC_POINT_new(group);
-  if (shared_point == NULL) {
-    OPENSSL_PUT_ERROR(ECDH, ERR_R_MALLOC_FAILURE);
-    goto err;
-  }
-
-  if (!ec_point_mul_scalar(group, &shared_point->raw, NULL, &pub_key->raw,
-                           priv)) {
+  EC_RAW_POINT shared_point;
+  uint8_t buf[EC_MAX_BYTES];
+  size_t buflen;
+  if (!ec_point_mul_scalar(group, &shared_point, NULL, &pub_key->raw, priv) ||
+      !ec_point_get_affine_coordinate_bytes(group, buf, NULL, &buflen,
+                                            sizeof(buf), &shared_point)) {
     OPENSSL_PUT_ERROR(ECDH, ECDH_R_POINT_ARITHMETIC_FAILURE);
-    goto err;
-  }
-
-  BIGNUM *x = BN_CTX_get(ctx);
-  if (!x) {
-    OPENSSL_PUT_ERROR(ECDH, ERR_R_MALLOC_FAILURE);
-    goto err;
-  }
-
-  if (!EC_POINT_get_affine_coordinates_GFp(group, shared_point, x, NULL, ctx)) {
-    OPENSSL_PUT_ERROR(ECDH, ECDH_R_POINT_ARITHMETIC_FAILURE);
-    goto err;
-  }
-
-  buflen = (EC_GROUP_get_degree(group) + 7) / 8;
-  buf = OPENSSL_malloc(buflen);
-  if (buf == NULL) {
-    OPENSSL_PUT_ERROR(ECDH, ERR_R_MALLOC_FAILURE);
-    goto err;
-  }
-
-  if (!BN_bn2bin_padded(buf, buflen, x)) {
-    OPENSSL_PUT_ERROR(ECDH, ERR_R_INTERNAL_ERROR);
-    goto err;
+    return 0;
   }
 
   switch (out_len) {
@@ -152,15 +115,8 @@ int ECDH_compute_key_fips(uint8_t *out, size_t out_len, const EC_POINT *pub_key,
       break;
     default:
       OPENSSL_PUT_ERROR(ECDH, ECDH_R_UNKNOWN_DIGEST_LENGTH);
-      goto err;
+      return 0;
   }
 
-  ret = 1;
-
-err:
-  OPENSSL_free(buf);
-  EC_POINT_free(shared_point);
-  BN_CTX_end(ctx);
-  BN_CTX_free(ctx);
-  return ret;
+  return 1;
 }