From bff2c1853d0e24fa1b02e4583d789fafc18b2d91 Mon Sep 17 00:00:00 2001
From: David Benjamin <davidben@google.com>
Date: Thu, 26 Jan 2023 17:04:04 -0500
Subject: [PATCH] Simplify ECKeyShare slightly.

Since this was written, we've tidied up the EC code a bit:

1. While not quite yet infallible (but we should get there), the output
   of EC_GROUP_new_by_curve_name no longer needs to be freed.

2. BN_CTX no longer does anything in EC code, so just pass in NULL.

We really should build a real ECDH API, but for now just improve our use
of the current thing.

Change-Id: I44f5429afec06c28372ae70148eb8de263d716f3
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/57626
Reviewed-by: Adam Langley <agl@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
(cherry picked from commit 9cbff81cec055e34d3f6e267d5509d4aad6bed41)
---
 ssl/ssl_key_share.cc | 67 +++++++++++++++++---------------------------
 1 file changed, 25 insertions(+), 42 deletions(-)

diff --git a/ssl/ssl_key_share.cc b/ssl/ssl_key_share.cc
index 69eecfd2a8..2abe18cb17 100644
--- a/ssl/ssl_key_share.cc
+++ b/ssl/ssl_key_share.cc
@@ -38,35 +38,28 @@ namespace {
 
 class ECKeyShare : public SSLKeyShare {
  public:
-  ECKeyShare(int nid, uint16_t group_id) : nid_(nid), group_id_(group_id) {}
+  ECKeyShare(int nid, uint16_t group_id)
+      : group_(EC_GROUP_new_by_curve_name(nid)), group_id_(group_id) {}
 
   uint16_t GroupID() const override { return group_id_; }
 
   bool Offer(CBB *out) override {
     assert(!private_key_);
-    // Set up a shared |BN_CTX| for all operations.
-    UniquePtr<BN_CTX> bn_ctx(BN_CTX_new());
-    if (!bn_ctx) {
-      return false;
-    }
-    BN_CTXScope scope(bn_ctx.get());
-
     // Generate a private key.
-    UniquePtr<EC_GROUP> group(EC_GROUP_new_by_curve_name(nid_));
     private_key_.reset(BN_new());
-    if (!group || !private_key_ ||
+    if (!group_ || !private_key_ ||
         !BN_rand_range_ex(private_key_.get(), 1,
-                          EC_GROUP_get0_order(group.get()))) {
+                          EC_GROUP_get0_order(group_))) {
       return false;
     }
 
     // Compute the corresponding public key and serialize it.
-    UniquePtr<EC_POINT> public_key(EC_POINT_new(group.get()));
+    UniquePtr<EC_POINT> public_key(EC_POINT_new(group_));
     if (!public_key ||
-        !EC_POINT_mul(group.get(), public_key.get(), private_key_.get(), NULL,
-                      NULL, bn_ctx.get()) ||
-        !EC_POINT_point2cbb(out, group.get(), public_key.get(),
-                            POINT_CONVERSION_UNCOMPRESSED, bn_ctx.get())) {
+        !EC_POINT_mul(group_, public_key.get(), private_key_.get(),
+                      nullptr, nullptr, /*ctx=*/nullptr) ||
+        !EC_POINT_point2cbb(out, group_, public_key.get(),
+                            POINT_CONVERSION_UNCOMPRESSED, /*ctx=*/nullptr)) {
       return false;
     }
 
@@ -75,48 +68,38 @@ class ECKeyShare : public SSLKeyShare {
 
   bool Finish(Array<uint8_t> *out_secret, uint8_t *out_alert,
               Span<const uint8_t> peer_key) override {
+    assert(group_);
     assert(private_key_);
     *out_alert = SSL_AD_INTERNAL_ERROR;
 
-    // Set up a shared |BN_CTX| for all operations.
-    UniquePtr<BN_CTX> bn_ctx(BN_CTX_new());
-    if (!bn_ctx) {
-      return false;
-    }
-    BN_CTXScope scope(bn_ctx.get());
-
-    UniquePtr<EC_GROUP> group(EC_GROUP_new_by_curve_name(nid_));
-    if (!group) {
-      return false;
-    }
-
-    UniquePtr<EC_POINT> peer_point(EC_POINT_new(group.get()));
-    UniquePtr<EC_POINT> result(EC_POINT_new(group.get()));
-    BIGNUM *x = BN_CTX_get(bn_ctx.get());
+    UniquePtr<EC_POINT> peer_point(EC_POINT_new(group_));
+    UniquePtr<EC_POINT> result(EC_POINT_new(group_));
+    UniquePtr<BIGNUM> x(BN_new());
     if (!peer_point || !result || !x) {
       return false;
     }
 
     if (peer_key.empty() || peer_key[0] != POINT_CONVERSION_UNCOMPRESSED ||
-        !EC_POINT_oct2point(group.get(), peer_point.get(), peer_key.data(),
-                            peer_key.size(), bn_ctx.get())) {
+        !EC_POINT_oct2point(group_, peer_point.get(), peer_key.data(),
+                            peer_key.size(), /*ctx=*/nullptr)) {
       OPENSSL_PUT_ERROR(SSL, SSL_R_BAD_ECPOINT);
       *out_alert = SSL_AD_DECODE_ERROR;
       return false;
     }
 
     // Compute the x-coordinate of |peer_key| * |private_key_|.
-    if (!EC_POINT_mul(group.get(), result.get(), NULL, peer_point.get(),
-                      private_key_.get(), bn_ctx.get()) ||
-        !EC_POINT_get_affine_coordinates_GFp(group.get(), result.get(), x, NULL,
-                                             bn_ctx.get())) {
+    if (!EC_POINT_mul(group_, result.get(), NULL, peer_point.get(),
+                      private_key_.get(), /*ctx=*/nullptr) ||
+        !EC_POINT_get_affine_coordinates_GFp(group_, result.get(), x.get(),
+                                             NULL,
+                                             /*ctx=*/nullptr)) {
       return false;
     }
 
     // Encode the x-coordinate left-padded with zeros.
     Array<uint8_t> secret;
-    if (!secret.Init((EC_GROUP_get_degree(group.get()) + 7) / 8) ||
-        !BN_bn2bin_padded(secret.data(), secret.size(), x)) {
+    if (!secret.Init((EC_GROUP_get_degree(group_) + 7) / 8) ||
+        !BN_bn2bin_padded(secret.data(), secret.size(), x.get())) {
       return false;
     }
 
@@ -125,10 +108,10 @@ class ECKeyShare : public SSLKeyShare {
   }
 
   bool SerializePrivateKey(CBB *out) override {
+    assert(group_);
     assert(private_key_);
-    UniquePtr<EC_GROUP> group(EC_GROUP_new_by_curve_name(nid_));
     // Padding is added to avoid leaking the length.
-    size_t len = BN_num_bytes(EC_GROUP_get0_order(group.get()));
+    size_t len = BN_num_bytes(EC_GROUP_get0_order(group_));
     return BN_bn2cbb_padded(out, len, private_key_.get());
   }
 
@@ -140,7 +123,7 @@ class ECKeyShare : public SSLKeyShare {
 
  private:
   UniquePtr<BIGNUM> private_key_;
-  int nid_;
+  const EC_GROUP *const group_ = nullptr;
   uint16_t group_id_;
 };