Set an EVP_PKEY's algorithm and data together We internally half-initialize EVP_PKEYs everywhere, but there are very few places where we actually need to half-initialize them. This changes most of the EVP_PKEY_ASN1_METHOD callbacks so that output EVP_PKEYs are not half-initialized with the method first. Rather, the callback is expected to fill in the method and contents together. EVP_PKEY_copy_parameters remains as a goofy exception because it's an in-out parameter. In principle, it is possible to have a goofy parameter-less, key-only DSA object, and we need to fill in the parameters later. This was due to how DSA was embedded into X.509. But we don't support DSA in X.509 and we removed this parameterless state from the parser, so we probably can remove this now. (I've left it as-is for now.) Bug: 42290409 Change-Id: I2a576571d75ce755fd7e963be467aa5d94f20466 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81550 Auto-Submit: David Benjamin <davidben@google.com> Reviewed-by: Lily Chen <chlily@google.com> Commit-Queue: Lily Chen <chlily@google.com>

diff --git a/crypto/evp/evp.cc b/crypto/evp/evp.cc
index 786e9b7..6f782a8 100644
--- a/crypto/evp/evp.cc
+++ b/crypto/evp/evp.cc

@@ -45,14 +45,6 @@
   return ret;
 }
 
-static void free_it(EVP_PKEY *pkey) {
-  if (pkey->ameth && pkey->ameth->pkey_free) {
-    pkey->ameth->pkey_free(pkey);
-  }
-  pkey->pkey = nullptr;
-  pkey->ameth = nullptr;
-}
-
 void EVP_PKEY_free(EVP_PKEY *pkey) {
   if (pkey == NULL) {
     return;
@@ -62,7 +54,7 @@
     return;
   }
 
-  free_it(pkey);
+  evp_pkey_set0(pkey, nullptr, nullptr);
   OPENSSL_free(pkey);
 }
 
@@ -103,7 +95,11 @@
 
 int EVP_PKEY_copy_parameters(EVP_PKEY *to, const EVP_PKEY *from) {
   if (EVP_PKEY_id(to) == EVP_PKEY_NONE) {
-    evp_pkey_set_method(to, from->ameth);
+    // TODO(crbug.com/42290409): This shouldn't leave |to| in a half-empty state
+    // on error. The complexity here largely comes from parameterless DSA keys,
+    // which we no longer support, so this function can probably be trimmed
+    // down.
+    evp_pkey_set0(to, from->ameth, nullptr);
   } else if (EVP_PKEY_id(to) != EVP_PKEY_id(from)) {
     OPENSSL_PUT_ERROR(EVP, EVP_R_DIFFERENT_KEY_TYPES);
     return 0;
@@ -127,8 +123,9 @@
     return from->ameth->param_copy(to, from);
   }
 
-  // TODO(https://crbug.com/boringssl/536): If the algorithm takes no
-  // parameters, copying them should vacuously succeed.
+  // TODO(https://crbug.com/42290406): If the algorithm takes no parameters,
+  // copying them should vacuously succeed. Better yet, simplify this whole
+  // notion of parameter copying above.
   return 0;
 }
 
@@ -157,9 +154,13 @@
   return pkey->ameth != nullptr ? pkey->ameth->pkey_id : EVP_PKEY_NONE;
 }
 
-void evp_pkey_set_method(EVP_PKEY *pkey, const EVP_PKEY_ASN1_METHOD *method) {
-  free_it(pkey);
+void evp_pkey_set0(EVP_PKEY *pkey, const EVP_PKEY_ASN1_METHOD *method,
+                   void *pkey_data) {
+  if (pkey->ameth && pkey->ameth->pkey_free) {
+    pkey->ameth->pkey_free(pkey);
+  }
   pkey->ameth = method;
+  pkey->pkey = pkey_data;
 }
 
 int EVP_PKEY_type(int nid) {
@@ -192,7 +193,7 @@
   if (pkey && pkey->pkey) {
     // Some callers rely on |pkey| getting cleared even if |type| is
     // unsupported, usually setting |type| to |EVP_PKEY_NONE|.
-    free_it(pkey);
+    evp_pkey_set0(pkey, nullptr, nullptr);
   }
 
   // This function broadly isn't useful. It initializes |EVP_PKEY| for a type,
@@ -209,7 +210,7 @@
   }
 
   if (pkey) {
-    evp_pkey_set_method(pkey, ameth);
+    evp_pkey_set0(pkey, ameth, nullptr);
   }
 
   return 1;
@@ -233,12 +234,8 @@
   }
 
   bssl::UniquePtr<EVP_PKEY> ret(EVP_PKEY_new());
-  if (ret == nullptr) {
-    return nullptr;
-  }
-  evp_pkey_set_method(ret.get(), method);
-
-  if (!ret->ameth->set_priv_raw(ret.get(), in, len)) {
+  if (ret == nullptr ||  //
+      !method->set_priv_raw(ret.get(), in, len)) {
     return nullptr;
   }
 
@@ -263,12 +260,8 @@
   }
 
   bssl::UniquePtr<EVP_PKEY> ret(EVP_PKEY_new());
