Make some more half-empty EVP_PKEY states impossible
EVP_PKEY_{assign,set1}_FOO would check for NULL, return an error, but
still leave the EVP_PKEY assigned to that type on failure. Check for
NULL first, so that we don't leave it in that state.
I originally did this with a slightly more ambitious goal of also
banning EVP_PKEY_set1_EC_KEY if the EC_KEY has no parameters. That was
so that, in the happy future where we have EVP_PKEY_ALGs for P-256 and
P-384, EVP_PKEY_ASN1_METHOD would simply be renamed EVP_PKEY_ALG and
every key would have an associated EVP_PKEY_ALG.
For that to work, EVP_PKEY_set1_EC_KEY must never be ambiguous about
which EVP_PKEY_ALG to associate with the EVP_PKEY.
However, the existence of custom EC_GROUPs throws a spanner in that.
We need to support EVP_PKEY_set1_EC_KEY with an custom EC_GROUP (at
least until we manage to get Conscrypt to stop using this function). So,
at least for now, I'm thinking we say that EVP_PKEY_ALGs point to
EVP_PKEY_ASN1_METHODs but you can't go from EVP_PKEY back to
EVP_PKEY_ALG, and we'll see how irksome of an API that becomes.
(We can always go back to this idea later. The custom EC_GROUPs thing
isn't fatal if EC_KEYs with funny EC_GROUPs map to some goofy private
EVP_PKEY_ALG that can't parse anything.)
Still, half-empty states are generally bad, so I'm going to keep this
change on the branch and see if we can get it to stick.
Update-Note: Some half-empty, invalid EVP_PKEY states are now
impossible. Running through tests, no callers were tripping this. There
seems to be no legitimate reason to do this.
Bug: 42290409
Change-Id: I0211a38ab62268a05e3ff1d138a092e4feec10b1
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81549
Reviewed-by: Lily Chen <chlily@google.com>
Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/evp/evp_extra_test.cc b/crypto/evp/evp_extra_test.cc
index 12af146..f69ccbe 100644
--- a/crypto/evp/evp_extra_test.cc
+++ b/crypto/evp/evp_extra_test.cc
@@ -1253,13 +1253,21 @@
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_set1_RSA(pkey.get(), nullptr));
+ 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_set1_EC_KEY(pkey.get(), nullptr));
+ 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_set1_DH(pkey.get(), nullptr));
+ 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);
+ EXPECT_FALSE(EVP_PKEY_set1_DSA(pkey.get(), nullptr));
+ EXPECT_EQ(EVP_PKEY_id(pkey.get()), EVP_PKEY_NONE);
}
diff --git a/crypto/evp/p_dh_asn1.cc b/crypto/evp/p_dh_asn1.cc
index 0f86ed4..1362113 100644
--- a/crypto/evp/p_dh_asn1.cc
+++ b/crypto/evp/p_dh_asn1.cc
@@ -120,9 +120,12 @@
}
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;
- return key != NULL;
+ return 1;
}
DH *EVP_PKEY_get0_DH(const EVP_PKEY *pkey) {
diff --git a/crypto/evp/p_dsa_asn1.cc b/crypto/evp/p_dsa_asn1.cc
index b07a982..6e42764 100644
--- a/crypto/evp/p_dsa_asn1.cc
+++ b/crypto/evp/p_dsa_asn1.cc
@@ -251,9 +251,12 @@
}
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;
- return key != nullptr;
+ return 1;
}
DSA *EVP_PKEY_get0_DSA(const EVP_PKEY *pkey) {
diff --git a/crypto/evp/p_ec_asn1.cc b/crypto/evp/p_ec_asn1.cc
index c8047db..c950d9b 100644
--- a/crypto/evp/p_ec_asn1.cc
+++ b/crypto/evp/p_ec_asn1.cc
@@ -265,9 +265,12 @@
}
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;
- return key != NULL;
+ return 1;
}
EC_KEY *EVP_PKEY_get0_EC_KEY(const EVP_PKEY *pkey) {
diff --git a/crypto/evp/p_rsa_asn1.cc b/crypto/evp/p_rsa_asn1.cc
index 1c6f30b..99f414e 100644
--- a/crypto/evp/p_rsa_asn1.cc
+++ b/crypto/evp/p_rsa_asn1.cc
@@ -176,9 +176,12 @@
}
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;
- return key != NULL;
+ return 1;
}
RSA *EVP_PKEY_get0_RSA(const EVP_PKEY *pkey) {