From e84b2751c72b20a0d9d157392212949be54cff1d Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Thu, 21 Apr 2022 11:59:18 -0400 Subject: [PATCH] Ensure that read attribute paths are valid paths. Disallow out-of-range attribute ids and wildcard cluster with non-wildcard non-global attribute. Fixes https://github.com/project-chip/connectedhomeip/issues/14026 --- src/app/InteractionModelEngine.cpp | 7 ++- src/app/MessageDef/StatusIB.cpp | 2 +- src/app/MessageDef/StatusResponseMessage.cpp | 7 ++- src/app/ReadHandler.cpp | 29 ++++++++----- src/lib/core/DataModelTypes.h | 45 +++++++++++++++++--- 5 files changed, 70 insertions(+), 20 deletions(-) diff --git a/src/app/InteractionModelEngine.cpp b/src/app/InteractionModelEngine.cpp index 45fda184d0c79d..2a8e7dd50c05f7 100644 --- a/src/app/InteractionModelEngine.cpp +++ b/src/app/InteractionModelEngine.cpp @@ -332,7 +332,12 @@ CHIP_ERROR InteractionModelEngine::OnReadInitialRequest(Messaging::ExchangeConte ReadHandler * handler = mReadHandlers.CreateObject(*this, apExchangeContext, aInteractionType); if (handler) { - ReturnErrorOnFailure(handler->OnInitialRequest(std::move(aPayload))); + CHIP_ERROR err = handler->OnInitialRequest(std::move(aPayload)); + if (err != CHIP_NO_ERROR) + { + aStatus = StatusIB(err).mStatus; + return err; + } aStatus = Protocols::InteractionModel::Status::Success; return CHIP_NO_ERROR; diff --git a/src/app/MessageDef/StatusIB.cpp b/src/app/MessageDef/StatusIB.cpp index 3e956f9246b36e..37fbca49cab78b 100644 --- a/src/app/MessageDef/StatusIB.cpp +++ b/src/app/MessageDef/StatusIB.cpp @@ -82,7 +82,7 @@ CHIP_ERROR StatusIB::Parser::CheckSchemaValidity() const #if CHIP_DETAIL_LOGGING { - uint16_t status; + uint8_t status; ReturnErrorOnFailure(reader.Get(status)); PRETTY_PRINT("\tstatus = " ChipLogFormatIMStatus ",", ChipLogValueIMStatus(static_cast(status))); } diff --git a/src/app/MessageDef/StatusResponseMessage.cpp b/src/app/MessageDef/StatusResponseMessage.cpp index e68d727dd247c0..d0a605e1c47d98 100644 --- a/src/app/MessageDef/StatusResponseMessage.cpp +++ b/src/app/MessageDef/StatusResponseMessage.cpp @@ -16,6 +16,9 @@ #include "StatusResponseMessage.h" #include "MessageDefHelper.h" +#include "protocols/interaction_model/Constants.h" + +using namespace chip::Protocols::InteractionModel; namespace chip { namespace app { @@ -42,9 +45,9 @@ CHIP_ERROR StatusResponseMessage::Parser::CheckSchemaValidity() const VerifyOrReturnError(TLV::kTLVType_UnsignedInteger == reader.GetType(), CHIP_ERROR_WRONG_TLV_TYPE); #if CHIP_DETAIL_LOGGING { - uint16_t status; + uint8_t status; ReturnErrorOnFailure(reader.Get(status)); - PRETTY_PRINT("\tStatus = 0x%x,", status); + PRETTY_PRINT("\tStatus = " ChipLogFormatIMStatus ",", ChipLogValueIMStatus(static_cast(status))); } #endif // CHIP_DETAIL_LOGGING break; diff --git a/src/app/ReadHandler.cpp b/src/app/ReadHandler.cpp index f409fc0aceb7d6..19fa55dfb78260 100644 --- a/src/app/ReadHandler.cpp +++ b/src/app/ReadHandler.cpp @@ -400,46 +400,55 @@ CHIP_ERROR ReadHandler::ProcessAttributePathList(AttributePathIBs::Parser & aAtt AttributePathIB::Parser path; err = path.Init(reader); SuccessOrExit(err); - // TODO: MEIs (ClusterId and AttributeId) have a invalid pattern instead of a single invalid value, need to add separate - // functions for checking if we have received valid values. - // TODO: Wildcard cluster id with non-global attributes or wildcard attribute paths should be rejected. + err = path.GetEndpoint(&(attribute.mEndpointId)); if (err == CHIP_NO_ERROR) { - VerifyOrExit(!attribute.HasWildcardEndpointId(), err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH); + VerifyOrExit(!attribute.HasWildcardEndpointId(), err = CHIP_IM_GLOBAL_STATUS(InvalidAction)); } else if (err == CHIP_END_OF_TLV) { err = CHIP_NO_ERROR; } SuccessOrExit(err); - err = path.GetCluster(&(attribute.mClusterId)); + + ClusterId clusterId = kInvalidClusterId; + err = path.GetCluster(&clusterId); if (err == CHIP_NO_ERROR) { - VerifyOrExit(!attribute.HasWildcardClusterId(), err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH); + VerifyOrExit(IsValidClusterId(clusterId), err = CHIP_IM_GLOBAL_STATUS(InvalidAction)); + attribute.mClusterId = clusterId; } else if (err == CHIP_END_OF_TLV) { err = CHIP_NO_ERROR; } - SuccessOrExit(err); - err = path.GetAttribute(&(attribute.mAttributeId)); + + AttributeId attributeId = kInvalidAttributeId; + err = path.GetAttribute(&attributeId); if (CHIP_END_OF_TLV == err) { err = CHIP_NO_ERROR; } else if (err == CHIP_NO_ERROR) { - VerifyOrExit(!attribute.HasWildcardAttributeId(), err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH); + VerifyOrExit(IsValidAttributeId(attributeId), err = CHIP_IM_GLOBAL_STATUS(InvalidAction)); + attribute.mAttributeId = attributeId; } SuccessOrExit(err); + // A wildcard cluster requires that the attribute path either be + // wildcard or a global attribute. + VerifyOrExit(!attribute.HasWildcardClusterId() || attribute.HasWildcardAttributeId() || + IsGlobalAttribute(attribute.mAttributeId), + err = CHIP_IM_GLOBAL_STATUS(InvalidAction)); + err = path.GetListIndex(&(attribute.mListIndex)); if (CHIP_NO_ERROR == err) { VerifyOrExit(!attribute.HasWildcardAttributeId() && !attribute.HasWildcardListIndex(), - err = CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH); + err = CHIP_IM_GLOBAL_STATUS(InvalidAction)); } else if (CHIP_END_OF_TLV == err) { diff --git a/src/lib/core/DataModelTypes.h b/src/lib/core/DataModelTypes.h index 3dccb05932e64a..a2aca450784c4f 100644 --- a/src/lib/core/DataModelTypes.h +++ b/src/lib/core/DataModelTypes.h @@ -57,14 +57,47 @@ static constexpr CommandId kInvalidCommandId = 0xFFFF'FFFF; static constexpr EventId kInvalidEventId = 0xFFFF'FFFF; static constexpr FieldId kInvalidFieldId = 0xFFFF'FFFF; +static constexpr uint16_t ExtractIdFromMEI(uint32_t aMEI) +{ + constexpr uint32_t kIdMask = 0x0000'FFFF; + return static_cast(aMEI & kIdMask); +} + +static constexpr uint16_t ExtractVendorFromMEI(uint32_t aMEI) +{ + constexpr uint32_t kVendorMask = 0xFFFF'0000; + constexpr uint32_t kVendorShift = 16; + return static_cast((aMEI & kVendorMask) >> kVendorShift); +} + constexpr bool IsValidClusterId(ClusterId aClusterId) { - const ClusterId kIdMask = 0x0000'FFFF; - const ClusterId kVendorMask = 0xFFFF'0000; - const auto id = aClusterId & kIdMask; - const auto vendor = aClusterId & kVendorMask; - return (vendor == 0x0000'0000 && id <= 0x7FFF) || - (vendor >= 0x0001'0000 && vendor <= 0xFFFE'0000 && id >= 0xFC00 && id <= 0xFFFE); + const auto id = ExtractIdFromMEI(aClusterId); + const auto vendor = ExtractVendorFromMEI(aClusterId); + // Cluster id suffixes in the range 0x0000 to 0x7FFF indicate a standard + // 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); +} + +constexpr bool IsGlobalAttribute(AttributeId aAttributeId) +{ + const auto id = ExtractIdFromMEI(aAttributeId); + const auto vendor = ExtractVendorFromMEI(aAttributeId); + // Attribute id suffixes in the range 0xF000 to 0xFFFE indicate a standard + // global attribute. + return (vendor == 0x0000 && id >= 0xF000 && id <= 0xFFFE); +} + +constexpr bool IsValidAttributeId(AttributeId aAttributeId) +{ + const auto id = ExtractIdFromMEI(aAttributeId); + const auto vendor = ExtractVendorFromMEI(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); } constexpr bool IsValidDeviceTypeId(DeviceTypeId aDeviceTypeId)