Add ephemeral key allocator to FabricTable (#20082)

* Add ephemeral key allocator to FabricTable

- CASE requires P256 ephemeral keys
- CASE had a hack whereby "single slots" ephemeral keys
  for NXP HSM were used, which cannot work for multiple CASE
  session establishments
- Using raw P256Keypair prevents being able to use OS-backed
  or HW-backed keys, like can be done for operational keys

Issue #20036

This PR:

- Adds a way to get CASE ephemeral keys from the OperationalKeystore
  abstraction
- Funnels their access via the FabricTable
- Removes some HSM hacks (cannot remove all HSM usage just yet)
  in a way that now OperationalKeystore targeting NXP HSM could
  do the right thing

Testing done:

- Unit tests still pass
- Integration tests still passa
- Added unit tests to validate usage of new APIs
- Ran valgrind on the unit tests, found no leaks

* Restyled by clang-format

* Apply review comments

* Apply review comment from @bzbarsky-apple

* Better document usage

* Remove a test member that was added during debug of a prior CI run

Co-authored-by: Restyled.io <[email protected]>

src/app/clusters/operational-credentials-server/operational-credentials-server.cpp

@@ -50,9 +50,6 @@
 #include <platform/CHIPDeviceLayer.h>
 #include <string.h>
 #include <trace/trace.h>
-#if CHIP_CRYPTO_HSM
-#include <crypto/hsm/CHIPCryptoPALHsm.h>
-#endif
 
 using namespace chip;
 using namespace ::chip::Transport;

src/credentials/FabricTable.cpp

@@ -1334,6 +1334,28 @@ CHIP_ERROR FabricTable::ReadFabricInfo(TLV::ContiguousBufferTLVReader & reader)
     return CHIP_NO_ERROR;
 }
 
