commit 0a6c3fc9c309f95408a3194f087b5394c198d250
author David Benjamin <davidben@google.com> Mon Mar 29 16:11:12 2021 -0400
committer CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org> Thu Apr 01 22:10:25 2021 +0000
tree bab2d69f75b0bb57265c1a3071e52e32adb42834
parent 08b1729f07b2b471afddcd05602a60ad83cb3b7e

Rearrange SSLKeyShare::Serialize.

It's strange to have Serialize/Deserialize methods not inverses of each
other. Split the operation up and move the common parts out of the
subclass.

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

ssl/internal.h

diff --git a/ssl/internal.h b/ssl/internal.h
index 7bb11f1..e733e67 100644
--- a/ssl/internal.h
+++ b/ssl/internal.h
@@ -1066,6 +1066,10 @@
   // |Serialize|.
   static UniquePtr<SSLKeyShare> Create(CBS *in);
 
+  // Serializes writes the group ID and private key, in a format that can be
+  // read by |Create|.
+  bool Serialize(CBB *out);
+
   // GroupID returns the group ID.
   virtual uint16_t GroupID() const PURE_VIRTUAL;
 
@@ -1090,13 +1094,13 @@
   virtual bool Finish(Array<uint8_t> *out_secret, uint8_t *out_alert,
                       Span<const uint8_t> peer_key) PURE_VIRTUAL;
 
-  // Serialize writes the state of the key exchange to |out|, returning true if
-  // successful and false otherwise.
-  virtual bool Serialize(CBB *out) { return false; }
+  // SerializePrivateKey writes the private key to |out|, returning true if
+  // successful and false otherwise. It should be called after |Offer|.
+  virtual bool SerializePrivateKey(CBB *out) { return false; }
 
-  // Deserialize initializes the state of the key exchange from |in|, returning
-  // true if successful and false otherwise.  It is called by |Create|.
-  virtual bool Deserialize(CBS *in) { return false; }
+  // DeserializePrivateKey initializes the state of the key exchange from |in|,
+  // returning true if successful and false otherwise.
+  virtual bool DeserializePrivateKey(CBS *in) { return false; }
 };
 
 struct NamedGroup {

ssl/ssl_key_share.cc

diff --git a/ssl/ssl_key_share.cc b/ssl/ssl_key_share.cc
index 6cac3cf..d9a09c7 100644
--- a/ssl/ssl_key_share.cc
+++ b/ssl/ssl_key_share.cc
@@ -124,29 +124,17 @@
     return true;
   }
 
-  bool Serialize(CBB *out) override {
+  bool SerializePrivateKey(CBB *out) override {
     assert(private_key_);
-    CBB cbb;
     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()));
-    if (!CBB_add_asn1_uint64(out, group_id_) ||
-        !CBB_add_asn1(out, &cbb, CBS_ASN1_OCTETSTRING) ||
-        !BN_bn2cbb_padded(&cbb, len, private_key_.get()) ||
-        !CBB_flush(out)) {
-      return false;
-    }
-    return true;
+    return BN_bn2cbb_padded(out, len, private_key_.get());
   }
 
-  bool Deserialize(CBS *in) override {
+  bool DeserializePrivateKey(CBS *in) override {
     assert(!private_key_);
-    CBS private_key;
-    if (!CBS_get_asn1(in, &private_key, CBS_ASN1_OCTETSTRING)) {
-      return false;
-    }
-    private_key_.reset(BN_bin2bn(CBS_data(&private_key),
-                                 CBS_len(&private_key), nullptr));
+    private_key_.reset(BN_bin2bn(CBS_data(in), CBS_len(in), nullptr));
     return private_key_ != nullptr;
   }
 
@@ -189,16 +177,13 @@
     return true;
   }
 
-  bool Serialize(CBB *out) override {
-    return (CBB_add_asn1_uint64(out, GroupID()) &&
-            CBB_add_asn1_octet_string(out, private_key_, sizeof(private_key_)));
+  bool SerializePrivateKey(CBB *out) override {
+    return CBB_add_bytes(out, private_key_, sizeof(private_key_));
   }
 
-  bool Deserialize(CBS *in) override {
-    CBS key;
-    if (!CBS_get_asn1(in, &key, CBS_ASN1_OCTETSTRING) ||
-        CBS_len(&key) != sizeof(private_key_) ||
-        !CBS_copy_bytes(&key, private_key_, sizeof(private_key_))) {
+  bool DeserializePrivateKey(CBS *in) override {
+    if (CBS_len(in) != sizeof(private_key_) ||
+        !CBS_copy_bytes(in, private_key_, sizeof(private_key_))) {
       return false;
     }
     return true;
@@ -339,16 +324,28 @@
 
 UniquePtr<SSLKeyShare> SSLKeyShare::Create(CBS *in) {
   uint64_t group;
-  if (!CBS_get_asn1_uint64(in, &group) || group > 0xffff) {
+  CBS private_key;
+  if (!CBS_get_asn1_uint64(in, &group) || group > 0xffff ||
+      !CBS_get_asn1(in, &private_key, CBS_ASN1_OCTETSTRING)) {
     return nullptr;
   }
   UniquePtr<SSLKeyShare> key_share = Create(static_cast<uint16_t>(group));
-  if (!key_share || !key_share->Deserialize(in)) {
+  if (!key_share || !key_share->DeserializePrivateKey(&private_key)) {
     return nullptr;
   }
   return key_share;
 }
 
+bool SSLKeyShare::Serialize(CBB *out) {
+  CBB private_key;
+  if (!CBB_add_asn1_uint64(out, GroupID()) ||
+      !CBB_add_asn1(out, &private_key, CBS_ASN1_OCTETSTRING) ||
+      !SerializePrivateKey(&private_key) ||  //
+      !CBB_flush(out)) {
+    return false;
+  }
+  return true;
+}
 
 bool SSLKeyShare::Accept(CBB *out_public_key, Array<uint8_t> *out_secret,
                          uint8_t *out_alert, Span<const uint8_t> peer_key) {