Fix KeySetWrite command payload validation.

There are three fixes here: 1. Move the epoch key validity checks up front, since per spec those should happen before any internal state verification checks. 2. Add a check that the GroupKeySecurityPolicy in the keyset has a valid value for GroupKeySecurityPolicyEnum. 3. If we don't support MCSP, we should be failing out if the GroupKeySecurityPolicy is set to CacheAndSync. Fixes #26692
project-chip · May 22, 2023 · ed777af · ed777af
1 parent bfa0580
commit ed777af
Show file tree

Hide file tree

Showing 6 changed files with 753 additions and 247 deletions.
diff --git a/src/app/clusters/group-key-mgmt-server/group-key-mgmt-server.cpp b/src/app/clusters/group-key-mgmt-server/group-key-mgmt-server.cpp
@@ -109,6 +109,9 @@ class GroupKeyManagementAttributeAccess : public AttributeAccessInterface
     // Register for the GroupKeyManagement cluster on all endpoints.
     GroupKeyManagementAttributeAccess() : AttributeAccessInterface(Optional<EndpointId>(0), GroupKeyManagement::Id) {}
 
+    // TODO: Once there is MCSP support, this may need to change.
+    static constexpr bool IsMCSPSupported() { return false; }
+
     CHIP_ERROR Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder) override
     {
         VerifyOrDie(aPath.mClusterId == GroupKeyManagement::Id);
@@ -117,8 +120,15 @@ class GroupKeyManagementAttributeAccess : public AttributeAccessInterface
         {
         case GroupKeyManagement::Attributes::ClusterRevision::Id:
             return ReadClusterRevision(aPath.mEndpointId, aEncoder);
-        case Attributes::FeatureMap::Id:
-            return aEncoder.Encode(static_cast<uint32_t>(0));
+        case Attributes::FeatureMap::Id: {
+            uint32_t features = 0;
+            if (IsMCSPSupported())
+            {
+                // TODO: Once there is MCSP support, this will need to add the
+                // right feature bit.
+            }
+            return aEncoder.Encode(features);
+        }
         case GroupKeyManagement::Attributes::GroupKeyMap::Id:
             return ReadGroupKeyMap(aPath.mEndpointId, aEncoder);
         case GroupKeyManagement::Attributes::GroupTable::Id:
@@ -296,29 +306,32 @@ bool emberAfGroupKeyManagementClusterKeySetWriteCallback(
     chip::app::CommandHandler * commandObj, const chip::app::ConcreteCommandPath & commandPath,
     const chip::app::Clusters::GroupKeyManagement::Commands::KeySetWrite::DecodableType & commandData)
 {
-    auto provider = GetGroupDataProvider();
-    auto fabric   = Server::GetInstance().GetFabricTable().FindFabricWithIndex(commandObj->GetAccessingFabricIndex());
-
-    if (nullptr == provider || nullptr == fabric)
+    if (commandData.groupKeySet.epochKey0.IsNull() || commandData.groupKeySet.epochStartTime0.IsNull() ||
+        commandData.groupKeySet.epochKey0.Value().empty() || (0 == commandData.groupKeySet.epochStartTime0.Value()))
     {
-        commandObj->AddStatus(commandPath, Status::Failure);
+        // If the EpochKey0 field is null or its associated EpochStartTime0 field is null,
+        // then this command SHALL fail with an INVALID_COMMAND
+        commandObj->AddStatus(commandPath, Status::InvalidCommand);
         return true;
     }
 
-    uint8_t compressed_fabric_id_buffer[sizeof(uint64_t)];
-    MutableByteSpan compressed_fabric_id(compressed_fabric_id_buffer);
-    CHIP_ERROR err = fabric->GetCompressedFabricIdBytes(compressed_fabric_id);
-    if (CHIP_NO_ERROR != err)
+    if (commandData.groupKeySet.groupKeySecurityPolicy == GroupKeySecurityPolicyEnum::kUnknownEnumValue)
     {
-        commandObj->AddStatus(commandPath, Status::Failure);
+        // If a client indicates an enumeration value to the server, that is not
+        // supported by the server, because it is ... a new value unrecognized
+        // by a legacy server, then the server SHALL generate a general
+        // constraint error
+        commandObj->AddStatus(commandPath, Status::ConstraintError);
         return true;
     }
 
-    if (commandData.groupKeySet.epochKey0.IsNull() || commandData.groupKeySet.epochStartTime0.IsNull() ||
-        commandData.groupKeySet.epochKey0.Value().empty() || (0 == commandData.groupKeySet.epochStartTime0.Value()))
+    if (!GroupKeyManagementAttributeAccess::IsMCSPSupported() &&
+        commandData.groupKeySet.groupKeySecurityPolicy == GroupKeySecurityPolicyEnum::kCacheAndSync)
     {
-        // If the EpochKey0 field is null or its associated EpochStartTime0 field is null,
-        // then this command SHALL fail with an INVALID_COMMAND
+        // When CacheAndSync is not supported in the FeatureMap of this cluster,
+        // any action attempting to set CacheAndSync in the
+        // GroupKeySecurityPolicy field SHALL fail with an INVALID_COMMAND
+        // error.
         commandObj->AddStatus(commandPath, Status::InvalidCommand);
         return true;
     }
@@ -366,6 +379,24 @@ bool emberAfGroupKeyManagementClusterKeySetWriteCallback(
         keyset.num_keys_used++;
     }
 
+    auto provider = GetGroupDataProvider();
+    auto fabric   = Server::GetInstance().GetFabricTable().FindFabricWithIndex(commandObj->GetAccessingFabricIndex());
+
+    if (nullptr == provider || nullptr == fabric)
+    {
+        commandObj->AddStatus(commandPath, Status::Failure);
+        return true;
+    }
+
+    uint8_t compressed_fabric_id_buffer[sizeof(uint64_t)];
+    MutableByteSpan compressed_fabric_id(compressed_fabric_id_buffer);
+    CHIP_ERROR err = fabric->GetCompressedFabricIdBytes(compressed_fabric_id);
+    if (CHIP_NO_ERROR != err)
+    {
+        commandObj->AddStatus(commandPath, Status::Failure);
+        return true;
+    }
+
     // Set KeySet
     err = provider->SetKeySet(fabric->GetFabricIndex(), compressed_fabric_id, keyset);
     if (CHIP_NO_ERROR == err)

diff --git a/src/app/tests/suites/TestGroupKeyManagementCluster.yaml b/src/app/tests/suites/TestGroupKeyManagementCluster.yaml
@@ -91,8 +91,9 @@ tests:
                         EpochStartTime2: 1110002,
                     }
 
-    - label: "KeySet Write 2"
+    - label: "KeySet Write 2 CacheAndSync"
       command: "KeySetWrite"
+      PICS: GRPKEY.S.F00
       arguments:
           values:
               - name: "GroupKeySet"
@@ -108,9 +109,28 @@ tests:
                         EpochStartTime2: 2110002,
                     }
 
-    - label: "KeySet Write 3"
+    - label: "KeySet Write 2 TrustFirst"
+      command: "KeySetWrite"
+      PICS: "!GRPKEY.S.F00"
+      arguments:
+          values:
+              - name: "GroupKeySet"
+                value:
+                    {
+                        GroupKeySetID: 0x01a2,
+                        GroupKeySecurityPolicy: 0,
+                        EpochKey0: "\xd0\xd1\xd2\xd3\xd4\xd5\xd6\xd7\xd8\xd9\xda\xdb\xdc\xdd\xde\xdf",
+                        EpochStartTime0: 2110000,
+                        EpochKey1: "\xe0\xe1\xe2\xe3\xe4\xe5\xe6\xe7\xe8\xe9\xea\xeb\xec\xed\xee\xef",
+                        EpochStartTime1: 2110001,
+                        EpochKey2: "\xf0\xf1\xf2\xf3\xf4\xf5\xf6\xf7\xf8\xf9\xfa\xfb\xfc\xfd\xfe\xff",
+                        EpochStartTime2: 2110002,
+                    }
+
+    - label: "KeySet Write 3 CacheAndSync"
       identity: "beta"
       command: "KeySetWrite"
+      PICS: GRPKEY.S.F00
       arguments:
           values:
               - name: "GroupKeySet"
@@ -128,6 +148,27 @@ tests:
                         EpochStartTime2: 2110002,
                     }
 
