Refactor ICD Monitoring Table to not assume key type

Add Hmac Key Handle to ICD Monitoring Table

src/app/icd/ICDCheckInSender.cpp

@@ -61,7 +61,7 @@ CHIP_ERROR ICDCheckInSender::SendCheckInMsg(const Transport::PeerAddress & addr)
     VerifyOrReturnError(!buffer.IsNull(), CHIP_ERROR_NO_MEMORY);
     MutableByteSpan output{ buffer->Start(), buffer->MaxDataLength() };
 
-    ReturnErrorOnFailure(CheckinMessage::GenerateCheckinMessagePayload(mKeyHandle, mICDCounter, ByteSpan(), output));
+    ReturnErrorOnFailure(CheckinMessage::GenerateCheckinMessagePayload(mAesKeyHandle, mICDCounter, ByteSpan(), output));
     buffer->SetDataLength(static_cast<uint16_t>(output.size()));
 
     VerifyOrReturnError(mExchangeManager->GetSessionManager() != nullptr, CHIP_ERROR_INTERNAL);
@@ -89,8 +89,7 @@ CHIP_ERROR ICDCheckInSender::RequestResolve(ICDMonitoringEntry & entry, FabricTa
 
     AddressResolve::NodeLookupRequest request(peerId);
 
-    memcpy(mKeyHandle.AsMutable<Crypto::Symmetric128BitsKeyByteArray>(), entry.keyHandle.As<Crypto::Symmetric128BitsKeyByteArray>(),
-           sizeof(Crypto::Symmetric128BitsKeyByteArray));
+    Crypto::CopySymmetric128BitsKeyHandle(entry.aesKeyHandle, mAesKeyHandle);
 
     CHIP_ERROR err = AddressResolve::Resolver::Instance().LookupNode(request, mAddressLookupHandle);
 

src/app/icd/ICDCheckInSender.h

@@ -50,7 +50,7 @@ class ICDCheckInSender : public AddressResolve::NodeListener
 
     Messaging::ExchangeManager * mExchangeManager = nullptr;
 
-    Crypto::Aes128KeyHandle mKeyHandle = Crypto::Aes128KeyHandle();
+    Crypto::Aes128KeyHandle mAesKeyHandle = Crypto::Aes128KeyHandle();
 
     uint32_t mICDCounter = 0;
 };

src/app/icd/ICDMonitoringTable.cpp

@@ -25,7 +25,8 @@ enum class Fields : uint8_t
 {
     kCheckInNodeID    = 1,
     kMonitoredSubject = 2,
-    kKey              = 3,
+    kAesKeyHandle     = 3,
+    kHmacKeyHandle    = 4,
 };
 
 CHIP_ERROR ICDMonitoringEntry::UpdateKey(StorageKeyName & skey)
@@ -42,8 +43,14 @@ CHIP_ERROR ICDMonitoringEntry::Serialize(TLV::TLVWriter & writer) const
     ReturnErrorOnFailure(writer.Put(TLV::ContextTag(Fields::kCheckInNodeID), checkInNodeID));
     ReturnErrorOnFailure(writer.Put(TLV::ContextTag(Fields::kMonitoredSubject), monitoredSubject));
 
-    ByteSpan buf(keyHandle.As<Crypto::Symmetric128BitsKeyByteArray>());
-    ReturnErrorOnFailure(writer.Put(TLV::ContextTag(Fields::kKey), buf));
+    ByteSpan aesKeybuf;
+    Crypto::GetByteSpanFromSymmetric128BitsKeyHandle(aesKeyHandle, aesKeybuf);
+    ReturnErrorOnFailure(writer.Put(TLV::ContextTag(Fields::kAesKeyHandle), aesKeybuf));
+
+    ByteSpan hmacKeybuf;
+    Crypto::GetByteSpanFromSymmetric128BitsKeyHandle(hmacKeyHandle, hmacKeybuf);
+    ReturnErrorOnFailure(writer.Put(TLV::ContextTag(Fields::kHmacKeyHandle), hmacKeybuf));
+
     ReturnErrorOnFailure(writer.EndContainer(outer));
     return CHIP_NO_ERROR;
 }
