fix PSA test failure

project-chip · Oct 17, 2023 · b0bd539 · b0bd539
1 parent 25531fa
commit b0bd539
Show file tree

Hide file tree

Showing 3 changed files with 46 additions and 24 deletions.
diff --git a/src/app/icd/ICDMonitoringTable.cpp b/src/app/icd/ICDMonitoringTable.cpp
@@ -72,7 +72,7 @@ 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));
+ memcpy(key.AsMutable<Crypto::Aes128KeyByteArray>(), buf.data(), sizeof(Crypto::Aes128KeyByteArray));
  }
  break;
  default:

diff --git a/src/app/icd/ICDMonitoringTable.h b/src/app/icd/ICDMonitoringTable.h
@@ -59,6 +59,23 @@ 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.
+ * In any case, to prevent key leakage, one should either call the DeleteKey method
+ * or save the entry within the ICDMonitoring Table before this object goes out of scope.
+ *
+ * Calling SetKey() twice on the same object will result in the Key being deleted from
+ * the keyStore even if the entry was previously saved in the table. One should use a new
+ * object or manually clear the content of the key handle prior to calling SetKey() again.
+ *
+ * @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 Crypto API related failure
+ * CHIP_ERROR_XXX Crypto API related failure
+ */
  CHIP_ERROR SetKey(ByteSpan keyData);
  CHIP_ERROR DeleteKey(void);
 

diff --git a/src/app/tests/TestICDMonitoringTable.cpp b/src/app/tests/TestICDMonitoringTable.cpp
@@ -93,24 +93,27 @@ void TestSaveAndLoadRegistrationValue(nlTestSuite * aSuite, void * aContext)
  CHIP_ERROR err;
 
  // Insert first entry
- entry.checkInNodeID = kClientNodeId11;
- entry.monitoredSubject = kClientNodeId12;
- NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == entry.SetKey(ByteSpan(kKeyBuffer1a)));
- err = saving.Set(0, entry);
+ ICDMonitoringEntry entry1(&keystore);
+ entry1.checkInNodeID = kClientNodeId11;
+ entry1.monitoredSubject = kClientNodeId12;
+ NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == entry1.SetKey(ByteSpan(kKeyBuffer1a)));
+ err = saving.Set(0, entry1);
  NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == err);
 
  // Insert second entry
- entry.checkInNodeID = kClientNodeId12;
- entry.monitoredSubject = kClientNodeId11;
- NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == entry.SetKey(ByteSpan(kKeyBuffer2a)));
- err = saving.Set(1, entry);
+ ICDMonitoringEntry entry2(&keystore);
+ entry2.checkInNodeID = kClientNodeId12;
+ entry2.monitoredSubject = kClientNodeId11;
+ NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == entry2.SetKey(ByteSpan(kKeyBuffer2a)));
+ err = saving.Set(1, entry2);
  NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == err);
 
  // Insert one too many
- entry.checkInNodeID = kClientNodeId13;
- entry.monitoredSubject = kClientNodeId13;
- NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == entry.SetKey(ByteSpan(kKeyBuffer3a)));
- err = saving.Set(2, entry);
+ ICDMonitoringEntry entry3(&keystore);
+ entry3.checkInNodeID = kClientNodeId13;
+ entry3.monitoredSubject = kClientNodeId13;
+ NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == entry3.SetKey(ByteSpan(kKeyBuffer3a)));
+ err = saving.Set(2, entry3);
  NL_TEST_ASSERT(aSuite, CHIP_ERROR_INVALID_ARGUMENT == err);
 
  // Retrieve first entry
@@ -134,28 +137,30 @@ void TestSaveAndLoadRegistrationValue(nlTestSuite * aSuite, void * aContext)
  NL_TEST_ASSERT(aSuite, 2 == loading.Limit());
  NL_TEST_ASSERT(aSuite, CHIP_ERROR_NOT_FOUND == err);
 
- // Overwrite first entry
- entry.checkInNodeID = kClientNodeId13;
- entry.monitoredSubject = kClientNodeId11;
- NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == entry.SetKey(ByteSpan(kKeyBuffer1b)));
- err = saving.Set(0, entry);
+ // Remove first entry
+ saving.Remove(0);
+ ICDMonitoringEntry entry4(&keystore);
+ entry4.checkInNodeID = kClientNodeId13;
+ entry4.monitoredSubject = kClientNodeId11;
+ NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == entry4.SetKey(ByteSpan(kKeyBuffer1b)));
+ err = saving.Set(1, entry4);
  NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == err);
 
- // Retrieve first entry (modified)
+ // Retrieve first entry (not modified but shifted)
  err = loading.Get(0, entry);
  NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == err);
  NL_TEST_ASSERT(aSuite, kTestFabricIndex1 == entry.fabricIndex);
- NL_TEST_ASSERT(aSuite, kClientNodeId13 == entry.checkInNodeID);
+ NL_TEST_ASSERT(aSuite, kClientNodeId12 == entry.checkInNodeID);
  NL_TEST_ASSERT(aSuite, kClientNodeId11 == entry.monitoredSubject);
- NL_TEST_ASSERT(aSuite, entry.IsKeyEquivalent(ByteSpan(kKeyBuffer1b)));
+ NL_TEST_ASSERT(aSuite, entry.IsKeyEquivalent(ByteSpan(kKeyBuffer2a)));
 
- // Retrieve second entry (not modified)
+ // Retrieve second entry
  err = loading.Get(1, entry);
  NL_TEST_ASSERT(aSuite, CHIP_NO_ERROR == err);
  NL_TEST_ASSERT(aSuite, kTestFabricIndex1 == entry.fabricIndex);
- NL_TEST_ASSERT(aSuite, kClientNodeId12 == entry.checkInNodeID);
+ NL_TEST_ASSERT(aSuite, kClientNodeId13 == entry.checkInNodeID);
  NL_TEST_ASSERT(aSuite, kClientNodeId11 == entry.monitoredSubject);
- NL_TEST_ASSERT(aSuite, entry.IsKeyEquivalent(ByteSpan(kKeyBuffer2a)));
+ NL_TEST_ASSERT(aSuite, entry.IsKeyEquivalent(ByteSpan(kKeyBuffer1b)));
 }
 
 void TestSaveAllInvalidRegistrationValues(nlTestSuite * aSuite, void * aContext)