Bugfix: Group key map validation fixed. (project-chip#22303)

* Bugfix: Group key map validation fixed. * Group Data Provider: Added missing MaxGroupKeysPerFabric validation.
emargolis · Sep 1, 2022 · ef75ba4 · ef75ba4
1 parent 6c9f0a8
commit ef75ba4
Show file tree

Hide file tree

Showing 6 changed files with 235 additions and 109 deletions.
diff --git a/src/app/tests/suites/TestGroupKeyManagementCluster.yaml b/src/app/tests/suites/TestGroupKeyManagementCluster.yaml
@@ -39,7 +39,8 @@ tests:
       command: "readAttribute"
       attribute: "maxGroupKeysPerFabric"
       response:
-          value: 2
+          constraints:
+              minValue: 3
 
     - label: "KeySet Write 1"
       command: "KeySetWrite"
@@ -112,13 +113,28 @@ tests:
       response:
           error: CONSTRAINT_ERROR
 
+    - label: "Write Group Keys (too many)"
+      command: "writeAttribute"
+      attribute: "GroupKeyMap"
+      arguments:
+          value:
+              [
+                  { FabricIndex: 1, GroupId: 0x0101, GroupKeySetID: 0x01a1 },
+                  { FabricIndex: 1, GroupId: 0x0101, GroupKeySetID: 0x01a2 },
+                  { FabricIndex: 1, GroupId: 0x0102, GroupKeySetID: 0x01a1 },
+                  { FabricIndex: 1, GroupId: 0x0102, GroupKeySetID: 0x01a2 },
+              ]
+      response:
+          error: FAILURE
+
     - label: "Write Group Keys"
       command: "writeAttribute"
       attribute: "GroupKeyMap"
       arguments:
           value:
               [
                   { FabricIndex: 1, GroupId: 0x0101, GroupKeySetID: 0x01a1 },
+                  { FabricIndex: 1, GroupId: 0x0102, GroupKeySetID: 0x01a1 },
                   { FabricIndex: 1, GroupId: 0x0102, GroupKeySetID: 0x01a2 },
               ]
 
@@ -129,6 +145,7 @@ tests:
           value:
               [
                   { FabricIndex: 1, GroupId: 0x0101, GroupKeySetID: 0x01a1 },
+                  { FabricIndex: 1, GroupId: 0x0102, GroupKeySetID: 0x01a1 },
                   { FabricIndex: 1, GroupId: 0x0102, GroupKeySetID: 0x01a2 },
               ]
 

diff --git a/src/credentials/GroupDataProviderImpl.cpp b/src/credentials/GroupDataProviderImpl.cpp
@@ -414,6 +414,7 @@ struct GroupData : public GroupDataProvider::GroupInfo, PersistentData<kPersiste
         ReturnErrorOnFailure(writer.Put(TagNext(), static_cast<uint16_t>(next)));
         return writer.EndContainer(container);
     }
+
     CHIP_ERROR Deserialize(TLV::TLVReader & reader) override
     {
         ReturnErrorOnFailure(reader.Next(TLV::AnonymousTag()));
@@ -1420,7 +1421,7 @@ CHIP_ERROR GroupDataProviderImpl::SetGroupKeyAt(chip::FabricIndex fabric_index,
 
     // Insert last
     VerifyOrReturnError(fabric.map_count == index, CHIP_ERROR_INVALID_ARGUMENT);
-    VerifyOrReturnError(fabric.map_count < mMaxGroupKeysPerFabric, CHIP_ERROR_INVALID_LIST_LENGTH);
+    VerifyOrReturnError(fabric.map_count < mMaxGroupsPerFabric, CHIP_ERROR_INVALID_LIST_LENGTH);
 
     map.next = 0;
     ReturnErrorOnFailure(map.Save(mStorage));
@@ -1603,7 +1604,10 @@ CHIP_ERROR GroupDataProviderImpl::SetKeySet(chip::FabricIndex fabric_index, cons
         return keyset.Save(mStorage);
     }
 
-    // New keyset, insert first
+    // New keyset
+    VerifyOrReturnError(fabric.keyset_count < mMaxGroupKeysPerFabric, CHIP_ERROR_INVALID_LIST_LENGTH);
+
+    // Insert first
     keyset.next = fabric.first_keyset;
     ReturnErrorOnFailure(keyset.Save(mStorage));
     // Update fabric

diff --git a/src/credentials/tests/TestGroupDataProvider.cpp b/src/credentials/tests/TestGroupDataProvider.cpp
@@ -48,7 +48,7 @@ static const size_t kSize1  = strlen(kValue1) + 1;
 static const size_t kSize2  = strlen(kValue2) + 1;
 
 constexpr uint16_t kMaxGroupsPerFabric    = 5;
-constexpr uint16_t kMaxGroupKeysPerFabric = 8;
+constexpr uint16_t kMaxGroupKeysPerFabric = 4;
 
 // If test cases covering more than 2 fabrics are added, update `ResetProvider` function.
 constexpr chip::FabricIndex kFabric1 = 1;
@@ -93,6 +93,7 @@ constexpr uint16_t kKeysetId0 = 0x0;
 constexpr uint16_t kKeysetId1 = 0x1111;
 constexpr uint16_t kKeysetId2 = 0x2222;
 constexpr uint16_t kKeysetId3 = 0x3333;
+constexpr uint16_t kKeysetId4 = 0x4444;
 
 static const GroupInfo kGroupInfo1_1(kGroup1, "Group-1.1");
 static const GroupInfo kGroupInfo1_2(kGroup2, "Group-1.2");
@@ -123,6 +124,7 @@ static KeySet kKeySet0(kKeysetId0, SecurityPolicy::kCacheAndSync, 3);
 static KeySet kKeySet1(kKeysetId1, SecurityPolicy::kTrustFirst, 1);
 static KeySet kKeySet2(kKeysetId2, SecurityPolicy::kTrustFirst, 2);
 static KeySet kKeySet3(kKeysetId3, SecurityPolicy::kCacheAndSync, 3);
+static KeySet kKeySet4(kKeysetId4, SecurityPolicy::kTrustFirst, 1);
 
 uint8_t kZeroKey[EpochKey::kLengthBytes] = { 0 };
 
@@ -673,9 +675,7 @@ void TestGroupKeyIterator(nlTestSuite * apSuite, void * apContext)
     NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == provider->SetGroupKeyAt(kFabric1, 2, kGroup3Keyset2));
     NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == provider->SetGroupKeyAt(kFabric1, 3, kGroup3Keyset3));
     NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == provider->SetGroupKeyAt(kFabric1, 4, kGroup1Keyset0));