@@ -69,18 +76,43 @@ CHIP_ERROR ICDMonitoringEntry::Deserialize(TLV::TLVReader & reader)
             case to_underlying(Fields::kMonitoredSubject):
                 ReturnErrorOnFailure(reader.Get(monitoredSubject));
                 break;
-            case to_underlying(Fields::kKey): {
-                ByteSpan buf(keyHandle.AsMutable<Crypto::Symmetric128BitsKeyByteArray>());
+            case to_underlying(Fields::kAesKeyHandle): {
+                ByteSpan buf;
                 ReturnErrorOnFailure(reader.Get(buf));
                 // Since we are storing either the raw key or a key ID, we must
                 // simply copy the data as is in the keyHandle.
                 // Calling SetKey here would create another keyHandle in storage and will cause
                 // keyHandle leakage in some implementations.
-                memcpy(keyHandle.AsMutable<Crypto::Symmetric128BitsKeyByteArray>(), buf.data(),
-                       sizeof(Crypto::Symmetric128BitsKeyByteArray));
+                ReturnErrorOnFailure(Crypto::GetSymmetric128BitsKeyHandleFromByteSpan(aesKeyHandle, buf));
                 keyHandleValid = true;
             }
             break;
+            case to_underlying(Fields::kHmacKeyHandle): {
+                ByteSpan buf;
+                CHIP_ERROR error = reader.Get(buf);
+
+                if (error != CHIP_NO_ERROR)
+                {
+                    // If retreiving the hmac key handle failed, we need to set an invalid key handle
+                    // even if the AesKeyHandle is valid.
+                    keyHandleValid = false;
+                    return error;
+                }
+
+                // Since we are storing either the raw key or a key ID, we must
+                // simply copy the data as is in the keyHandle.
+                // Calling SetKey here would create another keyHandle in storage and will cause
+                // keyHandle leakage in some implementations.
+                error = Crypto::GetSymmetric128BitsKeyHandleFromByteSpan(hmacKeyHandle, buf);
+                if (error != CHIP_NO_ERROR)
+                {
+                    // If setting the KeyHandle from the buffer failed, we need to set an invalid key handle
+                    // even if the AesKeyHandle is valid.
+                    keyHandleValid = false;
+                    return error;
+                }
+            }
+            break;
             default:
                 break;
             }
@@ -108,16 +140,29 @@ CHIP_ERROR ICDMonitoringEntry::SetKey(ByteSpan keyData)
     Crypto::Symmetric128BitsKeyByteArray keyMaterial;
     memcpy(keyMaterial, keyData.data(), sizeof(Crypto::Symmetric128BitsKeyByteArray));
 
-    ReturnErrorOnFailure(symmetricKeystore->CreateKey(keyMaterial, keyHandle));
-    keyHandleValid = true;
+    // TODO - Add function to call PSA key lifetime
+    ReturnErrorOnFailure(symmetricKeystore->CreateKey(keyMaterial, aesKeyHandle));
+    CHIP_ERROR error = symmetricKeystore->CreateKey(keyMaterial, hmacKeyHandle);
 
-    return CHIP_NO_ERROR;
+    if (error == CHIP_NO_ERROR)
+    {
+        // If the creation of the HmacKeyHandle succeeds, both key handles are valid
+        keyHandleValid = true;
+    }
+    else
+    {
+        // Creation of the HmacKeyHandle failed, we need to delete the AesKeyHandle to avoid a key leakage
+        symmetricKeystore->DestroyKey(this->aesKeyHandle);
+    }
+
+    return error;
 }
 
 CHIP_ERROR ICDMonitoringEntry::DeleteKey()
 {
     VerifyOrReturnError(symmetricKeystore != nullptr, CHIP_ERROR_INTERNAL);
-    symmetricKeystore->DestroyKey(this->keyHandle);
+    symmetricKeystore->DestroyKey(this->aesKeyHandle);
+    symmetricKeystore->DestroyKey(this->hmacKeyHandle);
     keyHandleValid = false;
     return CHIP_NO_ERROR;
 }