-  if (ret == nullptr) {
-    return nullptr;
-  }
-  evp_pkey_set_method(ret.get(), method);
-
-  if (!ret->ameth->set_pub_raw(ret.get(), in, len)) {
+  if (ret == nullptr ||  //
+      !method->set_pub_raw(ret.get(), in, len)) {
     return nullptr;
   }
 

diff --git a/crypto/evp/evp_asn1.cc b/crypto/evp/evp_asn1.cc
index 6229ce3..7fca9b8 100644
--- a/crypto/evp/evp_asn1.cc
+++ b/crypto/evp/evp_asn1.cc

@@ -65,31 +65,19 @@
     return nullptr;
   }
   const EVP_PKEY_ASN1_METHOD *method = parse_key_type(&algorithm);
-  if (method == nullptr) {
+  if (method == nullptr || method->pub_decode == nullptr) {
     OPENSSL_PUT_ERROR(EVP, EVP_R_UNSUPPORTED_ALGORITHM);
     return nullptr;
   }
-  if (// Every key type defined encodes the key as a byte string with the same
-      // conversion to BIT STRING.
-      !CBS_get_u8(&key, &padding) ||
-      padding != 0) {
+  // Every key type defined encodes the key as a byte string with the same
+  // conversion to BIT STRING, so perform that common conversion ahead of time.
+  if (!CBS_get_u8(&key, &padding) || padding != 0) {
     OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR);
     return nullptr;
   }
 
-  // Set up an |EVP_PKEY| of the appropriate type.
   bssl::UniquePtr<EVP_PKEY> ret(EVP_PKEY_new());
-  if (ret == nullptr) {
-    return nullptr;
-  }
-  evp_pkey_set_method(ret.get(), method);
-
-  // Call into the type-specific SPKI decoding function.
-  if (ret->ameth->pub_decode == nullptr) {
-    OPENSSL_PUT_ERROR(EVP, EVP_R_UNSUPPORTED_ALGORITHM);
-    return nullptr;
-  }
-  if (!ret->ameth->pub_decode(ret.get(), &algorithm, &key)) {
+  if (ret == nullptr || !method->pub_decode(ret.get(), &algorithm, &key)) {
     return nullptr;
   }
 
@@ -118,7 +106,7 @@
     return nullptr;
   }
   const EVP_PKEY_ASN1_METHOD *method = parse_key_type(&algorithm);
-  if (method == nullptr) {
+  if (method == nullptr || method->priv_decode == nullptr) {
     OPENSSL_PUT_ERROR(EVP, EVP_R_UNSUPPORTED_ALGORITHM);
     return nullptr;
   }
@@ -127,17 +115,7 @@
 
   // Set up an |EVP_PKEY| of the appropriate type.
   bssl::UniquePtr<EVP_PKEY> ret(EVP_PKEY_new());
-  if (ret == nullptr) {
-    return nullptr;
-  }
-  evp_pkey_set_method(ret.get(), method);
-
-  // Call into the type-specific PrivateKeyInfo decoding function.
-  if (ret->ameth->priv_decode == nullptr) {
-    OPENSSL_PUT_ERROR(EVP, EVP_R_UNSUPPORTED_ALGORITHM);
-    return nullptr;
-  }
-  if (!ret->ameth->priv_decode(ret.get(), &algorithm, &key)) {
+  if (ret == nullptr || !method->priv_decode(ret.get(), &algorithm, &key)) {
     return nullptr;
   }
 

diff --git a/crypto/evp/internal.h b/crypto/evp/internal.h
index 51cfa64..ff6ead3 100644
--- a/crypto/evp/internal.h
+++ b/crypto/evp/internal.h

@@ -259,9 +259,11 @@
 extern const EVP_PKEY_CTX_METHOD hkdf_pkey_meth;
 extern const EVP_PKEY_CTX_METHOD dh_pkey_meth;
 
-// evp_pkey_set_method behaves like |EVP_PKEY_set_type|, but takes a pointer to
-// a method table. This avoids depending on every |EVP_PKEY_ASN1_METHOD|.
-void evp_pkey_set_method(EVP_PKEY *pkey, const EVP_PKEY_ASN1_METHOD *method);
+// evp_pkey_set0 sets |pkey|'s method to |method| and data to |pkey_data|,
+// freeing any key that may previously have been configured. This function takes
+// ownership of |pkey_data|, which must be of the type expected by |method|.
+void evp_pkey_set0(EVP_PKEY *pkey, const EVP_PKEY_ASN1_METHOD *method,
+                   void *pkey_data);
 
 
 #if defined(__cplusplus)

diff --git a/crypto/evp/p_dh_asn1.cc b/crypto/evp/p_dh_asn1.cc
index 1362113..8622de0 100644
--- a/crypto/evp/p_dh_asn1.cc
+++ b/crypto/evp/p_dh_asn1.cc

@@ -123,8 +123,7 @@
   if (key == nullptr) {
     return 0;
   }
-  evp_pkey_set_method(pkey, &dh_asn1_meth);
-  pkey->pkey = key;
+  evp_pkey_set0(pkey, &dh_asn1_meth, key);
   return 1;
 }
 

diff --git a/crypto/evp/p_dsa_asn1.cc b/crypto/evp/p_dsa_asn1.cc
index 6e42764..237b34b 100644
--- a/crypto/evp/p_dsa_asn1.cc
+++ b/crypto/evp/p_dsa_asn1.cc

@@ -254,8 +254,7 @@
   if (key == nullptr) {
     return 0;
   }
-  evp_pkey_set_method(pkey, &dsa_asn1_meth);
-  pkey->pkey = key;
+  evp_pkey_set0(pkey, &dsa_asn1_meth, key);
   return 1;
 }
 

diff --git a/crypto/evp/p_ec_asn1.cc b/crypto/evp/p_ec_asn1.cc
index c950d9b..10bde92 100644
--- a/crypto/evp/p_ec_asn1.cc
+++ b/crypto/evp/p_ec_asn1.cc

@@ -268,8 +268,7 @@
   if (key == nullptr) {
     return 0;
   }
-  evp_pkey_set_method(pkey, &ec_asn1_meth);
-  pkey->pkey = key;
+  evp_pkey_set0(pkey, &ec_asn1_meth, key);
   return 1;
 }
 