+Crypto::P256Keypair * FabricTable::AllocateEphemeralKeypairForCASE()
+{
+    if (mOperationalKeystore != nullptr)
+    {
+        return mOperationalKeystore->AllocateEphemeralKeypairForCASE();
+    }
+
+    return Platform::New<Crypto::P256Keypair>();
+}
+
+void FabricTable::ReleaseEphemeralKeypair(Crypto::P256Keypair * keypair)
+{
+    if (mOperationalKeystore != nullptr)
+    {
+        mOperationalKeystore->ReleaseEphemeralKeypair(keypair);
+    }
+    else
+    {
+        Platform::Delete<Crypto::P256Keypair>(keypair);
+    }
+}
+
 CHIP_ERROR FabricTable::StoreCommitMarker(const CommitMarker & commitMarker)
 {
     DefaultStorageKeyAllocator keyAlloc;

src/credentials/FabricTable.h

@@ -582,6 +582,26 @@ class DLL_EXPORT FabricTable
      */
     CHIP_ERROR SignWithOpKeypair(FabricIndex fabricIndex, ByteSpan message, Crypto::P256ECDSASignature & outSignature) const;
 
+    /**
+     * @brief Create an ephemeral keypair for use in session establishment.
+     *
+     * WARNING: The return value MUST be released by `ReleaseEphemeralKeypair`. This is because
+     *          Matter CHIPMem.h does not properly support UniquePtr in a way that would
+     *          safely allow classes derived from Crypto::P256Keypair to be released properly.
+     *
+     * This delegates to the OperationalKeystore if one exists, otherwise it directly allocates a base
+     * Crypto::P256Keypair instance
+     *
+     * @return a pointer to a dynamically P256Keypair (or derived class thereof), which may evaluate to nullptr
+     *         if running out of memory.
+     */
+    Crypto::P256Keypair * AllocateEphemeralKeypairForCASE();
+
+    /**
+     * @brief Release an ephemeral keypair previously created by `AllocateEphemeralKeypairForCASE()`
+     */
+    void ReleaseEphemeralKeypair(Crypto::P256Keypair * keypair);
+
     /**
      * This initializes a new keypair for the given fabric and generates a CSR for it,
      * so that it can be passed in a CSRResponse.

src/credentials/tests/TestFabricTable.cpp

@@ -1742,6 +1742,61 @@ void TestInvalidChaining(nlTestSuite * inSuite, void * inContext)
     // TODO: Write test
 }
 
+void TestEphemeralKeys(nlTestSuite * inSuite, void * inContext)
+{
+    // Initialize a fabric table with operational keystore
+    {
+        chip::TestPersistentStorageDelegate storage;
+
+        // Initialize a FabricTable
+        ScopedFabricTable fabricTableHolder;
+        NL_TEST_ASSERT(inSuite, fabricTableHolder.Init(&storage) == CHIP_NO_ERROR);
+        FabricTable & fabricTable = fabricTableHolder.GetFabricTable();
+
+        Crypto::P256ECDSASignature sig;
+        uint8_t message[] = { 'm', 's', 'g' };
+
+        Crypto::P256Keypair * ephemeralKeypair = fabricTable.AllocateEphemeralKeypairForCASE();
+        NL_TEST_ASSERT(inSuite, ephemeralKeypair != nullptr);
+        NL_TEST_ASSERT_SUCCESS(inSuite, ephemeralKeypair->Initialize());
+
+        NL_TEST_ASSERT_SUCCESS(inSuite, ephemeralKeypair->ECDSA_sign_msg(message, sizeof(message), sig));
+        NL_TEST_ASSERT_SUCCESS(inSuite, ephemeralKeypair->Pubkey().ECDSA_validate_msg_signature(message, sizeof(message), sig));
+
+        fabricTable.ReleaseEphemeralKeypair(ephemeralKeypair);
+    }
+
+    // Use a fabric table without an operational keystore: should still work
+    {
+        chip::TestPersistentStorageDelegate storage;
+
+        chip::Credentials::PersistentStorageOpCertStore opCertStore;
+        NL_TEST_ASSERT_SUCCESS(inSuite, opCertStore.Init(&storage));
+
+        FabricTable fabricTable;
+        FabricTable::InitParams initParams;
+        initParams.storage     = &storage;
+        initParams.opCertStore = &opCertStore;
+
+        NL_TEST_ASSERT_SUCCESS(inSuite, fabricTable.Init(initParams));
+
+        Crypto::P256ECDSASignature sig;
+        uint8_t message[] = { 'm', 's', 'g' };
+
+        Crypto::P256Keypair * ephemeralKeypair = fabricTable.AllocateEphemeralKeypairForCASE();
+        NL_TEST_ASSERT(inSuite, ephemeralKeypair != nullptr);
+        NL_TEST_ASSERT_SUCCESS(inSuite, ephemeralKeypair->Initialize());
+
+        NL_TEST_ASSERT_SUCCESS(inSuite, ephemeralKeypair->ECDSA_sign_msg(message, sizeof(message), sig));
+        NL_TEST_ASSERT_SUCCESS(inSuite, ephemeralKeypair->Pubkey().ECDSA_validate_msg_signature(message, sizeof(message), sig));
+
+        fabricTable.ReleaseEphemeralKeypair(ephemeralKeypair);
+
+        fabricTable.Shutdown();
+        opCertStore.Finish();
+    }
+}
+
 void TestCommitMarker(nlTestSuite * inSuite, void * inContext)
 {
     Crypto::P256PublicKey fIdx1PublicKey;
@@ -1765,7 +1820,7 @@ void TestCommitMarker(nlTestSuite * inSuite, void * inContext)
     //   - FabricID 2222, Node ID 66
     //      - Abort commit on second fabric
     {
-        // Initialize a FabricTable
+        // Initialize a fabric table
         ScopedFabricTable fabricTableHolder;
         NL_TEST_ASSERT(inSuite, fabricTableHolder.Init(&storage) == CHIP_NO_ERROR);
         FabricTable & fabricTable = fabricTableHolder.GetFabricTable();
@@ -1976,6 +2031,7 @@ static const nlTest sTests[] =
     NL_TEST_DEF("Test compressed fabric ID is properly generated", TestCompressedFabricId),
     NL_TEST_DEF("Test AddNOC root collision", TestAddNocRootCollision),
     NL_TEST_DEF("Test invalid chaining in AddNOC and UpdateNOC", TestInvalidChaining),
+    NL_TEST_DEF("Test ephemeral keys allocation", TestEphemeralKeys),
     NL_TEST_DEF("Test proper detection of Commit Marker on init", TestCommitMarker),
 
     NL_TEST_SENTINEL()

src/crypto/OperationalKeystore.h

@@ -168,6 +168,32 @@ class OperationalKeystore
      */
     virtual CHIP_ERROR SignWithOpKeypair(FabricIndex fabricIndex, const ByteSpan & message,
                                          Crypto::P256ECDSASignature & outSignature) const = 0;
+
+    /**
+     * @brief Create an ephemeral keypair for use in session establishment.
+     *
+     * The caller must Initialize() the P256Keypair if needed. It is not done by this method.
+     *
+     * This method MUST ONLY be used for CASESession ephemeral keys.
+     *
+     * NOTE: The stack will allocate as many of these as there are CASE sessions which
+     *       can be concurrently in the process of establishment. Implementations must
+     *       support more than one such keypair, or be implemented to match the limitations
+     *       enforced by a given product on its concurrent CASE session establishment limits.
+     *
+     * WARNING: The return value MUST be released by `ReleaseEphemeralKeypair`. This is because
+     *          Matter CHIPMem.h does not properly support UniquePtr in a way that would
+     *          safely allow classes derived from Crypto::P256Keypair to be released properly.
+     *
+     * @return a pointer to a P256Keypair (or derived class thereof), which may evaluate to nullptr
+     *         if running out of memory.
+     */
+    virtual Crypto::P256Keypair * AllocateEphemeralKeypairForCASE() = 0;
+
+    /**
+     * @brief Release an ephemeral keypair previously provided by `AllocateEphemeralKeypairForCASE()`
+     */
+    virtual void ReleaseEphemeralKeypair(Crypto::P256Keypair * keypair) = 0;
 };
 
 } // namespace Crypto