+    - label: "KeySet Write 3 TrustFirst"
+      identity: "beta"
+      command: "KeySetWrite"
+      PICS: "!GRPKEY.S.F00"
+      arguments:
+          values:
+              - name: "GroupKeySet"
+                value:
+                    {
+                        GroupKeySetID: 0x01a3,
+                        GroupKeySecurityPolicy: 0,
+                        EpochKey0:
+                            "\x00\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0a\x0b\x0c\x0d\x0e\x0f",
+                        EpochStartTime0: 2110000,
+                        EpochKey1: "\x10\x11\x12\x13\x14\x15\x16\x17\x18\x19\x1a\x1b\x1c\x1d\x1e\x1f",
+                        EpochStartTime1: 2110001,
+                        EpochKey2:
+                            "\x20\x21\x22\x23\x24\x25\x26\x27\x28\x29\x2a\x2b\x2c\x2d\x2e\x2f",
+                        EpochStartTime2: 2110002,
+                    }
+
     - label: "KeySet Read"
       command: "KeySetRead"
       arguments:
@@ -502,8 +543,9 @@ tests:
       response:
           error: NOT_FOUND
 
-    - label: "KeySet Read (not removed)"
+    - label: "KeySet Read (not removed) CacheAndSync"
       command: "KeySetRead"
