Fix KeySetRemove to fail on key set index 0 (#28524)

* Fix KeySetRemove to fail on key set index 0 Problem: - Removing KeySet index 0 is not allowed by spec, but SDK allowed it. - Checking that we ran without accessing fabric is not done, so error is FAILURE instead of UNSUPPORTED_ACCESS. - Fixes #28518 This PR: - Adds the check and tests against removing fabric index zero - Improves error reporting for several errors in the cluster - Combines validation logic for accessing fabric that was missing Testing: - Added unit tests and integration tests for additions (except for the unsupported access that requires PASE to check) * Regen tests with ZAP * Restyled by clang-format * Remove explicit check for undefined fabric index * Fix build * Rename ValidateAndGetProviderAndFabric to just GetProviderAndFabric * Added back cleanup for VerifyOrDie argument checking * Restyle --------- Co-authored-by: [email protected] <[email protected]> Co-authored-by: Restyled.io <[email protected]> Co-authored-by: Andrei Litvin <[email protected]> Co-authored-by: Andrei Litvin <[email protected]>
project-chip · Nov 17, 2023 · 5623729 · 5623729
1 parent 2932443
commit 5623729
Show file tree

Hide file tree

Showing 6 changed files with 107 additions and 46 deletions.
diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp
@@ -303,6 +303,9 @@ Status CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommandElem
 
     if (CommandIsFabricScoped(concretePath.mClusterId, concretePath.mCommandId))
     {
+        // SPEC: Else if the command in the path is fabric-scoped and there is no accessing fabric,
+        // a CommandStatusIB SHALL be generated with the UNSUPPORTED_ACCESS Status Code.
+
         // Fabric-scoped commands are not allowed before a specific accessing fabric is available.
         // This is mostly just during a PASE session before AddNOC.
         if (GetAccessingFabricIndex() == kUndefinedFabricIndex)
@@ -470,8 +473,7 @@ CHIP_ERROR CommandHandler::AddStatus(const ConcreteCommandPath & aCommandPath, c
     return AddStatusInternal(aCommandPath, StatusIB(aStatus));
 }
 
-CHIP_ERROR CommandHandler::AddStatusAndLogIfFailure(const ConcreteCommandPath & aCommandPath, const Status aStatus,
-                                                    const char * aMessage)
+void CommandHandler::AddStatusAndLogIfFailure(const ConcreteCommandPath & aCommandPath, const Status aStatus, const char * aMessage)
 {
     if (aStatus != Status::Success)
     {
@@ -482,7 +484,7 @@ CHIP_ERROR CommandHandler::AddStatusAndLogIfFailure(const ConcreteCommandPath &
                      ChipLogValueIMStatus(aStatus), aMessage);
     }
 
-    return AddStatus(aCommandPath, aStatus);
+    LogErrorOnFailure(AddStatus(aCommandPath, aStatus));
 }
 
 CHIP_ERROR CommandHandler::AddClusterSpecificSuccess(const ConcreteCommandPath & aCommandPath, ClusterStatus aClusterStatus)

diff --git a/src/app/CommandHandler.h b/src/app/CommandHandler.h
@@ -173,9 +173,10 @@ class CommandHandler : public Messaging::ExchangeDelegate
     CHIP_ERROR AddStatus(const ConcreteCommandPath & aCommandPath, const Protocols::InteractionModel::Status aStatus);
 
     // Same as AddStatus, but logs that the command represented by aCommandPath failed with the given
-    // error status and error message, if aStatus is an error.
-    CHIP_ERROR AddStatusAndLogIfFailure(const ConcreteCommandPath & aCommandPath, const Protocols::InteractionModel::Status aStatus,
-                                        const char * aMessage);
+    // error status and error message, if aStatus is an error. Errors on AddStatus are just logged
+    // (since the caller likely can only log and not further add a status).
+    void AddStatusAndLogIfFailure(const ConcreteCommandPath & aCommandPath, const Protocols::InteractionModel::Status aStatus,
+                                  const char * aMessage);
 
     CHIP_ERROR AddClusterSpecificSuccess(const ConcreteCommandPath & aCommandPath, ClusterStatus aClusterStatus);
 

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
@@ -401,7 +401,34 @@ ValidateKeySetWriteArguments(const chip::app::Clusters::GroupKeyManagement::Comm
     return Status::Success;
 }
 
-constexpr uint16_t GroupKeyManagementAttributeAccess::kClusterRevision;
+bool GetProviderAndFabric(chip::app::CommandHandler * commandObj, const chip::app::ConcreteCommandPath & commandPath,
+                          Credentials::GroupDataProvider ** outGroupDataProvider, const FabricInfo ** outFabricInfo)
+{
+    VerifyOrDie(commandObj != nullptr);
+    VerifyOrDie(outGroupDataProvider != nullptr);
+    VerifyOrDie(outFabricInfo != nullptr);
+
+    // Internal failures on internal inconsistencies.
+    auto provider = GetGroupDataProvider();
+    auto fabric   = Server::GetInstance().GetFabricTable().FindFabricWithIndex(commandObj->GetAccessingFabricIndex());
+
+    if (nullptr == provider)
+    {
+        commandObj->AddStatusAndLogIfFailure(commandPath, Status::Failure, "Internal consistency error on provider!");
+        return false;
+    }
+
+    if (nullptr == fabric)
+    {
+        commandObj->AddStatusAndLogIfFailure(commandPath, Status::Failure, "Internal consistency error on access fabric!");
+        return false;
+    }
+
+    *outGroupDataProvider = provider;
+    *outFabricInfo        = fabric;
+
+    return true;
+}
 
 GroupKeyManagementAttributeAccess gAttribute;
 
@@ -420,12 +447,13 @@ 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());
+    Credentials::GroupDataProvider * provider = nullptr;
+    const FabricInfo * fabric                 = nullptr;
 
-    if (nullptr == provider || nullptr == fabric)
+    // Flight-check fabric scoping.
+    if (!GetProviderAndFabric(commandObj, commandPath, &provider, &fabric))
     {
-        commandObj->AddStatusAndLogIfFailure(commandPath, Status::Failure, "Internal consistency error on provider/fabric");
+        // Command will already have status populated from validation.
         return true;
     }
 
@@ -516,32 +544,30 @@ bool emberAfGroupKeyManagementClusterKeySetWriteCallback(
     }
 
     // Send response
-    err = commandObj->AddStatus(commandPath, StatusIB(err).mStatus);
-    if (CHIP_NO_ERROR != err)
-    {
-        ChipLogDetail(Zcl, "GroupKeyManagementCluster: KeySetWrite failed: %" CHIP_ERROR_FORMAT, err.Format());
-    }
+    commandObj->AddStatus(commandPath, StatusIB(err).mStatus);
     return true;
 }
 
 bool emberAfGroupKeyManagementClusterKeySetReadCallback(
     chip::app::CommandHandler * commandObj, const chip::app::ConcreteCommandPath & commandPath,
     const chip::app::Clusters::GroupKeyManagement::Commands::KeySetRead::DecodableType & commandData)
 {
-    auto fabric     = commandObj->GetAccessingFabricIndex();
-    auto * provider = GetGroupDataProvider();
+    Credentials::GroupDataProvider * provider = nullptr;
+    const FabricInfo * fabric                 = nullptr;
 
-    if (nullptr == provider)
+    // Flight-check fabric scoping.
+    if (!GetProviderAndFabric(commandObj, commandPath, &provider, &fabric))
     {
-        commandObj->AddStatus(commandPath, Status::Failure);
+        // Command will already have status populated from validation.
         return true;
     }
 
+    FabricIndex fabricIndex = fabric->GetFabricIndex();
     GroupDataProvider::KeySet keyset;
-    if (CHIP_NO_ERROR != provider->GetKeySet(fabric, commandData.groupKeySetID, keyset))
+    if (CHIP_NO_ERROR != provider->GetKeySet(fabricIndex, commandData.groupKeySetID, keyset))
     {
         // KeySet ID not found
-        commandObj->AddStatus(commandPath, Status::NotFound);
+        commandObj->AddStatusAndLogIfFailure(commandPath, Status::NotFound, "Keyset ID not found in KeySetRead");
         return true;
     }
 
@@ -592,30 +618,41 @@ bool emberAfGroupKeyManagementClusterKeySetRemoveCallback(
     const chip::app::Clusters::GroupKeyManagement::Commands::KeySetRemove::DecodableType & commandData)
 
 {
-    auto fabric     = commandObj->GetAccessingFabricIndex();
-    auto * provider = GetGroupDataProvider();
-    Status status   = Status::Failure;
+    Credentials::GroupDataProvider * provider = nullptr;
+    const FabricInfo * fabric                 = nullptr;
 
-    if (nullptr != provider)
+    // Flight-check fabric scoping.
+    if (!GetProviderAndFabric(commandObj, commandPath, &provider, &fabric))
     {
-        // Remove keyset
-        CHIP_ERROR err = provider->RemoveKeySet(fabric, commandData.groupKeySetID);
-        if (CHIP_ERROR_KEY_NOT_FOUND == err)
-        {
-            status = Status::NotFound;
-        }
-        else if (CHIP_NO_ERROR == err)
-        {
-            status = Status::Success;
-        }
+        // Command will already have status populated from validation.
+        return true;
     }
 
-    // Send response
-    CHIP_ERROR send_err = commandObj->AddStatus(commandPath, status);
-    if (CHIP_NO_ERROR != send_err)
+    if (commandData.groupKeySetID == GroupDataProvider::kIdentityProtectionKeySetId)
+    {
+        // SPEC: This command SHALL fail with an INVALID_COMMAND status code back to the initiator if the GroupKeySetID being
+        // removed is 0, which is the Key Set associated with the Identity Protection Key (IPK).
+        commandObj->AddStatusAndLogIfFailure(commandPath, Status::InvalidCommand,
+                                             "Attempted to KeySetRemove the identity protection key!");
+        return true;
+    }
+
+    // Remove keyset
+    FabricIndex fabricIndex = fabric->GetFabricIndex();
+    CHIP_ERROR err          = provider->RemoveKeySet(fabricIndex, commandData.groupKeySetID);
+
+    Status status = Status::Success;
+    if (CHIP_ERROR_KEY_NOT_FOUND == err)
+    {
+        status = Status::NotFound;
+    }
+    else if (CHIP_NO_ERROR != err)
     {
-        ChipLogDetail(Zcl, "GroupKeyManagementCluster: KeySetRemove failed: %" CHIP_ERROR_FORMAT, send_err.Format());
+        status = Status::Failure;
     }
+
+    // Send status response.
+    commandObj->AddStatusAndLogIfFailure(commandPath, status, "KeySetRemove failed");
     return true;
 }
 
@@ -654,19 +691,21 @@ bool emberAfGroupKeyManagementClusterKeySetReadAllIndicesCallback(
     chip::app::CommandHandler * commandObj, const chip::app::ConcreteCommandPath & commandPath,
     const chip::app::Clusters::GroupKeyManagement::Commands::KeySetReadAllIndices::DecodableType & commandData)
 {
-    auto fabric     = commandObj->GetAccessingFabricIndex();
-    auto * provider = GetGroupDataProvider();
+    Credentials::GroupDataProvider * provider = nullptr;
+    const FabricInfo * fabric                 = nullptr;
 
-    if (nullptr == provider)
+    // Flight-check fabric scoping.
+    if (!GetProviderAndFabric(commandObj, commandPath, &provider, &fabric))
     {
-        commandObj->AddStatus(commandPath, Status::Failure);
+        // Command will already have status populated from validation.
         return true;
     }
 
-    auto keysIt = provider->IterateKeySets(fabric);
+    FabricIndex fabricIndex = fabric->GetFabricIndex();
+    auto keysIt             = provider->IterateKeySets(fabricIndex);
     if (nullptr == keysIt)
     {
-        commandObj->AddStatus(commandPath, Status::Failure);
+        commandObj->AddStatusAndLogIfFailure(commandPath, Status::Failure, "Failed iteration of key set indices!");
         return true;
     }
 

diff --git a/src/app/tests/suites/TestGroupKeyManagementCluster.yaml b/src/app/tests/suites/TestGroupKeyManagementCluster.yaml
@@ -556,6 +556,15 @@ tests:
       response:
           error: RESOURCE_EXHAUSTED
 
+    - label: "Try to remove KeySet index 0 should fail"
+      command: "KeySetRemove"
+      arguments:
+          values:
+              - name: "GroupKeySetID"
+                value: 0
+      response:
+          error: INVALID_COMMAND
+
     - label: "Write Group Keys (invalid)"
       command: "writeAttribute"
       attribute: "GroupKeyMap"

diff --git a/src/credentials/FabricTable.cpp b/src/credentials/FabricTable.cpp
@@ -510,6 +510,11 @@ FabricInfo * FabricTable::GetMutableFabricByIndex(FabricIndex fabricIndex)
 
 const FabricInfo * FabricTable::FindFabricWithIndex(FabricIndex fabricIndex) const
 {
+    if (fabricIndex == kUndefinedFabricIndex)
+    {
+        return nullptr;
+    }
+
     // Try to match pending fabric first if available
     if (HasPendingFabricUpdate() && (mPendingFabric.GetFabricIndex() == fabricIndex))
     {

diff --git a/src/credentials/tests/TestFabricTable.cpp b/src/credentials/tests/TestFabricTable.cpp
@@ -2391,6 +2391,11 @@ void TestFabricLookup(nlTestSuite * inSuite, void * inContext)
         NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricIndex() == 2);
         NL_TEST_ASSERT(inSuite, !fabricInfo->ShouldAdvertiseIdentity());
     }
+
+    // Attempt lookup of FabricIndex 0 --> should always fail.
+    {
+        NL_TEST_ASSERT(inSuite, fabricTable.FindFabricWithIndex(0) == nullptr);
+    }
 }
 
 void TestFetchCATs(nlTestSuite * inSuite, void * inContext)