[Credentials] Fix memory exposure (#22794)

* Fix memory exposure * Apply PR comments
project-chip · Mar 9, 2023 · 4345626 · 4345626
1 parent f09ccd9
commit 4345626
Show file tree

Hide file tree

Showing 2 changed files with 35 additions and 13 deletions.
diff --git a/src/credentials/GroupDataProviderImpl.cpp b/src/credentials/GroupDataProviderImpl.cpp
@@ -770,13 +770,27 @@ struct KeySetData : PersistentData<kPersistentBufferMax>
         {
             TLV::TLVType array, item;
             ReturnErrorOnFailure(writer.StartContainer(TagGroupCredentials(), TLV::kTLVType_Array, array));
+            uint8_t keyCount   = 0;
+            uint64_t startTime = 0;
+            uint16_t hash      = 0;
+            uint8_t encryptionKey[Crypto::CHIP_CRYPTO_SYMMETRIC_KEY_LENGTH_BYTES];
             for (auto & key : operational_keys)
             {
+                startTime = 0;
+                hash      = 0;
+                memset(encryptionKey, 0, sizeof(encryptionKey));
                 ReturnErrorOnFailure(writer.StartContainer(TLV::AnonymousTag(), TLV::kTLVType_Structure, item));
-                ReturnErrorOnFailure(writer.Put(TagStartTime(), static_cast<uint64_t>(key.start_time)));
-                ReturnErrorOnFailure(writer.Put(TagKeyHash(), key.hash));
-                ReturnErrorOnFailure(
-                    writer.Put(TagKeyValue(), ByteSpan(key.encryption_key, Crypto::CHIP_CRYPTO_SYMMETRIC_KEY_LENGTH_BYTES)));
+
+                if (keyCount++ < keys_count)
+                {
+                    startTime = key.start_time;
+                    hash      = key.hash;
+                    memcpy(encryptionKey, key.encryption_key, sizeof(encryptionKey));
+                }
+                ReturnErrorOnFailure(writer.Put(TagStartTime(), static_cast<uint64_t>(startTime)));
+                ReturnErrorOnFailure(writer.Put(TagKeyHash(), hash));
+                ReturnErrorOnFailure(writer.Put(TagKeyValue(), ByteSpan(encryptionKey)));
+
                 ReturnErrorOnFailure(writer.EndContainer(item));
             }
             ReturnErrorOnFailure(writer.EndContainer(array));

diff --git a/src/credentials/tests/TestGroupDataProvider.cpp b/src/credentials/tests/TestGroupDataProvider.cpp
@@ -165,16 +165,24 @@ void ResetProvider(GroupDataProvider * provider)
     provider->RemoveFabric(kFabric2);
 }
 
-bool CompareKeySets(const KeySet & keyset1, const KeySet & keyset2)
+bool CompareKeySets(const KeySet & retrievedKeySet, const KeySet & keyset2)
 {
-    VerifyOrReturnError(keyset1.policy == keyset2.policy, false);
-    VerifyOrReturnError(keyset1.num_keys_used == keyset2.num_keys_used, false);
-    VerifyOrReturnError(keyset1.epoch_keys[0].start_time == keyset2.epoch_keys[0].start_time, false);
-    VerifyOrReturnError(keyset1.epoch_keys[1].start_time == keyset2.epoch_keys[1].start_time, false);
-    VerifyOrReturnError(keyset1.epoch_keys[2].start_time == keyset2.epoch_keys[2].start_time, false);
-    VerifyOrReturnError(0 == memcmp(kZeroKey, keyset1.epoch_keys[0].key, EpochKey::kLengthBytes), false);
-    VerifyOrReturnError(0 == memcmp(kZeroKey, keyset1.epoch_keys[1].key, EpochKey::kLengthBytes), false);
-    VerifyOrReturnError(0 == memcmp(kZeroKey, keyset1.epoch_keys[2].key, EpochKey::kLengthBytes), false);
+    VerifyOrReturnError(retrievedKeySet.policy == keyset2.policy, false);
+    VerifyOrReturnError(retrievedKeySet.num_keys_used == keyset2.num_keys_used, false);
+
+    for (int i = 0; i < 3; i++)
+    {
+        if (i < retrievedKeySet.num_keys_used)
+        {
+            VerifyOrReturnError(retrievedKeySet.epoch_keys[i].start_time == keyset2.epoch_keys[i].start_time, false);
+        }
+        else
+        {
+            VerifyOrReturnError(retrievedKeySet.epoch_keys[i].start_time == 0, false);
+        }
+
+        VerifyOrReturnError(0 == memcmp(kZeroKey, retrievedKeySet.epoch_keys[i].key, EpochKey::kLengthBytes), false);
+    }
     return true;
 }