Limit EVP_PKEY_set_type to EVP_PKEY_X25519
Per the docs, the only real use case is setting EVP_PKEY_X25519. Outside
BoringSSL, the one caller using other key types to make test keys has
since been fixed. One call to EVP_PKEY_set_type(EVP_PKEY_NONE) remains,
but this CL does not break it.
This removes one path to making this flavor of half-empty keys for all
but X25519, where we're forced to have such keys for OpenSSL
compatibility.
Update-Note: EVP_PKEY_set_type will now only succeed for
EVP_PKEY_X25519. EVP_PKEY_set_type(EVP_PKEY_NONE) will continue to clear
the pkey and then fail. Going through code search, there are not
expected to be any affected callers.
Bug: 42290409
Change-Id: I20ca762be71f71a628f5894045e5b66b7773c215
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81548
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 2275c62..786e9b7 100644
--- a/crypto/evp/evp.cc
+++ b/crypto/evp/evp.cc
@@ -157,26 +157,6 @@
return pkey->ameth != nullptr ? pkey->ameth->pkey_id : EVP_PKEY_NONE;
}
-// evp_pkey_asn1_find returns the ASN.1 method table for the given |nid|, which
-// should be one of the |EVP_PKEY_*| values. It returns NULL if |nid| is
-// unknown.
-static const EVP_PKEY_ASN1_METHOD *evp_pkey_asn1_find(int nid) {
- switch (nid) {
- case EVP_PKEY_RSA:
- return &rsa_asn1_meth;
- case EVP_PKEY_EC:
- return &ec_asn1_meth;
- case EVP_PKEY_DSA:
- return &dsa_asn1_meth;
- case EVP_PKEY_ED25519:
- return &ed25519_asn1_meth;
- case EVP_PKEY_X25519:
- return &x25519_asn1_meth;
- default:
- return NULL;
- }
-}
-
void evp_pkey_set_method(EVP_PKEY *pkey, const EVP_PKEY_ASN1_METHOD *method) {
free_it(pkey);
pkey->ameth = method;
@@ -215,8 +195,14 @@
free_it(pkey);
}
- const EVP_PKEY_ASN1_METHOD *ameth = evp_pkey_asn1_find(type);
- if (ameth == NULL) {
+ // This function broadly isn't useful. It initializes |EVP_PKEY| for a type,
+ // but forgets to put anything in the |pkey|. The one pattern where it does
+ // anything is |EVP_PKEY_X25519|, where it's needed to make
+ // |EVP_PKEY_set1_tls_encodedpoint| work, so we support only that.
+ const EVP_PKEY_ASN1_METHOD *ameth;
+ if (type == EVP_PKEY_X25519) {
+ ameth = &x25519_asn1_meth;
+ } else {
OPENSSL_PUT_ERROR(EVP, EVP_R_UNSUPPORTED_ALGORITHM);
ERR_add_error_dataf("algorithm %d", type);
return 0;
diff --git a/crypto/evp/evp_extra_test.cc b/crypto/evp/evp_extra_test.cc
index d7e39f5..12af146 100644
--- a/crypto/evp/evp_extra_test.cc
+++ b/crypto/evp/evp_extra_test.cc
@@ -1065,15 +1065,6 @@
}
TEST(EVPExtraTest, Parameters) {
- auto new_pkey_with_type = [](int type) -> bssl::UniquePtr<EVP_PKEY> {
- bssl::UniquePtr<EVP_PKEY> pkey(EVP_PKEY_new());
- if (!pkey || //
- !EVP_PKEY_set_type(pkey.get(), type)) {
- return nullptr;
- }
- return pkey;
- };
-
auto new_pkey_with_curve = [](int curve_nid) -> bssl::UniquePtr<EVP_PKEY> {
bssl::UniquePtr<EVP_PKEY_CTX> ctx(
EVP_PKEY_CTX_new_id(EVP_PKEY_EC, nullptr));
@@ -1088,13 +1079,15 @@
};
// RSA keys have no parameters.
- bssl::UniquePtr<EVP_PKEY> rsa = new_pkey_with_type(EVP_PKEY_RSA);
+ bssl::UniquePtr<EVP_PKEY> rsa = LoadExampleRSAKey();
ASSERT_TRUE(rsa);
EXPECT_FALSE(EVP_PKEY_missing_parameters(rsa.get()));
- // EC keys have parameters.
- bssl::UniquePtr<EVP_PKEY> ec_no_params = new_pkey_with_type(EVP_PKEY_EC);
+ // EC keys have parameters, but it is possible to initialize an |EVP_PKEY|
+ // with a completely empty |EC_KEY|.
+ bssl::UniquePtr<EVP_PKEY> ec_no_params(EVP_PKEY_new());
ASSERT_TRUE(ec_no_params);
+ ASSERT_TRUE(EVP_PKEY_assign_EC_KEY(ec_no_params.get(), EC_KEY_new()));
EXPECT_TRUE(EVP_PKEY_missing_parameters(ec_no_params.get()));
bssl::UniquePtr<EVP_PKEY> p256 = new_pkey_with_curve(NID_X9_62_prime256v1);
@@ -1250,3 +1243,23 @@
ASSERT_TRUE(CBB_init(cbb.get(), 0));
EXPECT_FALSE(EVP_marshal_private_key(cbb.get(), pkey.get()));
}
+
+// APIs should avoid creating an |EVP_PKEY| that is missing its underlying data.
+// In some cases, we are stuck with this due to OpenSSL compatibility, but it is
+// preferable to reduce the number of kinds of half-empty states.
+TEST(EVPExtraTest, NoHalfEmptyKeys) {
+ bssl::UniquePtr<EVP_PKEY> pkey(EVP_PKEY_new());
+ ASSERT_TRUE(pkey);
+
+ EXPECT_FALSE(EVP_PKEY_set_type(pkey.get(), EVP_PKEY_RSA));
+ EXPECT_EQ(EVP_PKEY_id(pkey.get()), EVP_PKEY_NONE);
+
+ EXPECT_FALSE(EVP_PKEY_set_type(pkey.get(), EVP_PKEY_EC));
+ EXPECT_EQ(EVP_PKEY_id(pkey.get()), EVP_PKEY_NONE);
+
+ EXPECT_FALSE(EVP_PKEY_set_type(pkey.get(), EVP_PKEY_DH));
+ EXPECT_EQ(EVP_PKEY_id(pkey.get()), EVP_PKEY_NONE);
+
+ EXPECT_FALSE(EVP_PKEY_set_type(pkey.get(), EVP_PKEY_DSA));
+ EXPECT_EQ(EVP_PKEY_id(pkey.get()), EVP_PKEY_NONE);
+}
diff --git a/include/openssl/evp.h b/include/openssl/evp.h
index 8ce51fa..260db82 100644
--- a/include/openssl/evp.h
+++ b/include/openssl/evp.h
@@ -921,7 +921,8 @@
// EVP_PKEY_set_type sets the type of |pkey| to |type|. It returns one if
// successful or zero if the |type| argument is not one of the |EVP_PKEY_*|
-// values. If |pkey| is NULL, it simply reports whether the type is known.
+// values supported for use with this function. If |pkey| is NULL, it simply
+// reports whether the type is known.
//
// There are very few cases where this function is useful. Changing |pkey|'s
// type clears any previously stored keys, so there is no benefit to loading a
@@ -933,7 +934,8 @@
//
// The only API pattern which requires this function is
// |EVP_PKEY_set1_tls_encodedpoint| with X25519, which requires a half-empty
-// |EVP_PKEY| that was first configured with |EVP_PKEY_X25519|.
+// |EVP_PKEY| that was first configured with |EVP_PKEY_X25519|. Currently, all
+// other values of |type| will result in an error.
OPENSSL_EXPORT int EVP_PKEY_set_type(EVP_PKEY *pkey, int type);
// EVP_PKEY_set1_tls_encodedpoint replaces |pkey| with a public key encoded by