-    NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == provider->SetGroupKeyAt(kFabric1, 5, kGroup1Keyset1));
-    NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == provider->SetGroupKeyAt(kFabric1, 6, kGroup1Keyset2));
-    NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == provider->SetGroupKeyAt(kFabric1, 7, kGroup1Keyset3));
+    NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR != provider->SetGroupKeyAt(kFabric1, 5, kGroup1Keyset1));
 
     NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == provider->SetGroupKeyAt(kFabric2, 0, kGroup2Keyset0));
     NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == provider->SetGroupKeyAt(kFabric2, 1, kGroup2Keyset1));
@@ -684,8 +684,7 @@ void TestGroupKeyIterator(nlTestSuite * apSuite, void * apContext)
 
     // Iterate fabric 1
 
-    GroupKey expected_f1[]   = { kGroup3Keyset0, kGroup3Keyset1, kGroup3Keyset2, kGroup3Keyset3,
-                               kGroup1Keyset0, kGroup1Keyset1, kGroup1Keyset2, kGroup1Keyset3 };
+    GroupKey expected_f1[]   = { kGroup3Keyset0, kGroup3Keyset1, kGroup3Keyset2, kGroup3Keyset3, kGroup1Keyset0 };
     size_t expected_f1_count = sizeof(expected_f1) / sizeof(GroupKey);
 
     auto it      = provider->IterateGroupKeys(kFabric1);
@@ -738,6 +737,7 @@ void TestKeySets(nlTestSuite * apSuite, void * apContext)
     NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == provider->SetKeySet(kFabric1, kCompressedFabricId1, kKeySet0));
     NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == provider->SetKeySet(kFabric1, kCompressedFabricId1, kKeySet2));
     NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == provider->SetKeySet(kFabric1, kCompressedFabricId1, kKeySet3));
+    NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR != provider->SetKeySet(kFabric1, kCompressedFabricId1, kKeySet4));
     NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == provider->SetKeySet(kFabric2, kCompressedFabricId2, kKeySet3));
     NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == provider->SetKeySet(kFabric2, kCompressedFabricId2, kKeySet0));
     NL_TEST_ASSERT(apSuite, CHIP_NO_ERROR == provider->SetKeySet(kFabric2, kCompressedFabricId2, kKeySet2));

diff --git a/src/lib/core/CHIPConfig.h b/src/lib/core/CHIPConfig.h
@@ -978,7 +978,7 @@ extern const char CHIP_NON_PRODUCTION_MARKER[];
  * Binds to number of KeySet entries to support per fabric (Need at least 1 for Identity Protection Key)
  */
 #ifndef CHIP_CONFIG_MAX_GROUP_KEYS_PER_FABRIC
-#define CHIP_CONFIG_MAX_GROUP_KEYS_PER_FABRIC 2
+#define CHIP_CONFIG_MAX_GROUP_KEYS_PER_FABRIC 3
 #endif
 
 #if CHIP_CONFIG_MAX_GROUP_KEYS_PER_FABRIC < 1