Set an EVP_PKEY's algorithm and data together

Cherry-picked from BoringSSL b0ef87e5e3ba9a4f4b5bba4434f6f2e630507b48.

Replace evp_pkey_set_method with evp_pkey_set0, which sets both the
method and key data together, avoiding half-initialized EVP_PKEY states.

Adapted for AWS-LC:
- RSA/DSA/EC_KEY/DH assign functions and EVP_PKEY_new_raw_* are in
  different files than BoringSSL (FIPS module vs evp_extra)
- AWS-LC has a pkey.type field that must also be set (BoringSSL derives
  type from ameth)
- ed25519/x25519 set_priv_raw/set_pub_raw callbacks are shared with
  ed25519ph (AWS-LC specific), so callers (EVP_PKEY_new_raw_*,
  evp_asn1 decode) set the method via evp_pkey_set0 before invoking
  the callback, rather than having the callback set it
- KEM and PQDSA (AWS-LC specific) updated to create key first, then
  call evp_pkey_set0 with method+data together

Author: davidben

crypto/evp_extra/evp_asn1.c

@@ -130,31 +130,25 @@ EVP_PKEY *EVP_parse_public_key(CBS *cbs) {
   CBS oid;
 
   const EVP_PKEY_ASN1_METHOD *method = parse_key_type(&algorithm, &oid);
-  if (method == NULL) {
+  if (method == NULL || method->pub_decode == NULL) {
     OPENSSL_PUT_ERROR(EVP, EVP_R_UNSUPPORTED_ALGORITHM);
     return NULL;
   }
-  if (// Every key type defined encodes the key as a byte string with the same
-      // conversion to BIT STRING.
-      !CBS_get_u8(&key, &padding) ||
+  // 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 NULL;
   }
 
-  // Set up an |EVP_PKEY| of the appropriate type.
   EVP_PKEY *ret = EVP_PKEY_new();
   if (ret == NULL) {
     goto err;
   }
-  evp_pkey_set_method(ret, method);
+  evp_pkey_set0(ret, method, NULL);
 
-  // Call into the type-specific SPKI decoding function.
-  if (ret->ameth->pub_decode == NULL) {
-    OPENSSL_PUT_ERROR(EVP, EVP_R_UNSUPPORTED_ALGORITHM);
-    goto err;
-  }
-  if (!ret->ameth->pub_decode(ret, &oid, &algorithm, &key)) {
+  if (!method->pub_decode(ret, &oid, &algorithm, &key)) {
     goto err;
   }
 
@@ -198,7 +192,7 @@ EVP_PKEY *EVP_parse_private_key(CBS *cbs) {
   CBS oid;
 
   const EVP_PKEY_ASN1_METHOD *method = parse_key_type(&algorithm, &oid);
-  if (method == NULL) {
+  if (method == NULL || method->priv_decode == NULL) {
     OPENSSL_PUT_ERROR(EVP, EVP_R_UNSUPPORTED_ALGORITHM);
     return NULL;
   }
@@ -230,16 +224,10 @@ EVP_PKEY *EVP_parse_private_key(CBS *cbs) {
   if (ret == NULL) {
     goto err;
   }
-  evp_pkey_set_method(ret, method);
-
-  // Call into the type-specific PrivateKeyInfo decoding function.
-  if (ret->ameth->priv_decode == NULL) {
-    OPENSSL_PUT_ERROR(EVP, EVP_R_UNSUPPORTED_ALGORITHM);
-    goto err;
-  }
+  evp_pkey_set0(ret, method, NULL);
 
-  if (!ret->ameth->priv_decode(ret, &oid, &algorithm, &key,
-                               has_pub ? &public_key : NULL)) {
+  if (!method->priv_decode(ret, &oid, &algorithm, &key,
+                           has_pub ? &public_key : NULL)) {
     goto err;
   }
 

crypto/evp_extra/internal.h

@@ -41,9 +41,11 @@ extern const EVP_PKEY_METHOD dh_pkey_meth;
 extern const EVP_PKEY_METHOD dsa_pkey_meth;
 extern const EVP_PKEY_METHOD pqdsa_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);
 
 // Returns a reference to the list |non_fips_pkey_evp_methods|. The list has
 // size |NON_FIPS_EVP_PKEY_METHODS|.

crypto/evp_extra/p_dh_asn1.c

@@ -174,8 +174,7 @@ int EVP_PKEY_assign_DH(EVP_PKEY *pkey, DH *key) {
   if (key == NULL) {
     return 0;
   }
-  evp_pkey_set_method(pkey, &dh_asn1_meth);
-  pkey->pkey.dh = key;
+  evp_pkey_set0(pkey, &dh_asn1_meth, key);
   return 1;
 }
 

crypto/evp_extra/p_x25519.c

@@ -31,13 +31,10 @@ 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.ptr);
-  pkey->pkey.ptr = key;
+  evp_pkey_set0(pkey, &x25519_asn1_meth, key);
   return 1;
 }
 

crypto/fipsmodule/evp/evp.c

@@ -236,7 +236,11 @@ int EVP_read_pw_string_min(char *buf, int min_length, int length,
 int EVP_PKEY_copy_parameters(EVP_PKEY *to, const EVP_PKEY *from) {
   SET_DIT_AUTO_RESET;
   if (to->type == 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, NULL);
   } else if (to->type != from->type) {
     OPENSSL_PUT_ERROR(EVP, EVP_R_DIFFERENT_KEY_TYPES);
     return 0;
@@ -346,18 +350,20 @@ static const EVP_PKEY_ASN1_METHOD *evp_pkey_asn1_find(int nid) {
   return NULL;
 }
 
-void evp_pkey_set_method(EVP_PKEY *pkey, const EVP_PKEY_ASN1_METHOD *method) {
+void evp_pkey_set0(EVP_PKEY *pkey, const EVP_PKEY_ASN1_METHOD *method,
+                   void *pkey_data) {
   free_it(pkey);
   pkey->ameth = method;
-  pkey->type = pkey->ameth->pkey_id;
+  pkey->type = method ? method->pkey_id : EVP_PKEY_NONE;
+  pkey->pkey.ptr = pkey_data;
 }
 
 static int pkey_set_type(EVP_PKEY *pkey, int type, const char *str, int len) {
   if (pkey && pkey->pkey.ptr) {
     // This isn't strictly necessary, but historically |EVP_PKEY_set_type| would
     // clear |pkey| even if |evp_pkey_asn1_find| failed, so we preserve that
     // behavior.
-    free_it(pkey);
+    evp_pkey_set0(pkey, NULL, NULL);
   }
 
   const EVP_PKEY_ASN1_METHOD *ameth = NULL;
@@ -375,7 +381,7 @@ static int pkey_set_type(EVP_PKEY *pkey, int type, const char *str, int len) {
   }
 
   if (pkey) {
-    evp_pkey_set_method(pkey, ameth);
+    evp_pkey_set0(pkey, ameth, NULL);
   }
 
   return 1;
@@ -446,8 +452,7 @@ int EVP_PKEY_assign_RSA(EVP_PKEY *pkey, RSA *key) {
   }
   const EVP_PKEY_ASN1_METHOD *meth = evp_pkey_asn1_find(EVP_PKEY_RSA);
   assert(meth != NULL);
-  evp_pkey_set_method(pkey, meth);
-  pkey->pkey.ptr = key;
+  evp_pkey_set0(pkey, meth, key);
   return 1;
 }
 
@@ -485,8 +490,7 @@ int EVP_PKEY_assign_DSA(EVP_PKEY *pkey, DSA *key) {
   }
   const EVP_PKEY_ASN1_METHOD *meth = evp_pkey_asn1_find(EVP_PKEY_DSA);
   assert(meth != NULL);
-  evp_pkey_set_method(pkey, meth);
-  pkey->pkey.ptr = key;
+  evp_pkey_set0(pkey, meth, key);
   return 1;
 }
 
@@ -524,8 +528,7 @@ int EVP_PKEY_assign_EC_KEY(EVP_PKEY *pkey, EC_KEY *key) {
   }
   const EVP_PKEY_ASN1_METHOD *meth = evp_pkey_asn1_find(EVP_PKEY_EC);
   assert(meth != NULL);
-  evp_pkey_set_method(pkey, meth);
-  pkey->pkey.ptr = key;
+  evp_pkey_set0(pkey, meth, key);
   return 1;
 }
 
@@ -608,9 +611,9 @@ EVP_PKEY *EVP_PKEY_new_raw_private_key(int type, ENGINE *unused,
   if (ret == NULL) {
     goto err;
   }
-  evp_pkey_set_method(ret, method);
+  evp_pkey_set0(ret, method, NULL);
 
-  if (!ret->ameth->set_priv_raw(ret, in, len, NULL, 0)) {
+  if (!method->set_priv_raw(ret, in, len, NULL, 0)) {
     goto err;
   }
 
@@ -645,9 +648,9 @@ EVP_PKEY *EVP_PKEY_new_raw_public_key(int type, ENGINE *unused,
   if (ret == NULL) {
     goto err;
   }
-  evp_pkey_set_method(ret, method);
+  evp_pkey_set0(ret, method, NULL);
 
-  if (!ret->ameth->set_pub_raw(ret, in, len)) {
+  if (!method->set_pub_raw(ret, in, len)) {
     goto err;
   }
 

crypto/fipsmodule/evp/p_ed25519.c

@@ -31,14 +31,13 @@ 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];
   int result = ED25519_keypair_internal(pubkey_unused, key->key);
   if (result) {
     key->has_private = 1;
-    OPENSSL_free(pkey->pkey.ptr);
-    pkey->pkey.ptr = key;
+    evp_pkey_set0(pkey, &ed25519_asn1_meth, key);
+  } else {
+    OPENSSL_free(key);
   }
 
   return result;

crypto/fipsmodule/evp/p_kem.c

@@ -366,16 +366,14 @@ int EVP_PKEY_kem_set_params(EVP_PKEY *pkey, int nid) {
     return 0;
   }
 
-  evp_pkey_set_method(pkey, &kem_asn1_meth);
-
   KEM_KEY *key = KEM_KEY_new();
   if (key == NULL) {
     // KEM_KEY_new sets the appropriate error.
     return 0;
   }
 
   key->kem = kem;
-  pkey->pkey.kem_key = key;
+  evp_pkey_set0(pkey, &kem_asn1_meth, key);
 
   return 1;
 }

crypto/fipsmodule/evp/p_pqdsa.c

@@ -225,16 +225,14 @@ int EVP_PKEY_pqdsa_set_params(EVP_PKEY *pkey, int nid) {
     return 0;
   }
 
-  evp_pkey_set_method(pkey, &pqdsa_asn1_meth);
-
   PQDSA_KEY *key = PQDSA_KEY_new();
   if (key == NULL) {
     // PQDSA_KEY_new sets the appropriate error.
     return 0;
   }
 
   key->pqdsa = pqdsa;
-  pkey->pkey.pqdsa_key = key;
+  evp_pkey_set0(pkey, &pqdsa_asn1_meth, key);
 
   return 1;
 }