From ee524fc22cc483ebf099484adf3b94c2aba374d1 Mon Sep 17 00:00:00 2001 From: Alami-Amine Date: Thu, 6 Aug 2026 18:54:15 +0200 Subject: [PATCH] [crypto] Hold OpenSSL objects in an owning handle in the OpenSSL P256 PAL _create_evp_key_from_binary_p256_key and P256Keypair::ECDH_derive_secret released their OpenSSL objects through trailing goto-exit blocks. Add ScopedOpenSSLObject to CHIPCryptoPALOpenSSL.h and use it in both, which removes the cleanup blocks and makes the helper assign its out-parameter once, on success. The helper's out-parameter becomes a reference to the handle. Assigning to a handle releases whatever it held, so the null-out-parameter precondition and its CHIP_ERROR_INVALID_ARGUMENT path are no longer needed. ECDH_derive_secret keeps its exit label because SSLErrorLog() runs on every path, but the block is now only that call and the return. Add TestECDH_RemotePublicKeyNotOnCurve, the suite's first negative ECDH case, covering the remote-key decode failure path. --- src/crypto/CHIPCryptoPALOpenSSL.h | 56 +++++++++++ src/crypto/P256KeyPairOpenSSL.cpp | 131 +++++++------------------ src/crypto/tests/TestChipCryptoPAL.cpp | 20 ++++ 3 files changed, 113 insertions(+), 94 deletions(-) diff --git a/src/crypto/CHIPCryptoPALOpenSSL.h b/src/crypto/CHIPCryptoPALOpenSSL.h index 7358569f370a..0586f0eaff64 100644 --- a/src/crypto/CHIPCryptoPALOpenSSL.h +++ b/src/crypto/CHIPCryptoPALOpenSSL.h @@ -23,8 +23,12 @@ #pragma once #include "CHIPCryptoPAL.h" +#include +#include #include +#include + namespace chip { namespace Crypto { @@ -34,6 +38,58 @@ using boringssl_size_t_openssl_int = size_t; using boringssl_size_t_openssl_int = int; #endif +/** + * @brief Deleter for an owned OpenSSL object. Free is a template parameter, not a member, so the + * deleter is empty and the handle stays pointer-sized. + **/ +template +struct OpenSSLDelete +{ + void operator()(T * obj) const { Free(obj); } +}; + +/** + * @brief Owning handle for a heap-allocated OpenSSL object. + * + * Free is part of the type rather than derived from T because several types have both a plain and a + * zeroizing variant (EC_POINT_free / EC_POINT_clear_free, BN_free / BN_clear_free) and the right one + * depends on whether the object held secret material. Declare an alias per (type, free) pair used. + * + * Only a `void (T *)` cleanup can be named. Macros such as sk_X509_free and two-argument forms such + * as sk__pop_free cannot; do not adapt them with a local wrapper, since one with internal + * linkage yields a distinct specialization per translation unit. + * + * The handle always frees what it holds - the base is private and release() is not re-exported, so + * ownership cannot be handed out. Only pass get() to functions that will not free the object + * themselves. EVP_PKEY_set1_EC_KEY is fine; EVP_PKEY_assign_EC_KEY and the *_set0_* family take + * ownership and would leave it freed twice. + **/ +template +class ScopedOpenSSLObject : private std::unique_ptr> +{ + using Base = std::unique_ptr>; + +public: + using Base::Base; + using Base::get; + using Base::reset; + using Base::operator bool; +}; + +using ScopedEcGroup = ScopedOpenSSLObject; +using ScopedEcKey = ScopedOpenSSLObject; +using ScopedEcPoint = ScopedOpenSSLObject; +using ScopedEvpPkey = ScopedOpenSSLObject; +using ScopedEvpPkeyCtx = ScopedOpenSSLObject; + +// A handle must stay pointer-sized: the empty deleter is expected to be folded away, and nothing may +// add state to the handle itself. +static_assert(sizeof(ScopedEcGroup) == sizeof(EC_GROUP *), "ScopedEcGroup must be pointer-sized"); +static_assert(sizeof(ScopedEcKey) == sizeof(EC_KEY *), "ScopedEcKey must be pointer-sized"); +static_assert(sizeof(ScopedEcPoint) == sizeof(EC_POINT *), "ScopedEcPoint must be pointer-sized"); +static_assert(sizeof(ScopedEvpPkey) == sizeof(EVP_PKEY *), "ScopedEvpPkey must be pointer-sized"); +static_assert(sizeof(ScopedEvpPkeyCtx) == sizeof(EVP_PKEY_CTX *), "ScopedEvpPkeyCtx must be pointer-sized"); + enum class ECName { None = 0, diff --git a/src/crypto/P256KeyPairOpenSSL.cpp b/src/crypto/P256KeyPairOpenSSL.cpp index 8d5aa35182e4..0e0ee093da96 100644 --- a/src/crypto/P256KeyPairOpenSSL.cpp +++ b/src/crypto/P256KeyPairOpenSSL.cpp @@ -25,6 +25,7 @@ #include #include +#include #if !CHIP_CRYPTO_BORINGSSL && defined(OPENSSL_VERSION_NUMBER) && OPENSSL_VERSION_NUMBER >= 0x30200000L #include @@ -63,70 +64,36 @@ static inline const EC_KEY * to_const_EC_KEY(const P256KeypairContext * context) return *SafePointerCast(context); } -// helper function to populate octet key into EVP_PKEY out_evp_pkey. Caller must free out_evp_pkey -static CHIP_ERROR _create_evp_key_from_binary_p256_key(const P256PublicKey & key, EVP_PKEY ** out_evp_pkey) +// helper function to populate an octet key into out_evp_pkey. Anything out_evp_pkey already held is +// released; on failure it is left empty. +static CHIP_ERROR _create_evp_key_from_binary_p256_key(const P256PublicKey & key, ScopedEvpPkey & out_evp_pkey) { + out_evp_pkey.reset(); - CHIP_ERROR error = CHIP_NO_ERROR; - EC_KEY * ec_key = nullptr; - int result = -1; - EC_POINT * point = nullptr; - EC_GROUP * group = nullptr; - int nid = NID_undef; - - VerifyOrExit(*out_evp_pkey == nullptr, error = CHIP_ERROR_INVALID_ARGUMENT); - - nid = GetNidForCurve(MapECName(key.Type())); - VerifyOrExit(nid != NID_undef, error = CHIP_ERROR_INTERNAL); - - ec_key = EC_KEY_new_by_curve_name(nid); - VerifyOrExit(ec_key != nullptr, error = CHIP_ERROR_INTERNAL); - - group = EC_GROUP_new_by_curve_name(nid); - VerifyOrExit(group != nullptr, error = CHIP_ERROR_INTERNAL); - - point = EC_POINT_new(group); - VerifyOrExit(point != nullptr, error = CHIP_ERROR_INTERNAL); + int nid = GetNidForCurve(MapECName(key.Type())); + VerifyOrReturnError(nid != NID_undef, CHIP_ERROR_INTERNAL); - result = EC_POINT_oct2point(group, point, Uint8::to_const_uchar(key), key.Length(), nullptr); - VerifyOrExit(result == 1, error = CHIP_ERROR_INTERNAL); - - result = EC_KEY_set_public_key(ec_key, point); + // EVP_PKEY_set1_EC_KEY takes its own reference to ec_key, so this one is dropped on the way out. + ScopedEcKey ec_key(EC_KEY_new_by_curve_name(nid)); + VerifyOrReturnError(ec_key, CHIP_ERROR_INTERNAL); - VerifyOrExit(result == 1, error = CHIP_ERROR_INTERNAL); + ScopedEcGroup group(EC_GROUP_new_by_curve_name(nid)); + VerifyOrReturnError(group, CHIP_ERROR_INTERNAL); - *out_evp_pkey = EVP_PKEY_new(); - VerifyOrExit(*out_evp_pkey != nullptr, error = CHIP_ERROR_INTERNAL); + ScopedEcPoint point(EC_POINT_new(group.get())); + VerifyOrReturnError(point, CHIP_ERROR_INTERNAL); - result = EVP_PKEY_set1_EC_KEY(*out_evp_pkey, ec_key); - VerifyOrExit(result == 1, error = CHIP_ERROR_INTERNAL); - -exit: - if (ec_key != nullptr) - { - EC_KEY_free(ec_key); - ec_key = nullptr; - } - - if (error != CHIP_NO_ERROR && *out_evp_pkey) - { - EVP_PKEY_free(*out_evp_pkey); - out_evp_pkey = nullptr; - } + VerifyOrReturnError(EC_POINT_oct2point(group.get(), point.get(), Uint8::to_const_uchar(key), key.Length(), nullptr) == 1, + CHIP_ERROR_INTERNAL); + VerifyOrReturnError(EC_KEY_set_public_key(ec_key.get(), point.get()) == 1, CHIP_ERROR_INTERNAL); - if (point != nullptr) - { - EC_POINT_free(point); - point = nullptr; - } + ScopedEvpPkey evp_pkey(EVP_PKEY_new()); + VerifyOrReturnError(evp_pkey, CHIP_ERROR_INTERNAL); - if (group != nullptr) - { - EC_GROUP_free(group); - group = nullptr; - } + VerifyOrReturnError(EVP_PKEY_set1_EC_KEY(evp_pkey.get(), ec_key.get()) == 1, CHIP_ERROR_INTERNAL); - return error; + out_evp_pkey = std::move(evp_pkey); + return CHIP_NO_ERROR; } // Encode an ECDSA_SIG (r, s) pair as a raw P256ECDSASignature (r || s, each zero-padded to kP256_FE_Length). @@ -464,65 +431,41 @@ CHIP_ERROR P256Keypair::ECDH_derive_secret(const P256PublicKey & remote_public_k ERR_clear_error(); CHIP_ERROR error = CHIP_NO_ERROR; int result = -1; - EVP_PKEY * local_key = nullptr; - EVP_PKEY * remote_key = nullptr; + size_t out_buf_length = 0; - EVP_PKEY_CTX * context = nullptr; - size_t out_buf_length = 0; + ScopedEvpPkey local_key; + ScopedEvpPkey remote_key; + ScopedEvpPkeyCtx context; - EC_KEY * ec_key = EC_KEY_dup(to_const_EC_KEY(&mKeypair)); - VerifyOrExit(ec_key != nullptr, error = CHIP_ERROR_INTERNAL); + ScopedEcKey ec_key(EC_KEY_dup(to_const_EC_KEY(&mKeypair))); + VerifyOrExit(ec_key, error = CHIP_ERROR_INTERNAL); VerifyOrExit(mInitialized, error = CHIP_ERROR_UNINITIALIZED); - local_key = EVP_PKEY_new(); - VerifyOrExit(local_key != nullptr, error = CHIP_ERROR_INTERNAL); + local_key.reset(EVP_PKEY_new()); + VerifyOrExit(local_key, error = CHIP_ERROR_INTERNAL); - result = EVP_PKEY_set1_EC_KEY(local_key, ec_key); + result = EVP_PKEY_set1_EC_KEY(local_key.get(), ec_key.get()); VerifyOrExit(result == 1, error = CHIP_ERROR_INTERNAL); - error = _create_evp_key_from_binary_p256_key(remote_public_key, &remote_key); + error = _create_evp_key_from_binary_p256_key(remote_public_key, remote_key); SuccessOrExit(error); - context = EVP_PKEY_CTX_new(local_key, nullptr); - VerifyOrExit(context != nullptr, error = CHIP_ERROR_INTERNAL); + context.reset(EVP_PKEY_CTX_new(local_key.get(), nullptr)); + VerifyOrExit(context, error = CHIP_ERROR_INTERNAL); - result = EVP_PKEY_derive_init(context); + result = EVP_PKEY_derive_init(context.get()); VerifyOrExit(result == 1, error = CHIP_ERROR_INTERNAL); - result = EVP_PKEY_derive_set_peer(context, remote_key); + result = EVP_PKEY_derive_set_peer(context.get(), remote_key.get()); VerifyOrExit(result == 1, error = CHIP_ERROR_INTERNAL); out_buf_length = (out_secret.Length() == 0) ? out_secret.Capacity() : out_secret.Length(); - result = EVP_PKEY_derive(context, out_secret.Bytes(), &out_buf_length); + result = EVP_PKEY_derive(context.get(), out_secret.Bytes(), &out_buf_length); VerifyOrExit(result == 1, error = CHIP_ERROR_INTERNAL); SuccessOrExit(error = out_secret.SetLength(out_buf_length)); exit: - if (ec_key != nullptr) - { - EC_KEY_free(ec_key); - ec_key = nullptr; - } - - if (local_key != nullptr) - { - EVP_PKEY_free(local_key); - local_key = nullptr; - } - - if (remote_key != nullptr) - { - EVP_PKEY_free(remote_key); - remote_key = nullptr; - } - - if (context != nullptr) - { - EVP_PKEY_CTX_free(context); - context = nullptr; - } - SSLErrorLog(); return error; } diff --git a/src/crypto/tests/TestChipCryptoPAL.cpp b/src/crypto/tests/TestChipCryptoPAL.cpp index b74df84f7486..7ba1102cdbbc 100644 --- a/src/crypto/tests/TestChipCryptoPAL.cpp +++ b/src/crypto/tests/TestChipCryptoPAL.cpp @@ -1542,6 +1542,26 @@ TEST_F(TestChipCryptoPAL, TestECDH_EstablishSecret) EXPECT_TRUE(signatures_match); } +TEST_F(TestChipCryptoPAL, TestECDH_RemotePublicKeyNotOnCurve) +{ + HeapChecker heapChecker; + Test_P256Keypair keypair; + ASSERT_SUCCESS(keypair.Initialize(ECPKeyTarget::ECDH)); + + // Correct length and SEC1 uncompressed-point marker, but (0, 0) is not on P-256, so decoding the + // remote key fails. Exercises the failure path of remote key decoding, which must release + // everything it allocated and publish no secret. + static constexpr uint8_t kPointNotOnCurve[kP256_PublicKey_Length] = { 0x04 }; + P256PublicKey invalid_remote_public_key(kPointNotOnCurve); + + P256ECDHDerivedSecret out_secret; + ASSERT_EQ(out_secret.Length(), 0u); + + CHIP_ERROR err = keypair.ECDH_derive_secret(invalid_remote_public_key, out_secret); + EXPECT_NE(err, CHIP_NO_ERROR); + EXPECT_EQ(out_secret.Length(), 0u); +} + #if CHIP_CRYPTO_OPENSSL TEST_F(TestChipCryptoPAL, TestAddEntropySources) {