From 75cd04ea1a37e6006ad34589d067ce17336a0e12 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Tue, 26 Sep 2023 14:46:02 -0400 Subject: [PATCH] Check for valid ids in command and write paths. Read/subscribe already had such checks (in AttributePathIB::Parser::ParsePath), but commands and writes were missing them. --- src/app/CommandHandler.cpp | 13 ++----------- src/app/MessageDef/AttributePathIB.cpp | 3 +++ src/app/MessageDef/CommandPathIB.cpp | 20 ++++++++++++++++++++ src/app/MessageDef/CommandPathIB.h | 24 ++++++++++++++++++++++++ src/lib/core/DataModelTypes.h | 21 +++++++++++++++++++-- 5 files changed, 68 insertions(+), 13 deletions(-) diff --git a/src/app/CommandHandler.cpp b/src/app/CommandHandler.cpp index cd67ffd59e436f..a447597cc97cea 100644 --- a/src/app/CommandHandler.cpp +++ b/src/app/CommandHandler.cpp @@ -256,13 +256,7 @@ Status CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommandElem err = aCommandElement.GetPath(&commandPath); VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction); - err = commandPath.GetClusterId(&concretePath.mClusterId); - VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction); - - err = commandPath.GetCommandId(&concretePath.mCommandId); - VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction); - - err = commandPath.GetEndpointId(&concretePath.mEndpointId); + err = commandPath.GetConcreteCommandPath(concretePath); VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction); { @@ -362,10 +356,7 @@ Status CommandHandler::ProcessGroupCommandDataIB(CommandDataIB::Parser & aComman err = aCommandElement.GetPath(&commandPath); VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction); - err = commandPath.GetClusterId(&clusterId); - VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction); - - err = commandPath.GetCommandId(&commandId); + err = commandPath.GetGroupCommandPath(&clusterId, &commandId); VerifyOrReturnError(err == CHIP_NO_ERROR, Status::InvalidAction); groupId = mExchangeCtx->GetSessionHandle()->AsIncomingGroupSession()->GetGroupId(); diff --git a/src/app/MessageDef/AttributePathIB.cpp b/src/app/MessageDef/AttributePathIB.cpp index 12aec2a242da94..635d9122ecc303 100644 --- a/src/app/MessageDef/AttributePathIB.cpp +++ b/src/app/MessageDef/AttributePathIB.cpp @@ -174,7 +174,10 @@ CHIP_ERROR AttributePathIB::Parser::GetListIndex(DataModel::Nullable CHIP_ERROR AttributePathIB::Parser::GetGroupAttributePath(ConcreteDataAttributePath & aAttributePath) const { ReturnErrorOnFailure(GetCluster(&aAttributePath.mClusterId)); + VerifyOrReturnError(IsValidClusterId(aAttributePath.mClusterId), CHIP_IM_GLOBAL_STATUS(InvalidAction)); + ReturnErrorOnFailure(GetAttribute(&aAttributePath.mAttributeId)); + VerifyOrReturnError(IsValidAttributeId(aAttributePath.mAttributeId), CHIP_IM_GLOBAL_STATUS(InvalidAction)); CHIP_ERROR err = CHIP_NO_ERROR; DataModel::Nullable listIndex; diff --git a/src/app/MessageDef/CommandPathIB.cpp b/src/app/MessageDef/CommandPathIB.cpp index d4ca7b89b397a5..2ae710ea54dffe 100644 --- a/src/app/MessageDef/CommandPathIB.cpp +++ b/src/app/MessageDef/CommandPathIB.cpp @@ -26,6 +26,8 @@ #include +#include + using namespace chip; using namespace chip::TLV; @@ -114,6 +116,24 @@ CHIP_ERROR CommandPathIB::Parser::GetCommandId(chip::CommandId * const apCommand return GetUnsignedInteger(to_underlying(Tag::kCommandId), apCommandId); } +CHIP_ERROR CommandPathIB::Parser::GetConcreteCommandPath(ConcreteCommandPath & aCommandPath) const +{ + ReturnErrorOnFailure(GetGroupCommandPath(&aCommandPath.mClusterId, &aCommandPath.mCommandId)); + + return GetEndpointId(&aCommandPath.mEndpointId); +} + +CHIP_ERROR CommandPathIB::Parser::GetGroupCommandPath(ClusterId * apClusterId, CommandId * apCommandId) const +{ + ReturnErrorOnFailure(GetClusterId(apClusterId)); + VerifyOrReturnError(IsValidClusterId(*apClusterId), CHIP_IM_GLOBAL_STATUS(InvalidAction)); + + ReturnErrorOnFailure(GetCommandId(apCommandId)); + VerifyOrReturnError(IsValidCommandId(*apCommandId), CHIP_IM_GLOBAL_STATUS(InvalidAction)); + + return CHIP_NO_ERROR; +} + CommandPathIB::Builder & CommandPathIB::Builder::EndpointId(const chip::EndpointId aEndpointId) { // skip if error has already been set diff --git a/src/app/MessageDef/CommandPathIB.h b/src/app/MessageDef/CommandPathIB.h index 5362b65eefc7e6..21aa1e521d2184 100644 --- a/src/app/MessageDef/CommandPathIB.h +++ b/src/app/MessageDef/CommandPathIB.h @@ -79,6 +79,30 @@ class Parser : public ListParser * #CHIP_END_OF_TLV if there is no such element */ CHIP_ERROR GetCommandId(chip::CommandId * const apCommandId) const; + + /** + * @brief Get the concrete command path, if this command path is a concrete + * path. + * + * This will validate that the cluster id and command id are actually valid for a + * concrete path. + * + * @param [in] aCommandPath The command path object to write to. + */ + CHIP_ERROR GetConcreteCommandPath(ConcreteCommandPath & aCommandPath) const; + + /** + * @brief Get a group command path. + * + * This will validate that the cluster id and command id are actually valid for a + * group path. + * + * @param [out] apClusterId The cluster id in the path. + * @param [out] apCommandId The command id in the path. + * + * @return #CHIP_NO_ERROR on success + */ + CHIP_ERROR GetGroupCommandPath(ClusterId * apClusterId, CommandId * apCommandId) const; }; class Builder : public ListBuilder diff --git a/src/lib/core/DataModelTypes.h b/src/lib/core/DataModelTypes.h index dfbc5ef16ad400..1d84c00e114afc 100644 --- a/src/lib/core/DataModelTypes.h +++ b/src/lib/core/DataModelTypes.h @@ -73,6 +73,8 @@ static constexpr CommandId kInvalidCommandId = 0xFFFF'FFFF; static constexpr EventId kInvalidEventId = 0xFFFF'FFFF; static constexpr FieldId kInvalidFieldId = 0xFFFF'FFFF; +static constexpr uint16_t kMaxVendorId = VendorId::TestVendor4; + static constexpr uint16_t ExtractIdFromMEI(uint32_t aMEI) { constexpr uint32_t kIdMask = 0x0000'FFFF; @@ -86,6 +88,11 @@ static constexpr uint16_t ExtractVendorFromMEI(uint32_t aMEI) return static_cast((aMEI & kVendorMask) >> kVendorShift); } +static constexpr bool IsValidVendorId(uint16_t aVendorId) +{ + return aVendorId <= kMaxVendorId; +} + constexpr bool IsValidClusterId(ClusterId aClusterId) { const auto id = ExtractIdFromMEI(aClusterId); @@ -94,7 +101,7 @@ constexpr bool IsValidClusterId(ClusterId aClusterId) // cluster. // // Cluster id suffixes in the range 0xFC00 to 0xFFFE indicate an MS cluster. - return (vendor == 0x0000 && id <= 0x7FFF) || (vendor >= 0x0001 && vendor <= 0xFFFE && id >= 0xFC00 && id <= 0xFFFE); + return IsValidVendorId(vendor) && ((vendor == 0x0000 && id <= 0x7FFF) || (vendor >= 0x0001 && id >= 0xFC00 && id <= 0xFFFE)); } constexpr bool IsGlobalAttribute(AttributeId aAttributeId) @@ -113,7 +120,17 @@ constexpr bool IsValidAttributeId(AttributeId aAttributeId) // Attribute id suffixes in the range 0x0000 to 0x4FFF indicate a non-global // attribute (standard or MS). The vendor id must not be wildcard in this // case. - return (id <= 0x4FFF && vendor != 0xFFFF) || IsGlobalAttribute(aAttributeId); + return (id <= 0x4FFF && IsValidVendorId(vendor)) || IsGlobalAttribute(aAttributeId); +} + +constexpr bool IsValidCommandId(CommandId aCommandId) +{ + const auto id = ExtractIdFromMEI(aCommandId); + const auto vendor = ExtractVendorFromMEI(aCommandId); + + // Command id suffixes must be in the range 0x00 to 0xFF, and the prefix + // must be a valid prefix (standard or MC). + return id <= 0xFF && IsValidVendorId(vendor); } constexpr bool IsValidDeviceTypeId(DeviceTypeId aDeviceTypeId)