BoringSSL: Don't support parameterless DSA keys in SPKIs AND Set an EVP_PKEY's algorithm and data together (#3057)

*Note*: to be rebased on #3056.

### Description of changes: 
#### BoringSSL commit
google/boringssl@1bc58a3:
"Don't support parameterless DSA keys in SPKIs"

Changes (2 files):

- **crypto/evp_extra/p_dsa_asn1.c** — Removed the `CBS_len(params) == 0`
branch in `dsa_pub_decode` that created an empty `DSA_new()` without
parameters. Parameterless DSA keys in
`SubjectPublicKeyInfo` will no longer parse. This does not impact TLS,
where we have never supported DSA.
- **crypto/evp_extra/evp_tests.txt** — Updated the
DSA-1024-SPKI-No-Params test to expect `DECODE_ERROR` instead of
successful parsing.

#### BoringSSL commit
google/boringssl@b0ef87e:
"Set an EVP_PKEY's algorithm and data together"

Changes (8 files):

- **crypto/fipsmodule/evp/evp.c** — Replaced `evp_pkey_set_method` with
`evp_pkey_set0(pkey, method, pkey_data)` which sets `method`, `type`,
and `data` atomically. Removed `free_it` from
`pkey_set_type` (uses `evp_pkey_set0(pkey, NULL, NULL)` instead).
`EVP_PKEY_new_raw_*` functions set `method` via `evp_pkey_set0` before
calling callbacks.
- In `evp_pkey_set0` implementation: BoringSSL doesn't have the `type`
field — it derives the `type` from `ameth` at query time via
`EVP_PKEY_id()`. AWS-LC stores type as a separate field
that must be kept in sync.
- **crypto/evp_extra/internal.h** — Updated declaration
- **crypto/evp_extra/evp_asn1.c** — Moved `pub_decode`/`priv_decode`
NULL checks earlier; set method via `evp_pkey_set0` before calling
decode callbacks
- **crypto/evp_extra/p_dh_asn1.c**, **crypto/evp_extra/p_x25519.c**,
**crypto/fipsmodule/evp/p_ed25519.c** — Use `evp_pkey_set0` in keygen
- **crypto/fipsmodule/evp/p_kem.c**, **crypto/fipsmodule/evp/p_pqdsa.c**
— Create key first, then `evp_pkey_set0(pkey, method, key)` together

### Adaptations for AWS-LC:
ed25519/x25519 set_priv_raw/set_pub_raw callbacks are shared with
ed25519ph (AWS-LC specific), so the method must be set by the caller
before invoking the callback, not by the
callback itself.

#### Details
crypto/fipsmodule/evp/evp.c:
- `EVP_PKEY_new_raw_private_key` — calls `evp_pkey_set0(ret, method,
NULL)` then `method->set_priv_raw(...)`
- `EVP_PKEY_new_raw_public_key` — calls `evp_pkey_set0(ret, method,
NULL)` then `method->set_pub_raw(...)`

crypto/evp_extra/evp_asn1.c:
- `EVP_parse_public_key` — calls `evp_pkey_set0(ret, method, NULL)` then
`method->pub_decode(...)`
- `EVP_parse_private_key` — calls `evp_pkey_set0(ret, method, NULL)`
then `method->priv_decode(...)`

In BoringSSL's version, all four of these just called
`method->callback(ret, ...)` directly (no `method` pre-setting), and the
callbacks themselves called evp_pkey_set0. We couldn't do that because
the ed25519 callbacks are shared with ed25519ph.

By submitting this pull request, I confirm that my contribution is made
under the terms of the Apache 2.0 license and the ISC license.

---------

Co-authored-by: David Benjamin <davidben@google.com>

Author: nebeid

crypto/evp_extra/evp_asn1.c

@@ -77,31 +77,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;
   }
 
@@ -145,7 +139,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;
   }
@@ -177,16 +171,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/evp_tests.txt

@@ -208,12 +208,11 @@ Input = 308201b73082012c06072a8648ce3804013082011f02818100b3429b8b128c9079f9b72e
 ExpectNoRawPrivate
 ExpectNoRawPublic
 
-# The same key as above, but without the parameters.
+# The same key as above, but without the parameters. Although allowed by
+# RFC 3279, we do not support this.
 PublicKey = DSA-1024-SPKI-No-Params
-Type = DSA
 Input = 308192300906072a8648ce38040103818400028180258c30ebbb7f34fdc873ce679f6cea373c7886d75d4421b90920db034daedd292c64d8edd8cdbdd7f3ad23d74cfa2135247d0cef6ecf2e14f99e19d22a8c1266bd8fb8719c0e5667c716c45c7adbdabe548085bdad2dfee636f8d52fd6adb2193df6c4f0520fbd171b91882e0e4f321f8250ffecf4dbea00e114427d3ef96c1a
-ExpectNoRawPrivate
-ExpectNoRawPublic
+Error = DECODE_ERROR
 
 # DSA 1024 PKCS#8 v2 decoding with public key not supported
 PrivateKey = DSA-1024-WithPublicKey

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

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

crypto/evp_extra/p_dsa_asn1.c

@@ -16,21 +16,12 @@
 
 
 static int dsa_pub_decode(EVP_PKEY *out, CBS *oid, CBS *params, CBS *key) {
-  // See RFC 3279, section 2.3.2.
-
-  // Parameters may or may not be present.
-  DSA *dsa;
-  if (CBS_len(params) == 0) {
-    dsa = DSA_new();
-    if (dsa == NULL) {
-      return 0;
-    }
-  } else {
-    dsa = DSA_parse_parameters(params);
-    if (dsa == NULL || CBS_len(params) != 0) {
-      OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR);
-      goto err;
-    }
+  // See RFC 3279, section 2.3.2. Although RFC 3279 allows DSA parameters to be
+  // omitted, we require them to be present.
+  DSA *dsa = DSA_parse_parameters(params);
+  if (dsa == NULL || CBS_len(params) != 0) {
+    OPENSSL_PUT_ERROR(EVP, EVP_R_DECODE_ERROR);
+    goto err;
   }
 
   dsa->pub_key = BN_new();

crypto/evp_extra/p_x25519.c

@@ -20,13 +20,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

@@ -183,7 +183,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;
@@ -293,18 +297,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;
@@ -322,7 +328,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;
@@ -393,8 +399,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;
 }
 
@@ -432,8 +437,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;
 }
 
@@ -471,8 +475,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;
 }
 
@@ -555,9 +558,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;
   }
 
@@ -592,9 +595,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

@@ -8,6 +8,7 @@
 #include <openssl/mem.h>
 
 #include "internal.h"
+#include "../../evp_extra/internal.h"
 #include "../curve25519/internal.h"
 
 
@@ -20,14 +21,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

@@ -411,16 +411,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

@@ -233,16 +233,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;
 }