src/crypto/PersistentStorageOperationalKeystore.cpp

@@ -296,4 +296,19 @@ CHIP_ERROR PersistentStorageOperationalKeystore::SignWithOpKeypair(FabricIndex f
     return SignWithStoredOpKey(fabricIndex, mStorage, message, outSignature);
 }
 
+Crypto::P256Keypair * PersistentStorageOperationalKeystore::AllocateEphemeralKeypairForCASE()
+{
+    // DO NOT CUT AND PASTE without considering the ReleaseEphemeralKeypair().
+    // If allocating a derived class, then `ReleaseEphemeralKeypair` MUST
+    // de-allocate the derived class after up-casting the base class pointer.
+    return Platform::New<Crypto::P256Keypair>();
+}
+
+void PersistentStorageOperationalKeystore::ReleaseEphemeralKeypair(Crypto::P256Keypair * keypair)
+{
+    // DO NOT CUT AND PASTE without considering the AllocateEphemeralKeypairForCASE().
+    // This must delete the same concrete class as allocated in `AllocateEphemeralKeypairForCASE`
+    Platform::Delete<Crypto::P256Keypair>(keypair);
+}
+
 } // namespace chip

src/crypto/PersistentStorageOperationalKeystore.h

@@ -85,6 +85,8 @@ class PersistentStorageOperationalKeystore : public Crypto::OperationalKeystore
     void RevertPendingKeypair() override;
     CHIP_ERROR SignWithOpKeypair(FabricIndex fabricIndex, const ByteSpan & message,
                                  Crypto::P256ECDSASignature & outSignature) const override;
+    Crypto::P256Keypair * AllocateEphemeralKeypairForCASE() override;
+    void ReleaseEphemeralKeypair(Crypto::P256Keypair * keypair) override;
 
 protected:
     void ResetPendingKey()

src/crypto/tests/TestPersistentStorageOpKeyStore.cpp

@@ -24,6 +24,7 @@
 #include <lib/support/DefaultStorageKeyAllocator.h>
 #include <lib/support/Span.h>
 #include <lib/support/TestPersistentStorageDelegate.h>
+#include <lib/support/UnitTestExtendedAssertions.h>
 #include <lib/support/UnitTestRegistration.h>
 #include <nlunit-test.h>
 
@@ -194,12 +195,33 @@ void TestBasicLifeCycle(nlTestSuite * inSuite, void * inContext)
     opKeystore.Finish();
 }
 
+void TestEphemeralKeys(nlTestSuite * inSuite, void * inContext)
+{
+    chip::TestPersistentStorageDelegate storage;
+
+    PersistentStorageOperationalKeystore opKeyStore;
+    NL_TEST_ASSERT_SUCCESS(inSuite, opKeyStore.Init(&storage));
+
+    Crypto::P256ECDSASignature sig;
+    uint8_t message[] = { 'm', 's', 'g' };
+
+    Crypto::P256Keypair * ephemeralKeypair = opKeyStore.AllocateEphemeralKeypairForCASE();
+    NL_TEST_ASSERT(inSuite, ephemeralKeypair != nullptr);
+    NL_TEST_ASSERT_SUCCESS(inSuite, ephemeralKeypair->Initialize());
+
+    NL_TEST_ASSERT_SUCCESS(inSuite, ephemeralKeypair->ECDSA_sign_msg(message, sizeof(message), sig));
+    NL_TEST_ASSERT_SUCCESS(inSuite, ephemeralKeypair->Pubkey().ECDSA_validate_msg_signature(message, sizeof(message), sig));
+
+    opKeyStore.ReleaseEphemeralKeypair(ephemeralKeypair);
+
+    opKeyStore.Finish();
+}
+
 /**
  *   Test Suite. It lists all the test functions.
  */
 static const nlTest sTests[] = { NL_TEST_DEF("Test Basic Lifecycle of PersistentStorageOperationalKeystore", TestBasicLifeCycle),
-
-                                 NL_TEST_SENTINEL() };
+                                 NL_TEST_DEF("Test ephemeral key management", TestEphemeralKeys), NL_TEST_SENTINEL() };
 
 /**
  *  Set up the test suite.

src/lib/support/CHIPMem.h

@@ -160,6 +160,11 @@ inline T * New(Args &&... args)
 template <typename T>
 inline void Delete(T * p)
 {
+    if (p == nullptr)
+    {
+        return;
+    }
+
     p->~T();
     MemoryFree(p);
 }