Don't allow half-empty EVP_PKEYs to be passed into various APIs OpenSSL's API is poorly-typed and exposes many variations on empty states. They are not reachable from typical callers, this is merely a typing error in the OpenSSL API. Performing operations on such objects will sometimes crash. While not ideal, we do not consider this to be a security issue, nor much of a functional bug. Better would be to fix the APIs to eliminate as many half-empty states as possible, as we've been doing in crbug.com/42290409. Still, the X25519 one is difficult to avoid, so add some EVP-wide null checks on a few functions. Ultimately, however, this is a caller error. Bug: 42290409 Fixed: 489983951 Change-Id: I21dca6b3ee84659d812b9c17e5410fc5f516a7d0 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/90447 Auto-Submit: David Benjamin <davidben@google.com> Reviewed-by: Lily Chen <chlily@google.com> Commit-Queue: David Benjamin <davidben@google.com>
diff --git a/crypto/evp/evp.cc b/crypto/evp/evp.cc index 3ac7896..d3b945f 100644 --- a/crypto/evp/evp.cc +++ b/crypto/evp/evp.cc
@@ -75,6 +75,7 @@ auto *a_impl = FromOpaque(a); auto *b_impl = FromOpaque(b); return a_impl->ameth != nullptr && a_impl->ameth->pub_equal != nullptr && + a_impl->pkey != nullptr && b_impl->pkey != nullptr && a_impl->ameth->pub_equal(a_impl, b_impl); } @@ -119,10 +120,18 @@ int EVP_PKEY_missing_parameters(const EVP_PKEY *pkey) { auto *impl = FromOpaque(pkey); - if (impl->ameth && impl->ameth->param_missing) { + if (impl->ameth == nullptr) { + return 0; // EVP_PKEY_NONE is not parameterized, so nothing is missing. + } + if (impl->pkey == nullptr) { + // This is an invalid, half-empty object. Report something is missing to + // stop other parameter-based functions. + return 1; + } + if (impl->ameth->param_missing) { return impl->ameth->param_missing(impl); } - return 0; + return 0; // Not parameterized, so nothing is missing. } int EVP_PKEY_size(const EVP_PKEY *pkey) { @@ -398,8 +407,7 @@ int EVP_PKEY_has_public(const EVP_PKEY *pkey) { auto *impl = FromOpaque(pkey); - - if (impl == nullptr || impl->ameth == nullptr || + if (impl == nullptr || impl->ameth == nullptr || impl->pkey == nullptr || impl->ameth->pub_present == nullptr) { return 0; } @@ -408,8 +416,7 @@ int EVP_PKEY_has_private(const EVP_PKEY *pkey) { auto *impl = FromOpaque(pkey); - - if (impl == nullptr || impl->ameth == nullptr || + if (impl == nullptr || impl->ameth == nullptr || impl->pkey == nullptr || impl->ameth->priv_present == nullptr) { return 0; }
diff --git a/crypto/evp/evp_asn1.cc b/crypto/evp/evp_asn1.cc index f0e6d9a..e32a6f7 100644 --- a/crypto/evp/evp_asn1.cc +++ b/crypto/evp/evp_asn1.cc
@@ -89,6 +89,10 @@ OPENSSL_PUT_ERROR(EVP, EVP_R_UNSUPPORTED_ALGORITHM); return 0; } + if (impl->pkey == nullptr) { + OPENSSL_PUT_ERROR(EVP, EVP_R_NO_KEY_SET); + return 0; + } return impl->ameth->pub_encode(cbb, impl); } @@ -142,6 +146,10 @@ OPENSSL_PUT_ERROR(EVP, EVP_R_UNSUPPORTED_ALGORITHM); return 0; } + if (impl->pkey == nullptr) { + OPENSSL_PUT_ERROR(EVP, EVP_R_NO_KEY_SET); + return 0; + } return impl->ameth->priv_encode(cbb, impl); }
diff --git a/crypto/evp/evp_ctx.cc b/crypto/evp/evp_ctx.cc index 4e0ad0a..1a827bb 100644 --- a/crypto/evp/evp_ctx.cc +++ b/crypto/evp/evp_ctx.cc
@@ -70,6 +70,10 @@ OPENSSL_PUT_ERROR(EVP, ERR_R_PASSED_NULL_PARAMETER); return nullptr; } + if (impl->pkey == nullptr) { + OPENSSL_PUT_ERROR(EVP, EVP_R_NO_KEY_SET); + return nullptr; + } const EVP_PKEY_CTX_METHOD *pkey_method = impl->ameth->pkey_method; if (pkey_method == nullptr) { @@ -314,7 +318,7 @@ return 1; } - if (!impl->pkey) { + if (!impl->pkey || !FromOpaque(peer)->pkey) { OPENSSL_PUT_ERROR(EVP, EVP_R_NO_KEY_SET); return 0; }
diff --git a/crypto/evp/evp_extra_test.cc b/crypto/evp/evp_extra_test.cc index 8f57f84..c2bb028 100644 --- a/crypto/evp/evp_extra_test.cc +++ b/crypto/evp/evp_extra_test.cc
@@ -1304,6 +1304,54 @@ EXPECT_EQ(EVP_PKEY_id(pkey.get()), EVP_PKEY_NONE); } +// Due to an OpenSSL API flaw, it is possible to make a half-empty X25519 key. +// Using a key in this state is a caller error, but we gracefully handle this +// case. +TEST(EVPExtraTest, HalfEmptyX25519) { + UniquePtr<EVP_PKEY> half_empty(EVP_PKEY_new()); + ASSERT_TRUE(half_empty); + ASSERT_TRUE(EVP_PKEY_set_type(half_empty.get(), EVP_PKEY_X25519)); + + // A half-empty key has nothing. + EXPECT_FALSE(EVP_PKEY_has_public(half_empty.get())); + EXPECT_FALSE(EVP_PKEY_has_private(half_empty.get())); + + // We cannot copy parameters from a half-empty key. + EXPECT_TRUE(EVP_PKEY_missing_parameters(half_empty.get())); + UniquePtr<EVP_PKEY> pkey(EVP_PKEY_new()); + ASSERT_TRUE(pkey); + EXPECT_FALSE(EVP_PKEY_copy_parameters(pkey.get(), half_empty.get())); + + // A half-empty key cannot be serialized. + ScopedCBB cbb; + ASSERT_TRUE(CBB_init(cbb.get(), 0)); + EXPECT_FALSE(EVP_marshal_public_key(cbb.get(), half_empty.get())); + EXPECT_FALSE(EVP_marshal_private_key(cbb.get(), half_empty.get())); + + // A half-empty key cannot be used. + UniquePtr<EVP_PKEY_CTX> ctx(EVP_PKEY_CTX_new(half_empty.get(), nullptr)); + EXPECT_FALSE(ctx); + + // Make a real key. + ctx.reset(EVP_PKEY_CTX_new_id(EVP_PKEY_X25519, nullptr)); + ASSERT_TRUE(ctx); + ASSERT_TRUE(EVP_PKEY_keygen_init(ctx.get())); + EVP_PKEY *real_key_raw = nullptr; + ASSERT_TRUE(EVP_PKEY_keygen(ctx.get(), &real_key_raw)); + UniquePtr<EVP_PKEY> real_key(real_key_raw); + + // A half-empty key cannot be compared. + EXPECT_FALSE(EVP_PKEY_cmp(half_empty.get(), half_empty.get())); + EXPECT_FALSE(EVP_PKEY_cmp(half_empty.get(), real_key.get())); + EXPECT_FALSE(EVP_PKEY_cmp(real_key.get(), half_empty.get())); + + // A half-empty cannot be the peer in a Diffie-Hellman operation. + ctx.reset(EVP_PKEY_CTX_new(real_key.get(), nullptr)); + ASSERT_TRUE(ctx); + ASSERT_TRUE(EVP_PKEY_derive_init(ctx.get())); + EXPECT_FALSE(EVP_PKEY_derive_set_peer(ctx.get(), half_empty.get())); +} + // Test that parsers correctly handle trailing data. TEST(EVPExtraTest, TrailingData) { UniquePtr<EVP_PKEY> pkey = LoadExampleRSAKey();