diff --git a/crypto/evp/p_ed25519.cc b/crypto/evp/p_ed25519.cc
index d045148..540146a 100644
--- a/crypto/evp/p_ed25519.cc
+++ b/crypto/evp/p_ed25519.cc

@@ -31,14 +31,11 @@
     return 0;
   }
 
-  evp_pkey_set_method(pkey, &ed25519_asn1_meth);
-
   uint8_t pubkey_unused[32];
   ED25519_keypair(pubkey_unused, key->key);
   key->has_private = 1;
 
-  OPENSSL_free(pkey->pkey);
-  pkey->pkey = key;
+  evp_pkey_set0(pkey, &ed25519_asn1_meth, key);
   return 1;
 }
 

diff --git a/crypto/evp/p_ed25519_asn1.cc b/crypto/evp/p_ed25519_asn1.cc
index 8053422..6f1f572 100644
--- a/crypto/evp/p_ed25519_asn1.cc
+++ b/crypto/evp/p_ed25519_asn1.cc

@@ -45,9 +45,7 @@
   uint8_t pubkey_unused[32];
   ED25519_keypair_from_seed(pubkey_unused, key->key, in);
   key->has_private = 1;
-
-  ed25519_free(pkey);
-  pkey->pkey = key;
+  evp_pkey_set0(pkey, &ed25519_asn1_meth, key);
   return 1;
 }
 
@@ -65,9 +63,7 @@
 
   OPENSSL_memcpy(key->key + ED25519_PUBLIC_KEY_OFFSET, in, 32);
   key->has_private = 0;
-
-  ed25519_free(pkey);
-  pkey->pkey = key;
+  evp_pkey_set0(pkey, &ed25519_asn1_meth, key);
   return 1;
 }
 

diff --git a/crypto/evp/p_rsa_asn1.cc b/crypto/evp/p_rsa_asn1.cc
index 99f414e..ab23508 100644
--- a/crypto/evp/p_rsa_asn1.cc
+++ b/crypto/evp/p_rsa_asn1.cc

@@ -179,8 +179,7 @@
   if (key == nullptr) {
     return 0;
   }
-  evp_pkey_set_method(pkey, &rsa_asn1_meth);
-  pkey->pkey = key;
+  evp_pkey_set0(pkey, &rsa_asn1_meth, key);
   return 1;
 }
 

diff --git a/crypto/evp/p_x25519.cc b/crypto/evp/p_x25519.cc
index 91e3975..aa63ddc 100644
--- a/crypto/evp/p_x25519.cc
+++ b/crypto/evp/p_x25519.cc

@@ -31,13 +31,9 @@
     return 0;
   }
 
-  evp_pkey_set_method(pkey, &x25519_asn1_meth);
-
   X25519_keypair(key->pub, key->priv);
   key->has_private = 1;
-
-  OPENSSL_free(pkey->pkey);
-  pkey->pkey = key;
+  evp_pkey_set0(pkey, &x25519_asn1_meth, key);
   return 1;
 }
 

diff --git a/crypto/evp/p_x25519_asn1.cc b/crypto/evp/p_x25519_asn1.cc
index c31dcc5..f37111c 100644
--- a/crypto/evp/p_x25519_asn1.cc
+++ b/crypto/evp/p_x25519_asn1.cc

@@ -44,8 +44,7 @@
   X25519_public_from_private(key->pub, key->priv);
   key->has_private = 1;
 
-  x25519_free(pkey);
-  pkey->pkey = key;
+  evp_pkey_set0(pkey, &x25519_asn1_meth, key);
   return 1;
 }
 
@@ -64,8 +63,7 @@
   OPENSSL_memcpy(key->pub, in, 32);
   key->has_private = 0;
 
-  x25519_free(pkey);
-  pkey->pkey = key;
+  evp_pkey_set0(pkey, &x25519_asn1_meth, key);
   return 1;
 }