+      PICS: GRPKEY.S.F00
       arguments:
           values:
               - name: "GroupKeySetID"
@@ -523,6 +565,28 @@ tests:
                         EpochStartTime2: 2110002,
                     }
 
+    - label: "KeySet Read (not removed) TrustFirst"
+      command: "KeySetRead"
+      PICS: GRPKEY.S.F00
+      arguments:
+          values:
+              - name: "GroupKeySetID"
+                value: 0x01a2
+      response:
+          values:
+              - name: "GroupKeySet"
+                value:
+                    {
+                        GroupKeySetID: 0x01a2,
+                        GroupKeySecurityPolicy: 0,
+                        EpochKey0: null,
+                        EpochStartTime0: 2110000,
+                        EpochKey1: null,
+                        EpochStartTime1: 2110001,
+                        EpochKey2: null,
+                        EpochStartTime2: 2110002,
+                    }
+
     - label: "Remove Group 1"
       cluster: "Groups"
       endpoint: 1
@@ -608,8 +672,9 @@ tests:
                         EpochStartTime2: 1110002,
                     }
 
-    - label: "KeySet Write 2"
+    - label: "KeySet Write 2 CacheAndSync"
       command: "KeySetWrite"
+      PICS: GRPKEY.S.F00
       arguments:
           values:
               - name: "GroupKeySet"
@@ -625,6 +690,24 @@ tests:
                         EpochStartTime2: 2110002,
                     }
 
+    - label: "KeySet Write 2 TrustFirst"
+      command: "KeySetWrite"
+      PICS: "!GRPKEY.S.F00"
+      arguments:
+          values:
+              - name: "GroupKeySet"
+                value:
+                    {
+                        GroupKeySetID: 0x01a2,
+                        GroupKeySecurityPolicy: 0,
+                        EpochKey0: "\xd0\xd1\xd2\xd3\xd4\xd5\xd6\xd7\xd8\xd9\xda\xdb\xdc\xdd\xde\xdf",
+                        EpochStartTime0: 2110000,
+                        EpochKey1: "\xe0\xe1\xe2\xe3\xe4\xe5\xe6\xe7\xe8\xe9\xea\xeb\xec\xed\xee\xef",
+                        EpochStartTime1: 2110001,
+                        EpochKey2: "\xf0\xf1\xf2\xf3\xf4\xf5\xf6\xf7\xf8\xf9\xfa\xfb\xfc\xfd\xfe\xff",
+                        EpochStartTime2: 2110002,
+                    }
+
     - label: "Map Group 1 and Group 2 to KeySet 1 and group 2 to KeySet 2"
       command: "writeAttribute"
       attribute: "GroupKeyMap"

diff --git a/src/app/tests/suites/certification/PICS.yaml b/src/app/tests/suites/certification/PICS.yaml
@@ -3616,6 +3616,14 @@ PICS:
           client?"
       id: GRPKEY.C
 
+    #
+    # server / features
+    #
+    - label:
+          "Does the DUT(Server) support Group Key Management CacheAndSync
+          feature?"
+      id: GRPKEY.S.F00
+
     #
     # client / attributes
     #

diff --git a/src/app/tests/suites/certification/ci-pics-values b/src/app/tests/suites/certification/ci-pics-values
@@ -580,9 +580,11 @@ G.C.C03.Tx=0
 G.C.C04.Tx=0
 G.C.C05.Tx=0
 
-GRPKEY.C=1
-GRPKEY.S.A0001=0
 GRPKEY.S=1
+GRPKEY.S.F00=0 # No support for the CacheAndSync feature
+GRPKEY.S.A0001=0
+
+GRPKEY.C=1
 GRPKEY.C.A0000=1
 GRPKEY.C.A0001=0
 GRPKEY.C.C00.Tx=1