@@ -141,15 +186,15 @@ bool ICDMonitoringEntry::IsKeyEquivalent(ByteSpan keyData)
     uint64_t data = Crypto::GetRandU64(), validation, encrypted;
     validation    = data;
 
-    err = Crypto::AES_CCM_encrypt(reinterpret_cast<uint8_t *>(&data), sizeof(data), nullptr, 0, tempEntry.keyHandle, aead,
+    err = Crypto::AES_CCM_encrypt(reinterpret_cast<uint8_t *>(&data), sizeof(data), nullptr, 0, tempEntry.aesKeyHandle, aead,
                                   Crypto::CHIP_CRYPTO_AEAD_NONCE_LENGTH_BYTES, reinterpret_cast<uint8_t *>(&encrypted), mic,
                                   Crypto::CHIP_CRYPTO_AEAD_MIC_LENGTH_BYTES);
 
     data = 0;
     if (err == CHIP_NO_ERROR)
     {
         err = Crypto::AES_CCM_decrypt(reinterpret_cast<uint8_t *>(&encrypted), sizeof(encrypted), nullptr, 0, mic,
-                                      Crypto::CHIP_CRYPTO_AEAD_MIC_LENGTH_BYTES, keyHandle, aead,
+                                      Crypto::CHIP_CRYPTO_AEAD_MIC_LENGTH_BYTES, aesKeyHandle, aead,
                                       Crypto::CHIP_CRYPTO_AEAD_NONCE_LENGTH_BYTES, reinterpret_cast<uint8_t *>(&data));
     }
     tempEntry.DeleteKey();
@@ -175,8 +220,8 @@ ICDMonitoringEntry & ICDMonitoringEntry::operator=(const ICDMonitoringEntry & ic
     index             = icdMonitoringEntry.index;
     keyHandleValid    = icdMonitoringEntry.keyHandleValid;
     symmetricKeystore = icdMonitoringEntry.symmetricKeystore;
-    memcpy(keyHandle.AsMutable<Crypto::Symmetric128BitsKeyByteArray>(), icdMonitoringEntry.keyHandle.As<Crypto::Symmetric128BitsKeyByteArray>(),
-           sizeof(Crypto::Symmetric128BitsKeyByteArray));
+    Crypto::CopySymmetric128BitsKeyHandle(icdMonitoringEntry.aesKeyHandle, aesKeyHandle);
+    Crypto::CopySymmetric128BitsKeyHandle(icdMonitoringEntry.hmacKeyHandle, hmacKeyHandle);
 
     return *this;
 }
@@ -211,12 +256,14 @@ CHIP_ERROR ICDMonitoringTable::Set(uint16_t index, const ICDMonitoringEntry & en
     VerifyOrReturnError(kUndefinedNodeId != entry.checkInNodeID, CHIP_ERROR_INVALID_ARGUMENT);
     VerifyOrReturnError(kUndefinedNodeId != entry.monitoredSubject, CHIP_ERROR_INVALID_ARGUMENT);
     VerifyOrReturnError(entry.keyHandleValid, CHIP_ERROR_INVALID_ARGUMENT);
+
     ICDMonitoringEntry e(this->mFabric, index);
     e.checkInNodeID    = entry.checkInNodeID;
     e.monitoredSubject = entry.monitoredSubject;
     e.index            = index;
-    memcpy(e.keyHandle.AsMutable<Crypto::Symmetric128BitsKeyByteArray>(), entry.keyHandle.As<Crypto::Symmetric128BitsKeyByteArray>(),
-           sizeof(Crypto::Symmetric128BitsKeyByteArray));
+
+    Crypto::CopySymmetric128BitsKeyHandle(entry.aesKeyHandle, e.aesKeyHandle);
+    Crypto::CopySymmetric128BitsKeyHandle(entry.hmacKeyHandle, e.hmacKeyHandle);
 
     return e.Save(this->mStorage);
 }

src/app/icd/ICDMonitoringTable.h

@@ -33,7 +33,7 @@ using SymmetricKeystore = SessionKeystore;
 
 namespace chip {
 
-inline constexpr size_t kICDMonitoringBufferSize = 40;
+inline constexpr size_t kICDMonitoringBufferSize = 60;
 
 struct ICDMonitoringEntry : public PersistentData<kICDMonitoringBufferSize>
 {
@@ -104,7 +104,8 @@ struct ICDMonitoringEntry : public PersistentData<kICDMonitoringBufferSize>
     chip::FabricIndex fabricIndex                 = kUndefinedFabricIndex;
     chip::NodeId checkInNodeID                    = kUndefinedNodeId;
     uint64_t monitoredSubject                     = static_cast<uint64_t>(0);
-    Crypto::Aes128KeyHandle keyHandle             = Crypto::Aes128KeyHandle();
+    Crypto::Aes128KeyHandle aesKeyHandle          = Crypto::Aes128KeyHandle();
+    Crypto::Hmac128KeyHandle hmacKeyHandle        = Crypto::Hmac128KeyHandle();
     bool keyHandleValid                           = false;
     uint16_t index                                = 0;
     Crypto::SymmetricKeystore * symmetricKeystore = nullptr;

src/app/tests/TestICDMonitoringTable.cpp

@@ -155,6 +155,7 @@ void TestSaveAndLoadRegistrationValue(nlTestSuite * aSuite, void * aContext)
 
     // Retrieve first entry
     err = loading.Get(0, entry);
+
     NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == err);
     NL_TEST_ASSERT(aSuite, kTestFabricIndex1 == entry.fabricIndex);
     NL_TEST_ASSERT(aSuite, kClientNodeId11 == entry.checkInNodeID);
@@ -176,6 +177,7 @@ void TestSaveAndLoadRegistrationValue(nlTestSuite * aSuite, void * aContext)
 
     // Remove first entry
     saving.Remove(0);
+
     ICDMonitoringEntry entry4(&keystore);
     entry4.checkInNodeID    = kClientNodeId13;
     entry4.monitoredSubject = kClientNodeId11;

src/crypto/CHIPCryptoPAL.h

@@ -1688,6 +1688,12 @@ CHIP_ERROR ExtractSubjectFromX509Cert(const ByteSpan & certificate, MutableByteS
  **/
 CHIP_ERROR ExtractIssuerFromX509Cert(const ByteSpan & certificate, MutableByteSpan & issuer);
 
+void GetByteSpanFromSymmetric128BitsKeyHandle(const Symmetric128BitsKeyHandle & keyHandle, ByteSpan & byteSpan);
+
+CHIP_ERROR GetSymmetric128BitsKeyHandleFromByteSpan(Symmetric128BitsKeyHandle & keyHandle, const ByteSpan & byteSpan);
+
+void CopySymmetric128BitsKeyHandle(const Symmetric128BitsKeyHandle & from, Symmetric128BitsKeyHandle & to);
+
 /**
  * @brief Checks for resigned version of the certificate in the list and returns it.
  *

src/crypto/CHIPCryptoPALOpenSSL.cpp

@@ -2343,5 +2343,24 @@ CHIP_ERROR ReplaceCertIfResignedCertFound(const ByteSpan & referenceCertificate,
     return err;
 }
 
+void GetByteSpanFromSymmetric128BitsKeyHandle(const Symmetric128BitsKeyHandle & keyHandle, ByteSpan & byteSpan)
+{
+    byteSpan = ByteSpan(keyHandle.As<Symmetric128BitsKeyByteArray>());
+}
+
+CHIP_ERROR GetSymmetric128BitsKeyHandleFromByteSpan(Symmetric128BitsKeyHandle & keyHandle, const ByteSpan & byteSpan)
+{
+    VerifyOrReturnError(byteSpan.size() == sizeof(Symmetric128BitsKeyByteArray), CHIP_ERROR_INTERNAL);
+    memcpy(keyHandle.AsMutable<Symmetric128BitsKeyByteArray>(), byteSpan.data(), sizeof(Symmetric128BitsKeyByteArray));
+
+    return CHIP_NO_ERROR;
+}
+
+void CopySymmetric128BitsKeyHandle(const Symmetric128BitsKeyHandle & from, Symmetric128BitsKeyHandle & to)
+{
+    memcpy(to.AsMutable<Symmetric128BitsKeyByteArray>(), from.As<Symmetric128BitsKeyByteArray>(),
+           sizeof(Symmetric128BitsKeyByteArray));
+}
+
 } // namespace Crypto
 } // namespace chip

src/crypto/CHIPCryptoPALPSA.cpp

@@ -1057,5 +1057,23 @@ CHIP_ERROR Spake2p_P256_SHA256_HKDF_HMAC::PointIsValid(void * R)
     return CHIP_NO_ERROR;
 }
 
+void GetByteSpanFromSymmetric128BitsKeyHandle(const Symmetric128BitsKeyHandle & keyHandle, ByteSpan & byteSpan)
+{
+    byteSpan = ByteSpan(keyHandle.As<psa_key_id_t>());
+}
+
+CHIP_ERROR GetSymmetric128BitsKeyHandleFromByteSpan(Symmetric128BitsKeyHandle & keyHandle, const ByteSpan & byteSpan)
+{
+    VerifyOrReturnError(byteSpan.size() == sizeof(psa_key_id_t), CHIP_ERROR_INTERNAL);
+    memcpy(keyHandle.AsMutable<psa_key_id_t>(), buf.data(), sizeof(psa_key_id_t));
+
+    return CHIP_NO_ERROR;
+}
+
+void CopySymmetric128BitsKeyHandle(const Symmetric128BitsKeyHandle & from, Symmetric128BitsKeyHandle & to)
+{
+    memcpy(to.AsMutable<psa_key_id_t>(), from.As<psa_key_id_t>(), sizeof(psa_key_id_t));
+}
+
 } // namespace Crypto
 } // namespace chip

src/crypto/CHIPCryptoPALmbedTLS.cpp

@@ -1142,5 +1142,24 @@ CHIP_ERROR Spake2p_P256_SHA256_HKDF_HMAC::PointIsValid(void * R)
     return CHIP_NO_ERROR;
 }
 
+void GetByteSpanFromSymmetric128BitsKeyHandle(const Symmetric128BitsKeyHandle & keyHandle, ByteSpan & byteSpan)
+{
+    byteSpan = ByteSpan(keyHandle.As<Symmetric128BitsKeyByteArray>());
+}
+
+CHIP_ERROR GetSymmetric128BitsKeyHandleFromByteSpan(Symmetric128BitsKeyHandle & keyHandle, const ByteSpan & byteSpan)
+{
+    VerifyOrReturnError(byteSpan.size() == sizeof(Symmetric128BitsKeyByteArray), CHIP_ERROR_INTERNAL);
+    memcpy(keyHandle.AsMutable<Symmetric128BitsKeyByteArray>(), byteSpan.data(), sizeof(Symmetric128BitsKeyByteArray));
+
+    return CHIP_NO_ERROR;
+}
+
+void CopySymmetric128BitsKeyHandle(const Symmetric128BitsKeyHandle & from, Symmetric128BitsKeyHandle & to)
+{
+    memcpy(to.AsMutable<Symmetric128BitsKeyByteArray>(), from.As<Symmetric128BitsKeyByteArray>(),
+           sizeof(Symmetric128BitsKeyByteArray));
+}
+
 } // namespace Crypto
 } // namespace chip

src/crypto/SessionKeystore.h

@@ -29,12 +29,20 @@ namespace Crypto {
  * The session keystore interface provides an abstraction that allows the application to store
  * session keys in a secure environment. It uses the concept of key handles that isolate the
  * application from the actual key material.
+ *
+ * @note Refactor has begun to refactor this API into two disctinct APIs : SymmetrycKeyStore & SessionKeyDerivation
+ *       Work has not been completed so the  SessionKeystore has APIs that shouldn't go together for the time being
+ *       The SessionKeystore APIs are split into two sections, one for each futur API.
  */
 class SessionKeystore
 {
 public:
     virtual ~SessionKeystore() {}
 
+    /****************************
+     * SymmetricKeyStore APIs
+     *****************************/
+
     /**
      * @brief Import raw key material and return a key handle for a key that be used to do AES 128 encryption.
      *
@@ -59,6 +67,18 @@ class SessionKeystore
      */
     virtual CHIP_ERROR CreateKey(const Symmetric128BitsKeyByteArray & keyMaterial, Hmac128KeyHandle & key) = 0;
 
+    /**
+     * @brief Destroy key.
+     *
+     * The method can take an uninitialized handle in which case it is a no-op.
+     * As a result of calling this method, the handle is put in the uninitialized state.
+     */
+    virtual void DestroyKey(Symmetric128BitsKeyHandle & key) = 0;
+
+    /****************************
+     * SessionKeyDerivation APIs
+     *****************************/
+
     /**
      * @brief Derive key from a shared secret.
      *
@@ -83,14 +103,6 @@ class SessionKeystore
     virtual CHIP_ERROR DeriveSessionKeys(const ByteSpan & secret, const ByteSpan & salt, const ByteSpan & info,
                                          Aes128KeyHandle & i2rKey, Aes128KeyHandle & r2iKey,
                                          AttestationChallenge & attestationChallenge) = 0;
-
-    /**
-     * @brief Destroy key.
-     *
-     * The method can take an uninitialized handle in which case it is a no-op.
-     * As a result of calling this method, the handle is put in the uninitialized state.
-     */
-    virtual void DestroyKey(Symmetric128BitsKeyHandle & key) = 0;
 };
 
 /**

src/platform/nxp/crypto/se05x/CHIPCryptoPALHost.cpp

@@ -1349,5 +1349,24 @@ CHIP_ERROR ExtractVIDPIDFromX509Cert(const ByteSpan & certificate, AttestationCe
     return error;
 }
 
+void GetByteSpanFromSymmetric128BitsKeyHandle(const Symmetric128BitsKeyHandle & keyHandle, ByteSpan & byteSpan)
+{
+    byteSpan = ByteSpan(keyHandle.As<Symmetric128BitsKeyByteArray>());
+}
+
+CHIP_ERROR GetSymmetric128BitsKeyHandleFromByteSpan(Symmetric128BitsKeyHandle & keyHandle, const ByteSpan & byteSpan)
+{
+    VerifyOrReturnError(byteSpan.size() == sizeof(Symmetric128BitsKeyByteArray), CHIP_ERROR_INTERNAL);
+    memcpy(keyHandle.AsMutable<Symmetric128BitsKeyByteArray>(), byteSpan.data(), sizeof(Symmetric128BitsKeyByteArray));
+
+    return CHIP_NO_ERROR;
+}
+
+void CopySymmetric128BitsKeyHandle(const Symmetric128BitsKeyHandle & from, Symmetric128BitsKeyHandle & to)
+{
+    memcpy(to.AsMutable<Symmetric128BitsKeyByteArray>(), from.As<Symmetric128BitsKeyByteArray>(),
+           sizeof(Symmetric128BitsKeyByteArray));
+}
+
 } // namespace Crypto
 } // namespace chip