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>

Author: davidben

crypto/evp/evp.cc

@@ -45,14 +45,6 @@ EVP_PKEY *EVP_PKEY_new(void) {
   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 @@ void EVP_PKEY_free(EVP_PKEY *pkey) {
     return;
   }
 
-  free_it(pkey);
+  evp_pkey_set0(pkey, nullptr, nullptr);
   OPENSSL_free(pkey);
 }
 
@@ -103,7 +95,11 @@ int EVP_PKEY_cmp(const EVP_PKEY *a, const EVP_PKEY *b) {
 
 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 @@ int EVP_PKEY_copy_parameters(EVP_PKEY *to, const EVP_PKEY *from) {
     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 @@ int EVP_PKEY_id(const EVP_PKEY *pkey) {
   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 @@ int EVP_PKEY_set_type(EVP_PKEY *pkey, int type) {
   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 @@ int EVP_PKEY_set_type(EVP_PKEY *pkey, int type) {
   }
 
   if (pkey) {
-    evp_pkey_set_method(pkey, ameth);
+    evp_pkey_set0(pkey, ameth, nullptr);
   }
 
   return 1;
@@ -233,12 +234,8 @@ EVP_PKEY *EVP_PKEY_new_raw_private_key(int type, ENGINE *unused,
   }
 
   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 @@ EVP_PKEY *EVP_PKEY_new_raw_public_key(int type, ENGINE *unused,
   }
 
   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;
   }
 

crypto/evp/evp_asn1.cc

@@ -65,31 +65,19 @@ EVP_PKEY *EVP_parse_public_key(CBS *cbs) {
     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 @@ EVP_PKEY *EVP_parse_private_key(CBS *cbs) {
     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 @@ EVP_PKEY *EVP_parse_private_key(CBS *cbs) {
 
   // 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;
   }
 

crypto/evp/internal.h

@@ -259,9 +259,11 @@ extern const EVP_PKEY_CTX_METHOD x25519_pkey_meth;
 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)

crypto/evp/p_dh_asn1.cc

@@ -123,8 +123,7 @@ int EVP_PKEY_assign_DH(EVP_PKEY *pkey, DH *key) {
   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;
 }
 

crypto/evp/p_dsa_asn1.cc

@@ -254,8 +254,7 @@ int EVP_PKEY_assign_DSA(EVP_PKEY *pkey, DSA *key) {
   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;
 }
 

crypto/evp/p_ec_asn1.cc

@@ -268,8 +268,7 @@ int EVP_PKEY_assign_EC_KEY(EVP_PKEY *pkey, EC_KEY *key) {
   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;
 }
 

crypto/evp/p_ed25519.cc

@@ -31,14 +31,11 @@ static int pkey_ed25519_keygen(EVP_PKEY_CTX *ctx, EVP_PKEY *pkey) {
     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;
 }
 

crypto/evp/p_ed25519_asn1.cc

@@ -45,9 +45,7 @@ static int ed25519_set_priv_raw(EVP_PKEY *pkey, const uint8_t *in, size_t len) {
   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 @@ static int ed25519_set_pub_raw(EVP_PKEY *pkey, const uint8_t *in, size_t len) {
 
   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;
 }
 

crypto/evp/p_rsa_asn1.cc

@@ -179,8 +179,7 @@ int EVP_PKEY_assign_RSA(EVP_PKEY *pkey, RSA *key) {
   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;
 }
 

crypto/evp/p_x25519.cc

@@ -31,13 +31,9 @@ static int pkey_x25519_keygen(EVP_PKEY_CTX *ctx, EVP_PKEY *pkey) {
     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;
 }