[ICD] Remove PSA Crypto exclusion from Unit test (#29753)

* Remove PSA Crypto exclusion

* fix PSA test failure

* Add more comments

* Fix comments

* Add isKeySet flag

* address comments

src/app/icd/ICDManagementServer.cpp

@@ -39,7 +39,12 @@ Status ICDManagementServer::RegisterClient(PersistentStorageDelegate & storage,
  // Save
  entry.checkInNodeID = node_id;
  entry.monitoredSubject = monitored_subject;
- err = entry.SetKey(key);
+ if (entry.keyHandleValid)
+ {
+ entry.DeleteKey();
+ }
+
+ err = entry.SetKey(key);
  VerifyOrReturnError(CHIP_ERROR_INVALID_ARGUMENT != err, InteractionModel::Status::ConstraintError);
  VerifyOrReturnError(CHIP_NO_ERROR == err, InteractionModel::Status::Failure);
  err = table.Set(entry.index, entry);

src/app/icd/ICDMonitoringTable.cpp

@@ -72,7 +72,12 @@ CHIP_ERROR ICDMonitoringEntry::Deserialize(TLV::TLVReader & reader)
  case to_underlying(Fields::kKey): {
  ByteSpan buf(key.AsMutable<Crypto::Aes128KeyByteArray>());
  ReturnErrorOnFailure(reader.Get(buf));
- ReturnErrorOnFailure(this->SetKey(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 key in storage and will cause
+ // key leakage in some implementation.
+ memcpy(key.AsMutable<Crypto::Aes128KeyByteArray>(), buf.data(), sizeof(Crypto::Aes128KeyByteArray));
+ keyHandleValid = true;
  }
  break;
  default:
@@ -90,24 +95,20 @@ void ICDMonitoringEntry::Clear()
 {
  this->checkInNodeID = kUndefinedNodeId;
  this->monitoredSubject = kUndefinedNodeId;
- if (symmetricKeystore != nullptr)
- {
- symmetricKeystore->DestroyKey(this->key);
- }
+ this->keyHandleValid = false;
 }
 
 CHIP_ERROR ICDMonitoringEntry::SetKey(ByteSpan keyData)
 {
  VerifyOrReturnError(keyData.size() == sizeof(Crypto::Aes128KeyByteArray), CHIP_ERROR_INVALID_ARGUMENT);
  VerifyOrReturnError(symmetricKeystore != nullptr, CHIP_ERROR_INTERNAL);
-
- // DeleteKey in case the handle was already used.
- DeleteKey();
+ VerifyOrReturnError(!keyHandleValid, CHIP_ERROR_INTERNAL);
 
  Crypto::Aes128KeyByteArray keyMaterial;
  memcpy(keyMaterial, keyData.data(), sizeof(Crypto::Aes128KeyByteArray));
 
  ReturnErrorOnFailure(symmetricKeystore->CreateKey(keyMaterial, key));
+ keyHandleValid = true;
 
  return CHIP_NO_ERROR;
 }
@@ -116,13 +117,15 @@ CHIP_ERROR ICDMonitoringEntry::DeleteKey()
 {
  VerifyOrReturnError(symmetricKeystore != nullptr, CHIP_ERROR_INTERNAL);
  symmetricKeystore->DestroyKey(this->key);
+ keyHandleValid = false;
  return CHIP_NO_ERROR;
 }
 
 bool ICDMonitoringEntry::IsKeyEquivalent(ByteSpan keyData)
 {
  VerifyOrReturnValue(keyData.size() == Crypto::CHIP_CRYPTO_SYMMETRIC_KEY_LENGTH_BYTES, false);
  VerifyOrReturnValue(symmetricKeystore != nullptr, false);
+ VerifyOrReturnValue(keyHandleValid, false);
 
  ICDMonitoringEntry tempEntry(symmetricKeystore);
 
@@ -187,6 +190,7 @@ CHIP_ERROR ICDMonitoringTable::Set(uint16_t index, const ICDMonitoringEntry & en
  VerifyOrReturnError(index < this->Limit(), CHIP_ERROR_INVALID_ARGUMENT);
  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;
@@ -209,7 +213,7 @@ CHIP_ERROR ICDMonitoringTable::Remove(uint16_t index)
  // Shift remaining entries down one position
  while (CHIP_NO_ERROR == this->Get(static_cast<uint16_t>(index + 1), entry))
  {
- this->Set(index++, entry);
+ ReturnErrorOnFailure(this->Set(index++, entry));
  }
 
  // Remove last entry

src/app/icd/ICDMonitoringTable.h

@@ -59,6 +59,22 @@ struct ICDMonitoringEntry : public PersistentData<kICDMonitoringBufferSize>
  CHIP_ERROR Serialize(TLV::TLVWriter & writer) const override;
  CHIP_ERROR Deserialize(TLV::TLVReader & reader) override;
  void Clear() override;
+ /**
+ * @brief Set the Key object
+ * This method will create a new keyHandle. The key handle might contain either
+ * the raw key or a keyID depending on which Crypto implementation is used.
+ * To avoid leaking keys, API consumers must either call the DeleteKey method
+ * or save the entry within the ICDMonitoring Table before this object goes out of scope.
+ *
+ * A new entry object should be used for each key when adding entries to the ICDMonitoring
+ * table.
+ *
+ * @param keyData A byte span containing the raw key
+ * @return CHIP_ERROR CHIP_NO_ERROR success
+ * CHIP_ERROR_INVALID_ARGUMENT wrong size of the raw key
+ * CHIP_ERROR_INTERNAL No KeyStore for the entry or Key Handle already present
+ * CHIP_ERROR_XXX Crypto API related failure
+ */
  CHIP_ERROR SetKey(ByteSpan keyData);
  CHIP_ERROR DeleteKey(void);
 
@@ -83,6 +99,7 @@ struct ICDMonitoringEntry : public PersistentData<kICDMonitoringBufferSize>
  chip::NodeId checkInNodeID = kUndefinedNodeId;
  uint64_t monitoredSubject = static_cast<uint64_t>(0);
  Crypto::Aes128KeyHandle key = Crypto::Aes128KeyHandle();
+ bool keyHandleValid = false;
  uint16_t index = 0;
  Crypto::SymmetricKeystore * symmetricKeystore = nullptr;
 };