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), /*