project-chip · mergify · Sep 16, 2024 · Aug 9, 2024 · Aug 9, 2024 · Aug 9, 2024
diff --git a/src/app/chip_data_model.cmake b/src/app/chip_data_model.cmake
@@ -142,9 +142,6 @@ function(chip_configure_data_model APP_TARGET)
         ${CHIP_APP_BASE_DIR}/util/attribute-storage.cpp
         ${CHIP_APP_BASE_DIR}/util/attribute-table.cpp
         ${CHIP_APP_BASE_DIR}/util/binding-table.cpp
-        ${CHIP_APP_BASE_DIR}/icd/server/ICDMonitoringTable.cpp
-        ${CHIP_APP_BASE_DIR}/icd/server/ICDNotifier.cpp
-        ${CHIP_APP_BASE_DIR}/icd/server/ICDConfigurationData.cpp
         ${CHIP_APP_BASE_DIR}/util/DataModelHandler.cpp
         ${CHIP_APP_BASE_DIR}/util/ember-compatibility-functions.cpp
         ${CHIP_APP_BASE_DIR}/util/ember-global-attribute-access-interface.cpp

diff --git a/src/app/icd/server/BUILD.gn b/src/app/icd/server/BUILD.gn
@@ -93,7 +93,6 @@ source_set("manager") {
   deps = [ ":icd-server-config" ]
 
   public_deps = [
-    ":check-in-back-off",
     ":configuration-data",
     ":notifier",
     ":observer",
@@ -106,6 +105,7 @@ source_set("manager") {
 
   if (chip_enable_icd_checkin) {
     public_deps += [
+      ":check-in-back-off",
       ":monitoring-table",
       ":sender",
       "${chip_root}/src/app:app_config",

diff --git a/src/app/icd/server/ICDManager.h b/src/app/icd/server/ICDManager.h
@@ -21,7 +21,6 @@
 #include <app/AppConfig.h>
 #include <app/SubscriptionsInfoProvider.h>
 #include <app/TestEventTriggerDelegate.h>
-#include <app/icd/server/ICDCheckInBackOffStrategy.h>
 #include <app/icd/server/ICDConfigurationData.h>
 #include <app/icd/server/ICDNotifier.h>
 #include <app/icd/server/ICDStateObserver.h>
@@ -34,9 +33,10 @@
 #include <system/SystemClock.h>
 
 #if CHIP_CONFIG_ENABLE_ICD_CIP
-#include <app/icd/server/ICDCheckInSender.h>   // nogncheck
-#include <app/icd/server/ICDMonitoringTable.h> // nogncheck
-#endif                                         // CHIP_CONFIG_ENABLE_ICD_CIP
+#include <app/icd/server/ICDCheckInBackOffStrategy.h> // nogncheck
+#include <app/icd/server/ICDCheckInSender.h>          // nogncheck
+#include <app/icd/server/ICDMonitoringTable.h>        // nogncheck
+#endif                                                // CHIP_CONFIG_ENABLE_ICD_CIP
 
 namespace chip {
 namespace Crypto {

diff --git a/src/app/icd/server/ICDMonitoringTable.cpp b/src/app/icd/server/ICDMonitoringTable.cpp
@@ -140,7 +140,6 @@ CHIP_ERROR ICDMonitoringEntry::SetKey(ByteSpan keyData)
     Crypto::Symmetric128BitsKeyByteArray keyMaterial;
     memcpy(keyMaterial, keyData.data(), sizeof(Crypto::Symmetric128BitsKeyByteArray));
 
-    // TODO - Add function to set PSA key lifetime
     ReturnErrorOnFailure(symmetricKeystore->CreateKey(keyMaterial, aesKeyHandle));
     CHIP_ERROR error = symmetricKeystore->CreateKey(keyMaterial, hmacKeyHandle);
 
@@ -242,15 +241,22 @@ CHIP_ERROR ICDMonitoringTable::Get(uint16_t index, ICDMonitoringEntry & entry) c
 
 CHIP_ERROR ICDMonitoringTable::Find(NodeId id, ICDMonitoringEntry & entry)
 {
-    uint16_t index = 0;
-    while (index < this->Limit())
+    uint16_t index;
+    ICDMonitoringEntry tempEntry(mSymmetricKeystore);
+
+    for (index = 0; index < this->Limit(); index++)
     {
-        ReturnErrorOnFailure(this->Get(index++, entry));
-        if (id == entry.checkInNodeID)
+        if (this->Get(index, tempEntry) != CHIP_NO_ERROR)
+        {
+            break;
+        }
+        if (id == tempEntry.checkInNodeID)
         {
+            entry = tempEntry;
             return CHIP_NO_ERROR;
         }
     }
+
     entry.index = index;
     return CHIP_ERROR_NOT_FOUND;
 }
@@ -263,16 +269,26 @@ CHIP_ERROR ICDMonitoringTable::Set(uint16_t index, const ICDMonitoringEntry & en
     VerifyOrReturnError(entry.keyHandleValid, CHIP_ERROR_INVALID_ARGUMENT);
 
     ICDMonitoringEntry e(this->mFabric, index);
-    e.checkInNodeID    = entry.checkInNodeID;
-    e.monitoredSubject = entry.monitoredSubject;
-    e.clientType       = entry.clientType;
-    e.index            = index;
+    e.checkInNodeID     = entry.checkInNodeID;
+    e.monitoredSubject  = entry.monitoredSubject;
+    e.clientType        = entry.clientType;
+    e.index             = index;
+    e.symmetricKeystore = entry.symmetricKeystore;
 
     memcpy(e.aesKeyHandle.AsMutable<Crypto::Symmetric128BitsKeyByteArray>(),
            entry.aesKeyHandle.As<Crypto::Symmetric128BitsKeyByteArray>(), sizeof(Crypto::Symmetric128BitsKeyByteArray));
     memcpy(e.hmacKeyHandle.AsMutable<Crypto::Symmetric128BitsKeyByteArray>(),
            entry.hmacKeyHandle.As<Crypto::Symmetric128BitsKeyByteArray>(), sizeof(Crypto::Symmetric128BitsKeyByteArray));
 
+    ReturnErrorOnFailure(e.symmetricKeystore->PersistICDKey(e.aesKeyHandle));
+    CHIP_ERROR error = e.symmetricKeystore->PersistICDKey(e.hmacKeyHandle);
+    if (error != CHIP_NO_ERROR)
+    {
+        // If setting the persistence of the HmacKeyHandle failed, we need to delete the AesKeyHandle to avoid a key leak
+        e.symmetricKeystore->DestroyKey(e.aesKeyHandle);
+        return error;
+    }
+
     return e.Save(this->mStorage);
 }
 

diff --git a/src/app/server/BUILD.gn b/src/app/server/BUILD.gn
@@ -54,7 +54,6 @@ static_library("server") {
   public_deps = [
     "${chip_root}/src/app",
     "${chip_root}/src/app:test-event-trigger",
-    "${chip_root}/src/app/icd/server:check-in-back-off",
     "${chip_root}/src/app/icd/server:icd-server-config",
     "${chip_root}/src/app/icd/server:observer",
     "${chip_root}/src/lib/address_resolve",

diff --git a/src/app/server/Server.h b/src/app/server/Server.h
@@ -72,14 +72,14 @@
 #include <app/reporting/ReportSchedulerImpl.h>
 #include <transport/raw/UDP.h>
 
-#include <app/icd/server/ICDCheckInBackOffStrategy.h>
 #if CHIP_CONFIG_ENABLE_ICD_SERVER
 #include <app/icd/server/ICDManager.h> // nogncheck
 
 #if CHIP_CONFIG_ENABLE_ICD_CIP
 #include <app/icd/server/DefaultICDCheckInBackOffStrategy.h> // nogncheck
-#endif
-#endif
+#include <app/icd/server/ICDCheckInBackOffStrategy.h>        // nogncheck
+#endif                                                       // CHIP_CONFIG_ENABLE_ICD_CIP
+#endif                                                       // CHIP_CONFIG_ENABLE_ICD_SERVER
 
 namespace chip {
 
@@ -183,9 +183,11 @@ struct ServerInitParams
     Credentials::OperationalCertificateStore * opCertStore = nullptr;
     // Required, if not provided, the Server::Init() WILL fail.
     app::reporting::ReportScheduler * reportScheduler = nullptr;
+#if CHIP_CONFIG_ENABLE_ICD_CIP
     // Optional. Support for the ICD Check-In BackOff strategy. Must be initialized before being provided.
     // If the ICD Check-In protocol use-case is supported and no strategy is provided, server will use the default strategy.
     app::ICDCheckInBackOffStrategy * icdCheckInBackOffStrategy = nullptr;
+#endif // CHIP_CONFIG_ENABLE_ICD_CIP
 };
 
 /**

diff --git a/src/crypto/BUILD.gn b/src/crypto/BUILD.gn
@@ -67,6 +67,7 @@ source_set("public_headers") {
 
   public_deps = [
     ":crypto_buildconfig",
+    "${chip_root}/src/app/icd/server:icd-server-config",
     "${chip_root}/src/lib/asn1",
     "${chip_root}/src/lib/core",
     "${chip_root}/src/lib/core:types",

diff --git a/src/crypto/CHIPCryptoPALPSA.cpp b/src/crypto/CHIPCryptoPALPSA.cpp
@@ -263,6 +263,28 @@ void Hash_SHA256_stream::Clear()
     psa_hash_abort(toHashOperation(&mContext));
 }
 
+CHIP_ERROR FindFreeKeySlotInRange(psa_key_id_t & keyId, psa_key_id_t start, uint32_t range)
+{
+    psa_key_attributes_t attributes = PSA_KEY_ATTRIBUTES_INIT;
+    psa_key_id_t end                = start + range;
+
+    VerifyOrReturnError(start >= PSA_KEY_ID_USER_MIN && end - 1 <= PSA_KEY_ID_USER_MAX, CHIP_ERROR_INVALID_ARGUMENT);
+
+    for (keyId = start; keyId < end; keyId++)
+    {
+        psa_status_t status = psa_get_key_attributes(keyId, &attributes);
+        if (status == PSA_ERROR_INVALID_HANDLE)
+        {
+            return CHIP_NO_ERROR;
+        }
+        else if (status != PSA_SUCCESS)
+        {
+            return CHIP_ERROR_INTERNAL;
+        }
+    }
+    return CHIP_ERROR_NOT_FOUND;
+}
+
 CHIP_ERROR PsaKdf::Init(const ByteSpan & secret, const ByteSpan & salt, const ByteSpan & info)
 {
     psa_status_t status        = PSA_SUCCESS;

diff --git a/src/crypto/CHIPCryptoPALPSA.h b/src/crypto/CHIPCryptoPALPSA.h
@@ -55,19 +55,58 @@ namespace Crypto {
 #define CHIP_CONFIG_CRYPTO_PSA_KEY_ID_BASE 0x30000
 #endif // CHIP_CONFIG_CRYPTO_PSA_KEY_ID_BASE
 
+/**
+ * @def CHIP_CONFIG_CRYPTO_PSA_KEY_ID_END
+ *
+ * @brief
+ *   End of the PSA key identifier range used by Matter.
+ *
+ * This setting establishes the maximum limit for the key range specific to Matter, in order to
+ * prevent any overlap with other firmware components that also employ the PSA crypto API.
+ */
+#ifndef CHIP_CONFIG_CRYPTO_PSA_KEY_ID_END
+#define CHIP_CONFIG_CRYPTO_PSA_KEY_ID_END 0x3FFFF
+#endif // CHIP_CONFIG_CRYPTO_PSA_KEY_ID_END
+
+static_assert(PSA_KEY_ID_USER_MIN <= CHIP_CONFIG_CRYPTO_PSA_KEY_ID_BASE && CHIP_CONFIG_CRYPTO_PSA_KEY_ID_END <= PSA_KEY_ID_USER_MAX,
+              "Matter specific PSA key range doesn't fit within PSA allowed range");
+
+// Each ICD client requires storing two keys- AES and HMAC
+static constexpr uint32_t kMaxICDClientKeys = 2 * CHIP_CONFIG_CRYPTO_PSA_ICD_MAX_CLIENTS;
+
+static_assert(kMaxICDClientKeys >= CHIP_CONFIG_ICD_CLIENTS_SUPPORTED_PER_FABRIC * CHIP_CONFIG_MAX_FABRICS,
+              "Number of allocated ICD key slots is lower than maximum number of supported ICD clients");
+
 /**
  * @brief Defines subranges of the PSA key identifier space used by Matter.
  */
 enum class KeyIdBase : psa_key_id_t
 {
-    Minimum     = CHIP_CONFIG_CRYPTO_PSA_KEY_ID_BASE,
-    Operational = Minimum, ///< Base of the PSA key ID range for Node Operational Certificate private keys
-    Maximum     = Operational + kMaxValidFabricIndex,
+    Minimum          = CHIP_CONFIG_CRYPTO_PSA_KEY_ID_BASE,
+    Operational      = Minimum, ///< Base of the PSA key ID range for Node Operational Certificate private keys
+    DACPrivKey       = Operational + kMaxValidFabricIndex + 1,
+    ICDKeyRangeStart = DACPrivKey + 1,
+    Maximum          = ICDKeyRangeStart + kMaxICDClientKeys,
 };
 
-static_assert(to_underlying(KeyIdBase::Minimum) >= PSA_KEY_ID_USER_MIN && to_underlying(KeyIdBase::Maximum) <= PSA_KEY_ID_USER_MAX,
+static_assert(to_underlying(KeyIdBase::Minimum) >= CHIP_CONFIG_CRYPTO_PSA_KEY_ID_BASE &&
+                  to_underlying(KeyIdBase::Maximum) <= CHIP_CONFIG_CRYPTO_PSA_KEY_ID_END,
               "PSA key ID base out of allowed range");
 
+/**
+ * @brief Finds first free persistent Key slot ID within range.
+ *
+ * @param[out] keyId Key ID handler to which free ID will be set.
+ * @param[in]  start Starting ID in search range.
+ * @param[in]  range Search range.
+ *
+ * @retval CHIP_NO_ERROR               On success.
+ * @retval CHIP_ERROR_INTERNAL         On PSA crypto API error.
+ * @retval CHIP_ERROR_NOT_FOUND        On no free Key ID within range.
+ * @retval CHIP_ERROR_INVALID_ARGUMENT On search arguments out of PSA allowed range.
+ */
+CHIP_ERROR FindFreeKeySlotInRange(psa_key_id_t & keyId, psa_key_id_t start, uint32_t range);
+
 /**
  * @brief Calculates PSA key ID for Node Operational Certificate private key for the given fabric.
  */