From faac89a485d6a95b766afc10dd8e439cff584283 Mon Sep 17 00:00:00 2001 From: Tennessee Carmel-Veilleux Date: Thu, 10 Aug 2023 09:38:37 -0400 Subject: [PATCH] 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: tennessee.carmelveilleux@gmail.com Co-authored-by: Restyled.io Co-authored-by: Andrei Litvin Co-authored-by: Andrei Litvin --- src/app/CommandHandler.cpp | 8 +- src/app/CommandHandler.h | 7 +- .../group-key-mgmt-server.cpp | 119 ++++++++++++------ .../suites/TestGroupKeyManagementCluster.yaml | 9 ++ src/credentials/FabricTable.cpp | 5 + src/credentials/tests/TestFabricTable.cpp | 5 + 6 files changed, 107 insertions(+), 46 deletions(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index 14ad098965f5e5..df58e38c5129b1 100644 --- 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 index e2d2f4a6f75e1e..bc96583abd7597 100644 --- 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 index 9ff43d9a2ef834..51a9977977a042 100644 --- 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,11 +544,7 @@ 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; } @@ -528,20 +552,22 @@ 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 index 11d190c78738d6..82e10bc2120374 100644 --- 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 index 8ec4691f9f80fa..f9b7503a7bad9e 100644 --- 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 index 96ed242a684310..0ed6db832f3b17 100644 --- 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)