From 0d16348fa32cee91e94fc520fcf3ebf042b48a22 Mon Sep 17 00:00:00 2001 From: Boris Zbarsky Date: Mon, 20 Nov 2023 12:22:01 -0500 Subject: [PATCH] Make client handling of invalid IDs a bit more lenient. (#30452) Before this change, if a server happened to send an attribute report with an invalid cluster or attribute id (very common for vendor-prefixed things that people set up incorrectly), the entire read or subscription would fail and no reports would be processed after the invalid id. This causes wildcard subscriptions to completely fail if the device happens to have an invalid path configured somewhere. The new behavior is to completely skip that one AttributeReportIB and process the other ones in the list. Co-authored-by: Andrei Litvin --- src/app/ConcreteAttributePath.h | 2 + src/app/ConcreteClusterPath.h | 2 + src/app/MessageDef/AttributePathIB.cpp | 17 ++++--- src/app/MessageDef/AttributePathIB.h | 16 ++++++- src/app/ReadClient.cpp | 27 ++++++++++- src/app/tests/TestReadInteraction.cpp | 65 ++++++++++++++++++++++---- 6 files changed, 111 insertions(+), 18 deletions(-) diff --git a/src/app/ConcreteAttributePath.h b/src/app/ConcreteAttributePath.h index 312311f848a900..b06a4b8a3b3acf 100644 --- a/src/app/ConcreteAttributePath.h +++ b/src/app/ConcreteAttributePath.h @@ -48,6 +48,8 @@ struct ConcreteAttributePath : public ConcreteClusterPath mExpanded = false; } + bool IsValid() const { return ConcreteClusterPath::HasValidIds() && IsValidAttributeId(mAttributeId); } + bool operator==(const ConcreteAttributePath & aOther) const { return ConcreteClusterPath::operator==(aOther) && (mAttributeId == aOther.mAttributeId); diff --git a/src/app/ConcreteClusterPath.h b/src/app/ConcreteClusterPath.h index 6865fdb29a14c6..58b2f5b477f139 100644 --- a/src/app/ConcreteClusterPath.h +++ b/src/app/ConcreteClusterPath.h @@ -38,6 +38,8 @@ struct ConcreteClusterPath bool IsValidConcreteClusterPath() const { return !(mEndpointId == kInvalidEndpointId || mClusterId == kInvalidClusterId); } + bool HasValidIds() const { return IsValidEndpointId(mEndpointId) && IsValidClusterId(mClusterId); } + bool operator==(const ConcreteClusterPath & aOther) const { return mEndpointId == aOther.mEndpointId && mClusterId == aOther.mClusterId; diff --git a/src/app/MessageDef/AttributePathIB.cpp b/src/app/MessageDef/AttributePathIB.cpp index db9e4cdfe241ab..52e7c6b78660ec 100644 --- a/src/app/MessageDef/AttributePathIB.cpp +++ b/src/app/MessageDef/AttributePathIB.cpp @@ -171,13 +171,17 @@ CHIP_ERROR AttributePathIB::Parser::GetListIndex(DataModel::Nullable return GetNullableUnsignedInteger(to_underlying(Tag::kListIndex), apListIndex); } -CHIP_ERROR AttributePathIB::Parser::GetGroupAttributePath(ConcreteDataAttributePath & aAttributePath) const +CHIP_ERROR AttributePathIB::Parser::GetGroupAttributePath(ConcreteDataAttributePath & aAttributePath, + ValidateIdRanges aValidateRanges) 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)); + + if (aValidateRanges == ValidateIdRanges::kYes) + { + VerifyOrReturnError(IsValidClusterId(aAttributePath.mClusterId), CHIP_IM_GLOBAL_STATUS(InvalidAction)); + VerifyOrReturnError(IsValidAttributeId(aAttributePath.mAttributeId), CHIP_IM_GLOBAL_STATUS(InvalidAction)); + } CHIP_ERROR err = CHIP_NO_ERROR; DataModel::Nullable listIndex; @@ -204,9 +208,10 @@ CHIP_ERROR AttributePathIB::Parser::GetGroupAttributePath(ConcreteDataAttributeP return err; } -CHIP_ERROR AttributePathIB::Parser::GetConcreteAttributePath(ConcreteDataAttributePath & aAttributePath) const +CHIP_ERROR AttributePathIB::Parser::GetConcreteAttributePath(ConcreteDataAttributePath & aAttributePath, + ValidateIdRanges aValidateRanges) const { - ReturnErrorOnFailure(GetGroupAttributePath(aAttributePath)); + ReturnErrorOnFailure(GetGroupAttributePath(aAttributePath, aValidateRanges)); // And now read our endpoint. return GetEndpoint(&aAttributePath.mEndpointId); diff --git a/src/app/MessageDef/AttributePathIB.h b/src/app/MessageDef/AttributePathIB.h index 5035bbd46018c8..e1740d305d5578 100644 --- a/src/app/MessageDef/AttributePathIB.h +++ b/src/app/MessageDef/AttributePathIB.h @@ -45,6 +45,12 @@ enum class Tag : uint8_t kListIndex = 5, }; +enum class ValidateIdRanges : uint8_t +{ + kYes, + kNo, +}; + class Parser : public ListParser { public: @@ -136,10 +142,13 @@ class Parser : public ListParser * as ReplaceAll if that's appropriate to their context. * * @param [in] aAttributePath The attribute path object to write to. + * @param [in] aValidateRanges Whether to validate that the cluster/attribute + * IDs in the path are in the right ranges. * * @return #CHIP_NO_ERROR on success */ - CHIP_ERROR GetConcreteAttributePath(ConcreteDataAttributePath & aAttributePath) const; + CHIP_ERROR GetConcreteAttributePath(ConcreteDataAttributePath & aAttributePath, + ValidateIdRanges aValidateRanges = ValidateIdRanges::kYes) const; /** * @brief Get a group attribute path. This will set the ListOp to @@ -148,10 +157,13 @@ class Parser : public ListParser * endpoint id of the resulting path might have any value. * * @param [in] aAttributePath The attribute path object to write to. + * @param [in] aValidateRanges Whether to validate that the cluster/attribute + * IDs in the path are in the right ranges. * * @return #CHIP_NO_ERROR on success */ - CHIP_ERROR GetGroupAttributePath(ConcreteDataAttributePath & aAttributePath) const; + CHIP_ERROR GetGroupAttributePath(ConcreteDataAttributePath & aAttributePath, + ValidateIdRanges aValidateRanges = ValidateIdRanges::kYes) const; // TODO(#14934) Add a function to get ConcreteDataAttributePath from AttributePathIB::Parser directly. diff --git a/src/app/ReadClient.cpp b/src/app/ReadClient.cpp index 373d2051d3457f..bbc1c68a4b87b8 100644 --- a/src/app/ReadClient.cpp +++ b/src/app/ReadClient.cpp @@ -641,9 +641,12 @@ void ReadClient::OnResponseTimeout(Messaging::ExchangeContext * apExchangeContex CHIP_ERROR ReadClient::ProcessAttributePath(AttributePathIB::Parser & aAttributePathParser, ConcreteDataAttributePath & aAttributePath) { + // The ReportData must contain a concrete attribute path. Don't validate ID + // ranges here, so we can tell apart "malformed data" and "out of range + // IDs". CHIP_ERROR err = CHIP_NO_ERROR; // The ReportData must contain a concrete attribute path - err = aAttributePathParser.GetConcreteAttributePath(aAttributePath); + err = aAttributePathParser.GetConcreteAttributePath(aAttributePath, AttributePathIB::ValidateIdRanges::kNo); VerifyOrReturnError(err == CHIP_NO_ERROR, CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH_IB); return CHIP_NO_ERROR; } @@ -679,6 +682,17 @@ CHIP_ERROR ReadClient::ProcessAttributeReportIBs(TLV::TLVReader & aAttributeRepo StatusIB::Parser errorStatus; ReturnErrorOnFailure(status.GetPath(&path)); ReturnErrorOnFailure(ProcessAttributePath(path, attributePath)); + if (!attributePath.IsValid()) + { + // Don't fail the entire read or subscription when there is an + // out-of-range ID. Just skip that one AttributeReportIB. + ChipLogError(DataManagement, + "Skipping AttributeStatusIB with out-of-range IDs: (%d, " ChipLogFormatMEI ", " ChipLogFormatMEI ") ", + attributePath.mEndpointId, ChipLogValueMEI(attributePath.mClusterId), + ChipLogValueMEI(attributePath.mAttributeId)); + continue; + } + ReturnErrorOnFailure(status.GetErrorStatus(&errorStatus)); ReturnErrorOnFailure(errorStatus.DecodeStatusIB(statusIB)); NoteReportingData(); @@ -689,6 +703,17 @@ CHIP_ERROR ReadClient::ProcessAttributeReportIBs(TLV::TLVReader & aAttributeRepo ReturnErrorOnFailure(report.GetAttributeData(&data)); ReturnErrorOnFailure(data.GetPath(&path)); ReturnErrorOnFailure(ProcessAttributePath(path, attributePath)); + if (!attributePath.IsValid()) + { + // Don't fail the entire read or subscription when there is an + // out-of-range ID. Just skip that one AttributeReportIB. + ChipLogError(DataManagement, + "Skipping AttributeDataIB with out-of-range IDs: (%d, " ChipLogFormatMEI ", " ChipLogFormatMEI ") ", + attributePath.mEndpointId, ChipLogValueMEI(attributePath.mClusterId), + ChipLogValueMEI(attributePath.mAttributeId)); + continue; + } + DataVersion version = 0; ReturnErrorOnFailure(data.GetDataVersion(&version)); attributePath.mDataVersion.SetValue(version); diff --git a/src/app/tests/TestReadInteraction.cpp b/src/app/tests/TestReadInteraction.cpp index 25f26119b05c8a..dd163bcf210aa1 100644 --- a/src/app/tests/TestReadInteraction.cpp +++ b/src/app/tests/TestReadInteraction.cpp @@ -333,6 +333,7 @@ class TestReadInteraction static void TestReadClientGenerateOneEventPaths(nlTestSuite * apSuite, void * apContext); static void TestReadClientGenerateTwoEventPaths(nlTestSuite * apSuite, void * apContext); static void TestReadClientInvalidReport(nlTestSuite * apSuite, void * apContext); + static void TestReadClientInvalidAttributeId(nlTestSuite * apSuite, void * apContext); static void TestReadHandlerInvalidAttributePath(nlTestSuite * apSuite, void * apContext); static void TestProcessSubscribeRequest(nlTestSuite * apSuite, void * apContext); #if CHIP_CONFIG_ENABLE_ICD_SERVER @@ -390,12 +391,19 @@ class TestReadInteraction static void TestReadHandlerMalformedSubscribeRequest(nlTestSuite * apSuite, void * apContext); private: + enum class ReportType : uint8_t + { + kValid, + kInvalidNoAttributeId, + kInvalidOutOfRangeAttributeId, + }; + static void GenerateReportData(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload, - bool aNeedInvalidReport, bool aSuppressResponse, bool aHasSubscriptionId); + ReportType aReportType, bool aSuppressResponse, bool aHasSubscriptionId); }; void TestReadInteraction::GenerateReportData(nlTestSuite * apSuite, void * apContext, System::PacketBufferHandle & aPayload, - bool aNeedInvalidReport, bool aSuppressResponse, bool aHasSubscriptionId = false) + ReportType aReportType, bool aSuppressResponse, bool aHasSubscriptionId = false) { CHIP_ERROR err = CHIP_NO_ERROR; System::PacketBufferTLVWriter writer; @@ -428,12 +436,17 @@ void TestReadInteraction::GenerateReportData(nlTestSuite * apSuite, void * apCon AttributePathIB::Builder & attributePathBuilder = attributeDataIBBuilder.CreatePath(); NL_TEST_ASSERT(apSuite, attributeDataIBBuilder.GetError() == CHIP_NO_ERROR); - if (aNeedInvalidReport) + if (aReportType == ReportType::kInvalidNoAttributeId) { attributePathBuilder.Node(1).Endpoint(2).Cluster(3).ListIndex(5).EndOfAttributePathIB(); } + else if (aReportType == ReportType::kInvalidOutOfRangeAttributeId) + { + attributePathBuilder.Node(1).Endpoint(2).Cluster(3).Attribute(0xFFF18000).EndOfAttributePathIB(); + } else { + NL_TEST_ASSERT(apSuite, aReportType == ReportType::kValid); attributePathBuilder.Node(1).Endpoint(2).Cluster(3).Attribute(4).EndOfAttributePathIB(); } @@ -496,7 +509,7 @@ void TestReadInteraction::TestReadClient(nlTestSuite * apSuite, void * apContext ctx.GetLoopback().mNumMessagesToDrop = 1; ctx.DrainAndServiceIO(); - GenerateReportData(apSuite, apContext, buf, false /*aNeedInvalidReport*/, true /* aSuppressResponse*/); + GenerateReportData(apSuite, apContext, buf, ReportType::kValid, true /* aSuppressResponse*/); err = readClient.ProcessReportData(std::move(buf), ReadClient::ReportType::kContinuingTransaction); NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); } @@ -521,8 +534,7 @@ void TestReadInteraction::TestReadUnexpectedSubscriptionId(nlTestSuite * apSuite ctx.DrainAndServiceIO(); // For read, we don't expect there is subscription id in report data. - GenerateReportData(apSuite, apContext, buf, false /*aNeedInvalidReport*/, true /* aSuppressResponse*/, - true /*aHasSubscriptionId*/); + GenerateReportData(apSuite, apContext, buf, ReportType::kValid, true /* aSuppressResponse*/, true /*aHasSubscriptionId*/); err = readClient.ProcessReportData(std::move(buf), ReadClient::ReportType::kContinuingTransaction); NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_INVALID_ARGUMENT); } @@ -547,7 +559,7 @@ void TestReadInteraction::TestReadHandler(nlTestSuite * apSuite, void * apContex ReadHandler readHandler(nullCallback, exchangeCtx, chip::app::ReadHandler::InteractionType::Read, app::reporting::GetDefaultReportScheduler()); - GenerateReportData(apSuite, apContext, reportDatabuf, false /*aNeedInvalidReport*/, false /* aSuppressResponse*/); + GenerateReportData(apSuite, apContext, reportDatabuf, ReportType::kValid, false /* aSuppressResponse*/); err = readHandler.SendReportData(std::move(reportDatabuf), false); NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_INCORRECT_STATE); @@ -665,12 +677,46 @@ void TestReadInteraction::TestReadClientInvalidReport(nlTestSuite * apSuite, voi ctx.GetLoopback().mNumMessagesToDrop = 1; ctx.DrainAndServiceIO(); - GenerateReportData(apSuite, apContext, buf, true /*aNeedInvalidReport*/, true /* aSuppressResponse*/); + GenerateReportData(apSuite, apContext, buf, ReportType::kInvalidNoAttributeId, true /* aSuppressResponse*/); err = readClient.ProcessReportData(std::move(buf), ReadClient::ReportType::kContinuingTransaction); NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_IM_MALFORMED_ATTRIBUTE_PATH_IB); } +void TestReadInteraction::TestReadClientInvalidAttributeId(nlTestSuite * apSuite, void * apContext) +{ + CHIP_ERROR err = CHIP_NO_ERROR; + TestContext & ctx = *static_cast(apContext); + MockInteractionModelApp delegate; + + System::PacketBufferHandle buf = System::PacketBufferHandle::New(System::PacketBuffer::kMaxSize); + + app::ReadClient readClient(chip::app::InteractionModelEngine::GetInstance(), &ctx.GetExchangeManager(), delegate, + chip::app::ReadClient::InteractionType::Read); + + ReadPrepareParams readPrepareParams(ctx.GetSessionBobToAlice()); + err = readClient.SendRequest(readPrepareParams); + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + + // We don't actually want to deliver that message, because we want to + // synthesize the read response. But we don't want it hanging around + // forever either. + ctx.GetLoopback().mNumMessagesToDrop = 1; + ctx.DrainAndServiceIO(); + + GenerateReportData(apSuite, apContext, buf, ReportType::kInvalidOutOfRangeAttributeId, true /* aSuppressResponse*/); + + err = readClient.ProcessReportData(std::move(buf), ReadClient::ReportType::kContinuingTransaction); + // Overall processing should succeed. + NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR); + + // We should not have gotten any attribute reports or errors. + NL_TEST_ASSERT(apSuite, !delegate.mGotEventResponse); + NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 0); + NL_TEST_ASSERT(apSuite, !delegate.mGotReport); + NL_TEST_ASSERT(apSuite, !delegate.mReadError); +} + void TestReadInteraction::TestReadHandlerInvalidAttributePath(nlTestSuite * apSuite, void * apContext) { CHIP_ERROR err = CHIP_NO_ERROR; @@ -691,7 +737,7 @@ void TestReadInteraction::TestReadHandlerInvalidAttributePath(nlTestSuite * apSu ReadHandler readHandler(nullCallback, exchangeCtx, chip::app::ReadHandler::InteractionType::Read, app::reporting::GetDefaultReportScheduler()); - GenerateReportData(apSuite, apContext, reportDatabuf, false /*aNeedInvalidReport*/, false /* aSuppressResponse*/); + GenerateReportData(apSuite, apContext, reportDatabuf, ReportType::kValid, false /* aSuppressResponse*/); err = readHandler.SendReportData(std::move(reportDatabuf), false); NL_TEST_ASSERT(apSuite, err == CHIP_ERROR_INCORRECT_STATE); @@ -4941,6 +4987,7 @@ const nlTest sTests[] = NL_TEST_DEF("TestReadClientGenerateOneEventPaths", chip::app::TestReadInteraction::TestReadClientGenerateOneEventPaths), NL_TEST_DEF("TestReadClientGenerateTwoEventPaths", chip::app::TestReadInteraction::TestReadClientGenerateTwoEventPaths), NL_TEST_DEF("TestReadClientInvalidReport", chip::app::TestReadInteraction::TestReadClientInvalidReport), + NL_TEST_DEF("TestReadClientInvalidAttributeId", chip::app::TestReadInteraction::TestReadClientInvalidAttributeId), NL_TEST_DEF("TestReadHandlerInvalidAttributePath", chip::app::TestReadInteraction::TestReadHandlerInvalidAttributePath), NL_TEST_DEF("TestProcessSubscribeRequest", chip::app::TestReadInteraction::TestProcessSubscribeRequest), /*