[crypto] Hold OpenSSL objects in an owning handle in the OpenSSL P256 PAL + double-free fix

src/crypto/CHIPCryptoPALOpenSSL.h

@@ -23,8 +23,12 @@
 #pragma once
 
 #include "CHIPCryptoPAL.h"
+#include <openssl/ec.h>
+#include <openssl/evp.h>
 #include <openssl/x509.h>
 
+#include <memory>
+
 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 <typename T, void (*Free)(T *)>
+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_<TYPE>_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 <typename T, void (*Free)(T *)>
+class ScopedOpenSSLObject : private std::unique_ptr<T, OpenSSLDelete<T, Free>>
+{
+    using Base = std::unique_ptr<T, OpenSSLDelete<T, Free>>;
+
+public:
+    using Base::Base;
+    using Base::get;
+    using Base::reset;
+    using Base::operator bool;
+};
+
+using ScopedEcGroup    = ScopedOpenSSLObject<EC_GROUP, EC_GROUP_free>;
+using ScopedEcKey      = ScopedOpenSSLObject<EC_KEY, EC_KEY_free>;
+using ScopedEcPoint    = ScopedOpenSSLObject<EC_POINT, EC_POINT_free>;
+using ScopedEvpPkey    = ScopedOpenSSLObject<EVP_PKEY, EVP_PKEY_free>;
+using ScopedEvpPkeyCtx = ScopedOpenSSLObject<EVP_PKEY_CTX, EVP_PKEY_CTX_free>;
+
+// 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,

src/crypto/P256KeyPairOpenSSL.cpp

@@ -25,6 +25,7 @@
 
 #include <limits>
 #include <type_traits>
+#include <utility>
 
 #if !CHIP_CRYPTO_BORINGSSL && defined(OPENSSL_VERSION_NUMBER) && OPENSSL_VERSION_NUMBER >= 0x30200000L
 #include <openssl/core_names.h>
@@ -63,70 +64,36 @@ static inline const EC_KEY * to_const_EC_KEY(const P256KeypairContext * context)
     return *SafePointerCast<const EC_KEY * const *>(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;
 }

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)
 {