Skip to content

Commit 5b7171f

Browse files
davidbenBoringssl LUCI CQ
authored andcommitted
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>
1 parent 9b602f2 commit 5b7171f

File tree

5 files changed

+24
-4
lines changed

5 files changed

+24
-4
lines changed

crypto/evp/evp_extra_test.cc

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1253,13 +1253,21 @@ TEST(EVPExtraTest, NoHalfEmptyKeys) {
12531253

12541254
EXPECT_FALSE(EVP_PKEY_set_type(pkey.get(), EVP_PKEY_RSA));
12551255
EXPECT_EQ(EVP_PKEY_id(pkey.get()), EVP_PKEY_NONE);
1256+
EXPECT_FALSE(EVP_PKEY_set1_RSA(pkey.get(), nullptr));
1257+
EXPECT_EQ(EVP_PKEY_id(pkey.get()), EVP_PKEY_NONE);
12561258

12571259
EXPECT_FALSE(EVP_PKEY_set_type(pkey.get(), EVP_PKEY_EC));
12581260
EXPECT_EQ(EVP_PKEY_id(pkey.get()), EVP_PKEY_NONE);
1261+
EXPECT_FALSE(EVP_PKEY_set1_EC_KEY(pkey.get(), nullptr));
1262+
EXPECT_EQ(EVP_PKEY_id(pkey.get()), EVP_PKEY_NONE);
12591263

12601264
EXPECT_FALSE(EVP_PKEY_set_type(pkey.get(), EVP_PKEY_DH));
12611265
EXPECT_EQ(EVP_PKEY_id(pkey.get()), EVP_PKEY_NONE);
1266+
EXPECT_FALSE(EVP_PKEY_set1_DH(pkey.get(), nullptr));
1267+
EXPECT_EQ(EVP_PKEY_id(pkey.get()), EVP_PKEY_NONE);
12621268

12631269
EXPECT_FALSE(EVP_PKEY_set_type(pkey.get(), EVP_PKEY_DSA));
12641270
EXPECT_EQ(EVP_PKEY_id(pkey.get()), EVP_PKEY_NONE);
1271+
EXPECT_FALSE(EVP_PKEY_set1_DSA(pkey.get(), nullptr));
1272+
EXPECT_EQ(EVP_PKEY_id(pkey.get()), EVP_PKEY_NONE);
12651273
}

crypto/evp/p_dh_asn1.cc

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -120,9 +120,12 @@ int EVP_PKEY_set1_DH(EVP_PKEY *pkey, DH *key) {
120120
}
121121

122122
int EVP_PKEY_assign_DH(EVP_PKEY *pkey, DH *key) {
123+
if (key == nullptr) {
124+
return 0;
125+
}
123126
evp_pkey_set_method(pkey, &dh_asn1_meth);
124127
pkey->pkey = key;
125-
return key != NULL;
128+
return 1;
126129
}
127130

128131
DH *EVP_PKEY_get0_DH(const EVP_PKEY *pkey) {

crypto/evp/p_dsa_asn1.cc

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -251,9 +251,12 @@ int EVP_PKEY_set1_DSA(EVP_PKEY *pkey, DSA *key) {
251251
}
252252

253253
int EVP_PKEY_assign_DSA(EVP_PKEY *pkey, DSA *key) {
254+
if (key == nullptr) {
255+
return 0;
256+
}
254257
evp_pkey_set_method(pkey, &dsa_asn1_meth);
255258
pkey->pkey = key;
256-
return key != nullptr;
259+
return 1;
257260
}
258261

259262
DSA *EVP_PKEY_get0_DSA(const EVP_PKEY *pkey) {

crypto/evp/p_ec_asn1.cc

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -265,9 +265,12 @@ int EVP_PKEY_set1_EC_KEY(EVP_PKEY *pkey, EC_KEY *key) {
265265
}
266266

267267
int EVP_PKEY_assign_EC_KEY(EVP_PKEY *pkey, EC_KEY *key) {
268+
if (key == nullptr) {
269+
return 0;
270+
}
268271
evp_pkey_set_method(pkey, &ec_asn1_meth);
269272
pkey->pkey = key;
270-
return key != NULL;
273+
return 1;
271274
}
272275

273276
EC_KEY *EVP_PKEY_get0_EC_KEY(const EVP_PKEY *pkey) {

crypto/evp/p_rsa_asn1.cc

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -176,9 +176,12 @@ int EVP_PKEY_set1_RSA(EVP_PKEY *pkey, RSA *key) {
176176
}
177177

178178
int EVP_PKEY_assign_RSA(EVP_PKEY *pkey, RSA *key) {
179+
if (key == nullptr) {
180+
return 0;
181+
}
179182
evp_pkey_set_method(pkey, &rsa_asn1_meth);
180183
pkey->pkey = key;
181-
return key != NULL;
184+
return 1;
182185
}
183186

184187
RSA *EVP_PKEY_get0_RSA(const EVP_PKEY *pkey) {

0 commit comments

Comments
 (0)